diff --git a/actions/setup/js/add_comment.cjs b/actions/setup/js/add_comment.cjs index c5454510815..633ae302394 100644 --- a/actions/setup/js/add_comment.cjs +++ b/actions/setup/js/add_comment.cjs @@ -25,7 +25,7 @@ const { createDiscussionComment, resolveTopLevelDiscussionCommentId } = require( const { logStagedPreviewInfo } = require("./staged_preview.cjs"); const { ERR_NOT_FOUND } = require("./error_codes.cjs"); const { isPayloadUserBot } = require("./resolve_mentions.cjs"); -const { resolveMentionsForItem } = require("./resolve_mentions_from_payload.cjs"); +const { getMentionsGithubClient, resolveMentionsForItem } = require("./resolve_mentions_from_payload.cjs"); const { buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs"); const { generateHistoryUrl } = require("./generate_history_link.cjs"); const { resolveInvocationContext } = require("./invocation_context_helpers.cjs"); @@ -753,7 +753,8 @@ async function main(config = {}) { if (itemTargetResult.number != null || hasExplicitCommentId) { // Explicit item_number/issue_number: fetch the issue/PR to get its author try { - const { data: issueData } = await githubClient.rest.issues.get({ + const mentionsGithubClient = getMentionsGithubClient(githubClient); + const { data: issueData } = await mentionsGithubClient.rest.issues.get({ owner: repoParts.owner, repo: repoParts.repo, issue_number: itemNumber, diff --git a/actions/setup/js/collect_ndjson_output.cjs b/actions/setup/js/collect_ndjson_output.cjs index 31c1b949c06..6edd617c141 100644 --- a/actions/setup/js/collect_ndjson_output.cjs +++ b/actions/setup/js/collect_ndjson_output.cjs @@ -5,19 +5,17 @@ const { getErrorMessage } = require("./error_helpers.cjs"); const { repairJson, sanitizePrototypePollution } = require("./json_repair_helpers.cjs"); const { AGENT_OUTPUT_FILENAME, TMP_GH_AW_PATH } = require("./constants.cjs"); const { ERR_API, ERR_PARSE } = require("./error_codes.cjs"); -const { isPayloadUserBot } = require("./resolve_mentions.cjs"); const { parseIntTemplatable } = require("./templatable.cjs"); -const { getDefaultTargetRepo, parseAllowedRepos, resolveAndValidateRepo } = require("./repo_helpers.cjs"); const { isProbingNoopMessage } = require("./intent_probe.cjs"); const { buildEmptyOutputOutcome } = require("./empty_output_outcome.cjs"); +const MENTION_AWARE_OUTPUT_TYPES = new Set(["add_comment", "close_discussion", "create_discussion", "create_issue", "create_pull_request", "create_pull_request_review_comment", "reply_to_pull_request_review_comment"]); + async function main() { try { const fs = require("fs"); const { sanitizeContent } = require("./sanitize_content.cjs"); const { validateItem, getMaxAllowedForType, getMinRequiredForType, hasValidationConfig, MAX_BODY_LENGTH: maxBodyLength, resetValidationConfigCache } = require("./safe_output_type_validator.cjs"); - const { resolveAllowedMentionsFromPayload } = require("./resolve_mentions_from_payload.cjs"); - // Load validation config from file and set it in environment for the validator to read const validationConfigPath = process.env.GH_AW_VALIDATION_CONFIG_PATH || `${process.env.RUNNER_TEMP}/gh-aw/safeoutputs/validation.json`; /** @type {any} */ @@ -38,8 +36,10 @@ async function main() { const mentionsConfig = validationConfig?.mentions || null; const maxMentions = parseIntTemplatable(mentionsConfig?.max, 50); - // Resolve mentions for each output's destination before sanitizing it. + // Mention filtering happens in the trusted safe_outputs job. Preserve mentions + // in these output types until their destination and allowlist can be resolved. let allowedMentions = []; + let deferMentionFiltering = false; // maxBotMentions is populated after safeOutputsConfig is read below /** @type {number | undefined} */ @@ -68,7 +68,7 @@ async function main() { error: `Line ${lineNum}: ${fieldName} must be a string`, }; } - normalizedValue = sanitizeContent(value, { allowedAliases: allowedMentions, maxMentions, maxBotMentions, allowedAliasesSeen }); + normalizedValue = sanitizeContent(value, { allowedAliases: allowedMentions, maxMentions, maxBotMentions, allowedAliasesSeen, deferMentions: deferMentionFiltering }); break; case "boolean": if (typeof value !== "boolean") { @@ -99,11 +99,11 @@ async function main() { error: `Line ${lineNum}: ${fieldName} must be one of: ${inputSchema.options.join(", ")}`, }; } - normalizedValue = sanitizeContent(value, { allowedAliases: allowedMentions, maxMentions, maxBotMentions, allowedAliasesSeen }); + normalizedValue = sanitizeContent(value, { allowedAliases: allowedMentions, maxMentions, maxBotMentions, allowedAliasesSeen, deferMentions: deferMentionFiltering }); break; default: if (typeof value === "string") { - normalizedValue = sanitizeContent(value, { allowedAliases: allowedMentions, maxMentions, maxBotMentions, allowedAliasesSeen }); + normalizedValue = sanitizeContent(value, { allowedAliases: allowedMentions, maxMentions, maxBotMentions, allowedAliasesSeen, deferMentions: deferMentionFiltering }); } break; } @@ -224,46 +224,6 @@ async function main() { // indentation/pretty-printing, parsing will fail. const lines = outputContent.trim().split("\n"); - function resolveMentionRepo(item, itemType) { - const typeConfig = expectedOutputTypes[itemType]; - const defaultTargetRepo = getDefaultTargetRepo(typeConfig && typeof typeConfig === "object" ? typeConfig : undefined); - const allowedRepos = parseAllowedRepos(typeConfig?.allowed_repos ?? safeOutputsConfig?.allowed_repos); - return resolveAndValidateRepo(item, defaultTargetRepo, allowedRepos, "mention"); - } - - // Pre-scan: collect target issue authors from add_comment items with explicit item_number - // so they are included when sanitizing the corresponding comment. - const targetIssueAuthors = new Map(); - for (const line of lines) { - const trimmedLine = line.trim(); - if (!trimmedLine) continue; - try { - const preview = JSON.parse(trimmedLine); - const previewType = (preview?.type || "").replace(/-/g, "_"); - if (previewType === "add_comment" && preview.item_number != null && typeof preview.item_number === "number") { - const repoResult = resolveMentionRepo(preview, "add_comment"); - if (!repoResult.success) { - core.info(`[MENTIONS] Skipping target issue author lookup: ${repoResult.error}`); - continue; - } - try { - const { data: issueData } = await github.rest.issues.get({ - owner: repoResult.repoParts.owner, - repo: repoResult.repoParts.repo, - issue_number: preview.item_number, - }); - if (issueData.user?.login && !isPayloadUserBot(issueData.user)) { - targetIssueAuthors.set(`${repoResult.repo.toLowerCase()}#${preview.item_number}`, issueData.user.login); - } - } catch (fetchErr) { - core.info(`[MENTIONS] Could not fetch issue #${preview.item_number} author for mention allowlist: ${getErrorMessage(fetchErr)}`); - } - } - } catch { - // Ignore parse errors - main loop will report them - } - } - const parsedItems = []; const errors = collectionErrors; for (let i = 0; i < lines.length; i++) { @@ -286,6 +246,7 @@ async function main() { core.info(`[INGESTION] Line ${i + 1}: Original type='${originalType}', Normalized type='${itemType}'`); // Update item.type to normalized value item.type = itemType; + deferMentionFiltering = MENTION_AWARE_OUTPUT_TYPES.has(itemType); if (!expectedOutputTypes[itemType]) { core.warning(`[INGESTION] Line ${i + 1}: Type '${itemType}' not found in expected types: ${JSON.stringify(Object.keys(expectedOutputTypes))}`); errors.push(`Line ${i + 1}: Unexpected output type '${itemType}'. Expected one of: ${Object.keys(expectedOutputTypes).join(", ")}`); @@ -295,17 +256,6 @@ async function main() { core.info(`[INGESTION] Line ${i + 1}: Ignoring probing noop message (does not count against the noop budget): ${JSON.stringify(item.message)}`); continue; } - const repoResult = resolveMentionRepo(item, itemType); - allowedMentions = repoResult.success - ? await resolveAllowedMentionsFromPayload( - context, - github, - core, - mentionsConfig, - itemType === "add_comment" ? [targetIssueAuthors.get(`${repoResult.repo.toLowerCase()}#${item.item_number}`)].filter(Boolean) : undefined, - repoResult.repoParts - ) - : []; const typeCount = parsedItems.filter(existing => existing.type === itemType).length; const maxAllowed = getMaxAllowedForType(itemType, expectedOutputTypes); if (typeCount >= maxAllowed) { @@ -333,6 +283,7 @@ async function main() { allowedAliases: allowedMentions, maxMentions, maxBotMentions, + deferMentions: deferMentionFiltering, normalizeIssueClosingKeywords, dataEnabled: typeConfig !== null && typeof typeConfig === "object" && typeConfig.data_enabled === true, dataSchema: typeConfig !== null && typeof typeConfig === "object" ? typeConfig.data_schema : undefined, diff --git a/actions/setup/js/collect_ndjson_output.test.cjs b/actions/setup/js/collect_ndjson_output.test.cjs index a69872dcad2..c8d5f89f4d9 100644 --- a/actions/setup/js/collect_ndjson_output.test.cjs +++ b/actions/setup/js/collect_ndjson_output.test.cjs @@ -1391,7 +1391,7 @@ describe("collect_ndjson_output.cjs", () => { parsedOutput = JSON.parse(outputCall[1]); expect(parsedOutput.items[0].body).toBe("GitHub URLs: https://github.com/repo, https://api.github.com/users, https://githubusercontent.com/file. External: (example.com/redacted)"); }), - it("should handle @mentions neutralization", async () => { + it("should defer mention filtering for trusted output processing", async () => { const testFile = "/tmp/gh-aw/test-ndjson-output.txt", ndjsonContent = '{"type": "create_issue", "title": "@mention Test", "body": "Hey @username and @org/team, check this out! But preserve email@domain.com"}'; (fs.writeFileSync(testFile, ndjsonContent), (process.env.GH_AW_SAFE_OUTPUTS = testFile)); @@ -1400,9 +1400,10 @@ describe("collect_ndjson_output.cjs", () => { (fs.mkdirSync("/tmp/gh-aw/safeoutputs", { recursive: !0 }), fs.writeFileSync(configPath, __config), await eval(`(async () => { ${collectScript}; await main(); })()`)); const outputCall = mockCore.setOutput.mock.calls.find(call => "output" === call[0]), parsedOutput = JSON.parse(outputCall[1]); - expect(parsedOutput.items[0].body).toBe("Hey `@username` and `@org/team`, check this out! But preserve email@domain.com"); + expect(parsedOutput.items[0].body).toBe("Hey @username and @org/team, check this out! But preserve email@domain.com"); + expect(global.github.rest.repos.listCollaborators).not.toHaveBeenCalled(); }), - it("checks collaborators in each comment's target repository, never the workflow repository", async () => { + it("does not query collaborators during untrusted ingestion", async () => { global.context.payload.issue = { user: { login: "alice", type: "User" } }; const testFile = "/tmp/gh-aw/test-ndjson-output.txt"; fs.writeFileSync(testFile, [JSON.stringify({ type: "add_comment", repo: "target-org/first", body: "Hello @alice" }), JSON.stringify({ type: "add_comment", repo: "target-org/second", body: "Hello @alice" })].join("\n")); @@ -1411,11 +1412,11 @@ describe("collect_ndjson_output.cjs", () => { await eval(`(async () => { ${collectScript}; await main(); })()`); - expect(global.github.rest.repos.listCollaborators).toHaveBeenCalledWith(expect.objectContaining({ owner: "target-org", repo: "first" })); - expect(global.github.rest.repos.listCollaborators).toHaveBeenCalledWith(expect.objectContaining({ owner: "target-org", repo: "second" })); - expect(global.github.rest.repos.listCollaborators).not.toHaveBeenCalledWith(expect.objectContaining({ owner: "test-owner", repo: "test-repo" })); + expect(global.github.rest.repos.listCollaborators).not.toHaveBeenCalled(); + const parsed = JSON.parse(mockCore.setOutput.mock.calls.find(call => call[0] === "output")[1]); + expect(parsed.items.map(item => item.body)).toEqual(["Hello @alice", "Hello @alice"]); }), - it("keeps target issue authors scoped to their own repository and issue", async () => { + it("does not query target issue authors during untrusted ingestion", async () => { const testFile = "/tmp/gh-aw/test-ndjson-output.txt"; fs.writeFileSync( testFile, @@ -1433,10 +1434,10 @@ describe("collect_ndjson_output.cjs", () => { await eval(`(async () => { ${collectScript}; await main(); })()`); const parsed = JSON.parse(mockCore.setOutput.mock.calls.find(call => call[0] === "output")[1]); - expect(parsed.items.map(item => item.body)).toEqual(["Hello @first-author", "Hello `@first-author`"]); - expect(global.github.rest.issues.get).toHaveBeenCalledWith(expect.objectContaining({ owner: "target-org", repo: "first", issue_number: 7 })); + expect(parsed.items.map(item => item.body)).toEqual(["Hello @first-author", "Hello @first-author"]); + expect(global.github.rest.issues.get).not.toHaveBeenCalled(); }), - it("looks up explicit issue authors in a configured target-repo", async () => { + it("does not look up explicit issue authors during untrusted ingestion", async () => { const testFile = "/tmp/gh-aw/test-ndjson-output.txt"; fs.writeFileSync(testFile, JSON.stringify({ type: "add_comment", item_number: 7, body: "Hello @target-author" })); process.env.GH_AW_SAFE_OUTPUTS = testFile; @@ -1445,11 +1446,11 @@ describe("collect_ndjson_output.cjs", () => { await eval(`(async () => { ${collectScript}; await main(); })()`); - expect(global.github.rest.issues.get).toHaveBeenCalledWith(expect.objectContaining({ owner: "target-org", repo: "target-repo", issue_number: 7 })); + expect(global.github.rest.issues.get).not.toHaveBeenCalled(); const parsed = JSON.parse(mockCore.setOutput.mock.calls.find(call => call[0] === "output")[1]); expect(parsed.items[0].body).toBe("Hello @target-author"); }), - it("does not query either repository for a disallowed per-item override", async () => { + it("does not query repositories for per-item overrides during untrusted ingestion", async () => { const testFile = "/tmp/gh-aw/test-ndjson-output.txt"; fs.writeFileSync(testFile, JSON.stringify({ type: "add_comment", repo: "unauthorized/repo", body: "Hello @alice" })); process.env.GH_AW_SAFE_OUTPUTS = testFile; @@ -1460,7 +1461,7 @@ describe("collect_ndjson_output.cjs", () => { expect(global.github.rest.repos.listCollaborators).not.toHaveBeenCalled(); const parsed = JSON.parse(mockCore.setOutput.mock.calls.find(call => call[0] === "output")[1]); - expect(parsed.items[0].body).toBe("Hello `@alice`"); + expect(parsed.items[0].body).toBe("Hello @alice"); }), it("should preserve allowed aliases after max when no more than max occur", async () => { const allowed = Array.from({ length: 60 }, (_, i) => `user${i}`); @@ -1481,7 +1482,7 @@ describe("collect_ndjson_output.cjs", () => { const parsedOutput = JSON.parse(outputCall[1]); expect(parsedOutput.items[0].body).toBe("Thanks @user57, @user58, and @user59"); }), - it("should apply the mention limit across all fields in one item", async () => { + it("should defer mention limits across all fields to trusted output processing", async () => { const validationPath = "/tmp/gh-aw/safeoutputs/validation.json"; const validationConfig = JSON.parse(fs.readFileSync(validationPath, "utf8")); validationConfig.mentions = { allowContext: false, allowed: ["user1", "user2", "user3", "user4"], max: 3 }; @@ -1497,7 +1498,7 @@ describe("collect_ndjson_output.cjs", () => { const outputCall = mockCore.setOutput.mock.calls.find(call => call[0] === "output"); const parsedOutput = JSON.parse(outputCall[1]); expect(parsedOutput.items[0].title).toBe("@user1 @user2"); - expect(parsedOutput.items[0].body).toBe("@user3 `@user4` @user1"); + expect(parsedOutput.items[0].body).toBe("@user3 @user4 @user1"); }), it("should neutralize bot trigger phrases", async () => { const testFile = "/tmp/gh-aw/test-ndjson-output.txt", diff --git a/actions/setup/js/resolve_mentions_from_payload.cjs b/actions/setup/js/resolve_mentions_from_payload.cjs index d2f057ce8e4..f19321f040f 100644 --- a/actions/setup/js/resolve_mentions_from_payload.cjs +++ b/actions/setup/js/resolve_mentions_from_payload.cjs @@ -9,6 +9,21 @@ const { resolveMentionsLazily, isPayloadUserBot } = require("./resolve_mentions. const { getErrorMessage } = require("./error_helpers.cjs"); const { parseRepoSlug } = require("./repo_helpers.cjs"); +/** + * Use the mention-specific token for allowlist lookups instead of inheriting a + * downstream safe-output write token. + * @param {any} fallback + * @returns {any} + */ +function getMentionsGithubClient(fallback) { + const token = process.env.GH_AW_MENTIONS_GITHUB_TOKEN; + const globalState = /** @type {any} */ global; + if (token && typeof globalState.getOctokit === "function") { + return globalState.getOctokit(token); + } + return fallback; +} + /** * Push a non-bot user's login to the array if present. * @param {string[]} users - Target array @@ -182,6 +197,7 @@ async function resolveAllowedMentionsFromPayload(context, github, core, mentions if (!context || !github || !core) { return []; } + github = getMentionsGithubClient(github); // If mentions is explicitly set to false, return empty array (all mentions escaped) if (mentionsConfig === false || mentionsConfig?.enabled === false) { @@ -298,6 +314,7 @@ async function resolveDefaultMentions(context, github, core, mentionsConfig, def } module.exports = { + getMentionsGithubClient, resolveAllowedMentionsFromPayload, resolveMentionsForItem, resolveDefaultMentions, diff --git a/actions/setup/js/resolve_mentions_from_payload.test.cjs b/actions/setup/js/resolve_mentions_from_payload.test.cjs index cb808376cfb..959434b063d 100644 --- a/actions/setup/js/resolve_mentions_from_payload.test.cjs +++ b/actions/setup/js/resolve_mentions_from_payload.test.cjs @@ -18,7 +18,7 @@ vi.mock("./error_helpers.cjs", () => ({ getErrorMessage: vi.fn(err => (err instanceof Error ? err.message : String(err))), })); -const { resolveAllowedMentionsFromPayload, extractKnownAuthorsFromPayload, fetchTeamMembers, pushNonBotUser, pushNonBotAssignees } = await import("./resolve_mentions_from_payload.cjs"); +const { getMentionsGithubClient, resolveAllowedMentionsFromPayload, extractKnownAuthorsFromPayload, fetchTeamMembers, pushNonBotUser, pushNonBotAssignees } = await import("./resolve_mentions_from_payload.cjs"); /** @returns {{ info: ReturnType, warning: ReturnType, error: ReturnType }} */ function makeMockCore() { @@ -30,6 +30,33 @@ function makeMockGithub() { return {}; } +describe("getMentionsGithubClient", () => { + it("uses the configured mention token instead of the handler client", () => { + const fallback = makeMockGithub(); + const mentionClient = makeMockGithub(); + const originalToken = process.env.GH_AW_MENTIONS_GITHUB_TOKEN; + const originalGetOctokit = global.getOctokit; + process.env.GH_AW_MENTIONS_GITHUB_TOKEN = "mention-token"; + global.getOctokit = vi.fn(token => (token === "mention-token" ? mentionClient : fallback)); + + try { + expect(getMentionsGithubClient(fallback)).toBe(mentionClient); + expect(global.getOctokit).toHaveBeenCalledWith("mention-token"); + } finally { + if (originalToken === undefined) { + delete process.env.GH_AW_MENTIONS_GITHUB_TOKEN; + } else { + process.env.GH_AW_MENTIONS_GITHUB_TOKEN = originalToken; + } + if (originalGetOctokit === undefined) { + delete global.getOctokit; + } else { + global.getOctokit = originalGetOctokit; + } + } + }); +}); + describe("pushNonBotUser", () => { it("pushes a regular user login", () => { const users = /** @type {string[]} */ []; diff --git a/actions/setup/js/safe_output_handler_manager.cjs b/actions/setup/js/safe_output_handler_manager.cjs index 622902ec9b5..1106e5f09d0 100644 --- a/actions/setup/js/safe_output_handler_manager.cjs +++ b/actions/setup/js/safe_output_handler_manager.cjs @@ -411,8 +411,8 @@ async function loadHandlers(config, prReviewBufferRegistry, resolvedAllowedMenti } // Pass the mentions policy to handlers; aliases are resolved for each destination. - if (handlerConfig.mentions == null && config.mentions != null) { - handlerConfig.mentions = config.mentions; + if (MENTION_HANDLER_TYPES.has(type) && handlerConfig.mentions == null) { + handlerConfig.mentions = config.mentions ?? {}; } // Inject shared PR review buffer registry into handlers that need it if (PR_REVIEW_HANDLER_TYPES.has(type)) { @@ -426,7 +426,7 @@ async function loadHandlers(config, prReviewBufferRegistry, resolvedAllowedMenti if (handlerConfig[GITHUB_TOKEN_CONFIG_KEY] && typeof globalState.getOctokit === "function") { handlerGithubClient = globalState.getOctokit(handlerConfig[GITHUB_TOKEN_CONFIG_KEY]); } - if (MENTION_HANDLER_TYPES.has(type) && handlerConfig.mentions != null && handlerConfig.allowedMentionAliases == null) { + if (MENTION_HANDLER_TYPES.has(type) && handlerConfig.allowedMentionAliases == null) { if (Array.isArray(resolvedAllowedMentionAliases)) { handlerConfig.allowedMentionAliases = resolvedAllowedMentionAliases; } else { diff --git a/actions/setup/js/safe_output_type_validator.cjs b/actions/setup/js/safe_output_type_validator.cjs index 5e629f2283f..f41a89fb855 100644 --- a/actions/setup/js/safe_output_type_validator.cjs +++ b/actions/setup/js/safe_output_type_validator.cjs @@ -34,6 +34,7 @@ const ISSUE_INTENT_RATIONALE_MAX_LENGTH = 280; * maxMentions?: number, * allowedAliasesSeen?: Set, * maxBotMentions?: number, + * deferMentions?: boolean, * normalizeIssueClosingKeywords?: boolean, * dataEnabled?: boolean, * dataSchema?: any @@ -91,6 +92,7 @@ function normalizeIssueIntentRationale(rationale, options) { maxMentions: options?.maxMentions, allowedAliasesSeen: options?.allowedAliasesSeen, maxBotMentions: options?.maxBotMentions, + deferMentions: options?.deferMentions, }).trim(); // sanitizeContent appends "\n[Content truncated due to length]" when it truncates, // so clamp again to guarantee the GitHub API hard limit. @@ -119,6 +121,7 @@ function validateIssueIntentLabels(value, lineNum, itemType, fieldName, options) maxMentions: options?.maxMentions, allowedAliasesSeen: options?.allowedAliasesSeen, maxBotMentions: options?.maxBotMentions, + deferMentions: options?.deferMentions, }); if (!name) { return { isValid: false, error: `Line ${lineNum}: ${itemType} ${fieldName}[${i}] must be a non-empty string` }; @@ -154,6 +157,7 @@ function validateIssueIntentLabels(value, lineNum, itemType, fieldName, options) maxMentions: options?.maxMentions, allowedAliasesSeen: options?.allowedAliasesSeen, maxBotMentions: options?.maxBotMentions, + deferMentions: options?.deferMentions, }); if (!name) { return { @@ -560,6 +564,7 @@ function validateField(value, fieldName, validation, itemType, lineNum, options) maxMentions: options?.maxMentions, allowedAliasesSeen: options?.allowedAliasesSeen, maxBotMentions: options?.maxBotMentions, + deferMentions: options?.deferMentions, }); } return { isValid: true, normalizedValue: normalizedResult }; @@ -584,6 +589,7 @@ function validateField(value, fieldName, validation, itemType, lineNum, options) maxMentions: options?.maxMentions, allowedAliasesSeen: options?.allowedAliasesSeen, maxBotMentions: options?.maxBotMentions, + deferMentions: options?.deferMentions, }); } if (options?.normalizeIssueClosingKeywords && fieldName === "body" && NORMALIZE_CLOSER_BODY_TYPES.has(itemType)) { @@ -664,6 +670,7 @@ function validateField(value, fieldName, validation, itemType, lineNum, options) maxMentions: options?.maxMentions, allowedAliasesSeen: options?.allowedAliasesSeen, maxBotMentions: options?.maxBotMentions, + deferMentions: options?.deferMentions, }) : item ); diff --git a/actions/setup/js/sanitize_content.cjs b/actions/setup/js/sanitize_content.cjs index ddf832a66f6..727a21c5ddf 100644 --- a/actions/setup/js/sanitize_content.cjs +++ b/actions/setup/js/sanitize_content.cjs @@ -44,6 +44,7 @@ const RUNTIME_TO_MENTION_ALIAS_MAP = { * @property {number} [maxMentions] - Maximum number of unique allowed aliases to preserve * @property {Set} [allowedAliasesSeen] - Allowed aliases already preserved in this output item * @property {number} [maxBotMentions] - Maximum bot trigger references before filtering (default: 10) + * @property {boolean} [deferMentions] - Preserve mentions for filtering in the trusted safe-outputs job */ /** @@ -62,6 +63,7 @@ function sanitizeContent(content, maxLengthOrOptions) { let maxMentions; /** @type {number | undefined} */ let maxBotMentions; + let deferMentions = false; /** @type {Set | undefined} */ let allowedAliasesSeen; @@ -74,11 +76,12 @@ function sanitizeContent(content, maxLengthOrOptions) { allowedAliasesLowercase = expandAllowedAliases(normalizedAllowedAliases); maxMentions = maxLengthOrOptions.maxMentions; maxBotMentions = maxLengthOrOptions.maxBotMentions; + deferMentions = maxLengthOrOptions.deferMentions === true; allowedAliasesSeen = maxLengthOrOptions.allowedAliasesSeen; } // If no allowed aliases specified, use core sanitization (which neutralizes all mentions) - if (allowedAliasesLowercase.length === 0) { + if (allowedAliasesLowercase.length === 0 && !deferMentions) { return sanitizeContentCore(content, maxLength, maxBotMentions); } @@ -132,7 +135,9 @@ function sanitizeContent(content, maxLengthOrOptions) { // Neutralize mentions after truncation so the length boundary cannot split an // inserted code-span delimiter and reactivate a mention. - sanitized = neutralizeMentions(sanitized, allowedAliasesLowercase, maxMentions, allowedAliasesSeen); + if (!deferMentions) { + sanitized = neutralizeMentions(sanitized, allowedAliasesLowercase, maxMentions, allowedAliasesSeen); + } // Neutralize GitHub references if restrictions are configured sanitized = neutralizeGitHubReferences(sanitized, allowedGitHubRefs); diff --git a/actions/setup/js/sanitize_content.test.cjs b/actions/setup/js/sanitize_content.test.cjs index 8f9100432ec..67fea4f985e 100644 --- a/actions/setup/js/sanitize_content.test.cjs +++ b/actions/setup/js/sanitize_content.test.cjs @@ -314,6 +314,11 @@ describe("sanitize_content.cjs", () => { expect(result).toBe("Hello `@user`"); }); + it("should defer mention neutralization when requested", () => { + const result = sanitizeContent("Hello @user", { deferMentions: true }); + expect(result).toBe("Hello @user"); + }); + it("should not neutralize org/team mentions in allowedAliases", () => { const result = sanitizeContent("Hello @myorg/myteam", { allowedAliases: ["myorg/myteam"] }); expect(result).toBe("Hello @myorg/myteam"); diff --git a/docs/adr/66571-dedicated-mention-ingestion-credentials.md b/docs/adr/66571-dedicated-mention-ingestion-credentials.md new file mode 100644 index 00000000000..e7251125396 --- /dev/null +++ b/docs/adr/66571-dedicated-mention-ingestion-credentials.md @@ -0,0 +1,46 @@ +# ADR-66571: Resolve Mention Allowlists in the Trusted Safe-Output Job + +**Date**: 2026-11-19 +**Status**: Draft +**Deciders**: pelikhan [TODO: verify full decider list] + +--- + +### Context + +`safe-outputs.mentions.allowed-teams` lets a workflow preserve `@login` mentions for members of specific teams, but **Ingest agent output** runs in the untrusted `agent` job. Supplying a lookup token there would expose it to agent-controlled files and scripts, even if the token were read-only. The trusted `safe_outputs` job already re-sanitizes mention-aware content before publication, so mention candidates can remain intact through ingestion and be checked after the agent artifact is transferred (#50282). + +### Decision + +Mention/team allowlist resolution will be deferred to the trusted `safe_outputs` job. Ingestion will preserve mention candidates without performing directory, collaborator, or comment-author lookups and will not receive mention-specific credentials. The processor will use `safe-outputs.mentions.github-app`, then `safe-outputs.mentions.github-token`, then `github.token`; these credentials never inherit from global or per-handler safe-output credentials. A dedicated app token is minted in `safe_outputs` before **Process Safe Outputs**, with `issues: read` when `add-comment` is enabled and `members: read` when `allowed-teams` is configured. Permission overrides are restricted to `read` and `none`. + +### Alternatives Considered + +#### Alternative 1: Let ingestion inherit the existing safe-output write credentials + +Reuse `safe-outputs.github-token` / `safe-outputs.github-app` (or `GH_AW_GITHUB_TOKEN`) for the membership lookup. This was the smallest change and required no new schema surface. It was rejected because it would place write-scoped safe-output credentials inside the `agent` job, violating the formal security property that agents execute without GitHub write permissions and that write credentials stay confined to downstream safe-output jobs (`security_architecture_formal_test.go`). + +#### Alternative 2: Reuse the default Actions token for mention lookups in the agent job + +Keep ingestion token-less but perform directory lookups with the job's default Actions token. Rejected because the default token cannot access organization team membership, and the lookup would still execute in the untrusted job boundary. + +### Consequences + +#### Positive +- Mentions of allowed team members survive ingestion, so downstream handlers can notify those users — the behaviour #50282 asks for. +- Membership read access is granted with a narrowly scoped, separately configured credential; the agent job gains no write capability and downstream write credentials are unchanged. +- Mention credentials and mention lookups stay out of the agent job; the trusted handler sanitizes mention-aware content before publication. + +#### Negative +- Adds a third credential concept (`mentions.github-token` / `mentions.github-app`) with its own precedence and fallback chain (app → PAT → `GITHUB_TOKEN`), increasing the configuration surface users must understand and the compiler must keep consistent. +- Users who expect credentials to cascade from `safe-outputs.github-token` will see mentions silently escaped until they configure the mention-specific token; the non-inheritance rule is deliberate but is a latent surprise. +- Deferring mention filtering means mention-aware messages cross the artifact boundary before final mention sanitization; the safe-output handler must keep its sanitization step before every write. + +#### Neutral +- `MentionsConfig` gains `GitHubToken` and `GitHubApp`; the workflow JSON schema, generated frontmatter reference, safe-outputs reference, and the normative safe-outputs specification were updated in the same change. +- Installation owner/repository selection, wildcard scoping, relay repository selection, and missing-key guards reuse existing compiler helpers in the trusted job. +- Same-job token references for mention credentials are validated against the `safe_outputs` job. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/docs/src/content/docs/reference/frontmatter-full.md b/docs/src/content/docs/reference/frontmatter-full.md index abd33e732ef..02d3002b5b6 100644 --- a/docs/src/content/docs/reference/frontmatter-full.md +++ b/docs/src/content/docs/reference/frontmatter-full.md @@ -22160,6 +22160,211 @@ safe-outputs: # Format 2: Advanced configuration for @mention filtering with fine-grained # control mentions: + # Dedicated token for resolving mention allowlists in the trusted safe_outputs + # job. It is not exposed to agent-job ingestion and does not inherit other + # safe-output credentials or affect write operations. + # (optional) + github-token: "${{ secrets.GITHUB_TOKEN }}" + + # Dedicated GitHub App for resolving mention allowlists in the trusted + # safe_outputs job. Its token is read-only, takes precedence over + # mentions.github-token, and does not inherit safe-outputs.github-app. + # (optional) + github-app: + # Deprecated alias for client-id. GitHub App ID/client ID (e.g., '${{ vars.APP_ID + # }}'). + # (optional) + app-id: "example-value" + + # GitHub App client ID (e.g., '${{ vars.APP_ID }}'). Required to mint a GitHub App + # token. + # (optional) + client-id: "example-value" + + # GitHub App private key (e.g., '${{ secrets.APP_PRIVATE_KEY }}'). Required to + # mint a GitHub App token. + # (optional) + private-key: "example-value" + + # If true, skip token minting when client-id/private-key resolve to empty strings + # at runtime. Defaults to false. + # (optional) + ignore-if-missing: true + + # Optional owner of the GitHub App installation (defaults to current repository + # owner if not specified) + # (optional) + owner: "example-value" + + # Optional list of repositories to grant access to (defaults to current repository + # if not specified) + # (optional) + repositories: [] + # Array of strings + + # Optional extra GitHub App-only permissions to merge into the minted token. Takes + # effect for tools.github.github-app and safe-outputs.github-app; ignored in + # on.github-app and the top-level github-app fallback. Use to add GitHub App-only + # scopes (e.g. members, organization-administration) not expressible via standard + # handler declarations. + # (optional) + permissions: + # Permission level for repository administration (read/none; "write" is rejected + # by the compiler). GitHub App-only permission for repository administration. + # (optional) + administration: "read" + + # Permission level for Codespaces (read/none; "write" is rejected by the + # compiler). GitHub App-only permission. + # (optional) + codespaces: "read" + + # Permission level for Codespaces lifecycle administration (read/none; "write" is + # rejected by the compiler). GitHub App-only permission. + # (optional) + codespaces-lifecycle-admin: "read" + + # Permission level for Codespaces metadata (read/none; "write" is rejected by the + # compiler). GitHub App-only permission. + # (optional) + codespaces-metadata: "read" + + # Permission level for user email addresses (read/none; "write" is rejected by the + # compiler). GitHub App-only permission. + # (optional) + email-addresses: "read" + + # Permission level for repository environments (read/none; "write" is rejected by + # the compiler). GitHub App-only permission. + # (optional) + environments: "read" + + # Permission level for git signing (read/none; "write" is rejected by the + # compiler). GitHub App-only permission. + # (optional) + git-signing: "read" + + # Permission level for organization members (read/none; "write" is rejected by the + # compiler). Required for org team membership API calls. + # (optional) + members: "read" + + # Permission level for organization administration (read/none; "write" is rejected + # by the compiler). GitHub App-only permission. + # (optional) + organization-administration: "read" + + # Permission level for organization announcement banners (read/none; "write" is + # rejected by the compiler). GitHub App-only permission. + # (optional) + organization-announcement-banners: "read" + + # Permission level for organization Codespaces (read/none; "write" is rejected by + # the compiler). GitHub App-only permission. + # (optional) + organization-codespaces: "read" + + # Permission level for organization Copilot (read/none; "write" is rejected by the + # compiler). GitHub App-only permission. + # (optional) + organization-copilot: "read" + + # Permission level for organization custom org roles (read/none; "write" is + # rejected by the compiler). GitHub App-only permission. + # (optional) + organization-custom-org-roles: "read" + + # Permission level for organization custom properties (read/none; "write" is + # rejected by the compiler). GitHub App-only permission. + # (optional) + organization-custom-properties: "read" + + # Permission level for organization custom repository roles (read/none; "write" is + # rejected by the compiler). GitHub App-only permission. + # (optional) + organization-custom-repository-roles: "read" + + # Permission level for organization events (read/none; "write" is rejected by the + # compiler). GitHub App-only permission. + # (optional) + organization-events: "read" + + # Permission level for organization webhooks (read/none; "write" is rejected by + # the compiler). GitHub App-only permission. + # (optional) + organization-hooks: "read" + + # Permission level for organization members management (read/none; "write" is + # rejected by the compiler). GitHub App-only permission. + # (optional) + organization-members: "read" + + # Permission level for organization packages (read/none; "write" is rejected by + # the compiler). GitHub App-only permission. + # (optional) + organization-packages: "read" + + # Permission level for organization personal access token requests (read/none; + # "write" is rejected by the compiler). GitHub App-only permission. + # (optional) + organization-personal-access-token-requests: "read" + + # Permission level for organization personal access tokens (read/none; "write" is + # rejected by the compiler). GitHub App-only permission. + # (optional) + organization-personal-access-tokens: "read" + + # Permission level for organization plan (read/none; "write" is rejected by the + # compiler). GitHub App-only permission. + # (optional) + organization-plan: "read" + + # Permission level for organization self-hosted runners (read/none; "write" is + # rejected by the compiler). GitHub App-only permission. + # (optional) + organization-self-hosted-runners: "read" + + # Permission level for organization user blocking (read/none; "write" is rejected + # by the compiler). GitHub App-only permission. + # (optional) + organization-user-blocking: "read" + + # Permission level for repository custom properties (read/none; "write" is + # rejected by the compiler). GitHub App-only permission. + # (optional) + repository-custom-properties: "read" + + # Permission level for secret scanning alerts (read/none). Forwarded as + # permission-secret-scanning-alerts input for actions/create-github-app-token. + # (optional) + secret-scanning-alerts: "read" + + # Permission level for repository webhooks (read/none; "write" is rejected by the + # compiler). GitHub App-only permission. + # (optional) + repository-hooks: "read" + + # Permission level for single file access (read/none; "write" is rejected by the + # compiler). GitHub App-only permission. + # (optional) + single-file: "read" + + # Permission level for team discussions (read/none; "write" is rejected by the + # compiler). GitHub App-only permission. + # (optional) + team-discussions: "read" + + # Permission level for Dependabot vulnerability alerts (read/none; "write" is + # rejected by the compiler). Also available as a GITHUB_TOKEN scope. When used + # with a GitHub App, forwarded as permission-vulnerability-alerts input. + # (optional) + vulnerability-alerts: "read" + + # Permission level for GitHub Actions workflow files (read/none; "write" is + # rejected by the compiler). GitHub App-only permission. + # (optional) + workflows: "read" + # Allow mentions of repository collaborators (users with repository access, # excluding bots). Default: true # (optional) @@ -22178,10 +22383,11 @@ safe-outputs: # List of team slugs whose members are always allowed to be mentioned. Accepts # 'team-slug' (resolved against the current org) or 'org/team-slug' format. Team - # members are fetched from the GitHub API at runtime; bots are excluded. - # IMPORTANT: requires read:org scope — not available with the default - # GITHUB_TOKEN. Use a classic PAT with read:org, a fine-grained PAT with - # Members:Read, or a GitHub App with the Members:Read permission. Without the + # members are resolved in the trusted safe_outputs job; bots are excluded. + # Requires the mention-resolution token to have read:org scope, which the default + # github.token does not provide. Configure safe-outputs.mentions.github-token with + # a classic PAT or fine-grained PAT with Members:Read, or + # safe-outputs.mentions.github-app with the Members:Read permission. Without the # required scope, team lookups fail with a warning and those members are skipped. # (optional) allowed-teams: [] diff --git a/docs/src/content/docs/reference/safe-outputs.md b/docs/src/content/docs/reference/safe-outputs.md index 55599c6bc6b..e23f0bfbfbb 100644 --- a/docs/src/content/docs/reference/safe-outputs.md +++ b/docs/src/content/docs/reference/safe-outputs.md @@ -2097,7 +2097,7 @@ Accepts a literal integer or a GitHub Actions expression string (e.g., `${{ inpu By default, `@mentions` in AI-generated content are escaped with backticks unless the mentioned user is a verified collaborator or inferred from the event context (issue/PR author, assignees, etc.). Use `mentions:` to control this behavior: -Collaborator checks use the repository receiving each safe output. With `target-repo`, the workflow repository's collaborators are not used as a fallback when the token cannot read the target repository. The agent job sanitizes output first, so its token needs read access to the target repository to preserve collaborator mentions; a later handler token cannot restore escaped mentions. +Collaborator checks use the repository receiving each safe output. With `target-repo`, the workflow repository's collaborators are not used as a fallback when the token cannot read the target repository. The agent job preserves mention candidates without performing lookups; the trusted `safe_outputs` job filters them before publication. ```yaml wrap safe-outputs: @@ -2123,8 +2123,19 @@ safe-outputs: **`allowed-teams`** lets organizations allow all members of specific GitHub teams to be mentioned without listing individual usernames. Team members are fetched from the GitHub API at runtime using `GET /orgs/{org}/teams/{team_slug}/members`. Bot accounts within the team are excluded. Use `org/team-slug` for cross-org teams or just `team-slug` to resolve against the current repository's organization. +Configure `safe-outputs.mentions.github-token` or `safe-outputs.mentions.github-app` with read access to team membership. Mention and team lookups run in the trusted `safe_outputs` job, after the agent artifact is transferred. The token selection is mention app, mention token, then `github.token`; it never inherits `safe-outputs.github-token`, `safe-outputs.github-app`, per-output credentials, or `GH_AW_GITHUB_TOKEN`. + +```yaml wrap +safe-outputs: + mentions: + github-token: ${{ secrets.MENTIONS_READ_PAT }} + allowed-teams: [my-org/my-team] +``` + +Alternatively, configure `safe-outputs.mentions.github-app` with the standard `client-id`, `private-key`, `owner`, `repositories`, and `permissions` fields. Its token is minted in the trusted `safe_outputs` job before **Process Safe Outputs**, takes precedence over `mentions.github-token`, and requests `issues: read` when `add-comment` is enabled and `members: read` when `allowed-teams` is configured. Permission overrides may be `read` or `none`; `write` and other values are rejected. With `ignore-if-missing: true`, missing app credentials fall back to `mentions.github-token`, then `github.token`. Omitting the mention-specific app does not disable mention filtering. Downstream write credentials remain unchanged. + > [!IMPORTANT] -> `allowed-teams` requires the workflow token to have `read:org` scope. The default `GITHUB_TOKEN` does **not** include this scope. Use one of the following: +> `allowed-teams` requires the mention-resolution token to have `read:org` scope. The default `github.token` does **not** include this scope. Use one of the following: > - A **classic PAT** with the `read:org` scope stored as a repository secret > - A **fine-grained PAT** with the "Members" repository permission (read) > - A **GitHub App** installation token with the "Members" permission (read) diff --git a/docs/src/content/docs/specs/safe-outputs-specification.md b/docs/src/content/docs/specs/safe-outputs-specification.md index 414174dcc65..b84e5e87aba 100644 --- a/docs/src/content/docs/specs/safe-outputs-specification.md +++ b/docs/src/content/docs/specs/safe-outputs-specification.md @@ -319,7 +319,7 @@ The Safe Outputs MCP Gateway implements defense-in-depth through strict architec **Requirement AR1: Agent Isolation** -Agents MUST execute without GitHub write permissions. Only read-level tokens SHALL be accessible to agent processes. Write-capable tokens MUST reside exclusively in safe output job contexts. +Agents MUST execute without GitHub write permissions. Only read-level tokens SHALL be accessible to agent processes. Safe-output write credentials MUST remain confined to downstream safe-output processing jobs. Mention/team allowlist resolution MUST be deferred to the trusted `safe_outputs` job; ingestion in the `agent` job MUST perform no privileged mention or team lookups and MUST NOT receive mention-specific credentials. **Verification**: @@ -396,14 +396,14 @@ Safe Output Processors MAY target external APIs. Credentials for each external s - **Method**: Manual security audit and code review - **Tool**: Security review of workflow structure and GitHub Actions architecture -- **Criteria**: No GITHUB_TOKEN or credentials are accessible from agent job context; tokens only exist in safe output job contexts +- **Criteria**: Safe-output and mention-resolution credentials are inaccessible to agent processes; mention resolution occurs only in trusted safe-output processing after the agent artifact has been transferred - **Manual Check**: Audit all communication channels (artifacts, environment variables, network, filesystem) to confirm no credential leakage **Formal Definition**: ``` ∀ t ∈ [agent_start, agent_end]: - accessible_credentials(agent_context, t) ∩ safe_output_credentials = ∅ + accessible_credentials(agent_context, t) ∩ (safe_output_credentials ∪ mention_resolution_credentials) = ∅ ``` **Requirement AR5: Steering Issue Provenance** @@ -2129,11 +2129,23 @@ Requirements: Requirements: -- Detect @mentions: `@[a-zA-Z0-9_-]+` -- Check against allowed-aliases list -- Neutralize unauthorized: `@user` becomes `@ user` (add space) +- Ingestion MUST preserve mentions for mention-aware output types without performing allowlist lookups. +- The trusted safe-output processor MUST detect @mentions: `@[a-zA-Z0-9_-]+` +- Check each mention against the resolved allowed-aliases list +- Neutralize unauthorized mentions before publication - Preserve mentions in code blocks +**Ingestion credential requirements**: + +- Ingestion MUST use only the default GitHub Actions context and MUST NOT resolve collaborators, comment authors, or team membership. It MUST preserve mention candidates for the trusted safe-output processor to filter before any write operation. +- Mention resolution MUST run in the trusted `safe_outputs` job. Its lookup token MUST come only from `safe-outputs.mentions.github-app`, then `safe-outputs.mentions.github-token`, then `github.token`; it MUST NOT inherit `safe-outputs.github-token`, `safe-outputs.github-app`, per-output credentials, or `GH_AW_GITHUB_TOKEN`. +- When `safe-outputs.mentions.github-app` is configured, the compiler MUST mint its dedicated installation token in `safe_outputs` before **Process Safe Outputs**. The token MUST use the configured installation owner and repository scope, request `issues: read` whenever `add-comment` is enabled, and request `members: read` when `mentions.allowed-teams` is configured. Explicit permission overrides MAY select only `read` or `none`; the compiler MUST reject `write` and other levels. +- With `ignore-if-missing: true`, missing app credentials MUST skip minting and mention resolution MUST fall back to `safe-outputs.mentions.github-token`, then `github.token`. Mention credentials MUST NOT change downstream safe-output write credentials. +- Allowed team members' raw `@login` mentions MUST remain unmodified during untrusted ingestion and MUST be filtered in the trusted safe-output processor before publication. +- A mention token referencing `steps..outputs.*` MUST be produced by an earlier step in the `safe_outputs` job. +- Mention credentials MUST NOT be serialized into agent-visible validation or handler configuration. +- Compiler debug logging SHOULD identify the mention-resolution token source without logging credential values or token expressions. + **Transformation T6: Markdown Safety** Requirements: diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 7ffa91cb82d..f98de608c6e 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -12438,6 +12438,14 @@ "type": "object", "description": "Advanced configuration for @mention filtering with fine-grained control", "properties": { + "github-token": { + "$ref": "#/$defs/github_token_same_job", + "description": "Dedicated token for resolving mention allowlists in the trusted safe_outputs job. It is not exposed to agent-job ingestion and does not inherit other safe-output credentials or affect write operations." + }, + "github-app": { + "$ref": "#/$defs/github_app", + "description": "Dedicated GitHub App for resolving mention allowlists in the trusted safe_outputs job. Its token is read-only, takes precedence over mentions.github-token, and does not inherit safe-outputs.github-app." + }, "allowed-collaborators": { "type": "boolean", "description": "Allow mentions of repository collaborators (users with repository access, excluding bots). Default: true", @@ -12464,7 +12472,7 @@ }, "allowed-teams": { "type": "array", - "description": "List of team slugs whose members are always allowed to be mentioned. Accepts 'team-slug' (resolved against the current org) or 'org/team-slug' format. Team members are fetched from the GitHub API at runtime; bots are excluded. IMPORTANT: requires read:org scope \u2014 not available with the default GITHUB_TOKEN. Use a classic PAT with read:org, a fine-grained PAT with Members:Read, or a GitHub App with the Members:Read permission. Without the required scope, team lookups fail with a warning and those members are skipped.", + "description": "List of team slugs whose members are always allowed to be mentioned. Accepts 'team-slug' (resolved against the current org) or 'org/team-slug' format. Team members are resolved in the trusted safe_outputs job; bots are excluded. Requires the mention-resolution token to have read:org scope, which the default github.token does not provide. Configure safe-outputs.mentions.github-token with a classic PAT or fine-grained PAT with Members:Read, or safe-outputs.mentions.github-app with the Members:Read permission. Without the required scope, team lookups fail with a warning and those members are skipped.", "items": { "type": "string", "minLength": 1 diff --git a/pkg/workflow/agentic_output_test.go b/pkg/workflow/agentic_output_test.go index 03f41474056..40ae749f268 100644 --- a/pkg/workflow/agentic_output_test.go +++ b/pkg/workflow/agentic_output_test.go @@ -12,8 +12,245 @@ import ( "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/testutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +func TestOutputCollectionDoesNotUseMentionCredentials(t *testing.T) { + for _, tt := range []struct { + name string + safeOutputs *SafeOutputsConfig + }{ + {name: "no safe outputs"}, + {name: "default token", safeOutputs: &SafeOutputsConfig{}}, + { + name: "mention token is deferred to trusted job", + safeOutputs: &SafeOutputsConfig{Mentions: &MentionsConfig{GitHubToken: "${{ secrets.MENTIONS_PAT }}"}}, + }, + { + name: "per-handler token is not used", + safeOutputs: &SafeOutputsConfig{ + AddComments: &AddCommentsConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{GitHubToken: "${{ secrets.COMMENT_PAT }}"}}, + }, + }, + { + name: "global token is not used", + safeOutputs: &SafeOutputsConfig{ + GitHubToken: "${{ secrets.MENTIONS_PAT }}", + AddComments: &AddCommentsConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{GitHubToken: "${{ secrets.COMMENT_PAT }}"}}, + }, + }, + { + name: "safe outputs app is not used", + safeOutputs: &SafeOutputsConfig{ + GitHubApp: &GitHubAppConfig{AppID: "${{ vars.APP_ID }}", PrivateKey: "${{ secrets.APP_PRIVATE_KEY }}"}, + }, + }, + } { + t.Run(tt.name, func(t *testing.T) { + var yaml strings.Builder + require.NoError(t, NewCompiler().generateOutputCollectionStep(&yaml, &WorkflowData{SafeOutputs: tt.safeOutputs})) + assert.NotContains(t, yaml.String(), "github-token:") + assert.NotContains(t, yaml.String(), "MENTIONS_PAT") + assert.NotContains(t, yaml.String(), "safe-outputs-ingestion-app-token") + assert.Contains(t, yaml.String(), "setupGlobals(core, github, context, exec, io, getOctokit);") + }) + } +} + +func TestOutputCollectionGitHubAppToken(t *testing.T) { + for _, tt := range []struct { + name string + ignore bool + pat string + token string + }{ + {name: "app overrides PAT", pat: "${{ secrets.PAT }}", token: "${{ steps.safe-outputs-mentions-app-token.outputs.token }}"}, + {name: "missing credentials fall back to PAT", ignore: true, pat: "${{ secrets.PAT }}", token: "${{ steps.safe-outputs-mentions-app-token.outputs.token || secrets.PAT }}"}, + {name: "missing credentials fall back to default Actions token", ignore: true, token: "${{ steps.safe-outputs-mentions-app-token.outputs.token || github.token }}"}, + } { + t.Run(tt.name, func(t *testing.T) { + app := &GitHubAppConfig{ + AppID: "${{ vars.APP_ID }}", PrivateKey: "${{ secrets.APP_KEY }}", + Owner: "my-org", Repositories: []string{"my-repo", "another-repo"}, IgnoreIfMissing: tt.ignore, + } + data := &WorkflowData{SafeOutputs: &SafeOutputsConfig{ + GitHubToken: "${{ secrets.WRITE_PAT }}", AddComments: &AddCommentsConfig{}, + Mentions: &MentionsConfig{GitHubApp: app, GitHubToken: tt.pat, AllowedTeams: []string{"my-org/my-team"}}, + }} + compiler := NewCompiler() + output := strings.Join(compiler.addAppTokenMintingSteps(data), "") + assert.Contains(t, output, "owner: my-org\n") + assert.Contains(t, output, "repositories: |-\n my-repo\n another-repo\n") + assert.NotContains(t, output, "permission-contents:") + assert.Contains(t, output, "permission-issues: read\n") + assert.Contains(t, output, "permission-pull-requests: read\n") + assert.Contains(t, output, "permission-members: read\n") + assert.NotContains(t, output, ": write\n") + assert.NotContains(t, output, "steps.safe-outputs-app-token.outputs.token") + assert.NotContains(t, output, "secrets.WRITE_PAT") + assert.Contains(t, output, "id: safe-outputs-mentions-app-token\n") + if tt.ignore { + assert.Contains(t, output, "if: ${{ vars.APP_ID != '' && env.GH_AW_IGNORE_IF_MISSING_PRIVATE_KEY != '' }}") + assert.Contains(t, output, "GH_AW_IGNORE_IF_MISSING_PRIVATE_KEY: ${{ secrets.APP_KEY }}") + assert.NotContains(t, output, "if: ${{ secrets.") + } else { + assert.Contains(t, output, "- name: Generate GitHub App token for mention resolution\n") + } + assert.Equal(t, tt.token, safeOutputMentionsGitHubToken(&WorkflowData{SafeOutputs: data.SafeOutputs})) + assert.Nil(t, app.Permissions, "minting must not mutate the configured app") + }) + } +} + +func TestOutputCollectionGitHubAppOwnerAndWildcard(t *testing.T) { + for _, wildcard := range []bool{false, true} { + t.Run(map[bool]string{false: "repository-scoped", true: "installation-wide"}[wildcard], func(t *testing.T) { + app := &GitHubAppConfig{AppID: "${{ vars.APP_ID }}", PrivateKey: "${{ secrets.APP_KEY }}"} + if wildcard { + app.Repositories = []string{"*"} + } + data := &WorkflowData{ + On: "on:\n workflow_call:\n", + SafeOutputs: &SafeOutputsConfig{Mentions: &MentionsConfig{GitHubApp: app}}, + } + compiler := NewCompiler() + output := strings.Join(compiler.addAppTokenMintingSteps(data), "") + assert.Contains(t, output, "- name: Derive GitHub App owner for mention resolution\n") + assert.Contains(t, output, "GH_AW_TARGET_REPOSITORY: ${{ needs.activation.outputs.target_repo }}") + assert.Contains(t, output, "owner: ${{ steps.safe-outputs-mentions-app-token-owner.outputs.owner }}") + assert.NotContains(t, output, "permission-members:") + if wildcard { + assert.NotContains(t, output, "repositories:") + assert.True(t, compiler.wildcardAppTokenSteps[appTokenStepKey{ + jobName: "safe_outputs", id: "safe-outputs-mentions-app-token", clientID: app.AppID, privateKey: app.PrivateKey, + }]) + } else { + assert.Contains(t, output, "repositories: ${{ needs.activation.outputs.target_repo_name }}") + } + }) + } +} + +func TestOutputCollectionMentionCredentialsIndependent(t *testing.T) { + for _, tt := range []struct { + name string + pat string + }{ + {name: "default token"}, + {name: "dedicated mention PAT", pat: "${{ secrets.MENTIONS_PAT }}"}, + } { + t.Run(tt.name, func(t *testing.T) { + content := `--- +on: workflow_dispatch +engine: claude +safe-outputs: + github-token: ${{ secrets.WRITE_PAT }} + github-app: + client-id: ${{ vars.APP_ID }} + private-key: ${{ secrets.APP_KEY }} + mentions: + allowed-teams: [my-org/my-team] +` + if tt.pat != "" { + content += " github-token: " + tt.pat + "\n" + } + content += " add-comment:\n---\n# Independent mention credentials\n" + file := writeStepTokenWorkflow(t, "mention-credentials", content) + compiler := NewCompiler() + data, err := compiler.ParseWorkflowFile(file) + require.NoError(t, err) + require.Equal(t, tt.pat, data.SafeOutputs.Mentions.GitHubToken) + require.NoError(t, compiler.CompileWorkflow(file)) + lock, err := os.ReadFile(strings.TrimSuffix(file, ".md") + ".lock.yml") + require.NoError(t, err) + agent := extractJobSection(string(lock), "agent") + require.Contains(t, agent, "- name: Ingest agent output\n") + ingest := strings.SplitN(strings.SplitN(agent, "- name: Ingest agent output\n", 2)[1], "\n - ", 2)[0] + assert.NotContains(t, agent, "safe-outputs-ingestion-app-token") + assert.NotContains(t, agent, "MENTIONS_PAT") + assert.NotContains(t, ingest, "secrets.WRITE_PAT") + assert.NotContains(t, ingest, "github-token:") + assert.Equal(t, []string{"my-org/my-team"}, data.SafeOutputs.Mentions.AllowedTeams) + assert.Nil(t, data.SafeOutputs.Mentions.Enabled, "mention filtering policy must remain unchanged") + assert.Contains(t, ingest, "collect_ndjson_output.cjs") + safeOutputs := extractJobSection(string(lock), "safe_outputs") + assert.Contains(t, safeOutputs, "id: safe-outputs-app-token\n") + if tt.pat == "" { + assert.Contains(t, safeOutputs, "GH_AW_MENTIONS_GITHUB_TOKEN: ${{ github.token }}") + } else { + assert.Contains(t, safeOutputs, "GH_AW_MENTIONS_GITHUB_TOKEN: "+tt.pat) + } + }) + } +} + +func TestOutputCollectionMentionTokenRejectsLiteral(t *testing.T) { + file := writeStepTokenWorkflow(t, "invalid-mention-token", `--- +on: workflow_dispatch +safe-outputs: + add-comment: + mentions: + github-token: "literal-token" +--- +# Invalid mention credential +`) + _, err := NewCompiler().ParseWorkflowFile(file) + require.Error(t, err) + assert.Contains(t, err.Error(), "github-token") +} + +func TestAgenticOutputCollectionWithGitHubApp(t *testing.T) { + workflowFile := writeStepTokenWorkflow(t, "ingestion-app", `--- +on: workflow_dispatch +engine: claude +strict: false +safe-outputs: + github-token: ${{ secrets.PAT }} + github-app: + client-id: ${{ vars.APP_ID }} + private-key: ${{ secrets.WRITE_APP_KEY }} + owner: my-org + repositories: [my-repo] + permissions: + members: read + mentions: + github-token: ${{ secrets.MENTIONS_PAT }} + github-app: + client-id: ${{ vars.MENTIONS_APP_ID }} + private-key: ${{ secrets.MENTIONS_APP_KEY }} + owner: my-org + repositories: [my-repo] + permissions: + members: read + allowed-teams: [my-org/my-team] + add-comment: +--- +# Ingestion app token +`) + require.NoError(t, NewCompiler().CompileWorkflow(workflowFile)) + content, err := os.ReadFile(strings.TrimSuffix(workflowFile, ".md") + ".lock.yml") + require.NoError(t, err) + agent := extractJobSection(string(content), "agent") + assert.NotContains(t, agent, "safe-outputs-ingestion-app-token") + assert.NotContains(t, agent, "MENTIONS_APP_KEY") + assert.NotContains(t, agent, "steps.safe-outputs-app-token.outputs.token") + assert.NotContains(t, agent, "secrets.WRITE_APP_KEY") + assert.NotContains(t, agent, "private-key: ${{ secrets.MENTIONS_APP_KEY }}") + safeOutputs := extractJobSection(string(content), "safe_outputs") + assert.Contains(t, safeOutputs, "id: safe-outputs-mentions-app-token\n") + assert.Contains(t, safeOutputs, "GH_AW_MENTIONS_GITHUB_TOKEN: ${{ steps.safe-outputs-mentions-app-token.outputs.token }}") + assert.Contains(t, safeOutputs, "private-key: ${{ secrets.MENTIONS_APP_KEY }}") + assert.Contains(t, safeOutputs, "permission-issues: read\n") + assert.Contains(t, safeOutputs, "permission-pull-requests: read\n") + mintStart := strings.Index(safeOutputs, " - name: Generate GitHub App token for mention resolution\n") + require.GreaterOrEqual(t, mintStart, 0) + require.Contains(t, safeOutputs, "- name: Process Safe Outputs\n") + assert.Less(t, mintStart, strings.Index(safeOutputs, "- name: Process Safe Outputs\n")) + assert.Contains(t, safeOutputs, "github-token: ${{ steps.safe-outputs-app-token.outputs.token }}") + assert.NotContains(t, safeOutputs, "steps.safe-outputs-ingestion-app-token.outputs.token") +} + func TestAgenticOutputCollection(t *testing.T) { // Create temporary directory for test files tmpDir := testutil.TempDir(t, "agentic-output-test") @@ -31,6 +268,10 @@ tools: engine: claude strict: false safe-outputs: + github-token: ${{ secrets.WRITE_PAT }} + mentions: + github-token: ${{ secrets.MENTIONS_PAT }} + allowed-teams: [my-org/my-team] add-labels: allowed: ["bug", "enhancement"] --- @@ -86,9 +327,11 @@ This workflow tests the agentic output collection functionality. t.Error("runner.tool_cache must not be interpolated directly in the shell script") } - if !strings.Contains(lockContent, "- name: Ingest agent output") { - t.Error("Expected 'Ingest agent output' step to be in generated workflow") - } + require.Contains(t, lockContent, "- name: Ingest agent output\n") + ingestStep := strings.SplitN(strings.SplitN(lockContent, "- name: Ingest agent output\n", 2)[1], "\n - ", 2)[0] + assert.NotContains(t, ingestStep, "github-token:") + assert.NotContains(t, ingestStep, "MENTIONS_PAT") + assert.Contains(t, extractJobSection(lockContent, "safe_outputs"), "GH_AW_MENTIONS_GITHUB_TOKEN: ${{ secrets.MENTIONS_PAT }}") // Upload Safe Outputs and Upload sanitized agent output are now merged into the // unified 'agent' artifact — individual upload steps no longer exist. diff --git a/pkg/workflow/compiler_safe_outputs_steps.go b/pkg/workflow/compiler_safe_outputs_steps.go index 994fe847f3b..ba2a69d761c 100644 --- a/pkg/workflow/compiler_safe_outputs_steps.go +++ b/pkg/workflow/compiler_safe_outputs_steps.go @@ -233,6 +233,22 @@ func (c *Compiler) addAppTokenMintingSteps(data *WorkflowData) []string { } } + if mentions := data.SafeOutputs.Mentions; mentions != nil && mentions.GitHubApp != nil { + fallbackRepo := "" + if hasWorkflowCallTrigger(data.On) { + fallbackRepo = "${{ needs.activation.outputs.target_repo_name }}" + } + steps = append(steps, c.buildGitHubAppTokenMintStepForJob( + "safe_outputs", + mentions.GitHubApp, + buildMentionResolutionPermissions(data.SafeOutputs), + fallbackRepo, + inferSingleCheckoutRepositoryForGitHubAppOwner(data), + "Generate GitHub App token for mention resolution", + "safe-outputs-mentions-app-token", + )...) + } + return steps } @@ -368,6 +384,7 @@ func buildCustomScriptFilesStep(scripts map[string]*SafeScriptConfig) ([]string, func (c *Compiler) addSafeOutputCoreEnvVars(steps *[]string, data *WorkflowData) error { *steps = append(*steps, " GH_AW_AGENT_OUTPUT: ${{ steps.setup-agent-output-env.outputs.GH_AW_AGENT_OUTPUT }}\n") *steps = append(*steps, " GH_AW_COMMENT_ID: ${{ needs.activation.outputs.comment_id }}\n") + *steps = append(*steps, " GH_AW_MENTIONS_GITHUB_TOKEN: "+safeOutputMentionsGitHubToken(data)+"\n") // Add allowed domains configuration for URL sanitization in safe output handlers. // Without this, sanitizeContent() in safe_output_handler_manager.cjs only allows @@ -430,6 +447,29 @@ func (c *Compiler) addSafeOutputCoreEnvVars(steps *[]string, data *WorkflowData) return nil } +func safeOutputMentionsGitHubToken(data *WorkflowData) string { + if data == nil || data.SafeOutputs == nil || data.SafeOutputs.Mentions == nil { + return "${{ github.token }}" + } + mentions := data.SafeOutputs.Mentions + if mentions.GitHubApp == nil { + if mentions.GitHubToken != "" { + return mentions.GitHubToken + } + return "${{ github.token }}" + } + + token := "${{ steps.safe-outputs-mentions-app-token.outputs.token }}" + if !mentions.GitHubApp.shouldIgnoreMissingKey() { + return token + } + fallback := mentions.GitHubToken + if fallback == "" { + fallback = "${{ github.token }}" + } + return combineTokenExpressions(token, fallback) +} + // addCITriggerTokenEnvVar appends the GH_AW_CI_TRIGGER_TOKEN env var used to push an // empty commit after code changes to trigger CI events, working around the GITHUB_TOKEN // limitation where events don't trigger other workflows. The env var is only emitted diff --git a/pkg/workflow/github_app_permissions_validation.go b/pkg/workflow/github_app_permissions_validation.go index a288f3e3e59..dc8684f10c5 100644 --- a/pkg/workflow/github_app_permissions_validation.go +++ b/pkg/workflow/github_app_permissions_validation.go @@ -175,6 +175,31 @@ func validateGitHubMCPAppPermissionsNoWrite(workflowData *WorkflowData) error { return errors.New(strings.Join(lines, "\n")) } +func validateMentionGitHubAppPermissionsReadOnly(workflowData *WorkflowData) error { + if workflowData == nil || workflowData.SafeOutputs == nil || + workflowData.SafeOutputs.Mentions == nil || workflowData.SafeOutputs.Mentions.GitHubApp == nil { + return nil + } + + var invalidScopes []string + for scope, level := range workflowData.SafeOutputs.Mentions.GitHubApp.Permissions { + normalized := strings.ToLower(strings.TrimSpace(level)) + if normalized != string(PermissionRead) && normalized != string(PermissionNone) { + invalidScopes = append(invalidScopes, scope+" (level: "+level+")") + } + } + if len(invalidScopes) == 0 { + return nil + } + sort.Strings(invalidScopes) + return NewValidationError( + "safe-outputs.mentions.github-app.permissions", + strings.Join(invalidScopes, ", "), + "mention-resolution GitHub App permissions must be read-only; each level must be \"read\" or \"none\"", + "Change each permission level to \"read\" or \"none\". Write permissions are not allowed for mention resolution.", + ) +} + // warnGitHubAppPermissionsUnsupportedContexts emits a warning when // github-app.permissions is set in contexts that do not support it. // The permissions field takes effect for tools.github.github-app and diff --git a/pkg/workflow/github_app_permissions_validation_test.go b/pkg/workflow/github_app_permissions_validation_test.go index 82f6f9f6eb1..0fcda11b82e 100644 --- a/pkg/workflow/github_app_permissions_validation_test.go +++ b/pkg/workflow/github_app_permissions_validation_test.go @@ -210,6 +210,36 @@ func TestValidateGitHubAppOnlyPermissions(t *testing.T) { } } +func TestValidateMentionGitHubAppPermissionsReadOnly(t *testing.T) { + for _, tc := range []struct { + name string + permissions map[string]string + wantError bool + }{ + {name: "read and none are accepted", permissions: map[string]string{"members": "read", "issues": "none"}}, + {name: "write is rejected", permissions: map[string]string{"members": "write"}, wantError: true}, + {name: "unknown levels are rejected", permissions: map[string]string{"issues": "admin"}, wantError: true}, + } { + t.Run(tc.name, func(t *testing.T) { + err := validateMentionGitHubAppPermissionsReadOnly(&WorkflowData{ + SafeOutputs: &SafeOutputsConfig{Mentions: &MentionsConfig{ + GitHubApp: &GitHubAppConfig{Permissions: tc.permissions}, + }}, + }) + if tc.wantError { + if err == nil { + t.Fatal("expected invalid mention app permissions to fail") + } + if !strings.Contains(err.Error(), "safe-outputs.mentions.github-app.permissions") { + t.Fatalf("expected mention-specific diagnostic, got: %v", err) + } + } else if err != nil { + t.Fatalf("expected read-only permissions to pass: %v", err) + } + }) + } +} + func TestIsGitHubAppOnlyScope(t *testing.T) { tests := []struct { scope PermissionScope diff --git a/pkg/workflow/permissions_compiler_validator.go b/pkg/workflow/permissions_compiler_validator.go index 343adf228d3..a679a4c6a97 100644 --- a/pkg/workflow/permissions_compiler_validator.go +++ b/pkg/workflow/permissions_compiler_validator.go @@ -99,6 +99,9 @@ func (c *Compiler) validatePermissions(workflowData *WorkflowData, markdownPath if err := validateGitHubMCPAppPermissionsNoWrite(workflowData); err != nil { return nil, formatCompilerError(markdownPath, "error", err.Error(), err) } + if err := validateMentionGitHubAppPermissionsReadOnly(workflowData); err != nil { + return nil, formatCompilerError(markdownPath, "error", err.Error(), err) + } // Warn when github-app.permissions is set in contexts that don't support it warnGitHubAppPermissionsUnsupportedContexts(workflowData) diff --git a/pkg/workflow/safe_outputs_config_types.go b/pkg/workflow/safe_outputs_config_types.go index fa65ba7a6b4..aac3f1c9459 100644 --- a/pkg/workflow/safe_outputs_config_types.go +++ b/pkg/workflow/safe_outputs_config_types.go @@ -179,15 +179,18 @@ type MentionsConfig struct { // AllowContext determines if mentions from event context are allowed (default: true) AllowContext *bool `yaml:"allow-context,omitempty" json:"allowContext,omitempty"` + GitHubToken string `yaml:"github-token,omitempty" json:"-"` + GitHubApp *GitHubAppConfig `yaml:"github-app,omitempty" json:"-"` + // Allowed is a list of user/bot names always allowed (bots not allowed by default) Allowed []string `yaml:"allowed,omitempty" json:"allowed,omitempty"` // AllowedTeams is a list of team slugs whose members are always allowed to be mentioned. // Accepts "team-slug" (resolved against the current org) or "org/team-slug" format. - // Requires the workflow token to have read:org scope (a fine-grained PAT, classic PAT with - // read:org, or a GitHub App with the Members:Read permission). The default GITHUB_TOKEN - // does not include read:org and will produce a 403/404 warning; team members will be skipped - // but the workflow will not fail. + // Team membership is resolved in the trusted safe_outputs job and requires the mention + // resolution token to have read:org scope (a fine-grained PAT, classic PAT with read:org, + // or a GitHub App with the Members:Read permission). The default github.token does not + // include read:org; team members will be skipped with a warning if no suitable token is set. AllowedTeams []string `yaml:"allowed-teams,omitempty" json:"allowedTeams,omitempty"` // Max is the maximum number of mentions per message (default: 50). Supports integer or GitHub Actions expression. diff --git a/pkg/workflow/safe_outputs_mentions_test.go b/pkg/workflow/safe_outputs_mentions_test.go index 49664a69bae..b61a8be0325 100644 --- a/pkg/workflow/safe_outputs_mentions_test.go +++ b/pkg/workflow/safe_outputs_mentions_test.go @@ -50,6 +50,29 @@ func TestParseMentionsConfig_Boolean(t *testing.T) { } } +func TestParseMentionsCredentials(t *testing.T) { + config := parseMentionsConfig(map[string]any{ + "github-token": "${{ secrets.MENTIONS_PAT }}", + "github-app": map[string]any{ + "client-id": "${{ vars.MENTIONS_APP_ID }}", "private-key": "${{ secrets.MENTIONS_APP_KEY }}", + "ignore-if-missing": true, "permissions": map[string]any{"members": "read"}, + }, + }) + require.Equal(t, "${{ secrets.MENTIONS_PAT }}", config.GitHubToken) + require.NotNil(t, config.GitHubApp) + require.Equal(t, "${{ vars.MENTIONS_APP_ID }}", config.GitHubApp.AppID) + require.Equal(t, "${{ secrets.MENTIONS_APP_KEY }}", config.GitHubApp.PrivateKey) + require.True(t, config.GitHubApp.IgnoreIfMissing) + require.Equal(t, map[string]string{"members": "read"}, config.GitHubApp.Permissions) + runtimeConfig := buildMentionsHandlerConfig(config) + require.NotContains(t, runtimeConfig, "github-token") + require.NotContains(t, runtimeConfig, "github-app") + content, err := json.Marshal(config) + require.NoError(t, err) + require.NotContains(t, string(content), "MENTIONS_PAT") + require.NotContains(t, string(content), "MENTIONS_APP_KEY") +} + func TestParseMentionsConfig_Object(t *testing.T) { tests := []struct { name string diff --git a/pkg/workflow/safe_outputs_messages_config.go b/pkg/workflow/safe_outputs_messages_config.go index e0c66c12ef8..aa0e0f4135d 100644 --- a/pkg/workflow/safe_outputs_messages_config.go +++ b/pkg/workflow/safe_outputs_messages_config.go @@ -78,6 +78,10 @@ func parseMentionsConfig(mentions any) *MentionsConfig { // Handle object configuration if mentionsMap, ok := mentions.(map[string]any); ok { + config.GitHubToken = extractStringFromMap(mentionsMap, "github-token", nil) + if app, ok := mentionsMap["github-app"].(map[string]any); ok { + config.GitHubApp = parseAppConfig(app) + } // Parse allowed-collaborators (preferred) with fallback to deprecated allow-team-members if allowedCollaborators, exists := mentionsMap["allowed-collaborators"]; exists { if val, ok := allowedCollaborators.(bool); ok { diff --git a/pkg/workflow/safe_outputs_permissions.go b/pkg/workflow/safe_outputs_permissions.go index 53fdecfaf2c..13c498457d6 100644 --- a/pkg/workflow/safe_outputs_permissions.go +++ b/pkg/workflow/safe_outputs_permissions.go @@ -156,6 +156,15 @@ func computePermissionsForSafeOutputs(safeOutputs *SafeOutputsConfig, excludePer permissions.Merge(handlerPermissions) } + if safeOutputs.AddComments != nil && addCommentTargetsEnabled(safeOutputs.AddComments) { + // Mention resolution fetches explicit comment targets through the Issues API, + // including when add-comment is configured for pull requests only. Add this + // only when the selected safe-output token does not already have Issues access. + if _, hasIssuesPermission := permissions.Get(PermissionIssues); !hasIssuesPermission { + permissions.Set(PermissionIssues, PermissionRead) + } + } + if dispatchRepositoryPermissions := computeDispatchRepositoryPermissions(safeOutputs, excludePerHandlerApps); dispatchRepositoryPermissions != nil { permissions.Merge(dispatchRepositoryPermissions) } @@ -201,6 +210,30 @@ func computePermissionsForSafeOutputs(safeOutputs *SafeOutputsConfig, excludePer return permissions } +func buildMentionResolutionPermissions(safeOutputs *SafeOutputsConfig) *Permissions { + permissions := NewPermissions() + if safeOutputs == nil { + return permissions + } + if safeOutputs.AddComments != nil && addCommentTargetsEnabled(safeOutputs.AddComments) { + // The pre-handler author lookup uses issues.get for both issue and PR targets. + permissions.Set(PermissionIssues, PermissionRead) + if safeOutputs.AddComments.PullRequests == nil || *safeOutputs.AddComments.PullRequests { + permissions.Set(PermissionPullRequests, PermissionRead) + } + } + if safeOutputs.Mentions != nil && len(safeOutputs.Mentions.AllowedTeams) > 0 { + permissions.Set(PermissionMembers, PermissionRead) + } + return permissions +} + +func addCommentTargetsEnabled(config *AddCommentsConfig) bool { + return config != nil && + ((config.Issues == nil || *config.Issues) || + (config.PullRequests == nil || *config.PullRequests)) +} + func computeDispatchRepositoryPermissions(safeOutputs *SafeOutputsConfig, excludePerToolApps bool) *Permissions { if safeOutputs == nil || safeOutputs.DispatchRepository == nil || len(safeOutputs.DispatchRepository.Tools) == 0 { return nil diff --git a/pkg/workflow/safe_outputs_permissions_test.go b/pkg/workflow/safe_outputs_permissions_test.go index fd327a477e3..e42af7d23bb 100644 --- a/pkg/workflow/safe_outputs_permissions_test.go +++ b/pkg/workflow/safe_outputs_permissions_test.go @@ -122,7 +122,7 @@ func TestComputePermissionsForSafeOutputs(t *testing.T) { }, }, { - name: "add-comment with issues:false - no issues permission and no discussions by default", + name: "add-comment with issues:false - read permission for PR target author lookup", safeOutputs: &SafeOutputsConfig{ AddComments: &AddCommentsConfig{ BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, @@ -130,6 +130,7 @@ func TestComputePermissionsForSafeOutputs(t *testing.T) { }, }, expected: map[PermissionScope]PermissionLevel{ + PermissionIssues: PermissionRead, PermissionPullRequests: PermissionWrite, }, }, @@ -636,7 +637,7 @@ func TestComputePermissionsForSafeOutputsExcludesPerHandlerAppsFromGlobalAppToke perms := computePermissionsForSafeOutputs(safeOutputs, true) require.NotNil(t, perms) assert.Equal(t, PermissionWrite, perms.permissions[PermissionActions]) - assert.NotContains(t, perms.permissions, PermissionIssues) + assert.Equal(t, PermissionRead, perms.permissions[PermissionIssues]) } func TestComputePermissionsForSafeOutputsDispatchRepositoryAppSplit(t *testing.T) { @@ -699,11 +700,29 @@ Test workflow. perms := computePermissionsForSafeOutputs(workflowData.SafeOutputs, true) require.NotNil(t, perms) - assert.NotContains(t, perms.permissions, PermissionIssues) + assert.Equal(t, PermissionRead, perms.permissions[PermissionIssues]) assert.NotContains(t, perms.permissions, PermissionPullRequests) assert.Equal(t, PermissionWrite, perms.permissions[PermissionContents]) } +func TestMentionResolutionPermissionsIncludeIssuesReadForPullRequestOnlyComments(t *testing.T) { + issues := false + pullRequests := true + safeOutputs := &SafeOutputsConfig{ + AddComments: &AddCommentsConfig{Issues: &issues, PullRequests: &pullRequests}, + Mentions: &MentionsConfig{AllowedTeams: []string{"my-org/my-team"}}, + } + + jobPermissions := computePermissionsForSafeOutputs(safeOutputs, true) + require.Equal(t, PermissionRead, jobPermissions.permissions[PermissionIssues]) + assert.Equal(t, PermissionWrite, jobPermissions.permissions[PermissionPullRequests]) + + mentionTokenPermissions := buildMentionResolutionPermissions(safeOutputs) + assert.Equal(t, PermissionRead, mentionTokenPermissions.permissions[PermissionIssues]) + assert.Equal(t, PermissionRead, mentionTokenPermissions.permissions[PermissionPullRequests]) + assert.Equal(t, PermissionRead, mentionTokenPermissions.permissions[PermissionMembers]) +} + func TestBuildPreambleTokenStepsExcludesParsedPerHandlerApps(t *testing.T) { compiler := NewCompiler(WithVersion("1.0.0")) tmpDir := t.TempDir() diff --git a/pkg/workflow/safe_outputs_step_token_validation.go b/pkg/workflow/safe_outputs_step_token_validation.go index 1e5cd456163..6be7f42aa3f 100644 --- a/pkg/workflow/safe_outputs_step_token_validation.go +++ b/pkg/workflow/safe_outputs_step_token_validation.go @@ -17,7 +17,7 @@ var stepOutputReferencePattern = regexp.MustCompile(`steps\.([A-Za-z_][A-Za-z0-9 // collectSafeOutputStepTokenIDs returns the set of step ids referenced by // safe-outputs `github-token` expressions of the form `${{ steps..outputs. }}`, -// covering both the global token and per-output overrides. +// covering the global token, mention credentials, and per-output overrides. func collectSafeOutputStepTokenIDs(config *SafeOutputsConfig) map[string]struct{} { ids := make(map[string]struct{}) if config == nil { @@ -31,6 +31,9 @@ func collectSafeOutputStepTokenIDs(config *SafeOutputsConfig) map[string]struct{ } collect(config.GitHubToken) + if config.Mentions != nil { + collect(config.Mentions.GitHubToken) + } for _, handler := range safeOutputHandlers { if handler.StructField == "" { continue @@ -120,10 +123,15 @@ func (c *Compiler) validateSafeOutputStepTokenReferences(data *WorkflowData) err if declareIdx >= 0 && declareIdx < consumeIdx { continue } + field := "safe-outputs.github-token" + if data.SafeOutputs.Mentions != nil && + strings.Contains(data.SafeOutputs.Mentions.GitHubToken, "steps."+stepID+".outputs.") { + field = "safe-outputs.mentions.github-token" + } if declareIdx < 0 { safeOutputsStepTokenValidationLog.Printf("Job %q consumes steps.%s.outputs.* without declaring the step", jobName, stepID) return NewValidationError( - "safe-outputs.github-token", + field, fmt.Sprintf("${{ steps.%s.outputs.* }}", stepID), fmt.Sprintf("job %q has no step with id %q; step outputs are only available inside the job that produced them, so this token would be empty at runtime and requires the minting step to run in that job", jobName, stepID), fmt.Sprintf("Add the token-minting step to job %q:\n\n%s", jobName, stepMintingHint(jobName, stepID)), @@ -131,7 +139,7 @@ func (c *Compiler) validateSafeOutputStepTokenReferences(data *WorkflowData) err } safeOutputsStepTokenValidationLog.Printf("Job %q consumes steps.%s.outputs.* before the step that declares it", jobName, stepID) return NewValidationError( - "safe-outputs.github-token", + field, fmt.Sprintf("${{ steps.%s.outputs.* }}", stepID), fmt.Sprintf("job %q runs the step with id %q after the first step that consumes the token, so the token would be empty at runtime and requires the minting step to run before its first consumer", jobName, stepID), fmt.Sprintf("Move the token-minting step earlier in job %q, for example using its pre-steps:\n\n%s", jobName, stepMintingHint(jobName, stepID)), diff --git a/pkg/workflow/safe_outputs_step_token_validation_test.go b/pkg/workflow/safe_outputs_step_token_validation_test.go index 8608e95801d..c3e90bc27a9 100644 --- a/pkg/workflow/safe_outputs_step_token_validation_test.go +++ b/pkg/workflow/safe_outputs_step_token_validation_test.go @@ -104,6 +104,75 @@ safe-outputs: assert.Contains(t, err.Error(), "pre-steps:") } +func TestSameJobStepTokenMissingInSafeOutputMentionResolutionFails(t *testing.T) { + workflowFile := writeStepTokenWorkflow(t, "missing-agent", `--- +on: + workflow_dispatch: +permissions: + contents: read + id-token: write +engine: claude +strict: false +safe-outputs: + add-comment: + mentions: + allowed-teams: [my-org/my-team] + github-token: ${{ steps.octosts.outputs.token || secrets.GITHUB_TOKEN }} +jobs: + conclusion: + pre-steps: + - name: Mint token (conclusion) + id: octosts + uses: `+stsMintStep+` +--- + +# Missing safe-output mention-resolution token +`) + + err := NewCompiler().CompileWorkflow(workflowFile) + require.Error(t, err) + assert.Contains(t, err.Error(), `job "safe_outputs" has no step with id "octosts"`) + assert.Contains(t, err.Error(), "safe-outputs.mentions.github-token") + assert.Contains(t, err.Error(), "pre-steps:") +} + +func TestSameJobStepTokenMentionsOnlyMintedInAgentCompiles(t *testing.T) { + workflowFile := writeStepTokenWorkflow(t, "mentions-only", `--- +on: + workflow_dispatch: +permissions: + contents: read + id-token: write +engine: claude +strict: false +safe-outputs: + add-comment: + mentions: + allowed-teams: [my-org/my-team] + github-token: ${{ steps.mention_mint.outputs.token }} +jobs: + safe_outputs: + pre-steps: + - name: Mint mention token + id: mention_mint + uses: `+stsMintStep+` +--- + +# Mention-only same-job token +`) + + require.NoError(t, NewCompiler().CompileWorkflow(workflowFile)) + lockContent, err := os.ReadFile(filepath.Join(filepath.Dir(workflowFile), "mentions-only.lock.yml")) + require.NoError(t, err) + lockYAML := string(lockContent) + agent := extractJobSection(lockYAML, "agent") + assert.NotContains(t, agent, "steps.mention_mint.outputs.token") + safeOutputs := extractJobSection(lockYAML, "safe_outputs") + assert.Contains(t, safeOutputs, "GH_AW_MENTIONS_GITHUB_TOKEN: ${{ steps.mention_mint.outputs.token }}") + assert.Less(t, jobStepIDDeclarationIndex(safeOutputs, "mention_mint"), jobStepOutputConsumptionIndex(safeOutputs, "mention_mint")) + assert.NotContains(t, extractJobSection(lockYAML, "conclusion"), "steps.mention_mint.outputs.token") +} + // TestSameJobStepTokenMintedAfterConsumerFails verifies that safe-outputs.steps, which run // after the safe_outputs checkout and git credential steps, are reported as too late for a // token consumed by those steps. @@ -148,6 +217,9 @@ jobs: func TestCollectSafeOutputStepTokenIDs(t *testing.T) { config := &SafeOutputsConfig{ GitHubToken: "${{ steps.global_mint.outputs.token || secrets.GITHUB_TOKEN }}", + Mentions: &MentionsConfig{ + GitHubToken: "${{ steps.mention_mint.outputs.token }}", + }, CreateIssues: &CreateIssuesConfig{ BaseSafeOutputConfig: BaseSafeOutputConfig{ GitHubToken: "${{ steps.issue_mint.outputs.token }}", @@ -161,7 +233,8 @@ func TestCollectSafeOutputStepTokenIDs(t *testing.T) { } ids := collectSafeOutputStepTokenIDs(config) - assert.Len(t, ids, 2) + assert.Len(t, ids, 3) + assert.Contains(t, ids, "mention_mint") assert.Contains(t, ids, "global_mint") assert.Contains(t, ids, "issue_mint") diff --git a/pkg/workflow/security_architecture_formal_test.go b/pkg/workflow/security_architecture_formal_test.go index 6bcbd58f112..3ccf17a9814 100644 --- a/pkg/workflow/security_architecture_formal_test.go +++ b/pkg/workflow/security_architecture_formal_test.go @@ -416,12 +416,8 @@ Simulate a wildcard network violation. // TestFormal_P10_WriteTokenIsolatedToSafeOutput (P10 TokenIsolation) // -// Spec Section 5: write tokens must be absent from the agent job's environment -// and present only in the safe_outputs job. -// -// Compiles a real workflow with a safe-outputs github-app configuration and -// inspects the produced YAML to verify that the private key appears only in -// the safe_outputs mint step inputs (under with:) and not in the agent job. +// Spec Section 5: write tokens and app keys must be absent from the agent job. +// Mention filtering credentials are configured separately from write credentials. func TestFormal_P10_WriteTokenIsolatedToSafeOutput(t *testing.T) { md := `--- name: token-isolation-test @@ -438,7 +434,7 @@ safe-outputs: # Mission -Token isolation test: verify the private key is restricted to the safe_outputs job. +Token isolation test: verify the private key is unavailable during agent execution. ` tmpDir := t.TempDir() mdPath := filepath.Join(tmpDir, "workflow.md") @@ -462,9 +458,11 @@ Token isolation test: verify the private key is restricted to the safe_outputs j safeOutputsSection, hasSafeOutputs := sections["safe_outputs"] require.True(t, hasSafeOutputs, "compiled YAML must contain a safe_outputs job") - // The agent job must not carry the private key material in any form. assert.NotContains(t, agentSection, "APP_PRIVATE_KEY", - "agent job must not carry the private key material — token isolation requires it stays in safe_outputs") + "the safe-output write app key must not enter the agent job") + assert.NotContains(t, agentSection, "safe-outputs-ingestion-app-token") + assert.NotContains(t, agentSection, "permission-issues: write", + "the ingestion token must not request the safe_outputs job's write permissions") // The safe_outputs job must hold the private key in its mint step inputs (with: block). assert.Contains(t, safeOutputsSection, "private-key: ${{ secrets.APP_PRIVATE_KEY }}",