fix(otp): pin the stored-code format in the shared contract, and fix the tests it broke - #140
Merged
Merged
Conversation
…ntext The OTP record held the plaintext code, letting a reader of Redis reverse the six-digit keyspace offline. The core service now fingerprints the code (HMAC-SHA256 under the identifier key, hex) before it is stored or compared, held byte-identical with nest-auth so the two implementations share one otp: keyspace.
…the tests it broke
Lands the OTP fingerprint on rust-auth, closing a divergence that has been live
since nest-auth merged its half: nest-auth main writes keyed fingerprints into
the shared `otp:` records while this side still wrote and compared plaintext, so
a code minted on one backend could not verify on the other, in either direction.
The cherry-picked commit is unchanged and keeps its authorship. Two things are
added around it.
conformance/wire-contract.json gains the format. `credentialFormats` pinned the
refresh token, the password hash, the TOTP secret at rest, the recovery-code
digest and the WS ticket -- but never the OTP record's stored value, and
`recordEncodings` did not mention it either. The drift detector was silent on
exactly the field that drifted. The entry sits beside recoveryCodeDigest, and
the existing hmac-derivation vector is extended with `otpCode` /
`otpRecordCodeHex` so the chain runs secret -> key -> identifier -> stored code
in one place. The vector was computed independently rather than read back out of
this implementation, so it checks the code instead of following it. Patched
textually, not through a JSON round-trip, so the file stays byte-identical with
nest-auth's copy.
The tests the fingerprint broke are fixed by taking the code from where it now
is. Seven failed: the store no longer holds the plaintext, so anything that read
a code back out of it and submitted it was submitting a hash. They now read the
code from the mail the engine actually sent -- which is also how the recipient
gets it -- through a capturing provider on both harnesses. One test planted an
OTP directly into the store; it now plants through the service, so the record is
written the way production writes it.
Five further defects surfaced, none of which failed a test -- the 100% line gate
was the only thing that saw them:
- Two tests began skipping themselves. The mail send is detached, so reading the
mailbox the instant a flow returns is a race, and `let Some(x) = .. else
{ return }` turns a lost race into a pass. They now poll to a deadline and
assert before unwrapping.
- The await helper returned the first non-empty mailbox, so a second read
answered instantly with the FIRST code. The reads now consume, and the one
test that mails twice drains explicitly between sends.
- verify_email's vanished-account arm stopped being reached: the test submitted
a fingerprint, failed the OTP check, and its `OtpInvalid` assertion passed for
the wrong reason. Confirmed reached now by changing the arm's error and
watching the test go red.
fmt, clippy -D warnings, rustdoc -D warnings, the full suite, and llvm-cov at
100% lines and 100% functions.
There was a problem hiding this comment.
Pull request overview
Pins interoperable, HMAC-based OTP storage while updating affected tests.
Changes:
- Stores and verifies keyed OTP fingerprints.
- Adds shared wire-contract vectors.
- Captures mailed plaintext OTPs in test harnesses.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
crates/bymax-auth-core/src/services/otp.rs |
Implements keyed OTP fingerprints. |
crates/bymax-auth-core/src/services/auth/password_reset.rs |
Updates reset-flow tests. |
crates/bymax-auth-core/src/services/auth/mod.rs |
Adds capturing mail test support. |
crates/bymax-auth-core/src/services/auth/email_verification.rs |
Updates verification tests. |
crates/bymax-auth-core/src/services/auth/email_change.rs |
Adapts the test harness. |
crates/bymax-auth-core/src/engine/builder.rs |
Supplies the fingerprint key. |
crates/bymax-auth-axum/tests/common/mod.rs |
Adds adapter mail capture support. |
crates/bymax-auth-axum/tests/adapter.rs |
Uses mailed OTPs in integration tests. |
conformance/wire-contract.json |
Pins the shared OTP format and vector. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Two findings, both correct.
The first is the one this PR's own description spends a table explaining: a
mailbox read that races the detached send, unwrapped with `else { return }`, so
a lost race is a test that exercises nothing and reports success. I found five
of those, fixed them, wrote them up -- and left a sixth a few lines from one of
the fixes. It now polls and asserts like the second issuance below it. Confirmed
by grep that the only remaining direct reads are the ones inside the polling
helpers themselves, which is where they belong.
The second: inserting CapturingEmails immediately above `pub struct Harness`
left the pre-existing Harness doc comment attached to the new type, so rustdoc
described the mailer as "The built engine..." and Harness had no description at
all. The line is back on Harness.
fmt, clippy -D warnings, rustdoc -D warnings, the full suite, and llvm-cov at
100% lines and 100% functions.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (3)
crates/bymax-auth-core/src/services/auth/password_reset.rs:1121
- This new explanation contradicts the test's preceding description, which still says the code is read from the in-memory store. Update that summary to say the code comes from the captured email.
// From the mail; the stored record is a keyed fingerprint, not the code. Awaited and
crates/bymax-auth-core/src/services/auth/email_verification.rs:217
- This block now correctly reads from mail, but the preceding test description still says it reads the code from the in-memory store. Update that stale summary to match the test.
// From the mail the engine sent, not from the store: the `otp:` record holds a keyed
crates/bymax-auth-axum/tests/adapter.rs:2238
- The test now obtains the OTP from the captured email, so the earlier claim that it uses the in-memory OTP peek is stale and contradicts this new block.
// Recovered from the mail the engine actually sent, which is where the code is. The stored
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.
What
Lands the OTP fingerprint on rust-auth and pins its format in the shared contract.
Warning
This closes a divergence that is live right now. nest-auth
mainmerged its half (their #98), so it writes keyed fingerprints into the sharedotp:records while this side still writes and compares plaintext. For any deployment sharing one Redis, a code minted on one backend cannot verify on the other — in both directions — and has not been able to since that merge.The cherry-picked commit is unchanged and keeps its original authorship. Everything else here is what it took to land it.
The contract was silent on exactly the field that drifted
conformance/wire-contract.jsonexists so this class of split fails a test. It pinsrefreshToken,passwordHash,totpSecretAtRest,recoveryCodeDigestandwsTicket— but there was no entry for the OTP record's stored value, andrecordEncodingsdid not mention it either. Both repos could ship incompatible OTP storage with the conformance suite green, which is precisely what happened.Added:
Two deliberate choices:
nest-auth will mirror these exact bytes in a coordinated PR; the file has to move on both sides together or conformance goes red on whichever moves first.
Seven tests broke, and fixing them found five more defects
The store no longer holds the plaintext, so anything that read a code back out of it and submitted it was submitting a hash. They now take the code from the mail the engine actually sent — which is also how the recipient gets it — through a capturing provider on both harnesses. One test planted an OTP straight into the store; it now plants through the service, so the record is written the way production writes it.
None of the five below failed a test. The 100% line gate was the only thing that saw them:
let Some(x) = .. else { return }turns a lost race into a pass. They now poll to a deadline and assert before unwrapping.verify_email's vanished-account arm stopped being reachedOtpInvalidassertion passed for the wrong reason — reporting "no such account" where the code simply did not match.The last one is confirmed fixed rather than assumed: changing that arm's error now turns the test red, which it did not before.
The give-up path of the new polling helpers is reachable through a parameterised
_withinform with a test that passes a small count — the same idiomawait_rehash_withinalready uses in this repo.Verification
cargo fmt --all --checkcargo clippy --workspace --all-targets --all-features -- -D warningsRUSTDOCFLAGS="-D warnings" cargo docllvm-covlinesllvm-covfunctionsRun in an isolated worktree off
main.