Cross-user reference fix, request logging, credential cleanup and migration drift guard - #129
Merged
Merged
Conversation
…ce references The foreign keys on user_subscriptions require only that the referenced row exists, not that it belongs to the caller. Create and update took CategoryId and PaymentSourceId straight from the request body, so an authenticated user could attach another user's private category or payment source to their own subscription - and BuildDtoQuery joined both tables unfiltered, handing the resolved name and label back in the response. Two changes, because either alone leaves a hole: - ValidateCatalogReferenceAsync becomes ValidateReferencesAsync and checks all three references, collecting every violation into one ValidationProblem. A category must be a system default or the caller's; a payment source must be the caller's. The message is the same whether the row is missing or foreign, so a 400 never confirms that an id exists. - BuildDtoQuery filters the category and payment-source sides to what the caller may see before joining, so a subscription pointing at someone else's row resolves to a null name/label. This keeps the read path safe on its own, including for rows written before the check existed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rd started responses Three defects in TryHandleAsync, all of which surfaced as noise or a worse failure rather than a clean 500: - A client that backgrounds the app aborts the request, and the resulting OperationCanceledException was logged at Error and answered with a write to a connection that no longer exists. Mobile clients do this constantly, so the error rate measured disconnects rather than faults. Every background service already filters cancellation the same way; the handler was the one place that did not. - The ProblemDetails carried no traceId, so a user quoting "an unexpected error occurred" could not be matched to any log line - the log recorded only method and path, which on a busy endpoint identifies nothing. - The response write was unguarded by HasStarted. An exception raised after the response began streaming made the StatusCode assignment throw from inside the exception handler, replacing a partial response with a second failure. Returning false instead lets the server abort the connection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… reuse Serilog was configured but nothing on the request path reached it. The Microsoft.AspNetCore level override sits at Warning and UseSerilogRequestLogging was never called, so a 200, a 401, a 404 and a 429 all produced no output - the only line the API ever emitted per request was an unhandled exception. Nothing recorded who called what, when, or how long it took. - UseSerilogRequestLogging emits one summary line per request, enriched with the authenticated user id where there is one. Id only: email addresses, tokens and reset codes must not reach the sink. Placed after UseForwardedHeaders so the client address is the real one, and before UseExceptionHandler so a request ending in a 500 still gets its line. - The rate limiter's OnRejected wrote a 429 and logged nothing, making a limiter doing its job indistinguishable from an endpoint nobody calls. - AuthService now logs the refresh-token reuse branch. That path means a credential may have been stolen and signs the user out of every device; until now it left no trace, so "why was I logged out everywhere?" had no answer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing ever deleted a row from either table. AuthService only sets RevokedAt/UsedAt and filters on ExpiresAt at read time, so both grew without bound - and refresh_tokens grows fastest of anything in the schema: access tokens live 15 minutes and refresh tokens rotate on every use, so one active client writes a row every 15 minutes or so, a few thousand per user per month against a 0.5 GB database. The rows are retained credential material too. A SHA-256 hash of a dead token is not a live secret, but keeping every token ever issued enlarges what a database compromise yields for no benefit, and reset-code hashes cover a 10^6 keyspace that is cheap to reverse. ExpiredCredentialCleanupBackgroundService follows FxRateRefreshBackgroundService - same DailyUtcSchedule, scoped-DbContext and swallow-and-log shape - and runs at 03:00 UTC by default. ExecuteDelete rather than loading entities: the row count is unbounded by definition, so materialising it is the one way this job could exhaust memory. Retention windows are deliberate, not arbitrary. Refresh tokens survive 7 days past expiry because deleting one the moment it expires makes it indistinguishable from a token that never existed, and RefreshAsync treats those cases differently - the unknown-token path is what drives reuse detection. Reset codes get a day, since they are weaker and expire in 15 minutes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing caught an entity configuration change that never got a migration. SchemaMigrationTests applies the history and asserts the resulting columns against a hardcoded list, so it passes happily in exactly that case: it builds the old schema and asserts the old columns. The mismatch then surfaced only against a real database, as a query-time failure. HasPendingModelChanges compares the model built from the configurations against AppDbContextModelSnapshot. No database is touched - the connection string is never opened - so this runs in milliseconds without Docker, and the failure message names the command that fixes it. Verified in both directions: passes on the current tree, and fails with the intended message when a property is added to an entity without a migration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Update: the Testcontainers suites have now run. Docker came up locally, so the caveat in the description is resolved — nothing here is unverified any more.
The 6 No changes to the branch; this is the same commits, now exercised. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five issues from the architecture review, one commit each.
Closes #119
Closes #121
Closes #122
Closes #123
Closes #124
#119 — cross-user category/payment-source references (the one that matters)
The foreign keys on
user_subscriptionsrequire only that the referenced row exists, not that it belongs to the caller. Create and update tookCategoryIdandPaymentSourceIdstraight from the body, andBuildDtoQueryjoined both tables unfiltered — so an authenticated user could attach another user's private category or payment source to their own subscription and read the resolved name and label back out.Fixed on both sides, because either alone leaves a hole:
ValidateCatalogReferenceAsyncbecomesValidateReferencesAsync, checking all three references and collecting every violation into oneValidationProblem. A category must be a system default or the caller's; a payment source must be the caller's. Same message whether the row is missing or foreign, so a 400 never confirms an id exists.BuildDtoQueryfilters the category and payment-source sides to what the caller may see before joining, so a subscription pointing at a stranger's row resolves to a null name/label. This keeps the read path safe on its own, including for rows written before the check existed.New
CrossUserReferenceTestscovers create and update with foreign ids, the system-default category that a blunt "must be mine" check would wrongly reject, own-rows-still-work, and a read-path test that writes the cross-tenant reference directly to the database to prove the join scoping stands alone.#122 — GlobalExceptionHandler
Client disconnects (
OperationCanceledException) were logged at Error and answered with a write to a dead connection — routine mobile traffic counted as server faults. Added the same cancellation filter every background service already has. Also added atraceIdto theProblemDetailsso a quoted error maps to a log line, and aHasStartedguard so an exception mid-stream no longer throws from inside the exception handler.#121 — request logging
Serilog was configured but
UseSerilogRequestLoggingwas never called, andMicrosoft.AspNetCoresits at Warning — so 200s, 401s, 404s and 429s produced no output at all. Added the request log (enriched with user id only; no emails, tokens or reset codes), a warning on rate-limit rejection, and a warning on the refresh-token reuse branch — that path signs a user out of every device and previously left no trace.#123 — credential cleanup
Nothing ever deleted from
refresh_tokensorpassword_reset_codes. With 15-minute access tokens and rotation on every use, one active client writes a refresh-token row every ~15 minutes against a 0.5 GB database. New dailyExpiredCredentialCleanupBackgroundService, shaped afterFxRateRefreshBackgroundService, usingExecuteDelete.Retention windows are load-bearing: refresh tokens survive 7 days past expiry because deleting one at expiry makes it indistinguishable from a token that never existed, and
RefreshAsynctreats those cases differently — the unknown-token path is what drives reuse detection.#124 — migration drift
SchemaMigrationTestsasserts columns against a hardcoded list, so a configuration change with no migration passes: it builds the old schema and asserts the old columns.HasPendingModelChangescompares the model against the snapshot with no database involved. Verified in both directions — passes on this tree, fails with the intended message when a property is added without a migration.Testing
SubVora.Application.TestsSubVora.Infrastructure.Tests(Docker-free subset, incl. new drift test)GlobalExceptionHandlerTestsCrossUserReferenceTests,ExpiredCredentialCleanupTestsDocker was not running locally, so the Testcontainers-backed suites could not be exercised. The #119 and #123 tests are unverified at runtime and need a CI run (or a local run with Docker up) before this merges. Everything that does not need a container passes.
Not included
#120 (production migrations never run automatically) is left out deliberately — fixing it means overriding a decision recorded in
CLAUDE.mdanddocs/DEPLOYMENT.md("In theProductionenvironment the API does not migrate on startup — that is deliberate"). That is a call to make, not to assume. #124 here is a prerequisite either way.