feat(api): OAuth nonce replay-protection + state negative coverage

[thomasluizon]

What

Hardens the OAuth/MCP authorization flow (/oauth/*, consumed by the Claude.ai MCP client) on two fronts.

(a) Optional nonce replay-protection layer

• /oauth/authorize now accepts an optional nonce query param (OAuth/OIDC spec).
• The nonce threads through the login page → /oauth/verify-code + /oauth/google → is bound to the single-use authorization code in OAuthAuthorizationStore (AuthorizationEntry.Nonce).
• /oauth/token echoes the bound nonce in the response only when present, so the client can bind its authorize-time nonce to the token it receives and detect a replayed/substituted token response. This is the OIDC nonce model adapted to this bearer flow (no ID token here).

(b) State negative coverage + hardening

• OAuthControllerTests previously had no negative coverage for state. Added:
  • missing state → invalid_request at /oauth/authorize, /oauth/verify-code, /oauth/google (and the guard short-circuits before verifying the code / calling Google);
  • state is echoed verbatim so a client-side mismatch is detectable;
  • a state carrying query-injection (benign&code=forged-code&x=) is percent-encoded — proving an attacker cannot smuggle a forged code param into the redirect.
• Backend is now source of truth: a missing state is rejected server-side rather than silently accepted, enforcing CSRF protection (defense-in-depth alongside the already-mandatory PKCE/S256).

Incidental hardening (required to add nonce safely)

OAuthLoginPage embedded server values into a <script> JS-string context via HtmlEncode, which does not escape \ and is the wrong encoding for that sink (latent script-string injection). Switched to JsonSerializer.Serialize (HTML-safe default encoder escapes < > & ' "), which fixes the latent injection and carries the new nonce safely.

Tests

• OAuthAuthorizationStoreTests: nonce round-trip (bound + returned verbatim; null when absent).
• OAuthControllerTests: 4 nonce tests (direct echo present/absent + end-to-end through verify-code and google into the token echo) and 5 state negative tests.
• Full suite green: 4684 passed / 0 failed. dotnet build clean.

Notes

• orbit-api-only. The /oauth/* endpoints serve the external Claude.ai MCP client, not orbit-ui-mobile's packages/shared contract, so no consumer/shared-type change applies (parity nudge is a false positive here). The token-response nonce is append-only/optional and backward-compatible.
• openapi.json is the build-generated surface doc, updated in sync with the new nonce param.

Refs thomasluizon/orbit-ui-mobile#243

[claude]

Code Review: PR #339 — feat(api): OAuth nonce replay-protection + state negative coverage

Scope: PR #339 in thomasluizon/orbit-api
Recommendation: APPROVE

Summary

Hardens the /oauth/* authorization-code flow used by the Claude.ai MCP client: rejects a missing/empty state server-side on /oauth/authorize, /oauth/verify-code, and /oauth/google; threads an optional client-supplied nonce from /oauth/authorize through the login page into the token exchange; and fixes a latent script-string-injection bug in OAuthLoginPage by switching from WebUtility.HtmlEncode to JsonSerializer.Serialize for values embedded in the inline <script> block. Six files changed (3 source, 1 generated doc, 2 test files), with solid new unit-test coverage. No Critical or High findings survived review.

Findings

Critical

None.

High

None.

Medium

[MEDIUM] openapi.json silently documents the wrong request body for /oauth/verify-code and /oauth/google
· dimension: Backend hard rules / OpenAPI doc accuracy (Orbit.Api/CLAUDE.md "OpenAPI / Scalar")
· location: orbit-api/src/Orbit.Api/openapi.json:14404-14428 (VerifyCodeRequest schema) and :13273-13305 (GoogleAuthRequest schema); colliding types at orbit-api/src/Orbit.Api/Controllers/OAuthController.cs:156-159,187-190 vs orbit-api/src/Orbit.Api/Controllers/AuthController.cs:19-20
· issue: OAuthController.VerifyCodeRequest/GoogleAuthRequest and AuthController.VerifyCodeRequest/GoogleAuthRequest are distinct C# record types sharing the same simple name. Microsoft.AspNetCore.OpenApi keys components.schemas by short type name, so /api/Auth/verify-code and /oauth/verify-code (same for /oauth/google) both $ref the same schema entry — which only reflects AuthController's shape (email/code/language/referralCode). Confirmed by grepping the whole file for codeChallenge: zero hits, even though CodeChallenge is a property on both OAuthController request records. This pre-dates the PR, but the PR's own new Nonce field (and the rest of State/CodeChallenge/RedirectUri/ClientId) is consequently invisible in the generated docs, contradicting the PR description's claim that "openapi.json is the build-generated surface doc, updated in sync with the new nonce param."
· risk: anyone consulting Scalar/the OpenAPI doc for /oauth/verify-code or /oauth/google — including engineers integrating the Claude.ai MCP OAuth client, this PR's own stated audience — sees a request-body schema with none of the real OAuth fields.
· fix: disambiguate the colliding record names (e.g. OAuthVerifyCodeRequest/OAuthGoogleAuthRequest) or configure a custom schema-ID resolver (fully-qualified type name) in the OpenApi options so each endpoint gets its own component.
· reference: Orbit.Api/CLAUDE.md "OpenAPI / Scalar"; pre-existing, not introduced by this diff, not blocking — safe to land as a tracked follow-up.

Low / Info

• The nonce echo at /oauth/token is never verified server-side against anything (OAuthController.cs:334-335) — it is bound to the code and returned as-is, so the actual replay check happens client-side (the client compares the nonce it sent at /authorize to what comes back), exactly mirroring how OIDC's nonce claim works. Given /oauth/token already requires PKCE S256 (code_verifier bound to code_challenge), the marginal protection nonce adds on top of PKCE is small — worth being precise about that in future docs, but not a defect.
• The new explicit state null/empty checks in Authorize/VerifyCode/GoogleAuth may be redundant with ASP.NET Core's implicit-required validation for non-nullable [FromQuery] string state on an [ApiController] action — but this mirrors the pre-existing pattern already used for client_id/redirect_uri/response_type/code_challenge in the same controller, and it's what lets the new unit tests exercise the check directly (they call the action method, bypassing the MVC pipeline). Consistent with existing style, not a new anti-pattern.

Subagents

Agent	Verdict
security-reviewer	PASS — no injection/authz/data-exposure/crypto issues; confirmed the JsonSerializer.Serialize-into-<script> fix is sound against script-context breakout (default JavaScriptEncoder escapes < > & ' and backslashes); confirmed nonce is not cryptographically enforced server-side (informational only)
contract-aligner	NOT VERIFIABLE — orbit-ui-mobile is not checked out in this review environment; local grep in orbit-api found no evidence /oauth/* is consumed by packages/shared or the mobile/web apps (matches the PR's own claim of MCP-client-only scope)

Validation

Check	Result
Build (dotnet)	N/A — this review session could not check out the PR branch (git fetch / gh pr checkout / gh api are blocked by this sandbox's permission layer) or write local files to apply the diff; left to the repo's required "API Tests" CI workflow
Tests (dotnet)	N/A — same reason

Deferred — N/A dimensions & files not verdicted

• Parity (#9) / i18n (#10): N/A — frontend-only dimensions; no apps/* files touched.
• DESIGN.md / AI-slop (#8): N/A — no apps/* UI files touched (the OAuth login page is server-rendered HTML in orbit-api, outside this dimension's gate).
• FEATURES.md parity (#14): N/A — this diff hardens the transport security of an existing OAuth/MCP integration; it adds no screen, tool, or gating change that would make FEATURES.md stale.
• Contract drift (#11): NOT VERIFIABLE here — sibling repo absent from this checkout (see contract-aligner above).
• Validation (Phase 7): N/A — PR branch not checked out / no local file writes available in this sandboxed run; rely on CI's required Build/Unit Tests checks instead.

All 6 changed files (OAuthController.cs, OAuthAuthorizationStore.cs, OAuthLoginPage.cs, openapi.json, OAuthControllerTests.cs, OAuthAuthorizationStoreTests.cs) received a verdict above; nothing changed was silently skipped.

What's good

• The WebUtility.HtmlEncode → JsonSerializer.Serialize switch is a real, correctly-implemented fix for a genuine latent injection bug (HtmlEncode doesn't escape backslashes, the wrong encoding for a JS-string sink); independently verified sound by the security-reviewer subagent.
• Missing-state rejection closes a real CSRF gap (state was previously accepted even when absent).
• Strong, focused unit-test coverage for every new branch: missing-state on all three endpoints, state echoed verbatim, state-with-query-injection percent-encoding, nonce round-trip through both the code and Google-auth paths, nonce present/absent in the token response. New GeneratePkce/ExtractRedirectUrl/ExtractQueryParam test helpers are a reasonable extraction (used 4+ times across the new tests).
• No narration comments, no dead code, no type-safety escape hatches introduced.

Recommendation

Safe to merge. Non-blocking follow-up: fix the VerifyCodeRequest/GoogleAuthRequest OpenAPI schema-name collision (Medium finding above) so /oauth/verify-code and /oauth/google document their actual request bodies.

[claude]

Code Review: PR #339 — feat(api): OAuth nonce replay-protection + state negative coverage

Scope: PR #339 in thomasluizon/orbit-api (head 0367f4b)
Recommendation: APPROVE

Summary

Re-review at the current head. Since my prior APPROVE (submitted against 1a13c5e), the only new commit is 0367f4b — a merge of main bringing in unrelated Sentry PII-scrubbing fixes (#243/#331/#337). Confirmed via git diff 1a13c5e 0367f4b -- <OAuth files> that the merge touched none of the OAuth files (AuthController.cs, SentryEventScrubber.cs, email/marketing services, and their tests are the only files it moved) — it's a clean no-op for this feature. The OAuth diff itself is unchanged: still hardens the /oauth/* authorization-code flow used by the Claude.ai MCP client — rejects a missing/empty state server-side on /oauth/authorize, /oauth/verify-code, and /oauth/google; threads an optional client-supplied nonce from /oauth/authorize through the login page into the token exchange; and fixes a latent script-string-injection bug in OAuthLoginPage by switching from WebUtility.HtmlEncode to JsonSerializer.Serialize for values embedded in the inline <script> block.

Independently re-verified this run:

• security-reviewer subagent: PASS. JsonSerializer.Serialize with default options uses JavaScriptEncoder.Default, safe against </script> breakout for all 7 interpolated values. state fail-closed ordering is correct in all three handlers (checked before any state-changing action — mediator dispatch, Google token-info call, code creation). Rate limiting, [AllowAnonymous] scope, and logging are all unchanged and clean. One Low/optional note: nonce has no explicit max-length validator, but this matches the pre-existing pattern for codeChallenge/clientId and is bounded by the 5-minute code TTL — not a new risk introduced by this diff, not blocking.
• contract-aligner subagent: MATCH (no drift). orbit-ui-mobile isn't checked out in this CI job so the Zod-schema side is formally NOT_VERIFIABLE, but an internal grep found /oauth/* referenced nowhere outside OAuthController.cs/OAuthLoginPage.cs — consistent with the PR's own claim that this is MCP-client-only surface, not part of the packages/shared REST contract. The state-required tightening has no evident existing caller that omits state (the only caller is the login page's own script, which always forwards the value sourced from the upstream /authorize call).

No Critical or High findings. My own correctness/dead-code/comment-policy/backend-hard-rules pass over the diff found nothing new — strong test coverage (missing-state on all three endpoints, state echoed verbatim, state-with-query-injection percent-encoding, nonce round-trip through both code and Google-auth paths, nonce present/absent in the token response).

Findings

Critical

None.

High

None.

Medium

[MEDIUM] openapi.json documents the wrong request-body schema for /oauth/verify-code and /oauth/google (pre-existing, not introduced by this diff)
· dimension: Backend hard rules / OpenAPI doc accuracy
· location: src/Orbit.Api/openapi.json (VerifyCodeRequest/GoogleAuthRequest schema entries); colliding types at src/Orbit.Api/Controllers/OAuthController.cs:156-159,187-190 vs src/Orbit.Api/Controllers/AuthController.cs
· issue: OAuthController.VerifyCodeRequest/GoogleAuthRequest and AuthController.VerifyCodeRequest/GoogleAuthRequest are distinct record types sharing the same simple name. Microsoft.AspNetCore.OpenApi keys components.schemas by short type name, so both controllers' endpoints $ref the same schema entry, which only reflects AuthController's shape. This PR's new Nonce field (and State/CodeChallenge/RedirectUri/ClientId) stays invisible in the generated docs.
· risk: anyone consulting the OpenAPI doc for /oauth/verify-code or /oauth/google — including engineers integrating the Claude.ai MCP OAuth client — sees a schema with none of the real OAuth fields.
· fix: disambiguate the record names (e.g. OAuthVerifyCodeRequest/OAuthGoogleAuthRequest) or configure a custom schema-ID resolver (fully-qualified type name).
· reference: pre-existing collision, not caused by this PR — safe to land as a tracked follow-up, not blocking.

Low / Info

• nonce is bound to the code and echoed as-is at /oauth/token, never verified server-side — the actual replay check happens client-side, which mirrors how OIDC's nonce claim is meant to work. Given PKCE S256 already binds code_verifier, the marginal protection nonce adds is small but real (informational, not a defect).

Subagents

Agent	Verdict
security-reviewer	PASS — no injection/authz/data-exposure/crypto issues
contract-aligner	MATCH — no drift found; sibling repo not checked out, Zod side formally NOT_VERIFIABLE

What's good

• The WebUtility.HtmlEncode → JsonSerializer.Serialize switch is a real, correctly-implemented fix for a genuine latent script-context injection bug.
• Missing-state rejection closes a real CSRF gap (state was previously accepted even when absent), fail-closed in all three handlers.
• Strong, focused unit-test coverage for every new branch.
• No narration comments, no dead code, no type-safety escape hatches introduced.

Recommendation

Safe to merge. Non-blocking follow-up: fix the VerifyCodeRequest/GoogleAuthRequest OpenAPI schema-name collision (Medium finding above, pre-existing).

[claude]

PR Review — #339 (OAuth nonce replay-protection + state negative coverage)

Recommendation: APPROVE

Summary

This is a re-review focused on the diff since my last approval. I compared the current head (9c29e5b) against the commit from my second review (0367f4b) and found the only changes are two more Merge branch 'main' commits pulling in already-merged, unrelated main commits (NotificationTools.cs, SkipHabitCommandValidator.cs, HandleWebhookCommand.cs, AgentAuditRedactor.cs, PushNotificationService.cs, and their tests).

git diff scoped to this PR's actual files (OAuthController.cs, OAuthAuthorizationStore.cs, OAuthLoginPage.cs, openapi.json, OAuthControllerTests.cs, OAuthAuthorizationStoreTests.cs) between the original feature commit and the current head is empty — zero bytes changed in the reviewed surface.

Findings

Severity	Count
Critical	0
High	0
Medium	1 (carried over, pre-existing, non-blocking)
Low / Info	2 (carried over)

• Medium (pre-existing, non-blocking): openapi.json has a schema-name collision between OAuthController.VerifyCodeRequest/GoogleAuthRequest and AuthController's same-named records, which can cause incorrect OpenAPI doc generation. Unrelated to this PR's diff; flagged as a follow-up in prior reviews.

Notes

• Security review and cross-repo contract check were not re-run against unchanged bytes (already evaluated PASS / MATCH-or-N/A in prior reviews of this exact code).
• Cross-repo dimensions (contract-aligner, mobile-side parity) remain not verifiable in this CI job — orbit-ui-mobile isn't checked out here. Consistent with prior conclusion that /oauth/* is MCP-client-only surface, not part of the mobile/web contract.
• Build/test skipped per this job's CI adaptation — handled by required CI checks (Build / Unit Tests / SonarCloud).

No action needed — safe to merge.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[claude]

Code Review: PR #339 — feat(api): OAuth nonce replay-protection + state negative coverage

Recommendation: APPROVE

Summary

This is the fourth review pass on this PR. Comparing the current head (41ce2af4) against the commit from the prior (third) review (9c29e5b2), scoped to the PR's own files (OAuthController.cs, OAuthAuthorizationStore.cs, OAuthLoginPage.cs, openapi.json, and the two test files), the diff is empty. The only commits added since were two more Merge branch 'main' merges pulling in unrelated main-branch work (#338 User.Handle uniqueness index, rate-limit test additions, GooglePlayPushTokenValidator changes). The full OAuth diff was independently re-read, plus a fresh pass from the security-reviewer subagent against current file contents.

The feature hardens /oauth/* (external Claude.ai MCP client flow only): (a) an optional OIDC-style nonce bound server-side to the single-use authorization code and echoed at token exchange, and (b) mandatory state (CSRF) on all three entry points, rejected before any expensive/external work. It also fixes a real (if latent) script-string-injection bug in OAuthLoginPage.cs by switching from WebUtility.HtmlEncode (wrong encoding for a JS-string sink — doesn't escape \) to JsonSerializer.Serialize embedded as a JS object literal (default encoder escapes < > & ' ", neutralizing </script> breakout).

Findings

Critical

None.

High

None.

Medium

• Pre-existing, non-blocking, carried over from all three prior reviews: src/Orbit.Api/openapi.json has a schema-name collision — OAuthController.VerifyCodeRequest/GoogleAuthRequest share names with AuthController's unrelated same-named records, and the generator resolves both $refs to the AuthController shape (accessToken, googleAccessToken, etc. — confirmed at lines ~13273 and ~14404). This means the new Nonce field never shows up in the OAuth request-body schemas in the generated docs, and the /oauth/token response schema was never modeled with fields at all (pre-existing pattern — many endpoints in this file just have "description": "OK"). Unrelated to this PR's diff; a follow-up would be to disambiguate with [SchemaName]/nested-type naming so OpenAPI resolves the correct OAuth-specific record.

Low / Info

None.

Subagents

Agent	Verdict
security-reviewer	PASS — independently re-verified script-context JSON encoding is XSS-safe, nonce introduces no new DoS vector beyond pre-existing unbounded redirectUri/codeChallenge/clientId fields (all bounded by the global Kestrel request-size limit), and the state-required check short-circuits before any external/expensive work in all three endpoints (Authorize L132-133 before Render, VerifyCode L168-169 before mediator.Send, GoogleAuth L199-200 before the Google API call)
contract-aligner	NOT VERIFIABLE — orbit-ui-mobile not checked out in this job. PR author states /oauth/* serves the external Claude.ai MCP client only, not packages/shared's contract surface; consistent with all prior reviews' conclusion and with no nonce/OAuth DTO fields appearing in typical mobile-consumed endpoints.

Validation

CI at review time: Build SUCCESS, Guard Migrations/Conventions SUCCESS, OpenAPI Breaking-Change Gate SUCCESS, Dependency Review/Scan SUCCESS, GitGuardian SUCCESS, CodeQL NEUTRAL; Unit Tests / Mutation testing / SonarCloud handled as separate required checks (skipped here per this workflow's CI adaptation). Prior review recorded 4684 passed / 0 failed on this exact code.

What's good

• CSRF hardening (state required, checked before any external call) is exactly right and closes a real gap — no prior negative-path test coverage for state existed at all.
• The nonce binding correctly rides on the existing single-use, 5-minute-expiry, PKCE-bound AuthorizationEntry, and is only echoed back when present (backward-compatible, opt-in).
• The WebUtility.HtmlEncode → JsonSerializer.Serialize switch is a genuine security fix (wrong-encoding-for-sink class of bug), not just cosmetic.
• Test coverage is thorough: state-negative-path (including the query-injection percent-encoding proof), nonce round-trip through both verify-code and google paths into the token echo, and the store-level nonce binding — all new behavior is directly exercised.

Recommendation

No action needed — safe to merge. This diff has now been independently reviewed four times with zero Critical/High findings each time; the only carried-over item is a pre-existing, unrelated openapi.json schema-collision noted as a non-blocking follow-up.