Skip to content

feat(passkeys): add CAPTCHA to options endpoint for authentication#2416

Merged
fadymak merged 5 commits intomasterfrom
fm/auth-1111-passkeys-captcha-on-auth
Mar 27, 2026
Merged

feat(passkeys): add CAPTCHA to options endpoint for authentication#2416
fadymak merged 5 commits intomasterfrom
fm/auth-1111-passkeys-captcha-on-auth

Conversation

@fadymak
Copy link
Copy Markdown
Contributor

@fadymak fadymak commented Mar 10, 2026

  • Adds CAPTCHA to the /passkeys/authentication/options endpoint.
  • Refactors the CAPTCHA implementation to use DI for easier mocking in tests while keeping a small subset of live tests against the provider APIs with the test tokens

@fadymak fadymak requested a review from a team as a code owner March 10, 2026 08:57
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 10, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (1)
  • develop

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 5701ab00-5f62-404c-802a-e7c450652e2f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@fadymak fadymak force-pushed the fm/auth-1102-passkeys-management-endpoints branch from 492898e to bff8a56 Compare March 10, 2026 12:13
@fadymak fadymak changed the base branch from fm/auth-1102-passkeys-management-endpoints to master March 10, 2026 17:48
@fadymak fadymak changed the base branch from master to fm/auth-1102-passkeys-management-endpoints March 10, 2026 17:50
@fadymak fadymak force-pushed the fm/auth-1111-passkeys-captcha-on-auth branch from 4cc4c2c to 83d2b89 Compare March 10, 2026 17:56
Base automatically changed from fm/auth-1102-passkeys-management-endpoints to master March 11, 2026 13:15
@fadymak fadymak force-pushed the fm/auth-1111-passkeys-captcha-on-auth branch from 83d2b89 to 8bd82c2 Compare March 11, 2026 13:30
@fadymak fadymak force-pushed the fm/auth-1111-passkeys-captcha-on-auth branch from ccb8e3d to b1941a0 Compare March 13, 2026 10:07
Copy link
Copy Markdown
Contributor

@cstockton cstockton left a comment

Choose a reason for hiding this comment

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

Looks good, could push through as is but having the ctx would be ideal.

Copilot AI review requested due to automatic review settings March 27, 2026 08:09
Copy link
Copy Markdown

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

Adds CAPTCHA enforcement to the passkeys authentication options flow and refactors CAPTCHA verification to be dependency-injected for testability.

Changes:

  • Introduces a CaptchaVerifier interface + HTTPCaptchaVerifier implementation with configurable timeout.
  • Injects/verifies CAPTCHA in middleware for POST /passkeys/authentication/options, with mocks used in API tests.
  • Adds provider “live” verification tests for hCaptcha and Turnstile.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
internal/security/captcha.go Refactors CAPTCHA verification into an interface-based HTTP verifier with context support and timeout.
internal/security/captcha_test.go Adds live provider tests for hCaptcha/Turnstile using test tokens/secrets.
internal/conf/configuration.go Extends CAPTCHA config with a Timeout duration field.
internal/api/options.go Adds WithCaptchaVerifier(...) option for DI.
internal/api/api.go Stores/verifies default CAPTCHA verifier on API init; wires verifyCaptcha into passkey auth options route.
internal/api/middleware.go Moves request parsing for captcha token into API middleware and uses injected verifier.
internal/api/helpers.go Updates request param type union to use the new local captchaRequest type.
internal/api/captcha_mock_test.go Introduces a mock verifier for tests.
internal/api/middleware_test.go Switches middleware captcha tests to use the mock verifier instead of live providers.
internal/api/passkey_test.go Injects mock captcha verifier into passkey test suite and resets its state per test.
internal/api/passkey_authentication_test.go Adds passkey auth options tests covering captcha required/valid/invalid/disabled scenarios.
Comments suppressed due to low confidence (2)

internal/api/middleware.go:248

  • a.captchaVerifier.Verify(...) can legally return (nil, nil) (e.g., a buggy/mock implementation). In that case verificationResult.Success will panic. Consider guarding against a nil verificationResult and treating it as an internal error (or a failed verification) to keep the middleware robust.
	verificationResult, err := a.captchaVerifier.Verify(
		ctx,
		token,
		utilities.GetIPAddress(req),
	)
	if err != nil {
		return nil, apierrors.NewInternalServerError("captcha verification process failed").WithInternalError(err)
	}

	if !verificationResult.Success {
		return nil, apierrors.NewBadRequestError(apierrors.ErrorCodeCaptchaFailed, "captcha protection: request disallowed (%s)", strings.Join(verificationResult.ErrorCodes, ", "))
	}

internal/conf/configuration.go:733

  • CaptchaConfiguration.Timeout is user-configurable but Validate() doesn’t sanity-check it. A negative duration (or extremely low timeout) could cause immediate failures or surprising behavior. Consider validating Timeout >= 0 (and possibly enforcing a sensible minimum) when CAPTCHA is enabled.
type CaptchaConfiguration struct {
	Enabled  bool          `json:"enabled" default:"false"`
	Provider string        `json:"provider" default:"hcaptcha"`
	Secret   string        `json:"provider_secret"`
	Timeout  time.Duration `json:"timeout" split_words:"true" default:"10s"`
}

func (c *CaptchaConfiguration) Validate() error {
	if !c.Enabled {
		return nil
	}

	if c.Provider != "hcaptcha" && c.Provider != "turnstile" {
		return fmt.Errorf("unsupported captcha provider: %s", c.Provider)
	}

	c.Secret = strings.TrimSpace(c.Secret)

	if c.Secret == "" {
		return errors.New("captcha provider secret is empty")
	}

	return nil

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

@fadymak fadymak merged commit c7b58be into master Mar 27, 2026
4 checks passed
@fadymak fadymak deleted the fm/auth-1111-passkeys-captcha-on-auth branch March 27, 2026 08:24
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