Repository navigation
Harden the account system: taken addresses, abandoned guests, and a turnstile - #12
Merged
Merged
Conversation
Registering with an address that already has an account printed "User already registered" and stopped there. That is accurate and useless: the person reading it is a guest with a browser full of boards, and the account they are being told about is reachable only by signing in — which leaves those boards with the guest, exactly as signing out would. The form sent them round the same loop with no way through. register() now recognises the case and answers `taken`, with a message that names the cost. The two paths disagree on the word for it — attaching to a guest gives `email_exists`, signing up with no session gives `user_already_exists` — so both are matched, and neither is assumed to cover the other. Checked against the live project rather than guessed. A signed-out caller has no boards to strand, so it gets the shorter sentence. The form offers to sign in as that address, next to the error rather than in place of it: a button on its own would be an invitation without the price on it. The browser suite's after-hook allows a 422 alongside the 400 it already allowed. Chrome logs every failed request at error level, and a taken address is a 422 by design. Still filtered by status rather than by dropping network noise, so a 500 from the stack fails the suite as before. config.toml stops describing somewhere a push would break. site_url is the deployed site now, not the dev server, which is safe in both directions because nothing reads it: there is no redirect-based auth flow here — no OAuth, no magic link, no emailRedirectTo, no password reset — and with confirmations off no mail carrying a link is sent either. additional_redirect_urls lists both origins for the same reason. `env(...)` would be the answer if that changed, and the file says so, along with which values are still unaudited against the hosted project. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TbSwVkGcNQZohV9VoeACRT
Every first visit is a real row in auth.users, and Supabase has no automatic cleanup for them: ordinary traffic grows the table forever, and anyone who wants to can grow it faster by asking the signup endpoint for guests in a loop. sweep_anonymous_users deletes anonymous users who own no board, are on nobody else's board, and have not been seen for a while — pg_cron runs it daily. The interesting half is what it keeps, so every test is paired: a guest with a board and a guest without, swept together. The one that would be a catastrophe rather than a bug has its own test, because is_anonymous is the whole guard between housekeeping and deleting the customers. Only the schedule is conditional, not the function. These migrations are applied to a hosted project, a local stack, and a bare Postgres that `apply.js --stub` fits out with just enough of GoTrue; pg_cron is absent from the third and `create extension` on a missing extension is an error rather than a no-op. The stub grows the three columns the sweep reads, added with `if not exists` so it stays a no-op against a real project. test:db now runs its files one at a time. There is one database behind all of them and rls.test.js clears public.boards before each of its own tests; run concurrently that wipe lands mid-fixture here, and the failure reads as a foreign key bug in the sweep rather than as two suites sharing a table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TbSwVkGcNQZohV9VoeACRT
Anonymous sign-in is the only endpoint here that takes nothing and answers with a row in auth.users, and the nightly sweep is a mop rather than a lock. A Turnstile token is now sent with it. The site key is a value that can be absent, exactly as the Supabase config is: with no key there is no widget, no script and no token, which is what every test and every local dev server runs as. The project only demands a token once CAPTCHA protection is switched on for it, so the two halves move in one direction — the key ships first, the setting is turned on afterwards. The reverse rejects every sign-in. The token is taken per sign-in rather than at construction: it is single-use and short-lived, so one fetched at load would be spent or stale by the time a retry wanted it. A CAPTCHA that will not issue one fails the start the way a refused sign-in does — answered, not thrown — so the gate shows it and keeps its retry. The script loader is memoised on the document rather than in a module variable, because two apps on one page share the document and would otherwise fetch it twice; a failed load clears the memo so a retry is a retry. An onload that leaves no global rejects rather than failing later on undefined — that is a proxy serving an error page with a 200. Not verified against Cloudflare: the tests drive a fake widget. What a real challenge does in a real browser is still unproven. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TbSwVkGcNQZohV9VoeACRT
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds optional Turnstile CAPTCHA support, duplicate-email registration handling, deployed Supabase Auth URLs, and a secured scheduled sweep for stale anonymous users. It also adds browser, Node, and database coverage. ChangesAuthentication flows
Anonymous-user cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant AccountForm
participant createSupabaseAuth
participant Turnstile
participant Supabase
AccountForm->>createSupabaseAuth: submit authentication request
createSupabaseAuth->>Turnstile: request CAPTCHA token
Turnstile-->>createSupabaseAuth: return token or failure
createSupabaseAuth->>Supabase: send request with captchaToken
Supabase-->>createSupabaseAuth: return success or duplicate-email error
createSupabaseAuth-->>AccountForm: return structured result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/AccountForm.jsx`:
- Around line 76-89: Update the generic mode-switch handler in AccountForm to
reset taken alongside error whenever switching modes. Preserve the existing
duplicate-address action reset and ensure returning to registration after
switching through sign-in does not retain stale taken state.
In `@src/platform/turnstile.js`:
- Line 38: Update loadTurnstile and createTurnstileCaptcha to require and use
the document supplied by the factory caller instead of reading
globalThis.document. At the composition boundary in src/shell/auth.js, pass the
browser document explicitly when creating the Turnstile instance, ensuring every
document dependency arrives through factory arguments.
- Around line 44-56: Add a destroy() method to the returned CAPTCHA provider and
invoke it from the owning authentication lifecycle’s destroy path. Track the
script created in the loading flow, clear its onload and onerror handlers during
cleanup, and remove it from the document only while loading is pending. Ensure
repeated or post-settlement cleanup is safe.
In `@test/db/sweep.test.js`:
- Line 24: Update the test lifecycle around the database setup to begin a
transaction in beforeEach and roll it back in afterEach, preventing the global
sweep from persisting deletions. In the permission test’s nested transaction,
replace transaction usage with savepoints while preserving its existing
permission assertions and cleanup behavior.
- Line 22: Remove the direct pg import from the sweep test and update its
database access to use an approved existing helper or access path. If no
approved path exists, document an explicit test dependency exception instead of
retaining the import.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c57cdf1-fe17-48be-8c55-4d1c6d839f01
📒 Files selected for processing (12)
package.jsonsrc/components/AccountForm.jsxsrc/platform/auth.jssrc/platform/turnstile.jssrc/shell/auth.jssupabase/config.tomlsupabase/migrations/20260805190000_sweep_anonymous_users.sqltest/browser/auth.test.jstest/db/stub-auth.sqltest/db/sweep.test.jstest/node/auth.test.jstest/node/turnstile.test.js
…in flight All three were real. The generic mode switch cleared `error` but not `taken`, so a trip to sign-in and back put the "sign in as …" button under a form with nothing wrong with it. Cleared with the error it belongs to. turnstile.js reached for globalThis.document instead of taking it. The document now arrives through the factory, from the composition boundary in shell/auth.js, the same way the Supabase client and the storage do. A script still loading kept its handlers and its tag after whoever asked for it had gone. createTurnstileCaptcha grows a destroy() that drops a load in flight — handlers cleared, tag removed — and the auth port calls it from the destroy() it already had. Safe twice, and safe once the load has landed, both tested. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TbSwVkGcNQZohV9VoeACRT
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/platform/turnstile.js (1)
108-128: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClean up active Turnstile widgets in
destroy().After
turnstile.render()returns a widget ID,cancel(doc)does nothing. Store the ID, callturnstile.remove(widgetId), remove the container, and reject the pending CAPTCHA promise. Add a test for destruction afterrender()and before any callback.
loadTurnstileis document-scoped. Ensure one CAPTCHA instance cannot cancel another instance's pending load.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/turnstile.js` around lines 108 - 128, Update the captcha flow and captcha.destroy around turnstile.render so the returned widget ID is retained, the active widget is removed via turnstile.remove(widgetId), the container is cleaned up, and any pending CAPTCHA promise is rejected when destroyed before a callback. Add coverage for destruction after render but before callback, and scope pending-load cancellation in loadTurnstile to the owning document so one CAPTCHA instance cannot cancel another’s load.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/platform/turnstile.js`:
- Line 128: Update the loadTurnstile/captcha.destroy flow so cancellation is
scoped to the provider that initiated the shared document load. Track active
consumers per doc and only invoke cancel(doc) after the final consumer is
destroyed, preserving the pending loader and script for other providers.
---
Outside diff comments:
In `@src/platform/turnstile.js`:
- Around line 108-128: Update the captcha flow and captcha.destroy around
turnstile.render so the returned widget ID is retained, the active widget is
removed via turnstile.remove(widgetId), the container is cleaned up, and any
pending CAPTCHA promise is rejected when destroyed before a callback. Add
coverage for destruction after render but before callback, and scope
pending-load cancellation in loadTurnstile to the owning document so one CAPTCHA
instance cannot cancel another’s load.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e9d00e0b-c5ef-4533-9eac-f1ddafbe606b
📒 Files selected for processing (5)
src/components/AccountForm.jsxsrc/platform/auth.jssrc/platform/turnstile.jssrc/shell/auth.jstest/node/turnstile.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/platform/auth.js
- src/shell/auth.js
- src/components/AccountForm.jsx
Two more from the review, both right. sweep.test.js called sweep_anonymous_users() with autocommit on. The function deletes every eligible row in the database, not only the ones the test made — so pointed at the hosted project, an idle `npm run test:db` would delete real visitors' guests for real. Each test now runs in a transaction that is rolled back, and the permission test uses savepoints rather than a transaction inside the transaction. The sweep is still exercised; its effects no longer outlive the assertion. The destroy() added last commit cancelled per document, and the script is shared per document. Two apps on one page share it, so destroying either provider pulled the script out from under the other and left it awaiting a promise that could never settle — a worse leak than the one being fixed. The loader now tracks its consumers and cancels once the last one has withdrawn. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TbSwVkGcNQZohV9VoeACRT
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/platform/turnstile.js`:
- Around line 98-101: Update the cancellation logic in the pending-load cleanup
block so it returns immediately when pending.consumers.delete(consumer) is
false, ensuring only registered consumers can cancel the load. Add coverage for
a consumerless direct load followed by destruction of an unrelated provider,
verifying the direct caller’s Promise remains pending.
- Line 149: Update the provider cleanup around captcha.destroy to track rendered
widget IDs and pending Promise reject callbacks, then have destroy call
turnstile.remove for each widget, reject pending Promises, and remove each
container owned by the provider. Add coverage for destroying the provider after
render() but before its callback executes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6205dcfc-9a9c-4069-9237-d6de72467bab
📒 Files selected for processing (3)
src/platform/turnstile.jstest/db/sweep.test.jstest/node/turnstile.test.js
Set.delete() answers false when the consumer never joined, and the previous check read that as "nobody left" — so a provider destroyed before it ever asked for a token, or a direct loadTurnstile(doc) with no consumer at all, could pull the script out from under whoever was actually waiting. Standing to cancel now requires having joined. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TbSwVkGcNQZohV9VoeACRT
Owner
Author
|
Answering the three open threads.
For the record, the substantive findings from the earlier rounds are all in:
342 node tests, 42 db tests, coverage 99.1%. |
Owner
Author
|
@coderabbitai review |
|
boubou666
added a commit
that referenced
this pull request
Aug 5, 2026
#12 added the CAPTCHA and its site key variable, and never gave the deploy workflow the line that passes one to the other. Vite inlines what is in the environment at build time, so the key sat on the repository while every build shipped without it — readTurnstileConfig answered null and the deployed site quietly carried no CAPTCHA at all. Which is the failure mode that feature was designed to have, and it hid the mistake perfectly: nothing broke, nothing logged, and the only way to notice was to look for the key in the bundle and not find it. Claude-Session: https://claude.ai/code/session_01TbSwVkGcNQZohV9VoeACRT Co-authored-by: boubou666 <thomas.epitech@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 5, 2026
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.
Three things the account system needed once it had real users, in the order they bite.
Say what a taken address costs
Registering with an address that already has an account printed "User already registered" and stopped. Accurate and useless: the person reading it is a guest with a browser full of boards, and the account they are being told about is reachable only by signing in — which leaves those boards with the guest, exactly as signing out would. The form sent them round the same loop with no way through.
register()now answerstaken, with a message naming the cost, and the form offers to sign in as that address — next to the error rather than in place of it, because a button on its own is an invitation without the price on it.The two paths disagree on the word for it: attaching to a guest gives
email_exists, signing up with no session givesuser_already_exists. Both are matched, neither assumed to cover the other, and this was checked against the live project rather than guessed.The browser suite's after-hook now allows a 422 alongside the 400 it already allowed — Chrome logs every failed request at error level, and a taken address is a 422 by design. Still filtered by status, so a 500 from the stack fails the suite as before.
Sweep the guests who left nothing behind
Every first visit is a real row in
auth.usersand Supabase has no automatic cleanup: ordinary traffic grows the table forever, and anyone who wants to can grow it faster by asking the signup endpoint for guests in a loop.sweep_anonymous_usersdeletes anonymous users who own no board, are on nobody else's board, and have not been seen for a while;pg_cronruns it daily. The interesting half is what it keeps, so every test is paired — a guest with a board and a guest without, swept together. A registered user never being swept has its own test:is_anonymousis the whole guard between housekeeping and deleting the customers.Only the schedule is conditional, not the function. These migrations reach a hosted project, a local stack, and a bare Postgres that
apply.js --stubfits out with just enough of GoTrue;pg_cronis absent from the third andcreate extensionon a missing extension is an error, not a no-op.test:dbnow runs its files one at a time. There is one database behind all of them andrls.test.jsclearspublic.boardsbefore each of its own tests; run concurrently that wipe lands mid-fixture and reads as a foreign key bug in the sweep rather than as two suites sharing a table.A turnstile in front of the one door anyone can open
Anonymous sign-in takes nothing and answers with a row in
auth.users. The sweep is a mop; this is a lock.The site key is a value that can be absent, exactly as the Supabase config is — no key, no widget, no script, no token, which is what every test and every local dev server runs as. The token is taken per sign-in rather than at construction: it is single-use and short-lived, so one fetched at load would be spent by the time a retry wanted it. A CAPTCHA that will not issue one fails the start answered rather than thrown, so the gate shows it and keeps its retry.
config.toml stops describing somewhere a push would break
site_urlis the deployed site now, which is safe in both directions because nothing reads it: there is no redirect-based auth flow here — no OAuth, no magic link, noemailRedirectTo, no password reset — and with confirmations off no mail carrying a link is sent either.env(...)would be the answer if that changed, and the file says so, along with which values remain unaudited against the hosted project.Deploying this safely
The two halves of the CAPTCHA move in one direction only. Merge and deploy first, then set
VITE_TURNSTILE_SITE_KEY, confirm sign-in still works on the live site, and only then enable CAPTCHA protection in Supabase. Enabling the project setting while the site sends no token rejects every sign-in.Setting the variable is itself a change in failure modes: the live site starts depending on Cloudflare's script loading, where today it cannot fail that way. Deliberately left for a human to decide.
Verified, and not
Node suite 338/338, db suite 42/42 against a real local stack, browser account suite green including the new case, coverage 99.1%.
Not verified: anything involving real Cloudflare. The Turnstile tests drive a fake widget, so what an actual challenge does in an actual browser is unproven.
Found, not fixed
apply.js --stubagainst a bare Postgres is already broken onmain: it fails at20260804154000_board_broadcast.sqlwithschema "realtime" does not exist, an earlier migration than any here. The README's promise that RLS runs against any Postgres does not currently hold.test:nodehas the same parallel-suites-share-one-database flakiness fixed here fortest:db—sync.test.jsand the supabase repository suite each failed once and passed on re-run.🤖 Generated with Claude Code
https://claude.ai/code/session_01TbSwVkGcNQZohV9VoeACRT
Summary by CodeRabbit
New Features
Bug Fixes
Tests