fix: prefix quoted git patch headers in PR bridge#3122
fix: prefix quoted git patch headers in PR bridge#3122robert-inkeep wants to merge 8 commits intomainfrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
I could not sync this public PR into Patch application failed. The diff could not be applied cleanly. Please rebase your PR on the latest main. |
Security fix for quoted git patch path bypass in PR bridgeThe Key changes
Test suite — A new Files
|
There was a problem hiding this comment.
Medium severity — the fix addresses the real bypass but introduces a crash on mixed-quote rename diffs, and has a minor robustness gap in the ---/+++ handlers.
Claude Opus ｜ 𝕏
| if (match[1] && match[3]) { | ||
| const fromPath = prefixedPath(`"${match[1]}"`).replace(/^"|"$/g, ''); | ||
| const toPath = prefixedPath(`"${match[3]}"`).replace(/^"|"$/g, ''); | ||
| return `diff --git "a/${fromPath}" "b/${toPath}"`; | ||
| } | ||
|
|
||
| return `diff --git a/${prefixedPath(match[2])} b/${prefixedPath(match[4])}`; |
There was a problem hiding this comment.
Bug: TypeError on mixed-quote rename diffs. Git independently quotes each side of the diff --git header. A rename from normal.txt to file with spaces.txt produces:
diff --git a/normal.txt "b/file with spaces.txt"
The regex correctly captures this — match[2]="normal.txt", match[3]="file with spaces.txt" — but the branching logic only handles two of the four cases:
| a-side | b-side | match[1] |
match[2] |
match[3] |
match[4] |
Branch | Result |
|---|---|---|---|---|---|---|---|
| unquoted | unquoted | - | ✓ | - | ✓ | else | OK |
| quoted | quoted | ✓ | - | ✓ | - | if | OK |
| unquoted | quoted | - | ✓ | ✓ | - | else | prefixedPath(undefined) → crash |
| quoted | unquoted | ✓ | - | - | ✓ | if | match[3] is falsy → falls to else → crash |
Suggested fix: resolve each side independently:
const fromRaw = match[1] ?? match[2];
const toRaw = match[3] ?? match[4];
const fromQuoted = match[1] !== undefined;
const toQuoted = match[3] !== undefined;
const fromPath = fromQuoted
? prefixedPath(`"${fromRaw}"`).replace(/^"|"$/g, '')
: prefixedPath(fromRaw);
const toPath = toQuoted
? prefixedPath(`"${toRaw}"`).replace(/^"|"$/g, '')
: prefixedPath(toRaw);
const aPrefix = fromQuoted ? '"a/' : 'a/';
const aSuffix = fromQuoted ? '"' : '';
const bPrefix = toQuoted ? '"b/' : 'b/';
const bSuffix = toQuoted ? '"' : '';
return `diff --git ${aPrefix}${fromPath}${aSuffix} ${bPrefix}${toPath}${bSuffix}`;This handles all four combinations without branching on which capture groups matched.
| if (line.startsWith('--- ')) { | ||
| if (line === '--- /dev/null') { | ||
| return line; | ||
| } | ||
| if (line.startsWith('--- a/')) { | ||
| return `--- a/${prefixedPath(line.slice(6))}`; | ||
| } | ||
| if (line.startsWith('--- "a/')) { | ||
| const match = line.match(/^--- "a\/(.+)"$/); | ||
| if (match) { | ||
| const nextPath = prefixedPath(`"${match[1]}"`).replace(/^"|"$/g, ''); | ||
| return `--- "a/${nextPath}"`; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Robustness gap: If a --- line doesn't match /dev/null, a/, or "a/, it falls through silently to the next if block. Because this is a security-sensitive function (prevents writes outside the mirror boundary), consider throwing on unrecognized ---/+++ formats rather than letting them pass through:
if (line.startsWith('--- ')) {
if (line === '--- /dev/null') return line;
if (line.startsWith('--- a/')) return `--- a/${prefixedPath(line.slice(6))}`;
if (line.startsWith('--- "a/')) {
const match = line.match(/^--- "a\/(.+)"$/);
if (match) {
const nextPath = prefixedPath(`"${match[1]}"`).replace(/^"|"$/g, '');
return `--- "a/${nextPath}"`;
}
}
throw new Error(`Unrecognized --- header format: ${line}`);
}Same for the +++ handler. A fail-closed approach is safer here — an unexpected format silently passing through is exactly the class of bug this PR is fixing.
There was a problem hiding this comment.
PR Review Summary
(4) Total Issues | Risk: High
🟠⚠️ Major (2) 🟠⚠️
Inline Comments:
- 🟠 Major:
bridge-public-pr-to-monorepo.mjs:105-111Mixed quoted/unquoted path combinations not handled - potential bypass - 🟠 Major:
bridge-public-pr-to-monorepo.mjs:113-127Unrecognized---/+++line patterns fall through unchanged - potential bypass
🧹 While You're Here (1) 🧹
🧹 1) bridge-public-pr-to-monorepo.mjs:143-154 Rename/copy headers also need quoted path handling
Issue: The rename from, rename to, copy from, and copy to headers at lines 143-154 use line.slice() directly without checking for or handling quoted paths.
Why: Git can quote these paths too when they contain special characters (tabs, newlines, etc.). This creates an inconsistency where the fix addresses quoted paths for diff --git, ---, and +++ lines but not for rename/copy lines, leaving a potential bypass vector for renamed files with special characters.
Fix: Apply the same quoted-path detection pattern used for --- "a/ to the rename/copy lines:
if (line.startsWith('rename from ')) {
const content = line.slice('rename from '.length);
if (content.startsWith('"') && content.endsWith('"')) {
const match = content.match(/^"(.+)"$/);
if (match) {
const nextPath = prefixedPath(`"${match[1]}"`).replace(/^"|"$/g, '');
return `rename from "${nextPath}"`;
}
}
return `rename from ${prefixedPath(content)}`;
}Refs: bridge-public-pr-to-monorepo.mjs:143-154
💭 Consider (2) 💭
💭 1) bridge-public-pr-to-monorepo.mjs:82-88 C-style escape sequences in quoted paths could bypass traversal check
Issue: Git uses C-style quoting for special characters. A path like "a/foo\056\056/bar" contains octal escapes for .. (\056 = .) which would bypass the literal . and .. segment check on line 86.
Why: This is an edge case attack vector. The unquoted variable only strips outer quotes but doesn't decode escape sequences before validation.
Fix: Decode C-style escape sequences (\t, \n, \\, \ooo octal, \xNN hex) before splitting into segments for traversal validation.
Refs: Git diff quoting
💭 2) bridge-public-pr-to-monorepo.mjs:75-158 No unit tests for critical security-sensitive function
Issue: The prefixPatchPaths function is security-critical and now significantly more complex. The PR mentions manual testing, but there are no automated tests.
Why: Increases risk of regressions. The function is already exported (line 595), making it testable.
Fix: Consider adding a test file with cases for: unquoted paths, quoted paths, mixed quoting, /dev/null, path traversal rejection, escaped characters.
🚫 REQUEST CHANGES
Summary: This PR is a good start at fixing the quoted path bypass vulnerability, but the fix is incomplete. The two Major findings identify remaining bypass vectors: (1) mixed quoted/unquoted path combinations are not handled in the diff header regex logic, and (2) unrecognized ---/+++ line patterns silently pass through without prefixing. Both could allow an attacker to write files outside the mirror boundary. I recommend addressing these before merging to ensure the security fix is complete.
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
bridge-public-pr-to-monorepo.mjs:113-127 |
Implicit undefined return | Merged with fallthrough bypass finding - same root cause |
bridge-public-pr-to-monorepo.mjs:128-142 |
+++ fallthrough bypass |
Merged with --- fallthrough finding - called out in inline comment refs |
bridge-public-pr-to-monorepo.mjs:98-111 |
DevOps duplicate of mixed quoting | Merged with primary appsec/standards finding |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-appsec |
4 | 0 | 1 | 0 | 2 | 0 | 1 |
pr-review-standards |
3 | 0 | 0 | 1 | 1 | 0 | 1 |
pr-review-devops |
3 | 0 | 1 | 0 | 0 | 0 | 2 |
| Total | 10 | 0 | 2 | 1 | 2 | 0 | 3 |
Note: Findings were deduplicated across reviewers; all three independently identified the mixed quoting and fallthrough issues.
| const toPath = prefixedPath(`"${match[3]}"`).replace(/^"|"$/g, ''); | ||
| return `diff --git "a/${fromPath}" "b/${toPath}"`; | ||
| } | ||
|
|
||
| return `diff --git a/${prefixedPath(match[2])} b/${prefixedPath(match[4])}`; | ||
| } | ||
| if (line.startsWith('--- a/')) { | ||
| return `--- a/${prefixedPath(line.slice(6))}`; | ||
| if (line.startsWith('--- ')) { |
There was a problem hiding this comment.
🟠 MAJOR: Mixed quoted/unquoted path combinations not handled
Issue: The regex captures four groups for quoted/unquoted combinations, but the logic only handles two cases: both quoted (match[1] && match[3]) or both unquoted (fallthrough to line 111). A malicious patch like diff --git "a/file.md" b/../../etc/passwd would have match[1] set but match[4] set (not match[3]), causing the regex to match but the conditional logic to fall through incorrectly.
Why: This creates a security bypass vector. An attacker could craft a diff with mixed quoting to have one path processed and the other pass through without the mirror prefix, potentially writing files outside the mirror boundary.
Fix: Handle all four combinations explicitly:
const fromPath = match[1] ?? match[2];
const toPath = match[3] ?? match[4];
const fromQuoted = Boolean(match[1]);
const toQuoted = Boolean(match[3]);
const prefixedFrom = prefixedPath(fromQuoted ? `"${fromPath}"` : fromPath).replace(/^"|"$/g, '');
const prefixedTo = prefixedPath(toQuoted ? `"${toPath}"` : toPath).replace(/^"|"$/g, '');
const formatPath = (path, quoted) => quoted ? `"${path}"` : path;
return `diff --git ${formatPath(`a/${prefixedFrom}`, fromQuoted)} ${formatPath(`b/${prefixedTo}`, toQuoted)}`;Refs:
| return line; | ||
| } | ||
| if (line.startsWith('--- a/')) { | ||
| return `--- a/${prefixedPath(line.slice(6))}`; | ||
| } | ||
| if (line.startsWith('--- "a/')) { | ||
| const match = line.match(/^--- "a\/(.+)"$/); | ||
| if (match) { | ||
| const nextPath = prefixedPath(`"${match[1]}"`).replace(/^"|"$/g, ''); | ||
| return `--- "a/${nextPath}"`; | ||
| } | ||
| } | ||
| } | ||
| if (line.startsWith('+++ b/')) { | ||
| return `+++ b/${prefixedPath(line.slice(6))}`; | ||
| if (line.startsWith('+++ ')) { | ||
| if (line === '+++ /dev/null') { |
There was a problem hiding this comment.
🟠 MAJOR: Unrecognized --- line patterns fall through unchanged
Issue: Lines starting with --- that don't match the three handled patterns (/dev/null, a/..., "a/...") fall through without any explicit return statement at the end of the if (line.startsWith('--- ')) block. This means they pass through unchanged to the final return line at the bottom of the map callback.
Why: For a security fix, unknown variants passing through silently is dangerous. An attacker could craft a novel format (e.g., --- README.md without a/ prefix, or single-quoted --- 'a/file') to bypass path prefixing. The same issue exists for the +++ block at lines 128-142, which is even more critical since +++ determines the actual file creation path.
Fix: Add explicit handling for unrecognized patterns. Either:
- Strict (recommended for security): Throw an error for unrecognized formats
- Defensive: Log a warning and apply a safe default
if (line.startsWith('--- ')) {
if (line === '--- /dev/null') return line;
if (line.startsWith('--- a/')) return `--- a/${prefixedPath(line.slice(6))}`;
if (line.startsWith('--- "a/')) {
const match = line.match(/^--- "a\/(.+)"$/);
if (match) {
const nextPath = prefixedPath(`"${match[1]}"`).replace(/^"|"$/g, '');
return `--- "a/${nextPath}"`;
}
}
// Reject unrecognized --- patterns for security
throw new Error(`Rejecting patch with unrecognized --- format: ${line}`);
}Refs:
- bridge-public-pr-to-monorepo.mjs:128-142 — same issue in
+++block
Preview URLsUse these stable preview aliases for testing this PR:
These point to the same Vercel preview deployment as the bot comment, but they stay stable and easier to find. Raw Vercel deployment URLs
|
|
@claude please address your and pullfrog comments |
|
@claude (inlower case) please fix comments from u and pull frog |
|
Claude finished @vnv-varun's task in 1m 34s —— View job Fixing review comments from Claude and Pullfrog
Changes made1. Mixed quoted/unquoted path handling — Rewrote the 2. Fail-closed 3. Rename/copy headers — These already pass through Note: the "while you're here" suggestion from Claude about rename/copy quoted paths is already handled — |
- Handle all four quoted/unquoted combinations in diff --git headers (previously only both-quoted or both-unquoted worked; mixed combos crashed) - Throw on unrecognized --- and +++ header formats instead of silently passing them through (fail-closed for security) - Rename/copy headers already go through prefixedPath which handles quotes Co-authored-by: Varun Varahabhotla <[email protected]> Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
@claude rerun PR checks |
|
Claude finished @vnv-varun's task in 1m 5s —— View job Rerunning PR Checks
Pushed an empty commit ( |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
✅ Prior Major Issues Resolved
The two Major issues from the prior review have been correctly addressed:
-
Mixed quoted/unquoted path combinations — Fixed via commit
11d09efec. Lines 103-106 now usematch[1] ?? match[2]andmatch[3] ?? match[4]to correctly extract paths regardless of quoting combination. ThefromQuoted/toQuotedflags preserve original quoting in output. -
Unrecognized
---/+++fallthrough bypass — Fixed via commit11d09efec. Lines 134 and 150 now throwErrorfor unrecognized header formats, implementing fail-closed behavior as recommended.
🕐 Pending Recommendations (3)
Items from the prior review that remain unaddressed:
- 🧹
bridge-public-pr-to-monorepo.mjs:152-163Rename/copy headers don't handle quoted paths — same potential bypass vector exists forrename from,rename to,copy from,copy tolines - 💭
bridge-public-pr-to-monorepo.mjs:82-88C-style escape sequences (e.g.,\056\056for..) could bypass traversal check - 💭
scopeNo unit tests for security-critical function
💡 APPROVE WITH SUGGESTIONS
Summary: The two Major security bypass vectors identified in the prior review have been correctly fixed. The mixed-quote path handling now properly extracts paths from all four quoting combinations, and unrecognized ---/+++ formats now throw errors instead of passing through. The PR is safe to merge — the remaining pending items (rename/copy quoted paths, C-style escapes, missing tests) are opportunistic improvements that can be addressed in follow-up work if desired.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-appsec |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: This is a re-review. The appsec reviewer confirmed prior fixes are correctly implemented and found no new issues in the delta.
|
@claude can u address these too: Items from the prior review that remain unaddressed: 🧹 #3122 (review) Rename/copy headers don't handle quoted paths — same potential bypass vector exists for rename from, rename to, copy from, copy to lines |
|
Claude finished @vnv-varun's task in 3m 39s —— View job Addressing remaining review items
Changes made1. C-style escape sequence decoding — Added 2. Rename/copy quoted paths — Verified these are already handled correctly. The 3. Unit tests — Added comprehensive Vitest test suite (
Note: I was unable to run the tests locally due to sandbox permission restrictions, but CI will validate them. |
- Add decodeCStyleEscapes() to handle Git's C-style quoting (\ooo octal, \xNN hex, \t, \n, etc.) before path traversal validation, preventing bypass via encoded ".." segments (e.g., \056\056 or \x2e\x2e) - Add comprehensive Vitest test suite for prefixPatchPaths covering: unquoted/quoted/mixed paths, /dev/null, path traversal rejection, C-style escape bypass, rename/copy headers, fail-closed behavior - Note: rename/copy headers already handled correctly via prefixedPath() Co-authored-by: Varun Varahabhotla <[email protected]> Co-Authored-By: Claude Opus 4.6 <[email protected]>
Motivation
diff --git "a/...") to escape the configured mirror prefix and write files to the repo root, enabling public PRs to place files outside the mirror boundary.Description
prefixPatchPathsin.github/scripts/bridge-public-pr-to-monorepo.mjsto recognize and rewrite quoteddiff --githeaders as well as unquoted ones.--- "a/..."and+++ "b/..."header forms while preserving the special-case/dev/nullbehavior.Testing
prefixPatchPathswith a quoted-path patch containing an escaped tab in the filename and verified the output is prefixed underpublic/agents/....README.md) and verified unquoted headers remain correctly prefixed.Codex Task