Repository navigation
fix: keep staged safe-output assets runner-owned - #74171
Conversation
Apply the existing built-in MCP runner identity policy to safeoutputs so private staged assets remain readable by host artifact uploads. Preserve restrictive modes, guarded file access and explicit user overrides; cover startup wiring and private Claim directories. 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.
|
|
β 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.
|
|
π§ Matt Pocock Skills Reviewer has completed the skills-based review. β Warning threat detection engine error What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
β Test Quality Sentinel completed test quality analysis. Warning threat detection engine error What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. 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.
|
|
β PR Code Quality Reviewer completed the code quality review. Warning threat detection engine error What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
Version the safe outputs and compiler threat specifications, define runner-owned private asset staging and retained guards, and map runtime regression evidence without adding a compiler rejection or lock migration. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Warning threat detection engine error What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Request changes
The runner-identity policy does not cover the accepted safe-outputs backend alias. Configurations using that key can still create root-owned staged assets and hit the EACCES failure this PR targets. I left an inline comment with the exact fix and requested regression coverage.
π Code quality review by PR Code Quality Reviewer Β· copilot Β· gpt50 Β· ~2 AIC Β· β 1.7K
Comment /review to run again
| }); | ||
| if (!servers || typeof servers !== "object") return; | ||
| for (const [name, server] of Object.entries(servers)) { | ||
| if (name !== "agenticworkflows" && name !== "safeoutputs") continue; |
There was a problem hiding this comment.
The identity policy misses the safe-outputs alias that this launcher already accepts, so configurations using that key still run without --user/HOME=/tmp and can reproduce the root-owned staged-asset EACCES failure this PR is meant to fix.
π‘ Handle both accepted names
waitForSafeOutputsReadiness recognizes both safeoutputs and safe-outputs (line 269), but this allow-list recognizes only safeoutputs. Include safe-outputs here, or normalize the server name once before applying the policy, and add a regression test for the alias.
There was a problem hiding this comment.
π΅ Needs a closer look
The container identity changes still need native Linux/Docker validation of non-root backend startup and host artifact readability/upload, since macOS validation skipped the filesystem regression.
No open issues π
What changed in this PR
This PR extends gh-awβs runner-identity policy to the safeoutputs backend so host artifact uploads can read privately staged assets without weakening the existing file guards.
Changes:
- Defaults both built-in backends to the runnerβs UID:GID and container-local
HOME, preserving explicit overrides. - Adds identity-policy, startup-wiring, and Linux filesystem regression coverage.
- Updates specifications and documentation to define the ownership contract and its verification limits; live container-to-host recovery remains unverified.
| File | Description |
|---|---|
specs/βcompiler-threat-detection-spec.md |
Defines the runtime staging ownership boundary and maps supporting evidence. |
specs/βcompiler-threat-detection-changelog.md |
Records the ownership contract in version 1.0.91. |
docs/βsrc/βcontent/βdocs/βspecs/βsafe-outputs-specification.md |
Specifies private staging access and transport requirements in version 1.29.15. |
docs/βsrc/βcontent/βdocs/βreference/βmcp-gateway.md |
Documents built-in backend identity and HOME defaults. |
actions/βsetup/βjs/βstart_mcp_gateway.test.cjs |
Covers both built-ins, overrides, startup wiring, and private asset readability. |
actions/βsetup/βjs/βstart_mcp_gateway.cjs |
Applies the shared runner-identity policy to safeoutputs before configuration serialization. |
Review effort: Balanced (auto)
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review π§
Applied /diagnosing-bugs, /tdd, and /codebase-design. Requesting changes because this breaks Windows gateway startup for windows.lock.yml.
π Key Themes and Highlights
Key Themes
- Windows regression (blocking):
applyBuiltinMCPRunnerIdentitynow coverssafeoutputs. Onwindows-latest,process.getuidandprocess.getgidare undefined, so the gateway throwsRunner identity is required for the safeoutputs container.windows.lock.ymlrenders asafeoutputscontainer in itsagentjob onwindows-latest. - Test gap: the rejection table never covers a platform with no UID or GID, which is the path the Windows regression hits.
- Linux filesystem test scope: it checks mode bits under the runner UID and GID, not the container-to-host ownership boundary the PR targets.
- Docs:
docs/src/content/docs/reference/tools.md(about line 203) still describes the default only foragentic-workflows. It is outside this diff, so it was not commented inline. Update it in this PR or a follow-up.
Positive Highlights
- The rename to
applyBuiltinMCPRunnerIdentityis consistent across source, tests, and exports. - Explicit container
--userand configuredHOMEvalues are preserved, and-uparsing covers Docker attached-username forms.
Verification Limits
npm cifailed in this sandbox with a TLS error, so vitest was not run. Findings come from static analysis and a simulated non-POSIX process, which reproduced the throw.
@copilot please address the review comments above.
π§ Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer Β· copilot Β· routed: haiku55 xhigh Β· ~7 AIC Β· β 5.4K
Comment /matt to run again
| ])("rejects invalid runner identities (%s, %s)", (uid, gid) => { | ||
| const backend = { container: "alpine:3.24" }; | ||
| expect(() => applyAgenticWorkflowsRunnerIdentity({ mcpServers: { agenticworkflows: backend } }, uid, gid)).toThrow("Runner identity is required"); | ||
| expect(() => applyBuiltinMCPRunnerIdentity({ mcpServers: { [name]: backend } }, uid, gid)).toThrow("Runner identity is required"); |
There was a problem hiding this comment.
[/tdd] The rejection table covers NaN, Infinity, and negative IDs, but not the case where the platform provides no IDs. That is the path the new safeoutputs default reaches on Windows. Passing undefined does not exercise it, because default parameters replace it with the real process.getuid?.() value.
π‘ Suggested test
Stub process.getuid and process.getgid to undefined (for example with vi.spyOn, restored in afterEach), then call applyBuiltinMCPRunnerIdentity with a safeoutputs container. Assert the outcome matches the fix chosen for the Windows regression, and that the backend config is not partially mutated.
@copilot please address this.
| expect(safeoutputs.env).toEqual({ HOME: "/tmp" }); | ||
| }); | ||
|
|
||
| it.skipIf(process.platform !== "linux")("keeps guarded assets and private Claim directories readable by the host uploader", () => { |
There was a problem hiding this comment.
[/codebase-design] This test checks the mode bits that writeGuardedFile produces, but it runs as a child process under the runner UID and GID. It does not exercise the container-to-host ownership boundary this PR is meant to fix, so passing here does not prove the host uploader can read the assets.
π‘ Suggested clarification
State this limit in the test name or a short comment so future readers do not assume container ownership is covered. Where Docker is available, add a Docker-backed check for the container-owned path.
@copilot please address this.
| // Docker parses any suffix after -u as a username or UID, including -uroot. | ||
| if (args.some(arg => arg === "--user" || arg === "-u" || arg.startsWith("--user=") || (arg.startsWith("-u") && !arg.startsWith("--")))) continue; | ||
| if (!Number.isSafeInteger(uid) || !Number.isSafeInteger(gid) || uid === undefined || gid === undefined || uid < 0 || gid < 0) { | ||
| throw codedError(`Runner identity is required for the ${name} container`, ERR_CONFIG); |
There was a problem hiding this comment.
[/diagnosing-bugs] Windows regression: applyBuiltinMCPRunnerIdentity now applies to safeoutputs. On windows-latest, process.getuid and process.getgid are undefined, so this throws Runner identity is required for the safeoutputs container. windows.lock.yml renders a safeoutputs container in its agent job on windows-latest, so that workflow now fails at gateway startup. Before this PR only agenticworkflows reached this branch, and that container never appears in Windows lock files.
π‘ Suggested fix
When uid or gid is unavailable (non-POSIX), skip the identity default for safeoutputs instead of throwing, or gate the default on process.platform !== "win32". Keep the throw for explicitly invalid IDs so the existing rejection tests still pass. Add a regression test that removes process.getuid and process.getgid and asserts the Windows path does not throw.
@copilot please address this.
|
Warning threat detection engine error What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. PR FinisherPushed a fix for review feedback: the runner-identity policy now also covers the Validation: Remaining: the container-to-host ownership test (thread on test line 484) needs Linux Docker and is left for a human; threads to be resolved after the push is published.
|
|
π This pull request is included in a new release. Release: |
Summary
Seven scheduled workflows failed asset publication because
safeoutputscreated private staged PNGs as the container's default root user. The host artifact uploader then failed withEACCESduring ZIP creation; missing-artifact andPush assetsfailures were downstream cascades. This addresses the ownership mismatch exposed by the guarded staging change in #73587 without weakening its protections.Extend the existing built-in MCP runner-identity policy to
safeoutputs, alongsideagenticworkflows, before gateway configuration serialization. Both default to the runner's UID:GID and a writable, container-localHOME. Explicit user and HOME overrides, other backends, credentials, mounts, and restrictive0600/0700modes remain unchanged.Add regression coverage for both built-ins, startup wiring, explicit overrides, and guarded asset/Claim-directory readability. Document the identity defaults and update Safe Outputs to version 1.29.15 and the compiler threat specification to version 1.0.91, with synchronized histories. The specifications define the private staging/access contract, retain asset guards, distinguish producer failures from publication cascades, and map implementation and evidence limits. This is a supporting runtime boundary, not a new compiler threat rule.
No workflow source or generated lock changes are needed because the fix is in the shared runtime launcher.
Validation
make fmt-cjs && make lint-cjspassed.npm run typecheck --prefix actions/setup/js --silentpassed.TMPDIR=<canonical private session directory> make agent-report-progresspassed: 212 startup tests passed; the Linux-only filesystem regression was skipped on macOS.cd actions/setup/js && npm run test:js -- --no-file-parallelism symlink_guard.test.cjs safe_outputs_handlers.test.cjs upload_assets.test.cjspassed: 466 tests.bash scripts/check-safe-outputs-conformance.shreports pre-existing dispatch-token conformance failures under SEC-012, plus existing low/medium diagnostics. The two failing permission/authentication predicates were confirmed against the committed baseline; those implementations are unchanged by this PR.The canonical TMPDIR avoids an existing macOS
/varsymlink-sensitive test failure. Docker/Colima were unavailable, so Linux container-to-host behavior and live Actions recovery remain unverified. No workflows were dispatched or rerun.