Skip to content

fix: refuse zero-value totp, blank password characters and non-canonical email local parts - #479

Merged
Jaro-c merged 4 commits into
developfrom
fix/input-review-round-2
Sep 26, 2026
Merged

Jaro-c merged 4 commits into
developfrom
fix/input-review-round-2

Conversation

@Jaro-c

@Jaro-c Jaro-c commented Sep 26, 2026

Copy link
Copy Markdown
Member

Closes #478 in four commits, one per package, each building and passing on its own.

password

  • isPrintable also refuses U+2800 BRAILLE PATTERN BLANK and the code points in Unicode's Other_Default_Ignorable_Code_Point (the Hangul fillers, U+034F and the like), which unicode.IsPrint admits although they render as nothing. Variation selectors stay accepted: they modify a visible base. The package doc and docs/password.md say what is refused. A new password holding one of these is refused now; existing hashes verify as before, since Verify applies no policy.
  • Tests pin what the review found unpinned: Hash normalises to NFC (NFD register, NFD and NFC sign-in); all four *bool policy snapshots, by flipping the caller's variable after New; the Memory and Iterations ceilings at New, with the value at the ceiling accepted; and a stored hash with a bare label ($v$, $m$, ...) refused with ErrInvalidHash rather than panicking.

totp

  • Every method refuses on a zero value or a nil pointer: Enroll, VerifyStep and Verify with ErrNotInitialised, VerifyRecoveryCode with (0, false), and HashRecoveryCode now returns (string, error) so it can report it, the change fix(auth): refuse the inputs that produced output under an empty key #425 made to apikey.Hash for the same reason. The sentinel is in docs/errors.md.
  • FuzzVerify has an oracle: RFC 4226 written apart from the package (HMAC-SHA1, dynamic truncation, six digits) decides which codes the fixed clock's window accepts, and VerifyStep and Verify (with an in-memory recorder) must agree exactly. Seeds cover every step of the window and one outside it on each side.

email

  • The local part has one canonical spelling: NFC, then lowercase. Control (Cc, C1 included), format, line and paragraph separator and default-ignorable characters in it are refused, and so is a non-ASCII letter whose lowercase is ASCII (U+0130 to i). U+212A KELVIN SIGN is canonically equivalent to K and becomes k through NFC, which the test states. josé@ now stores as josé@, so a row stored from decomposed input before this change no longer matches; I know of none.
  • evictExpired, which nothing in production called, is gone; the eviction inside store is tested with a full cache of expired entries, and an empty MX answer with no error is pinned as ErrDomainNoMX. The maxCacheSize comment no longer describes a background goroutine.

username: the fuzz target asserts the [a-z0-9_-] set, the length bounds and the reserved list on every accepted value, with seeds that put a disallowed byte between allowed ones, since the start and end rules refuse the old seeds before isAllowed sees them.

Sabotage, each edit confirmed to change the file:

Sabotage Result
drop NFC in Hash TestHash_normalisesTheInputItHashes fails
drop any one of the three policy snapshots its subtest fails
drop the memory or the iterations ceiling refusesWorkFactorsPastTheCeilings fails
drop the label prefix check the bare-label test fails, with the old panic
drop the default-ignorable or the braille refusal the blank-rendering test fails
drop NFC / the control refusal / the ASCII-fold refusal in the local part the matching local-part test fails
drop the eviction / the empty-answer branch their tests fail
drop the Enroll, HashRecoveryCode or VerifyRecoveryCode guard the zero-value test fails
make stepMatches accept every code FuzzVerify fails on its seeds
make isAllowed accept every byte FuzzValidateAndNormalize fails on the new seeds

gofmt, go vet ./..., go test -race -count=1 ./..., gosec and the nine example builds are clean. Coverage: password 98.8%, totp 95.4%, email 95.9%, username 94.8%.

Signed-off-by: Jaro-c 75870284+Jaro-c@users.noreply.github.com

…olicy snapshots, the ceilings and the label parser

Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…get an RFC 4226 oracle

Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…the empty MX answer

Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
@Jaro-c Jaro-c added type:bug A defect to fix type:test Tests type:security Security-relevant change prio:P2 Medium priority status:review In review effort:M Medium area:password Subsystem: password area:email Subsystem: email area:username Subsystem: username area:totp Area: totp labels Sep 26, 2026
@Jaro-c
Jaro-c merged commit aed321d into develop Sep 26, 2026
19 checks passed
@Jaro-c
Jaro-c deleted the fix/input-review-round-2 branch September 26, 2026 00:58
@Jaro-c Jaro-c added the breaking Backwards-incompatible change label Sep 27, 2026
@Jaro-c Jaro-c mentioned this pull request Sep 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:email Subsystem: email area:password Subsystem: password area:totp Area: totp area:username Subsystem: username breaking Backwards-incompatible change effort:M Medium prio:P2 Medium priority status:review In review type:bug A defect to fix type:security Security-relevant change type:test Tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant