domain: split-token base64 and constant-time verify duplicated across three credential types #65

New issue

Closed

opened 2026-07-03 01:43:16 +00:00 by rosa · 1 comment

rosa commented

2026-07-03 01:43:16 +00:00

Owner

Found during a code review of the domain crate.

Location: crates/domain/src/models/access_token.rs:237, authorization_code.rs:315, user.rs:337 (parse/Display); access_token.rs:319, authorization_code.rs:427, user.rs:474 (ct_eq verify)
Type: Cleanup — duplication

Problem

The 32-byte base64url split-token encode/decode ([0..=15]/[16..=31]) parse and Display are copy-pasted verbatim across AccessToken, AuthorizationCode, and ConfirmationToken, and the constant-time subtle::ct_eq verify block is likewise triplicated. credential.rs documents itself as the place this logic is written once, yet only generate/parse/hash live there.

Cost

A change to the wire format or the comparison must be edited in three parse fns, three Display impls, and three verify paths, with copy-pasted error strings; a fourth credential family will copy it again.

Suggested fix

Add encode(&Identifier, &Verifier) -> String, decode(SecretString) -> Result<(Identifier, Verifier)>, and VerifierHash::matches(&VerifierHash) -> bool to credential.rs and have all three wrappers delegate.

Found during a code review of the `domain` crate. **Location:** `crates/domain/src/models/access_token.rs:237`, `authorization_code.rs:315`, `user.rs:337` (parse/Display); `access_token.rs:319`, `authorization_code.rs:427`, `user.rs:474` (ct_eq verify) **Type:** Cleanup — duplication ## Problem The 32-byte base64url split-token encode/decode ([0..=15]/[16..=31]) parse and Display are copy-pasted verbatim across AccessToken, AuthorizationCode, and ConfirmationToken, and the constant-time `subtle::ct_eq` verify block is likewise triplicated. `credential.rs` documents itself as the place this logic is written once, yet only generate/parse/hash live there. ## Cost A change to the wire format or the comparison must be edited in three parse fns, three Display impls, and three verify paths, with copy-pasted error strings; a fourth credential family will copy it again. ## Suggested fix Add `encode(&Identifier, &Verifier) -> String`, `decode(SecretString) -> Result<(Identifier, Verifier)>`, and `VerifierHash::matches(&VerifierHash) -> bool` to `credential.rs` and have all three wrappers delegate.

rosa added the

needs-triage

label

2026-07-03 01:43:58 +00:00

rosa commented

2026-07-04 02:43:59 +00:00

Author

Owner

Agent Brief

Category: enhancement
Summary: Move the split-token base64 wire format and constant-time verifier comparison into the shared credential module, and have all three credential families delegate

Current behavior:
The shared credential module (credential.rs in the domain crate's models) defines Identifier, Verifier, and VerifierHash and its module doc promises the common logic is "written once" there — but only generate, parse, and hash actually are. Three pieces are copy-pasted across the AccessToken, AuthorizationCode, and ConfirmationToken families instead:

Encoding (Display impls): concatenate identifier bytes + verifier bytes and base64-encode with the padded URL-safe alphabet. Verbatim triplicate.
Decoding (parse on the minted-credential types, taking a SecretString): base64-decode, ensure exactly 32 bytes, split into [0..=15] identifier and [16..=31] verifier. Near-verbatim triplicate with copy-pasted error strings differing only in the credential noun.
Verifier comparison (in AccessTokenRecord::verify, AuthorizationCodeRecord's redemption checks, and Confirmation's verify methods): a subtle::ct_eq of the stored VerifierHash against the presented verifier's hash. Verbatim triplicate; only the returned error variant differs.

Any change to the wire format or the comparison must be made in three parse functions, three Display impls, and three verify paths; a fourth credential family would copy all of it again.

Desired behavior:
The credential module is the single home for this logic, as its doc already claims:

encode(&Identifier, &Verifier) -> String — the split-token wire format (id bytes + verifier bytes, padded URL-safe base64).
decode(SecretString) -> Result<(Identifier, Verifier), anyhow::Error> — the inverse: decode, length-check, split.
VerifierHash::matches(&VerifierHash) -> bool — the constant-time comparison, keeping subtle::ct_eq inside the shared module.

The three families' Display, parse, and verify paths delegate to these. Per-family error variants (e.g. VerifierMismatch on each family's error enum) and per-family error context (which credential failed to decode) stay at the wrapper layer — the shared module stays credential-agnostic. Follow the glossary: these types are the minted form of a credential (CONTEXT.md, Credentials section).

Key interfaces:

The new functions live beside Identifier/Verifier/VerifierHash with the same pub(crate) visibility the module already uses.
Delegating call sites: AccessToken/AuthorizationCode/ConfirmationToken parse + Display, and the verifier checks in AccessTokenRecord::verify, AuthorizationCodeRecord's redemption, and Confirmation's verify methods.
The wire format itself is pinned by existing round-trip tests (token_roundtrips_through_base64, code_roundtrips_through_base64, confirmation_token_encode_decode_test) and the verifier-mismatch tests — they should pass unmodified.

Acceptance criteria:

encode, decode, and VerifierHash::matches exist in the shared credential module; the comparison inside matches is constant-time (subtle)
The three families' Display, parse, and verifier checks delegate — no base64 engine use or ct_eq call remains in the three credential families for the split-token format (the PKCE challenge comparison is separate and stays)
Existing round-trip and verifier-mismatch tests pass unmodified (wire format and comparison semantics unchanged)
No public API changes outside the domain crate
The ci mise task passes

Out of scope:

The PKCE CodeChallenge comparison in the authorization-code redemption — different shape (string compare of a base64-encoded hash), not part of the triplication
Changing the wire format, base64 alphabet, or padding in any way
Merging the per-family newtypes (AccessTokenId vs AuthorizationCodeId etc.) or their error enums — the type-safety split is deliberate
The URL-path-safe identifier encoding helpers (to_url_safe/from_url_safe) — different format (no-pad, identifier only), different purpose (routes)

## Agent Brief **Category:** enhancement **Summary:** Move the split-token base64 wire format and constant-time verifier comparison into the shared credential module, and have all three credential families delegate **Current behavior:** The shared credential module (`credential.rs` in the domain crate's models) defines `Identifier`, `Verifier`, and `VerifierHash` and its module doc promises the common logic is "written once" there — but only `generate`, `parse`, and `hash` actually are. Three pieces are copy-pasted across the `AccessToken`, `AuthorizationCode`, and `ConfirmationToken` families instead: 1. **Encoding** (`Display` impls): concatenate identifier bytes + verifier bytes and base64-encode with the padded URL-safe alphabet. Verbatim triplicate. 2. **Decoding** (`parse` on the minted-credential types, taking a `SecretString`): base64-decode, ensure exactly 32 bytes, split into `[0..=15]` identifier and `[16..=31]` verifier. Near-verbatim triplicate with copy-pasted error strings differing only in the credential noun. 3. **Verifier comparison** (in `AccessTokenRecord::verify`, `AuthorizationCodeRecord`'s redemption checks, and `Confirmation`'s verify methods): a `subtle::ct_eq` of the stored `VerifierHash` against the presented verifier's hash. Verbatim triplicate; only the returned error variant differs. Any change to the wire format or the comparison must be made in three parse functions, three `Display` impls, and three verify paths; a fourth credential family would copy all of it again. **Desired behavior:** The credential module is the single home for this logic, as its doc already claims: - `encode(&Identifier, &Verifier) -> String` — the split-token wire format (id bytes + verifier bytes, padded URL-safe base64). - `decode(SecretString) -> Result<(Identifier, Verifier), anyhow::Error>` — the inverse: decode, length-check, split. - `VerifierHash::matches(&VerifierHash) -> bool` — the constant-time comparison, keeping `subtle::ct_eq` inside the shared module. The three families' `Display`, `parse`, and verify paths delegate to these. Per-family error *variants* (e.g. `VerifierMismatch` on each family's error enum) and per-family error *context* (which credential failed to decode) stay at the wrapper layer — the shared module stays credential-agnostic. Follow the glossary: these types are the minted form of a credential (CONTEXT.md, Credentials section). **Key interfaces:** - The new functions live beside `Identifier`/`Verifier`/`VerifierHash` with the same `pub(crate)` visibility the module already uses. - Delegating call sites: `AccessToken`/`AuthorizationCode`/`ConfirmationToken` `parse` + `Display`, and the verifier checks in `AccessTokenRecord::verify`, `AuthorizationCodeRecord`'s redemption, and `Confirmation`'s verify methods. - The wire format itself is pinned by existing round-trip tests (`token_roundtrips_through_base64`, `code_roundtrips_through_base64`, `confirmation_token_encode_decode_test`) and the verifier-mismatch tests — they should pass unmodified. **Acceptance criteria:** - [ ] `encode`, `decode`, and `VerifierHash::matches` exist in the shared credential module; the comparison inside `matches` is constant-time (`subtle`) - [ ] The three families' `Display`, `parse`, and verifier checks delegate — no base64 engine use or `ct_eq` call remains in the three credential families for the split-token format (the PKCE challenge comparison is separate and stays) - [ ] Existing round-trip and verifier-mismatch tests pass unmodified (wire format and comparison semantics unchanged) - [ ] No public API changes outside the domain crate - [ ] The `ci` mise task passes **Out of scope:** - The PKCE `CodeChallenge` comparison in the authorization-code redemption — different shape (string compare of a base64-encoded hash), not part of the triplication - Changing the wire format, base64 alphabet, or padding in any way - Merging the per-family newtypes (`AccessTokenId` vs `AuthorizationCodeId` etc.) or their error enums — the type-safety split is deliberate - The URL-path-safe identifier encoding helpers (`to_url_safe`/`from_url_safe`) — different format (no-pad, identifier only), different purpose (routes)

rosa added

enhancement

ready-for-agent

and removed

needs-triage

labels

2026-07-04 02:43:59 +00:00

rosa referenced this issue from a commit

2026-07-04 03:02:05 +00:00

refactor: Write the split-token wire format and verifier compare once

rosa referenced this issue from a pull request that will close it,

2026-07-04 03:02:45 +00:00