Repository navigation
Conversation
Make patch and bundle generation local-only, deny explicit and implicit Git transports, and remove generation credential forwarding. Add regression coverage, checkout diagnostics, and compiler threat/TLA+ generation contracts for GHSA-4fwp-8v7m-jhhr. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ 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.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ 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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review result: COMMENT
I found no actionable blocking issues in the changed lines. The implementation consistently routes generator Git commands through the offline guard and removes generation-time fetch/auth forwarding. The specialist pass was unavailable due to tool-permission failure, so this assessment is based on the independent static review of the capped diff and selected source files; no tests or builds were run per review instructions.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~3 AIC · ⌖ ~1 AIC · ⊞ 1.7K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs (security-fix regression focus) and /codebase-design (interface consistency). This is a well-scoped security hardening fix for GHSA-4fwp-8v7m-jhhr: it replaces the fetch-on-missing-ref helper with a strictly local-only check (hasLocalOriginRemoteTrackingRef), removes token forwarding from both generators and handlers, and adds a solid 174-line offline regression suite (git_artifact_offline.test.cjs) that verifies remote helpers never execute, even with a working ext:: helper fixture. Two minor, non-blocking suggestions posted inline.
📋 Key Themes & Highlights
Key Themes
- Regex broadening risk (
git_patch_utils.cjs:282):describeGitFailure's match list was extended with generic words (missing,bad object,unable to read,not allowed) that could cause unrelated errors to get a misleading partial-clone diagnostic appended. - Silently-ignored
options.token: both generators still accept atokenoption (exercised by tests) but it's now a no-op with no warning, which could mask the behavior change for existing callers.
Positive Highlights
- ✅ The offline guard (
GIT_ALLOW_PROTOCOL="",GIT_NO_LAZY_FETCH=1) is applied via a singleexecLocalGitSyncwrapper and theofflineGitOptionsmerge order correctly prevents callerenvoverrides from relaxing it — verified by a dedicated regression test ("cannot relax the transport guard through caller options"). - ✅ Good test design: the new suite exercises a genuinely harmless-but-working remote helper fixture to prove non-execution, rather than only asserting on mocked calls.
- ✅ Fail-closed behavior for missing incremental baselines is correctly preserved (no silent diff-widening), with clear checkout-provisioning guidance in the error messages.
- ✅ Threat-model documentation (
compiler-threat-detection-spec.md,SafeOutputGeneration.tla) is thorough and honestly scoped, explicitly noting the TLA+ model has not yet been checked with TLC.
Neither issue blocks approval; they are worth a quick follow-up.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · ~125 AIC · ⌖ ~20 AIC · ⊞ 10.7K
Comment /matt to run again
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Reply to Ponytail Reviewer completion: No code change is needed because this comment reports completion without an actionable finding. I am not treating |
|
Reply to PR Code Quality Reviewer completion: No additional code change is needed; the associated review found no actionable blocking issues. Its static-review and specialist-tool limitations remain distinct from runtime validation. |
|
Reply to Matt Pocock Skills Reviewer completion: I accepted the two actionable suggestions from the associated review. Their implementation and validation are addressed separately in the diagnostic-matching and legacy-token inline threads. |
|
Reply to Test Quality Sentinel completion: No code change is needed because this status comment includes no concrete test-quality finding. I am not treating its no-op result as evidence that tests passed. |
|
Reply to Design Decision Gate completion: No additional design change or ADR is warranted by this comment, which contains no concrete design objection or ADR draft. The existing local-only generation boundary and separate publication authority remain unchanged. |
There was a problem hiding this comment.
🟡 Changes recommended
Repository-configured Git programs can still execute with the handler’s environment despite transport denial, leaving a critical security gap alongside failing test expectations and baseline-handling regressions.
2 open issues
What changed in this PR
This PR hardens gh-aw’s safe-output patch and bundle generation against repository-controlled Git remote helpers, addressing GHSA-4fwp-8v7m-jhhr through local-only artifact generation.
Changes:
- Applies offline Git guards throughout generation and inspection, and removes generation-token forwarding.
- Rejects missing incremental baselines and adds checkout-provisioning guidance for unavailable refs and objects.
- Adds offline regression coverage, threat-specification contracts, and a bounded TLA+ generation model. TLC verification is not claimed.
| File | Description |
|---|---|
specs/workflow-security/SafeOutputGeneration.tla |
Models local-only generation, failures, and fault controls. |
specs/workflow-security/SafeOutputGeneration.cfg |
Configures generation-model safety invariants. |
specs/workflow-security/check_transport.py |
Adds generation profiles and model selection. |
specs/compiler-threat-detection-spec.md |
Defines the runtime generation-security contracts. |
specs/compiler-threat-detection-changelog.md |
Records specification version 1.0.80. |
docs/src/content/docs/reference/checkout.md |
Documents required checkout preparation. |
actions/setup/js/safe_outputs_handlers.cjs |
Uses offline Git inspection and stops forwarding generation tokens. |
actions/setup/js/git_patch_utils.test.cjs |
Updates partial-clone diagnostic expectations. |
actions/setup/js/git_patch_utils.cjs |
Makes utility Git calls offline and revises diagnostics. |
actions/setup/js/git_patch_integration.test.cjs |
Updates integration coverage for local-only baselines. |
actions/setup/js/git_helpers.cjs |
Adds a local-only execution wrapper and removes tracking-ref fetching. |
actions/setup/js/git_artifact_offline.test.cjs |
Adds remote-helper and missing-object regressions. |
actions/setup/js/generate_git_patch.test.cjs |
Updates filtered-generation failure mocking. |
actions/setup/js/generate_git_patch.cjs |
Removes fetching and adds fail-closed generation errors. |
actions/setup/js/generate_git_bundle.cjs |
Applies offline generation and preserves resolved-base failures. |
Review effort: Max (auto)
Preserve upstream specification 1.0.80 and advance generation specification to 1.0.81. Validation: build and all 353 workflow compilations passed. Final gate encountered unchanged upstream custom-linter findings in pkg/parser/frontmatter_hash.go and macOS executable-path test failures in pkg/fileutil; 726 impacted JavaScript tests passed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use command-local program guards and index-only filtered bundle synthesis. Update generation model runner test coverage and document the separate configured-program boundary. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Reply to #73672 (comment): No code change is requested by this completion status. I am not treating reported_no_action as proof of validation or publication readiness. |
|
Reply to #73672 (comment): Acknowledged the code quality review completion. This status contains no actionable finding and does not establish runtime validation. |
|
Reply to #73672 (comment): The two actionable inline suggestions were addressed separately in commit 0ba7c3f. This completion notice does not require another code change. |
|
Reply to #73672 (comment): No code change is requested by this test analysis status. I am not treating reported_no_action as evidence that tests passed. |
|
Reply to #73672 (comment): This completion status contains no concrete design objection or ADR request, so no additional change is warranted from this notice. It does not establish formal TLC validation or publication readiness. |
|
Reply to #73672 (comment): Agreed; this completion notice does not require a code change. Its reported_no_action outcome remains separate from validation and publication evidence. |
|
Reply to #73672 (comment): Acknowledged; this follow-up requests no additional code change. Static-review outcomes and specialist-tool limitations remain distinct from runtime validation. |
|
Reply to #73672 (comment): The diagnostic-matching and legacy-token suggestions were addressed separately in commit 0ba7c3f. This follow-up requires no additional change and does not establish a complete security fix. |
|
Reply to #73672 (comment): Agreed; this status contains no concrete test finding to address. Passing tests must be established by test results, not the no-op outcome. |
Ran pr-finisher at |
|
Warning threat detection engine error What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. PR Finisher
Sous-chef head: 7c58248
|
|
@copilot resolve the merge conflicts on this branch. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Resolved and pushed in merge commit |
|
Warning threat detection engine error What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. PR Finisher
Sous-chef head: 8576722
|
|
Warning threat detection engine error What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. PR Finisher
|
|
Commit pushed:
|
PR Finisher
|
|
Commit pushed:
|
PR FinisherAddressed the security review thread on Validation: a Node smoke test of the new logic passed. vitest and Remaining:
|
|
@copilot fix ci failures |
|
PR finisher dispatch requested for the following outstanding work:
General fix: treat env-provided roots as untrusted input and constrain them to approved base directories before scanning. Best fix here without changing core functionality: In actions/setup/js/validate_recovery_envelopes.cjs, compute canonical absolute base roots from the trusted roots argument, then accept GH_AW_RECOVERY_ADDITIONAL_ROOTS entries only if their canonical real path is equal to or under one of those trusted roots. Reject (throw) on invalid/non-string entries (already present) and on disallowed roots. Then scan only trusted defaults plus validated additional roots. - #73672 (comment)
The dedicated pr-finisher workflow will apply the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not trigger CI. Sous-chef head: d86e541
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed and pushed in |
|
@copilot resolve the merge conflicts on this branch. |
PR Finisher: no changes submittedThe PR conflicts with Hand-off:
No validation was run.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Resolved and pushed in merge commit Validation: all 355 workflows recompiled; four changed workflow sources passed compilation validation. Build, standard Go lint, JavaScript lint, schema checks, impacted Go tests, and secret scanning passed. Incoming JavaScript suites: 1,997 passed, three skipped; PR-finisher tests: 81 passed. The full gate is not green: custom Go lint reports the exact same 176 pre-merge diagnostics, and two impacted JavaScript fixtures timed out (1,062 others passed). Both timed-out cases passed isolated reruns with unchanged timeouts. Security analysis timed out without a verdict. Final working tree is clean; the PR itself was not merged. |
Pin the additional-root list on every validation step, including an empty list, and pass it explicitly rather than trusting inherited job environment. Reject roots and symlink traversals outside the canonical artifact boundary. Keep experimental drive-memory mounts unsupported and fail closed instead of authorizing unredacted external packs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Safe-output Git artifact generation stays offline and disables repository-configured programs. Incomplete checkouts retain a monorepo-aware compatibility path: package selected local change objects in the agent, then recover the required trusted history and baseline data in an isolated safe-outputs publisher.
Security and recovery design
GIT_ALLOW_PROTOCOL=""andGIT_NO_LAZY_FETCH=1. Disable external diff/textconv, hooks, fsmonitor, signing, maintenance, and conversion drivers command-locally. Remove generation token forwarding and authorization-header injection.Compatibility and limits
Missing data never authorizes fetching from the agent checkout. Eligible non-staged PR outputs can return
result: "queued"/recovery: "object-pack"; queued is not published. Recovery must independently authenticate, reconstruct, and pass normal publisher policies.Recovery preserves full/incremental outputs, bundle and explicit
amtransport, binary contents, executable modes, symlinks, gitlinks, and filtered resolved-tree synthesis. Unfiltered bundles preserve merge topology. Existing unfilteredamlimitations remain: conflicting merge-history replay fails closed, aborts, and restores the baseline rather than publishing partial commits.The 100,000-object / 64 MiB uncompressed packaging limits apply to selected change objects, not the entire monorepo. The pack/import limit is 80 MiB. Trusted ancestry search uses depths 1, 16, 64, 256, 1,024, 4,096, 16,384, and 65,536, with 60-second command deadlines and a shared 180-second network-operation deadline. Tree inspection is bounded to one million entries; required baseline blob hydration is bounded to 64 MiB, checked after each transferred batch. These are not a network-byte budget.
Dynamic PR updates without a trusted event baseline, staged outputs, oversized selected changes, exhausted recovery budgets, unsupported filtering, or missing agent-created bytes still require checkout provisioning. No patched release or deployed remediation is claimed.
Validation and limitations
am, actual local publisher pushes, exact baselines, merge behavior, exclusions, binary/mode/symlink/gitlink changes, hostile configuration, bounds, tampering, and cleanup. New cases cover depth-one PR merges and execute both manual recovery command sequences, checking final trees, configured exclusions, remote updates, and immutable uploads.origin/main; unrelated lint cleanup was not mixed into this security fix.Changeset
✨ PR Review Safe Output Test - Run 41320035197
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
update.googleapis.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
✨ PR Review Safe Output Test - Run 41390024653
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
update.googleapis.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.