refactor(proof/passkey): readability + maintainability sweep#69
Merged
Conversation
…idations service.go was 559 LOC mixing ceremony orchestration, validation predicates, and scaffolding. Split it per ceremony, mirroring the exchange/ and management/ shape from PRs #63 and #65: - service.go (~170 LOC) — Service, NewService, newService, validateConfig, scaffolding helpers (passkeyAuthenticatorSelection, sessionRequiringUserVerification, enforcedTimeout, registrationTimeout, loginTimeout, identityForCredential, credentialIDString). - registration.go — BeginRegistration, FinishRegistration, registrationUser, finishRegistrationUser, credentialExclusions. - login.go — BeginLogin, FinishLogin, validateLinkedPrincipal, discoverableUserHandler. - validations.go — the four pure validate* predicates that confirm a store-returned RegistrationResult matches the verified ceremony state. No logic changes; tests unmodified at this commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add concise godocs to every previously-undocumented private item across clone.go, errors.go, login.go, registration.go, service.go, validations.go, and webauthn_user.go. Several name the security invariant the helper preserves — defensive-copy header on clone.go, the 8-point binding invariant on credentialsForUser, the upstream seam rationale on relyingParty, ErrCloneWarning's chained semantics on cloneWarning. Annotate the two high-blast-radius branches in the login path: - The clone-warning rejection branch persists the updated counter state *before* refusing the login, defending against replay against the pre-clone counter. - The validateLinkedPrincipal call defends against a cross-principal collision where a credential resolves to a different principal than the discoverable handler returned. Annotate sessionRequiringUserVerification with its downgrade-defense rationale: even if a tampered SessionData claims a weaker flag, the finish step forces Required before the verifying parse. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add godocs to every private item in the session subpackage: the sessionKind tag, memorySession, the clone family, defaultMemoryOptions, and the memory store's put/take/pruneExpiredLocked helpers. Annotate the wrong-flow rejection in MemoryStore.take: the session is deleted before the kind and expiry checks so a session presented for the wrong ceremony (registration session at login finish, or vice versa) is consumed rather than left available for replay, and the kind-mismatch path returns ErrNotFound rather than a more specific error to avoid leaking whether the ID belonged to the other flow. The session/clone.go header now documents the defensive-copy security rationale: a caller could otherwise mutate the SessionData byte slices, credential parameter list, or extensions map post-Put, silently corrupting the session a Take would return. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lift the 1115-LOC service_test.go into four files mirroring the production layout: service_test.go (config validation + ceremony timeout enforcement), registration_test.go (10 Begin/Finish registration tests), login_test.go (11 Begin/Finish login tests), helpers_test.go (consts + newTestService + testConfig/User/Credential fixtures + fakeRelyingParty + fakeStore + handleKey/identityKey). No test logic changes; each function is lifted verbatim. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ryStore Make MemoryStore's implementation of session.Store a compile-time check so an accidental method-signature drift breaks the build rather than a downstream caller. passkey.Store assertions already live in store/memory/passkey_test.go and store/postgres/passkey_test.go. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
golangci-lint fmt flagged a leading tab in the multi-line defensive-copy header that survived my godoc commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
Ninth slice (and final
proof/slice) of the multi-session readability sweep (afteraccess/#61,authz/#62,exchange/#63,http/#64,management/#65,onboarding/#66,proof/apikey/#67,proof/oidc/#68). Same 10-criteria bar.proof/passkey/is the highest-blast-radius package in the tree: WebAuthn/FIDO2 ceremony orchestration with credential binding, clone-warning detection, and session-storage replay protection. This is also the largest single-subpackage sweep so far (2733 LOC combined acrosspasskey/andpasskey/session/).Per-ceremony file split
service.go(559 LOC) split into:service.go(~170 LOC) — scaffolding only:Service,NewService,newService,validateConfig, and the shared helpers (passkeyAuthenticatorSelection,sessionRequiringUserVerification,enforcedTimeout,registrationTimeout,loginTimeout,identityForCredential,credentialIDString).registration.go—BeginRegistration,FinishRegistrationand their private helpers (registrationUser,finishRegistrationUser,credentialExclusions).login.go—BeginLogin,FinishLoginand their private helpers (validateLinkedPrincipal,discoverableUserHandler).validations.go— the four purevalidate*predicates that confirm a Store-returned RegistrationResult matches the verified ceremony state.service_test.go(1115 LOC) split correspondingly:service_test.go(cross-cutting): config validation + ceremony timeout enforcement.registration_test.go— 10 Begin/Finish registration tests.login_test.go— 11 Begin/Finish login tests.helpers_test.go— fixtures (testConfig,testUser,testCredential), thefakeRelyingParty, and thefakeStore.Security inline comments
The package is the highest blast radius in the proof/ tree; the comments are deliberately verbose:
service.go::sessionRequiringUserVerification— downgrade-attack defense rationale (Finish forces Required even if SessionData was tampered after Begin).login.goclone-warning rejection — persists the updated counter before refusing the login, defending against replay against the pre-clone counter.login.govalidateLinkedPrincipalcall — cross-principal collision defense (the discoverable handler's user must match the link the Store returns).webauthn_user.go::credentialsForUser— the 8-point binding invariant (RPID, PrincipalID, UserHandle, CredentialID, WebAuthn.ID coherence) that every credential must satisfy before reaching the upstream library.clone.goheader — defensive-copy security rationale: the webauthn.Credential carries mutable byte slices and maps that callers could otherwise overwrite, corrupting stored credentials or skewing the next verification.session/memory.go::take— wrong-flow rejection (session deleted before kind/expiry checks) and the kind-mismatch returningErrNotFoundrather than a more specific error to avoid leaking which flow the ID belonged to.session/clone.goheader — defensive-copy rationale for SessionData (UserID, AllowedCredentialIDs, CredParams, Extensions).session/memory.go::sessionKindandmemorySessiongodocs name the cross-flow-replay defense the kind tag enforces.Other changes
clone.go,errors.go,login.go,registration.go,service.go,validations.go,webauthn_user.go,session/clone.go,session/memory.go,session/options.go. Several name the security invariant the helper preserves.var _ Store = (*MemoryStore)(nil)assertion added tosession/memory.go.passkey.Storeassertions already lived instore/memory/passkey_test.goandstore/postgres/passkey_test.go.Explicitly NOT changed
passkey/sessionis a peer concern (session-storage adapter) and the boundary is correct.passkey.Storeandsession.Storeare package-local with multi-implementation; they stay hand-rolled per convention.rpID,userHandle,credentialID,ceremony,assertion,attestationwere already consistent.Commits
refactor(proof/passkey): split service.go into registration/login/validationsrefactor(proof/passkey): godocs + inline security commentsrefactor(proof/passkey/session): godocs + inline security commentstest(proof/passkey): split service_test.go per ceremonyrefactor(proof/passkey/session): compile-time port assertion for MemoryStorechore(proof/passkey): drop stray tab in clone.go header commentTest plan
moon run root:check --summary minimal— format, lint, build, unit, Testcontainers integration all green.🤖 Generated with Claude Code