fix(policy): block POST to sentry.io to prevent multi-tenant data exfiltration (#1437)#1565
Conversation
…iltration sentry.io is a multi-tenant SaaS — any client with a project ID can POST to any Sentry project, not just NemoClaw's. The baseline sandbox policy allowed POST to sentry.io with path '/**', which turned the host into a generic exfiltration channel: a compromised agent inside the sandbox could ship stack traces, env vars, file contents, etc. to a Sentry project controlled by an attacker via the public envelope endpoint (https://sentry.io/api/<any-project-id>/envelope/). Path-pattern restrictions cannot fix this because the project ID is part of the URL and there is no server-side allowlist of legitimate projects. This is a follow-up to NVIDIA#1214 (which added 'protocol: rest' for sentry.io) — that PR closed the wire-protocol gap, this PR closes the remaining HTTP-method-level gap. Changes: - nemoclaw-blueprint/policies/openclaw-sandbox.yaml: drop the 'method: POST, path: /**' allow rule for sentry.io. GET stays allowed because GET has no request body and is harmless for exfil. Side effect: Claude Code's crash telemetry to Sentry is silently dropped. That is the correct tradeoff for a sandbox whose stated goal is preventing data egress, and the sandbox already blocks many similar telemetry channels by default. - test/validate-blueprint.test.ts: walk every endpoint in network_policies, find sentry.io, and assert (a) at least one sentry.io entry exists, (b) no sentry.io entry has a POST allow rule, (c) the GET allow rule is preserved. Verified by stashing the policy fix and re-running: the test correctly fails on main with the unfixed policy, and passes with the fix in place. Closes NVIDIA#1437
📝 WalkthroughWalkthroughThe changes remove POST method access to sentry.io endpoints from the sandbox network policy to mitigate data exfiltration risk, and introduce regression tests to validate that sentry.io endpoints permit only GET requests. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/validate-blueprint.test.ts (1)
122-130: Make the GET regression test self-contained.If this test is run alone, it can pass vacuously when no
sentry.ioendpoint exists. Add a local non-empty assertion here too.Suggested patch
it("regression `#1437`: sentry.io retains GET (harmless, no body for exfil)", () => { const sentryEndpoints = findEndpoints((h) => h === "sentry.io"); + expect(sentryEndpoints.length).toBeGreaterThan(0); for (const ep of sentryEndpoints) { const rules = Array.isArray(ep.rules) ? ep.rules : []; const hasGet = rules.some( (r) => r && r.allow && typeof r.allow.method === "string" && r.allow.method.toUpperCase() === "GET", ); expect(hasGet).toBe(true); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/validate-blueprint.test.ts` around lines 122 - 130, The test currently can pass vacuously if no sentry.io endpoints exist; update the regression test in validate-blueprint.test.ts (the "regression `#1437`: sentry.io retains GET..." case) to assert that the found sentryEndpoints is non-empty before checking rules: use findEndpoints(...) result (sentryEndpoints) and add an assertion that its length is > 0 (or that at least one endpoint object exists) so the subsequent hasGet assertions are meaningful; keep the existing per-endpoint hasGet check for each ep.rules to ensure a GET rule exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/validate-blueprint.test.ts`:
- Around line 122-130: The test currently can pass vacuously if no sentry.io
endpoints exist; update the regression test in validate-blueprint.test.ts (the
"regression `#1437`: sentry.io retains GET..." case) to assert that the found
sentryEndpoints is non-empty before checking rules: use findEndpoints(...)
result (sentryEndpoints) and add an assertion that its length is > 0 (or that at
least one endpoint object exists) so the subsequent hasGet assertions are
meaningful; keep the existing per-endpoint hasGet check for each ep.rules to
ensure a GET rule exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 125af1fc-f730-4cc7-b414-1d65fc5ee6e5
📒 Files selected for processing (2)
nemoclaw-blueprint/policies/openclaw-sandbox.yamltest/validate-blueprint.test.ts
Summary
Closes #1437.
The baseline sandbox network policy allowed
POSTtosentry.iowithpath: "/**". Because sentry.io is a multi-tenant SaaS — any client with a project ID can post to any Sentry project — this turned the host into a generic exfiltration channel: a compromised agent inside the sandbox could ship stack traces, environment variables, file contents, etc. to a Sentry project controlled by an attacker via the public envelope endpoint:Path-pattern restrictions cannot fix this on their own — the project ID is part of the URL and there is no server-side allowlist of legitimate projects from the policy engine's perspective.
This is a follow-up to #1214, which added
protocol: restto the sentry.io entry. That PR closed the wire-protocol gap; this PR closes the remaining HTTP-method-level gap.Fix
Drop the
method: POST, path: "/**"allow rule from the sentry.io endpoint innemoclaw-blueprint/policies/openclaw-sandbox.yaml.GETstays allowed becauseGEThas no request body and is harmless for exfiltration.Side effect: Claude Code's crash telemetry to Sentry is silently dropped from inside the sandbox. That is the right tradeoff for a sandbox whose stated goal is preventing data egress, and the sandbox already blocks many similar telemetry channels by default.
A long inline comment in the YAML documents the rationale so a future contributor doesn't re-add the rule "to fix Sentry telemetry" without understanding why it was removed.
Test plan
test/validate-blueprint.test.tswalk every endpoint in every entry undernetwork_policiesand assert:sentry.ioendpoint still exists (so a future edit that removes the host entirely also gets noticed)sentry.ioendpoint has anyPOSTallow rulesentry.ioretains itsGETallow rulemain. Theno POST allow ruleassertion correctly fails, proving the test catches the bug.validate-blueprinttests pass, including the 2 new regressions.sentry.io(grep -rn sentry test/ nemoclaw-blueprint/), so there is no risk of breaking unrelated coverage.Why GET stays
GETrequests cannot carry a request body. Exfiltration viaGETis bounded by the URL length limit and stripped of any meaningful data structure. The realistic exfil channels (envelope, store, security report endpoints) all usePOST. KeepingGETlets any read-only Sentry SDK code paths (e.g. fetching public DSN config) continue to work without re-introducing the exfiltration vector.Summary by CodeRabbit
Bug Fixes
Tests