feat(scope): operator-armed cross-host roam (BOB_HTTP_ROAM_AUTHORIZED, default-off) - #176
feat(scope): operator-armed cross-host roam (BOB_HTTP_ROAM_AUTHORIZED, default-off)#176vmihalis wants to merge 4 commits into
Conversation
…, default-off) When the operator sets BOB_HTTP_ROAM_AUTHORIZED=<target_domain> (target-bound, mirroring the IDOR live arm BOB_IDOR_PROVISION_AUTHORIZED), validateHttpScanScope stops rejecting a URL whose host is OUTSIDE the session's target_domain — so Bob may follow an engagement off the apex (OAuth/SSO IdP, CDN, sibling app, an authorized redirect/SSRF chain). The env is the out-of-band operator gate a confined MCP/Bash agent cannot set. Single chokepoint: validateHttpScanScope backs the HTTP tools (safe-fetch), the browser driver (navigate/authed_fetch via assertSafeResolvedRequestUrl), and redirect-following — so one relaxation covers them all. A roamed request returns scope_decision: allowed, reason: operator_armed_roam (every roamed host stays visible in the audit). Deliberately NOT relaxed: - Lab-attested private targets: the lab path returns BEFORE the roam check, so a 127.0.0.1/RFC1918 session can never pivot to 169.254.169.254 or a LAN neighbour even with roam armed (asserted). - block_internal_hosts: a separate DNS-resolution policy (safe-fetch resolveSafeAddress); roam relaxes the target-DOMAIN boundary only, never internal-host blocking. Roam alone is not SSRF-to-internal. Target-bound: roam authorizes ONLY the session whose target_domain equals the env value (trim + case-insensitive, whole-target, not a suffix), so arming one engagement never relaxes another. Tests: test/operator-armed-roam.test.js (7) — off blocks, on allows, wrong-target blocks, suffix-no-match, first-party unaffected, lab-locked-under-roam, helper unit. docs/FIRST_RUN.md documents the arm + the two non-relaxed boundaries. test:mcp 3180/0, test:prompts clean.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds an operator-armed HTTP roam mode gated by ChangesCross-host roam scope
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant validateHttpScanScope
participant roamAuthorizedForTarget
participant safeFetch
participant httpScan
httpScan->>validateHttpScanScope: validate requested host
validateHttpScanScope->>roamAuthorizedForTarget: check target-bound authorization
roamAuthorizedForTarget-->>validateHttpScanScope: authorization result
validateHttpScanScope-->>httpScan: scope decision
httpScan->>safeFetch: execute request with scope decision
safeFetch->>safeFetch: strip credentials on cross-origin or downgrade redirect
safeFetch-->>httpScan: response and scope reason
httpScan-->>httpScan: write audited scope_reason
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@codex review |
|
✅ Action performedReview finished.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27db587474
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🪓 Brutalist Review
All four critics independently converge on one headline: the operator-armed roam branch in scope.js makes validateHttpScanScope return allowed:true for ANY off-target host, and the PR's own comment/docs claim internal/metadata IPs "stay blocked" — but that guarantee is enforced by a separate, default-OFF block_internal_hosts policy, and the browser navigate/session-start paths hardcode blockInternalHosts:false. Verified: navigate (browser-driver.js:531) and browser-navigate.js:33 call assertSafeResolvedRequestUrl with blockInternalHosts:false, which returns early without DNS resolution (safe-fetch.js:135-137), so under roam the scope kernel is the sole gate — turning roam into an internal-SSRF vector on the browser surface. bob_http_scan is genuinely defended (it threads session policy into safeFetch→resolveSafeAddress). Critics agree the lab-attested ordering is correct (lab path returns at scope.js:309 before the roam check, confirmed by the test). Secondary agreement: roam is all-or-nothing (no per-host allowlist), env normalization is weaker than target-domain normalization (fail-closed for IDN/trailing-dot), and the new test never exercises the actual dangerous case (roam-armed-for-a-public-target → request to an internal host).
Inline comments: 7 (1 🔴 critical · 1 🟠 high · 5 🟡 medium)
Per-CLI breakdown
✅ Claude (default, 201791ms)
Native Claude critic. Traced scope.js → safe-fetch.js → policy resolver → browser driver. Confirmed lab-attested ordering is correct and bob_http_scan is defended, but flagged HIGH: the "Roam != SSRF-to-internal" guarantee is false because block_internal_hosts defaults OFF in normal/legacy modes and navigate hardcodes false. Also HIGH: roam is all-or-nothing (no allowlist); MEDIUM: tests never exercise the real internal-host case. Recommended a synchronous isBlockedInternalHost guard in the roam branch.
✅ Codex (default, 56237ms)
Codex critic ran read-only and was unable to read the filesystem in-environment (bwrap loopback failure + connector path unreadable), so it reasoned from the diff and observable contracts. Flagged HIGH SSRF (scope kernel emits affirmative allowed for metadata/internal hosts under roam), MEDIUM audit-reason collapse, MEDIUM test gaps (enumerated the missing internal host classes), and MEDIUM env-normalization weakness. Could not independently verify branch ordering due to the shell failure.
✅ agy (Gemini 3.5 Flash (Medium), 204733ms)
Antigravity/Gemini critic read browser-driver.js, safe-fetch.js, and scope.js. Flagged CRITICAL: browser navigate disables internal-host checks (blockInternalHosts:false) and now relies on a roam-relaxed scope kernel → total SSRF bypass on navigation. Also HIGH wildcard-trust/no-allowlist, MEDIUM (rated high by critic, downgraded — fail-closed) IDN/FQDN normalization mismatch, LOW misleading error message, and MEDIUM test gaps. Assessed technical-debt interest rate as high due to implicit env-based state propagation.
✅ glm (Claude) (glm-5.1, 379697ms)
Claude-routed GLM-5.1 client. Most thorough end-to-end trace: confirmed bob_http_scan is defended (session policy → safeFetch → resolveSafeAddress) but proved the navigate path is a CRITICAL internal-SSRF oracle under roam (browser-navigate.js:32 / browser-driver.js:530 hardcode blockInternalHosts:false; assertSafeResolvedRequestUrl returns early without resolving). Noted only IP-literal IMDS is incidentally blocked by an accidental psl.parse throw, defeated by using a hostname. Also MEDIUM untested psl throw-as-filter, MEDIUM audit-recording gap (http-scan ledger specifics unverified by orchestrator), and MEDIUM test gaps. Verified lab-attested ordering is sound.
Out-of-diff findings (3)
maintainability
- 🟡 medium
mcp/lib/safe-fetch.js— agy [unanchored]: assertSafeRequestUrl calls validateHttpScanScope without forwarding opts (lab authorization / session options) - 🔵 low
mcp/lib/scope.js— agy [unanchored]: Roamed-host PSL failure throws a target_domain-flavored error, misattributing the fault to the session target
security
- 🟡 medium
mcp/lib/scope.js— glm (Claude) [unanchored]: Persisted audit may drop the operator_armed_roam reason that FIRST_RUN.md promises is recorded
Brutalist orchestrator schemaVersion=1 · context_id=64ebc78c-a6e3-4561-ae25-a6ec0db736fc
…irects; audit reason; IDN arm (round 1) Round-1 review found a CRITICAL SSRF and related issues — fixed: - CRITICAL (agy/Codex P1): bob_browser_navigate calls the scope kernel with blockInternalHosts:false and relies on it entirely, so roam returning allowed for ANY host was an SSRF-to-metadata/localhost bypass. Roam now allows only valid PUBLIC DNS hosts (assertHttpScopeDomain gate) — an IP literal, loopback, RFC1918/link-local, cloud-metadata, or non-public name falls through to the cross-host block regardless of block_internal_hosts. New test asserts 169.254.169.254 / 127.0.0.1 / 10.x / [::1] / *.internal / *.local stay blocked under roam armed for a PUBLIC target (the case that would have caught it). - P1 #2 (Codex): safeFetch reused options.headers across redirects, so a credentialed first-party scan following a 302 to a roamed host leaked the target's Cookie/Authorization. Now strips them on a redirect whose host is NOT first-party to targetDomain (cross-site); first-party subdomain redirects keep them. stripCredentialHeaders exported + unit-tested. - P2 (Codex): scopeAuditFields now persists scope_reason, so a roamed request is loud in http-audit (operator_armed_roam) instead of an inferred URL-host diff. - P3 (Codex/agy): roamAuthorizedForTarget normalizes BOTH sides via normalizeDnsHostToAscii, so an operator arming the Unicode form of an IDN target matches the punycode session domain. Tested. glm's 'psl.parse-throw as the only IP filter' is replaced by the explicit assertHttpScopeDomain public-DNS gate (tested with IP literals). docs/FIRST_RUN.md updated: roam = public-only, creds-stripped-cross-site, audited. test:mcp 3185/0, 12 roam tests, check:syntax clean.
Round-1 review addressed —
|
| Finding | Resolution |
|---|---|
| 🔴 CRITICAL (agy ×4) / Codex P1 — browser navigate SSRF; "block internal before roam" | Fixed. bob_browser_navigate calls assertSafeResolvedRequestUrl(..., {blockInternalHosts:false}) and leans entirely on the scope kernel — so roam returning allowed for any host was an SSRF-to-metadata bypass. Roam now gates on assertHttpScopeDomain(host) (the same public-DNS bar the target clears): an IP literal, loopback, RFC1918/link-local, cloud-metadata, or non-public name falls through to the cross-host block, regardless of block_internal_hosts. |
| 🟠 Codex P1 — strip target credentials on roamed redirects | Fixed. safeFetch reused options.headers across redirects; it now strips Cookie/Authorization on a redirect whose host is not first-party to targetDomain (cross-site). First-party subdomain redirects keep their headers. stripCredentialHeaders exported + unit-tested. |
| 🟡 Codex P2 — persist roam reason in audit | Fixed. scopeAuditFields now emits scope_reason, so a roamed request is scope_reason: operator_armed_roam in http-audit.jsonl instead of an inferred URL-host diff. |
| 🟡 Codex P3 / agy — normalize IDN arms | Fixed. roamAuthorizedForTarget normalizes both sides via normalizeDnsHostToAscii, so arming the Unicode form of an IDN target matches the punycode session domain. Tested. |
| 🟡 glm — psl.parse-throw as the only IP filter, untested | Fixed. Replaced by the explicit assertHttpScopeDomain public-DNS gate; tested directly against IP literals. |
| 🟡 Codex — audit reason collapses public vs toxic host class | Moot now — toxic (internal/IP) hosts are no longer roamed (they're blocked), so operator_armed_roam only ever labels a public cross-host allow. |
| 🟡 Claude / agy — tests never exercise the safety claim; no internal-IP/IDN cases | Fixed. New tests: roam armed for a PUBLIC target blocks 169.254.169.254/127.0.0.1/10.x/[::1]/*.internal/*.local/bare-host (the exact case that would have caught the CRITICAL); IDN normalization; cross-site cred-strip decision + stripCredentialHeaders unit. |
| 🟠 agy HIGH — wildcard trust: roam is allow-all for out-of-scope hosts | By operator design, now bounded. The operator explicitly chose "Everywhere". The wildcard is bounded to public hosts, with internal/metadata blocked, target-bound arming, credentials stripped cross-site, lab targets pinned, and every roam audited. |
Remaining honest gap: a fully-wired browser-driver navigation test under roam needs the Patchright subprocess + DNS control for a public second host (localhost isn't public, so roam's own gate blocks it) — disproportionate. The security property the browser path delegates to the kernel (internal IPs blocked under roam) is tested at the scope layer.
test:mcp 3185/0, 12 roam tests, check:syntax clean. docs/FIRST_RUN.md updated (public-only, creds-stripped, audited).
|
@codex review |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcp/lib/http-scan.js`:
- Around line 48-53: The audit fields in httpScan() are using the initial scope
decision, so redirected requests can keep the wrong scope_reason and suffix
metadata after a roam. Update the safeFetch() flow to return the final or
per-hop scope decision for the redirect-followed request, then have httpScan()
persist that returned decision instead of the pre-redirect one. Use the existing
safeFetch and httpScan symbols to locate the handoff and ensure the audited
fields reflect the final cross-host hop.
In `@mcp/lib/safe-fetch.js`:
- Around line 39-52: Cross-site redirects are still forwarding custom auth
headers because stripCredentialHeaders only removes Cookie and Authorization.
Update the redirect header sanitization used by safeFetch/httpScan to drop all
provenance-bound or sensitive auth-profile headers (for example X-Api-Key,
X-Auth-Token, and any other credential-bearing headers) before the next hop.
Keep same-site redirects intact, and make the stripping logic broader or
policy-driven so arbitrary secret headers cannot be replayed to an off-target
host.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cc303969-c5b1-40f8-ab6b-8bf77aa8411a
📒 Files selected for processing (5)
docs/FIRST_RUN.mdmcp/lib/http-scan.jsmcp/lib/safe-fetch.jsmcp/lib/scope.jstest/operator-armed-roam.test.js
✅ Files skipped from review due to trivial changes (1)
- docs/FIRST_RUN.md
🚧 Files skipped from review as they are similar to previous changes (1)
- mcp/lib/scope.js
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0c3562645
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🪓 Brutalist Review
All four critics independently converge on two genuine, verified defects that the PR's own stated invariants deny. (1) SSRF-to-internal: the roam gate at scope.js:333-337 is a string-only check with no DNS resolution, and the browser-navigate path hardcodes blockInternalHosts:false (browser-driver.js:531) while assertSafeResolvedRequestUrl skips resolution when blocking is off (safe-fetch.js:151-152) — so a roamed public hostname that resolves to an internal/metadata IP is allowed; Claude and Codex frame this via DNS-rebind/attacker A-records, while agy and the GLM-routed Claude frame it via the lab-attestation escape that assertHttpScopeDomain(host) inherits (it reads host-keyed lab artifacts for IP literals, scope.js:252-256). Both routes defeat 'never an SSRF-to-internal primitive.' (2) Credential exposure: stripCredentialHeaders runs only on redirects (safe-fetch.js:385-393), so a direct hop-1 request to a roamed host replays the target's auth — confirmed because http-scan.js:210 attaches auth-profile creds to the initial headers — and the strip covers only authorization/cookie while auth profiles inject arbitrary header keys (auth.js:80). Secondary agreements: roam is a process-global, target-string-only grant with no per-host allowlist or session binding (exfil + cross-engagement-bleed risk), and the docs overstate the default posture (internal blocking is OFF by default). The headline blocker: the feature ships a documented security guarantee its load-bearing chokepoint does not enforce. Note: the critics' file citations of mcp/lib/browser-driver.js are actually at mcp/browser-driver.js, but every referenced behavior was verified present.
Inline comments: 7 (2 🔴 critical · 2 🟠 high · 3 🟡 medium)
Per-CLI breakdown
✅ Claude (default, 278342ms)
Native Claude: traced scope.js -> safe-fetch.js -> browser-driver.js. Headline CRITICAL: roam is an SSRF-to-internal primitive — string-only roam gate + navigate path's hardcoded blockInternalHosts:false means a public hostname resolving to an internal IP is allowed. Also HIGH blanket public-internet/exfil grant, MEDIUM custom auth-header leak, MEDIUM lab-attestation reuse, LOW doc posture. Credited what the PR gets right (out-of-band gating, IDNA target binding, lab pin, audit visibility).
✅ Codex (default, 82350ms)
Codex: shell sandbox failed (bwrap loopback) so review was diff-only, clearly marked. CRITICAL roam SSRF-to-internal via DNS; HIGH process-global (not session-bound) authorization causing cross-engagement bleed; HIGH hop-1 direct credential replay; MEDIUM docs/impl disagreement on the internal-host boundary. Strong on the scope-binding and SSRF boundary failures.
✅ agy (Gemini 3.5 Flash (Medium), 59066ms)
Antigravity/Gemini: CRITICAL cross-session lab-target escape (assertHttpScopeDomain reads host-keyed lab-authorization artifacts), HIGH hop-1 credential leak, MEDIUM single-target env var concurrency bottleneck, MEDIUM https->http protocol-downgrade keeps creds. Best architectural framing of the stateful-validator-reuse root cause; the cited file paths used mcp/lib/ but the lab/scope logic is confirmed in mcp/lib/scope.js.
✅ glm (Claude) (glm-5.1, 419208ms)
GLM-routed Claude: most thorough trace. HEADLINE high/borderline-critical lab-attestation escape via assertHttpScopeDomain reuse with a concrete operator-env exploit and a demonstration that the new test cannot catch it; MEDIUM hop-1 credential replay; MEDIUM doc overstates chokepoint coverage + navigate DNS-rebind residual; LOW silent arm no-op for subdomain sessions. Judged the core idea sound, the public-only leg's validator wrong.
Brutalist orchestrator schemaVersion=1 · context_id=538f4977-5519-4c1e-8272-f9c872c12f90
…ss-site credential/body protection (round 2) Round-2 found TWO more CRITICALs + credential vectors. All fixed (operator keeps allow-all-public; the exfiltration-to-any-PUBLIC-host is logged accepted risk): CRITICALs (SSRF): - Lab-escape (agy/Codex): the roam public-host check called assertHttpScopeDomain which HONORS a lab attestation, so a concurrent BOB_LAB_TARGET reclassified an internal host as roamable. Added ignoreLabAttestation; roam uses the lab-BLIND form, so an attested internal host is rejected. Tested. - DNS rebind (Claude/Codex): the gate validated the host STRING, not the resolved IP, and the browser path disables block_internal_hosts. The roam decision now carries enforce_internal_block:true; safeFetch + assertSafeResolvedRequestUrl ALWAYS resolve + block internal IPs for a roamed request (IP pinned for connect), so a public name resolving to 169.254/127/10.x is blocked. Tested. Credential / exfil-of-target-secrets vectors: - Initial direct request to a roamed host (agy HIGH): http-scan no longer applies the target's auth_profile when initialScopeDecision.reason is operator_armed_roam. - Cross-site redirect (CodeRabbit CRITICAL + Claude/Codex): stripCredentialHeaders is now an ALLOWLIST (keep user-agent/accept/…); drops Cookie, Authorization, Proxy-Authorization, AND custom X-Api-Key/X-Auth-Token headers — not a 2-name denylist. Body dropped on cross-site (covers 307/308). Protocol downgrade (https->http) also strips (agy). - Audit (CodeRabbit/Codex): safeFetch carries the final hop's scopeReason; a first-party request that redirects INTO a roam now audits operator_armed_roam. docs/FIRST_RUN.md updated (lab-blind, resolved-IP, comprehensive strip). test:mcp 3187/0, 14 roam tests, check:syntax clean.
Round-2 review addressed —
|
| Finding | Fix |
|---|---|
🔴 Lab-attestation escape (agy / Codex P1) — roam's assertHttpScopeDomain(host) honored a concurrent BOB_LAB_TARGET, reclassifying an internal host as roamable |
Added ignoreLabAttestation; the roam gate uses the lab-blind form, so an attested internal host is rejected. Test: with a valid attestation the host is accepted by the normal check but rejected by the lab-blind form. |
🔴 DNS-rebind / resolved-IP (Claude / Codex P1) — the gate validated the host string; a public name resolving to 169.254/127/10.x + the browser path's blockInternalHosts:false = SSRF |
The roam decision now carries enforce_internal_block:true; both safeFetch and assertSafeResolvedRequestUrl always resolve and block internal IPs for a roamed request (resolved IP is pinned for the connection). Test: roam armed for a public target blocks 169.254/127.0.0.1/10.x/[::1]/*.internal. |
Credential / target-secret vectors — fixed
| Finding | Fix |
|---|---|
| 🟠 Initial direct request to a roamed host (agy HIGH) | http-scan no longer applies the target's auth_profile when initialScopeDecision.reason === operator_armed_roam. |
| 🔴 Custom auth headers leak cross-site (CodeRabbit) + 🟡 (Claude) + 🟡 proxy creds (Codex P2) | stripCredentialHeaders is now an allowlist (keep user-agent/accept/…) — drops Cookie, Authorization, Proxy-Authorization, and arbitrary X-Api-Key/X-Auth-Token headers, not a two-name denylist. |
| 🟡 Body replayed on cross-site 307/308 (Codex P2) | Body dropped on any cross-site redirect. |
| 🟡 Protocol downgrade keeps creds (agy) | https→http downgrade also triggers the strip, even same-host. |
| 🟠/🟡 Audit doesn't record redirect-roam (CodeRabbit Major / Codex P2) | safeFetch carries the final hop's scopeReason; a first-party request that redirects into a roam now audits operator_armed_roam. |
| 🟡 FIRST_RUN.md overstates (glm) | Doc rewritten to match: lab-blind, resolved-IP rebind protection, comprehensive credential+body stripping. |
The one design objection (operator-accepted)
🟠 "Roam grants the entire public internet — removes the anti-exfiltration boundary" (Claude HIGH). This is inherent to "everywhere", not a bug. The operator explicitly chose allow-all-public ("let bob do anything he wants") over an allowlist, accepting that Bob may reach/send-to any public host. Logging that here as the accepted risk. (Internal/metadata hosts are not reachable — see the two CRITICAL fixes; only public hosts, with target credentials never leaving the target site.)
test:mcp 3187/0, 14 roam tests, check:syntax clean.
|
@codex review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
mcp/lib/safe-fetch.js (1)
365-377: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSanitize credentials before the first roamed hop.
safeFetch()still sendscurrentHeadersunchanged on an initialoperator_armed_roamrequest; redirect stripping only runs after a response. That leaves explicit caller-providedCookie,Authorization, API-key, or token headers exposed to the off-target host even though the auth_profile path is skipped.Proposed guard for the initial roamed hop
const hopBlockInternal = blockInternalHosts || !!(scopeDecision && scopeDecision.enforce_internal_block); + if (redirects === 0 && scopeDecision && scopeDecision.reason === "operator_armed_roam") { + currentHeaders = stripCredentialHeaders(currentHeaders); + } const response = await requestOnce(currentUrl, { ...options, headers: currentHeaders,This is the remaining caller-provided-header variant of the earlier direct-hop credential concern. As per coding guidelines, “Flag secret, cookie, token, API key… exposure… as high-priority findings.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/lib/safe-fetch.js` around lines 365 - 377, The initial operator_armed_roam request in safeFetch still forwards currentHeaders without stripping secrets, so sanitize caller-provided credentials before calling requestOnce on the first roamed hop. Update the flow around assertSafeRequestUrl, currentHeaders, and the requestOnce invocation so Cookie, Authorization, API-key, token, and similar sensitive headers are removed or filtered before the off-target request is sent, while preserving the existing redirect stripping behavior for later hops.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@mcp/lib/safe-fetch.js`:
- Around line 365-377: The initial operator_armed_roam request in safeFetch
still forwards currentHeaders without stripping secrets, so sanitize
caller-provided credentials before calling requestOnce on the first roamed hop.
Update the flow around assertSafeRequestUrl, currentHeaders, and the requestOnce
invocation so Cookie, Authorization, API-key, token, and similar sensitive
headers are removed or filtered before the off-target request is sent, while
preserving the existing redirect stripping behavior for later hops.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 81c6e33d-60db-40f2-b988-c1db951afc77
📒 Files selected for processing (5)
docs/FIRST_RUN.mdmcp/lib/http-scan.jsmcp/lib/safe-fetch.jsmcp/lib/scope.jstest/operator-armed-roam.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/FIRST_RUN.md
- mcp/lib/scope.js
- test/operator-armed-roam.test.js
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39b36a0765
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| reason: "operator_armed_roam", | ||
| enforce_internal_block: true, |
There was a problem hiding this comment.
When BOB_HTTP_ROAM_AUTHORIZED is armed, this branch turns an off-target host into an allowed result and only annotates it with enforce_internal_block; that flag is honored by safeFetch/assertSafeResolvedRequestUrl, but not by callers that only use assertSafeRequestUrl. I checked mcp/lib/offensive-runner.js:527, which scope-checks URL flags this way and then starts the Dockerized tool, so a roamed attacker-controlled public hostname can be resolved later inside the tool to an internal/metadata address and bypass the boundary that this feature claims remains enforced.
Useful? React with 👍 / 👎.
| headers: currentHeaders, | ||
| blockInternalHosts: hopBlockInternal, | ||
| method: currentMethod, | ||
| body: currentBody, |
There was a problem hiding this comment.
For an initial URL that is allowed only because of operator_armed_roam, safeFetch still sends the caller's original headers and body on the first hop; the new stripping logic only runs after a redirect is processed. Shared callers such as the IDOR producer pass auth-profile headers directly into safeFetch (mcp/lib/offensive-idor-producer.js:1035-1038), so with roam armed a cross-host candidate can receive the target's Authorization/Cookie/custom token headers before any redirect logic has a chance to sanitize them.
Useful? React with 👍 / 👎.
| const parsed = new URL(url); | ||
| await resolveSafeAddress(parsed.hostname, options); | ||
| await resolveSafeAddress(parsed.hostname, { ...options, blockInternalHosts: true }); |
There was a problem hiding this comment.
When a roamed URL is checked for the browser path, this helper performs a Node DNS lookup and returns, but the subsequent browser navigation uses page.goto(url) in mcp/browser-driver.js:539 and resolves the hostname again. With roam armed, an attacker-controlled public hostname can answer public during preflight and then rebind to 169.254/loopback for Chromium, so the promised internal-host block for roamed browser navigation is bypassed unless the checked address is pinned or navigation is otherwise intercepted.
Useful? React with 👍 / 👎.
| const auditScopeDecision = scopeReason | ||
| ? { ...(initialScopeDecision || {}), reason: scopeReason } | ||
| : initialScopeDecision; |
There was a problem hiding this comment.
When an in-target URL redirects to a roamed host, safeFetch only returns the final hop's reason, and this merge keeps the initial decision's registrable_domain/public_suffix while replacing just reason. The resulting http-audit row can say scope_reason: operator_armed_roam but still attribute the request to the target's domain instead of the roamed host, which weakens the audit trail for cross-host traffic; carry the final scope decision, not just its reason.
Useful? React with 👍 / 👎.
| const scopeDecision = assertSafeRequestUrl(currentUrl, targetDomain, { blockInternalHosts }); | ||
| // A roamed hop ALWAYS resolves + blocks internal IPs, even if the caller disabled blockInternalHosts — | ||
| // a public name that resolves to an internal IP cannot become a roam SSRF (DNS rebinding). (CRITICAL.) | ||
| const hopBlockInternal = blockInternalHosts || !!(scopeDecision && scopeDecision.enforce_internal_block); |
There was a problem hiding this comment.
For a roamed hop this sets hopBlockInternal, but proxy-backed egress can still be used because the proxy compatibility check only looks at the session's block_internal_hosts policy. In that configuration the target host is resolved again by the HTTP/HTTPS/SOCKS proxy (the local pre-resolution in requestOnce does not pin what the proxy connects to), so an operator-armed roam to an attacker-controlled public name can pass Bob's local DNS check and then resolve to metadata/private space from the proxy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🪓 Brutalist Review
All four critics agree the scope.js kernel itself is well-built — ignoreLabAttestation, enforce_internal_block, the allowlist (not denylist) credential strip, target-binding, and IDN normalization are all correct and tested. The consensus failure is that roam's security invariants are enforced per-caller and per-transport instead of at one chokepoint: (1) bob_browser_navigate to a roamed host is NOT IP-pinned — only the launch target is in Chrome's --host-resolver-rules, so the Node-side resolve is a TOCTOU check and a roamed public name can DNS-rebind to internal/metadata (flagged independently by codex, agy, and glm as the headline hole); (2) the off-site credential strip lives only in http-scan's initial-hop guard and safeFetch's redirect path, so other credentialed callers that now inherit roam via assertSafeRequestUrl — the IDOR producer's first hop and the mass-read producer's cookie rebind — can replay target credentials to a roamed host; (3) on a first-party→roamed redirect the audit keeps the original host's suffix metadata, undercutting "every roamed host stays visible." Disagreement is mainly severity: claude rates the credential-strip placement HIGH and the browser path medium, while codex/agy/glm rate the browser-navigate rebind critical. Headline: the kernel is sound, but the relaxation leaks through every transport and caller that didn't receive the bespoke guard.
Inline comments: 5 (1 🟠 high · 4 🟡 medium)
Per-CLI breakdown
✅ Claude (default, 299288ms)
Native Claude. Kernel sound; invariants enforced per-caller not at chokepoint. Headline HIGH: initial-hop credential strip lives only in http-scan, so other credentialed safeFetch callers (IDOR producer, followRedirects:false) leak target creds to roamed hosts. Plus roam+egress-proxy IP-pin degradation (MEDIUM), browser-navigate not pinned (MEDIUM), redirect-into-roam audit suffix mismatch (LOW).
✅ Codex (default, 233489ms)
Reviewed read-only via GitHub connector (local sandbox bwrap failed). Two CRITICALs: browser-navigate roamed host not pinned (DNS-rebind SSRF); target auth cookies rebound to roamed hosts via mass-read producer's cookieObjectsFromProfile. MEDIUMs: final-hop roam audit keeps initial suffix metadata; validateHttpScanScope/assertSafeRequestUrl is now a global primitive that silently widened cookie/endpoint/browser scope.
✅ agy (Gemini 3.5 Flash (Medium), 81694ms)
Architecture-led: dual-resolver TOCTOU between Node validator and Chrome/proxy clients. CRITICAL browser-navigate DNS rebind; HIGH egress-proxy bypasses requestOptions.lookup IP pin; MEDIUM audit-trail evasion on redirect loops (error path + redirect-back-to-first-party mislabel); LOW/MEDIUM operator cannot arm multiple targets (comma list breaks normalizeDnsHostToAscii).
✅ glm (Claude) (glm-5.1, 427946ms)
GLM-routed Claude. CRITICAL: bob_browser_navigate roamed host is DNS-rebindable to internal/metadata — kernel does one-shot resolve, not a connection pin. HIGH: roam = any public host on the internet, no allowlist/relationship-to-target check, blast radius >> docs. MEDIUMs: browser path has no credential-strip analog (leans on Chrome origin scoping); authed_fetch can't roam while navigate can (asymmetry). LOWs: error-path audit hides roamed hop; initial-hop guard only covers auth_profile, not caller-supplied credential headers.
Out-of-diff findings (9)
security
- 🟠 high
mcp/browser-driver.js— Codex [unanchored]: CRITICAL: bob_browser_navigate to a roamed host is not IP-pinned — DNS rebinding becomes browser-side SSRF (browser-driver.js not in diff; enabled by this change) - 🟠 high
mcp/browser-driver.js— agy [unanchored]: CRITICAL: browser DNS rebinding — only the launch targetUrl is pinned via --host-resolver-rules; roamed navigate hosts are re-resolved by Chrome - 🟠 high
mcp/browser-driver.js— glm (Claude) [unanchored]: CRITICAL: navigate's roam guard is a one-shot check, not a connection pin — the PR's 'pins the resolved IP' guarantee is false for the browser - 🟠 high
mcp/lib/offensive-massread-producer.js— Codex [unanchored]: CRITICAL: target auth cookies can be rebound to roamed hosts via the browser mass-read path - 🟡 medium
mcp/browser-driver.js— glm (Claude) [unanchored]: Browser path has no credential-strip analog — the off-site credential guarantee is delegated to Chrome's origin model, not enforced - 🔵 low
mcp/lib/http-scan.js— glm (Claude) [sub-threshold]: roamedInitialRequest guard covers only auth_profile headers, not caller-supplied credential headers
design
- 🔵 low
mcp/browser-driver.js— glm (Claude) [unanchored]: authed_fetch cannot roam (pin-blocked) while navigate can — the two browser transports handle roam oppositely, undocumented - 🔵 low
mcp/lib/scope.js— agy [sub-threshold]: Operator cannot arm roam for multiple targets — a comma/space list breaks normalizeDnsHostToAscii and disables roam entirely
maintainability
- 🔵 low
mcp/lib/http-scan.js— glm (Claude) [unanchored]: Error-path audit hides the roamed hop, undercutting the PR's own 'loud in the trail' goal
Brutalist orchestrator schemaVersion=1 · context_id=25a1a9dd-d755-4a1c-bb75-60908c14ecf1
| // (initialScopeDecision.reason === operator_armed_roam): a credential bound to the target must never | ||
| // be sent to a different site, even on a direct operator-armed roam request (round-2 agy HIGH) — the | ||
| // redirect path strips cross-site too; this closes the initial hop. A roamed host proceeds uncredentialed. | ||
| const roamedInitialRequest = !!(initialScopeDecision && initialScopeDecision.reason === "operator_armed_roam"); |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟠 high
[Claude 🟠 high] security — Initial-hop credential strip lives only in http-scan; other credentialed safeFetch callers leak target creds to roamed hosts
The PR's stated invariant — "a credential bound to the target must never be sent to a different site, even on a direct operator-armed roam request" — is implemented ONLY here in http-scan.js (the roamedInitialRequest guard skipping applyAuthProfileHeaders). But safeFetch is the chokepoint, and it only strips credentials on REDIRECT hops; the first hop trusts options.headers verbatim. So any other credentialed caller of safeFetch that reaches a roamed host on its first request leaks the target's credentials off-site. Verified concrete path: offensive-idor-producer.js:1035 (runProbe) calls safeFetch with headers carrying the IDOR identity's real auth, followRedirects:false, and NO roam guard. With roam armed, validateHttpScanScope now ALLOWS a cross-host url; followRedirects:false means the redirect-strip never runs and there is no roamedInitialRequest check — so the target's session credentials are sent directly to a roamed host. offensive-cors/reflect producers and signup.js are the same shape. Fix: move the invariant into safeFetch itself — before the first requestOnce, if the hop host is not first-party (or scopeDecision.reason===operator_armed_roam), apply stripCredentialHeaders. Relying on 6+ call sites to each re-implement the guard is how this regresses.
| const auditTs = new Date().toISOString(); | ||
| // Audit the FINAL hop's scope reason: a first-party request that redirects INTO a roamed host must | ||
| // record operator_armed_roam, not the initial first-party decision (round-2 CodeRabbit/Codex). | ||
| const auditScopeDecision = scopeReason |
There was a problem hiding this comment.
🪓 Brutalist — 2 critics, rollup: 🟡 medium
[agy 🟡 medium] security — Audit-trail evasion on redirect loops: roamed-then-back-to-first-party (and error hops) mislabeled
The scope reason is audited from response.scopeReason (the final hop). Scenario A: a scan initiated to a roamed host (allowed via operator roam) that redirects back to the first-party domain terminates with scopeReason "first_party_host" — the log attributes a roam-initiated request to first_party_host. Scenario B: a first-party scan that redirects to a roamed host and then fails (DNS error/timeout) falls into the catch block, which audits with initialScopeDecision.reason (first_party_host), obscuring that a roamed connection was attempted. Net: scans targeting roamed hosts or failing on a roamed redirect hop are recorded as first_party_host, making out-of-scope activity appear authorized under standard policy. Audit the per-hop decisions, not just the final hop's reason.
[Claude 🟡 medium] security — Redirect-into-roam audit row keeps the original first-party host's suffix metadata
auditScopeDecision spreads the INITIAL (first-party) decision and overwrites only reason. So when a first-party request redirects into a roamed host, the audit row shows scope_reason:operator_armed_roam and final_url: (correct) — but registrable_domain / public_suffix / public_suffix_source still describe the ORIGINAL first-party target, not the roamed host the request actually landed on. The whole selling point of roam is "every roamed host stays visible in the audit"; here the host-descriptor fields lie about which host was reached. safeFetch only carries response.scopeReason back, not the roamed decision's suffix info — carry the full final-hop scopeDecision instead and audit that.
| const scopeDecision = assertSafeRequestUrl(currentUrl, targetDomain, { blockInternalHosts }); | ||
| // A roamed hop ALWAYS resolves + blocks internal IPs, even if the caller disabled blockInternalHosts — | ||
| // a public name that resolves to an internal IP cannot become a roam SSRF (DNS rebinding). (CRITICAL.) | ||
| const hopBlockInternal = blockInternalHosts || !!(scopeDecision && scopeDecision.enforce_internal_block); |
There was a problem hiding this comment.
🪓 Brutalist — 2 critics, rollup: 🟡 medium
[agy 🟡 medium] security — IP-pin bypassed under egress proxy: roamed host can be DNS-rebound at the proxy's resolution layer
requestOnce's lookup override (the IP pin) is only honored when Node establishes the socket directly. Custom HTTP/SOCKS proxy agents create their own sockets and tunnels and do NOT invoke requestOptions.lookup — they resolve the hostname themselves or hand it to the proxy to resolve. When an egress proxy agent is passed and blockInternalHosts is forced for a roamed request, resolveSafeAddress verifies the host is public and sets the lookup, but the proxy ignores it. So the pin is completely bypassed when an egress proxy is in use: a roamed hostname can be rebound to a private IP at the proxy resolution level, allowing unauthorized internal access. The new hopBlockInternal forcing here gives a false sense of safety for the proxy path.
[Claude 🟡 medium] security — Roam internal-block degrades from a pin to a point-in-time check under an egress proxy; http-scan path does not fail closed
The "roam can't reach internal, even via DNS rebinding" promise rests on pinning the resolved IP via requestOnce's lookup override (safe-fetch.js:212-219). For a roamed hop hopBlockInternal forces resolution, so selectedAddress is computed and lookup is set — BUT when options.agent is an egress proxy, the proxy does its own DNS for the host and ignores that lookup. The local resolveSafeAddress then degrades from a pin to a mere point-in-time check, which a rebinding attacker defeats (public at check time, internal at the proxy's connect time). The codebase already fails CLOSED for this elsewhere (browser-driver authed_fetch and offensive-confirmer both throw under proxy+blockInternalHosts), but http-scan→safeFetch for a roamed request proceeds on the weakened check. The doc claim "pins the resolved IP for the connection, so DNS rebinding can't smuggle one through" is false for the proxy case. Fix: a roamed hop (enforce_internal_block) must fail closed when options.agent is a proxy, mirroring confirmer/authed_fetch — or restrict roam to direct egress like the lab path does.
| // Carry THIS hop's scope reason on the response so the caller can audit the FINAL hop — in particular a | ||
| // first-party request that REDIRECTS into a roamed host audits operator_armed_roam, not the initial | ||
| // first-party decision (round-2 CodeRabbit/Codex). The returned response is the final hop's. | ||
| response.scopeReason = scopeDecision && scopeDecision.reason ? scopeDecision.reason : null; |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Codex 🟡 medium] security — safeFetch returns only response.scopeReason, so the final-hop roam audit is materially wrong on first-party→roamed redirects
safeFetch returns only response.scopeReason back to the caller, and httpScan builds auditScopeDecision by copying the initial decision and replacing only reason. The suffix fields, registrable_domain, and audit base host remain from the initial URL. Impact: a request from target.com redirected to idp.example.org audits scope_reason:operator_armed_roam but still carries target-domain suffix metadata, so the "every roamed host stays visible in the audit" guarantee is unreliable unless someone manually parses final_url. Carry the full final-hop scope decision (including roamed suffix info) out of safeFetch, not just the reason string.
| // browser navigate path does) — closing the round-2 CRITICALs (lab-escape SSRF + rebind-to-internal). | ||
| let roamedHostIsPublic = false; | ||
| try { | ||
| assertHttpScopeDomain(host, { ignoreLabAttestation: true }); |
There was a problem hiding this comment.
🪓 Brutalist — 2 critics, rollup: 🟡 medium
[glm (Claude) 🟡 medium] security — Roam authorizes ANY public host on the internet, not engagement-related hosts — no allowlist, blast radius >> docs
The roam gate runs assertHttpScopeDomain(host, {ignoreLabAttestation:true}) and, if the host is any registrable public domain, returns allowed. There is NO relationship check between the roamed host and the target — not a suffix, not an allowlist, not an operator-supplied host set. BOB_HTTP_ROAM_AUTHORIZED=target.com is target-bound (it only arms sessions whose target_domain===target.com), but once armed it disables host confinement for that session ENTIRELY: bob_http_scan/safeFetch can hit example.com, a competitor's infrastructure, a third-party victim — anything public. The docs frame this as "follow an OAuth IdP / CDN / sibling app," implying a bounded set; the implementation is "the whole public internet minus RFC1918/metadata." For a scope kernel whose entire purpose is confinement this is a much larger relaxation than an operator reading the docs would infer. Fix: narrow to an explicit operator host-allowlist (e.g. BOB_HTTP_ROAM_HOSTS=idp.example,cdn.example), or at minimum state plainly in the docs that arming roam authorizes requests to ANY public host.
[Codex 🟡 medium] design — Roam bolted into the global scope primitive — every assertSafeRequestUrl caller silently inherited roam semantics
validateHttpScanScope is now both the "request may be sent" authority AND the "cookie may be installed / surface endpoint may be used" authority. Callers use assertSafeRequestUrl for different security questions but all now inherit operator_armed_roam: I verified resolveSurfaceEndpoint (offensive-http-common.js:563/587/606), offensive-runner, oob-collector, reflect/xss-exec/idor producers all call assertSafeRequestUrl with SCOPE_VALIDATION_OPTS. Adding a request-scope exception silently widened cookie scope, offensive producer endpoint scope, browser start scope, and navigate scope. This is the root cause of the cookie-rebind leak. One load-bearing scope function is doing too many jobs; the "request roam" vs "credential/scope ownership" distinction is not encoded in the API, so more leaks will appear as new tools reuse the helper.
There was a problem hiding this comment.
🪓 Brutalist Review
Both working critics (native Claude and the GLM-routed Claude client) independently converged on the same headline: the roam feature's central safety claim — 'a roamed request pins the resolved IP so DNS rebinding can't smuggle one through' — is TRUE for the http-scan path but FALSE for the browser bob_browser_navigate path the docs explicitly name. navigate() validates the roamed host with a Node-side resolve-and-discard (assertSafeResolvedRequestUrl) then hands the raw URL to Chrome's page.goto, which re-resolves independently with no --host-resolver-rules pin for the roamed host — a TOCTOU/rebinding SSRF-to-metadata that roam newly opens (cross-host navigation was previously blocked outright). I verified this path routes through validateHttpScanScope's roam branch, so the gap is real, not theoretical; it is HIGH rather than critical only because it is contingent on the operator arming roam and an attacker controlling DNS for a roamed host. The critics agree the scope-kernel core (scope.js) is well-designed and the round-2 CRITICALs (lab-blind roam, allowlist cred-strip, IDN normalization) are genuinely closed. Secondary agreements: the direct-roam path strips auth_profile but not caller-supplied credential headers (asymmetric with the redirect allowlist), and the redirect-into-roam audit row records operator_armed_roam beside the target's stale suffix fields. GLM adds two useful transport-seam observations the native critic missed: authed_fetch+roam is unusable (fails closed, doc mismatch) and roam is host-unbounded once armed (arbitrary uncredentialed public egress). agy false-refused and codex hit a rate limit, so this is a two-critic review. Net recommendation the critics share: the scope-kernel core is approvable, but do not merge the README 'pins the resolved IP' claim for the browser path until navigate actually pins-or-refuses roamed hosts.
Inline comments: 3 (3 🟡 medium)
Per-CLI breakdown
✅ Claude (default, 238673ms)
Native Claude critic. Read the actual changed files plus browser-driver.js and traced every roam-reachable egress path end-to-end. Headline: the feature's own 'pins the resolved IP so DNS rebinding can't smuggle through' guarantee is FALSE for the browser-navigate path it names (assertSafeResolvedRequestUrl resolves-and-discards; page.goto re-resolves in Chrome unpinned) — SSRF-to-metadata under armed roam. Also flagged the direct-roam caller-header cred-strip asymmetry (medium) and the redirect-into-roam audit-suffix conflation (low). Verified correct: lab-attestation isolation, exact-host IDNA target binding, http-tool rebinding pin, and the redirect credential-strip allowlist.
✅ glm (Claude) (glm-5.1, 559085ms)
Claude-routed GLM-5.1 client. Independently confirmed the browser-navigate DNS-rebinding SSRF (three transports, three rebinding postures: http pins / authed_fetch refuses-unpinned / navigate resolves-and-hopes). Additionally found: authed_fetch+roam is dead-on-arrival (always refused as unpinned — doc/impl mismatch, medium); audit records stale first-party suffix with operator_armed_roam and never persists enforce_internal_block (medium); roam is target-bound for arming but host-UNBOUNDED for use, an uncredentialed arbitrary-public-egress primitive (medium, design); integration paths untested (low); proxy+roam no-internal not guaranteed (low).
✅ agy (Gemini 3.5 Flash (Medium), 12694ms)
Refused the task ('Sorry, I cannot fulfill your request to analyze the provided code snippet for security vulnerabilities'). Produced no findings — a safety-filter false refusal on a legitimate defensive security review, not a substantive result.
❌ Codex (default, 29397ms)
Failed: hit rate/usage limit before producing output. No findings available from this critic.
Out-of-diff findings (5)
security
- 🟠 high
mcp/browser-driver.js— Claude [unanchored]: DNS-rebinding SSRF on the roamed browser-navigate path — the README 'pins the resolved IP' guarantee is FALSE for this transport - 🟠 high
mcp/browser-driver.js— glm (Claude) [unanchored]: Roamed bob_browser_navigate resolves-and-hopes (no IP pin) — README rebinding invariant broken for the navigate transport - 🔵 low
mcp/lib/safe-fetch.js— glm (Claude) [unanchored]: Proxy egress + roam does not guarantee the no-internal property (lab forbids proxies; roam does not)
correctness
- 🟡 medium
mcp/browser-driver.js— glm (Claude) [unanchored]: authed_fetch + roam is dead-on-arrival — docs claim the chokepoint covers authed_fetch but roamed hosts are always refused
testing
- 🔵 low
test/operator-armed-roam.test.js— glm (Claude) [unanchored]: Load-bearing integration paths (redirect+roam cred-strip, rebind-block, browser roam) are asserted only in prose
Brutalist orchestrator schemaVersion=1 · context_id=1821424f-0300-46e1-a212-ef1ec9b3ff98
| // (initialScopeDecision.reason === operator_armed_roam): a credential bound to the target must never | ||
| // be sent to a different site, even on a direct operator-armed roam request (round-2 agy HIGH) — the | ||
| // redirect path strips cross-site too; this closes the initial hop. A roamed host proceeds uncredentialed. | ||
| const roamedInitialRequest = !!(initialScopeDecision && initialScopeDecision.reason === "operator_armed_roam"); |
There was a problem hiding this comment.
🪓 Brutalist — 2 critics, rollup: 🟡 medium
[Claude 🟡 medium] security — Direct roamed request strips auth_profile but replays caller-supplied credential headers (args.headers) unfiltered
On a direct operator-armed roam request, applyAuthProfileHeaders is correctly withheld — but headers = args.headers || {} (http-scan.js:196) is passed to safeFetch unfiltered on that first hop. The redirect path applies the full stripCredentialHeaders allowlist (safe-fetch.js), but the initial direct roam hop does not. A caller/agent that placed Cookie, Authorization, or a custom X-Api-Key in args.headers replays it verbatim to the roamed cross-host origin on the first hop — while the identical header would be stripped if reached via redirect. The allowlist built for cross-site protection is applied asymmetrically: strict on redirects, absent on the direct initial roam. Fix: when roamedInitialRequest is true, run headers = stripCredentialHeaders(headers) before dispatch, mirroring the redirect path.
[glm (Claude) 🔵 low] security — Direct roamed path uses weaker cred-stripping than the redirect path (caller headers pass through)
http-scan.js blocks applyAuthProfileHeaders for a roamed initial request (correct), but args.headers (agent-supplied) flow through unfiltered. So the redirect path uses an allowlist (stripCredentialHeaders) and strips custom auth headers, while the direct roamed path only blocks the profile application. An agent that manually places the target's Cookie/Authorization/X-Api-Key into args.headers replays them to the roamed host. The README's guarantee is scoped to auth_profile specifically, so this is technically in-spec — but the two paths use different cred-stripping strengths, and defense-in-depth would run the same allowlist on a direct roamed request.
| const roamedInitialRequest = !!(initialScopeDecision && initialScopeDecision.reason === "operator_armed_roam"); | |
| const roamedInitialRequest = !!(initialScopeDecision && initialScopeDecision.reason === "operator_armed_roam"); | |
| if (!roamedInitialRequest) { | |
| headers = applyAuthProfileHeaders(headers, auth); | |
| } else { | |
| headers = stripCredentialHeaders(headers); | |
| } |
| const auditTs = new Date().toISOString(); | ||
| // Audit the FINAL hop's scope reason: a first-party request that redirects INTO a roamed host must | ||
| // record operator_armed_roam, not the initial first-party decision (round-2 CodeRabbit/Codex). | ||
| const auditScopeDecision = scopeReason |
There was a problem hiding this comment.
🪓 Brutalist — 2 critics, rollup: 🟡 medium
[glm (Claude) 🟡 medium] correctness — Audit records stale first-party suffix with operator_armed_roam; enforce_internal_block never persisted
The PR threads the final-hop reason but not the final-hop's suffix fields. On the canonical roam case (a first-party request that redirects INTO a roamed host), initialScopeDecision is first_party_host (registrable_domain: target) and scopeReason is operator_armed_roam; the merged object emits scope_reason: operator_armed_roam next to registrable_domain: <target> — the audit says 'roamed' while describing the target, not where the request landed. For a control whose entire rationale is making roamed hops 'loud in the trail,' the suffix provenance is misleading. Additionally, enforce_internal_block is never persisted at all — for a security-critical control, the audit should record that internal-blocking was enforced on the roamed hop. Thread the full final-hop scope decision (roamed host's registrable_domain/public_suffix + enforce_internal_block) instead of overlaying only reason.
[Claude 🔵 low] correctness — Audit row conflates two hosts on redirect-into-roam: scope_reason=operator_armed_roam alongside the TARGET's suffix fields
auditScopeDecision = { ...initialScopeDecision, reason: scopeReason } threads the final-hop reason but not the final-hop's suffix fields. On a first-party→roam redirect, reason becomes operator_armed_roam while registrable_domain/public_suffix/public_suffix_source (via scopeAuditFields) remain the TARGET's, not the roamed host's. The audit row then reads scope_reason: operator_armed_roam next to the target's registrable domain, with only final_url revealing where the request actually landed. For a feature whose selling point is 'every roamed host stays visible in the audit,' a row whose suffix fields describe a different host than its reason is a triage trap. Carry the final hop's full suffix info rather than only overriding reason.
| // already returned, so this never relaxes an attested private target; block_internal_hosts (enforced | ||
| // separately at DNS resolution in safe-fetch.js) still blocks internal/metadata IPs. The roamed host | ||
| // is described from ITS OWN public-suffix info so the audit shows exactly where the request went. | ||
| if (roamAuthorizedForTarget(domain)) { |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[glm (Claude) 🟡 medium] design — Roam is target-bound for arming but host-UNBOUNDED for use — an uncredentialed arbitrary-public-egress primitive
BOB_LAB_TARGET binds a grant to exact named hosts. BOB_HTTP_ROAM_AUTHORIZED binds only the target identity; once armed, validateHttpScanScope permits ANY registrable public host, and the agent — including a prompt-injected one — chooses which public hosts to roam to. Credentials are correctly stripped to roamed hosts (so it is not a cred-leak), but it remains an uncredentialed arbitrary-public-egress / data-exfil primitive the agent controls for the session: phone-home to attacker-controlled public hosts, probe third-party APIs from Bob's egress, etc. The asymmetry with the exact-host lab grant is worth flagging: this is a far wider blast radius than 'follow an engagement off the apex' implies. If the intent is engagement-specific hosts, an allowlist (like BOB_LAB_TARGET's exact-host binding) would match the lab feature's posture; as shipped, roam is 'the whole public internet for this session, minus internal.'
1 participant
Operator-armed cross-host roam (default-OFF)
Lets the operator authorize Bob to follow an engagement off the apex — an OAuth/SSO IdP, a CDN/asset host, a sibling app, or an authorized redirect/SSRF chain — when the program scope covers those hosts. Requested by the operator ("Everywhere — let bob decide what he needs to explore").
The change
validateHttpScanScoperejects a URL whose host is outside the session'starget_domain. This adds a target-bound, env-armed relaxation of exactly that branch:BOB_IDOR_PROVISION_AUTHORIZED: a confined MCP/Bash agent cannot set the server'sprocess.env, so this is an operator decision, not an agent one.target_domainequals the value (trim + case-insensitive, whole-target — a public-suffix fragment likenldoes not match). Arming one engagement never relaxes another.validateHttpScanScopebacks the HTTP tools (safe-fetch), the browser driver (navigate/authed_fetchviaassertSafeResolvedRequestUrl), and redirect-following — one relaxation covers them all.scope_decision: allowed, reason: operator_armed_roam, so every roamed host is visible in the request audit.What roam deliberately does NOT relax (the two SSRF-relevant boundaries)
127.0.0.1/RFC1918 session can never pivot to169.254.169.254or a LAN neighbour even with roam armed. Asserted by a dedicated test.block_internal_hostsis a separate DNS-resolution policy (safe-fetch.js resolveSafeAddress), independent ofvalidateHttpScanScope. Roam relaxes the target-domain boundary only — to also reach internal IPs the operator must separately disableblock_internal_hosts. Roam alone is not SSRF-to-internal.Tests (
test/operator-armed-roam.test.js, 7)off → cross-host blocked · on → cross-host allowed (
operator_armed_roam, roamed host described from its own suffix) · armed-for-a-different-target → blocked · suffix-no-match → blocked · first-party URL stillfirst_party_host· lab target stays locked under roam (no 169.254 pivot) ·roamAuthorizedForTargetunit (empty/whitespace/mismatch/exact).docs/FIRST_RUN.mddocuments the arm + both non-relaxed boundaries.test:mcp3180/0,test:promptsclean,check:syntaxclean.Scope / honest notes
Summary by CodeRabbit