-
-
Notifications
You must be signed in to change notification settings - Fork 288
feat: add CAPTCHA protection to subscribe form (#729) #803
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
otherwiseGG
wants to merge
21
commits into
rajnandan1:main
Choose a base branch
from
otherwiseGG:729-integrate-captcha
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 20 commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
26763d3
feat: add captcha site-data keys and verification controller
79acfdc
feat: add public captcha-config.json endpoint
4c9bc0b
fix: require captcha verification before sending subscribe OTP email …
651c5f8
feat: add Captcha.svelte widget
8e216fc
feat: gate subscribe form's Continue button on captcha verification
10ba6d3
feat: add Captcha Providers admin page (shared panel extracted from A…
e58433c
fix: scope api-server auto-loader glob to known handler filenames
b056903
feat: add real provider logos to Captcha Providers page
9f00c14
fix: trim transparent padding from captcha provider logos
9b6e408
fix: gate captcha widget render on provider .ready() when available
0d75635
fix: keep subscribe dialog open when captcha challenge overlay is cli…
8b8d20c
fix: re-enable pointer-events on captcha challenge overlays under bod…
d86f6b8
docs: add Captcha docs page, match Captcha Providers intro to Analyti…
0b745e0
fix: fail closed on misconfigured captcha, add verify request timeout
9ea7149
fix: widen captchaToken type to allow null
ff2a6bf
fix: reset captcha widget after rejected token, keep required state o…
07de0a3
fix: surface the server's specific error message on failed subscribe …
996dc10
fix: loadScript waits for an in-flight script load instead of resolvi…
8521b7d
logic fixes
644c9e4
fix: remove failed script tag so a captcha load retry doesn't hang fo…
3d0edb5
fix: send user back to solve a fresh captcha on Resend instead of rep…
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| export interface ProviderRequirement { | ||
| label: string; | ||
| type: string; | ||
| placeholder: string; | ||
| required: boolean; | ||
| value: string; | ||
| } | ||
|
|
||
| export interface ProviderDefinition { | ||
| label: string; | ||
| logo?: string; | ||
| key: string; | ||
| isEnabled: boolean; | ||
| activeInSite: boolean; | ||
| requirements: ProviderRequirement[]; | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,172 @@ | ||
| <script module lang="ts"> | ||
| // Module scope (shared across every Captcha instance, unlike a plain | ||
| // const in the instance script below) — so if two instances ever load | ||
| // the same provider script concurrently, the second one awaits the | ||
| // first's in-flight load instead of finding the <script> tag already | ||
| // present and resolving immediately before it's actually finished. | ||
| const scriptLoadPromises = new Map<string, Promise<void>>(); | ||
|
|
||
| export function loadScript(src: string): Promise<void> { | ||
| const existing = scriptLoadPromises.get(src); | ||
| if (existing) { | ||
| return existing; | ||
| } | ||
|
|
||
| const promise = new Promise<void>((resolvePromise, rejectPromise) => { | ||
| const existingScript = document.querySelector(`script[src="${src}"]`); | ||
| if (existingScript) { | ||
| existingScript.addEventListener("load", () => resolvePromise(), { once: true }); | ||
| existingScript.addEventListener( | ||
| "error", | ||
| () => rejectPromise(new Error(`Failed to load ${src}`)), | ||
| { once: true } | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| const script = document.createElement("script"); | ||
| script.src = src; | ||
| script.async = true; | ||
| script.onload = () => resolvePromise(); | ||
| script.onerror = () => { | ||
| // Remove the failed tag so a retry (after this promise is evicted | ||
| // from the cache below) creates a fresh <script> with fresh | ||
| // listeners, instead of finding this dead one whose error event | ||
| // already fired and will never fire again. | ||
| script.remove(); | ||
| rejectPromise(new Error(`Failed to load ${src}`)); | ||
| }; | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| document.head.appendChild(script); | ||
| }).catch((err) => { | ||
| // Don't cache a permanent failure — a transient network blip | ||
| // shouldn't block every future attempt to load this script. | ||
| scriptLoadPromises.delete(src); | ||
| throw err; | ||
| }); | ||
|
|
||
| scriptLoadPromises.set(src, promise); | ||
| return promise; | ||
| } | ||
| </script> | ||
|
|
||
| <script lang="ts"> | ||
| import { onMount } from "svelte"; | ||
| import { resolve } from "$app/paths"; | ||
| import clientResolver from "$lib/client/resolver.js"; | ||
|
|
||
| interface Props { | ||
| onVerify: (token: string) => void; | ||
| onReady?: (required: boolean) => void; | ||
| } | ||
|
|
||
| let { onVerify, onReady }: Props = $props(); | ||
|
|
||
| type ProviderName = "hcaptcha" | "recaptcha" | "turnstile"; | ||
|
|
||
| const PROVIDER_SCRIPT: Record<ProviderName, string> = { | ||
| hcaptcha: "https://js.hcaptcha.com/1/api.js", | ||
| recaptcha: "https://www.google.com/recaptcha/api.js", | ||
| turnstile: "https://challenges.cloudflare.com/turnstile/v0/api.js" | ||
| }; | ||
|
|
||
| // All three providers' checkbox-widget SDKs converge on the same | ||
| // `global.render(container, { sitekey, callback })` shape, so one code | ||
| // path covers all of them instead of three near-duplicate branches. | ||
| const PROVIDER_GLOBAL: Record<ProviderName, string> = { | ||
| hcaptcha: "hcaptcha", | ||
| recaptcha: "grecaptcha", | ||
| turnstile: "turnstile" | ||
| }; | ||
|
|
||
| let provider = $state<ProviderName | null>(null); | ||
| let siteKey = $state<string | null>(null); | ||
| let container: HTMLDivElement | undefined = $state(); | ||
| let widgetId: string | number | undefined; | ||
|
|
||
| // Resets the rendered widget so the user can solve it again, e.g. after | ||
| // the server rejects a token (expired/already used). Exposed to the | ||
| // parent via `bind:this`. | ||
| export function reset() { | ||
| if (!provider || widgetId === undefined) return; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const global = (window as any)[PROVIDER_GLOBAL[provider]]; | ||
| global?.reset?.(widgetId); | ||
| } | ||
|
|
||
| onMount(async () => { | ||
| // Tracks whether we've already told the parent a provider is required, | ||
| // so a later failure (script load, SDK init) never walks that back — | ||
| // the server enforces the check regardless, so the button should stay | ||
| // disabled rather than falsely suggesting the form can be submitted. | ||
| let providerConfirmed = false; | ||
|
|
||
| try { | ||
| const response = await fetch(clientResolver(resolve, "/captcha-config.json")); | ||
| const config = await response.json(); | ||
|
|
||
| if (!config.provider || !config.siteKey) { | ||
| onReady?.(false); | ||
| return; | ||
| } | ||
|
|
||
| provider = config.provider as ProviderName; | ||
| siteKey = config.siteKey; | ||
| providerConfirmed = true; | ||
| onReady?.(true); | ||
|
|
||
| await loadScript(PROVIDER_SCRIPT[provider]); | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const global = (window as any)[PROVIDER_GLOBAL[provider]]; | ||
| const renderWidget = () => { | ||
| if (container && global?.render) { | ||
| widgetId = global.render(container, { | ||
| sitekey: siteKey, | ||
| callback: (token: string) => onVerify(token) | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| // reCAPTCHA (and hCaptcha) attach `.render` asynchronously after the | ||
| // script's onload fires — calling render() immediately can hit | ||
| // "grecaptcha.render is not a function". Their SDKs expose `.ready()` | ||
| // to gate on; Turnstile has no such method, so it just falls through | ||
| // to an immediate render as before. | ||
| if (global?.ready) { | ||
| global.ready(renderWidget); | ||
| } else { | ||
| renderWidget(); | ||
| } | ||
| } catch (err) { | ||
| console.error("Failed to load captcha widget", err); | ||
| if (!providerConfirmed) { | ||
| onReady?.(false); | ||
| } | ||
| } | ||
|
otherwiseGG marked this conversation as resolved.
|
||
| }); | ||
| </script> | ||
|
|
||
| {#if provider} | ||
| <div bind:this={container} data-testid="captcha-widget"></div> | ||
| {/if} | ||
|
|
||
| <style> | ||
| /* | ||
| * The subscribe Dialog sets `pointer-events: none` on <body> while open | ||
| * (its scroll-lock). Google reCAPTCHA's expanded image-challenge (and | ||
| * similarly hCaptcha/Turnstile challenge overlays) are injected as direct | ||
| * children of <body> by the provider's own script, so they inherit that | ||
| * `none` and become click-through — clicks meant for the challenge fall | ||
| * through to whatever's underneath with pointer-events re-enabled, which | ||
| * is our own dialog. `:has()` re-arms pointer-events on the iframe and | ||
| * its whole wrapper chain regardless of how deep the provider nests it. | ||
| */ | ||
| :global(body *:has(> iframe[src*="recaptcha"])), | ||
| :global(body *:has(> iframe[src*="hcaptcha.com"])), | ||
| :global(body *:has(> iframe[src*="challenges.cloudflare.com"])), | ||
| :global(iframe[src*="recaptcha"]), | ||
| :global(iframe[src*="hcaptcha.com"]), | ||
| :global(iframe[src*="challenges.cloudflare.com"]) { | ||
| pointer-events: auto !important; | ||
| } | ||
| </style> | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| import { afterEach, describe, expect, it, vi } from "vitest"; | ||
| import { render } from "vitest-browser-svelte"; | ||
| import Captcha, { loadScript } from "./Captcha.svelte"; | ||
|
|
||
| describe("Captcha", () => { | ||
| afterEach(() => { | ||
| vi.unstubAllGlobals(); | ||
| }); | ||
|
|
||
| it("renders nothing and reports not-required when no provider is configured", async () => { | ||
| vi.stubGlobal("fetch", vi.fn().mockResolvedValue({ json: async () => ({ provider: null, siteKey: null }) })); | ||
| const onReady = vi.fn(); | ||
| const onVerify = vi.fn(); | ||
|
|
||
| const screen = await render(Captcha, { onVerify, onReady }); | ||
| await expect.element(screen.getByTestId("captcha-widget")).not.toBeInTheDocument(); | ||
|
|
||
| expect(onReady).toHaveBeenCalledWith(false); | ||
| expect(onVerify).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("renders the widget container and reports required when a provider is configured", async () => { | ||
| vi.stubGlobal( | ||
| "fetch", | ||
| vi.fn().mockResolvedValue({ json: async () => ({ provider: "turnstile", siteKey: "site-key-123" }) }), | ||
| ); | ||
| const onReady = vi.fn(); | ||
| const onVerify = vi.fn(); | ||
|
|
||
| const screen = await render(Captcha, { onVerify, onReady }); | ||
| await expect.element(screen.getByTestId("captcha-widget")).toBeInTheDocument(); | ||
|
|
||
| expect(onReady).toHaveBeenCalledWith(true); | ||
| }); | ||
|
|
||
| it("does not walk back the required state when the provider SDK fails after being detected", async () => { | ||
| vi.stubGlobal( | ||
| "fetch", | ||
| vi.fn().mockResolvedValue({ json: async () => ({ provider: "hcaptcha", siteKey: "site-key-123" }) }), | ||
| ); | ||
|
|
||
| // Pre-seed the script tag so loadScript finds it already present (no | ||
| // real network call needed), then fire its load event shortly after — | ||
| // loadScript now always waits for a real load/error event rather than | ||
| // assuming presence means "already loaded" (see the concurrent-load | ||
| // test below), so this simulates it finishing normally. Then make the | ||
| // provider global's render() throw, simulating the SDK failing to | ||
| // initialize after the script itself loaded. | ||
| const script = document.createElement("script"); | ||
| script.src = "https://js.hcaptcha.com/1/api.js"; | ||
| document.head.appendChild(script); | ||
| setTimeout(() => script.dispatchEvent(new Event("load")), 10); | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (window as any).hcaptcha = { | ||
| render: () => { | ||
| throw new Error("provider SDK init failed"); | ||
| }, | ||
| }; | ||
|
|
||
| const onReady = vi.fn(); | ||
| await render(Captcha, { onVerify: vi.fn(), onReady }); | ||
|
|
||
| await vi.waitFor(() => expect(onReady).toHaveBeenCalledWith(true)); | ||
| // Give the (failing) render attempt a chance to run and hit the catch | ||
| // block before asserting it never reported not-required afterwards. | ||
| await new Promise((r) => setTimeout(r, 50)); | ||
| expect(onReady).not.toHaveBeenCalledWith(false); | ||
|
|
||
| document.head.removeChild(script); | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| delete (window as any).hcaptcha; | ||
| }); | ||
|
|
||
| }); | ||
|
|
||
| describe("loadScript", () => { | ||
| const src = "https://example.com/captcha-test-script.js"; | ||
|
|
||
| afterEach(() => { | ||
| document.head.querySelectorAll(`script[src="${src}"]`).forEach((el) => el.remove()); | ||
| }); | ||
|
|
||
| it("shares one in-flight load across concurrent callers instead of resolving early", async () => { | ||
| const onload1 = vi.fn(); | ||
| const onload2 = vi.fn(); | ||
|
|
||
| const p1 = loadScript(src).then(onload1); | ||
| const p2 = loadScript(src).then(onload2); | ||
|
|
||
| const scripts = document.head.querySelectorAll(`script[src="${src}"]`); | ||
| expect(scripts.length).toBe(1); | ||
|
|
||
| // Neither caller should resolve just because a second call saw the | ||
| // first call's still-loading <script> tag. | ||
| await Promise.resolve(); | ||
| await Promise.resolve(); | ||
| expect(onload1).not.toHaveBeenCalled(); | ||
| expect(onload2).not.toHaveBeenCalled(); | ||
|
|
||
| scripts[0].dispatchEvent(new Event("load")); | ||
| await Promise.all([p1, p2]); | ||
|
|
||
| expect(onload1).toHaveBeenCalled(); | ||
| expect(onload2).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("removes a failed script tag so a retry creates a fresh one instead of hanging on stale listeners", async () => { | ||
| // Own URL, distinct from `src` above -- the module-level cache is a | ||
| // singleton shared across tests in this file, so reusing `src` here | ||
| // would just return the already-resolved promise the earlier test left | ||
| // behind instead of exercising a fresh load. | ||
| const failSrc = "https://example.com/captcha-test-script-retry.js"; | ||
|
|
||
| const failedLoad = loadScript(failSrc); | ||
| const scriptsAfterFirstAttempt = document.head.querySelectorAll(`script[src="${failSrc}"]`); | ||
| expect(scriptsAfterFirstAttempt.length).toBe(1); | ||
|
|
||
| scriptsAfterFirstAttempt[0].dispatchEvent(new Event("error")); | ||
| await expect(failedLoad).rejects.toThrow(); | ||
|
|
||
| // The failed tag must be gone -- otherwise a retry would find a dead | ||
| // script whose error event already fired (and never will again), | ||
| // attach listeners that never trigger, and hang forever. | ||
| expect(document.head.querySelectorAll(`script[src="${failSrc}"]`).length).toBe(0); | ||
|
|
||
| const retryLoad = loadScript(failSrc); | ||
| const scriptsAfterRetry = document.head.querySelectorAll(`script[src="${failSrc}"]`); | ||
| expect(scriptsAfterRetry.length).toBe(1); | ||
|
|
||
| scriptsAfterRetry[0].dispatchEvent(new Event("load")); | ||
| await expect(retryLoad).resolves.toBeUndefined(); | ||
|
|
||
| document.head.querySelectorAll(`script[src="${failSrc}"]`).forEach((el) => el.remove()); | ||
| }); | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.