docs: fill documentation gaps — passkeys, OIDC, rate-limiting, email-verification, magic-links, SMTP, crypto#156
Merged
Conversation
- passkeys.md: document 5-minute challenge expiry and credential counter
update behaviour on FinishAuthentication (non-fatal, used for clone
detection)
- oidc.md: warn that link-nonce state is in-memory and unsuitable for
multi-instance deployments without sticky sessions
- rate-limiting.md: warn that per-IP visitor state is in-memory and
enforcement does not span instances in multi-instance deployments
- email-verification.md: add note that SendVerification does NOT delete
the verification token when SendEmail fails (contrast with
PasswordResetHandler which does delete the token on failure)
- magic-links.md: document that auto-provisioned accounts use the email
address as the initial display name
- smtp.md: add Security and timeouts section documenting TLS 1.2 minimum,
10 s dial timeout, and 30 s session timeout
- crypto.md: note that SecretEncrypter.Encrypt returns ("", nil) for
empty input — no ciphertext is produced or stored
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
email-verification.md: SendEmail=nil returns HTTP 503 before any DB write; it does not create/store a token silently. oidc.md: In-memory nonce is consumed by the Link handler, so a cross-instance nonce miss fails the /oidc/link?nonce=… request with HTTP 401, not the OIDC callback with a redirect error.
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.
Summary
Automated documentation gap analysis found several places where code behaviour was not reflected in the docs. This PR fills those gaps across seven files.
Changes
docs/handler/passkeys.mdBeginRegistration/BeginAuthenticationstore a challenge that expires after 5 minutes. Added this to the registration/authentication flow description so integrators know the time budget for the client-side WebAuthn ceremony.FinishAuthenticationcallsPasskeyStore.UpdateCredentialDatato persist the updated WebAuthn signature counter after every successful login. Counter update failures are logged as warnings and do not fail authentication. The counter is the primary mechanism for detecting cloned authenticators.docs/handler/oidc.mdOIDCHandlerholds pending link nonces in a process-localmap[string]linkNonce. In multi-instance deployments the nonce created on one pod may not exist on the pod handling the OIDC callback, silently breaking the account-linking flow. Added a!!! warningcallout recommending sticky sessions or a shared store.docs/auth/rate-limiting.mdRateLimitertracks per-IP token buckets in an in-memory map — enforcement does not span instances. Added a!!! warningcallout advising operators to supplement with an external rate-limiter (e.g. Redis) for multi-instance deployments.docs/handler/email-verification.mdSendEmailfailure: WhenSendEmailreturns an error,SendVerificationlogs the failure and returns HTTP 200 — the stored token is not deleted. This contrasts withPasswordResetHandler, which does delete the reset token on email failure. Added a!!! notecallout to document this behaviour and the intentional difference.docs/handler/magic-links.mdAuthHandler.UpdateProfileso users know how to change it.docs/smtp.mdtlsandstarttlsmodes.docs/auth/crypto.mdSecretEncrypter.Encrypt("")returns("", nil)— no ciphertext is produced. Added a comment in the code block so callers are not surprised when an empty field round-trips as an empty string.How gaps were identified
A
exploresub-agent compared every exported Go symbol inauth/andhandler/against the corresponding Markdown documentation, then cross-checked behaviour notes in code comments against what was written in the docs.Testing
Documentation-only change. No code was modified. Existing tests are unaffected.
Greptile Summary
This PR adds documentation for seven previously undocumented or under-documented behaviours across the passkeys, OIDC, rate-limiting, email-verification, magic-links, SMTP, and crypto modules. The new content is accurate: the 5-minute challenge expiry, credential-counter semantics, in-memory rate-limiter and nonce warnings, token-retention contrast between
EmailVerificationHandlerandPasswordResetHandler, auto-provisioned display name, and the SMTP TLS/timeout section all match the source code.Confidence Score: 4/5
Documentation-only PR; most new content is accurate, but two previously-flagged inaccuracies (oidc.md failure mode, email-verification.md nil-sender behaviour) remain unresolved in this HEAD.
All newly-added documentation was verified against source and is accurate. Score is held at 4 rather than 5 because two factual inaccuracies noted in prior review threads (oidc.md still describes the wrong failure endpoint; email-verification.md line 16 still mis-states nil-SendEmail behaviour) have not been corrected in this HEAD despite the author's commit-message claim of a fix.
docs/handler/oidc.md and docs/handler/email-verification.md — both still contain inaccuracies tracked in prior review threads.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[smtp.Send called] --> B[Dial TCP\n10 s timeout] B -->|tls mode| C[TLS handshake\nmin TLS 1.2] B -->|starttls / none| D[Plain TCP conn established] C --> E[Set 30 s session deadline\nmin ctx deadline, now+30s] D --> E E -->|starttls| F[STARTTLS upgrade\nmin TLS 1.2] E -->|none| G[Plain SMTP session] F --> H[AUTH → MAIL FROM → RCPT TO → DATA] G --> H H --> I[QUIT]Reviews (2): Last reviewed commit: "docs: fix nil-sender and nonce-consumpti..." | Re-trigger Greptile