Repository navigation
fix: confine asset uploads to verified regular files - #73587
Conversation
Reject asset source and parent symlinks, hard links, special files, and prefix lookalikes. Pin directory traversal on Linux, bind reads to verified descriptors, and stage/publish the exact bounded bytes that were hashed using exclusive destination creation. Harden legacy and Claim publishers and add harmless sentinel and race regressions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
There was a problem hiding this comment.
Request changes
The macOS implementation still performs writes through an unpinned absolute path, allowing an ancestor swap to create a file outside the permitted root before post-operation validation detects the change. See the inline comment on symlink_guard.cjs:106.
Static review only; tests were not run.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~2 AIC · ⌖ ~1 AIC · ⊞ 1.7K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd (per pr-triage: bug_fix). This is a well-reasoned, well-tested CWE-59 symlink-following fix — the TOCTOU-safe primitives in symlink_guard.cjs (descriptor-pinned traversal, nlink===1 enforcement, O_NOFOLLOW/O_EXCL) and the regression suite covering symlinks, hardlinks, dangling links, and race-window swaps are solid. No blocking issues found; leaving a few non-blocking suggestions.
📋 Key Themes & Highlights
Key Themes
- Error-code consistency: new
symlink_guard.cjsthrows raise bareError(...)without the codebase'sERR_*prefix convention (error_codes.cjs), making them harder to classify at call sites/monitoring compared to other modules. - Edge-case test gap: the new existing-destination content check in
upload_assets.cjsisn't wrapped in its own try/catch — worth confirming a size-mismatch failure there surfaces a clean message rather than an unhandled exception, and adding a regression test for it. - Possible Windows coverage gap: the PR description and tests only discuss Linux/macOS behavior for the new guarded-path primitives; I couldn't fully confirm Windows semantics or whether CI exercises this path there — flagged as a question, not a confirmed bug.
Positive Highlights
- ✅ Excellent TOCTOU regression coverage (race-window file swaps, dangling/hard links, snapshot consistency)
- ✅ The
isWithinclosure replacing the oldstartsWithpath-prefix check insafe_outputs_handlers.cjsfixes a real path-prefix bypass (e.g./workspace-siblingno longer false-matching/workspace) — nice incidental hardening - ✅ Existing-destination content verification in
upload_assets.cjscloses a prior silent-acceptance gap
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · ~165 AIC · ⌖ ~20 AIC · ⊞ 10.7K
Comment /matt to run again
There was a problem hiding this comment.
🟡 Changes recommended
A macOS symlink swap-and-restore race can bypass directory verification, allowing guarded operations to read or create files outside the guarded directory.
2 open issues
What changed in this PR
This PR hardens gh-aw’s safe-output asset pipeline against unsafe file paths and source replacement during staging and publication.
Changes:
- Add bounded, descriptor-verified regular-file reads and exclusive no-follow writes.
- Use the verified bytes for hashing and staging, and guard both legacy and Claim-scoped publication.
- Add security regression tests and document file acceptance requirements.
| File | Description |
|---|---|
docs/src/content/docs/reference/safe-outputs.md |
Documents accepted file types and snapshot protections. |
actions/setup/js/work_queue_upload_assets.cjs |
Uses guarded reads for Claim-scoped staged assets. |
actions/setup/js/work_queue_git_adapter_parity.test.cjs |
Tests rejection of linked Claim assets before publication. |
actions/setup/js/upload_assets.test.cjs |
Adds source, destination-link, and snapshot-consistency tests. |
actions/setup/js/upload_assets.cjs |
Guards publication and verifies existing destination content. |
actions/setup/js/symlink_guard.test.cjs |
Tests bounded reads, path swaps, exclusive writes, and descriptor cleanup. |
actions/setup/js/symlink_guard.cjs |
Adds directory traversal checks and guarded file-access helpers. |
actions/setup/js/safe_outputs_handlers.test.cjs |
Tests path boundaries, unsafe links, and staging consistency. |
actions/setup/js/safe_outputs_handlers.cjs |
Tightens workspace containment and stages verified file snapshots. |
Review effort: Max (auto)
Fail closed outside Linux instead of using unpinned paths. Preserve coded validation failures, distinguish existing-destination I/O errors, and cover ancestor swap-and-restore and destination size mismatches. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Response to Test Quality Sentinel: Accepted as an informational status notice, not evidence of verification; no concrete test change is requested. The review fixes were independently validated by the final repository gate with 488 passing tests. |
|
Response to Design Decision Gate: Accepted as an informational status notice; this comment contains no ADR draft or requested design change. No additional code or documentation change is warranted by the notice. |
|
Response to Ponytail Reviewer: Accepted as an informational status notice, with the caveat that reported_no_action does not verify completion. No concrete change is requested here, so no additional implementation change was made. |
|
Response to Matt Pocock Skills Reviewer: Accepted as a completion notice requiring no separate code change. The substantive inline feedback was addressed in 944aa5d, with replies posted and the corresponding threads resolved. |
|
Response to PR Code Quality Reviewer: Accepted as a completion notice requiring no separate code change. The substantive ancestor-swap feedback was addressed in 944aa5d by failing closed outside Linux, and the inline thread was answered and resolved. |
|
The five original status notices already have individual linked responses. This repeated batch also includes those responses themselves; none introduces new review feedback.
No new code changes, commits, or push are needed for this batch. Existing responses are reused for the original notices rather than duplicated, and these acknowledgements should not be treated as new actionable review requests. |
Motivation
Addresses code scanning alert 1039 (CWE-59). Agent-controlled upload paths could follow symlinks into files outside the allowed workspace or
/tmp, and separate reads for hashing and copying allowed source replacement between validation and staging.Changes
/proc/self/fd; guarded asset access fails closed on macOS and Windows rather than using unsafe absolute-path revalidation.Validation
make agent-report-progresspassed after the review fixes, including the gh-aw build, scoped lint, JavaScript typecheck, and 488 tests across four suites. Lint reported warnings but no errors.Local validation ran on macOS. Tests model Linux descriptor-relative lookup with real file I/O; native Linux execution is not verified by that simulation. Native non-Linux asset access is intentionally unsupported and tested to fail closed. No workflow was dispatched and no scanning alert was dismissed.