fix(api): scrub secrets from agent audit log + cascade-delete SentProactiveCheckin (#243)

[thomasluizon]

Part of the #243 prod-readiness campaign (security batch, iteration 1). Paired with thomasluizon/orbit-ui-mobile's security PR. Refs thomasluizon/orbit-ui-mobile#243.

Findings fixed at source (from /prod-readiness + CodeQL)

SEC-H2 (High) — unredacted secrets in the agent audit trail

AgentAuditRedactor masks sensitive JSON fields (code, token, password, secret, refresh_token, verifier, authorization, apikey) at any nesting depth before an agent-tool argument body is persisted to AgentAuditLogs. Wired into both paths that previously only truncated the raw body despite the Redacted* naming:

• legacy MCP audit path (WebApplicationExtensions.TryAuditLegacyMcpAsync)
• structured agent path (AgentOperationExecutor.RedactArguments)

A body that does not parse as JSON is masked whole rather than stored raw. Closes a data-leakage gap where an OTP code / bearer token could land in the audit trail.

SEC-H3 (High security / Critical code-quality) — orphaned PII on account deletion

SentProactiveCheckin was the one user-owned Sent* table with no OnDelete(Cascade) FK and no explicit delete in AccountResetRepository.DeleteAllUserDataAsync, so a user's proactive-checkin history survived account deletion. This adds:

• an ON DELETE CASCADE FK to Users (migration, with a pre-constraint orphan cleanup so it applies cleanly if the pre-cascade bug already left dangling rows)
• an explicit ExecuteDeleteAsync in the reset repository, mirroring every sibling table

CodeQL — least-privilege workflow permissions

sonarcloud.yml: top-level permissions: contents: read (actions/missing-workflow-permissions).

Verification

• dotnet build: 0 errors
• dotnet test (new + touched): 17 passed — AgentAuditRedactorTests (masking, nesting, non-JSON, truncation, non-sensitive passthrough) + AccountDeletionServiceDbTests.DeleteAllUserDataAsync_RemovesSentProactiveCheckins

Not in this batch (verified, deferred)

The security audit's other Critical/High findings were read against the code and found to be false positives / over-rated (Play-notification dedup is already correct via the MessageId unique constraint + atomic SaveChangesAsync; the Play-verify and chat-context "TOCTOU"s are self-healing / re-gated) — except access-token revocation, which is a real design decision filed separately. See the consolidated report.

The SonarCloud Code Analysis gate is red-and-required by design during #243; this merges via --admin once the other required checks pass.

🤖 Generated with Claude Code

[gitguardian]

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.

---

^{_{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}}

[thomasluizon]

Paired consumer PR: thomasluizon/orbit-ui-mobile#448 (ReDoS + CI permissions). Both merge via --admin past the red-by-design Sonar gate once the other required checks pass.

[claude]

Code Review: PR #317 -- thomasluizon/orbit-api

Scope: PR #317, fix(api): scrub secrets from agent audit log + cascade-delete SentProactiveCheckin (#243)
Recommendation: APPROVE

Summary
Two independent security/data-integrity fixes: (1) a new AgentAuditRedactor that JSON-tree-walks and masks sensitive keys before persisting agent-tool arguments to AgentAuditLogs.RedactedArguments, replacing a blind 1000-char truncation on both the legacy MCP audit path and the structured agent path; (2) an ON DELETE CASCADE FK plus explicit ExecuteDeleteAsync so SentProactiveCheckin rows are purged on account deletion, mirroring the existing SentStreakFreezeAlert pattern exactly; plus a least-privilege permissions: contents: read addition to sonarcloud.yml. All changed files were read in full and verified against the working tree. Migration, FK direction, and cascade config all check out correct and consistent with sibling Sent* tables. One legitimate, non-blocking Medium finding on the redactor key-matching gap.

Findings

Critical: None.

High: None.

Medium:

MEDIUM: Redactor asymmetric exact/substring matching misses auth-code-shaped field names

• dimension: Security (data exposure)
• location: orbit-api/src/Orbit.Infrastructure/Services/AgentAuditRedactor.cs:19-27
• issue: "auth" is only in the exact-match SensitiveKeys set, while "token"/"password"/"secret"/"credential"/"apikey" are substring-fragment matched via SensitiveFragments. A field literally named authCode, authorizationCode, or authValue normalizes to "authcode"/"authorizationcode"/"authvalue" -- none equal an exact SensitiveKeys entry, none contain a SensitiveFragments substring -- so it is NOT redacted.
• risk: No current AgentOperation JSON schema in AgentCatalogService.Operations.cs uses this exact shape today (verify_auth_code OTP field is literally "code", which the exact set does catch), so this does not leak a real secret right now. But it is a real inconsistency in a security allowlist: the next OAuth/session-adjacent agent tool that names its field authorizationCode (a very natural name) would silently bypass redaction and land in the audit trail.
• fix: Add "auth" to SensitiveFragments (verified safe -- grepped all AgentOperation property names in AgentCatalogService.Operations.cs and found no benign field containing "auth" as a substring today, so no over-redaction risk), or explicitly enumerate authcode/authorizationcode in SensitiveKeys.
• reference: orbit-api hard rule -- Security / Data exposure

Low / Info:

• Info: GitGuardian flagged a "Generic Password" secret in tests/Orbit.Infrastructure.Tests/Services/AgentAuditRedactorTests.cs. Verified: it is the literal test fixture value "s3cret-value" in an InlineData theory (plus "TOK123"/"PW456" in a sibling test) -- not a real credential. False positive, no action needed.
• Info: The redact-then-serialize-then-truncate-to-1000-chars flow can still produce a truncated (invalid-JSON) string in RedactedArguments for large payloads -- but this is pre-existing behavior (the old code truncated the raw body identically); not a regression introduced by this diff.

Subagents

Agent	Verdict
security-reviewer	PASS (1 Medium: auth-fragment gap in AgentAuditRedactor; migration SQL injection-safe; FK direction correct; redaction confirmed to run before truncation; confirmed AgentAuditRedactor.Redact is the sole writer of RedactedArguments on both paths; sonarcloud.yml permissions confirmed sufficient for the job steps)
contract-aligner	N/A -- diff touches no DTO, Controller route, or packages/shared surface

Validation

Check	Result
Build (dotnet)	N/A -- skipped per instructions (CI runs Build as a separate required check)
Tests (dotnet)	N/A -- skipped per instructions (CI runs Unit Tests as a separate required check); PR body claims 17 passed including new AgentAuditRedactorTests and AccountDeletionServiceDbTests.DeleteAllUserDataAsync_RemovesSentProactiveCheckins

Deferred -- N/A dimensions

• DESIGN.md/AI-slop, Parity, i18n, Contract drift/backward-compat, FEATURES.md parity -- all N/A: backend-only diff, no DTO/endpoint/packages/shared surface changed, no user-facing strings or features touched. Sibling orbit-ui-mobile repo not checked out in this job but not relevant here since no contract surface moved.

What is good

• SentProactiveCheckin FK config (OrbitDbContext.cs:206) is a verbatim mirror of the existing SentStreakFreezeAlert cascade pattern (:215) -- good consistency, and the migration model snapshot / designer files match it exactly.
• The migration correctly cleans up pre-existing orphan rows before adding the FK constraint, so it applies cleanly even if the pre-cascade bug already left dangling rows -- good defensive migration authoring.
• AgentAuditRedactor.Redact redacts the full parsed JSON tree (nested objects and arrays) before truncating the serialized output, so a secret cannot survive by sitting past the 1000-char cutoff -- correctly closes the original truncation-only gap.
• Good test coverage on the new redactor: top-level sensitive keys, nested/array fields, non-sensitive passthrough, non-JSON whole-body masking, null/empty passthrough, and truncation-to-cap are all exercised.
• AccountResetRepository new delete call is placed correctly among the other Sent*/user-owned-data deletes, consistent with the file existing ordering pattern.

Recommendation
Merge as-is. Fix the Medium ("auth" to SensitiveFragments) as a quick, low-risk follow-up before any agent tool exposes an OAuth/session-code-shaped argument -- it is a one-line change with no evidence of current impact, so it does not need to block this PR.

---

Automated review via /pr-review skill.

[claude]

Code Review: PR #317 — fix(api): scrub secrets from agent audit log + cascade-delete SentProactiveCheckin (#243)

Recommendation: APPROVE

Summary

Two independent, well-scoped security fixes from the #243 prod-readiness batch: (1) AgentAuditRedactor now recursively masks sensitive JSON fields (code/token/password/secret/verifier/authorization/apikey, at any nesting depth) before agent-tool argument bodies are persisted to AgentAuditLogs, replacing the previous behavior of merely truncating raw text; (2) SentProactiveCheckin gets an ON DELETE CASCADE FK plus an explicit ExecuteDeleteAsync in AccountResetRepository, closing a PII-orphaning gap on account deletion. A third, trivial change adds least-privilege permissions: contents: read to sonarcloud.yml. All changes are narrowly targeted, covered by new/updated unit tests, and consistent with sibling patterns elsewhere in the codebase.

Findings

Critical

None.

High

None.

Medium

None.

Low / Info

• [Info] Redactor doesn't recurse into a bare top-level JSON scalar — src/Orbit.Infrastructure/Services/AgentAuditRedactor.cs RedactNode only switches on JsonObject/JsonArray; a top-level bare JSON string/number would pass through Truncate() unmasked. Verified against every real argument schema in AgentCatalogService.Operations.cs and the MCP/chat tool implementations — all tool arguments are always JSON objects, so this path isn't reachable with current payloads. Not a fix-blocking finding; worth a defensive test if a future capability ever accepts a bare-scalar argument.

Subagents

Agent	Verdict
security-reviewer	PASS — cross-checked the redactor's key-matching (exact-match set + Contains-based fragment match after normalization) against every real secret-bearing argument key in AgentCatalogService.Operations.cs / MCP tool schemas (code, access_token, refresh_token, google_access_token, google_refresh_token, etc.) — all caught. Confirmed both audit-write call sites (AgentOperationExecutor.cs:361, WebApplicationExtensions.cs:428) route through the shared redactor with no remaining raw-truncate path. Migration/DbContext/Designer/Snapshot cascade config all consistent. sonarcloud.yml permissions correct least-privilege for a read-only-checkout build job.
contract-aligner	N/A — no DTO, Controller route, or packages/shared contract surface touched by this diff.

Validation

Check	Result
Build (dotnet)	N/A — CI scope; Build runs as a separate required check
Tests (dotnet)	N/A — CI scope; Unit Tests run as a separate required check

Deferred — N/A dimensions & files not verdicted

• Dimension 8 (DESIGN.md/AI-slop) — N/A, no apps/* files touched.
• Dimension 9 (Parity) / Dimension 10 (i18n) — N/A, backend-only diff; frontend-owned dimensions don't fire from this repo.
• Dimension 14 (FEATURES.md parity) — N/A, internal security/data-hygiene fix with no user-facing feature/screen/tool/plan-gating change.
• /second-opinion cross-model step — not invoked; no Critical finding survived the skeptic pass to escalate.
• All 11 changed files (workflow, controller-extension, migration ×3, DbContext, repository, redactor, executor, 2 test files) received an explicit verdict above — nothing left un-reviewed.

What's good

• The redaction is root-caused, not patched: it operates on the parsed JSON tree at any depth rather than a flat truncate, and non-JSON bodies are masked whole rather than leaking up to 1000 raw chars (the prior behavior).
• Both audit-write paths (legacy MCP + structured agent) were updated in lockstep — no orphaned raw-truncate path left behind.
• The cascade migration includes a pre-constraint orphan cleanup (DELETE ... WHERE "UserId" NOT IN (SELECT "Id" FROM "Users")), so it applies cleanly even if the pre-cascade bug already left dangling rows — good defensive migration hygiene.
• Solid, focused test coverage: AgentAuditRedactorTests covers top-level masking, nested/array masking, non-sensitive passthrough, non-JSON whole-body masking, null/empty passthrough, and truncation cap; the new AccountDeletionServiceDbTests case exercises the actual cascade-delete path end to end.

Recommendation

Clean to merge as-is. No action required before merge; the one Info-level observation (bare-scalar JSON not reachable today) is optional forward-hardening, not a blocker.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarQube Cloud

[claude]

Code Review: PR #317 — thomasluizon/orbit-api

Scope: PR #317, fix(api): scrub secrets from agent audit log + cascade-delete SentProactiveCheckin (#243)
Recommendation: APPROVE

Summary

Two independent, well-scoped security/data-hygiene fixes plus a trivial CI hardening. (1) A new AgentAuditRedactor JSON-tree-walks agent-tool argument payloads and masks sensitive keys (exact + normalized-substring match) at any nesting depth before persisting to AgentAuditLogs.RedactedArguments, replacing a blind 1000-char raw truncation on both the legacy MCP audit path and the structured agent path. (2) SentProactiveCheckin gets an ON DELETE CASCADE FK plus an explicit ExecuteDeleteAsync in AccountResetRepository, mirroring the existing SentStreakFreezeAlert pattern exactly, closing a PII-orphaning gap on account deletion. (3) permissions: contents: read added to sonarcloud.yml. All 11 changed files were reviewed (10 in full detail; the EF-generated migration .Designer.cs snapshot spot-checked as boilerplate). Prior bot reviews on this same PR already converged on APPROVE; my independent pass plus an independent security-reviewer subagent pass confirm the same, with no new issues found.

Findings

Critical

None.

High

None.

Medium

None.

Low / Info

• [Info, non-blocking, already surfaced in a prior review on this PR] AgentAuditRedactor.cs:19-21 — "auth" is only in the exact-match SensitiveKeys set, not in SensitiveFragments. A hypothetical future field literally named authorizationCode/authCode (compound, not the bare word auth/authorization) would normalize to a string that matches neither the exact set nor a fragment, and would not be redacted. Verified against every real argument schema in AgentCatalogService.Operations.cs (send_auth_code/verify_auth_code/exchange_google_auth/refresh_auth_session/logout_auth_session use code, access_token, google_access_token, refresh_token — all caught today). Not exploitable with any current payload; worth a one-line defensive addition ("auth" to SensitiveFragments) as a follow-up, not a blocker.
• [Info] AgentAuditRedactor.RedactNode only switches on JsonObject/JsonArray; a bare top-level JSON scalar would pass through Truncate() unmasked. Not reachable — every current agent-tool argument schema is a JSON object.

Subagents

Agent	Verdict
security-reviewer	PASS — independently verified redaction runs before truncation on both call sites (WebApplicationExtensions.cs:428, AgentOperationExecutor.cs:360-361), no leftover raw-truncate path exists anywhere in src/, JsonNode.Parse depth-limit exceptions fail safe (whole-body mask), FK direction/index/orphan-cleanup on SentProactiveCheckins all correct, and contents: read is sufficient for sonarcloud.yml (SonarScanner authenticates via SONAR_TOKEN, not GITHUB_TOKEN).
contract-aligner	N/A — diff touches no DTO, Controller route, or packages/shared contract surface.

Validation

Check	Result
Build (dotnet)	N/A — skipped per CI adaptations; Build runs as a separate required check
Tests (dotnet)	N/A — skipped per CI adaptations; Unit Tests runs as a separate required check. PR body claims 17 passed incl. new AgentAuditRedactorTests and AccountDeletionServiceDbTests.DeleteAllUserDataAsync_RemovesSentProactiveCheckins

Deferred — N/A dimensions & files not verdicted

• Parity / i18n — N/A, frontend-only dimensions, don't fire on an orbit-api diff.
• DESIGN.md/AI-slop — N/A, no apps/* files touched.
• Contract drift/backward-compat — N/A, no DTO/response/request shape or packages/shared field changed; no old-mobile-client break possible from this diff. (Sibling orbit-ui-mobile repo not checked out in this job — not verifiable in CI, but also not applicable here.)
• FEATURES.md parity — N/A, internal security/data-hygiene fix, no user-facing feature/screen/plan-gating surface changed.
• 20260710222149_AddSentProactiveCheckinUserCascade.Designer.cs (2515-line EF model snapshot) — spot-checked for the one changed entity block (SentProactiveCheckin cascade config, matches OrbitDbContextModelSnapshot.cs); not read line-by-line in full since it is machine-generated and mirrors the model builder.
• Prior-review reconciliation: two earlier claude bot reviews on this exact PR state were checked via gh pr view --json reviews (both APPROVE, no Critical/High); the one previously-raised Medium ("auth" fragment gap) was already downgraded to non-blocking in the second prior review and is carried here as Info, consistent with that history — not re-flagged as blocking.

What's good

• Redaction is root-caused, not patched: operates on the full parsed JSON tree at any depth, masks the entire value (not just a leaf) so a secret nested under a sensitive key is fully discarded, and non-JSON bodies are masked whole rather than leaking raw text — closing the original truncation-only gap on both the legacy MCP and structured agent audit paths.
• The cascade migration includes a pre-constraint orphan cleanup (DELETE ... WHERE "UserId" NOT IN (SELECT "Id" FROM "Users")), so it applies cleanly even if the pre-cascade bug already left dangling rows — good defensive migration authoring.
• SentProactiveCheckin FK/fluent config is a verbatim mirror of the existing SentStreakFreezeAlert pattern — consistent, low-risk, easy to verify by comparison.
• Solid, focused test coverage: AgentAuditRedactorTests covers top-level/nested/array masking, non-sensitive passthrough, non-JSON whole-body masking, null/empty passthrough, and truncation cap; the new AccountDeletionServiceDbTests case exercises the cascade-delete path end to end via a real SQLite in-memory DB.
• No narration-comment (ORBIT0001) or redundant-rollback (ORBIT0002) risk in any changed file — all comments in the new/touched files are XML-doc.

Recommendation

Approve and merge as-is. The one Info-level follow-up ("auth" → SensitiveFragments) is optional forward-hardening for a field name that doesn't exist in the codebase today — worth a quick follow-up ticket, not a blocker.