Universal Add Factor - #136
Conversation
A concurrent same-identity PutObject race returns 412 while the winner's factor is already in metadata. Returning Inserted made the loser report the wrong factor_id and skip encryption-key reconciliation; route that case through FactorAlreadyExists instead. Lost-ACK (timeout) with factor present still reconciles as Inserted.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5d60a9d02
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .lookup_consistent(FactorScope::Main, &factor_to_lookup) | ||
| .await? | ||
| { | ||
| Some(existing_backup_id) if existing_backup_id == backup_id => { |
There was a problem hiding this comment.
When request B reaches this branch while request A owns the newly inserted lookup row, B is allowed to continue even though A can still delete that row at the rollback on line 416. For example, A can fail definitively because its encrypted key type already exists while B, adding the same factor without that key, successfully writes the factor metadata; A then deletes the shared lookup row, leaving B's successfully added factor untraceable during recovery. Coordinate ownership or recheck metadata before A rolls back a row that another request may have adopted.
Useful? React with 👍 / 👎.
| let (verified_backup_id, _metadata) = auth_handler | ||
| .clone() | ||
| .verify( | ||
| &request.existing_factor_authorization, |
There was a problem hiding this comment.
When the same OIDC ID token and session keypair are used for both sides of a same-account metadata-only upgrade, this verification records the token's nonce as used, and the later validate_factor_registration call verifies the same token again and fails with already_used before reaching the duplicate/key-only path. Consequently, the advertised same-account upgrade cannot use the single OIDC sign-in already proving possession of that account and instead requires two independently minted ID tokens; handle this case by verifying the new-factor challenge signature without consuming the OIDC nonce a second time.
Useful? React with 👍 / 👎.
Map 412 + present factor to Inserted when our attempted factor id is already in metadata, so sync add retries stay successful; keep FactorAlreadyExists only when a different id won the race.
Before deleting a FactorLookup row after a NotInserted metadata write, re-read metadata and skip the delete if the factor is already present so a concurrent same-backup request is not left untraceable.
When the same ID token and session keypair authorize both existing and new factor sides, skip a second Redis nonce mark so registration can still verify the new-factor challenge after the first verify consumed it.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 32846c7. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32846c76fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if lookup_insert_succeeded && write.should_rollback_lookup() { | ||
| let factor_still_absent = match backup_storage.get_metadata_by_backup_id(&backup_id).await { | ||
| Ok(Some((metadata, _))) => !metadata.factors.iter().any(|f| f.kind == new_factor_kind), |
There was a problem hiding this comment.
When request A inserts the lookup and request B receives the same-backup conditional collision, B is allowed to continue; if A's metadata write then fails, this pre-delete read can observe the factor as absent, B can commit it immediately afterward, and A will still unconditionally delete the lookup, leaving B's successfully added factor untraceable. Fresh evidence in this revision is that the added safeguard is only a pre-delete S3 read, so the original race remains between this read and FactorLookup::delete; coordinate ownership or recheck/heal after deletion instead.
Useful? React with 👍 / 👎.
| if metadata | ||
| .keys | ||
| .iter() | ||
| .any(|k| k.flattened_kind() == encryption_key.flattened_kind()) | ||
| { | ||
| // Key type already present — nothing to do (idempotent). | ||
| return Ok(()); |
There was a problem hiding this comment.
When a same-factor upgrade supplies an encryption key whose flattened type already exists but whose encrypted key or Turnkey IDs differ, this returns Ok(()); the handler consequently responds successfully even though none of the requested key data was stored. Fresh evidence in this revision is that the new reconciliation helper checks only flattened_kind, so both sequential replacements and the loser of concurrent same-type upgrades are incorrectly reported as applied; reserve the no-op for an exactly equal key or return a conflict.
Useful? React with 👍 / 👎.
Ignore Apple aud None vs explicit default when detecting the same ID token + session keypair so same-account upgrades do not hit already_used.
…back After deleting a lookup row on NotInserted, re-read metadata and re-insert if the factor appeared in the race window so an adopter is not left untraceable.
No-op add_encryption_key_only only when the existing key equals the requested one; same flattened kind with different material returns OnlyOneEncryptionKeyPerTypeAllowed instead of a false success.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa792ec6c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let needs_heal = match backup_storage.get_metadata_by_backup_id(&backup_id).await { | ||
| Ok(Some((metadata, _))) => { | ||
| metadata.factors.iter().any(|f| f.kind == new_factor_kind) | ||
| } |
There was a problem hiding this comment.
When request A inserts the lookup and later rolls it back while request B has adopted that same-backup row, B can commit its factor immediately after this post-delete metadata read returns absent; A then finishes without restoring the lookup, leaving B's successfully added factor untraceable. Fresh evidence in this revision is that the new post-delete heal narrows the race but still leaves an uncoordinated interval after the final read; use ownership/conditional coordination or make the successful writer verify and restore its lookup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Codex Review: Didn't find any major issues. What shall we delve into next?
Reviewed commit: 28dbca3528
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if Self::is_precondition_failed_put(&e) | ||
| && !Self::metadata_contains_factor_id( | ||
| &metadata, | ||
| attempted_factor_id, | ||
| target_scope, | ||
| ) |
There was a problem hiding this comment.
When two requests concurrently add the same factor identity and request A receives a timeout while request B wins the conditional write, reconciliation sees B's matching FactorKind but skips the ID comparison because the error is not a 412, then reports A as inserted and returns A's nonexistent factor ID. Fresh evidence in this revision is that attempted_factor_id is now available but its comparison is gated on is_precondition_failed_put; use the ID to distinguish the request's write from a concurrent winner for all ambiguous errors.
Useful? React with 👍 / 👎.
| self.put_object() | ||
| .bucket(self.environment.s3_bucket()) | ||
| .key(get_metadata_key(backup_id)) | ||
| .if_match(e_tag) | ||
| .body(ByteStream::from(serde_json::to_vec(&metadata)?)) | ||
| .send() | ||
| .await?; |
There was a problem hiding this comment.
When two independently authorized same-account upgrades add the same encryption key concurrently, both can read metadata without that key, one write succeeds, and the other receives a 412 from this if_match; the loser propagates an error even though the exact requested key is now stored. Re-read after a conditional failure and return idempotent success for an exact key match, while retaining the conflict for different key material.
Useful? React with 👍 / 👎.
After metadata lands, re-insert/verify the lookup row so a concurrent inserter's NotInserted rollback cannot leave the factor untraceable.
Treat kind-present + different attempted id as FactorAlreadyExists on timeouts and other ambiguous failures, not only 412, so the loser does not report Inserted with the wrong factor id.
After if_match precondition failure, re-read metadata and succeed when the exact requested key is already stored; keep the conflict for different material of the same kind.
|
@codex review extensively (all feedback points) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28dbca3528
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .factors | ||
| .iter() | ||
| .find(|f| f.kind == new_factor_kind) | ||
| .map_or(new_factor_id, |f| f.id.clone()); |
There was a problem hiding this comment.
When a duplicate re-add races with deletion of the existing factor, the initial write reports FactorAlreadyExists, but this later read can no longer find that factor; map_or then substitutes the newly generated, never-persisted ID, and ensure_main_factor_lookup restores a lookup row for a factor absent from metadata. The endpoint consequently returns success with a nonexistent factor ID and leaves a stale lookup. Fresh evidence in this revision is the fallback to new_factor_id; require the matching stored factor to exist or retry the full metadata write instead.
Useful? React with 👍 / 👎.
| if !Self::is_precondition_failed_put(&put_err) { | ||
| return Err(put_err); |
There was a problem hiding this comment.
When the key-only PutObject is accepted by S3 but the client receives a timeout or dispatch/response error, this branch immediately reports failure without checking whether the exact key landed. The same file correctly classifies such failures as ambiguous for factor writes, and here the add-factor challenge and OIDC nonce have already been consumed, so the caller sees a failed upgrade that actually succeeded and cannot simply replay the request. Re-read and reconcile exact key equality for all ambiguous errors, not only 412 responses.
Useful? React with 👍 / 👎.
| if let Err(delete_err) = factor_lookup | ||
| .delete(FactorScope::Main, &factor_to_lookup) | ||
| .await |
There was a problem hiding this comment.
When this DeleteItem removes the lookup row but its response is lost to a timeout or dispatch error, the error branch only logs and skips the post-delete metadata check and healing logic. If a same-factor adopter committed metadata and completed ensure_main_factor_lookup before the delayed delete landed, this rollback then removes the lookup last and leaves the successfully added factor untraceable. Fresh evidence in this revision is that healing runs only when delete returns Ok; reconcile ambiguous delete outcomes with a consistent read and heal, or use conditional ownership coordination.
Useful? React with 👍 / 👎.
Do not fall back to a never-persisted factor id when the matching factor is gone (e.g. concurrent delete); return factor_not_found and avoid restoring a stale lookup.
Re-read metadata after timeouts and other ambiguous errors (not only 412) so a lost ACK of an exact key upgrade is reported as success.
Always re-check metadata and restore the lookup after a rollback delete attempt, including when delete returns a timeout/dispatch error that may still have removed a concurrent writer's row.
|
Split into a stacked review series (same end state as this branch tip
Prefer reviewing/merging that stack bottom-up. This PR can be closed once the stack lands (or kept as a reference until then). Snapshot: |
|
@github-copilot review |
Summary
Closes the Backup & Recovery gap where only Passkey could authorize add-factor. An existing OIDC (Google/Apple) Main Factor can now authorize adding Passkey or another OIDC factor.
Revives Luke’s universal add-factor work on this branch, rebased onto current
main, scoped per Slack, and hardened for review / Bugbot / Codex feedback.Linear: Accept multiple factors for B&R for a Google factor — “if you have a Google factor you can't add any other factor.”
In scope
existingFactorKindon/v1/add-factor/challenge(defaults toPASSKEYfor current clients).platform: IOS | ANDROID(Android uses Google Password Manager registration, same as create-backup).add_encryption_key_only(metadata-only Turnkey upgrade), including with a single OIDC ID token + session keypair for both sides.Out of scope (explicit Slack decision)
Compatibility
Existing Passkey → OIDC clients keep working: same request shape,
newFactorChallengeremains a base64 string for OIDC,existingFactorKindoptional. Stricter server behavior only on challenge replay and lookup races (correct failures, not contract breaks).What changed
AuthHandler::verify(challenge consumed / replay-safe).use_challenge_tokenerrors (already_used).registration_mismatch).FactorKindequality compares issuer/sub only (ignoresturnkey_provider_id) so same-account re-adds upgrade keys instead of duplicating factors.aud: Nonevs explicit default still matches).ConditionalCheckFailedis idempotent; cross-backup →factor_already_exists; re-read useslookup_consistent; rollback heals if a concurrent writer landed the factor; successful writer alwaysensure_main_factor_lookup.resolve_put_object_outcome: kind present + our factor id →Inserted; kind present + different id →FactorAlreadyExistsfor all ambiguous errors (not only412).add_encryption_key_only: exact key match is idempotent; same kind with different material → conflict; concurrent412re-reads and succeeds on exact match.mainAPIs:backupMetadataresponse,FactorMetadataWrite, one-encryption-key-per-type rules.not_supported.Test plan
encryptedBackupKeymetadata-only upgrade (two sessions)turnkeyProviderId→ no duplicate factorPASSKEY_REGISTRATIONchallenge shape + completable registrationregistration_mismatchfactor_already_exists(other backup’s lookup untouched)already_used412/ timeout; encryption key exact-match / mismatch / concurrent412Note
High Risk
Large changes to authentication, factor registration, and distributed lookup/metadata consistency for backup recovery; mistakes could allow wrong factors, duplicate mappings, or broken add-factor flows.
Overview
Universal add-factor closes the gap where only Passkey could authorize adding a factor: an existing OIDC (Google/Apple) main factor can now add Passkey or another OIDC factor, alongside the existing Passkey → OIDC path. EC/keychain main factors are explicitly rejected for add-factor.
/v1/add-factor/challengegains optionalexistingFactorKind(defaults to Passkey), Passkey registration as a new factor (iOS/Android WebAuthn ceremonies), and binding of the existing-factor token to the exact new-factor descriptor (OIDC token or SHA-256 of registration state)./v1/add-factorimplements OIDC-existing auth viaAuthHandler::verify, passkey challenge replay protection, strict new/existing factor pairing, and same-session OIDC upgrades where one JWT + session keypair can authorize both sides without double-consuming the OIDC nonce.Identity and storage: OIDC
FactorKindequality ignoresturnkey_provider_id(issuer/sub only), matching lookup keys.add_encryption_key_onlysupports metadata-only Turnkey key attachment when the factor already exists. S3resolve_put_object_outcomedistinguishes our factor id vs a concurrent winner; Dynamolookup_consistentand lookup heal/ensure logic tighten races between metadata andFactorLookup.Reviewed by Cursor Bugbot for commit 5f2e405. Bugbot is set up for automated code reviews on this repo. Configure here.