Skip to content

Add startup validation for TOTPHandler UsedCodes misconfiguration#259

Merged
veverkap merged 2 commits into
mainfrom
copilot/add-validate-method-to-totp-handler
May 11, 2026
Merged

Add startup validation for TOTPHandler UsedCodes misconfiguration#259
veverkap merged 2 commits into
mainfrom
copilot/add-validate-method-to-totp-handler

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 11, 2026

Bug Fix

TOTP lacked the same startup validation guard used by other handlers. If UsedCodes was nil, replay-protection paths (isReplay, recordUsed) could panic at runtime instead of failing fast at boot.

  • What was the bug?

    TOTPHandler had no Validate() method, so a missing UsedCodes dependency was only discovered when handling enroll/verify requests.

  • How did you fix it?

    Added TOTPHandler.Validate() to enforce non-nil UsedCodes with a clear startup error:

    • TOTPHandler misconfigured: UsedCodes is required
    • Updated TOTP handler docs/examples to call h.Validate() at startup, matching other handlers’ configuration pattern.
  • Testing

    Added focused unit coverage for the new validation contract:

    • returns error when UsedCodes is nil
    • returns nil when UsedCodes is configured
func (h *TOTPHandler) Validate() error {
	if h.UsedCodes == nil {
		return errors.New("TOTPHandler misconfigured: UsedCodes is required")
	}
	return nil
}

Greptile Summary

This PR adds TOTPHandler.Validate() to catch missing dependencies (TOTP, Users, UsedCodes) at server startup rather than panicking on the first request. Documentation and README examples are updated to call h.Validate() after construction, matching the pattern already used by other handlers in the codebase.

  • handler/totp.go: New Validate() method checks all three required fields and returns a descriptive error for each, consistent with the existing nil-guard panics in isReplay and recordUsed.
  • handler/totp_test.go: Adds two tests covering the UsedCodes == nil error case and the fully-configured happy path, but leaves the TOTP == nil and Users == nil branches untested.
  • README.md / docs/handler/totp.md: Updated quickstart snippets to include the startup Validate() call.

Confidence Score: 5/5

Safe to merge — the new Validate() method correctly guards all three required fields and the change is additive with no effect on existing request-handling paths.

The implementation is correct and consistent with the existing nil-panic guards already in place. The only gap is that test cases for the nil-TOTP and nil-Users branches were not added alongside the nil-UsedCodes test, leaving those two code paths without direct coverage.

handler/totp_test.go — the two new Validate() error branches (nil TOTP, nil Users) have no corresponding test cases.

Important Files Changed

Filename Overview
handler/totp.go Adds Validate() checking all three required fields (TOTP, Users, UsedCodes); implementation is correct and consistent with the panic guards already present in isReplay/recordUsed.
handler/totp_test.go Adds two validation tests but only covers the UsedCodes == nil and happy-path cases; nil-TOTP and nil-Users guards introduced in this PR have no corresponding test cases.
README.md Adds h.Validate() startup call to the TOTP setup example, matching the pattern used by other handlers.
docs/handler/totp.md Adds h.Validate() call to the docs quickstart snippet; no issues.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Server startup] --> B["h := &TOTPHandler{...}"]
    B --> C["h.Validate()"]
    C --> D{TOTP == nil?}
    D -- yes --> E["return error: TOTP is required"]
    D -- no --> F{Users == nil?}
    F -- yes --> G["return error: Users is required"]
    F -- no --> H{UsedCodes == nil?}
    H -- yes --> I["return error: UsedCodes is required"]
    H -- no --> J[return nil — handler is ready]
    J --> K[Register HTTP routes]
Loading

Fix All in Claude Code

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
handler/totp_test.go:67-78
**Missing test cases for nil TOTP and nil Users guards**

`Validate()` now checks three fields (`TOTP`, `Users`, `UsedCodes`), but the new tests only exercise the `UsedCodes == nil` path and the all-configured happy path. The two checks added for `h.TOTP` and `h.Users` are untested — a future refactor that accidentally removes or short-circuits those branches would go undetected. Adding `TestTOTPValidate_missingTOTP_returnsError` and `TestTOTPValidate_missingUsers_returnsError` (same pattern as the existing nil test) would give full coverage for all three guards introduced in this PR.

Reviews (2): Last reviewed commit: "fix(totp): validate TOTP and Users depen..." | Re-trigger Greptile

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a startup-time validation hook to TOTPHandler so deployments fail fast when the replay-protection cache (UsedCodes) is not configured, avoiding runtime panics on enroll/verify paths.

Changes:

  • Added (*TOTPHandler).Validate() that errors when UsedCodes is nil.
  • Added unit tests covering the new validation contract.
  • Updated README + TOTP handler docs to show calling Validate() at startup.
Show a summary per file
File Description
README.md Documents calling TOTPHandler.Validate() during startup to catch misconfiguration early.
handler/totp.go Introduces Validate() to enforce non-nil UsedCodes.
handler/totp_test.go Adds tests asserting Validate() fails when UsedCodes is nil and succeeds when configured.
docs/handler/totp.md Updates TOTP handler documentation example to include startup validation.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 4/4 changed files
  • Comments generated: 0

Comment thread handler/totp.go
Copy link
Copy Markdown
Contributor

@veverkap veverkap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: added nil checks for h.TOTP and h.Users in Validate() so all three required dependencies are caught at startup before any request handler can panic.

@veverkap veverkap merged commit 59dea99 into main May 11, 2026
14 checks passed
@veverkap veverkap deleted the copilot/add-validate-method-to-totp-handler branch May 11, 2026 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants