docs: fix nil-sender HTTP 503 documentation for EmailVerification and PasswordReset handlers#159
Merged
Conversation
…dlers Both README.md and docs were incorrectly stating that when SendEmail (EmailVerificationHandler) or SendResetEmail (PasswordResetHandler) is nil, tokens are still created. In reality both handlers return HTTP 503 immediately before any database write. Changes: - docs/handler/email-verification.md: add missing 503 row to HTTP status codes table for SendVerification when SendEmail is nil - docs/handler/password-reset.md: add warning admonition about nil SendResetEmail and add missing 503 row to HTTP status codes table - README.md: correct the 'nil sender' prose for both handlers and add 503 rows to their respective error-response tables; update the SendVerification blanket-200 note to exclude the 503 case Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR corrects documentation for EmailVerificationHandler and PasswordResetHandler to match the actual runtime behavior when their email-sender function fields are nil (immediate HTTP 503, no DB writes), and updates status-code tables accordingly.
Changes:
- Add missing
503 Service Unavailablerows to handler status-code tables where sender funcs can benil. - Update README prose and error-response tables to describe the 503 misconfiguration behavior (and adjust the blanket “always 200” note to exclude 503).
- Add a warning admonition to
PasswordResetHandlerdocs recommending a no-op sender for tests.
Show a summary per file
| File | Description |
|---|---|
| docs/handler/email-verification.md | Documents 503 when SendEmail is nil via status-code table update. |
| docs/handler/password-reset.md | Adds warning admonition for nil SendResetEmail and documents 503 in status-code table. |
| README.md | Fixes nil-sender behavior descriptions and adds 503 rows to error-response tables for both handlers. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
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.
What
Corrects factually wrong documentation about the behavior of
EmailVerificationHandlerandPasswordResetHandlerwhen their sender function fields arenil, and adds missing HTTP 503 status rows to all relevant status-code tables.Why
Both
README.mdand the handler reference docs incorrectly stated that whenSendEmail/SendResetEmailisnil, tokens are still created and stored (just not delivered). This was described as "useful in tests". The actual implementation returns HTTP 503 immediately, before any database write, in both handlers — the same pattern already correctly documented forMagicLinkHandler.This inaccuracy could lead integrators to:
Changes
docs/handler/email-verification.mdSendVerification | 503 Service Unavailable | SendEmail is nilrow to HTTP status codes tabledocs/handler/password-reset.md!!! warningadmonition about nilSendResetEmail(matching the style ofemail-verification.mdandmagic-links.md); add missingRequestReset | 503 Service Unavailablerow to HTTP status codes tableREADME.md503 Service Unavailablerows to each handler's error-response table; update theSendVerificationblanket-200 note to exclude the 503 caseVerification
Confirmed against source:
handler/email_verification.go:66-69—if h.SendEmail == nil { writeError(... StatusServiceUnavailable ...)handler/password_reset.go:73-76—if h.SendResetEmail == nil { writeError(... StatusServiceUnavailable ...)Both checks occur after request-body validation (400) but before any store call, consistent with the updated documentation.
Warning
The following domain was blocked by the firewall during workflow execution:
proxy.golang.orgTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
Greptile Summary
This docs-only PR corrects factually wrong nil-sender behaviour for
EmailVerificationHandlerandPasswordResetHandler— both return HTTP 503 immediately (before any DB write) when the sender function isnil, not the previously documented no-op token-creation path. TheREADME.md,email-verification.md, andpassword-reset.mdall receive 503 rows in their status tables and updated prose, verified against the source implementations.!!! infocallout at the bottom ofdocs/handler/email-verification.md(line 67) still says "Only the missing-emailvalidation check surfaces as a non-200 error," which now contradicts the 503 row added four lines above — this was flagged in a prior review and remains unaddressed.Confidence Score: 4/5
Safe to merge after fixing the one remaining stale callout in email-verification.md.
One P1 finding: the
!!! infocallout indocs/handler/email-verification.mddirectly contradicts the new 503 row added in this same PR. All other changes are accurate and consistent with the source code.docs/handler/email-verification.md — the closing info callout needs updating to include the 503 case.
Important Files Changed
!!! infocallout (line 67) still incorrectly states that only the 400 case is a non-200 error.!!! warningfor nilSendResetEmail, qualifies the enumeration-prevention callout to exclude 400/429/503, and adds the 503 row to the status table — all consistent with the source code.SendVerification— all verified against the source code.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[POST /verify-email/send or POST /password-reset/request] --> B{Rate limit exceeded?\nPasswordReset only} B -- Yes --> R429[429 Too Many Requests] B -- No --> C{Valid JSON &\nemail present?} C -- No --> R400[400 Bad Request] C -- Yes --> D{SendEmail /\nSendResetEmail == nil?} D -- Yes --> R503[503 Service Unavailable\nbefore any DB write] D -- No --> E[Store lookup & token creation] E --> F{Store error?} F -- Yes --> R500[500 Internal Server Error] F -- No --> G[Send email] G --> R200[200 OK\nalways — prevents enumeration]Comments Outside Diff (4)
docs/handler/email-verification.md, line 39 (link)This sentence still says "For all valid requests it returns HTTP 200 regardless of whether the address is registered," but a request with a valid
emailfield whenSendEmailisnilis structurally valid yet now documented (and actually) returns HTTP 503. The same inconsistency appears on line 59 (Always) and line 67 (Only the missing-email validation check surfaces as a non-200 error). All three need updating to match the 503 row that was just added to the status table.Prompt To Fix With AI
docs/handler/email-verification.md, line 67 (link)"Only the missing-
emailvalidation check surfaces as a non-200 error" is now wrong — the nil-SendEmail503 is also a non-200 error. This callout should at minimum say "the 400 and 503 cases" to stay consistent with the rest of the updated docs.Prompt To Fix With AI
docs/handler/password-reset.md, line 46 (link)The info callout says "
RequestResetalways returns HTTP 200 … regardless of whether the email is registered," but that is no longer true whenSendResetEmailisnil. The same "always" qualifier appears in the status table row on line 62. Both need qualifying — theREADME.mdequivalent was already corrected in this PR.Prompt To Fix With AI
docs/handler/email-verification.md, line 67 (link)The
!!! infocallout says "Only the missing-emailvalidation check surfaces as a non-200 error," butSendEmail == nilnow also surfaces as a non-200 error (503). This directly contradicts the new 503 row added to the status table four lines above. TheREADME.mdand the updated prose on line 39 both already use "the400and503cases."Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "docs: correct 200 OK conditions to exclu..." | Re-trigger Greptile