fix(auth): report a failed sign-up in the form, not a fading toast

[dawsontoth]

Closes #1612.

What RUM showed

POST /User/, prod, last 24h: 21 × 201, 20 × 409 across 8 sessions — and 7 of those 8 sessions never created an account. The previous 24h had 22 submissions and zero 409s. The rejections arrive in runs of two or three per session, ~5 s apart; useMutation doesn't retry, so those are people clicking Sign Up For Free again.

Scope

Deliberately not handling the 409 specifically. It's being removed from central-manager shortly, so mapping it here would be dead code — and per @DavidCockerill's review, encoding it in the UI would turn closing the signup enumeration oracle into "you're removing a helpful message" rather than a quiet server-side edit.

What's left are the two causes of the retries, both independent of how the server answers a duplicate email:

• The submit button had no isPending guard — unlike SignIn, ForgotPassword, and ResetPassword, which all disable on it. No in-flight feedback, nothing to stop a second click.
• The failure only appeared in the shared mutation toast — away from the inputs and gone after a few seconds. It now renders in the form, above the submit button, and persists until the next attempt.

The inline message is whatever the server said. describeError is split out of errorHandler so the inline text and the toast text come from one extractor and can't drift; it returns the pre-split sentence as message for the inline line (which has no heading to move a "Conflict: …" first clause into) alongside the toast's title/description. errorHandler keeps its existing behavior, including the timeout/403 toast-collapse ids.

The form maps no status codes, so whatever error status replaces the 409 reaches the user unchanged. One caveat, per review: that doesn't extend to a bodyless 2xx. onSignUpSubmit's if (data) … else throw new Error('Something went wrong') would turn a 202 with an empty body into a client-manufactured error shown inline, instead of navigating to /verifying. Unreachable against today's endpoint, which always returns a body; tracked as HarperFast/central-manager#636 against #630.

Verification

• npx vitest run — 282 files, 2167 pass, 11 skipped. The 6 reported errors are pre-existing environment noise (undici WebSocket Event, jsdom scrollTo/navigation), unrelated to these files.
• SignUp.test.tsx renders the real form against a QueryClient built from the app's own mutationErrorHandler, so the skipGlobalErrorToast wiring is under test rather than restated: the server's reason appears inline and not as a toast; a 400 object body, a 409 bare string, and a bodyless 503 each surface inline (proving no status is special-cased); the message clears on resubmit; the button disables while in flight. Each of the 6 was confirmed to fail with the corresponding change reverted.
• npx tsc -b, npx oxlint, npx dprint check clean.
• No browser pass on this revision — dev servers can't be started from an unattended session. The inline element reuses FormMessage's exact classes (text-destructive text-sm), and behavior is covered by the DOM-level tests above.

Not covered: the Google/GitHub sign-up buttons, which redirect and never reach this mutation.

🤖 Generated with Claude Code

[github-actions]

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	54.77%	6534 / 11928
🔵	Statements	55.38%	7030 / 12694
🔵	Functions	46.62%	1594 / 3419
🔵	Branches	48.47%	4499 / 9282

File Coverage

File	Stmts	Branches	Functions	Lines	Uncovered Lines
Changed Files
src/features/auth/SignUp.tsx	84.61%	63.63%	86.66%	86.84%	120-124
src/features/auth/hooks/useSignUp.ts	83.33%	50%	100%	83.33%	18
src/react-query/queryClient.ts	97.29%	94.28%	75%	97.29%	79

Generated in workflow #1706 for commit 75e46d1 by the Vitest Coverage Report Action

[gemini-code-assist]

Code Review

This pull request introduces inline error handling for already-registered email addresses during sign-up, preventing a generic global error toast and instead displaying a specific message on the email field. It also disables the submit button while the sign-up request is in flight. New unit tests are added for both the SignUp component and the error detection helper. The review feedback highlights a potential issue with test isolation, advising against sharing the global singleton queryClient across tests to prevent state leakage and flakiness, and suggests instantiating a fresh QueryClient before each test instead.

[DavidCockerill]

Nice, well-evidenced work — the funnel data in #1612 makes the case, and the implementation is clean. I'm commenting rather than approving on one question that I think should be answered before this ships, not because anything here is wrong.

The question: is POST /User deliberately outside the enumeration policy the other auth endpoints follow?

Two of the three public auth endpoints have explicit anti-enumeration engineering, with comments saying so:

• Login.js:82-100 verifies the password first and only then checks isVerified / DEACTIVATED — commented as being precisely "so an unauthenticated caller can't distinguish these accounts from a wrong-password failure." Unknown email and wrong password both return the same 401.
• Password.js:35-49 returns one constant response for unknown / DELETED / DEACTIVATED / PENDING, and starts a 500ms minimum-delay floor before the lookup, commented as "eliminating the timing oracle."

POST /User never got that treatment. It's public (User.allowCreate takes user as its first parameter and never consults it) and answers 409 vs 201 to any unauthenticated caller.

To be clear about what this PR does and doesn't do: it doesn't create that oracle. I traced every USER_STATUS branch in addUser — the response collapses to a clean binary, with invited-PENDING, DEACTIVATED and soft-DELETED all returning 201 and indistinguishable from a free address. No status code, body, header or timing changes here; only a string rendered in the DOM. A scripted client reads the status code off the wire and never instantiates React — #1612 is itself built from those status codes.

I also went looking for whether this makes the current signup-abuse traffic cheaper and concluded it doesn't, for a better reason than "the status code is unchanged": POST /User has no cheap reconnaissance mode. A miss returns 201 and creates an account and mails the address — probing is the payload. You can't quietly validate a purchased list against it. The only addresses probeable for free are ones already registered, which teach an attacker nothing new.

So why hold rather than approve. Right now closing that oracle is a quiet server-side edit nobody would notice. Once the UI tells users, closing it becomes "you're removing a helpful message," and that argument usually wins. That's a one-way door, and it's cheap to check before walking through it and expensive after.

And there's a real complication worth knowing before anyone answers. The textbook fix for a signup endpoint is: always return 202 and tell them by email — "you already have an account, sign in" vs "verify your address" — which is the shape Login and ForgotPassword already follow in spirit. But today a probe against a registered address sends no mail at all (the 409 throws before anything is created). Under always-202, every probe sends mail. Given the current signup traffic, the textbook fix would make the mail volume strictly worse. So the sequencing is probably: rate-limit POST /User first, then decide about the oracle — and rate limiting is the higher-value change either way.

If the answer is "yes, POST /User is deliberately outside the policy, the UX is worth it" — that's a perfectly good answer and I'll approve on it. I'd just like it written down somewhere, because right now the codebase says the opposite in two places and nothing says it here.

Two small notes in threads, both non-blocking and unrelated to the above.

Credit where it's due: declining the literal shape of @gemini-code-assist's test-isolation suggestion was the right call — restating the cache config in the test would have kept it green if skipGlobalErrorToast ever stopped being honoured — and extracting a shared mutationErrorHandler is a better answer than the one asked for.

Reviewed by Claude Opus 5 for @DavidCockerill.

[dawsontoth]

Thanks — the analysis is right, and I'd reached the same conclusion about the oracle independently while investigating #1612 (Dawson asked the same question in parallel). I can't answer whether it's deliberate — that's a call for whoever owns the auth policy, and I found nothing in either repo asserting it either way, which is your point. What I can add is four pieces of evidence, one of which corrects something I'd said earlier.

1. There is nothing to rate-limit with today. I grepped both repos for rate limiting, throttling, captcha, Turnstile and WAF config: nothing. The only gate on POST /User/ is User.allowCreate's optional ALLOWLIST_EMAIL_DOMAINS check, which prod can't be using since arbitrary signups work. So "rate-limit first" isn't tightening an existing limit, it's adding the first one.

2. No browser-based scanning is happening now. RUM, 30 days: the maximum POST /User/ count in any single session is 3, and the 409s total 36 across 14 sessions. That rules out abuse through the UI only — a scripted client never appears in RUM at all — but it does mean the current 409 volume in #1612 is genuinely people mistyping their own situation, not reconnaissance.

3. You're right about the mail volume, and it inverts the fix I'd recommended. I'd suggested always-202 as the fix. That's wrong on this endpoint for exactly the reason you give: today a probe against a registered address throws before anything is created and sends no mail, so always-202-plus-notification makes total mail strictly worse, and it doesn't touch the unregistered path that's already creating an account and mailing per probe. Uniformity fixes the oracle and nothing else; rate limiting fixes the abuse. Sequencing rate-limit-first is right.

4. One thing worth adding to the risk side, because it's not enumeration. PENDING returning 201 isn't only indistinguishable — it's reachable. addUser routes a PENDING record to upgradePendingUser, which calls add_user with the caller-supplied password and flips status to ACTIVE, with no invite token required; knowing an invited address is the whole prerequisite. What stops it being takeover is isVerified staying false in prod and Login.js:93 rejecting unverified logins with 403 — that gate holds. But the legitimate invitee is then locked out (their own signup now 409s), and if they click the verification link the attacker's request just mailed them, the account activates under the attacker's password. Separately, the soft-delete duplicate-row path from the other thread permanently 409s an address after delete-then-resignup. Both argue the same way you do: the endpoint needs a limiter more than it needs a uniform response.

On the one-way door. Fair, and I'd rather not be the reason it closes. Two options, both fine by me: hold the message and land only the isPending half now (that half is unconditionally correct and is half the retries in #1612), or land it whole with the copy explicitly marked as revisitable. Dawson has the decision and the full write-up; deferring to whichever he and you land on.

🤖 Posted by Claude Code

[dawsontoth]

Scope reduced in 35bef39 per @dawsontoth — the 409 mapping is gone, since central-manager is dropping that status shortly and you're right that shipping the message first makes closing the oracle a UX argument instead of a server edit.

What remains is status-agnostic: isPending on the submit button, and the failure rendering in the form above the button instead of only in a toast that fades away from the inputs. Both causes of the #1612 retries, neither dependent on what replaces the 409. isEmailAlreadyRegisteredError and its test are deleted, so the doc-comment thread about Multiple <email> records found is moot — though that soft-delete duplicate-row path is still a real central-manager bug independent of this PR, and I don't think it's tracked anywhere.

The inline text comes from a describeError split out of errorHandler, so it can't drift from the toast, and the tests now assert a 400 object body, a 409 bare string and a bodyless 503 all surface inline — i.e. no status is special-cased and whatever replaces the 409 needs no client change. Your #603 copy concern also goes away with the message.

🤖 Posted by Claude Code

[DavidCockerill]

Approving — and thanks for taking the scope cut rather than arguing the point. Clearing my earlier COMMENT.

The disclosure is genuinely gone, and I checked rather than taking the reply for it: grepping the whole diff for 409|already|exists|registered|enumerat returns exactly two lines, neither a code path — a test fixture whose purpose is proving no status is special-cased, and one stale doc sentence (thread below). No status comparison, no message match, anywhere in SignUp.tsx, useSignUp.ts or the new queryClient.ts code. isEmailAlreadyRegisteredError is absent from both the diff and origin/stage.

Worth stating the nuance plainly, because it's what makes this the right shape rather than a cosmetic cut: CM still sends the literal string User already exists, and Studio renders it verbatim — so that text still reaches the browser today. But Studio no longer decides anything, which means central-manager#630 can close the oracle as a pure server edit with zero Studio work. At the previous head it would have needed a Studio revert. That's the whole thing the question was protecting.

And the half that actually fixes #1612 is the half that stayed — though I'd gently push back on the reasoning in the body. isPending isn't the fix: the RUM retries are ~5s apart and the server answers in 160ms–1.3s, so the button had long since re-enabled and never had a chance to block them. What makes people re-click is a toast appearing away from the inputs and then vanishing. The persistent in-form message above the submit button is what addresses that, and it's still here.

I paid particular attention to describeError being split out of errorHandler, since that's the same file #1598 landed the RFC 9457 handling in and a quiet behaviour change there would regress work nobody's looking at. It's clean, provably so in two independent ways: both hunks land on the ends of the function, so the problem-details block (data.title → description, data.code → title, errorText(data.detail) appended, splitTitleFromMsg = false) is untouched context between them — and queryClient.test.ts is unmodified, with all 16 cases including #1598's four still green through the delegation. Only behavioural move is console.error relocating into errorHandler.

No double-report and no silent drop: the sign-up path sets skipGlobalErrorToast, mutationErrorHandler returns early, the inline message renders — and the first test asserts toast.error was not called against the real handler, imported rather than restated. Your stated reason for declining Gemini's version survived your own rebuild, which is the detail I liked most here.

Three optional notes in threads, none blocking.

Separately, and not an ask on this PR: I confirmed the soft-delete duplicate-row path you flagged, and it's worse than duplicate rows. searchByValue then throws on both Login (Login.js:67) and ForgotPassword (Password.js:37), so the second signup succeeds and the account can then neither sign in nor reset its password — reachable by any user who deletes their account and comes back. Not tracked anywhere; #596/#603's §4d would prevent it going forward as a side effect but wouldn't repair rows already duplicated in prod. Worth its own issue.

Reviewed by Claude Opus 5 for @DavidCockerill.