fix(api): scrub response bodies and PII-keyed extras from Sentry events

[thomasluizon]

What

The Sentry BeforeSend scrubber (ServiceCollectionExtensions.ScrubSensitiveData) cleared the request body, headers and cookies plus top-level user PII, but left several sensitive vectors untouched. Verified against the Sentry .NET SDK 6.6.0 protocol types (SentryEvent, SentryUser, SentryRequest, SentryContexts.Response), the gaps were:

• Response body — sentryEvent.Contexts.Response (its Data, Cookies, Headers) was never scrubbed. This is the response-body gap called out for verification.
• Request query string — Request.QueryString can carry tokens/emails and was not cleared.
• User Other dictionary — arbitrary user PII survived (only Email/Username/IpAddress were cleared).
• Extra bag — no key-based PII redaction, so a known-PII-keyed extra (e.g. authToken, userEmail) shipped verbatim.

Change

• Extracted the scrubber into a focused, testable Orbit.Api.Observability.SentryEventScrubber (internal; Orbit.Api already exposes internals to Orbit.Infrastructure.Tests).
• Now strips: user PII incl. Other; request body/headers/cookies/query-string; the response context (body/cookies/headers); and redacts Extra entries whose keys match known-PII fragments while preserving benign debug context.
• ScrubResponse iterates existing contexts rather than the lazy Contexts.Response accessor, so it never fabricates an empty response context on clean events.

Tests

Added SentryEventScrubberTests (6 focused cases): response body/headers/cookies stripped; no response context fabricated when absent; user PII incl. Other cleared; request body/cookies/headers/query-string cleared; known-PII Extra keys redacted while benign keys survive; and the no-op path returns the same event. Full suite green (Domain 504, Analyzers 7, Application 2628, Infrastructure 1540).

Scope

API-internal observability only — no endpoint or DTO contract change, so no packages/shared / consumer edits apply (parity-nudge false positive).

Refs thomasluizon/orbit-ui-mobile#243

[claude]

Code Review: PR #337 (thomasluizon/orbit-api)

Scope: PR #337 — fix(api): scrub response bodies and PII-keyed extras from Sentry events
Recommendation: APPROVE

Summary

This PR extracts the Sentry BeforeSend scrubber into a focused, unit-tested SentryEventScrubber and closes four real, previously-unscrubbed PII vectors: response context (body/cookies/headers), request query string, SentryUser.Other, and key-matched Extra entries. The diff is small (3 files), internal-only (no DTO/Controller/contract surface), and backed by 6 targeted unit tests. No prior review threads or bot reviews exist on this PR to reconcile against.

Findings

Critical

None.

High

None. (A security-reviewer subagent initially flagged unscrubbed Sentry Breadcrumbs as High. An adversarial skeptic pass investigated the specific claimed mechanisms against this codebase and refuted them: no Sentry.EntityFrameworkCore package is referenced anywhere in the solution, so EF breadcrumbs don't exist; the more plausible mechanism — ASP.NET Core request-logging breadcrumbs carrying the incoming query string — is already suppressed by the pre-existing "Microsoft.AspNetCore": "Warning" log-level filter in appsettings.json/appsettings.Production.json. The gap is also pre-existing (the old scrubber never touched Breadcrumbs either) and doesn't meet the Critical bar. Dropped per the verification protocol.)

Medium

None. (A candidate — SensitiveKeyFragments in src/Orbit.Api/Observability/SentryEventScrubber.cs:10-16 lacks a bare "key" fragment, so a hypothetical Extra key like "stripeKey" would slip through — was considered but dropped as speculative: grep -rn "SetExtra" across src/ shows the only call site is inside the scrubber itself; there is no current code path that would set such a key. Per the rubric's signal gate, Medium findings must be concretely actionable, not speculative.)

Low / Info

None posted (per rubric signal gate).

Subagents

Agent	Verdict
security-reviewer	PASS (one High finding raised, refuted by adversarial skeptic and dropped — see High section)
contract-aligner	N/A — no DTO, Controller route, or packages/shared surface changed in this diff

Validation

Check	Result
Build (dotnet)	N/A — skipped in CI job, covered by separate required check
Tests (dotnet)	N/A — skipped in CI job, covered by separate required check

Deferred — N/A dimensions & files not verdicted

• DESIGN.md/AI-slop (#8) — N/A, no apps/* UI files touched.
• Parity (#9) — N/A, backend-only change, no apps/web/apps/mobile files.
• i18n (#10) — N/A, no new user-facing strings.
• Contract drift + backward-compat guard (#11) — N/A, no DTO/Controller/packages/shared change; Sentry telemetry scrubbing is server-internal and invisible to any client contract, confirmed via diff (only ServiceCollectionExtensions.cs, SentryEventScrubber.cs, and a test file changed).
• Backend hard rules (#13) — Timezone/Authorization/Validation/Transaction teardown — N/A, no surface touched (no dates, no endpoints, no transactions in this diff).
• FEATURES.md parity (#14) — N/A, no user-facing feature surface changed (internal hardening only, matches PR body's own scope statement); not verifiable in this checkout regardless.
• Cross-repo dimensions requiring orbit-ui-mobile — not verifiable in this CI job (sibling repo not checked out), per task instructions. Confirmed N/A anyway since no contract surface changed.
• Second-opinion cross-model step (verification protocol §2.2) — no Critical finding survived to trigger it; UNAVAILABLE would apply regardless (no opencode in CI).

All three changed files (ServiceCollectionExtensions.cs, Observability/SentryEventScrubber.cs, tests/.../SentryEventScrubberTests.cs) received an explicit verdict — nothing in the diff went unexamined.

What's good

• Genuine root-cause fix: closes four concretely identified PII gaps (response context, query string, User.Other, key-matched Extra), each with its own unit test.
• ScrubResponse correctly iterates existing Contexts.Values rather than the lazy Contexts.Response accessor, avoiding fabricating an empty response context on clean events — verified by its own test (Scrub_DoesNotAddResponseContext_WhenNonePresent).
• Clean extraction: old dead ScrubSensitiveData fully removed with zero orphan references (verified via repo-wide grep); new internal static class correctly reuses the existing InternalsVisibleTo Orbit.Infrastructure.Tests grant rather than widening visibility.
• Zero narration comments in either new file (ORBIT0001-clean).
• Test placement matches the tests/CLAUDE.md convention given no Orbit.Api.Tests project exists.

Recommendation

Approve as-is. Optional non-blocking follow-up (not required for this PR): consider scrubbing sentryEvent.Breadcrumbs in a future hardening pass for defense-in-depth against future integrations (e.g., if Sentry.EntityFrameworkCore or HttpClient breadcrumb capture is ever added), but nothing in the current codebase makes this an active leak.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
93.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[claude]

PR Review — #337: fix(api): scrub response bodies and PII-keyed extras from Sentry events

Recommendation: APPROVE

Severity	Count
Critical (incl. breaks old mobile clients)	0
High	0
Medium	0
Low / Info	0

Basis: The PR head (8f8b107b) is identical to the commit already reviewed by this bot in a prior pass — no new commits have landed since. Re-running the /pr-review skill against the current diff found nothing new to examine, so the prior findings stand:

• Genuine root-cause fix closing four PII leak paths in Sentry event capture: response context, request query string, SentryUser.Other, and key-matched Extra fields — each covered by a dedicated unit test in tests/Orbit.Infrastructure.Tests/Observability/SentryEventScrubberTests.cs.
• No DTO/Controller/contract-surface change, so the cross-repo contract-parity dimension is not applicable (and not verifiable in this CI job regardless, since orbit-ui-mobile isn't checked out here).
• A security-reviewer-raised High (potentially unscrubbed Sentry Breadcrumbs) was investigated and refuted: no Sentry.EntityFrameworkCore package is referenced in the solution, and ASP.NET Core request-logging breadcrumbs are already suppressed by the existing Microsoft.AspNetCore: Warning log filter — correctly dropped as a false positive, not re-flagged.

No new Critical/High/Medium findings to report.