Skip to content

Commit c0b3bdf

Browse files
pelikhanpelikhan
authored
Merge branch 'main' into copilot/fix-push-wiki-memory-count
2 parents da04381 + eacfb51 commit c0b3bdf

7 files changed

Lines changed: 374 additions & 90 deletions

‎actions/setup/js/collect_ndjson_output.cjs‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -323,8 +323,12 @@ async function main() {
323323
}
324324
continue;
325325
}
326-
// Update item with normalized values
327-
Object.assign(item, validationResult.normalizedItem);
326+
// SECURITY: Use normalizedItem (which strips infrastructure-only fields
327+
// like patch_path, bundle_path, base_commit, diff_size) instead of the
328+
// original item, to prevent agent-injected transport metadata from
329+
// reaching the privileged handler.
330+
core.info(`Line ${i + 1}: Valid ${itemType} item`);
331+
parsedItems.push(validationResult.normalizedItem);
328332
} else {
329333
// Fall back to validateItemWithSafeJobConfig for unknown types
330334
const jobOutputType = expectedOutputTypes[itemType];
@@ -341,10 +345,9 @@ async function main() {
341345
}
342346
Object.assign(item, validation.normalizedItem);
343347
}
348+
core.info(`Line ${i + 1}: Valid ${itemType} item`);
349+
parsedItems.push(item);
344350
}
345-
346-
core.info(`Line ${i + 1}: Valid ${itemType} item`);
347-
parsedItems.push(item);
348351
} catch (error) {
349352
const errorMsg = getErrorMessage(error);
350353
errors.push(`Line ${i + 1}: Invalid JSON - ${errorMsg}`);

‎actions/setup/js/create_pull_request.cjs‎

Lines changed: 50 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -1847,58 +1847,58 @@ gh pr create --title '${title}' --base ${baseBranch} --head ${branchName} --repo
18471847
}
18481848
}
18491849

1850-
// Push the applied commits to the branch (with fallback to issue creation on failure)
1851-
// Note: when manifestProtectionFallback is set we still push the branch so the
1852-
// fallback issue can include a compare URL. Genuine push failures are handled in
1853-
// the catch block below.
1854-
{
1855-
try {
1856-
branchName = await handleRemoteBranchCollision(branchName, preserveBranchName, { recreateRef, githubClient, owner: repoParts.owner, repo: repoParts.repo });
1850+
// Push the applied commits to the branch (with fallback to issue creation on failure)
1851+
// Note: when manifestProtectionFallback is set we still push the branch so the
1852+
// fallback issue can include a compare URL. Genuine push failures are handled in
1853+
// the catch block below.
1854+
{
1855+
try {
1856+
branchName = await handleRemoteBranchCollision(branchName, preserveBranchName, { recreateRef, githubClient, owner: repoParts.owner, repo: repoParts.repo });
18571857

1858-
await pushSignedCommits({
1859-
githubClient,
1860-
owner: repoParts.owner,
1861-
repo: repoParts.repo,
1862-
branch: branchName,
1863-
baseRef: `origin/${baseBranch}`,
1864-
cwd: process.cwd(),
1865-
signedCommits,
1866-
resolvedTemporaryIds,
1867-
currentRepo: itemRepo,
1868-
});
1869-
core.info("Changes pushed to branch");
1858+
await pushSignedCommits({
1859+
githubClient,
1860+
owner: repoParts.owner,
1861+
repo: repoParts.repo,
1862+
branch: branchName,
1863+
baseRef: `origin/${baseBranch}`,
1864+
cwd: process.cwd(),
1865+
signedCommits,
1866+
resolvedTemporaryIds,
1867+
currentRepo: itemRepo,
1868+
});
1869+
core.info("Changes pushed to branch");
18701870

1871-
// Count new commits on PR branch relative to base, used to restrict
1872-
// the extra empty CI-trigger commit to exactly 1 new commit.
1873-
try {
1874-
const { stdout: countStr } = await exec.getExecOutput("git", ["rev-list", "--count", `origin/${baseBranch}..HEAD`]);
1875-
newCommitCount = parseInt(countStr.trim(), 10);
1876-
core.info(`${newCommitCount} new commit(s) on branch relative to origin/${baseBranch}`);
1877-
} catch {
1878-
// Non-fatal - newCommitCount stays 0, extra empty commit will be skipped
1879-
core.info("Could not count new commits - extra empty commit will be skipped");
1880-
}
1881-
} catch (pushError) {
1882-
// Push failed - create fallback issue instead of PR (if fallback is enabled)
1883-
core.error(`Git push failed: ${pushError instanceof Error ? pushError.message : String(pushError)}`);
1884-
1885-
if (manifestProtectionFallback) {
1886-
// Push failed specifically for a protected-file modification. Don't create
1887-
// a generic push-failed issue — fall through to the manifestProtectionFallback
1888-
// block below, which will create the proper protected-file review issue with
1889-
// patch artifact download instructions (since the branch was not pushed).
1890-
core.warning("Git push failed for protected-file modification - deferring to protected-file review issue");
1891-
manifestProtectionPushFailedError = pushError;
1892-
} else if (!fallbackAsIssue) {
1893-
// Fallback is disabled - return error without creating issue
1894-
core.error("fallback-as-issue is disabled - not creating fallback issue");
1895-
const error = `Failed to push changes: ${pushError instanceof Error ? pushError.message : String(pushError)}`;
1896-
return {
1897-
success: false,
1898-
error,
1899-
error_type: "push_failed",
1900-
};
1901-
} else {
1871+
// Count new commits on PR branch relative to base, used to restrict
1872+
// the extra empty CI-trigger commit to exactly 1 new commit.
1873+
try {
1874+
const { stdout: countStr } = await exec.getExecOutput("git", ["rev-list", "--count", `origin/${baseBranch}..HEAD`]);
1875+
newCommitCount = parseInt(countStr.trim(), 10);
1876+
core.info(`${newCommitCount} new commit(s) on branch relative to origin/${baseBranch}`);
1877+
} catch {
1878+
// Non-fatal - newCommitCount stays 0, extra empty commit will be skipped
1879+
core.info("Could not count new commits - extra empty commit will be skipped");
1880+
}
1881+
} catch (pushError) {
1882+
// Push failed - create fallback issue instead of PR (if fallback is enabled)
1883+
core.error(`Git push failed: ${pushError instanceof Error ? pushError.message : String(pushError)}`);
1884+
1885+
if (manifestProtectionFallback) {
1886+
// Push failed specifically for a protected-file modification. Don't create
1887+
// a generic push-failed issue — fall through to the manifestProtectionFallback
1888+
// block below, which will create the proper protected-file review issue with
1889+
// patch artifact download instructions (since the branch was not pushed).
1890+
core.warning("Git push failed for protected-file modification - deferring to protected-file review issue");
1891+
manifestProtectionPushFailedError = pushError;
1892+
} else if (!fallbackAsIssue) {
1893+
// Fallback is disabled - return error without creating issue
1894+
core.error("fallback-as-issue is disabled - not creating fallback issue");
1895+
const error = `Failed to push changes: ${pushError instanceof Error ? pushError.message : String(pushError)}`;
1896+
return {
1897+
success: false,
1898+
error,
1899+
error_type: "push_failed",
1900+
};
1901+
} else {
19021902
core.warning("Git push operation failed - creating fallback issue instead of pull request");
19031903

19041904
const runUrl = buildWorkflowRunUrl(context, context.repo);

‎actions/setup/js/generate_git_patch.cjs‎

Lines changed: 41 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,9 @@ function debugLog(message) {
4949
* @param {string[]} [options.excludedFiles] - Glob patterns for files to exclude from the patch.
5050
* Each pattern is passed to `git format-patch` as a `:(exclude)<pattern>` magic pathspec so
5151
* matching files are never included in the generated patch.
52+
* @param {string} [options.pinnedSha] - SECURITY: When set, use this SHA as the branch tip instead
53+
* of resolving refs/heads/<branchName>. Prevents TOCTOU races where the agent flips the branch
54+
* ref between patch and bundle generation.
5255
* @returns {Promise<Object>} Object with patch info or error
5356
*/
5457
async function generateGitPatch(branchName, baseBranch, options = {}) {
@@ -130,15 +133,25 @@ async function generateGitPatch(branchName, baseBranch, options = {}) {
130133
// can use it directly. The From <sha> header in format-patch output contains the
131134
// *new* commit SHA which won't exist in the target checkout.
132135
let baseCommitSha = null;
136+
let resolvedTipRef = null;
133137

134138
try {
135139
// Strategy 1: If we have a branch name, check if that branch exists and get its diff
136140
if (branchName) {
137-
debugLog(`Strategy 1: Checking if branch '${branchName}' exists locally`);
138-
// Check if the branch exists locally
141+
resolvedTipRef = options.pinnedSha || branchName;
142+
// SECURITY: When pinnedSha is provided, use it directly as the tip commit instead
143+
// of dereferencing refs/heads/<branchName>. This prevents TOCTOU races where the
144+
// agent can flip the branch ref between patch and bundle generation.
145+
const tipRef = resolvedTipRef;
139146
try {
140-
execGitSync(["show-ref", "--verify", "--quiet", `refs/heads/${branchName}`], { cwd });
141-
debugLog(`Strategy 1: Branch '${branchName}' exists locally`);
147+
if (options.pinnedSha) {
148+
debugLog(`Strategy 1: Using pinned SHA ${options.pinnedSha} (branch: ${branchName})`);
149+
} else {
150+
debugLog(`Strategy 1: Checking if branch '${branchName}' exists locally`);
151+
// Check if the branch exists locally
152+
execGitSync(["show-ref", "--verify", "--quiet", `refs/heads/${branchName}`], { cwd });
153+
debugLog(`Strategy 1: Branch '${branchName}' exists locally`);
154+
}
142155

143156
// Determine base ref for patch generation
144157
let baseRef;
@@ -251,7 +264,7 @@ async function generateGitPatch(branchName, baseBranch, options = {}) {
251264
}
252265

253266
if (defaultBranchRef) {
254-
baseRef = execGitSync(["merge-base", "--", defaultBranchRef, branchName], { cwd }).trim();
267+
baseRef = execGitSync(["merge-base", "--", defaultBranchRef, tipRef], { cwd }).trim();
255268
debugLog(`Strategy 1 (full): Computed merge-base: ${baseRef}`);
256269
} else {
257270
// No remote refs available - fall through to Strategy 2
@@ -265,12 +278,12 @@ async function generateGitPatch(branchName, baseBranch, options = {}) {
265278
debugLog(`Strategy 1: Resolved baseRef ${baseRef} to SHA ${baseCommitSha}`);
266279

267280
// Count commits to be included
268-
const commitCount = parseInt(execGitSync(["rev-list", "--count", `${baseRef}..${branchName}`], { cwd }).trim(), 10);
269-
debugLog(`Strategy 1: Found ${commitCount} commits between ${baseRef} and ${branchName}`);
281+
const commitCount = parseInt(execGitSync(["rev-list", "--count", `${baseRef}..${tipRef}`], { cwd }).trim(), 10);
282+
debugLog(`Strategy 1: Found ${commitCount} commits between ${baseRef} and ${tipRef}`);
270283

271284
if (commitCount > 0) {
272285
// Generate patch from the determined base to the branch
273-
const patchContent = execGitSync(["format-patch", `${baseRef}..${branchName}`, "--stdout", ...excludeArgs()], { cwd });
286+
const patchContent = execGitSync(["format-patch", `${baseRef}..${tipRef}`, "--stdout", ...excludeArgs()], { cwd });
274287

275288
if (patchContent && patchContent.trim()) {
276289
fs.writeFileSync(patchPath, patchContent, "utf8");
@@ -299,16 +312,25 @@ async function generateGitPatch(branchName, baseBranch, options = {}) {
299312
// error preserves the "incremental" contract that the patch reflects only
300313
// the new commits.
301314
if (!patchGenerated && mode === "incremental") {
302-
debugLog(`Strategy 1 (incremental): format-patch produced no output for ${baseRef}..${branchName} despite ${commitCount} incremental commit(s), refusing to fall through to checkout-base strategies`);
315+
debugLog(`Strategy 1 (incremental): format-patch produced no output for ${baseRef}..${tipRef} despite ${commitCount} incremental commit(s), refusing to fall through to checkout-base strategies`);
303316
return {
304317
success: false,
305-
error: `Cannot generate incremental patch: git format-patch produced no output for ${baseRef}..${branchName} despite ${commitCount} incremental commit(s).`,
318+
error: `Cannot generate incremental patch: git format-patch produced no output for ${baseRef}..${tipRef} despite ${commitCount} incremental commit(s).`,
306319
patchPath: patchPath,
307320
};
308321
}
309322
} catch (branchError) {
310-
// Branch does not exist locally
323+
// Branch does not exist locally (or pinnedSha failed)
311324
debugLog(`Strategy 1: Branch '${branchName}' does not exist locally - ${getErrorMessage(branchError)}`);
325+
if (options.pinnedSha) {
326+
// SECURITY: When pinnedSha is set, fail closed — do not fall through to
327+
// other strategies that would resolve a different commit.
328+
return {
329+
success: false,
330+
error: `Pinned SHA ${options.pinnedSha} failed to generate patch: ${getErrorMessage(branchError)}`,
331+
patchPath: patchPath,
332+
};
333+
}
312334
if (mode === "incremental") {
313335
return {
314336
success: false,
@@ -510,7 +532,7 @@ async function generateGitPatch(branchName, baseBranch, options = {}) {
510532
// of the PR head. Use the merge-base as the effective diff base to exclude those
511533
// merged upstream commits from the size measurement.
512534
let diffBaseForSize = baseCommitSha;
513-
if (mode === "incremental" && baseCommitSha && branchName && defaultBranch) {
535+
if (mode === "incremental" && baseCommitSha && resolvedTipRef && defaultBranch) {
514536
try {
515537
let baseBranchRemoteRef = null;
516538
try {
@@ -527,15 +549,15 @@ async function generateGitPatch(branchName, baseBranch, options = {}) {
527549
// baseCommitSha as the diff base.
528550
let baseIsAncestorOfBranch = false;
529551
try {
530-
execGitSync(["merge-base", "--is-ancestor", "--", baseCommitSha, branchName], { cwd });
552+
execGitSync(["merge-base", "--is-ancestor", "--", baseCommitSha, resolvedTipRef], { cwd });
531553
baseIsAncestorOfBranch = true;
532554
} catch {
533-
// baseCommitSha is not an ancestor of branchName (rebase / force-push)
534-
debugLog(`Strategy 1 (incremental): baseCommitSha ${baseCommitSha} is not an ancestor of ${branchName} (rebase/force-push?); skipping merge-base adjustment`);
555+
// baseCommitSha is not an ancestor of tipRef (rebase / force-push)
556+
debugLog(`Strategy 1 (incremental): baseCommitSha ${baseCommitSha} is not an ancestor of ${resolvedTipRef} (rebase/force-push?); skipping merge-base adjustment`);
535557
}
536558

537559
if (baseIsAncestorOfBranch) {
538-
const mb = execGitSync(["merge-base", "--", baseBranchRemoteRef, branchName], { cwd }).trim();
560+
const mb = execGitSync(["merge-base", "--", baseBranchRemoteRef, resolvedTipRef], { cwd }).trim();
539561
// Check if mb is already an ancestor of baseCommitSha.
540562
// If it is, baseCommitSha is "later" and the agent did NOT merge the default
541563
// branch ahead of the PR head — keep baseCommitSha as the diff base.
@@ -561,15 +583,15 @@ async function generateGitPatch(branchName, baseBranch, options = {}) {
561583
}
562584

563585
let diffSize = null;
564-
if (mode === "incremental" && diffBaseForSize && branchName) {
586+
if (mode === "incremental" && diffBaseForSize && resolvedTipRef) {
565587
diffSize = computeIncrementalDiffSize({
566588
baseRef: diffBaseForSize,
567-
headRef: branchName,
589+
headRef: resolvedTipRef,
568590
cwd,
569591
tmpPath: `${patchPath}.diff.tmp`,
570592
excludedFiles: options.excludedFiles,
571593
});
572-
debugLog(`Final: diffSize=${diffSize ?? "(n/a)"} bytes (baseRef=${diffBaseForSize}..${branchName})`);
594+
debugLog(`Final: diffSize=${diffSize ?? "(n/a)"} bytes (baseRef=${diffBaseForSize}..${resolvedTipRef})`);
573595
}
574596

575597
debugLog(`Final: SUCCESS - patchSize=${patchSize} bytes, patchLines=${patchLines}, diffSize=${diffSize ?? "(n/a)"} bytes, baseCommit=${baseCommitSha || "(unknown)"}`);

‎actions/setup/js/safe_output_type_validator.cjs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -545,6 +545,14 @@ function validateItem(item, itemType, lineNum, options) {
545545
}
546546

547547
const normalizedItem = { ...item };
548+
// SECURITY: Strip infrastructure fields that must only be set by the MCP handler,
549+
// never by the agent. If an agent injects these via NDJSON output, it could bypass
550+
// file-protection policy (patch_path/bundle_path point to attacker-controlled files)
551+
// or circumvent size limits (diff_size).
552+
delete normalizedItem.patch_path;
553+
delete normalizedItem.bundle_path;
554+
delete normalizedItem.base_commit;
555+
delete normalizedItem.diff_size;
548556
const errors = [];
549557

550558
// Run custom validation first if defined

0 commit comments

Comments
 (0)