Feat/anomaly detection - #13
Merged
Merged
Conversation
Refactor roadmap section for clarity and consistency.
Added permissions for write access to contents in the release workflow.
Covers: a policy violation rejecting SignUp/ChangePassword with every broken rule reported together, a satisfying password succeeding, the breach checker never being called when policy already rejected the password, and current-password verification running before the new password's policy check on ChangePassword.
That case needed security.PasswordPolicy, which didn't exist yet on the branch this smoke test was first written on — deferred there, added back now that password policy exists.
Runnable end-to-end check with no database dependency: go run ./cmd/smoketest/password-policy. Walks the default policy applying automatically when Config.PasswordPolicy is left unset, a short password rejected, a 73-byte password rejected (bcrypt's real limit is 72), a custom stricter policy reporting multiple violations together, and a password satisfying that custom policy succeeding.
applyDefaults detected "unset" via PasswordPolicy.MaxLength == 0 alone. A policy that set MinLength/character requirements but left MaxLength untouched — exactly what a caller would naturally write — looked unset and got silently overwritten by DefaultPasswordPolicy, which has none of those requirements. Comparing the whole struct against its zero value instead means only a truly untouched Config.PasswordPolicy gets defaulted; setting even one field counts as a real custom policy and is used as-is.
Feature/2fa totp
Feature/webauthn passkeys
Feature/magic link
…overy-codes Fix/oauth second factor and recovery codes
Feat/breached password check
Feat/password policy
Swapping the real crypto/rand.Reader package variable to simulate an entropy-source failure isn't portable: on at least one real platform (reported: Termux/Android), a failed read through that actual global triggers the Go runtime's own unrecoverable fatal-error path instead of returning a normal error — crashing the entire test binary rather than failing one test. CryptoRandTokenGenerator now holds its own randReader field (defaults to crypto/rand.Reader, set in the constructor), and the regression test injects a fake failing reader directly into a same-package instance instead of mutating any global state. Same coverage, no process-crashing side effect.
TestGoWebAuthnProvider_LoginRoundTrip asserted the returned credential's SignCount must be nonzero after a real login. virtualwebauthn's simulated credential starts at counter 0 and never auto-increments on its own — and a counter of 0 is itself a legitimate, spec-allowed value (many real platform authenticators never track one at all), so the assertion was simply testing a false assumption, not a real property of the code under test. Now manually sets the simulated credential's counter to a known nonzero value before the login call and asserts FinishLogin correctly surfaces that exact value — actually exercising the pass-through path this test cares about (persisting whatever the authenticator reports, which is what makes cloned-authenticator detection possible later) instead of an assumption that doesn't hold.
Missed when SignUp/ChangePassword's signature changed on the stacked feat/password-policy branch — this file was written earlier on feat/breached-password-check and never revisited, so it built fine on that branch alone but failed to compile once merged after the password-policy signature change landed on top. Same class of miss as an earlier one on login_second_factor_test.go — a full grep across every call site for a changed signature, not just the files touched in that commit, is the actual fix to the process here.
Feat/password policy2
Evaluate is pure and store-free so thresholds stay testable without a backend. AnomalyStore is its own interface, not more AuditStore queries: every read is a windowed aggregate that needs its own indexes, and the in-memory rate limiter's multi-instance gap rules it out too.
login_attempts.user_id is nullable with ON DELETE SET NULL, matching audit_events: an unknown-email failure still carries real evidence about the IP, which is what per-IP velocity counts. All three reads are backed by partial indexes on (key, created_at) filtered by outcome.
Detection runs in completePrimaryAuth, the one tail password, magic-link and OAuth all reach, so all three are covered once. Report-only and nil-safe: no Anomalies store means no behaviour change. Failure paths record attempts so velocity counts have input.
Thresholds tested with no store at all; the detector tested through real Login/LoginWithOAuth/CompleteMagicLink so path coverage is proven rather than assumed. Negative cases carry the design: first login is clean, a familiar login stays quiet, and a broken store never blocks.
54 checks over six scenarios, all in memory: every signal, the detection-off path, and the negative cases that matter most — a flagged login still issues tokens and keeps its sessions.
Item 8 removed from NEXT.md and its remaining items renumbered, per that file's own convention. Item 9's spec updated now that the shared per-IP failure data it was written to maybe-build already exists.
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
2 participants
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.
No description provided.