fix(mcp): reject unsafe Deep Agents projections - #10911
Conversation
Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe changes reject unsafe managed projection objects, including symlinks, FIFOs, directories, and other non-regular files. Deep Agents MCP status reports a dedicated error, exits with status 2, and preserves credential-inspection behavior. ChangesDeep Agents projection safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change makes unsafe Deep Agents projection entries fail clearly rather than reporting healthy status. No merge-blocking risk remains in the finalized findings. Sequence Diagram(s)sequenceDiagram
participant MCPStatus as Deep Agents MCP status
participant Projection as Managed projection read helpers
participant Filesystem
MCPStatus->>Projection: Inspect managed projection
Projection->>Filesystem: Open and validate projection path
Filesystem-->>Projection: Return filesystem metadata or open error
Projection-->>MCPStatus: Raise UnsafeManagedProjectionError
MCPStatus-->>MCPStatus: Emit unsafe-path diagnostic and exit 2
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes address issue Full details: Out of Scope Changes checkExplanation The changes remain within scope. Production updates implement unsafe projection detection and status propagation. Fixture and test updates support the issue scenarios and verify unchanged credential-handling behavior.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
…x-10754-final-entry
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
P0: none.
P1: two fail-open paths can make an unsafe managed projection appear healthy. Both reproduce on exact head b0b7f9537757f627b8990e3a3fea1a1e224b3ad9.
| if (!result) | ||
| return credentialInspectionFailure ?? { registered: null, detail: "sandbox unreachable" }; | ||
| if (result.status !== 0 && adapter === "deepagents-config") { | ||
| const detail = redactBridgeSecretsForDisplay( |
There was a problem hiding this comment.
[P1] Classify the fixed marker before redacting display text
The redactor runs before the startsWith check and replaces configured credential values globally. A valid credential that overlaps the protocol marker therefore changes the marker, so this unsafe result is not recognized and status falls through to exit 0. On this exact head, GITHUB_TOKEN=Unsafe makes the dangling-symlink, valid-target-symlink, and FIFO regression cases all return 0; the unset-token control passes. Match a strict marker on the raw bounded command output first, redact only the selected display error, and add this collision case to the public-boundary test.
There was a problem hiding this comment.
Fixed in da6ee52. Unsafe projection evidence is now recognized only from the raw strict type marker before credential redaction; only the extracted path is redacted. The command-boundary regression runs all three unsafe entry cases with GITHUB_TOKEN=Unsafe and requires exit 2 with empty stdout.
| " assert_managed_source_stable(path, None)", | ||
| " return b'', None, None", | ||
| " except OSError:", | ||
| " linked = os.stat(path, follow_symlinks=False)", |
There was a problem hiding this comment.
[P1] Fail closed if the rejected symlink disappears during classification
After os.open(...O_NOFOLLOW) has already rejected the final symlink, an untrusted sandbox process can unlink it before this follow-up os.stat. That FileNotFoundError escapes into the outer missing-projection handler, which prints absent and exits 0; a deterministic command-level reproduction produced exactly that result. Preserve the original no-follow failure or raise the typed unsafe error when this lookup disappears or changes, and cover the transition with a regression test.
There was a problem hiding this comment.
Leaving this unchanged because it is the removal race explicitly excluded from #10754. If the entry disappears between the no-follow open failure and fallback classification, the observable final state is absent and retains the existing exit-0 behavior. Closing that interleaving would require broader race semantics outside this PR.
Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
…x-10754-final-entry
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
P0: none.
P1: one fail-open classification path remains on exact head da6ee529684f89ff79f8427ac36b0f7663c15db4. The earlier credential-redaction finding is fixed.
| " assert_managed_source_stable(path, None)", | ||
| " return b'', None, None", | ||
| " except OSError:", | ||
| " linked = os.stat(path, follow_symlinks=False)", |
There was a problem hiding this comment.
[P1] Preserve every unsafe type when the initial open fails
This fallback only converts symlinks to UnsafeManagedProjectionError. On this exact head, a mode-000 FIFO makes os.open fail with EACCES; this os.stat(..., follow_symlinks=False) still sees the FIFO, but line 67 rethrows the generic error. When credential observation is absent, getAdapterRegistration discards that generic adapter error and public mcp status returns exit 0 with the FIFO still present. The unchanged unlink race has the same result: if the rejected symlink disappears before this os.stat, its FileNotFoundError reaches the outer missing-projection handler and returns absent/0. Classify every non-regular linked value with describe_managed_projection_type, preserve a controlled nonzero result if classification disappears or changes, and add public-boundary regressions for the mode-000 FIFO and unlink-between-open/stat cases.
There was a problem hiding this comment.
Fixed the in-scope FIFO path in 235be35. The fallback now classifies every non-regular lstat result with describe_managed_projection_type, and the public command-boundary regression uses a mode-000 FIFO while still requiring exit 2, type-specific stderr, and empty stdout. I did not add the unlink-between-open/stat case because removal races are explicitly excluded from #10754.
…fix-10754-final-entry
Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
…fix-10754-final-entry
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Exact head reviewed: 235be35cb885397087e00ef5b5bd342fb20a730b.
P0
- None.
P1
- None.
The prior permission-denied FIFO fail-open is fixed: the rejected-open fallback now classifies every non-regular final entry, and the public mode-000 FIFO regression exits 2 without healthy output. Focused exact-head tests passed 30/30; required CI and managed-runtime image checks completed successfully; all eight commits are Verified; CodeRabbit completed with no actionable finding.
Approval is withheld because the repository's trusted checker still returns allPass: false: changes is recorded as skipped and test-e2e-sandbox failed the unchanged blueprint apply smoke. In addition, all nine exact-head Advisor specialists failed before producing review artifacts. I will re-evaluate when the exact-head gate evidence is complete and passing.
…x-10754-final-entry
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Dismissed at the reviewer request to clear prior review state.
|
Maintainer-authorized inherited-CI waiver for exact head
Per maintainer direction, this inherited npm failure is waived for readiness and merge. The candidate remains unchanged. |
|
Maintainer waiver for the remaining inherited E2E failure on exact head
This is not candidate-owned. This PR does not change the failing shell suite or the managed-image inputs it exercises. The exact signature also reproduced on unrelated PR #10889 in run 33773205634. The obsolete shell suite is being replaced by #10941. The branch was refreshed with canonical fix #10930, so the gateway-pinned apply-smoke failure is no longer waived. This supplements the earlier npm 10.9.8 package-contract waiver. All focused issue #10754 tests, CLI shards, managed-image builds and runtime activation checks passed. Rebecca Sliter explicitly authorized merging past this inherited E2E failure. |
|
PR Review Advisor finished for commit |
3 participants
Outcome
mcp statusnow rejects a dangling symbolic link, a symbolic link to valid content, or a FIFO at the managed Deep Agents MCP projection entry. It prints the unsafe file type, exits with status2, and emits no healthy server status.Reason
These unsafe entries previously appeared as an ordinary adapter mismatch and returned exit status
0. That hid a final-path replacement behind credential or adapter status handling, including when a legacy unsupported credential was still attached.Related issues
Fixes #10754
Changes
mcp statusboundary, before credential-observation skips can hide it. Regular files, missing files, parse failures, and the legacy Deep Agents path keep their existing status behavior.Verification
#10754selector passed all 3 matching command-boundary tests. The public regressions useGITHUB_TOKEN=Unsafe, an unsupported persisted credential, a mode-000FIFO, and a directory for the generic non-regular fallback.npm run typecheck:clipassed.npx vitest run --project integration test/automation/pull-requests/growth-guardrails.test.tspassed all 39 tests.npm run validate:prpassed for the tree committed as147adef40ce378e526b03dd433c826ff61f8c699, based on canonicalmainat36f54548583a4db32b78b02a81df0222d7ef070b.Review notes
npm run review:localwas attempted again after the final repair. The sandboxed run could not fetch its pinned dependency. The network-enabled retry installed the dependency, but its temporary OpenShell gateway refused every connection during configuration, so no local specialist analysis ran. Earlier attempts on the preceding repair commits failed at the same boundary. All nine hosted PR Review Advisor reports for commitda6ee529684f89ff79f8427ac36b0f7663c15db4were reviewed from run33697632693. Behavior, Code Reduction, Dependency Use, Design and Architecture, Documentation, Migration Completion, and Trust found no issue. The Operations recovery request is explicitly outside #10754. The Test Design request concerns pre-existing source-shape and command-rewrite tests; the new regression exercises the command boundary and unmodified generated status command directly. CodeRabbit reported no actionable finding on that candidate.The first expanded Advisor run for commit
235be35cb885397087e00ef5b5bd342fb20a730band its maintainer-authorized full retry both used the original3509b5a437ed3ec4309669c4121ae5b718895bfdworkflow revision. Every specialist failed because the old workflow required each session to preload the completediff.patch; no specialist report was produced. Canonical main then addedfix(review): inspect specialist diffs on demand (#10952), so this branch integratedd4eff54a8d213a3a8fe5650703c8e708eab4dd7d.All nine expanded Advisor reports for the refreshed commit
f8568042723efd24286449af4c12a00b91ca6ec3were reviewed from successful run33757287610. Security, Delivery, Reduction, Operability, Migration, Customer Behavior, and Architecture found no issue. Verification requested command-boundary coverage for the existing genericnon-regular filebranch; commit460b9e0fb2f449046f4a74ca9a162b96594db856adds a directory case through the generated command and public dispatch. Documentation requested a recovery procedure, but recovery and lifecycle workflows are explicitly outside #10754 and no supported procedure exists to document in this PR.All nine Advisor reports for
460b9e0fb2f449046f4a74ca9a162b96594db856were reviewed from successful run33759151935. Security, Verification, Delivery, Reduction, Operability, and Customer Behavior found no issue. Migration requested dispatch-level proof that the credential-observation change preserves the legacy Deep Agents result. Architecture found that status duplicated the adapter's unsafe-result grammar. Commitaeef43cec96bfb3b43bd75eab3d42638dd4ad7aaadds the legacy regression and makes the adapter-status module own the strict typed-result parser consumed by status. Documentation repeated the recovery request, which remains outside the accepted scope. CodeRabbit completed against460b9e0fb2f449046f4a74ca9a162b96594db856with no actionable comments.All nine final Advisor reports for
0a6b3bc4a355d25888f315e282638c1412f7f412were reviewed from run33761302554. The first Security attempt failed its reviewer evidence-read contract and produced no artifact; its targeted second attempt succeeded and found no issue. Architecture, Customer Behavior, Delivery, Migration, Operability, Reduction, Security, and Verification found no issue. Documentation repeated the out-of-scope recovery request. CodeRabbit reviewed through exact commit0a6b3bc4a355d25888f315e282638c1412f7f412, reported minimal merge risk, and generated no actionable comment. Its docstring-coverage warning is advisory and concerns existing small helper functions rather than a correctness defect.All nine Advisor reports for refreshed commit
1b5d57c5e01660447586fa985aea85b3f08f4f21were reviewed from run33781865168. Architecture, Customer Behavior, Delivery, Documentation, Migration, Operability, Reduction, and Verification found no issue. Security correctly found that the unsupported-attached-credential shortcut could bypass the Deep Agents projection inspection. Commit147adef40ce378e526b03dd433c826ff61f8c699moves the Deep Agents inspection ahead of that skip and adds command-boundary regressions for unsafe and regular projections. Hosted exact-head review is required again for this repair.cjagwani identified that credential redaction could turn an arbitrary
GITHUB_TOKEN=Unsafevalue into the unsafe-projection marker. The repair now recognizes only a strict raw typed marker, extracts its path, and redacts only that path before reporting the trusted diagnostic. The regression test covers all three file types with that credential value. The follow-up review found that a permission-denied FIFO could fail the initial open and bypass the symlink-only fallback. The fallback now classifies every non-regular entry with the same type helper, and the public regression uses a mode-000FIFO. The same comment also describes an entry disappearing between the no-follow open and fallback classification; removal races are explicitly excluded from #10754, so this PR intentionally leaves absent-at-classification behavior unchanged.The first ready-state self-hosted E2E run failed before reaching this change because the existing blueprint plan smoke rejected
nim-service.localas private or reserved. PR #10908 owned and merged the isolated-plan fix. The next candidate passed plan and completed apply, then the unchanged fixture failed withApply did not use the gateway-pinned base-policy read. PR #10930 owned and merged that isolated fixture repair. After integrating canonical main,Staging QA base permission regression (Deep Agents Code)passed on exact commit1b5d57c5e01660447586fa985aea85b3f08f4f21.The ordinary CI run
33781868761for1b5d57c5e01660447586fa985aea85b3f08f4f21passed 1,238 package-contract tests and every CLI shard, but its pre-existingmanaged-image-registry-transport.test.tsomit-dev install failed when npm itself crashed withCannot read properties of null (reading 'edgesOut'). The PR changes neither that test nor dependency manifests. Canonical main failed the same test, line, and npm signature; the test also passes intermittently on recent main commits. The maintainer explicitly waived this inherited failure for the exact candidate after reviewing the evidence.The same candidate's self-hosted gateway-isolation E2E run passed 44 tests and failed only test 27 because the unchanged harness could not find
normalize_mutable_config_perms. The same signature reproduced on unrelated PR #10889, and PR #10941 is replacing the obsolete suite. The maintainer explicitly waived this inherited failure. Exact-head CI after147adef40ce378e526b03dd433c826ff61f8c699must be classified before merge; these waivers do not cover any new signature.This change is limited to the final managed Deep Agents projection entry and
mcp status. Parent-directory descriptor pinning, recovery, cleanup, lifecycle changes, removal races, socket behavior, other agents, and unrelated hardening remain outside #10754.Signed-off-by: Rebecca Sliter 571084+rsliter@users.noreply.github.com