Fix stale-owner API key: charge pre-auth IP budget before per-key quota (#1404)

[Chris0Jeky]

Summary

Closes #1404.

Fixes the stale-owner API-key defect surfaced by Codex P2 on PR #1401. Account deletion/deactivation only sets User.IsActive = false; the API-key row stays active. After #1401 the per-key pre-auth budget (60/min) was charged BEFORE the owner-active check, so a stale-owner key (active key row, inactive/deleted owner) fell into a loop: the first ~60 requests/window 401'd and charged the per-IP failure budget, the rest 429'd via the per-key check (which correctly does NOT charge the IP budget). Net effect — the 120-permit IP failure budget never exhausted, the pre-auth pre-check never tripped, and the SHA-256 + ApiKeys lookup ran on every request indefinitely.

The fix

In ApiKeyMiddleware, the owner's IsActive is now resolved in the INITIAL ApiKeys key lookup via a correlated projection on the UserId FK (there is no ApiKey→User navigation). A stale-owner key is now rejected with 401 — charging the pre-auth IP failure budget via WriteErrorResponse — BEFORE the per-key quota charge. Consequences:

• Stale-owner keys once again reach eventual full pre-DB rejection: sustained traffic exhausts the shared IP failure budget and trips the pre-auth IP pre-check before any DB work (restores pre-Enforce MCP per-key budget before auth-stage DB work (one budget, checked once) #1401 behaviour for this case).
• The standalone Users SELECT is removed from the auth path entirely — one folded query instead of two (happy-path perf win).
• The Enforce MCP per-key budget before auth-stage DB work (one budget, checked once) #1401 ordering is unchanged for genuinely valid keys: the per-key budget remains the first gate they meet, and a valid-but-over-quota key still 429s without spending the IP failure budget.

Key/owner active-state is still computed in memory from projected scalar columns (RevokedAt/ExpiresAt/owner flag), so expiry is evaluated against the same wall clock as before — nothing is pushed into SQL. OwnerIsActive is null when the owner row is absent (hard-deleted user), so that case fails the == true gate too.

Out of scope (tracked alternative, deliberately NOT implemented): key-revocation-on-deactivation.

Files changed

• backend/src/Taskdeck.Api/Middleware/ApiKeyMiddleware.cs — fold owner-active resolution into the initial ApiKeys lookup; reject stale-owner keys before the per-key charge; drop the separate Users query; add the ApiKeyAuthProjection result type.
• backend/tests/Taskdeck.Api.Tests/ApiKeyMiddlewarePerKeyRateLimitTests.cs — new StaleOwnerKey_Returns401_ChargesIpFailureBudget_BeforePerKeyCharge (unit, real SQLite + command interceptor); updated the over-quota and (renamed) under-quota DB-work-counting tests to assert the single folded lookup and the absence of a standalone Users SELECT.
• backend/tests/Taskdeck.Api.Tests/McpHttpTransportApiKeyTests.cs — new integration test McpEndpoint_StaleOwnerKey_IsRateLimitedBeforeDatabaseLookup mirroring the nonexistent-key pre-auth-budget convention: first attempt 401s, second attempt 429s via McpAuthenticationPerIp.

Test evidence

dotnet build backend/Taskdeck.sln -c Release
  -> Build succeeded.

dotnet test backend/Taskdeck.sln -c Release -m:1 --filter "FullyQualifiedName~ApiKey"
  -> Taskdeck.Domain.Tests:  Passed: 11, Failed: 0
  -> Taskdeck.Api.Tests:     Passed: 67, Failed: 0
  -> Taskdeck.Cli.Tests:     Passed: 12, Failed: 0

dotnet test backend/tests/Taskdeck.Api.Tests --filter \
  "FullyQualifiedName~StaleOwner|...UnderQuotaValidKey_PassesThrough_WithFoldedOwnerLookup|...OverQuotaValidKey|...McpAuthenticationRateLimit"
  -> Passed: 13, Failed: 0

Acceptance mapping:

• Stale-owner 401 + IP-budget charge + eventual pre-auth trip: unit test asserts AuthenticationFailedItemKey set and ApiKeyIdItemKey NOT set (per-key never charged); integration test asserts second attempt is 429 via McpAuthenticationPerIp.
• Valid active-owner keys unchanged: UnderQuotaValidKey_PassesThrough_WithFoldedOwnerLookup, OverQuotaValidKey_Returns429_..., and the existing per-key-count / IP-budget tests still pass.
• Standalone Users lookup gone: interceptor assertions (ContainSingle SELECT, no standalone Users SELECT) confirm one folded query.

Verification scope note: targeted filters only — the full backend suite is the coordinator's to run.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Chris0Jeky]

Coordinator adversarial review — two independent read-only lenses (overnight run 2026-07-17)

Lens A — security / auth-ordering / abuse-economics: MERGE-SAFE, 0 findings.
All six attack categories survived refutation: per-request-class gate ordering (no class charges less or does more DB work than intended; stale-owner now charges the IP failure budget exactly once, before the per-key charge), no new existence oracle (the differentiated 401 message for stale-owner vs nonexistent predates this PR and requires possession of a real key plaintext to observe), null/deleted-owner handling, expiry-clock equivalence with ApiKey.IsActive (same clock, same > boundary), #1381/#1401 invariants (valid keys still never touch the failure budget; per-key 429 still charges nothing), and DoS economics (budget still fully stops DB work after trip; the dominant fabricated-key flood path got no more expensive). Test sweep found no tautological assertions.

Lens B — EF-translation / blast radius / test rigor: MERGE-SAFE, 1 LOW.

• LOW — ApiKeyMiddleware.cs (projection + owner-gate comments): the "hard-deleted owner → OwnerIsActive is null" scenario the comments present as live is unreachable, and the null branch is untested. ApiKeyConfiguration sets the User FK to DeleteBehavior.Cascade (and SQLite runs with PRAGMA foreign_keys=ON), so a hard-deleted user cascades away the key rows — the lookup misses and takes the nonexistent-key 401 path, never a found-key-with-null-owner. App-level "deletion" (AccountDeletionService) is soft (IsActive=false), which is the false branch. The != true gate is correct and harmlessly defensive, but the comment invites future readers to trust a scenario that cannot occur, and the branch has no pinning test.

Lens B otherwise confirmed: single-statement SQLite translation of the correlated projection (real-SQLite tests assert ContainSingle(IsSelect)), in-memory keyIsActive character-equivalent to the entity property, UpdateLastUsedAsync still targets the right row, no other consumer of the changed context-item semantics, interceptor state per-test-instance (no cross-test leakage).

Required fixes (zero-skip; one batched round)

1. Reword the null-owner comments to state plainly that the null branch is defensive-only and unreachable (cascade FK removes keys on hard delete; app deletion is soft) — stop presenting it as a live runtime case.
2. Add one test pinning the REAL hard-delete behavior: delete the owner row (cascade) → the key lookup misses → nonexistent-key 401 path (AuthenticationFailedItemKey set, no ApiKeyIdItemKey). That closes the coverage gap with the truth instead of testing an unreachable branch.

Fix evidence comment to follow after the push.

[Chris0Jeky]

Fix evidence — review round (comment 5006673128)

Both adversarial lenses returned MERGE-SAFE with one LOW finding; fixed zero-skip in commit 7fb32873.

#	Finding	Fix	Verification
1 (LOW)	Middleware comments presented the "hard-deleted owner → OwnerIsActive is null" scenario as a live runtime case. It is unreachable: ApiKeyConfiguration sets the Users FK to DeleteBehavior.Cascade and Microsoft.Data.Sqlite enforces PRAGMA foreign_keys=ON, so a hard-deleted user cascades its ApiKey rows away and the lookup takes the nonexistent-key 401 path; app-level account deletion (AccountDeletionService) is soft (IsActive=false).	7fb32873 — reworded all four comment sites in ApiKeyMiddleware.cs (fold comment, projection inline comment, owner-gate comment, ApiKeyAuthProjection XML doc) to state the null branch is defensive-only and unreachable, with the soft-delete reality named as the only live stale-owner scenario. The != true gate is unchanged.	Build succeeded; comments only, no behavior change — full ApiKey filter re-run below confirms no regression.
2 (follow-on from 1)	Add a test pinning the REAL hard-delete behavior so the defensive-only claim stays honest.	7fb32873 — new HardDeletedOwner_CascadesKeyAway_TakesNonexistentKey401Path in ApiKeyMiddlewarePerKeyRateLimitTests.cs (colocated with StaleOwnerKey_...): ExecuteDeleteAsync on the owner row in the real SQLite test db, asserts the cascade removed the key row, then asserts the auth attempt takes the nonexistent-key 401 path — AuthenticationFailedItemKey set, ApiKeyIdItemKey NOT set, exactly one folded SELECT, no standalone Users SELECT, no last-used UPDATE, 401 error contract.	Test passes (included in counts below).

Test evidence (post-fix)

dotnet build backend/Taskdeck.sln -c Release
  -> Build succeeded.

dotnet test backend/Taskdeck.sln -c Release -m:1 --filter "FullyQualifiedName~ApiKey" --no-build
  -> Taskdeck.Domain.Tests:  Passed: 11, Failed: 0
  -> Taskdeck.Api.Tests:     Passed: 68, Failed: 0   (was 67; +1 new hard-delete cascade test)
  -> Taskdeck.Cli.Tests:     Passed: 12, Failed: 0

Scope note: targeted filters only, per the wave protocol — the full suite is the coordinator's gate.