fix(api): harden auth/session surface (#243)

[thomasluizon]

Hardens the auth/session surface. All four changes preserve the happy path and none regress the #329 concurrent double-rotation guard (its tests still pass).

What changed

1. Account-wide session revocation

• New IAuthSessionService.RevokeAllSessionsAsync(userId) revokes every active session for a user (RevokedAtUtc == null), for use on password/security events.
• Wired to a new authenticated POST /api/auth/logout-all (LogoutAllSessionsCommand), returning { message: "Logged out of all sessions" }.
• Single-session logout is untouched.

2. Explicit pre-rotation validation (complements #329)

• UserSession.Rotate(...) now returns Result and guards, before mutating, that the new hash is non-empty and the session is still usable (CanUse). A rotation can no longer land on a revoked/expired session.
• This is an explicit in-memory invariant that layers on top of fix(api): stop concurrent refresh-token double-rotation + close three test gaps (#243) #329's xmin optimistic-concurrency token (which rejects the losing writer at DB commit). RefreshSessionAsync maps a rotate-guard failure to INVALID_SESSION.

3. Refresh-token format/length validation

• RefreshSessionCommandValidator (and LogoutSessionCommandValidator, which accepts the same token) now enforce the exact server-issued shape — 128 uppercase-hex characters — via a shared RefreshTokenRules. Malformed tokens are rejected with a 400 before any DB lookup.

4. Refresh rate limit partitions by session, not IP

• The refresh endpoints (Refresh, RefreshOperation) move from the shared auth policy to a dedicated refresh policy partitioned by a SHA-256 hash of the refresh token — a stable per-session identity — instead of the caller IP, mirroring the existing email-partition mechanism. A stolen/targeted refresh token can no longer be replayed faster by rotating source IPs (the cross-IP gap IP partitioning leaves open). Falls back to IP when no token is present; the key is hashed so logs never carry the raw secret.

Tests (behavior + edge + failure)

• UserSessionTests: Rotate succeeds on active/non-expiring sessions; fails (SESSION_NOT_ACTIVE) on revoked/expired without mutating; fails (TOKEN_HASH_REQUIRED) on blank hash.
• AuthSessionServiceRevokeAllTests: in-memory DB proves only the target user's active sessions are revoked (other users and already-revoked rows untouched); no-sessions succeeds; empty userId fails.
• LogoutAllSessionsCommand{Handler,Validator}Tests.
• Refresh/logout validator tests: server-issued token passes; wrong length, non-hex, lowercase, empty/whitespace fail.
• DistributedRateLimitPartitionKeyTests + DistributedRateLimitFilterTests: refresh requests partition by hashed token (stable across requests, distinct per token, never leaks the secret), fall back when blank/absent, and don't apply to unrelated policies.

Notes

• New endpoint is mapped in the agent catalog (AuthManage) and openapi.json regenerated (additive only).
• orbit-api-only: the new endpoint is additive/append-only, so no consumer (orbit-ui-mobile) change is required in lockstep.

Refs thomasluizon/orbit-ui-mobile#243

[claude]

Automated Review — PR #334: fix(api): harden auth/session surface (#243)

Recommendation: Request changes

Severity	Count
Critical	0
High	1
Medium	1
Low / Info	0

High — rate-limit bypass on /api/auth/refresh and /api/auth/operations/refresh

src/Orbit.Api/RateLimiting/DistributedRateLimitAttribute.cs:167-197 (TryResolveRefreshTokenPartitionKey).

This PR switches both refresh endpoints from the "auth" policy (IP-partitioned, shared 10-req/min bucket per IP) to a new "refresh" policy partitioned by SHA256(RefreshToken) (src/Orbit.Api/Controllers/AuthController.cs:207,228). The only gate before hashing the client-supplied token into the partition key is !string.IsNullOrWhiteSpace(rawToken) — the actual format check (RefreshTokenRules, enforced via RefreshSessionCommandValidator) only runs later, inside mediator.Send(...), after the rate-limit filter has already made its allow/deny decision.

An attacker varying the RefreshToken body field per request (trivial, zero cost — e.g. appending a counter) gets a fresh, never-throttled partition key on every call, from a single IP, with no need to rotate IPs. Each bypassed call still drives a full DistributedRateLimitService.TryAcquireAsync round-trip — on the relational path this opens a Serializable-isolation DB transaction, deletes expired buckets, and inserts/updates a row (src/Orbit.Infrastructure/Services/DistributedRateLimitService.cs:72-171) — before the request is even validated or rejected. This turns the mitigation itself into a resource-exhaustion vector, and defeats the stated purpose of the change (closing the cross-IP brute-force gap on refresh tokens) since a per-request-random token also evades the "per-session" partitioning goal.

Fix: validate token shape (or at minimum length/charset) in TryResolveRefreshTokenPartitionKey before using it as a partition key, falling back to IP partitioning for malformed input — mirroring how TryResolveEmailPartitionKey only needs a plausible string, but refresh tokens have a fixed, checkable format (RefreshTokenRules) that's cheap to check inline.

Medium — RevokeAllSessionsAsync missing concurrency-exception handling

src/Orbit.Infrastructure/Services/AuthSessionService.cs:104-123.

UserSession carries an optimistic-concurrency token, and the sibling RefreshSessionAsync (lines 73-81) explicitly catches DbUpdateConcurrencyException and returns a controlled Result.Failure. The new RevokeAllSessionsAsync (backing the new POST /api/auth/logout-all endpoint) batches SaveChangesAsync over all of a user's active sessions with no such catch — a concurrent write to any one session (e.g. another device mid-refresh at the same moment) throws unhandled, propagating as a 500 via UnhandledExceptionHandler instead of a graceful Result failure. All-or-nothing semantics mean no partial/silent revocation occurs, which keeps this at Medium rather than High, but it's a real gap in exactly the security-incident scenario ("log out everywhere because I think I'm compromised") this endpoint exists for.

Fix: wrap the SaveChangesAsync in RevokeAllSessionsAsync in the same try/catch (DbUpdateConcurrencyException) pattern used in RefreshSessionAsync.

What's good

• UserSession.Rotate() now returning Result and guarding CanUse()/non-blank hash before mutating closes a real prior gap.
• RevokeAllSessionsAsync correctly scopes to the target user's own active sessions only.
• RefreshTokenRules centralizes format validation cleanly.
• logout-all derives the user id only from the JWT (no enumeration surface) and has solid test coverage across the new/changed surfaces.

Not verifiable in CI

• Cross-repo parity/contract checks (orbit-ui-mobile) — sibling repo not checked out in this job. Local inspection shows the change is additive-only (new logout-all endpoint, no field renamed/removed), so no consumer break is expected, but this could not be confirmed against the actual TypeScript client.
• dotnet build/test — skipped per instruction; covered by separate required CI checks (Build / Unit Tests / SonarCloud).

[thomasluizon]

Both review findings are addressed at source in 5658611 and the flagged thread is resolved:

• High (rate-limit bypass): TryResolveRefreshTokenPartitionKey now accepts a token only if it matches the exact server-issued shape (RefreshTokenRules.IsWellFormed — the single source of format truth); malformed/varied tokens fall back to IP partitioning, so the trivial vary-the-body bypass is closed and unvalidated input never reaches the rate-limit DB round-trip.
• Medium (revoke-all concurrency): RevokeAllSessionsAsync now catches DbUpdateConcurrencyException (DiscardChanges + INVALID_SESSION), mirroring RefreshSessionAsync, so a concurrent refresh during logout-all fails gracefully instead of 500.

New failure tests cover both. All required checks are green (Build, Unit Tests, Mutation ×3, CodeQL ×3, SonarCloud Analysis + Code Analysis, Dependency Scan, OpenAPI gate). The automated re-review completed and posted an empty COMMENTED verdict (no new findings) without flipping to APPROVE, so reviewDecision still shows the stale CHANGES_REQUESTED — no outstanding actionable request remains.

[claude]

Code Review: PR #334 — fix(api): harden auth/session surface (#243)

Scope: PR #334 in thomasluizon/orbit-api (fix/auth-session-hardening -> main)
Recommendation: NEEDS WORK

Summary

This PR adds account-wide session revocation (POST /api/auth/logout-all), tightens the
UserSession.Rotate invariant (guards against rotating a revoked/expired session), enforces
exact refresh-token shape at the validator layer, and moves the refresh endpoints to a
token-partitioned rate-limit policy instead of IP partitioning. Test coverage for the new
surface is thorough (handler, validator, domain, and infrastructure/service tests, including
a concurrency-conflict test using a SaveChangesInterceptor).

This is a re-review of an already-reviewed PR. A prior automated review round flagged the same
rate-limit-bypass issue below as High, and the PR author's follow-up commit (5658611,
"address review — inline refresh-token format gate + revoke-all concurrency") narrowed it but
did not close it — see the finding for why. The concurrency-handling half of that same fix
commit (RevokeAllSessionsAsync now catching DbUpdateConcurrencyException) is verified
correct and that half of the prior Medium finding is resolved.

Findings

Critical

None.

High

[HIGH] refresh rate-limit is still bypassable with zero extra infrastructure — the fix only raised the bar, it didn't close the gap
· dimension: 12. Security (rate-limit coverage / defense-in-depth)
· location: src/Orbit.Api/RateLimiting/DistributedRateLimitAttribute.cs:171-201 (TryResolveRefreshTokenPartitionKey), consumed by AuthController.Refresh / RefreshOperation (src/Orbit.Api/Controllers/AuthController.cs:207,228) via [DistributedRateLimit("refresh")]
· issue: For unauthenticated requests under the refresh policy, the rate-limit partition key is refresh:token:{SHA256(token)} whenever the client-supplied RefreshToken value passes RefreshTokenRules.IsWellFormed — a format-only check (exactly 128 uppercase-hex characters, src/Orbit.Application/Auth/Validators/RefreshTokenRules.cs:8-10), with no existence check against real sessions. This is exactly the mechanism a prior review round on this PR flagged as High (the gate back then was only !string.IsNullOrWhiteSpace(rawToken) — see commit 8579504); the author's fix (commit 5658611) replaced the blank-check with the format check, and the PR-author response comment states "the trivial vary-the-body bypass is closed." It is narrowed, not closed: generating a fresh, well-formed 128-char uppercase-hex string per request is exactly as cheap as the blank-string bypass was (Convert.ToHexString(RandomNumberGenerator.GetBytes(64)) — the same one-liner the server itself uses to mint real tokens). The PR's own new test TryResolveRefreshTokenPartitionKey_Refresh_DifferentTokensMapToDifferentKeys (tests/Orbit.Infrastructure.Tests/RateLimiting/DistributedRateLimitPartitionKeyTests.cs) documents this: two arbitrary, non-existent, well-formed tokens ('A'x128, 'B'x128) map to two different partitions, by design.
· risk: Before this PR, refresh/operations/refresh shared the auth policy and partitioned purely by IP (10 req/min per IP) — an attacker needed distributed source IPs (real infrastructure cost) to exceed that cap. After this PR, a single-IP attacker with no proxy/botnet needs only to vary the last hex digits of the token field per request to mint an unthrottled partition every time, fully defeating the 10-req/min cap on this endpoint from one machine. Each such request still pays the full DistributedRateLimitService.TryAcquireCoreAsync cost — on the relational path this opens a Serializable-isolation DB transaction, runs an expired-bucket delete, and inserts a row (src/Orbit.Infrastructure/Services/DistributedRateLimitService.cs:72-171) — before the request is even validated by RefreshSessionCommandValidator (which runs later, inside mediator.Send, only if the filter's next() is reached — rate-limit resolution always runs first regardless of pass/fail). Actual credential compromise via guessing a valid 512-bit token remains cryptographically infeasible either way, so the practical impact is DB/resource-exhaustion (DoS) against the auth surface, not account takeover — which is why this stays High rather than Critical.
· fix: Partitioning by a hash of attacker-controlled input can never be sufficient on its own — the key needs to be tied to something the attacker can't mint for free. Options: (a) do a cheap existence check (e.g. hash lookup against UserSessions.TokenHash, which is already indexed for RefreshSessionAsync/RevokeSessionAsync) before granting a token-partition, falling back to IP partitioning when the token doesn't resolve to a real session; or (b) keep the token partition but layer it under a coarser per-IP ceiling as well (both partitions must pass), so varying the token alone can no longer produce unlimited throughput from one IP. Update DistributedRateLimitPartitionKeyTests/DistributedRateLimitFilterTests to cover "well-formed but non-existent token still throttles by IP" once fixed.
· reference: orbit-api hard rule "Rate limiting: apply [DistributedRateLimit] to abuse-prone endpoints" (Orbit.Api/CLAUDE.md); rubric dimension 12 (Security — Authentication & authorization / rate-limit coverage); OWASP API4:2023 (Unrestricted Resource Consumption)

Medium

None. (The concurrency-handling gap flagged in the prior review round — RevokeAllSessionsAsync not catching DbUpdateConcurrencyException — is fixed in src/Orbit.Infrastructure/Services/AuthSessionService.cs:120-128, mirroring RefreshSessionAsync's existing pattern, and is covered by AuthSessionServiceRevokeAllTests.RevokeAllSessionsAsync_ConcurrencyConflict_ReturnsInvalidSessionWithoutThrowing.)

Low / Info

[INFO] RefreshTokenRules format tightening is backward-compatible
· dimension: 11. Contract drift + backward-compat guard
· location: src/Orbit.Application/Auth/Validators/RefreshSessionCommandValidator.cs, LogoutSessionCommandValidator.cs
· note: The new 128-uppercase-hex format gate rejects malformed tokens with 400 before DB lookup. Since AuthSessionService.GenerateRefreshToken() (unchanged by this PR) has always produced exactly this shape (Convert.ToHexString(RandomNumberGenerator.GetBytes(64))), every already-issued, already-shipped mobile-client token still satisfies the new check — not a compat break, no client-side change needed.

Subagents

Agent	Verdict
security-reviewer	Independent review launched in parallel; findings fold into the High finding above (root cause independently confirmed via direct git-history comparison against the prior review round's flagged commit).
contract-aligner	NOT VERIFIABLE — orbit-ui-mobile is not checked out in this job. PR body states the change is additive-only (new logout-all endpoint, no field renamed/removed/newly-required), consistent with the diff (packages/shared not touched, not in this repo).

Validation

Check	Result
Build (dotnet)	N/A — skipped in CI; Build / Unit Tests / SonarCloud run as separate required checks
Tests (dotnet)	N/A — skipped in CI; Build / Unit Tests / SonarCloud run as separate required checks

Deferred — N/A dimensions & files not verdicted

• Dimension 8 (DESIGN.md/AI-slop): N/A — no apps/* UI files in this diff.
• Dimension 9 (Parity): N/A — frontend-only dimension, not verifiable from orbit-api; PR is orbit-api-only per its own body.
• Dimension 10 (i18n): N/A — no new user-facing locale strings; the new "Logged out of all sessions" message mirrors the existing hardcoded-English pattern already used by every other Auth endpoint response in this file.
• Dimension 11 (Contract drift): contract-aligner leg not verifiable — see Subagents table.
• Dimension 14 (FEATURES.md parity): N/A — no user-facing feature surface ships in this PR (backend-only, additive; PR body states no mobile-client wiring happens in this PR).
• All 22 changed files were read in full diff context and given a verdict; nothing was skipped for size.

What's good

• UserSession.Rotate() returning Result and guarding CanUse()/non-blank hash before mutating closes a real pre-existing gap, and is well tested (UserSessionTests, all 4 cases including the non-mutation assertion on failure).
• RevokeAllSessionsAsync correctly scopes to the target user's own active sessions only, is concurrency-safe, and LogoutAllSessionsCommandHandlerTests cover both success and failure propagation.
• logout-all derives the user id only from the JWT claim (no enumeration surface), sits behind [Authorize], and is rate-limited.
• Test coverage across the new/changed surface is genuinely thorough — including the exact test that documents the rate-limit finding's mechanism (DifferentTokensMapToDifferentKeys), which made this review faster, not slower.
• openapi.json regeneration is additive-only, matching the PR's stated intent.

Recommendation

Close the refresh-token rate-limit gap for real before merging: tie the per-token partition to
an actual session lookup (or add a per-IP ceiling alongside the per-token one) so a
well-formed-but-nonexistent token can no longer mint unlimited unthrottled partitions from a
single IP. Everything else in this PR is solid and well-tested; once the rate-limit fix lands,
this is a straightforward approve.

[claude]

Code Review: PR #334 — fix(api): harden auth/session surface (#243)

Scope: PR #334 in thomasluizon/orbit-api (branch fix/auth-session-hardening → main)
Recommendation: NEEDS WORK

Summary

This PR adds account-wide session revocation (POST /api/auth/logout-all), an explicit pre-rotation invariant on UserSession.Rotate, strict refresh-token format validation, and a per-session (rather than per-IP) rate-limit partition for the refresh endpoints. The individual pieces are well-tested and the refresh-token partitioning logic itself is sound (SHA-256'd, format-gated, falls back safely to IP). One finding blocks merge: the new bulk session-revocation path fails all-or-nothing under a single concurrency conflict, which can silently defeat the exact security workflow this PR is meant to enable.

Findings

Critical

None.

High

[HIGH] RevokeAllSessionsAsync fails to revoke ANY session when one session in the batch has a concurrent write
· dimension: Correctness (#1) / Security — authentication & session handling (#12)
· location: src/Orbit.Infrastructure/Services/AuthSessionService.cs:104-131
· issue: RevokeAllSessionsAsync loads every active UserSession for the user via FindTrackedAsync (tracked, so each row's original xmin is captured), calls session.Revoke(nowUtc) on all of them in a foreach, then issues exactly one unitOfWork.SaveChangesAsync() for the whole batch. UserSession.xmin is mapped as an EF Core concurrency token (OrbitDbContext.cs → ConfigureConcurrencyTokens/MapXminToken), and UnitOfWork.SaveChangesAsync is a bare context.SaveChangesAsync() with no per-entity loop and no execution-strategy/transaction isolation between entities (EnableRetryOnFailure only retries transient errors, not DbUpdateConcurrencyException). EF Core wraps all pending inserts/updates from one SaveChanges call in a single implicit transaction, so if any one of the user's other sessions was concurrently modified between the read and the save (e.g. another device's RefreshSessionAsync rotating its own TokenHash at the same moment — an entirely ordinary, non-malicious event), the whole transaction rolls back on DbUpdateConcurrencyException, unitOfWork.DiscardChanges() reverts every Modified entry back to Unchanged, and the method returns Result.Failure(ErrorMessages.InvalidSession) — none of the user's sessions get revoked, not even the ones with no conflict.
· risk: This is the account-wide "logout everywhere" control this PR's own body says is "for use on password/security events." Because the failure is all-or-nothing rather than per-row, (a) an ordinary user with 2+ active devices can have a legitimate logout-all attempt silently no-op whenever another of their own devices happens to auto-refresh at the same instant, and (b) an attacker holding one compromised/stolen refresh token can indefinitely keep re-triggering a conflict by looping refresh calls against that single session (bounded only by the new refresh rate limit, ~10/min), repeatedly defeating the victim's attempt to revoke it via logout-all. The failure does surface as an error to the caller (not a false "success"), so it is not a silent security bypass — but it is a real, reachable reliability/security gap in the marquee feature of this PR.
· fix: Make the bulk revoke resilient to a single row's conflict — e.g. ExecuteUpdateAsync per row (bypasses in-memory concurrency tracking and issues one UPDATE per session, so an unrelated conflict doesn't roll back the others), or catch DbUpdateConcurrencyException per-session with an individual SaveChangesAsync/retry-with-reload loop so a stale row is skipped/retried without discarding the rest of the batch. Report partial success (e.g. how many of N sessions were revoked) rather than a single boolean.
· reference: orbit-api hard rule "No workarounds — root-cause every bug" + Security dimension #12 (authentication & session handling); confirmed independently by a security-reviewer subagent pass and by an adversarial skeptic pass (both concluded the EF Core batching/xmin mechanics make this reachable, not theoretical).

Medium

[MEDIUM] HasSessionForTokenAsync doesn't exclude revoked/expired sessions
· dimension: Security — defense in depth (#12)
· location: src/Orbit.Infrastructure/Services/AuthSessionService.cs:133-136
· issue: The rate-limiter's "does this refresh token map to a real session?" check is userSessionRepository.AnyAsync(session => session.TokenHash == tokenHash) — it matches on TokenHash alone, with no RevokedAtUtc == null / not-expired condition.
· risk: Low on its own (it only affects rate-limit partitioning, not authentication — RefreshSessionAsync still independently checks CanUse before honoring the token), but it means a revoked or expired token that once mapped to a real session keeps earning a stable, private per-token rate-limit bucket forever instead of falling back to per-IP once it's no longer a legitimate credential.
· fix: Add && session.RevokedAtUtc == null (and optionally an expiry check) to the AnyAsync predicate so only currently-usable sessions earn the per-token partition.
· reference: Security dimension #12, defense-in-depth.

Low / Info

None posted (signal gate — no concretely-actionable Low/Info items beyond what's captured above).

Subagents

Agent	Verdict
security-reviewer	FAIL (1 High: RevokeAllSessionsAsync all-or-nothing batch failure; 1 Low/Info: HasSessionForTokenAsync revoked-session check; all other categories PASS — controller [Authorize] placement, refresh-token partition-key logic, Rotate Result handling at its one production callsite all confirmed correct)
contract-aligner	NOT VERIFIABLE — sibling orbit-ui-mobile repo is not checked out in this CI job. The diff is additive-only on the API side (new POST /api/auth/logout-all, openapi.json regenerated); the PR body states no consumer change is required in lockstep, and no existing field/DTO was renamed or removed, so this reads as Info/forward-compatible per dimension #11 — but the shared-types side could not be independently checked.

Validation

Check	Result
Build (dotnet)	N/A — skipped per CI adaptation; Build/Unit Tests/SonarCloud run as separate required checks on this PR
Tests (dotnet)	N/A — same as above

Deferred — N/A dimensions & files not verdicted

• Dimension #8 (DESIGN.md/AI-slop) — N/A, no apps/* UI files in this diff.
• Dimension #9 (Parity) / #10 (i18n) — N/A, frontend-only dimensions; this diff touches only orbit-api.
• Dimension #11 (Contract drift) — field-by-field comparison not verifiable in CI (sibling repo not checked out); classified from the diff alone as additive-only (see Subagents table).
• Dimension #14 (FEATURES.md parity) — this PR ships a new authenticated capability (logout-all) but no new screen/tool/gating change ships in this diff; the consumer-facing surfacing is tracked separately via the linked issue (thomasluizon/orbit-ui-mobile#243). Not flagged as a blocking finding here since there is no user-visible feature change in this diff to point FEATURES.md at yet — worth a reminder in the linked mobile PR when the UI for "logout everywhere" ships.
• All 24 changed files (11 source, 13 test) were read and given an explicit verdict; none were skipped for size.

What's good

• The refresh-token partition-key design (TryExtractRefreshToken → RefreshTokenRules.IsWellFormed → HasSessionForTokenAsync → SHA-256 hash) is genuinely well thought through: it closes the "vary the body to mint a fresh bucket" bypass without creating a cheaper-than-a-real-token way to mint private buckets, and the partition key that gets logged is a one-way hash, never the raw secret.
• UserSession.Rotate returning Result and guarding CanUse/non-empty hash before mutating is a clean, minimal invariant that composes correctly with the existing xmin-based concurrent-refresh guard (#329) without duplicating it.
• Strong test coverage across the diff: domain guard tests (UserSessionTests), handler/validator tests (LogoutAllSessionsCommand*Tests), service tests including an explicit concurrency-conflict simulation (AuthSessionServiceRevokeAllTests), and rate-limit filter/partition-key tests covering both the happy path and the forged-token fallback.
• openapi.json was regenerated and is additive-only.

Recommendation

Fix the all-or-nothing failure mode in RevokeAllSessionsAsync (per-row concurrency handling instead of one batched SaveChanges) before merging — this is the one finding standing between this PR and approval. The HasSessionForTokenAsync revoked-session check is a good-to-have hardening, not a blocker. Everything else in the diff is solid.

[claude]

PR Review: fix(api) harden auth/session surface (issue 243)

Recommendation: APPROVE

Severity counts - Critical: 0, High: 0, Medium: 1, Low/Info: 2.

This PR has been through prior automated review rounds on this branch; both previously-flagged issues (rate-limit bypass via body variance, revoke-all concurrency handling) are confirmed fixed in the current diff. This review covers the latest commit (89d2dab) for regressions and re-verifies end-to-end.

Findings:

MEDIUM - Refresh-policy DB lookup runs ahead of the rate-limit gate.
Location: src/Orbit.Api/RateLimiting/DistributedRateLimitAttribute.cs lines 36-40 (ResolvePartitionKeyAsync, called before the try block at line 43), calling into AuthSessionService.HasSessionForTokenAsync (src/Orbit.Infrastructure/Services/AuthSessionService.cs lines 133-137).
For the refresh policy, ResolvePartitionKeyAsync now does its own indexed DB existence check (TokenHash unique index, OrbitDbContext.cs line 333) to decide the partition key, and this call sits outside the filter own try/catch block. Two consequences: (1) If the lookup throws (transient DB blip), it bypasses LogRateLimitEvaluationFailed and the fail-open/fail-closed resilience branch this filter is otherwise built around; it still surfaces as a 500 captured by Sentry via UnhandledExceptionHandler, but loses the rate limiter structured log line and its one resilience decision point. (2) More importantly, the DB round trip happens before the rate-limit counter is consulted at all: any unauthenticated POST /api/auth/refresh (and /operations/refresh) request whose body is 128 uppercase-hex chars (trivially scriptable, no real token needed) triggers one indexed query, unthrottled by the very policy meant to bound this endpoint. The index keeps each query cheap, so this is not a query-plan risk, but it is unbounded query volume ahead of the rate-limit gate - a low-cost DB-amplification vector on an anonymous endpoint.
Fix: move ResolvePartitionKeyAsync inside the existing try block (or apply a cheap floor limit, e.g. IP-based, before doing the session lookup), so both DB dependencies in this filter share one resilience/throttle path.

Low / Info (non-blocking):

• RefreshTokenRules.IsWellFormed (src/Orbit.Application/Auth/Validators/RefreshTokenRules.cs lines 9-11) is not constant-time, but it only gates format (128 hex chars) and never compares against a secret, so there is no practical timing side channel.
• HasSessionForTokenAsync matches on TokenHash only, not CanUse(), so a revoked/expired token still earns its own rate-limit partition bucket instead of falling back to IP. Rate-limit precision only; RefreshSessionAsync/RevokeSessionAsync independently re-check CanUse() before anything security-sensitive.
• Backward-compat verified safe: RefreshTokenRules enforces exactly 128 uppercase-hex chars; traced GenerateRefreshToken() back to its original commit (f4f8ea7), always Convert.ToHexString(RandomNumberGenerator.GetBytes(64)), so no shipped mobile client holds a token shape this would newly reject.
• DistributedRateLimitPartitionKeyTests.cs line 132 InlineData labeled right-length is actually 122 chars, not 128, so it only exercises the wrong-length branch, not the right-length-wrong-characters branch its name implies. That branch is still covered separately via the lowercase case in the same test, so no real coverage gap, just a misleading fixture name.

What is good:

• UserSession.Rotate() now returns Result and re-checks CanUse()/non-blank hash before mutating, closing a real TOCTOU gap, with tests proving no mutation on failure.
• RevokeAllSessionsAsync scopes correctly to the target user active sessions, cleanly handles DbUpdateConcurrencyException (mirrors RefreshSessionAsync), and is thoroughly tested including the concurrency-conflict path.
• The refresh rate-limit fix (gating the token partition on a real, indexed session lookup, falling back to IP for forged tokens) correctly closes the previously-flagged vary-the-body-for-a-fresh-unthrottled-bucket bypass.
• logout-all derives the user id only from the JWT claim (HttpContext.GetUserId()), never the request body, so there is no enumeration surface, and it sits behind the Authorize attribute plus rate limiting.
• No raw tokens or token hashes appear in logs; partition keys and LogAllSessionsRevoked log only hashes/UserId, consistent with the codebase logging rules.
• Thorough test coverage across every new/changed surface (handler, validator, domain, infrastructure).

Not verifiable in this environment:

• Cross-repo contract check (packages/shared/src/api/endpoints.ts, Zod schemas in orbit-ui-mobile): sibling repo is not checked out here. API-side inspection shows the change is additive-only, a new POST /api/auth/logout-all (no request body, a message string response) and an internal rate-limit-policy rename (auth to refresh) with no wire-shape change to any existing DTO, so no backward-incompatible surface is introduced from the API side. Confirming the endpoints.ts/Zod side needs a session with orbit-ui-mobile checked out.

Recommendation: Approve. The single Medium finding (share the rate-limit resilience/throttle path between both DB calls in DistributedRateLimitAttribute) is a worthwhile follow-up but does not change client-visible behavior or security posture on its own.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
89.7% Coverage on New Code
0.1% Duplication on New Code

See analysis details on SonarQube Cloud

[claude]

Code Review: PR #334 — fix(api): harden auth/session surface (#243)

Scope: PR #334 in thomasluizon/orbit-api (fix/auth-session-hardening → main)
Recommendation: APPROVE

Summary

Adds account-wide session revocation (POST /api/auth/logout-all), an in-memory pre-rotation guard on UserSession.Rotate layered on top of #329's xmin optimistic-concurrency token, strict 128-char-uppercase-hex refresh-token format validation, and a new refresh rate-limit policy partitioning unauthenticated refresh requests by SHA-256(refresh token) instead of IP. Session-invalidation logic (retry loop, idempotent revoke, xmin interaction) was verified correct; one concretely-actionable Medium finding survives review, no Critical/High.

Findings

Critical

None.

High

None.

Medium

[MEDIUM] New DB-dependent rate-limit partition-key resolution isn't covered by the filter's existing fail-open/fail-closed handling
· dimension: Security (#12) / Backend hard rules (#13, error handling)
· location: orbit-api/src/Orbit.Api/RateLimiting/DistributedRateLimitAttribute.cs:36-40 (call site) and :106-125 (ResolvePartitionKeyAsync)
· issue: OnActionExecutionAsync calls `await ResolvePartitionKeyAsync(...)` on lines 36-40, outside the try/catch wrapping `distributedRateLimitService.TryAcquireAsync` (lines 43-68). For the new `refresh` policy, ResolvePartitionKeyAsync now does a real DB round-trip via `authSessionService.HasSessionForTokenAsync` (line 118-119) for any unauthenticated request whose body contains a well-formed 128-char hex string — including a forged one, since RefreshTokenRules.IsWellFormed only checks shape, not authenticity.
· risk: A session-store hiccup on /api/auth/refresh or /api/auth/operations/refresh produces a generic unhandled-exception 500 instead of the filter's structured `LogRateLimitEvaluationFailed` diagnostic, making refresh-specific rate-limit health harder to distinguish from an ordinary handler failure. A flood of forged-but-well-formed tokens also forces one indexed DB lookup per request before the token bucket is consulted — cheap per call, but DB load now scales with raw request volume rather than throttled volume.
· fix: Move the HasSessionForTokenAsync call inside the existing try/catch (or wrap ResolvePartitionKeyAsync in its own try/catch reusing LogRateLimitEvaluationFailed and the same fail-open-only-for-"support" decision) so every dependency the filter touches shares one consistent failure/observability path.
· reference: orbit-api hard rule "Error handling"; the filter's own existing pattern (lines 50-68) that this new code path doesn't follow

Low / Info

None posted (signal gate).

Subagents

Agent	Verdict
security-reviewer	FAIL (1 Medium) — independently confirmed the finding above; explicitly cleared authorization/IDOR, timing/info-leakage, revocation atomicity/idempotency, the #329 xmin guard, and no-raw-token-in-logs
contract-aligner	NOT VERIFIABLE — sibling orbit-ui-mobile not checked out in this run. One new additive endpoint (POST /api/auth/logout-all); no field renamed/removed on any existing DTO, so no already-shipped client is affected. PR cross-references orbit-ui-mobile#243 for consumer wiring.

Validation

Check	Result
Build (dotnet)	N/A — CI runs Build/Tests as separate required checks
Tests (dotnet)	N/A — same

Deferred

• Dimension 8 (DESIGN.md/AI-slop) — N/A, no apps/* files touched.
• Dimension 9 (Parity) — N/A, frontend-only, no ui-mobile diff available.
• Dimension 10 (i18n) — N/A, no frontend strings changed.
• Dimension 11 (Contract drift) — not verifiable in CI (sibling repo absent); judged from diff alone (see contract-aligner row).
• Dimension 14 (FEATURES.md) — N/A, no listed trigger (no new screen/route/tab; AuthController.LogoutAll added only to the AuthManage capability's controllerActions catalog for audit attribution, no mcpTools binding; no plan-gating/platform/locale change).
• All 24 changed files (10 source + 14 test) read and verdicted; nothing skipped for size/time.

What's good

• RevokeAllSessionsAsync's retry loop re-queries fresh via ChangeTracker.Clear() each attempt — no double-revoke, no partial write, exhausted retries return real failure not false success.
• UserSession.Revoke is idempotent (RevokedAtUtc ??= revokedAtUtc).
• Rotate's new pre-check doesn't interfere with the #329 xmin guard — confirmed via the untouched AuthSessionServiceConcurrentRefreshTests still passing (exactly one row mutation on the losing writer).
• Refresh-token partition key is always the SHA-256 hash, never raw, on every logged path.
• RefreshTokenRules's 128-hex-uppercase shape exactly matches GenerateRefreshToken()'s output — no backward-compat break for already-issued tokens.
• Thorough test coverage: transient vs. persistent concurrency conflicts (custom SaveChangesInterceptors), cross-user isolation, already-revoked no-ops, malformed/lowercase/wrong-length token rejection, IP-fallback for forged tokens.

Recommendation

Approve as-is. The one Medium finding is a defense-in-depth/observability gap, not a blocking defect — fine to land now with a tracked follow-up, or fix with a one-line try/catch move before merge if preferred.