-
Notifications
You must be signed in to change notification settings - Fork 540
Add deterministic body footer templates to safe outputs #58841
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
473602d
4205664
ff8db86
c6bda37
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ const { generateWorkflowIdMarker, generateWorkflowCallIdMarker, generateCloseKey | |
| const { parseBoolTemplatable, parseIntTemplatable } = require("./templatable.cjs"); | ||
| const { assembleMarkdownBodyParts } = require("./markdown_body_helpers.cjs"); | ||
| const { getBodyHeader, getDisclosureHeader } = require("./messages_header.cjs"); | ||
| const { getBodyFooterMessage } = require("./messages_footer.cjs"); | ||
| const { generateHistoryUrl } = require("./generate_history_link.cjs"); | ||
| const { normalizeBranchName } = require("./normalize_branch_name.cjs"); | ||
| const { pushExtraEmptyCommit } = require("./extra_empty_commit.cjs"); | ||
|
|
@@ -64,6 +65,8 @@ const { | |
| } = require("./create_pull_request_helpers.cjs"); | ||
| const { isStackedEnabled, parseStackMetadata, hasCircularStackDependency, buildStackMetadataLines, stackedDisabledError, circularStackError, verifyStackBaseBranchExists, createStackTracker } = require("./stacked_pull_requests.cjs"); | ||
|
|
||
| const MAX_GITHUB_BODY_LENGTH = 65536; | ||
|
|
||
| /** | ||
| * @typedef {import('./types/handler-factory').HandlerFactoryFunction} HandlerFactoryFunction | ||
| */ | ||
|
|
@@ -449,6 +452,10 @@ async function rewriteBundleBranchAsSingleCommit(baseBranch, execApi, bundleFile | |
| * @returns {Promise<{data: any, issueRepoParts: {owner: string, repo: string}}>} | ||
| */ | ||
| async function createFallbackIssue(githubClient, repoParts, title, body, labels, assignees) { | ||
| if (body.length > MAX_GITHUB_BODY_LENGTH) { | ||
| throw new Error(`Fallback issue body exceeds GitHub's maximum length of ${MAX_GITHUB_BODY_LENGTH} characters`); | ||
| } | ||
|
|
||
| const payload = { | ||
| owner: repoParts.owner, | ||
| repo: repoParts.repo, | ||
|
|
@@ -1673,6 +1680,13 @@ async function main(config = {}) { | |
| footerParts.push(footer); | ||
| } | ||
|
|
||
| const bodyFooter = getBodyFooterMessage(config.body_footer, { workflowName, runUrl }); | ||
| if (bodyFooter) { | ||
| const renderedBodyFooter = bodyFooter.trimEnd(); | ||
| bodyLines.push(``, renderedBodyFooter); | ||
|
|
||
| footerParts.push(renderedBodyFooter); | ||
| } | ||
|
|
||
| // Add standalone workflow-id marker for searchability (consistent with comments) | ||
| // Always add XML markers even when footer is disabled | ||
| if (workflowId) { | ||
|
|
@@ -1701,6 +1715,9 @@ async function main(config = {}) { | |
| const issueSafeBody = neutralizeClosingKeywordsForIssueBody(body); | ||
| // Footer section (footer + workflow-id marker) used when ordering protected-files notices | ||
| const footerContent = footerParts.join("\n\n"); | ||
| const issueSafeFooterContent = neutralizeClosingKeywordsForIssueBody(footerContent); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This adds a second footer code path for protected-file fallback issues, but the tests never exercise it, so a regression here will ship silently. 💡 Add a focused fallback-path test
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] New 💡 Suggested testAdd a case to the manifest-protection fallback tests that configures @copilot please address this. |
||
| const hiddenFallbackMetadata = [callerWorkflowId && generateWorkflowCallIdMarker(callerWorkflowId), closeOlderKey && generateCloseKeyMarker(closeOlderKey)].filter(Boolean).join("\n"); | ||
| const issueSafeFallbackFooter = [issueSafeFooterContent, hiddenFallbackMetadata].filter(Boolean).join("\n"); | ||
|
|
||
| // Build labels array - merge config labels with message labels | ||
| let labels = [...envLabels]; | ||
|
|
@@ -1903,7 +1920,7 @@ async function main(config = {}) { | |
| .replace(/\s+/g, " ") | ||
| .trim(); | ||
| const pushErrorSection = buildPushErrorSection(getErrorMessage(pushError), pushFailureMessage); | ||
| const fallbackBody = `${issueSafeBody} | ||
| const fallbackBody = `${issueSafeMainBodyContent} | ||
|
|
||
| --- | ||
|
|
||
|
|
@@ -1929,7 +1946,9 @@ git push ${shellQuote(pushRemoteUrl || "origin")} ${shellQuote(branchName)} | |
| gh pr create --title ${shellQuote(title)} --base ${shellQuote(baseBranch)} --head ${shellQuote(getPullRequestHeadRef(branchName))} --repo ${shellQuote(`${repoParts.owner}/${repoParts.repo}`)} | ||
| \`\`\` | ||
|
|
||
| </details>`; | ||
| </details> | ||
|
|
||
| ${issueSafeFallbackFooter}`; | ||
|
|
||
| try { | ||
| const { data: issue, issueRepoParts } = await createFallbackIssue(githubClient, repoParts, title, fallbackBody, mergeFallbackIssueLabels(effectiveFallbackLabels), configAssignees); | ||
|
|
@@ -2272,7 +2291,7 @@ gh pr create --title ${shellQuote(title)} --base ${shellQuote(baseBranch)} --hea | |
| .replace(/\s+/g, " ") | ||
| .trim(); | ||
| const pushErrorSection = buildPushErrorSection(getErrorMessage(pushError), pushFailureMessage); | ||
| const fallbackBody = `${issueSafeBody} | ||
| const fallbackBody = `${issueSafeMainBodyContent} | ||
|
|
||
| --- | ||
|
|
||
|
|
@@ -2299,7 +2318,9 @@ gh pr create --title ${shellQuote(title)} --base ${shellQuote(baseBranch)} --hea | |
| \`\`\` | ||
|
|
||
| </details> | ||
| ${patchPreview}`; | ||
| ${patchPreview} | ||
|
|
||
| ${issueSafeFallbackFooter}`; | ||
|
|
||
| try { | ||
| const { data: issue, issueRepoParts } = await createFallbackIssue(githubClient, repoParts, title, fallbackBody, mergeFallbackIssueLabels(effectiveFallbackLabels), configAssignees); | ||
|
|
@@ -2462,7 +2483,7 @@ ${patchPreview}`; | |
| const pushFailedTemplatePath = getPromptPath("manifest_protection_push_failed_fallback.md"); | ||
| fallbackBody = renderTemplateFromFile(pushFailedTemplatePath, { | ||
| main_body: issueSafeMainBodyContent, | ||
| footer: footerContent, | ||
| footer: issueSafeFooterContent, | ||
| files: fileList, | ||
| apply_instructions: applyInstructions, | ||
| branch_name: branchName, | ||
|
|
@@ -2473,7 +2494,7 @@ ${patchPreview}`; | |
| } else { | ||
| // Normal case — push succeeded, provide compare URL. | ||
| const createPrUrl = buildManifestProtectionCreatePrUrl(githubServer, repoParts, baseBranch, branchName, title, undefined, getPullRequestHeadRef(branchName)); | ||
| fallbackBody = renderManifestProtectionFallbackBody(issueSafeMainBodyContent, footerContent, fileList, createPrUrl); | ||
| fallbackBody = renderManifestProtectionFallbackBody(issueSafeMainBodyContent, issueSafeFooterContent, fileList, createPrUrl); | ||
| } | ||
|
|
||
| try { | ||
|
|
@@ -2484,7 +2505,7 @@ ${patchPreview}`; | |
| if (!manifestProtectionPushFailedError) { | ||
| try { | ||
| const createPrUrl = buildManifestProtectionCreatePrUrl(githubServer, repoParts, baseBranch, branchName, title, issue.number, getPullRequestHeadRef(branchName)); | ||
| const fallbackBodyWithCloseKeyword = renderManifestProtectionFallbackBody(issueSafeMainBodyContent, footerContent, fileList, createPrUrl); | ||
| const fallbackBodyWithCloseKeyword = renderManifestProtectionFallbackBody(issueSafeMainBodyContent, issueSafeFooterContent, fileList, createPrUrl); | ||
|
|
||
| await withRetry( | ||
| () => | ||
|
|
@@ -2524,6 +2545,9 @@ ${patchPreview}`; | |
|
|
||
| // Try to create the pull request, with fallback to issue creation | ||
| try { | ||
| if (body.length > MAX_GITHUB_BODY_LENGTH) { | ||
| throw new Error(`Pull request body exceeds GitHub's maximum length of ${MAX_GITHUB_BODY_LENGTH} characters`); | ||
| } | ||
| const { data: pullRequest } = await createOrUpdatePullRequest({ | ||
| githubClient, | ||
| repoParts, | ||
|
|
@@ -2795,7 +2819,8 @@ ${patchPreview}`; | |
|
|
||
| const fallbackTemplatePath = getPromptPath("pr_permission_denied_fallback.md"); | ||
| const fallbackBody = renderTemplateFromFile(fallbackTemplatePath, { | ||
| body: issueSafeBody, | ||
| body: issueSafeMainBodyContent, | ||
| footer: issueSafeFallbackFooter, | ||
| branch_name: branchName, | ||
| create_pr_url: createPrUrl, | ||
| faq_url: FAQ_CREATE_PR_PERMISSIONS_URL, | ||
|
|
@@ -2858,7 +2883,7 @@ ${patchPreview}`; | |
| patchPreview = generatePatchPreview(patchContent); | ||
| } | ||
|
|
||
| const fallbackBody = `${issueSafeBody} | ||
| const fallbackBody = `${issueSafeMainBodyContent} | ||
|
|
||
| --- | ||
|
|
||
|
|
@@ -2872,7 +2897,9 @@ To create the pull request manually: | |
| \`\`\`sh | ||
| gh pr create --title "${title}" --base ${baseBranch} --head ${getPullRequestHeadRef(branchName)} --repo ${repoParts.owner}/${repoParts.repo} | ||
| \`\`\` | ||
| ${patchPreview}`; | ||
| ${patchPreview} | ||
|
|
||
| ${issueSafeFallbackFooter}`; | ||
|
|
||
| try { | ||
| const { data: issue, issueRepoParts } = await createFallbackIssue(githubClient, repoParts, title, fallbackBody, mergeFallbackIssueLabels(effectiveFallbackLabels), configAssignees); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -293,6 +293,19 @@ function getFooterMessage(ctx) { | |
| return renderedDefaultFooter + getRunAgainHints(renderedDefaultFooter); | ||
| } | ||
|
|
||
| /** | ||
| * Render a deterministic footer configured for a generated issue or pull request body. | ||
| * @param {string|undefined} template - Body footer template | ||
| * @param {{workflowName: string, runUrl: string}} ctx - Template context | ||
| * @returns {string} Rendered body footer, or an empty string when not configured | ||
| */ | ||
| function getBodyFooterMessage(template, ctx) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actions/setup/js/messages_footer.cjs:302: yagni: new getBodyFooterMessage helper for one renderTemplate call. Inline it at the two call sites and drop the wrapper. |
||
| if (!template) { | ||
| return ""; | ||
| } | ||
| return renderTemplate(template, toSnakeCase(ctx)); | ||
| } | ||
|
|
||
| /** | ||
| * @param {string|undefined} commandsJSON | ||
| * @returns {string|undefined} | ||
|
|
@@ -772,6 +785,7 @@ function generateFooterWithMessages(workflowName, runUrl, workflowSource, workfl | |
|
|
||
| module.exports = { | ||
| getDetectionCautionAlert, | ||
| getBodyFooterMessage, | ||
| getFooterMessage, | ||
| getFooterInstallMessage, | ||
| getFooterWorkflowRecompileMessage, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/codebase-design]
body_footeris appended tobodyLinesdirectly, bypassing thesanitizeContentcall applied toprocessedBodya few lines above (line 919). Sincebody-footeris workflow-author-controlled (not agent output), this is lower risk than the main body, but it's inconsistent with the codebase's existing security posture where all body content flows through sanitization before hittingbodyLines.💡 Consideration
If
body-footertemplates ever interpolate values that could contain untrusted data (e.g. a future placeholder pulling from PR/issue titles), this unsanitized path would become an injection vector. Worth a one-line comment documenting the assumption thatbody-footeris always static/workflow-author-controlled and never agent-influenced.@copilot please address this.