fix(dynamodb): enforce username uniqueness in getOrCreateHandle#113
Merged
Conversation
DynamoDbUserLookup.getOrCreateHandle did a read-then-conditional-put whose `attribute_not_exists(pk)` condition guarded the random-handle key (USER#<handle>), not the username, and a GSI cannot enforce uniqueness. Two concurrent first-time getOrCreateHandle calls for the same username therefore both minted a handle and both wrote successfully, leaving two user rows sharing USERNAME#<name>. Lookups (findFirst over the unenforced GSI) then resolved non-deterministically, splitting one identity across two handles and stranding a registered passkey at login. Write the user row and a dedicated username-uniqueness marker (pk = USERNAME#<lower>) atomically via TransactWriteItems, each with attribute_not_exists(pk). The marker is the real guard: only one racer can create USERNAME#<name>, so the loser's transaction is cancelled. On TransactionCanceledException, recover the winner's handle from the marker with a strongly-consistent read (the username GSI is only eventually consistent and may lag); if no marker exists the cancellation was transient, so rethrow rather than return an unpersisted handle. The marker leaves gsi1pk unset so the sparse username GSI never indexes it. Mirrors the JDBI backend's ON CONFLICT (username) path. Adds an 8-thread concurrent-race test asserting all racers converge on one handle and the persisted lookup agrees. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Spotless 8.x's google-java-format lazily loads Guava classes that fail to
initialize (NoClassDefFoundError com/google/common/collect/ImmutableCollection)
when the task work is served from a Gradle cache instead of running fresh. The
build cache was already disabled repo-wide for this, but the configuration cache
triggers the same crash on a cache-hit run (e.g. a second `check` or a clean
`check`), leaving the gate red through no fault of the code.
Extend the existing per-task cacheIf { false } workaround with
notCompatibleWithConfigurationCache so the spotless tasks always run fresh. The
configuration cache is retained for invocations that don't run spotless.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
wolpert
enabled auto-merge (rebase)
July 3, 2026 14:41
|
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.



Summary
Fixes a concurrency/data-integrity bug in the DynamoDB
UserLookupimplementation surfaced by a security review of the persistence layer.DynamoDbUserLookup.getOrCreateHandledid a read-then-conditional-put whoseattribute_not_exists(pk)condition guarded the random-handle key (USER#<handle>), not the username — and a GSI cannot enforce uniqueness. Two concurrent first-timegetOrCreateHandlecalls for the same username therefore both minted a handle and both wrote successfully, leaving two user rows sharingUSERNAME#<name>. Subsequent lookups (findFirstover the unenforced GSI) resolved non-deterministically, splitting one identity across two handles and stranding a registered passkey at login (emptyallowCredentials→ lockout). The oldcatch (ConditionalCheckFailedException)branch was effectively dead code, since the handle-keyed condition never collided on the username.This is an integrity/availability bug (no cross-user data access or auth bypass). It's reachable both by an attacker timing two registration-start calls for a fresh username and organically via a double-submitting client. The JDBI backend was already correct (
INSERT ... ON CONFLICT (username) ... RETURNING).The fix
pk = USERNAME#<lowercased>) atomically viaTransactWriteItems, each guarded byattribute_not_exists(pk). The marker is the real guard — only one racer can createUSERNAME#<name>, so the loser's transaction is cancelled.TransactionCanceledException, recover the winner's handle from the marker with a strongly-consistentGetItem(the username GSI is only eventually consistent and may lag). If no marker exists, the cancellation was transient — rethrow (→ 503) rather than return an unpersisted handle, closing a latent bug in the old.orElse(handle).gsi1pkunset so the sparse username GSI never indexes it; only the realUSER#row is returned by lookups.Tests
Adds
concurrentGetOrCreateForSameUsernameConvergesOnOneHandle— an 8-threadCountDownLatchrace (mirroring the refresh-token race test) asserting all racers converge on exactly one handle and the persisted lookup agrees. It fails against the old code and passes against the fix, on DynamoDB Local via Testcontainers.Build fix (second commit)
While verifying,
./gradlew checkintermittently failed on:pk-auth-backup-codes:spotlessJavaCheck— aNoClassDefFoundErrorin Spotless's google-java-format worker, unrelated to the code (reproduces on a pristine tree). It's the documented Spotless 8.x lazy-Guava-load bug: the build cache was already disabled for it, but the configuration cache triggers the same crash on a cache-hit run. Extended the existing per-task workaround withnotCompatibleWithConfigurationCacheso the spotless tasks always run fresh; the config cache is retained for invocations that don't run spotless.Verification
./gradlew clean check(configuration cache enabled) → BUILD SUCCESSFUL, including the new race test and Error Prone-Werror -Xlint:all../gradlew check(the previously-failing cache-hit scenario) → BUILD SUCCESSFUL.🤖 Generated with Claude Code