diff --git a/setup/js/add_labels.cjs b/setup/js/add_labels.cjs index 49f7cd9..3409570 100644 --- a/setup/js/add_labels.cjs +++ b/setup/js/add_labels.cjs @@ -278,6 +278,11 @@ const main = createCountGatedHandler({ throw new Error(`Failed to resolve GraphQL node ID for ${contextType} #${itemNumber}`); } + // Detect whether the item is a pull request. The REST issues endpoint returns a + // `pull_request` field for PRs, and PR node IDs start with "PR_". The GraphQL + // updateIssue mutation only accepts Issue node IDs; PRs must use updatePullRequest. + const itemIsPR = Boolean(issueData?.pull_request) || issueNodeId.startsWith("PR_"); + const repoLabels = await fetchAllRepoLabels(githubClient, repoParts.owner, repoParts.repo); const labelIdByName = new Map(repoLabels.map(label => [label.name.toLowerCase(), label.id])); @@ -290,9 +295,27 @@ const main = createCountGatedHandler({ const labelIntentUpdates = buildIssueIntentLabelUpdates(mergedSpecs, labelIdByName); core.info(`Adding ${uniqueLabels.length} labels to ${contextType} #${itemNumber} in ${itemRepo} via GraphQL intent mutation`); - const result = await withRetry( - () => - githubClient.graphql( + // Both updateIssue and updatePullRequest use LabelUpdateInput (rationale/confidence/suggest), + // which is gated by the "update_issue_suggestions" GraphQL feature flag. + const intentHeaders = { "GraphQL-Features": "update_issue_suggestions" }; + const [mutationQuery, mutationVars, getResultLabels] = itemIsPR + ? [ + `mutation($prId: ID!, $labels: [LabelUpdateInput!]!) { + updatePullRequest(input: { pullRequestId: $prId, labels: $labels }) { + pullRequest { + id + labels(first: 100) { + nodes { + name + } + } + } + } + }`, + { prId: issueNodeId, labels: labelIntentUpdates, headers: intentHeaders }, + r => r?.updatePullRequest?.pullRequest?.labels?.nodes, + ] + : [ `mutation($issueId: ID!, $labels: [LabelUpdateInput!]!) { updateIssue(input: { id: $issueId, labels: $labels }) { issue { @@ -305,14 +328,13 @@ const main = createCountGatedHandler({ } } }`, - { issueId: issueNodeId, labels: labelIntentUpdates, headers: { "GraphQL-Features": "update_issue_suggestions" } } - ), - RATE_LIMIT_RETRY_CONFIG, - `add_labels to ${contextType} #${itemNumber} in ${itemRepo}` - ); + { issueId: issueNodeId, labels: labelIntentUpdates, headers: intentHeaders }, + r => r?.updateIssue?.issue?.labels?.nodes, + ]; + const result = await withRetry(() => githubClient.graphql(mutationQuery, mutationVars), RATE_LIMIT_RETRY_CONFIG, `add_labels to ${contextType} #${itemNumber} in ${itemRepo}`); core.info(`Successfully added ${uniqueLabels.length} labels to ${contextType} #${itemNumber} in ${itemRepo}`); - const afterLabels = result?.updateIssue?.issue?.labels?.nodes || []; + const afterLabels = getResultLabels(result) || []; return attachExecutionState( { success: true, diff --git a/setup/js/artifact_client.cjs b/setup/js/artifact_client.cjs index 8ac97f1..37576e6 100644 --- a/setup/js/artifact_client.cjs +++ b/setup/js/artifact_client.cjs @@ -368,14 +368,23 @@ class DefaultArtifactClient { const zipLike = isZipResponse(location, contentType); if (zipLike && !options.skipDecompress) { ensureUnzipAvailable(); - const tempZip = path.join(fs.mkdtempSync(path.join(os.tmpdir(), "gh-aw-artifact-download-")), "artifact.zip"); - digest = await streamToFile(blobResponse, tempZip); - const unzipResult = spawnSync("unzip", ["-q", tempZip, "-d", destination], { encoding: "utf8" }); - if (unzipResult.error) { - throw unzipResult.error; - } - if (unzipResult.status !== 0) { - throw new Error(`unzip failed: ${unzipResult.stderr || unzipResult.stdout || "unknown error"}`); + const tempDownloadDir = fs.mkdtempSync(path.join(os.tmpdir(), "gh-aw-artifact-download-")); + const tempZip = path.join(tempDownloadDir, "artifact.zip"); + try { + digest = await streamToFile(blobResponse, tempZip); + const unzipResult = spawnSync("unzip", ["-q", tempZip, "-d", destination], { encoding: "utf8" }); + if (unzipResult.error) { + throw unzipResult.error; + } + if (unzipResult.status !== 0) { + throw new Error(`unzip failed: ${unzipResult.stderr || unzipResult.stdout || "unknown error"}`); + } + } finally { + try { + fs.rmSync(tempDownloadDir, { recursive: true, force: true }); + } catch { + // Ignore cleanup errors — best effort only. + } } } else { const fileName = parseFilenameFromContentDisposition(blobResponse.headers.get("content-disposition") || ""); @@ -398,6 +407,7 @@ class DefaultArtifactClient { let artifactName = String(name || "").trim(); let uploadPath = ""; let contentType = "application/zip"; + let tmpDir = ""; if (options.skipArchive) { if (files.length !== 1) { @@ -406,52 +416,62 @@ class DefaultArtifactClient { uploadPath = files[0]; contentType = "application/octet-stream"; } else { - const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "gh-aw-artifact-upload-")); + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "gh-aw-artifact-upload-")); uploadPath = path.join(tmpDir, `${artifactName || "artifact"}.zip`); createZipFromFiles(files, rootDirectory, uploadPath); } - const { workflowRunBackendId, workflowJobRunBackendId } = getBackendIdsFromRuntimeToken(); - const createRequest = { - workflowRunBackendId, - workflowJobRunBackendId, - name: artifactName, - version: 7, - mimeType: contentType, - }; - const expiresAt = formatRetentionTimestamp(options.retentionDays); - if (expiresAt) { - createRequest.expiresAt = expiresAt; - } + try { + const { workflowRunBackendId, workflowJobRunBackendId } = getBackendIdsFromRuntimeToken(); + const createRequest = { + workflowRunBackendId, + workflowJobRunBackendId, + name: artifactName, + version: 7, + mimeType: contentType, + }; + const expiresAt = formatRetentionTimestamp(options.retentionDays); + if (expiresAt) { + createRequest.expiresAt = expiresAt; + } - /** @type {any} */ - const createResponse = await twirpRequest("CreateArtifact", createRequest); - const signedUploadUrl = createResponse?.signedUploadUrl || createResponse?.signed_upload_url; - if (!createResponse?.ok || !signedUploadUrl) { - throw new Error("CreateArtifact returned an invalid response"); - } + /** @type {any} */ + const createResponse = await twirpRequest("CreateArtifact", createRequest); + const signedUploadUrl = createResponse?.signedUploadUrl || createResponse?.signed_upload_url; + if (!createResponse?.ok || !signedUploadUrl) { + throw new Error("CreateArtifact returned an invalid response"); + } - const uploadSize = await uploadFileToSignedURL(uploadPath, signedUploadUrl, contentType); - const sha256 = await hashFile(uploadPath); + const uploadSize = await uploadFileToSignedURL(uploadPath, signedUploadUrl, contentType); + const sha256 = await hashFile(uploadPath); - const finalizeRequest = { - workflowRunBackendId, - workflowJobRunBackendId, - name: artifactName, - size: String(uploadSize), - hash: `sha256:${sha256}`, - }; - /** @type {any} */ - const finalizeResponse = await twirpRequest("FinalizeArtifact", finalizeRequest); - if (!finalizeResponse?.ok) { - throw new Error("FinalizeArtifact returned an invalid response"); - } + const finalizeRequest = { + workflowRunBackendId, + workflowJobRunBackendId, + name: artifactName, + size: String(uploadSize), + hash: `sha256:${sha256}`, + }; + /** @type {any} */ + const finalizeResponse = await twirpRequest("FinalizeArtifact", finalizeRequest); + if (!finalizeResponse?.ok) { + throw new Error("FinalizeArtifact returned an invalid response"); + } - return { - id: Number(finalizeResponse.artifactId ?? finalizeResponse.artifact_id ?? 0) || undefined, - size: uploadSize, - digest: sha256, - }; + return { + id: Number(finalizeResponse.artifactId ?? finalizeResponse.artifact_id ?? 0) || undefined, + size: uploadSize, + digest: sha256, + }; + } finally { + if (tmpDir) { + try { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } catch { + // Ignore cleanup errors — best effort only. + } + } + } } } diff --git a/setup/js/check_command_position.cjs b/setup/js/check_command_position.cjs index 2910b32..fecca17 100644 --- a/setup/js/check_command_position.cjs +++ b/setup/js/check_command_position.cjs @@ -117,10 +117,10 @@ async function main() { return; } - // Normalize whitespace and resolve the matched slash command at the start of the text. - const trimmedText = text.trim(); - const matchedCommand = resolveMatchedCommand(trimmedText, commands); - const firstWord = trimmedText.split(/\s+/)[0]; + // Resolve the matched slash command at the start of the text. + // Commands must appear at position zero to match the compile-time activation conditions. + const matchedCommand = resolveMatchedCommand(text, commands); + const firstWord = text.trimStart().split(/\s+/)[0]; core.info(`Checking command position. First word in text: ${firstWord}`); core.info(`Looking for commands: ${commands.map(c => `/${c}`).join(", ")}`); diff --git a/setup/js/close_expired_discussions.cjs b/setup/js/close_expired_discussions.cjs index 3d3c552..b0c8156 100644 --- a/setup/js/close_expired_discussions.cjs +++ b/setup/js/close_expired_discussions.cjs @@ -2,59 +2,10 @@ // const { executeExpiredEntityCleanup } = require("./expired_entity_main_flow.cjs"); -const { generateExpiredEntityFooter, getExpiredEntityCautionAlert } = require("./generate_footer.cjs"); -const { formatDateInProjectTimeZone } = require("./project_timezone.cjs"); -const { sanitizeContent } = require("./sanitize_content.cjs"); +const { addDiscussionComment, closeDiscussionAsOutdated, createClosedRecord, createExpiredEntityHandler } = require("./expired_entity_handler_factory.cjs"); const { getWorkflowMetadata } = require("./workflow_metadata_helpers.cjs"); const { resolveExecutionOwnerRepo } = require("./repo_helpers.cjs"); -/** - * Add comment to a GitHub Discussion using GraphQL - * @param {any} github - GitHub GraphQL instance - * @param {string} discussionId - Discussion node ID - * @param {string} message - Comment body - * @returns {Promise<{id: string, url: string}>} Comment details - */ -async function addDiscussionComment(github, discussionId, message) { - const result = await github.graphql( - ` - mutation($dId: ID!, $body: String!) { - addDiscussionComment(input: { discussionId: $dId, body: $body }) { - comment { - id - url - } - } - }`, - { dId: discussionId, body: sanitizeContent(message) } - ); - - return result.addDiscussionComment.comment; -} - -/** - * Close a GitHub Discussion as OUTDATED using GraphQL - * @param {any} github - GitHub GraphQL instance - * @param {string} discussionId - Discussion node ID - * @returns {Promise<{id: string, url: string}>} Discussion details - */ -async function closeDiscussionAsOutdated(github, discussionId) { - const result = await github.graphql( - ` - mutation($dId: ID!) { - closeDiscussion(input: { discussionId: $dId, reason: OUTDATED }) { - discussion { - id - url - } - } - }`, - { dId: discussionId } - ); - - return result.closeDiscussion.discussion; -} - /** * Check if a discussion already has an expiration comment and fetch its closed state * @param {any} github - GitHub GraphQL instance @@ -106,60 +57,46 @@ async function main() { summaryHeading: "Expired Discussions Cleanup", enableDedupe: true, // Discussions may have duplicates across pages includeSkippedHeading: true, - processEntity: async discussion => { - core.info(` Checking for existing expiration comment and closed state on discussion #${discussion.number}`); - const { hasComment, isClosed } = await hasExpirationComment(github, discussion.id); - - if (isClosed) { - core.warning(` Discussion #${discussion.number} is already closed, skipping`); - return { - status: "skipped", - record: { - number: discussion.number, - url: discussion.url, - title: discussion.title, - }, - }; - } - - if (hasComment) { - core.warning(` Discussion #${discussion.number} already has an expiration comment, skipping to avoid duplicate`); - - core.info(` Attempting to close discussion #${discussion.number} without adding another comment`); - await closeDiscussionAsOutdated(github, discussion.id); - core.info(` ✓ Discussion closed successfully`); - - return { - status: "skipped", - record: { - number: discussion.number, - url: discussion.url, - title: discussion.title, - }, - }; - } + processEntity: createExpiredEntityHandler({ + workflowName, + workflowId, + runUrl, + entityNoun: "discussion", + entityLabel: "Discussion", + core, + footerSuffix: "\n\n", + beforeComment: async discussion => { + core.info(` Checking for existing expiration comment and closed state on discussion #${discussion.number}`); + const { hasComment, isClosed } = await hasExpirationComment(github, discussion.id); + + if (isClosed) { + core.warning(` Discussion #${discussion.number} is already closed, skipping`); + return { + status: "skipped", + record: createClosedRecord(discussion), + }; + } - const cautionAlert = getExpiredEntityCautionAlert(workflowName, runUrl); - const expirationText = `This discussion was automatically closed because it expired on ${formatDateInProjectTimeZone(discussion.expirationDate) || discussion.expirationDate.toISOString()}.`; - const closingMessage = (cautionAlert ? cautionAlert + "\n\n" : "") + expirationText + generateExpiredEntityFooter(workflowName, runUrl, workflowId) + "\n\n"; + if (hasComment) { + core.warning(` Discussion #${discussion.number} already has an expiration comment, skipping to avoid duplicate`); - core.info(` Adding closing comment to discussion #${discussion.number}`); - await addDiscussionComment(github, discussion.id, closingMessage); - core.info(` ✓ Comment added successfully`); + core.info(` Attempting to close discussion #${discussion.number} without adding another comment`); + await closeDiscussionAsOutdated(github, discussion.id); + core.info(` ✓ Discussion closed successfully`); - core.info(` Closing discussion #${discussion.number} as outdated`); - await closeDiscussionAsOutdated(github, discussion.id); - core.info(` ✓ Discussion closed successfully`); + return { + status: "closed", + record: createClosedRecord(discussion), + }; + } - return { - status: "closed", - record: { - number: discussion.number, - url: discussion.url, - title: discussion.title, - }, - }; - }, + return undefined; + }, + beforeCommentLog: discussion => ` Adding closing comment to discussion #${discussion.number}`, + beforeCloseLog: discussion => ` Closing discussion #${discussion.number} as outdated`, + addComment: (discussion, message) => addDiscussionComment(github, discussion.id, message), + closeEntity: discussion => closeDiscussionAsOutdated(github, discussion.id), + }), }); } diff --git a/setup/js/close_expired_issues.cjs b/setup/js/close_expired_issues.cjs index e29c8dc..60ce9f8 100644 --- a/setup/js/close_expired_issues.cjs +++ b/setup/js/close_expired_issues.cjs @@ -2,52 +2,10 @@ // const { executeExpiredEntityCleanup } = require("./expired_entity_main_flow.cjs"); -const { generateExpiredEntityFooter, getExpiredEntityCautionAlert } = require("./generate_footer.cjs"); -const { formatDateInProjectTimeZone } = require("./project_timezone.cjs"); -const { sanitizeContent } = require("./sanitize_content.cjs"); +const { addIssueThreadComment, closeIssue, createExpiredEntityHandler } = require("./expired_entity_handler_factory.cjs"); const { getWorkflowMetadata } = require("./workflow_metadata_helpers.cjs"); const { resolveExecutionOwnerRepo } = require("./repo_helpers.cjs"); -/** - * Add comment to a GitHub Issue using REST API - * @param {any} github - GitHub REST instance - * @param {string} owner - Repository owner - * @param {string} repo - Repository name - * @param {number} issueNumber - Issue number - * @param {string} message - Comment body - * @returns {Promise} Comment details - */ -async function addIssueComment(github, owner, repo, issueNumber, message) { - const result = await github.rest.issues.createComment({ - owner: owner, - repo: repo, - issue_number: issueNumber, - body: sanitizeContent(message), - }); - - return result.data; -} - -/** - * Close a GitHub Issue using REST API - * @param {any} github - GitHub REST instance - * @param {string} owner - Repository owner - * @param {string} repo - Repository name - * @param {number} issueNumber - Issue number - * @returns {Promise} Issue details - */ -async function closeIssue(github, owner, repo, issueNumber) { - const result = await github.rest.issues.update({ - owner: owner, - repo: repo, - issue_number: issueNumber, - state: "closed", - state_reason: "not_planned", - }); - - return result.data; -} - async function main() { const { owner, repo } = resolveExecutionOwnerRepo(); core.info(`Operating on repository: ${owner}/${repo}`); @@ -61,26 +19,16 @@ async function main() { resultKey: "issues", entityLabel: "Issue", summaryHeading: "Expired Issues Cleanup", - processEntity: async issue => { - const cautionAlert = getExpiredEntityCautionAlert(workflowName, runUrl); - const expirationText = `This issue was automatically closed because it expired on ${formatDateInProjectTimeZone(issue.expirationDate) || issue.expirationDate.toISOString()}.`; - const closingMessage = (cautionAlert ? cautionAlert + "\n\n" : "") + expirationText + generateExpiredEntityFooter(workflowName, runUrl, workflowId); - - await addIssueComment(github, owner, repo, issue.number, closingMessage); - core.info(` ✓ Comment added successfully`); - - await closeIssue(github, owner, repo, issue.number); - core.info(` ✓ Issue closed successfully`); - - return { - status: "closed", - record: { - number: issue.number, - url: issue.url, - title: issue.title, - }, - }; - }, + processEntity: createExpiredEntityHandler({ + workflowName, + workflowId, + runUrl, + entityNoun: "issue", + entityLabel: "Issue", + core, + addComment: (issue, message) => addIssueThreadComment(github, owner, repo, issue.number, message), + closeEntity: issue => closeIssue(github, owner, repo, issue.number), + }), }); } diff --git a/setup/js/close_expired_pull_requests.cjs b/setup/js/close_expired_pull_requests.cjs index 7894b1f..d1dd3ca 100644 --- a/setup/js/close_expired_pull_requests.cjs +++ b/setup/js/close_expired_pull_requests.cjs @@ -2,51 +2,10 @@ // const { executeExpiredEntityCleanup } = require("./expired_entity_main_flow.cjs"); -const { generateExpiredEntityFooter, getExpiredEntityCautionAlert } = require("./generate_footer.cjs"); -const { formatDateInProjectTimeZone } = require("./project_timezone.cjs"); -const { sanitizeContent } = require("./sanitize_content.cjs"); +const { addIssueThreadComment, closePullRequest, createExpiredEntityHandler } = require("./expired_entity_handler_factory.cjs"); const { getWorkflowMetadata } = require("./workflow_metadata_helpers.cjs"); const { resolveExecutionOwnerRepo } = require("./repo_helpers.cjs"); -/** - * Add comment to a GitHub Pull Request using REST API - * @param {any} github - GitHub REST instance - * @param {string} owner - Repository owner - * @param {string} repo - Repository name - * @param {number} prNumber - Pull request number - * @param {string} message - Comment body - * @returns {Promise} Comment details - */ -async function addPullRequestComment(github, owner, repo, prNumber, message) { - const result = await github.rest.issues.createComment({ - owner: owner, - repo: repo, - issue_number: prNumber, - body: sanitizeContent(message), - }); - - return result.data; -} - -/** - * Close a GitHub Pull Request using REST API - * @param {any} github - GitHub REST instance - * @param {string} owner - Repository owner - * @param {string} repo - Repository name - * @param {number} prNumber - Pull request number - * @returns {Promise} Pull request details - */ -async function closePullRequest(github, owner, repo, prNumber) { - const result = await github.rest.pulls.update({ - owner: owner, - repo: repo, - pull_number: prNumber, - state: "closed", - }); - - return result.data; -} - async function main() { const { owner, repo } = resolveExecutionOwnerRepo(); core.info(`Operating on repository: ${owner}/${repo}`); @@ -60,26 +19,16 @@ async function main() { resultKey: "pullRequests", entityLabel: "Pull Request", summaryHeading: "Expired Pull Requests Cleanup", - processEntity: async pr => { - const cautionAlert = getExpiredEntityCautionAlert(workflowName, runUrl); - const expirationText = `This pull request was automatically closed because it expired on ${formatDateInProjectTimeZone(pr.expirationDate) || pr.expirationDate.toISOString()}.`; - const closingMessage = (cautionAlert ? cautionAlert + "\n\n" : "") + expirationText + generateExpiredEntityFooter(workflowName, runUrl, workflowId); - - await addPullRequestComment(github, owner, repo, pr.number, closingMessage); - core.info(` ✓ Comment added successfully`); - - await closePullRequest(github, owner, repo, pr.number); - core.info(` ✓ Pull request closed successfully`); - - return { - status: "closed", - record: { - number: pr.number, - url: pr.url, - title: pr.title, - }, - }; - }, + processEntity: createExpiredEntityHandler({ + workflowName, + workflowId, + runUrl, + entityNoun: "pull request", + entityLabel: "Pull Request", + core, + addComment: (pr, message) => addIssueThreadComment(github, owner, repo, pr.number, message), + closeEntity: pr => closePullRequest(github, owner, repo, pr.number), + }), }); } diff --git a/setup/js/convert_gateway_config_shared.cjs b/setup/js/convert_gateway_config_shared.cjs index 21b41c8..1bc0603 100644 --- a/setup/js/convert_gateway_config_shared.cjs +++ b/setup/js/convert_gateway_config_shared.cjs @@ -201,9 +201,7 @@ function runGatewayConversion(options) { writeSecureOutput(outputPath, output); core.info(`${options.engine} configuration written to ${outputPath}`); - core.info(""); - core.info("Converted configuration:"); - core.info(output); + core.info(`Converted servers: ${Object.keys(servers).join(", ") || "(none)"}`); return output; } diff --git a/setup/js/exchange_otlp_workload_identity.cjs b/setup/js/exchange_otlp_workload_identity.cjs index 49fbc63..4502bc6 100644 --- a/setup/js/exchange_otlp_workload_identity.cjs +++ b/setup/js/exchange_otlp_workload_identity.cjs @@ -1,4 +1,5 @@ // @ts-check +// @safe-outputs-exempt SEC-004 — "body" references are HTTP transport payloads for OAuth token exchange, not GitHub content /** * Exchanges a GitHub OIDC token for a Google Cloud access token using * Workload Identity Federation, optionally impersonating a service account. diff --git a/setup/js/expired_entity_handler_factory.cjs b/setup/js/expired_entity_handler_factory.cjs new file mode 100644 index 0000000..9ce4a6a --- /dev/null +++ b/setup/js/expired_entity_handler_factory.cjs @@ -0,0 +1,214 @@ +// @ts-check +// + +const { generateExpiredEntityFooter, getExpiredEntityCautionAlert } = require("./generate_footer.cjs"); +const { formatDateInProjectTimeZone } = require("./project_timezone.cjs"); +const { sanitizeContent } = require("./sanitize_content.cjs"); + +/** + * @param {string} label + * @returns {string} + */ +function sentenceCaseLabel(label) { + return label.charAt(0).toUpperCase() + label.slice(1).toLowerCase(); +} + +/** + * @param {{number: number, url: string, title: string}} entity + * @returns {{number: number, url: string, title: string}} + */ +function createClosedRecord(entity) { + return { + number: entity.number, + url: entity.url, + title: entity.title, + }; +} + +/** + * @param {{ + * entity: {expirationDate: Date}, + * entityNoun: string, + * workflowName: string, + * workflowId: string, + * runUrl: string, + * footerSuffix?: string, + * }} options + * @returns {string} + */ +function createExpiredEntityClosingMessage({ entity, entityNoun, workflowName, workflowId, runUrl, footerSuffix = "" }) { + const cautionAlert = getExpiredEntityCautionAlert(workflowName, runUrl); + const expirationText = `This ${entityNoun} was automatically closed because it expired on ${formatDateInProjectTimeZone(entity.expirationDate) || entity.expirationDate.toISOString()}.`; + + return (cautionAlert ? cautionAlert + "\n\n" : "") + expirationText + generateExpiredEntityFooter(workflowName, runUrl, workflowId) + footerSuffix; +} + +/** + * Add comment to a GitHub Issue or Pull Request using REST API + * @param {any} github - GitHub REST instance + * @param {string} owner - Repository owner + * @param {string} repo - Repository name + * @param {number} issueNumber - Issue or Pull Request number + * @param {string} message - Comment body + * @returns {Promise} Comment details + */ +async function addIssueThreadComment(github, owner, repo, issueNumber, message) { + const result = await github.rest.issues.createComment({ + owner: owner, + repo: repo, + issue_number: issueNumber, + body: sanitizeContent(message), + }); + + return result.data; +} + +/** + * Close a GitHub Issue using REST API + * @param {any} github - GitHub REST instance + * @param {string} owner - Repository owner + * @param {string} repo - Repository name + * @param {number} issueNumber - Issue number + * @returns {Promise} Issue details + */ +async function closeIssue(github, owner, repo, issueNumber) { + const result = await github.rest.issues.update({ + owner: owner, + repo: repo, + issue_number: issueNumber, + state: "closed", + state_reason: "not_planned", + }); + + return result.data; +} + +/** + * Close a GitHub Pull Request using REST API + * @param {any} github - GitHub REST instance + * @param {string} owner - Repository owner + * @param {string} repo - Repository name + * @param {number} prNumber - Pull request number + * @returns {Promise} Pull request details + */ +async function closePullRequest(github, owner, repo, prNumber) { + const result = await github.rest.pulls.update({ + owner: owner, + repo: repo, + pull_number: prNumber, + state: "closed", + }); + + return result.data; +} + +/** + * Add comment to a GitHub Discussion using GraphQL + * @param {any} github - GitHub GraphQL instance + * @param {string} discussionId - Discussion node ID + * @param {string} message - Comment body + * @returns {Promise<{id: string, url: string}>} Comment details + */ +async function addDiscussionComment(github, discussionId, message) { + const result = await github.graphql( + ` + mutation($dId: ID!, $body: String!) { + addDiscussionComment(input: { discussionId: $dId, body: $body }) { + comment { + id + url + } + } + }`, + { dId: discussionId, body: sanitizeContent(message) } + ); + + return result.addDiscussionComment.comment; +} + +/** + * Close a GitHub Discussion as OUTDATED using GraphQL + * @param {any} github - GitHub GraphQL instance + * @param {string} discussionId - Discussion node ID + * @returns {Promise<{id: string, url: string}>} Discussion details + */ +async function closeDiscussionAsOutdated(github, discussionId) { + const result = await github.graphql( + ` + mutation($dId: ID!) { + closeDiscussion(input: { discussionId: $dId, reason: OUTDATED }) { + discussion { + id + url + } + } + }`, + { dId: discussionId } + ); + + return result.closeDiscussion.discussion; +} + +/** + * @param {{ + * core: {info: (msg: string) => void, warning: (msg: string) => void}, + * workflowName: string, + * workflowId: string, + * runUrl: string, + * entityNoun: string, + * entityLabel: string, + * footerSuffix?: string, + * beforeComment?: (entity: any) => Promise<{status: "closed" | "skipped", record: any} | undefined>, + * beforeCommentLog?: (entity: any) => string, + * beforeCloseLog?: (entity: any) => string, + * addComment: (entity: any, message: string) => Promise, + * closeEntity: (entity: any) => Promise, + * }} options + * @returns {(entity: any) => Promise<{status: "closed" | "skipped", record: any}>} + */ +function createExpiredEntityHandler(options) { + const core = options.core; + return async entity => { + const earlyResult = options.beforeComment ? await options.beforeComment(entity) : undefined; + if (earlyResult) { + return earlyResult; + } + + const closingMessage = createExpiredEntityClosingMessage({ + entity, + entityNoun: options.entityNoun, + workflowName: options.workflowName, + workflowId: options.workflowId, + runUrl: options.runUrl, + footerSuffix: options.footerSuffix, + }); + + if (options.beforeCommentLog) { + core.info(options.beforeCommentLog(entity)); + } + await options.addComment(entity, closingMessage); + core.info(` ✓ Comment added successfully`); + + if (options.beforeCloseLog) { + core.info(options.beforeCloseLog(entity)); + } + await options.closeEntity(entity); + core.info(` ✓ ${sentenceCaseLabel(options.entityLabel)} closed successfully`); + + return { + status: "closed", + record: createClosedRecord(entity), + }; + }; +} + +module.exports = { + addDiscussionComment, + addIssueThreadComment, + closeDiscussionAsOutdated, + closeIssue, + closePullRequest, + createClosedRecord, + createExpiredEntityClosingMessage, + createExpiredEntityHandler, +}; diff --git a/setup/js/generate_git_bundle.cjs b/setup/js/generate_git_bundle.cjs index 7d0e010..8b99a3b 100644 --- a/setup/js/generate_git_bundle.cjs +++ b/setup/js/generate_git_bundle.cjs @@ -244,17 +244,24 @@ async function generateGitBundle(branchName, baseBranch, options = {}) { } const tempWorktree = fs.mkdtempSync(path.join(os.tmpdir(), "gh-aw-filtered-bundle-")); + // Repository hooks (post-checkout, pre-applypatch, ...) must not run for these + // internal synthesis operations: a repository configured for Git LFS (or any other + // hook requiring tooling absent from the safe-outputs environment) would otherwise + // fail `git worktree add` / `git am` and make a valid branch look unusable. + const tempHooksDir = fs.mkdtempSync(path.join(os.tmpdir(), "gh-aw-filtered-bundle-hooks-")); + const noHooksArgs = ["-c", `core.hooksPath=${tempHooksDir}`]; try { - execGitSync(["worktree", "add", "--detach", tempWorktree, baseCommitSha], { cwd }); - execGitSync(["am", "--3way", patchResult.patchPath], { cwd: tempWorktree }); + execGitSync([...noHooksArgs, "worktree", "add", "--detach", tempWorktree, baseCommitSha], { cwd }); + execGitSync([...noHooksArgs, "am", "--3way", patchResult.patchPath], { cwd: tempWorktree }); execGitSync(["bundle", "create", bundlePath, `${baseCommitSha}..HEAD`], { cwd: tempWorktree }); } finally { try { - execGitSync(["worktree", "remove", "--force", tempWorktree], { cwd }); + execGitSync([...noHooksArgs, "worktree", "remove", "--force", tempWorktree], { cwd }); } catch (removeError) { debugLog(`Failed to remove temporary filtered-bundle worktree ${tempWorktree}: ${getErrorMessage(removeError)}`); } fs.rmSync(tempWorktree, { recursive: true, force: true }); + fs.rmSync(tempHooksDir, { recursive: true, force: true }); } } else { const bundleCreateArgs = ["bundle", "create", bundlePath, `${baseRef}..${branchName}`]; diff --git a/setup/js/github_api_helpers.cjs b/setup/js/github_api_helpers.cjs index 413e634..7e68485 100644 --- a/setup/js/github_api_helpers.cjs +++ b/setup/js/github_api_helpers.cjs @@ -55,8 +55,8 @@ function logGraphQLError(error, operation, hints = {}) { } if (error.status) core.info(`HTTP status: ${error.status}`); - if (error.request) core.info(`Request: ${JSON.stringify(error.request, null, 2)}`); - if (error.data) core.info(`Response data: ${JSON.stringify(error.data, null, 2)}`); + if (error.request) core.info("Request details omitted"); + if (error.data) core.info("Response data omitted"); } /** diff --git a/setup/js/log_parser_bootstrap.cjs b/setup/js/log_parser_bootstrap.cjs index 21f1285..4e1174d 100644 --- a/setup/js/log_parser_bootstrap.cjs +++ b/setup/js/log_parser_bootstrap.cjs @@ -4,6 +4,7 @@ const { generatePlainTextSummary, generateCopilotCliStyleSummary, wrapAgentLogInSection, formatSafeOutputsPreview } = require("./log_parser_shared.cjs"); const { getErrorMessage } = require("./error_helpers.cjs"); const { ERR_API, ERR_CONFIG, ERR_VALIDATION } = require("./error_codes.cjs"); +const { redactStepSummaryContent } = require("./redact_secrets.cjs"); const INFERENCE_ACCESS_ERROR_PATTERN = /Access denied by policy settings|invalid access to inference/i; const CLAUDE_RATE_LIMIT_PATTERN = /rate_limit_error|429 Too Many Requests|"api_error_status"\s*:\s*429|request rejected \(429\)|rate limit/i; const CLAUDE_OVERLOAD_PATTERN = /overloaded_error|"overloaded"/i; @@ -380,7 +381,7 @@ async function runLogParser(options) { } } - await core.summary.addRaw(fullMarkdown).write(); + await core.summary.addRaw(redactStepSummaryContent(fullMarkdown)).write(); } else { // Fallback path: markdown exists but no structured log entries were parsed. // Suppress the "parsed successfully" message for Claude since it always produces @@ -412,7 +413,7 @@ async function runLogParser(options) { fullMarkdown += "\n" + safeOutputsMarkdown; } } - await core.summary.addRaw(fullMarkdown).write(); + await core.summary.addRaw(redactStepSummaryContent(fullMarkdown)).write(); } } else { core.error(`Failed to parse ${parserName} log`); @@ -431,7 +432,7 @@ async function runLogParser(options) { } else { const diagnostics = buildClaudeStartupDiagnostics(content); if (diagnostics.summaryMarkdown) { - await core.summary.addRaw(diagnostics.summaryMarkdown).write(); + await core.summary.addRaw(redactStepSummaryContent(diagnostics.summaryMarkdown)).write(); } if (diagnostics.inferenceAccessError) { diff --git a/setup/js/mcp_cli_bridge.cjs b/setup/js/mcp_cli_bridge.cjs index 223ead8..ffb8c74 100644 --- a/setup/js/mcp_cli_bridge.cjs +++ b/setup/js/mcp_cli_bridge.cjs @@ -12,9 +12,9 @@ const { getErrorMessage } = require("./error_helpers.cjs"); * * Protocol flow: initialize → notifications/initialized → (periodic ping) → tools/call * - * All interactions are logged via core.* (shim.cjs) to console and - * appended as JSONL entries to /tmp/gh-aw/mcp-cli-audit/.jsonl - * for auditing. + * Operation metadata is logged via core.* (shim.cjs) and appended as JSONL + * entries to /tmp/gh-aw/mcp-cli-audit/.jsonl for auditing. Audit logs + * omit payloads and are removed after 24 hours. * * Usage (called by generated CLI wrappers): * node mcp_cli_bridge.cjs \ @@ -40,6 +40,8 @@ const { renderToolRecommendedExample, renderToolSignature, summarizeHelpText } = /** Directory for JSONL audit logs (writable inside AWF sandbox via /tmp mount) */ const AUDIT_LOG_DIR = "/tmp/gh-aw/mcp-cli-audit"; +const AUDIT_LOG_RETENTION_MS = 24 * 60 * 60 * 1000; +const SAFE_AUDIT_FIELDS = new Set(["event", "tool", "statusCode", "hasSession", "elapsedMs", "totalElapsedMs", "pingId", "intervalMs", "pid", "toolCount", "argumentBytes", "responseBytes"]); /** Default timeout (ms) for HTTP calls to the local MCP gateway */ const DEFAULT_HTTP_TIMEOUT_MS = 15000; @@ -78,14 +80,53 @@ const SAFEOUTPUTS_SERVER_NAME = "safeoutputs"; // --------------------------------------------------------------------------- /** - * Ensure the JSONL audit log directory exists. + * Ensure the JSONL audit log directory exists and remove expired records. + * @param {string} [auditDir] */ -function ensureAuditDir() { +function ensureAuditDir(auditDir = AUDIT_LOG_DIR) { try { - fs.mkdirSync(AUDIT_LOG_DIR, { recursive: true }); + fs.mkdirSync(auditDir, { recursive: true, mode: 0o700 }); + fs.chmodSync(auditDir, 0o700); + const cutoff = Date.now() - AUDIT_LOG_RETENTION_MS; + for (const filename of fs.readdirSync(auditDir)) { + const logPath = path.join(auditDir, filename); + const stat = fs.lstatSync(logPath); + if (stat.isFile() && filename.endsWith(".jsonl") && stat.mtimeMs < cutoff) { + fs.rmSync(logPath); + } + } } catch (err) { const core = global.core; - core.warning(`Failed to create audit log directory ${AUDIT_LOG_DIR}: ${getErrorMessage(err)}`); + core.warning(`Failed to prepare audit log directory ${auditDir}: ${getErrorMessage(err)}`); + } +} + +/** + * Retain only non-payload fields that are safe and useful for diagnostics. + * @param {Record} entry + * @returns {Record} + */ +function sanitizeAuditEntry(entry) { + /** @type {Record} */ + const safeEntry = {}; + for (const [key, value] of Object.entries(entry)) { + if (SAFE_AUDIT_FIELDS.has(key) && (typeof value === "string" || typeof value === "number" || typeof value === "boolean")) { + safeEntry[key] = value; + } + } + return safeEntry; +} + +/** + * Return the serialized UTF-8 size of a value without exposing its content. + * @param {unknown} value + * @returns {number} + */ +function serializedSize(value) { + try { + return Buffer.byteLength(JSON.stringify(value) ?? ""); + } catch { + return 0; } } @@ -94,19 +135,26 @@ function ensureAuditDir() { * * @param {string} serverName - Server name (used as filename prefix) * @param {Record} entry - Log entry object + * @param {string} [auditDir] */ -function auditLog(serverName, entry) { +function auditLog(serverName, entry, auditDir = AUDIT_LOG_DIR) { + let fd; try { - const logPath = path.join(AUDIT_LOG_DIR, `${serverName}.jsonl`); + const safeServerName = serverName.replace(/[^a-zA-Z0-9._-]/g, "_"); + const logPath = path.join(auditDir, `${safeServerName}.jsonl`); const record = { timestamp: new Date().toISOString(), server: serverName, - ...entry, + ...sanitizeAuditEntry(entry), }; - fs.appendFileSync(logPath, JSON.stringify(record) + "\n", { mode: 0o644 }); + fd = fs.openSync(logPath, fs.constants.O_APPEND | fs.constants.O_CREAT | fs.constants.O_WRONLY | fs.constants.O_NOFOLLOW, 0o600); + fs.fchmodSync(fd, 0o600); + fs.writeSync(fd, JSON.stringify(record) + "\n"); } catch (err) { const core = global.core; core.warning(`Failed to write audit log for ${serverName}: ${getErrorMessage(err)}`); + } finally { + if (fd !== undefined) fs.closeSync(fd); } } @@ -328,12 +376,13 @@ async function mcpNotifyInitialized(serverUrl, apiKey, sessionId, serverName) { async function mcpToolsCall(serverUrl, apiKey, sessionId, toolName, toolArgs, serverName) { const core = global.core; const startMs = Date.now(); - core.info(`[${serverName}] MCP tools/call: tool=${toolName}, args=${JSON.stringify(toolArgs)}`); + const argumentBytes = serializedSize(toolArgs); + core.info(`[${serverName}] MCP tools/call: tool=${toolName}, argumentBytes=${argumentBytes}`); auditLog(serverName, { event: "tools_call_start", tool: toolName, - arguments: toolArgs, + argumentBytes, }); /** @type {Record} */ @@ -363,7 +412,7 @@ async function mcpToolsCall(serverUrl, apiKey, sessionId, toolName, toolArgs, se tool: toolName, statusCode: resp.statusCode, elapsedMs, - response: resp.body, + responseBytes: serializedSize(resp.body), }); return resp; @@ -1402,12 +1451,9 @@ async function main() { ensureAuditDir(); - core.info(`[${serverName}] Bridge invoked: url=${serverUrl}, toolsFile=${toolsFile}, userArgs=${JSON.stringify(userArgs)}`); + core.info(`[${serverName}] Bridge invoked: argumentCount=${Math.max(0, userArgs.length - 1)}`); auditLog(serverName, { event: "bridge_invoked", - url: serverUrl, - toolsFile, - userArgs, pid: process.pid, }); @@ -1448,8 +1494,9 @@ async function main() { return; } - core.info(`[${serverName}] Calling tool '${toolName}' with args: ${JSON.stringify(toolArgs)}${jsonOutput ? " (--json)" : ""}`); - auditLog(serverName, { event: "call_start", tool: toolName, arguments: toolArgs }); + const argumentBytes = serializedSize(toolArgs); + core.info(`[${serverName}] Calling tool '${toolName}' (${argumentBytes} argument bytes${jsonOutput ? ", JSON output" : ""})`); + auditLog(serverName, { event: "call_start", tool: toolName, argumentBytes }); const callStartMs = Date.now(); /** @type {(() => void) | null} */ @@ -1527,5 +1574,9 @@ module.exports = { readStdinSync, ensureSafeOutputsTools, getToolCallTimeoutMs, + auditLog, + ensureAuditDir, + sanitizeAuditEntry, + serializedSize, main, }; diff --git a/setup/js/parse_copilot_log.cjs b/setup/js/parse_copilot_log.cjs index 7289e2e..9b428d9 100644 --- a/setup/js/parse_copilot_log.cjs +++ b/setup/js/parse_copilot_log.cjs @@ -143,7 +143,7 @@ function parseCopilotLog(logContent) { // Generate conversation markdown using shared function const conversationResult = generateConversationMarkdown(canonicalLogEntries, { - formatToolCallback: (toolUse, toolResult) => formatToolUse(toolUse, toolResult, { includeDetailedParameters: true }), + formatToolCallback: (toolUse, toolResult) => formatToolUse(toolUse, toolResult, { includeDetailedParameters: false }), formatInitCallback: initEntry => formatInitializationSummary(initEntry, { includeSlashCommands: false, diff --git a/setup/js/parse_threat_detection_results.cjs b/setup/js/parse_threat_detection_results.cjs index 95f73ce..fc3ba92 100644 --- a/setup/js/parse_threat_detection_results.cjs +++ b/setup/js/parse_threat_detection_results.cjs @@ -497,7 +497,8 @@ async function main() { /** * Helper to set detection failure/warning outputs based on continue-on-error mode. - * In warn mode: sets conclusion=warning, success=false, does NOT fail the job. + * In warn mode, engine failures with tooling reasons fail closed; all other + * failures set conclusion=warning, success=false, and do not fail the job. * In error mode: sets conclusion=failure, success=false, fails the job. * @param {string} reason - Categorized reason (e.g. "threat_detected", "agent_failure", "parse_error") * @param {string} message - Human-readable error message @@ -505,7 +506,8 @@ async function main() { function setDetectionFailure(reason, message) { core.setOutput("reason", reason); core.exportVariable("GH_AW_DETECTION_REASON", reason); - if (isWarnMode) { + const mustFail = detectionExecutionOutcome === "failure" && (reason === "agent_failure" || reason === "parse_error"); + if (isWarnMode && !mustFail) { core.warning(`⚠️ ${message}`); core.setOutput("conclusion", "warning"); core.exportVariable("GH_AW_DETECTION_CONCLUSION", "warning"); diff --git a/setup/js/pr_review_buffer.cjs b/setup/js/pr_review_buffer.cjs index 56bd1a1..4672725 100644 --- a/setup/js/pr_review_buffer.cjs +++ b/setup/js/pr_review_buffer.cjs @@ -352,17 +352,19 @@ function createReviewBuffer() { // Add footer to review body if we should and we have footer context if (shouldAddFooter && footerContext) { - body += generateFooterWithMessages( - footerContext.workflowName, - footerContext.runUrl, - footerContext.workflowSource, - footerContext.workflowSourceURL, - footerContext.triggeringIssueNumber, - footerContext.triggeringPRNumber, - footerContext.triggeringDiscussionNumber, - undefined, - { skipDetectionCaution: true } - ); + body += + "\n\n" + + generateFooterWithMessages( + footerContext.workflowName, + footerContext.runUrl, + footerContext.workflowSource, + footerContext.workflowSourceURL, + footerContext.triggeringIssueNumber, + footerContext.triggeringPRNumber, + footerContext.triggeringDiscussionNumber, + undefined, + { skipDetectionCaution: true } + ); const callerWorkflowId = process.env.GH_AW_CALLER_WORKFLOW_ID || ""; if (callerWorkflowId) { diff --git a/setup/js/push_experiment_state.cjs b/setup/js/push_experiment_state.cjs index 654bd8e..7d10f83 100644 --- a/setup/js/push_experiment_state.cjs +++ b/setup/js/push_experiment_state.cjs @@ -202,6 +202,35 @@ function mergeExperimentStateJSONL(remoteContent, localContent) { return merged.length > 0 ? `${merged.map(entry => JSON.stringify(entry)).join("\n")}\n` : ""; } +function mergeAppendOnlyJSONL(remoteContent, localContent) { + const merged = []; + const seen = new Set(); + for (const content of [remoteContent, localContent]) { + for (const line of content.split(/\r?\n/)) { + const trimmed = line.trim(); + if (!trimmed) { + continue; + } + let outputLine; + let key; + try { + const entry = JSON.parse(trimmed); + outputLine = JSON.stringify(entry); + key = `json:${stableJSONStringify(entry)}`; + } catch { + core.warning(`mergeAppendOnlyJSONL: preserving unparseable line during merge`); + outputLine = trimmed; + key = `raw:${trimmed}`; + } + if (!seen.has(key)) { + seen.add(key); + merged.push(outputLine); + } + } + } + return merged.length > 0 ? `${merged.join("\n")}\n` : ""; +} + function readGitStageFile(workspaceDir, stage, filePath) { return execGitSync(["show", `:${stage}:${filePath}`], { cwd: workspaceDir, @@ -221,11 +250,18 @@ function resolveExperimentStateRebaseConflict({ cwd }) { .map(file => file.trim()) .filter(Boolean); - if (conflictedFiles.length === 0 || (!conflictedFiles.includes("state.json") && !conflictedFiles.includes("state.jsonl"))) { + const appendFiles = new Set( + (process.env.GH_AW_STATE_FILES || "") + .split(",") + .map(name => name.trim()) + .filter(name => Boolean(name) && name.endsWith(".jsonl") && name !== "state.jsonl") + ); + const hasMergeableConflict = conflictedFiles.some(file => file === "state.json" || file === "state.jsonl" || appendFiles.has(file)); + if (conflictedFiles.length === 0 || !hasMergeableConflict) { return false; } - const allowedConflicts = new Set(["state.json", "state.jsonl", "assignments.json"]); + const allowedConflicts = new Set(["state.json", "state.jsonl", "assignments.json", ...appendFiles]); for (const file of conflictedFiles) { if (!allowedConflicts.has(file)) { return false; @@ -255,6 +291,16 @@ function resolveExperimentStateRebaseConflict({ cwd }) { } } + for (const file of conflictedFiles.filter(name => appendFiles.has(name))) { + try { + const remoteState = readGitStageFile(cwd, 2, file); + const localState = readGitStageFile(cwd, 3, file); + fs.writeFileSync(path.join(cwd, file), mergeAppendOnlyJSONL(remoteState, localState), "utf8"); + } catch (err) { + throw new Error(`Failed to resolve ${file} rebase conflict: ${getErrorMessage(err)}`, { cause: err }); + } + } + if (conflictedFiles.includes("assignments.json")) { try { const localAssignments = readGitStageFile(cwd, 3, "assignments.json"); @@ -341,6 +387,7 @@ async function main() { .split(",") .map(name => name.trim()) .filter(Boolean); + const appendFiles = new Set(candidateFiles.filter(name => name.endsWith(".jsonl") && name !== "state.jsonl")); const ghToken = process.env.GH_TOKEN || process.env.GITHUB_TOKEN || ""; const githubRunId = process.env.GITHUB_RUN_ID || "unknown"; const githubServerUrl = (process.env.GITHUB_SERVER_URL || "https://github.com").replace(/\/$/, ""); @@ -424,7 +471,13 @@ async function main() { const src = path.join(stateDir, name); const dest = path.join(workspaceDir, name); try { - fs.copyFileSync(src, dest); + if (appendFiles.has(name) && fs.existsSync(dest)) { + const existingContent = fs.readFileSync(dest, "utf8"); + const newContent = fs.readFileSync(src, "utf8"); + fs.writeFileSync(dest, mergeAppendOnlyJSONL(existingContent, newContent), "utf8"); + } else { + fs.copyFileSync(src, dest); + } core.info(`Copied ${name}`); } catch (err) { core.setFailed(`Failed to copy ${name}: ${getErrorMessage(err)}`); @@ -517,6 +570,7 @@ module.exports = { checkoutOrCreateBranch, mergeExperimentStateJSON, mergeExperimentStateJSONL, + mergeAppendOnlyJSONL, mergeExperimentRuns, resolveExperimentStateRebaseConflict, }; diff --git a/setup/js/redact_secrets.cjs b/setup/js/redact_secrets.cjs index 98080c2..6d0d15a 100644 --- a/setup/js/redact_secrets.cjs +++ b/setup/js/redact_secrets.cjs @@ -82,7 +82,27 @@ const BUILT_IN_PATTERNS = [ * These are the canonical paths produced by the gateway setup scripts. * The list is defined as a module-level constant so tests can replace entries. */ -const MCP_GATEWAY_CONFIG_PATHS = [path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw/mcp-config/gateway-output.json"), path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw/mcp-config/mcp-servers.json")]; +// Shell setup scripts write under /tmp, while CJS converters use RUNNER_TEMP. +const MCP_GATEWAY_CONFIG_PATHS = [ + ...new Set( + [ + path.join("/tmp", "gh-aw/mcp-config/gateway-output.json"), + path.join("/tmp", "gh-aw/mcp-config/mcp-servers.json"), + path.join("/tmp", "gh-aw/mcp-config/config.toml"), + path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw/mcp-config/gateway-output.json"), + path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw/mcp-config/mcp-servers.json"), + path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw/mcp-config/config.toml"), + process.env.HOME ? path.join(process.env.HOME, ".copilot/mcp-config.json") : "", + process.env.GITHUB_WORKSPACE ? path.join(process.env.GITHUB_WORKSPACE, ".gemini/settings.json") : "", + ].filter(Boolean) + ), +]; + +/** + * Minimum credential length required before an Authorization value is treated + * as a gateway token. Guards against redacting short placeholder values. + */ +const MIN_GATEWAY_TOKEN_LENGTH = 6; /** * Extracts MCP gateway bearer tokens from known configuration files. @@ -96,26 +116,42 @@ const MCP_GATEWAY_CONFIG_PATHS = [path.join(process.env.RUNNER_TEMP || "/tmp", " function extractMCPGatewayTokens(configPaths) { /** @type {Set} */ const tokens = new Set(); + + /** + * Records an Authorization header value plus, for a bearer header, the bare + * credential so the token is redacted even when logged without the prefix. + * @param {unknown} value - Raw Authorization header value + */ + const addAuthorizationValue = value => { + if (typeof value !== "string") return; + const trimmed = value.trim(); + const bearerMatch = /^[Bb]earer\s+(.+)$/.exec(trimmed); + // The minimum-length guard applies to the credential itself, never to the + // bearer prefix, so short values cannot slip through by being prefixed. + const credential = bearerMatch ? bearerMatch[1].trim() : trimmed; + if (credential.length < MIN_GATEWAY_TOKEN_LENGTH) return; + tokens.add(trimmed); + tokens.add(credential); + }; + for (const configPath of configPaths) { try { if (!fs.existsSync(configPath)) continue; const raw = fs.readFileSync(configPath, "utf8"); - const config = /** @type {Record} */ JSON.parse(raw); + let config; + try { + config = /** @type {Record} */ JSON.parse(raw); + } catch { + // Codex writes TOML (`http_headers = { Authorization = "..." }`); the key + // is matched case-insensitively to tolerate formatting differences. + for (const match of raw.matchAll(/\bAuthorization\s*=\s*"([^"]+)"/gi)) { + addAuthorizationValue(match[1]); + } + continue; + } const servers = /** @type {Record} */ config.mcpServers || {}; for (const server of Object.values(servers)) { - const auth = /** @type {string|undefined} */ server?.headers?.Authorization; - if (typeof auth === "string" && auth.trim().length >= 6) { - const trimmed = auth.trim(); - tokens.add(trimmed); - // Also add just the credential portion when the value is a "Bearer " header - // so the bare token is redacted even when it appears without the "Bearer " prefix. - if (/^[Bb]earer /.test(trimmed)) { - const tokenPart = trimmed.slice(7).trim(); - if (tokenPart.length >= 6) { - tokens.add(tokenPart); - } - } - } + addAuthorizationValue(server?.headers?.Authorization); } } catch { // Silently skip unreadable or malformed files — absence of the gateway @@ -184,6 +220,31 @@ function redactSecrets(content, secretValues) { return { content: redacted, redactionCount }; } +/** + * Redacts credential-shaped strings from content destined for the GitHub Actions + * step summary. + * + * Step summaries reproduce agent-controlled data (tool inputs, tool outputs, agent + * text, safe-output titles), and `::add-mask::` processing does not scrub + * `$GITHUB_STEP_SUMMARY`, so the built-in credential patterns used for artifact + * redaction are applied before the summary is written. Redaction failures are + * non-fatal because the summary is best-effort output. + * + * @param {string} content - Markdown destined for the step summary + * @returns {string} Content with credential-shaped strings replaced + */ +function redactStepSummaryContent(content) { + if (typeof content !== "string" || content.length === 0) { + return content; + } + try { + return redactBuiltInPatterns(content).content; + } catch (error) { + core.warning(`Failed to redact step summary content: ${getErrorMessage(error)}`); + return content; + } +} + /** * Process a single file for secret redaction * @param {string} filePath - Path to the file @@ -257,7 +318,7 @@ async function main() { core.info("Scanning for built-in credential patterns and custom secrets"); // Find all target files in /tmp/gh-aw and ${RUNNER_TEMP}/gh-aw directories - const targetExtensions = [".txt", ".json", ".log", ".md", ".mdx", ".yml", ".jsonl"]; + const targetExtensions = [".txt", ".json", ".log", ".md", ".mdx", ".yml", ".jsonl", ".patch"]; const tmpFiles = findFiles("/tmp/gh-aw", targetExtensions); const optFiles = findFiles(`${process.env.RUNNER_TEMP}/gh-aw`, targetExtensions); const files = [...tmpFiles, ...optFiles]; @@ -282,4 +343,4 @@ async function main() { } } -module.exports = { main, redactSecrets, redactBuiltInPatterns, extractMCPGatewayTokens, BUILT_IN_PATTERNS, MCP_GATEWAY_CONFIG_PATHS }; +module.exports = { main, redactSecrets, redactBuiltInPatterns, redactStepSummaryContent, extractMCPGatewayTokens, BUILT_IN_PATTERNS, MCP_GATEWAY_CONFIG_PATHS }; diff --git a/setup/js/report_failed_jobs.cjs b/setup/js/report_failed_jobs.cjs index a049e3b..91067cd 100644 --- a/setup/js/report_failed_jobs.cjs +++ b/setup/js/report_failed_jobs.cjs @@ -7,6 +7,7 @@ const { renderTemplateFromFile, getPromptPath } = require("./messages_core.cjs") const { generateFooterWithExpiration, createExpirationLine } = require("./ephemerals.cjs"); const { generateXMLMarker } = require("./messages.cjs"); const { parseBoolTemplatable } = require("./templatable.cjs"); +const { sanitizeContent } = require("./sanitize_content.cjs"); const GITHUB_API_VERSION = "2022-11-28"; const FAILED_JOBS_ISSUE_EXPIRES_HOURS = 24 * 7; // 1 week @@ -48,10 +49,12 @@ function isActionsReadPermissionError(error) { function formatFailedJobsList(jobs) { return jobs .map(job => { - if (job.html_url) { - return `- [\`${job.name}\`](${job.html_url})`; + const safeName = sanitizeContent(job.name); + if (job.html_url && job.html_url.startsWith("https://")) { + const safeUrl = sanitizeContent(job.html_url); + return `- [\`${safeName}\`](${safeUrl})`; } - return `- \`${job.name}\``; + return `- \`${safeName}\``; }) .join("\n"); } diff --git a/setup/js/safe_output_summary.cjs b/setup/js/safe_output_summary.cjs index 86c9b6a..74ae58d 100644 --- a/setup/js/safe_output_summary.cjs +++ b/setup/js/safe_output_summary.cjs @@ -11,6 +11,34 @@ const { displayFileContent } = require("./display_file_helpers.cjs"); const { getErrorMessage } = require("./error_helpers.cjs"); const { computeSafeOutputsStatus } = require("./safe_outputs_status.cjs"); +const ERROR_CODES = require("./error_codes.cjs"); +const { redactStepSummaryContent } = require("./redact_secrets.cjs"); + +/** + * Error codes that may be rendered verbatim in a step summary. + * Handler errors are built from caught exception messages, which can embed request + * URLs, payloads, or credentials, so only the machine-readable code prefix is shown. + * @type {Set} + */ +const SUMMARY_SAFE_ERROR_CODES = new Set(Object.values(ERROR_CODES)); + +/** @type {string} Rendered when an error carries no allowlisted code prefix */ +const UNCLASSIFIED_ERROR_CODE = "UNCLASSIFIED"; + +/** + * Reduces an error message to an allowlisted error code so that raw exception text + * (which may contain secrets) never reaches the step summary. + * @param {any} error - The error message produced by a safe-output handler + * @returns {string} An allowlisted error code + */ +function toSummarySafeErrorCode(error) { + const text = typeof error === "string" ? error : String(error ?? ""); + const match = text.match(/^\s*([A-Z][A-Z0-9_]*)\s*:/); + if (match && SUMMARY_SAFE_ERROR_CODES.has(match[1])) { + return match[1]; + } + return UNCLASSIFIED_ERROR_CODE; +} /** * Generate a step summary for a single safe-output message @@ -92,11 +120,6 @@ function generateSafeOutputSummary(options) { if (message.title) { summary += `**Title:** ${message.title}\n\n`; } - if (message.body && typeof message.body === "string") { - const maxBodyLength = 500; - const bodyPreview = message.body.length > maxBodyLength ? message.body.substring(0, maxBodyLength) + "..." : message.body; - summary += `**Body Preview:**\n\`\`\`\`\`\`\n${bodyPreview}\n\`\`\`\`\`\`\n\n`; - } } } else if (success && result) { // Add result-specific information based on type @@ -118,41 +141,21 @@ function generateSafeOutputSummary(options) { if (message.title) { summary += `**Title:** ${message.title}\n\n`; } - // Prefer result.body (final posted body including footer) over message.body (submitted body) - const bodyToShow = result && typeof result.body === "string" ? result.body : message.body; - if (bodyToShow && typeof bodyToShow === "string") { - // Truncate body if too long - const maxBodyLength = 500; - const bodyPreview = bodyToShow.length > maxBodyLength ? bodyToShow.substring(0, maxBodyLength) + "..." : bodyToShow; - summary += `**Body Preview:**\n\`\`\`\`\`\`\n${bodyPreview}\n\`\`\`\`\`\`\n\n`; - } if (message.labels && Array.isArray(message.labels)) { summary += `**Labels:** ${message.labels.join(", ")}\n\n`; } } } else if (error) { - // Show error information - summary += `**Error:** ${error}\n\n`; - - // Add original message details for debugging - if (message) { - summary += `**Message Details:**\n\`\`\`\`\`\`json\n${JSON.stringify(message, null, 2).substring(0, 1000)}\n\`\`\`\`\`\`\n\n`; - } + // Show only an allowlisted error code; raw exception text and message content are + // omitted because they can embed URLs, payloads, or credentials. + summary += `**Error:** \`${toSummarySafeErrorCode(error)}\` (see the job logs for details)\n\n`; } // Display secrecy and integrity security metadata fields if present in the message. // secrecy indicates the confidentiality level of the message content. // integrity indicates the trustworthiness level of the message source. + // message.data is intentionally omitted to prevent secret leakage into step summaries. if (message) { - if (message.data !== undefined) { - let renderedData = ""; - try { - renderedData = JSON.stringify(message.data, null, 2); - } catch { - renderedData = String(message.data); - } - summary += `**Data:**\n\`\`\`\`\`\`json\n${renderedData}\n\`\`\`\`\`\`\n\n`; - } if (message.secrecy !== undefined && message.secrecy !== null) { summary += `**Secrecy:** \`${message.secrecy}\`\n\n`; } @@ -228,7 +231,7 @@ async function writeSafeOutputSummaries(results, messages) { } try { - await core.summary.addRaw(summaryContent).write(); + await core.summary.addRaw(redactStepSummaryContent(summaryContent)).write(); core.info(`📝 Safe output summaries written to step summary`); } catch (error) { core.warning(`Failed to write safe output summaries: ${getErrorMessage(error)}`); diff --git a/setup/js/safe_outputs_config_redact.cjs b/setup/js/safe_outputs_config_redact.cjs index 591fbbc..e132f65 100644 --- a/setup/js/safe_outputs_config_redact.cjs +++ b/setup/js/safe_outputs_config_redact.cjs @@ -1,14 +1,13 @@ // @ts-check /** - * Returns true when a config key should be treated as sensitive (e.g. tokens). - * The check is intentionally scoped to keys containing "token" because those are - * the only secret-bearing fields produced by the safe-outputs compiler today. + * Returns true when a config key should be treated as sensitive. * @param {string} key * @returns {boolean} */ function isSensitiveConfigKey(key) { - return /token/i.test(key); + const normalizedKey = key.replace(/[^a-z0-9]/gi, "").toLowerCase(); + return /(token|apikey|authorization|password|passwd|privatekey|cookie|secret|credential|headers)/.test(normalizedKey); } /** diff --git a/setup/js/safe_outputs_handlers.cjs b/setup/js/safe_outputs_handlers.cjs index cc9bd28..37b7b5d 100644 --- a/setup/js/safe_outputs_handlers.cjs +++ b/setup/js/safe_outputs_handlers.cjs @@ -2,6 +2,7 @@ /// const fs = require("fs"); +const os = require("os"); const path = require("path"); const crypto = require("crypto"); @@ -2092,6 +2093,61 @@ function createHandlers(server, appendSafeOutput, config = {}) { return defaultHandler("dismiss_pull_request_review")(args); }; + /** + * Resolve an allowed-root path to its canonical form, falling back to path.resolve when the + * directory does not yet exist (e.g. GITHUB_WORKSPACE before checkout). + * @param {string} root + * @returns {string} + */ + function canonicalizeAllowedRoot(root) { + try { + return fs.realpathSync(root); + } catch { + return path.resolve(root); + } + } + + /** + * Validate that a canonical absolute path does not refer to sensitive system or credential + * locations. Returns an error message string, or null if the path is safe. + * + * Rejected patterns: + * - Any path with a ".git" directory component (prevents .git/config leakage). + * - System directories: /etc, /proc, /sys, /dev, /run, /boot, /lib*, /usr/lib*. + * - HOME credential/config subtrees: .ssh, .aws, .netrc, .npmrc, .gitconfig, .gnupg, + * .config, .docker, .kube, .azure, .gcp. + * + * @param {string} canonicalPath - Resolved absolute path (output of fs.realpathSync or path.resolve) + * @returns {string|null} + */ + function validateUploadSourcePath(canonicalPath) { + const parts = canonicalPath.split(path.sep); + if (parts.some(p => p === ".git")) { + return `path contains sensitive repository metadata (.git): ${canonicalPath}`; + } + + const systemDenied = ["/etc", "/proc", "/sys", "/dev", "/run", "/boot", "/lib", "/lib64", "/usr/lib", "/usr/local/lib"]; + for (const denied of systemDenied) { + const normalDenied = path.resolve(denied); + if (canonicalPath === normalDenied || canonicalPath.startsWith(normalDenied + path.sep)) { + return `path refers to a system directory: ${canonicalPath}`; + } + } + + const homeDir = os.homedir(); + if (homeDir) { + const sensitiveNames = [".ssh", ".aws", ".gnupg", ".docker", ".kube", ".azure", ".gcp", ".config", ".netrc", ".npmrc", ".gitconfig", ".gitcredentials", ".git-credentials"]; + for (const name of sensitiveNames) { + const sensitive = path.join(path.resolve(homeDir), name); + if (canonicalPath === sensitive || canonicalPath.startsWith(sensitive + path.sep)) { + return `path refers to a sensitive HOME location: ${canonicalPath}`; + } + } + } + + return null; + } + /** * Recursively copy all regular files from srcDir into destDir, preserving the relative * path structure under srcDir. Non-regular entries (sockets, devices, pipes, symlinks) @@ -2117,10 +2173,39 @@ function createHandlers(server, appendSafeOutput, config = {}) { const srcPath = path.join(srcDir, ent.name); const destPath = path.join(destDir, ent.name); if (ent.isDirectory()) { + // Reject sensitive directory names at every level (e.g. foo/.git/config). + let canonicalSrcPath; + try { + canonicalSrcPath = fs.realpathSync(srcPath); + } catch (err) { + throw new Error(`Failed to resolve canonical path for ${srcPath}: ${getErrorMessage(err)}`, { cause: err }); + } + const sensitiveErr = validateUploadSourcePath(canonicalSrcPath); + if (sensitiveErr) { + throw { + code: -32602, + message: `${ERR_VALIDATION}: upload_artifact: ${sensitiveErr}`, + }; + } copyDirectoryRecursive(srcPath, destPath); } else if (ent.isFile() && !ent.isSymbolicLink() && !fs.existsSync(destPath)) { + // Revalidate each file's canonical path before copying. + let canonicalSrcPath; + try { + canonicalSrcPath = fs.realpathSync(srcPath); + } catch (err) { + throw new Error(`Failed to resolve canonical path for ${srcPath}: ${getErrorMessage(err)}`, { cause: err }); + } + const sensitiveErr = validateUploadSourcePath(canonicalSrcPath); + if (sensitiveErr) { + throw { + code: -32602, + message: `${ERR_VALIDATION}: upload_artifact: ${sensitiveErr}`, + }; + } try { fs.copyFileSync(srcPath, destPath); + fs.chmodSync(destPath, 0o600); } catch (err) { throw new Error(`Failed to copy file ${srcPath} to ${destPath}: ${getErrorMessage(err)}`, { cause: err }); } @@ -2172,7 +2257,41 @@ function createHandlers(server, appendSafeOutput, config = {}) { }; } + // Canonicalize to detect traversal escapes and symlink chains. + let canonicalFilePath; + try { + canonicalFilePath = fs.realpathSync(filePath); + } catch (err) { + throw { + code: -32602, + message: `${ERR_VALIDATION}: upload_artifact: failed to resolve canonical path for ${filePath}: ${getErrorMessage(err)}`, + }; + } + + // Reject sensitive paths (system dirs, .git, HOME credentials). + const sensitiveError = validateUploadSourcePath(canonicalFilePath); + if (sensitiveError) { + throw { + code: -32602, + message: `${ERR_VALIDATION}: upload_artifact: ${sensitiveError}`, + }; + } + + // Enforce allowed canonical source roots: staging dir and GITHUB_WORKSPACE. + // RUNNER_TEMP is intentionally excluded — only the specific staging subdirectory is allowed. const stagingDir = path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw", "safeoutputs", "upload-artifacts"); + const allowedRoots = [canonicalizeAllowedRoot(stagingDir)]; + if (process.env.GITHUB_WORKSPACE) { + allowedRoots.push(canonicalizeAllowedRoot(process.env.GITHUB_WORKSPACE)); + } + const withinAllowedRoot = allowedRoots.some(root => canonicalFilePath === root || canonicalFilePath.startsWith(root + path.sep)); + if (!withinAllowedRoot) { + throw { + code: -32602, + message: `${ERR_VALIDATION}: upload_artifact: path is outside allowed source roots (GITHUB_WORKSPACE, staging directory): ${canonicalFilePath}`, + }; + } + if (!fs.existsSync(stagingDir)) { try { fs.mkdirSync(stagingDir, { recursive: true }); @@ -2190,6 +2309,7 @@ function createHandlers(server, appendSafeOutput, config = {}) { if (!fs.existsSync(destPath)) { try { fs.copyFileSync(filePath, destPath); + fs.chmodSync(destPath, 0o600); } catch (err) { throw new Error(`Failed to copy file ${filePath} to ${destPath}: ${getErrorMessage(err)}`, { cause: err }); } diff --git a/setup/js/sanitize_content_core.cjs b/setup/js/sanitize_content_core.cjs index f3effeb..94481a1 100644 --- a/setup/js/sanitize_content_core.cjs +++ b/setup/js/sanitize_content_core.cjs @@ -209,12 +209,149 @@ function sanitizeDomainName(domain) { return joined; } +/** + * Character class (as a source fragment) for the delimiters that may introduce a + * protocol-relative URL. A "//" is only treated as the start of a URL when it + * sits at the start of the string or immediately after one of these, so that + * "//" segments inside the path of an absolute URL (e.g. + * "https://github.com//issues") are not misread as a new URL. + * + * Beyond whitespace/bracket/quote, this includes the delimiters that actually + * precede a URL in rendered contexts: "<" and "=" for HTML attributes and + * CommonMark angle-bracket link destinations (``, + * `[a]()`), and ",", ">", "|" and "`" which separate URLs in prose, + * tables and markup. Omitting these left renderable URLs unfiltered. + * + * Shared by the userinfo-stripping pre-pass and the protocol-relative filtering + * pass so the two cannot disagree about what counts as a URL start: if the + * strip pass recognized a URL the filter pass did not (or vice versa), a + * spoofed host could be normalized into a form that is then trusted. + */ +const URL_START_DELIMITERS = "[\\s([{\"'<=,>|`]"; + +/** + * Character class (as a source fragment) matching one character of a URL + * authority (the "userinfo@host:port" component). + * + * The authority ends at the path/query/fragment ("/", "?", "#") and at + * whitespace, but it must ALSO end at the delimiters that terminate a URL in + * prose and markup. Consuming those was a bypass rather than a cosmetic issue: + * with a class of merely [^\s/?#], the authority of the first URL in + * "https://x.com,https://github.com@evil.com/" swallowed ",https:", so the + * global scan resumed past the second URL's scheme and never stripped its + * userinfo — leaving the spoofed host to be read as the allowlisted github.com. + * The same applied to adjacent markdown images "![a](//x.com)![b](//host@evil)". + */ +const URL_AUTHORITY_CHAR = "[^\\s/?#,()<>[\\]{}\"'`|\\\\]"; + +/** + * Remove the ASCII tab, CR and LF characters that URL parsers discard. + * + * WHATWG URL parsing strips these from anywhere in a URL before parsing, so + * "//github.com\tA@evil.com/x" is fetched as host "evil.com" with the userinfo + * "github.comA" — while a regex that treats them as terminators sees only the + * allowlisted "github.com". Callers must therefore compare hosts on the + * stripped form to match what a browser will actually request. + * + * @param {string} authority - The raw authority text + * @returns {string} The authority with tab/CR/LF removed + */ +function stripUrlIgnorableWhitespace(authority) { + return authority.replace(/[\t\r\n]/g, ""); +} + +/** + * Strip URL userinfo (user:password@) from all scheme://... URLs in a string. + * This must run before any domain filtering so that credentials embedded in + * a URL authority are never passed to the allowlist check or returned to the + * caller. + * + * Examples: + * https://user:REDACTED@example.com/repo.git → https://example.com/repo.git + * git://user@example.com/repo.git → git://example.com/repo.git + * + * @param {string} s - The string to process + * @returns {string} The string with userinfo removed from all URLs + */ +function stripUrlUserinfo(s) { + // Capture the authority-like component right after scheme:// - everything + // up to the start of the path (/), query (?), fragment (#), or whitespace. + // + // The scheme quantifier is bounded ({0,30}) rather than unbounded (*). With + // an unbounded quantifier, a long run of characters that never resolves to + // "://" (e.g. hundreds of thousands of plain letters) forces the scheme + // group to greedily consume the whole remainder and then backtrack one + // character at a time before the match attempt fails at that start + // position - and this repeats at every subsequent start position, giving + // O(n^2) behavior on pathological input even though there is no nested + // quantifier. Real URL schemes are always short (RFC 3986 examples and IANA + // registrations top out well under 30 characters), so bounding the + // quantifier keeps this linear without affecting legitimate matches. + // + // Once captured, look for the LAST "@" within that authority component (in + // plain JS, not regex) and drop everything up to and including it. Using + // the last "@" ensures chained userinfo values (e.g. "a@b@c@host") are + // fully stripped, while stopping the authority match at "?"/"#" ensures an + // ordinary URL whose query string happens to contain "@" is left untouched. + // + // Tab/CR/LF are allowed *inside* the authority and then discarded, because + // URL parsers discard them too: without this, "https://github.com\tA@evil.com/" + // would present an authority of just the allowlisted "github.com" to the + // filter while a browser fetches evil.com. Tolerating them is safe because a + // rewrite only happens when the cleaned authority contains "@" — so an + // ordinary host that merely happens to be followed by a newline and more + // prose is left untouched. + const schemeUserinfoRegex = new RegExp(`([a-z][a-z0-9+.-]{0,30}://)((?:${URL_AUTHORITY_CHAR}|[\\t\\r\\n])*)`, "gi"); + return s.replace(schemeUserinfoRegex, (match, scheme, authority) => { + const cleaned = stripUrlIgnorableWhitespace(authority); + const at = cleaned.lastIndexOf("@"); + if (at === -1) return match; + return scheme + cleaned.slice(at + 1); + }); +} + +/** + * Strip URL userinfo (user:password@) from protocol-relative URLs (//host/path). + * + * Browsers on an HTTPS page resolve "//host/path" to "https://host/path", so a + * protocol-relative URL carries the same userinfo-spoofing risk as an explicit + * https:// URL: in "//github.com@evil.com/x" the real host is evil.com, but a + * host pattern that stops at "@" would read it as the allowlisted github.com. + * stripUrlUserinfo() cannot cover this form because it requires a scheme. + * + * The "//" is only treated as a protocol-relative URL when it appears at the + * start of the string or immediately after a URL-introducing delimiter (see + * URL_START_DELIMITERS) — the same anchoring used by the protocol-relative pass + * in sanitizeUrlDomains() — so "//" segments inside the path of an absolute URL + * (e.g. "https://github.com//issues") are left untouched. + * + * Backslashes are accepted in the separator position because URL parsers treat + * "\" as "/" for special schemes, so "\\github.com@evil.com/x" and + * "/\github.com@evil.com/x" both resolve to host evil.com in a browser. + * + * @param {string} s - The string to process + * @returns {string} The string with userinfo removed from protocol-relative URLs + */ +function stripProtocolRelativeUserinfo(s) { + const protoRelativeUserinfoRegex = new RegExp(`(^|${URL_START_DELIMITERS})([/\\\\]{2})((?:${URL_AUTHORITY_CHAR}|[\\t\\r\\n])*)`, "g"); + return s.replace(protoRelativeUserinfoRegex, (match, prefix, _slashes, authority) => { + const cleaned = stripUrlIgnorableWhitespace(authority); + const at = cleaned.lastIndexOf("@"); + if (at === -1) return match; + return prefix + "//" + cleaned.slice(at + 1); + }); +} + /** * Sanitize URL protocols - replace non-https with /redacted * @param {string} s - The string to process * @returns {string} The string with non-https protocols redacted */ function sanitizeUrlProtocols(s) { + // Strip userinfo (user:password@) from all scheme:// URLs before any other + // processing so that credentials never appear in redaction summaries or logs. + s = stripUrlUserinfo(s); + // Normalize percent-encoded colons before applying the protocol filter. // This prevents bypasses via javascript%3Aalert(1) (single-encoded), // javascript%253Aalert(1) (double-encoded), or deeper nesting. @@ -255,7 +392,6 @@ function sanitizeUrlProtocols(s) { // remains useful without recording an empty-string entry. const truncated = fullMatch.length > 12 ? fullMatch.substring(0, 12) + "..." : fullMatch; core.info(`Redacted URL: ${truncated}`); - core.debug(`Redacted URL (full): ${fullMatch}`); addRedactedDomain(scheme.toLowerCase() + "://"); return "(redacted)"; } @@ -263,7 +399,6 @@ function sanitizeUrlProtocols(s) { const sanitized = sanitizeDomainName(domainLower); const truncated = domainLower.length > 12 ? domainLower.substring(0, 12) + "..." : domainLower; core.info(`Redacted URL: ${truncated}`); - core.debug(`Redacted URL (full): ${fullMatch}`); addRedactedDomain(domainLower); return sanitized ? `(${sanitized}/redacted)` : "(redacted)"; }); @@ -278,7 +413,6 @@ function sanitizeUrlProtocols(s) { const protocol = protocolMatch[1] + ":"; const truncated = match.length > 12 ? match.substring(0, 12) + "..." : match; core.info(`Redacted URL: ${truncated}`); - core.debug(`Redacted URL (full): ${match}`); addRedactedDomain(protocol); } return "(redacted)"; @@ -292,6 +426,14 @@ function sanitizeUrlProtocols(s) { * @returns {string} The string with unknown domains redacted */ function sanitizeUrlDomains(s, allowed) { + // Strip userinfo (user:password@) from HTTPS URLs before any domain filtering + // so that credentials are never passed to the allowlist check or preserved + // in the output for an allowed domain. Protocol-relative URLs (//host/path) + // are stripped too, since browsers resolve them to https:// and they are + // subject to the same allowlist check below. + s = stripUrlUserinfo(s); + s = stripProtocolRelativeUserinfo(s); + // Match HTTPS URLs with optional port and path // This regex is designed to: // 1. Match https:// URIs with explicit protocol @@ -337,7 +479,6 @@ function sanitizeUrlDomains(s, allowed) { const sanitized = sanitizeDomainName(hostname); const truncated = hostname.length > 12 ? hostname.substring(0, 12) + "..." : hostname; core.info(`Redacted URL: ${truncated}`); - core.debug(`Redacted URL (full): ${match}`); addRedactedDomain(hostname); // Return sanitized domain format return sanitized ? `(${sanitized}/redacted)` : "(redacted)"; @@ -376,13 +517,32 @@ function sanitizeUrlDomains(s, allowed) { // The path stop-condition (?!\/\/) stops before the next protocol-relative URL // (analogous to how the httpsUrlRegex stops before the next https:// URL). // Capture groups: + // Second pass: handle protocol-relative URLs (//hostname/path). + // Browsers on HTTPS pages resolve these to https://, so they must be subject + // to the same domain allowlist check as explicit https:// URLs. + // We only treat // as a protocol-relative URL when it appears at the start of + // the string or immediately after a URL-introducing delimiter. The delimiter + // set is shared with the userinfo-stripping pre-pass (URL_START_DELIMITERS) + // so the two passes cannot disagree about where a URL begins. This avoids + // matching // segments inside the path of an allowed https:// URL, such as + // "https://github.com//issues". + // The separator accepts backslashes ("\\host", "/\host") because URL parsers + // treat "\" as "/" for special schemes, so those forms reach the same host. + // The path stop-condition (?!\/\/) stops before the next protocol-relative URL + // (analogous to how the httpsUrlRegex stops before the next https:// URL). + // Capture groups: // 1: prefix (start-of-string or delimiter) - // 2: full protocol-relative URL (starting with //) + // 2: separator (// or a backslash variant) // 3: hostname (and optional port) // 4: optional path - const protoRelativeUrlRegex = /(^|[\s([{"'])(\/\/([\w.-]+(?::\d+)?)(\/(?:(?!\/\/)[^\s,])*)?)/gi; + const protoRelativeUrlRegex = new RegExp(`(^|${URL_START_DELIMITERS})([/\\\\]{2})([\\w.-]+(?::\\d+)?)((?:/(?:(?![/\\\\]{2})[^\\s,])*)?)`, "gi"); - s = s.replace(protoRelativeUrlRegex, (match, prefix, url, hostnameWithPort) => prefix + applyDomainFilter(url, hostnameWithPort)); + s = s.replace(protoRelativeUrlRegex, (match, prefix, _separator, hostnameWithPort, path = "") => { + // Normalize the separator to "//" so a backslash form can never survive as + // an allowed URL in a shape the regex would not re-examine. + const url = `//${hostnameWithPort}${path}`; + return prefix + applyDomainFilter(url, hostnameWithPort); + }); return s; } @@ -1334,6 +1494,11 @@ function sanitizeContentCore(content, maxLength, maxBotMentions) { return ""; } + // Apply truncation early to avoid running expensive operations on oversized inputs. + // This is a pre-pass truncation on raw content; a second truncation pass is applied + // later after normalization (which may reduce length via stripping invisible chars). + content = applyTruncation(content, maxLength); + // Build list of allowed domains from environment and GitHub context const allowedDomains = buildAllowedDomains(); diff --git a/setup/js/setup_threat_detection.cjs b/setup/js/setup_threat_detection.cjs index 3e13b40..a088012 100644 --- a/setup/js/setup_threat_detection.cjs +++ b/setup/js/setup_threat_detection.cjs @@ -179,8 +179,16 @@ async function main() { // Note: creation of /tmp/gh-aw/threat-detection and detection.log is handled by a separate shell step - // Write rendered prompt to step summary using HTML details/summary - await core.summary.addRaw("
\nThreat Detection Prompt\n\n" + "``````markdown\n" + promptContent + "\n" + "``````\n\n
\n").write(); + // Write rendered prompt to step summary using HTML details/summary. + // On the external detector path this prompt is never used (threat-detect renders its own + // template and writes it to the step summary), so the write is suppressed to avoid showing + // two different prompts for a single detection run. + const skipPromptSummary = (process.env.GH_AW_DETECTION_SKIP_PROMPT_SUMMARY || "").toLowerCase() === "true"; + if (skipPromptSummary) { + core.info("Skipping threat detection prompt step summary (external detector renders its own prompt)"); + } else { + await core.summary.addRaw("
\nThreat Detection Prompt\n\n" + "``````markdown\n" + promptContent + "\n" + "``````\n\n
\n").write(); + } core.info("Threat detection setup completed"); } diff --git a/setup/js/slash_command_matcher.cjs b/setup/js/slash_command_matcher.cjs index 7aa25c8..07c0c86 100644 --- a/setup/js/slash_command_matcher.cjs +++ b/setup/js/slash_command_matcher.cjs @@ -9,7 +9,7 @@ * @returns {string} */ function parseSlashCommand(text) { - const match = /^\/([a-zA-Z0-9][a-zA-Z0-9._-]*)(?=$|\s)/.exec(String(text).trim()); + const match = /^\/([a-zA-Z0-9][a-zA-Z0-9._-]*)(?=$|\s)/.exec(String(text)); return match ? match[1] : ""; } diff --git a/setup/js/start_mcp_gateway.cjs b/setup/js/start_mcp_gateway.cjs index 1a902d8..78659e7 100644 --- a/setup/js/start_mcp_gateway.cjs +++ b/setup/js/start_mcp_gateway.cjs @@ -27,6 +27,7 @@ require("./shim.cjs"); * Optional: * - GH_AW_ENGINE: Engine type (copilot, codex, claude, gemini) * - GH_AW_MCP_CLI_SERVERS: JSON array of server names to exclude from agent config + * - GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES: JSON array of custom gateway environment variable names */ const { spawn, execSync } = require("child_process"); @@ -39,6 +40,8 @@ const { getErrorMessage } = require("./error_helpers.cjs"); /** @type {number | null} */ let activeGatewayPid = null; +const customGatewayEnvMarker = "__GH_AW_MCP_GATEWAY_CUSTOM_ENV__"; +const customGatewayEnvNamePattern = /^[A-Z_][A-Z0-9_]*$/; // --------------------------------------------------------------------------- // Timing helpers @@ -69,6 +72,37 @@ function sleep(ms) { return new Promise(resolve => setTimeout(resolve, ms)); } +/** + * Replaces the compiler marker with atomic Docker -e arguments. Runtime values + * never enter MCP_GATEWAY_DOCKER_COMMAND, preventing Docker argument injection. + * + * @param {string[]} args + * @param {NodeJS.ProcessEnv} env + * @returns {string[]} + */ +function injectCustomGatewayEnvArgs(args, env = process.env) { + const markerIndex = args.indexOf(customGatewayEnvMarker); + if (markerIndex === -1) { + return args; + } + + let names; + try { + names = JSON.parse(env.GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES || "[]"); + } catch (err) { + throw new Error(`GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES must be valid JSON: ${getErrorMessage(err)}`, { cause: err }); + } + if (!Array.isArray(names) || !names.every(name => typeof name === "string" && customGatewayEnvNamePattern.test(name))) { + throw new Error("GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES must be an array of valid environment variable names"); + } + + // Missing indexed transport values intentionally become empty container env vars. + // This preserves deterministic NAME→slot mapping and keeps Docker argument injection + // impossible even if the compiler/runtime metadata ever diverges. + const customArgs = names.flatMap((name, index) => ["-e", `${name}=${env[`GH_AW_MCP_GATEWAY_ENV_${index}`] || ""}`]); + return [...args.slice(0, markerIndex), ...customArgs, ...args.slice(markerIndex + 1)]; +} + /** * Builds targeted context for a JSON.parse error so logs can point at the likely key. * @@ -513,7 +547,6 @@ async function main() { // ----------------------------------------------------------------------- const logDir = "/tmp/gh-aw/mcp-logs/"; const outputPath = path.join(configDir, "gateway-output.json"); - const stderrLogPath = "/tmp/gh-aw/mcp-logs/stderr.log"; // Clean up any stale gateway container from a previous run on this runner. // On persistent self-hosted runners a prior job's gateway container may still @@ -537,18 +570,18 @@ async function main() { const gatewayStartTime = nowMs(); // Split docker command into args, respecting simple quoting - const args = dockerCommand.match(/(?:[^\s"']+|"[^"]*"|'[^']*')+/g) || []; + let args = Array.from(dockerCommand.match(/(?:[^\s"']+|"[^"]*"|'[^']*')+/g) || []); const cmd = args.shift(); if (!cmd) { core.setFailed("ERROR: MCP_GATEWAY_DOCKER_COMMAND did not contain an executable command"); return; } + args = injectCustomGatewayEnvArgs(args); const outputFd = fs.openSync(outputPath, "w", 0o600); - const stderrFd = fs.openSync(stderrLogPath, "w", 0o600); const child = spawn(cmd, args, { - stdio: ["pipe", outputFd, stderrFd], + stdio: ["pipe", outputFd, "ignore"], env: { ...process.env, MCP_GATEWAY_LOG_DIR: logDir }, detached: true, }); @@ -587,13 +620,6 @@ async function main() { } catch { core.error("No stdout output available"); } - core.error(""); - core.error("Gateway stderr logs:"); - try { - core.error(fs.readFileSync(stderrLogPath, "utf8")); - } catch { - core.error("No stderr logs available"); - } core.setFailed("ERROR: Gateway process exited immediately after start"); return; } @@ -615,13 +641,6 @@ async function main() { } catch { core.error("No stdout output available"); } - core.error(""); - core.error("Gateway stderr logs (debug output):"); - try { - core.error(fs.readFileSync(stderrLogPath, "utf8")); - } catch { - core.error("No stderr logs available"); - } core.setFailed(`ERROR: Gateway process (PID: ${gatewayPid}) exited during initialization`); return; } @@ -727,13 +746,6 @@ async function main() { core.error("No stdout output available"); } core.error(""); - core.error("Gateway stderr logs (debug output):"); - try { - core.error(fs.readFileSync(stderrLogPath, "utf8")); - } catch { - core.error("No stderr logs available"); - } - core.error(""); core.error("Checking network connectivity to gateway port..."); try { // Validate gatewayPort is numeric to prevent shell injection @@ -791,13 +803,6 @@ async function main() { } catch { core.error("No stdout output available"); } - core.error(""); - core.error("Gateway stderr logs:"); - try { - core.error(fs.readFileSync(stderrLogPath, "utf8")); - } catch { - core.error("No stderr logs available"); - } try { process.kill(gatewayPid); } catch { @@ -826,13 +831,6 @@ async function main() { core.error(""); core.error("Gateway error details:"); core.error(JSON.stringify(gatewayOutput, null, 2)); - core.error(""); - core.error("Gateway stderr logs:"); - try { - core.error(fs.readFileSync(stderrLogPath, "utf8")); - } catch { - core.error("No stderr logs available"); - } try { process.kill(gatewayPid); } catch { @@ -945,12 +943,11 @@ async function main() { if (fs.existsSync(checkScript)) { core.info("Running MCP server checks..."); - // Store diagnostics in /tmp/gh-aw/mcp-logs/start-gateway.log // Pass apiKey via MCP_GATEWAY_API_KEY env var (already set) rather than // as a shell argument to avoid shell metacharacter injection risks. const safePort = String(gatewayPort).replace(/[^0-9]/g, ""); try { - execSync(`bash "${checkScript}" "${outputPath}" "http://localhost:${safePort}" "$MCP_GATEWAY_API_KEY" 2>&1 | tee /tmp/gh-aw/mcp-logs/start-gateway.log`, { stdio: "inherit", env: process.env }); + execSync(`bash "${checkScript}" "${outputPath}" "http://localhost:${safePort}" "$MCP_GATEWAY_API_KEY"`, { stdio: "inherit", env: process.env }); } catch { core.error("ERROR: MCP server checks failed - no servers could be connected"); core.error("Gateway process will be terminated"); @@ -1064,6 +1061,7 @@ module.exports = { hasNonEmptyOTLPHeaders, isOTLPIfMissingIgnore, getJSONParseErrorContext, + injectCustomGatewayEnvArgs, normalizeSinkVisibilityEncoding, resolveCopilotConfigPaths, }; diff --git a/setup/js/update_project.cjs b/setup/js/update_project.cjs index d4f50a1..5833d09 100644 --- a/setup/js/update_project.cjs +++ b/setup/js/update_project.cjs @@ -380,20 +380,23 @@ async function findExistingItemByContentId(github, projectId, contentId) { while (hasNextPage) { const result = await github.graphql( - `query($projectId: ID!, $after: String) { - node(id: $projectId) { - ... on ProjectV2 { - items(first: 100, after: $after) { + `query($contentId: ID!, $after: String) { + node(id: $contentId) { + ... on Issue { + projectItems(first: 100, after: $after) { nodes { - id - content { - ... on Issue { - id - } - ... on PullRequest { - id - } - } + ...ProjectItemProject + } + pageInfo { + hasNextPage + endCursor + } + } + } + ... on PullRequest { + projectItems(first: 100, after: $after) { + nodes { + ...ProjectItemProject } pageInfo { hasNextPage @@ -402,20 +405,27 @@ async function findExistingItemByContentId(github, projectId, contentId) { } } } + } + fragment ProjectItemProject on ProjectV2Item { + id + project { id } }`, - { projectId, after: endCursor } + { contentId, after: endCursor } ); - if (!result?.node?.items) { - core.warning(`Project ${projectId} not found or inaccessible; stopping item search.`); + if (!result?.node) { + core.warning(`Content ${contentId} not found or inaccessible; stopping project item search.`); break; } - const found = result.node.items.nodes.find(item => item.content?.id === contentId); + const projectItems = result.node.projectItems; + if (!projectItems) break; + + const found = projectItems.nodes.find(item => item.project?.id === projectId); if (found) return found; - hasNextPage = result.node.items.pageInfo.hasNextPage; - endCursor = result.node.items.pageInfo.endCursor; + hasNextPage = projectItems.pageInfo.hasNextPage; + endCursor = projectItems.pageInfo.endCursor; } return null; diff --git a/setup/js/upload_artifact.cjs b/setup/js/upload_artifact.cjs index 11a44cd..74187b9 100644 --- a/setup/js/upload_artifact.cjs +++ b/setup/js/upload_artifact.cjs @@ -35,6 +35,7 @@ const { isStagedMode } = require("./safe_output_helpers.cjs"); */ const fs = require("fs"); +const os = require("os"); const path = require("path"); const { DefaultArtifactClient } = require("./artifact_client.cjs"); const { getErrorMessage } = require("./error_helpers.cjs"); @@ -123,6 +124,75 @@ function isWithinRoot(filePath, root) { return resolved.startsWith(normalRoot + path.sep) || resolved === normalRoot; } +/** + * Validate that a canonical absolute path does not refer to sensitive system or credential + * locations. Mirrors the check in safe_outputs_handlers.cjs for defense-in-depth. + * + * @param {string} canonicalPath - Resolved absolute path + * @returns {string|null} Error message or null if safe + */ +function validateSourcePath(canonicalPath) { + const parts = canonicalPath.split(path.sep); + if (parts.some(p => p === ".git")) { + return `path contains sensitive repository metadata (.git): ${canonicalPath}`; + } + + const systemDenied = ["/etc", "/proc", "/sys", "/dev", "/run", "/boot", "/lib", "/lib64", "/usr/lib", "/usr/local/lib"]; + for (const denied of systemDenied) { + const normalDenied = path.resolve(denied); + if (canonicalPath === normalDenied || canonicalPath.startsWith(normalDenied + path.sep)) { + return `path refers to a system directory: ${canonicalPath}`; + } + } + + const homeDir = os.homedir(); + if (homeDir) { + const sensitiveNames = [".ssh", ".aws", ".gnupg", ".docker", ".kube", ".azure", ".gcp", ".config", ".netrc", ".npmrc", ".gitconfig", ".gitcredentials", ".git-credentials"]; + for (const name of sensitiveNames) { + const sensitive = path.join(path.resolve(homeDir), name); + if (canonicalPath === sensitive || canonicalPath.startsWith(sensitive + path.sep)) { + return `path refers to a sensitive HOME location: ${canonicalPath}`; + } + } + } + + return null; +} + +/** + * Resolve a root path to its canonical form, falling back to path.resolve when the + * directory does not yet exist (e.g. GITHUB_WORKSPACE before checkout). + * @param {string} root + * @returns {string} + */ +function canonicalizeRoot(root) { + try { + return fs.realpathSync(root); + } catch { + return path.resolve(root); + } +} + +/** + * Validate that a canonical absolute path is within one of the allowed source roots. + * Allowed roots: staging directory, GITHUB_WORKSPACE. + * RUNNER_TEMP is intentionally excluded — only the specific staging subdirectory is allowed. + * + * @param {string} canonicalPath - Resolved absolute path + * @returns {string|null} Error message or null if within an allowed root + */ +function validateAllowedRoot(canonicalPath) { + const allowedRoots = [canonicalizeRoot(STAGING_DIR)]; + if (process.env.GITHUB_WORKSPACE) { + allowedRoots.push(canonicalizeRoot(process.env.GITHUB_WORKSPACE)); + } + const withinAllowedRoot = allowedRoots.some(root => canonicalPath === root || canonicalPath.startsWith(root + path.sep)); + if (!withinAllowedRoot) { + return `path is outside allowed source roots (GITHUB_WORKSPACE, staging directory): ${canonicalPath}`; + } + return null; +} + /** * Recursively list all regular files under a directory. * @param {string} dir - Absolute directory path @@ -181,6 +251,7 @@ function copySingleFileToStaging(sourcePath, destRelPath) { } try { fs.copyFileSync(sourcePath, destPath); + fs.chmodSync(destPath, 0o600); } catch (err) { throw new Error(`Failed to copy file ${sourcePath} to ${destPath}: ${getErrorMessage(err)}`, { cause: err }); } @@ -207,10 +278,32 @@ function copyDirectoryToStaging(sourceDir, destRelDir) { continue; } if (stat.isDirectory()) { + // Reject sensitive directory names at every level (e.g. foo/.git/config). + let canonicalDir; + try { + canonicalDir = fs.realpathSync(srcFull); + } catch (err) { + return { copiedCount, error: `failed to resolve canonical path for ${srcFull}: ${err instanceof Error ? err.message : String(err)}` }; + } + const sensitiveErr = validateSourcePath(canonicalDir); + if (sensitiveErr) { + return { copiedCount, error: sensitiveErr }; + } const sub = copyDirectoryToStaging(srcFull, destRel); if (sub.error) return sub; copiedCount += sub.copiedCount; } else if (stat.isFile()) { + // Revalidate each file's canonical path before copying. + let canonicalFile; + try { + canonicalFile = fs.realpathSync(srcFull); + } catch (err) { + return { copiedCount, error: `failed to resolve canonical path for ${srcFull}: ${err instanceof Error ? err.message : String(err)}` }; + } + const sensitiveErr = validateSourcePath(canonicalFile); + if (sensitiveErr) { + return { copiedCount, error: sensitiveErr }; + } const result = copySingleFileToStaging(srcFull, destRel); if (result.error) return { copiedCount, error: result.error }; copiedCount++; @@ -241,6 +334,23 @@ function autoCopyToStaging(reqPath) { if (stat === null) { return { copied: false, relPath: "", error: `symlinks are not allowed: ${reqPath}` }; } + + // Canonicalize and validate before copying. + let canonical; + try { + canonical = fs.realpathSync(reqPath); + } catch (err) { + return { copied: false, relPath: "", error: `failed to resolve canonical path for ${reqPath}: ${err instanceof Error ? err.message : String(err)}` }; + } + const sensitiveErr = validateSourcePath(canonical); + if (sensitiveErr) { + return { copied: false, relPath: "", error: sensitiveErr }; + } + const rootErr = validateAllowedRoot(canonical); + if (rootErr) { + return { copied: false, relPath: "", error: rootErr }; + } + // Derive a relative destination path from the basename (or relative to filesystem root for nested paths). const relPath = path.basename(reqPath); if (stat.isDirectory()) { @@ -272,6 +382,23 @@ function autoCopyToStaging(reqPath) { if (stat === null) { return { copied: false, relPath: "", error: `symlinks are not allowed: ${candidate}` }; } + + // Canonicalize and validate before copying. + let canonical; + try { + canonical = fs.realpathSync(candidate); + } catch (err) { + return { copied: false, relPath: "", error: `failed to resolve canonical path for ${candidate}: ${err instanceof Error ? err.message : String(err)}` }; + } + const sensitiveErr = validateSourcePath(canonical); + if (sensitiveErr) { + return { copied: false, relPath: "", error: sensitiveErr }; + } + const rootErr = validateAllowedRoot(canonical); + if (rootErr) { + return { copied: false, relPath: "", error: rootErr }; + } + if (stat.isDirectory()) { const result = copyDirectoryToStaging(candidate, reqPath); if (result.error) return { copied: false, relPath: "", error: result.error }; diff --git a/setup/sh/check_mcp_servers.sh b/setup/sh/check_mcp_servers.sh index 79d6139..c200cd2 100755 --- a/setup/sh/check_mcp_servers.sh +++ b/setup/sh/check_mcp_servers.sh @@ -260,9 +260,7 @@ if [ $REQUIRED_SERVERS_FAILED -gt 0 ]; then echo " - MCP server unavailable or rejecting requests" echo " - Network connectivity or DNS issues" echo "" - echo "Check the gateway logs and individual server logs for more details:" - echo " /tmp/gh-aw/mcp-logs/stderr.log" - echo " /tmp/gh-aw/mcp-logs/start-gateway.log" + echo "Check the MCP server output above and individual server logs for more details." exit 1 elif [ $SERVERS_SUCCEEDED -eq 0 ] && [ $SERVERS_FAILED -eq 0 ]; then echo "ERROR: No HTTP servers were successfully checked" @@ -279,9 +277,7 @@ elif [ $SERVERS_SUCCEEDED -eq 0 ]; then echo "All configured HTTP MCP servers are optional but none connected successfully." echo "At least one server must connect for the gateway to be considered healthy." echo "" - echo "Check the gateway logs and individual server logs for more details:" - echo " /tmp/gh-aw/mcp-logs/stderr.log" - echo " /tmp/gh-aw/mcp-logs/start-gateway.log" + echo "Check the MCP server output above and individual server logs for more details." exit 1 else if [ $SERVERS_FAILED -gt 0 ]; then diff --git a/setup/sh/conclude_threat_detection.sh b/setup/sh/conclude_threat_detection.sh index 8920b5f..0450900 100755 --- a/setup/sh/conclude_threat_detection.sh +++ b/setup/sh/conclude_threat_detection.sh @@ -6,41 +6,28 @@ set -euo pipefail RESULT_FILE="${1:-/tmp/gh-aw/threat-detection/detection_result.json}" RESULT_DIR="$(dirname "${RESULT_FILE}")" DETECTION_LOG_FILE="${DETECTION_LOG_FILE:-${RESULT_DIR}/detection.log}" -DETECTION_STATUS_PREFIX="THREAT_DETECTION_STATUS:" -continue_on_error="${GH_AW_DETECTION_CONTINUE_ON_ERROR:-true}" -continue_on_error="$(echo "${continue_on_error}" | tr '[:upper:]' '[:lower:]')" -if [ "${RUN_DETECTION:-false}" != "true" ]; then - echo "conclusion=skipped" >> "${GITHUB_OUTPUT}" - echo "success=true" >> "${GITHUB_OUTPUT}" - echo "reason=" >> "${GITHUB_OUTPUT}" - exit 0 -fi - -if [ ! -f "${RESULT_FILE}" ]; then - detection_status="" - if [ -f "${DETECTION_LOG_FILE}" ]; then - detection_status="$(grep "${DETECTION_STATUS_PREFIX}" "${DETECTION_LOG_FILE}" | tail -n 1 || true)" - fi - - result_message="Detection result file not found at: ${RESULT_FILE} (execution outcome: ${DETECTION_AGENTIC_EXECUTION_OUTCOME:-unknown})" - if [ -n "${detection_status}" ]; then - result_message="${result_message}; detector status: ${detection_status}" - elif [ -f "${DETECTION_LOG_FILE}" ]; then - result_message="${result_message}; detection log exists at ${DETECTION_LOG_FILE} but did not include ${DETECTION_STATUS_PREFIX}" - else - result_message="${result_message}; detection log not found at ${DETECTION_LOG_FILE}" - fi - - if [ "${continue_on_error}" = "true" ]; then - echo "::warning::${result_message}; continuing because GH_AW_DETECTION_CONTINUE_ON_ERROR=true" +# threat-detect conclude handles every branch of the conclusion contract +# (skipped run, missing/malformed result file, warn-mode vs strict-mode +# hard-fail rules, status-reason mapping, diagnostics, and step summary). +# The only failure it cannot report on itself is its own absence from PATH. +if ! command -v threat-detect >/dev/null 2>&1; then + message="threat-detect binary not found on PATH" + continue_on_error="${GH_AW_DETECTION_CONTINUE_ON_ERROR:-true}" + if [ "${continue_on_error,,}" != "false" ]; then + echo "::warning::${message}; continuing because GH_AW_DETECTION_CONTINUE_ON_ERROR != false" echo "conclusion=warning" >> "${GITHUB_OUTPUT}" echo "success=false" >> "${GITHUB_OUTPUT}" echo "reason=agent_failure" >> "${GITHUB_OUTPUT}" exit 0 fi - echo "ERR_SYSTEM: ❌ ${result_message}" + echo "conclusion=failure" >> "${GITHUB_OUTPUT}" + echo "success=false" >> "${GITHUB_OUTPUT}" + echo "reason=agent_failure" >> "${GITHUB_OUTPUT}" + echo "ERR_SYSTEM: ${message}" exit 1 fi -threat-detect conclude --result-file "${RESULT_FILE}" +exec threat-detect conclude \ + --result-file "${RESULT_FILE}" \ + --detection-log "${DETECTION_LOG_FILE}" diff --git a/setup/sh/install_threat_detect_binary.sh b/setup/sh/install_threat_detect_binary.sh index 102a3f6..6bdd581 100755 --- a/setup/sh/install_threat_detect_binary.sh +++ b/setup/sh/install_threat_detect_binary.sh @@ -15,6 +15,11 @@ set +o histexpand # # Platform support: # - Linux (x64, arm64): Downloads pre-built binary +# - macOS: NOT supported. Agentic workflows require Linux container jobs, and the +# compiler rejects macOS runner labels (including +# safe-outputs.threat-detection.runs-on) before a workflow is generated. If this +# script is ever reached on Darwin it fails fast with an explicit message instead +# of attempting a download. # # Security features: # - Downloads directly from GitHub releases @@ -27,6 +32,7 @@ set -euo pipefail THREAT_DETECT_REPO="github/gh-aw-threat-detection" THREAT_DETECT_INSTALL_DIR="/usr/local/bin" THREAT_DETECT_INSTALL_NAME="threat-detect" +MACOS_FAQ_URL="https://github.github.com/gh-aw/reference/faq/#why-are-macos-runners-not-supported" # Parse arguments: treat the first non-flag argument as VERSION, all -- arguments as flags. THREAT_DETECT_VERSION="" @@ -79,6 +85,24 @@ ARCH="$(uname -m)" echo "Installing threat-detect with checksum verification (version: ${THREAT_DETECT_VERSION}, os: ${OS}, arch: ${ARCH})" +# Fail fast on unsupported platforms before any network access. Only Linux is supported: +# agentic workflows require Linux container jobs, and the compiler rejects macOS runner +# labels (including safe-outputs.threat-detection.runs-on) at compile time. +case "$OS" in + Linux) ;; + Darwin) + echo "ERROR: macOS is not a supported platform for threat-detect." + echo " Agentic workflows require Linux container jobs; use a Linux runner instead." + echo " See ${MACOS_FAQ_URL} for details." + exit 1 + ;; + *) + echo "ERROR: Unsupported operating system: ${OS}" + echo " threat-detect is only published for Linux (x64, arm64)." + exit 1 + ;; +esac + # Download URLs BASE_URL="https://github.com/${THREAT_DETECT_REPO}/releases/download/${THREAT_DETECT_VERSION}" CHECKSUMS_URL="${BASE_URL}/checksums.txt" @@ -150,39 +174,7 @@ install_linux_binary() { maybe_sudo mv "${TEMP_DIR}/${binary_name}" "${THREAT_DETECT_INSTALL_DIR}/${THREAT_DETECT_INSTALL_NAME}" } -install_darwin_binary() { - # Determine binary name based on architecture - local binary_name - case "$ARCH" in - x86_64) binary_name="threat-detect-darwin-x64" ;; - arm64) binary_name="threat-detect-darwin-arm64" ;; - *) echo "ERROR: Unsupported macOS architecture: ${ARCH}"; exit 1 ;; - esac - - local binary_url="${BASE_URL}/${binary_name}" - echo "Downloading binary from \"${binary_url}\"..." - curl -fsSL --retry 5 --retry-delay 10 --retry-max-time 180 -o "${TEMP_DIR}/${binary_name}" "${binary_url}" - - # Verify checksum - verify_checksum "${TEMP_DIR}/${binary_name}" "${binary_name}" - - # Make binary executable and install - chmod +x "${TEMP_DIR}/${binary_name}" - maybe_sudo mv "${TEMP_DIR}/${binary_name}" "${THREAT_DETECT_INSTALL_DIR}/${THREAT_DETECT_INSTALL_NAME}" -} - -case "$OS" in - Linux) - install_linux_binary - ;; - Darwin) - install_darwin_binary - ;; - *) - echo "ERROR: Unsupported operating system: ${OS}" - exit 1 - ;; -esac +install_linux_binary # In rootless mode, add the install dir to PATH for subsequent steps. if [ "$ROOTLESS" = "true" ]; then diff --git a/setup/sh/prepare_threat_detection_files.sh b/setup/sh/prepare_threat_detection_files.sh new file mode 100755 index 0000000..044fc80 --- /dev/null +++ b/setup/sh/prepare_threat_detection_files.sh @@ -0,0 +1,51 @@ +#!/usr/bin/env bash +set +o histexpand + +set -euo pipefail + +SOURCE_DIR="${1:-/tmp/gh-aw}" +DETECTION_DIR="${2:-${SOURCE_DIR}/threat-detection}" +PROMPT_SOURCE_DIR="${SOURCE_DIR}/aw-prompts" +PROMPT_DETECTION_DIR="${DETECTION_DIR}/aw-prompts" + +copy_optional_file() { + local source_file="$1" + local destination_file="$2" + + if [ -f "${source_file}" ]; then + cp "${source_file}" "${destination_file}" + fi +} + +mkdir -p "${PROMPT_DETECTION_DIR}" +rm -f "${SOURCE_DIR}/agent_usage.json" + +copy_optional_file "${PROMPT_SOURCE_DIR}/prompt.txt" "${PROMPT_DETECTION_DIR}/prompt.txt" +if [ ! -s "${PROMPT_DETECTION_DIR}/prompt.txt" ]; then + echo "::warning::ERR_VALIDATION: Missing or empty detection context prompt at ${PROMPT_DETECTION_DIR}/prompt.txt. Ensure the agent artifact includes ${PROMPT_SOURCE_DIR}/prompt.txt. Detection will continue with fallback workflow context." +fi + +copy_optional_file "${PROMPT_SOURCE_DIR}/prompt-template.txt" "${PROMPT_DETECTION_DIR}/prompt-template.txt" +copy_optional_file "${PROMPT_SOURCE_DIR}/prompt-import-tree.json" "${PROMPT_DETECTION_DIR}/prompt-import-tree.json" +copy_optional_file "${SOURCE_DIR}/aw_info.json" "${DETECTION_DIR}/aw_info.json" +copy_optional_file "${SOURCE_DIR}/agent_output.json" "${DETECTION_DIR}/agent_output.json" + +if [ -d "${SOURCE_DIR}/comment-memory" ]; then + mkdir -p "${DETECTION_DIR}/comment-memory" + for memory_file in "${SOURCE_DIR}"/comment-memory/*.md; do + if [ -f "${memory_file}" ]; then + cp "${memory_file}" "${DETECTION_DIR}/comment-memory/" + fi + done +fi + +for artifact_pattern in aw-*.patch aw-*.bundle; do + for artifact_file in "${SOURCE_DIR}"/${artifact_pattern}; do + if [ -f "${artifact_file}" ]; then + cp "${artifact_file}" "${DETECTION_DIR}/" + fi + done +done + +echo "Prepared threat detection files:" +ls -la "${DETECTION_DIR}" diff --git a/setup/sh/start_mcp_gateway.sh b/setup/sh/start_mcp_gateway.sh index c7dd9ac..5a749ed 100755 --- a/setup/sh/start_mcp_gateway.sh +++ b/setup/sh/start_mcp_gateway.sh @@ -168,7 +168,7 @@ GATEWAY_START_TIME=$(date +%s%3N) # Note: MCP_GATEWAY_DOCKER_COMMAND is the full docker command with all flags, mounts, and image # Pass MCP_GATEWAY_LOG_DIR to the container via -e flag echo "$MCP_CONFIG" | MCP_GATEWAY_LOG_DIR="$MCP_GATEWAY_LOG_DIR" $MCP_GATEWAY_DOCKER_COMMAND \ - > /tmp/gh-aw/mcp-config/gateway-output.json 2> /tmp/gh-aw/mcp-logs/stderr.log & + > /tmp/gh-aw/mcp-config/gateway-output.json 2> /dev/null & GATEWAY_PID=$! echo "Gateway started with PID: $GATEWAY_PID" @@ -181,9 +181,6 @@ else echo "" echo "Gateway stdout output:" cat /tmp/gh-aw/mcp-config/gateway-output.json 2>/dev/null || echo "No stdout output available" - echo "" - echo "Gateway stderr logs:" - cat /tmp/gh-aw/mcp-logs/stderr.log 2>/dev/null || echo "No stderr logs available" exit 1 fi echo "" @@ -200,9 +197,6 @@ if ! ps -p $GATEWAY_PID > /dev/null 2>&1; then echo "" echo "Gateway stdout (errors are written here per MCP Gateway Specification):" cat /tmp/gh-aw/mcp-config/gateway-output.json 2>/dev/null || echo "No stdout output available" - echo "" - echo "Gateway stderr logs (debug output):" - cat /tmp/gh-aw/mcp-logs/stderr.log || echo "No stderr logs available" exit 1 fi echo "Gateway process is still running (PID: $GATEWAY_PID)" @@ -316,9 +310,6 @@ else echo "Gateway stdout (errors are written here per MCP Gateway Specification):" cat /tmp/gh-aw/mcp-config/gateway-output.json 2>/dev/null || echo "No stdout output available" echo "" - echo "Gateway stderr logs (debug output):" - cat /tmp/gh-aw/mcp-logs/stderr.log || echo "No stderr logs available" - echo "" echo "Checking network connectivity to gateway port..." netstat -tlnp 2>/dev/null | grep ":${MCP_GATEWAY_PORT}" || ss -tlnp 2>/dev/null | grep ":${MCP_GATEWAY_PORT}" || echo "Port ${MCP_GATEWAY_PORT} does not appear to be listening" kill $GATEWAY_PID 2>/dev/null || true @@ -350,9 +341,6 @@ if [ ! -s /tmp/gh-aw/mcp-config/gateway-output.json ]; then echo "" echo "Gateway stdout (should contain error or config):" cat /tmp/gh-aw/mcp-config/gateway-output.json 2>/dev/null || echo "No stdout output available" - echo "" - echo "Gateway stderr logs:" - cat /tmp/gh-aw/mcp-logs/stderr.log || echo "No stderr logs available" kill $GATEWAY_PID 2>/dev/null || true exit 1 fi @@ -367,9 +355,6 @@ if jq -e '.error' /tmp/gh-aw/mcp-config/gateway-output.json >/dev/null 2>&1; the echo "" echo "Gateway error details:" cat /tmp/gh-aw/mcp-config/gateway-output.json - echo "" - echo "Gateway stderr logs:" - cat /tmp/gh-aw/mcp-logs/stderr.log || echo "No stderr logs available" kill $GATEWAY_PID 2>/dev/null || true exit 1 fi @@ -456,20 +441,15 @@ echo "Checking MCP server functionality..." MCP_CHECK_START=$(date +%s%3N) if [ -f ${RUNNER_TEMP}/gh-aw/actions/check_mcp_servers.sh ]; then echo "Running MCP server checks..." - # Store check diagnostic logs in /tmp/gh-aw/mcp-logs/start-gateway.log for artifact upload - # Use tee to output to both stdout and the log file - # Enable pipefail so the exit code comes from check_mcp_servers.sh, not tee - set -o pipefail if ! bash ${RUNNER_TEMP}/gh-aw/actions/check_mcp_servers.sh \ /tmp/gh-aw/mcp-config/gateway-output.json \ "http://localhost:${MCP_GATEWAY_PORT}" \ - "${MCP_GATEWAY_API_KEY}" 2>&1 | tee /tmp/gh-aw/mcp-logs/start-gateway.log; then + "${MCP_GATEWAY_API_KEY}"; then echo "ERROR: MCP server checks failed - no servers could be connected" echo "Gateway process will be terminated" kill $GATEWAY_PID 2>/dev/null || true exit 1 fi - set +o pipefail print_timing $MCP_CHECK_START "MCP server connectivity checks" else echo "WARNING: MCP server check script not found at ${RUNNER_TEMP}/gh-aw/actions/check_mcp_servers.sh"