Repository navigation
Fix built-in MCP backend log file ownership - #69235
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ 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.
|
|
✅ PR Code Quality Reviewer completed the code quality 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.
|
|
✅ 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.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
I did not find a blocking regression in the large-record redaction rewrite or the backend UID/GID normalization.
What I checked
- the iterative JSON-string scanner still preserves unchanged records, keeps numeric-token masking outside quoted strings, and intentionally fails closed if a validated token cannot be decoded
- the
agenticworkflowscontainer-user injection is scoped to that backend, preserves explicit--userandHOMEoverrides, and happens before the config is serialized to the gateway - the added coverage exercises the risky edges here: multi-MiB payloads, malformed prefixes, private-file cleanup, and explicit container-user preservation
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 35.2 AIC · ⌖ 5.57 AIC · ⊞ 21.7K
Comment /review to run again
|
@lpcox please review |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design to the mask-redaction scanner and container-identity fix. Both defects are root-caused (stack-sensitive regex alternation; missing UID:GID propagation to backend containers), and the fail-closed cleanup path is preserved and well tested — approving with two minor non-blocking suggestions.
📋 Key Themes & Highlights
Key Themes
-uflag detection inapplyAgenticWorkflowsRunnerIdentity(start_mcp_gateway.cjs:1500) could false-positive on unrelated args like-update, silently skipping the ownership fix in that edge case. Low likelihood given current MCP arg conventions, but cheap to tighten.transformJSONStrings(add_mask_redaction.cjs) reimplements a meaningful slice of JSON string-literal grammar as a private helper. It's a clean, deep fix for the stack-exhaustion bug, but as an un-exported helper it's only exercised indirectly throughredactArtifactMaskedValues; direct unit tests would make future regressions easier to localize.
Positive Highlights
- Root cause properly addressed: replacing repeated regex alternation with an iterative scanner removes the stack-exhaustion failure mode entirely rather than papering over symptoms (verified locally: a 2 MiB synthetic record redacts in ~30ms with no stack error).
- Decode failures in the scanner correctly throw instead of silently swallowing, propagating to the existing fail-closed processFile cleanup — confirmed the error path does not leak raw secret-bearing messages (RedactionOperationError constructs a sanitized stage=/code= message only).
- Strong regression coverage: 10 MiB JSON/mixed-log synthetics, escaped-quote edge cases, malformed-prefix recovery, and container-ownership preservation (explicit --user/-u passthrough) are all tested.
- Backend identity fix narrowly targets the agenticworkflows container and leaves other MCP servers, credentials, and file permissions untouched — consistent with the project's fail-closed posture.
Note: sandbox npm registry access was unavailable, so the new/changed tests were verified by direct code reading and a manual reproduction of the scanner logic rather than running vitest in this environment.
@copilot please address the two inline suggestions when convenient.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 85.1 AIC · ⌖ 15 AIC · ⊞ 11.7K
Comment /matt to run again
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Malformed-string recovery can leave encoded secrets unmasked without triggering fail-closed cleanup.
2 open findings
What changed in this PR
Improves artifact-redaction reliability by addressing backend file ownership and large-record tokenization.
Changes:
- Applies runner identity and default HOME to the built-in MCP backend.
- Replaces quoted-string regex matching with iterative scanning.
- Adds regression tests, documentation, and patch changesets.
| File | Description |
|---|---|
| docs/src/content/docs/reference/tools.md | Documents backend identity defaults. |
| docs/src/content/docs/reference/imports.md | Documents large-record masking. |
| actions/setup/js/start_mcp_gateway.test.cjs | Tests backend identity configuration. |
| actions/setup/js/start_mcp_gateway.cjs | Applies backend user and HOME defaults. |
| actions/setup/js/redact_secrets.test.cjs | Tests private artifacts and fail-closed cleanup. |
| actions/setup/js/add_mask_redaction.test.cjs | Tests large records and malformed prefixes. |
| actions/setup/js/add_mask_redaction.cjs | Introduces iterative JSON-string scanning. |
| .changeset/patch-large-record-mask-redaction.md | Records the large-record masking patch. |
| .changeset/patch-agentic-workflows-backend-ownership.md | Records the backend ownership patch. |
🧠 Review effort: Max
Preserve Docker named-user overrides and add direct scanner tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep this PR scoped to backend file ownership and runner identity. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
pelikhan
changed the title
|
🎉 This pull request is included in a new release. Release: |
Summary
Ownership/path-access fix only. Secret parsing, its tests, documentation, and changeset are now in independent PR #69244. Both target main and can be reviewed/merged independently; this PR does not modify the parser or claim to fix its large-record/malformed-token defects.
Run the built-in container-backed
agenticworkflowsMCP backend as the runner UID:GID unless a container user is explicitly configured. Supply container-local writableHOME=/tmponly when absent. Preserve explicit named/numeric users, environment, credentials, actor validation, and other servers.Verified mechanism and evidence limits
Pinned
gh-aw-mcpgv0.4.33 does not propagate the gateway container's user to backend Docker launches. Built-in development/release images default to root; downloaded logs are created with mode0600. These backend-owned files are unreadable by the runner's post-agent redactor. Fix the producer identity instead of widening permissions or skipping redaction.Historical optimizer run 38805693127 logged 115
stage=read, code=EACCESdiagnostics; reviewer run 38840691314 logged 3 after an independent timeout. These are diagnostic counts, not unique-file counts. Exact failed paths/owners were omitted, so source confirms the mechanism but not definitive attribution of every historical failure.The performance run's
stage=redact, code=unknownis a separate parsing investigation, covered by #69244 with historical attribution limits. Summary overflow, timeout tuning, and health code are unchanged. Fail-closed removal and upload gates remain unchanged.Validation
npm run typecheck --prefix actions/setup/jspassed.PATH="/opt/homebrew/bin:$PATH" TMPDIR="$(cd "${TMPDIR:-/tmp}" && pwd -P)" make agent-report-progresspassed build, formatting, lint, schema freshness, and 113 impacted gateway tests. Four pre-existing gateway lint warnings remain, no new warnings.Docker daemon unavailable, so real container-ownership integration remains unverified. No live workflow reruns/dispatches or raw logs/secret values committed.
Authored Markdown/security impact