fix(forms): escape stored HTML, gate submit on is_public, enforce RBAC on legacy forms routes - #1057
Open
mmcintosh wants to merge 3 commits into
Open
fix(forms): escape stored HTML, gate submit on is_public, enforce RBAC on legacy forms routes#1057mmcintosh wants to merge 3 commits into
mmcintosh wants to merge 3 commits into
Conversation
…C on legacy forms routes The legacy forms/form_submissions tables (0004_forms.sql) went live via #1022, reactivating admin-forms.ts and public-forms.ts routes that previously 503'd harmlessly on missing tables. Their existing gaps are now reachable: - No escapeHtml on admin-authored display_name/description/category/name rendered into admin and public HTML (R8) — stored XSS. - JSON.stringify(formioSchema) embedded raw in a <script> tag — a </script> in any schema field value (e.g. a component label) breaks out of the tag. - admin-forms.ts gated by requireAuth() only; the 'forms:manage' nav permission is UI-only (requirePermission is a no-op stub) — any authenticated user of any role could manage forms and read submissions. - POST /:identifier/submit checked is_active but not is_public — a form marked private was still directly submittable, bypassing the GET routes' gate. Fixes: escapeHtml() on every admin-controlled string rendered into HTML; a new jsonForScript() helper (escapes `<` to <) for the two schema/ settings script embeds; requireRole(['admin','editor']) on admin-forms.ts; is_public added to the submit route's form lookup, matching the GET routes. Real-SQLite integration tests added (public-forms.integration.test.ts) since the existing mock-DB suite matches on `sql.includes(...)` and can't catch a dropped WHERE condition; admin-forms.test.ts covers the new RBAC gate.
… turnstile-config on is_public Follow-up from an independent review of 517a2e9. Findings addressed: - admin-forms.ts submissions viewer dumped sub.submission_data (populated by the anonymous public submit endpoint) via JSON.stringify into a <pre> tag with no escaping. The pre-existing sanitizeDeep() only recurses into object VALUES (Object.entries(value), key never touched) — an anonymous submitter fully controls both. A malicious JSON key survived storage unescaped and executed in an admin/editor's session the moment they opened that form's submissions page. Worse than anything in the original patch: requires zero privileges to plant. Fixed both layers — escapeHtml() at the render site (defense in depth) and sanitizeInput() on keys in sanitizeDeep() (root cause, matches values' treatment). - public-forms.ts's GET /:identifier/turnstile-config selects on is_active only, same bug class as the already-fixed submit route — a private form's existence was still probeable via 200-vs-404. Added is_public = 1 to match the other GET routes. Note: this route currently 500s unconditionally (both before and after this fix) for an unrelated, pre-existing reason — migration 0004_forms.sql never added the turnstile_enabled/turnstile_settings columns this query names explicitly, so it errors before the WHERE clause is evaluated. Not fixed here (a migration change, separate scope+review); the integration test documents current real behavior rather than asserting something not yet true. The submit handler is unaffected (uses SELECT *, degrades to global-settings inheritance rather than erroring). - admin-forms-builder.template.ts rendered data.name unescaped in two spots while display_name right next to it was escaped — currently inert only because of creation-time regex validation (^[a-z0-9_]+$), not defense in depth. Escaped for consistency with every other field this area handles. New/extended tests verify all three: a key-escaping regression in public-forms.test.ts, a submissions-page XSS regression in admin-forms.test.ts, and an is_public regression for turnstile-config in the integration suite.
Covers what's actually reachable through a real UI flow: display_name has no validation (unlike name's ^[a-z0-9_]+$ regex) and renders on both the admin builder and public form page. The is_public submit-gate fix isn't included here — there's no admin UI/API path to ever set is_public=0 today, so it's covered by the real-SQLite integration suite instead (see file header for detail). Written per project policy; not run locally.
mmcintosh
marked this pull request as ready for review
August 18, 2026 01:41
lane711
added a commit
that referenced
this pull request
Sep 8, 2026
* feat(auth): two-factor authentication (TOTP + backup codes)
Adds a two-factor-auth core plugin: TOTP enrolment with a scannable QR, single-use
backup codes, a per-account lockout, second-factor enforcement on the passwordless
sign-in paths, and an administrative reset for users who lose their authenticator.
Better Auth's twoFactor() is composed unconditionally, so enrolment state alone
decides whether a user is challenged. The plugin owns the enrolment surface
(/admin/two-factor); core owns the login challenge (/auth/two-factor) so that
turning plugins off stops new enrolments without locking out users who already
have a second factor.
What is enforced where:
- Challenge on password sign-in — Better Auth, via twoFactorRedirect.
- Challenge on magic-link / email-OTP — guardPasswordlessSecondFactor in the
/auth/* catch-all, since BA challenges only the password paths and would
otherwise hand an enrolled account a session with no code.
- Session upgrade after verification — POST /auth/two-factor/complete mints
the same JWT a password login mints, so a 2FA session is never weaker than
a password one. It derives the credential from the session's own user and
never from request input.
- Policy (issuer, lockout, backup-code count) is clamped on read, so no
stored value or failed load can disable the lockout or drop below 5 codes.
The QR is rendered server-side (qrcode-svg, already a core dep, Workers-safe)
because these pages have no client bundler and a CDN script on a page handling
TOTP secrets is not a trade worth making. Its size is COMPUTED from the symbol's
module count rather than fixed: scannability is CSS pixels per module, and the
issuer is operator-configurable to 64 chars and appears in the URI twice, so the
symbol ranges 49-69 modules. A fixed container delivered 4.14 px/module by
default and 3.06 with a long issuer — well-formed, and too small for a phone to
decode. It is now a constant 6.00 px/module at any issuer length, with the size
stamped on the <svg> rather than left to CSS.
Route input is pinned to ^otpauth://totp/ so the endpoint cannot render
arbitrary text as a QR from our origin — a phishing primitive, since a QR is
unreadable to the human deciding whether to trust it.
── Administrative reset (break-glass) ──
Without one, the complete list of ways back into an account with a lost
authenticator was an unused backup code or `wrangler d1 execute --remote`.
Everything an operator would reach for first is closed by design: password reset
deliberately mints no session, magic link and email OTP are refused for enrolled
users, and self-disable needs the session you cannot get. So a sole admin who
enrolled and lost phone plus codes locked the entire organisation out of the
portal until someone with Cloudflare credentials intervened.
POST /admin/two-factor-reset clears a user's second factor, and migration 0004
adds auth_user.two_factor_required so the reset can demand re-enrolment instead
of silently downgrading the account to password-only. The flag lives on auth_user
rather than auth_two_factor because the reset DELETEs that row — a flag stored
there would be destroyed by the action that sets it. required and enabled are
independent: required && !enrolled is what redirects to /admin/two-factor;
required && enrolled means enrolled and not permitted to turn it off.
Controls on the action, and why these:
- Admin role, matching how every other user-management route in admin-users.ts
is gated, so it is governed by role assignments operators already reason about.
- The target's email must be typed back. Not a second factor — it defends
against the realistic failure, which is resetting the wrong row from a list
of similar-looking ones. Password confirmation was considered and rejected:
Better Auth hashes with its own scrypt (salt:key, 161 chars) that
AuthManager.verifyPassword cannot read, and reimplementing it wrong would
break the break-glass itself.
- Every use writes a two_factor_reset security event naming both the actor and
the subject. Best-effort: an unavailable audit sink must not be the reason a
lockout recovery fails.
- A user under the requirement may not turn the factor back off. The redirect
middleware cannot cover that — POST /auth/two-factor/disable is not under
/admin/*, and /admin/two-factor (which hosts the disable form) has to stay
exempt or enrolment would be impossible — so a second guard sits in the
/auth/* catch-all beside guardPasswordlessSecondFactor, ahead of
auth.handler, and the page replaces the form with an explanation. Without it
a user told to enrol could enrol and immediately switch it off, leaving the
account password-only, BA no longer challenging, the passwordless paths
reopened and /api/* ungated, with no audit event to tell the admin.
Deliberately keyed to the REQUIREMENT and not to being enrolled, so ordinary
self-service 2FA management is untouched.
Mounted on its own prefix and NOT behind the plugin's deactivate→404 gate, for
the same reason the login challenge is not: deactivating the plugin stops nothing
about verification, so a recovery path that vanished with the surface would
disappear exactly when the lockout it fixes is still happening. The enforcement
middleware fails OPEN on any DB error and stands aside entirely when the plugin
is off, since /admin/two-factor 404s then and enforcing would loop the user to a
page that cannot exist.
The reset does not revoke the target's sessions — it is a recovery action, not a
containment one; deactivating the account is what containment is for.
Also fixes the plugin sidebar rendering icon NAMES as text ("lock-closed" beside
Two-Factor Auth, "book-open" for API Reference, "variable" for Global
Variables): plugin-menu projected `resolveIcon(icon) || icon`, leaking unmapped
names into markup the layout interpolates.
Verified in a browser: enrolment page and QR render, the displayed QR was proved
in-page to encode the same URI as the manual-entry secret, and the enrolment flow
was confirmed end-to-end with a physical phone scan.
Verified against a running wrangler dev server on real D1 and real Better Auth,
not only under the test harness: 0004 applied to an existing install without
locking out its enrolled user; the reset drove end-to-end (panel render, typed-
email refusal leaving the DB untouched, reset, target then signing in with
password alone, forced re-enrolment redirect for browsers and 403 for JSON, the
break-glass route correctly NOT exempt, audit event naming actor and subject);
the disable guard refused a mandated account while leaving ordinary self-service
disable working; and the 0004 self-heal restored the column after it was dropped
out from under a running app.
Known and deliberately out of scope: CSRF validation is inert across the whole
cookie-authenticated admin surface, because csrfProtection exempts requests with
no auth_token cookie and sign-in mints a Better Auth session cookie instead.
Pre-existing and repo-wide; the fix is the "OD2 Option B" csrf.ts port, which
changes global request handling and needs its own audit. Comments that claimed
CSRF was enforced on these routes have been corrected.
Tests: real Better Auth over real SQLite for the round trip and the lockout, and
41 new cases for the reset — SQL effects, the role and typed-email gates, the
enforcement middleware's exemptions, JSON branch and fail-open paths, the disable
guard, and the four states the enrolment page renders.
Notably the middleware must NOT exempt /admin/two-factor-reset, which a naive
prefix check would hand to the very users who owe an enrolment. E2E in
tests/e2e/101-two-factor-auth.spec.ts and 102-two-factor-admin-reset.spec.ts
(enrolling cases use throwaway accounts so they cannot take out the shared admin).
two-factor-lockout-engages.test.ts gets an explicit 30s timeout: it makes ~10
sequential BA round trips through real scrypt and landed at ~5.1s against the 5s
default, so any added concurrency turned it into a timeout that looked exactly
like a broken lockout.
npm test: 25 failed / 1886 passed — the same 25 pre-existing beta.25 failures as
on the parent commit, verified by diffing failing-test names against a clean
baseline run. Zero regressions.
* chore(auth): renumber 2FA migrations 0003/0004→0006/0007, E2E specs 101/102→106/107
main has since taken 0003 (session_org) and 0004 (forms) for unrelated features
— this branch's 0003_two_factor_lockout.sql/0004_two_factor_required.sql
collided. Renumbered to 0006/0007, deliberately skipping 0005 (reserved for
the FTS5 search PR, also in flight and further along in review). Synced both
migrations/ copies, updated the self-heal comments and test-harness filename
references in migrations.ts/d1-sqlite.ts/two-factor-adapter-create.test.ts,
regenerated the bundle.
E2E specs renumbered 101/102→106/107 to stay clear of both currently-open
PRs (#1057 claims 104, #1058 claims 105) — real main's actual highest spec
is 100, not 103 as an earlier broad search suggested.
* chore(auth): fix remaining stale migration 0003/0004 comment references
The renumbering commit updated services/migrations.ts but missed 5 more
files with the same stale references, found by review — cosmetic only,
the actual filenames/logic were already correctly 0006/0007 everywhere.
* fix(auth): close second-factor gaps found in review of #1059
Eight issues from the review of the two-factor PR, plus two nits.
1. OAuth sign-in bypassed the second factor. The already-linked branch in
oauth-providers mints a session JWT directly, so Better Auth's TOTP
challenge never runs — anyone holding the provider credential skipped a
factor the account owner had deliberately enrolled in. Both the
already-linked and the auto-link branches now refuse an account with a
verified second factor and send it to password sign-in. This cannot
strand anyone: BA requires password verification to reach
/auth/two-factor/enable, so an enrolled account always has a password
login.
2. Forced re-enrolment was only enforced on /admin/*. The same session
cookie drives /api/*, so a user told to enrol kept full read/write access
through the JSON API while the admin who set the requirement believed the
account was blocked. enforceTwoFactorEnrolment is now mounted on /api/*
too, registered ahead of the API route mounts so it actually covers them.
3. API keys are exempt from that gate. Keys are separately revocable and the
only place to mint a replacement (/admin/api-keys) is behind the same
gate, so gating them would take running integrations offline with no way
back. api-key-auth now sets an explicit `authMethod: 'api-key'` marker
and the middleware returns early on it.
4. The gate hit D1 on every request. It now caches the "owes nothing"
verdict for 60s. Only the negative is cached, and never when the read
failed — caching a fail-open default would turn a one-request D1 blip
into a minute of unenforced policy. resetUserTwoFactor (the only writer
of two_factor_required) invalidates the entry.
5. The middleware answered htmx requests with a 403 JSON body. htmx sends
`Accept: */*`, so the HX-Request branch has to come first; it now
redirects via HX-Redirect.
6. The issuer went into the otpauth:// URI on a deny-list. Replaced with a
Unicode-aware allow-list — the first structural character missed forges a
different account entry in the victim's authenticator, and the deny-list
also mangled legitimate names like "Société Générale (EU) Ltd."
7. GET /users/:id/edit selected two_factor_required unconditionally. On an
install that has not run migration 0007 the unknown column rejects the
whole statement, 500ing the page. Falls back through
ensureTwoFactorRequiredColumn self-heal, then a select without the column.
8. The disable guard compared a raw pathname, so POST
/auth/two-factor/disable/ (trailing slash) reached the same BA endpoint
unguarded and deleted an enrolment an admin had mandated. Paths are now
normalized, in both the passwordless guard and the disable guard.
Nits: dropped a dead .catch() in plugin-service (loadTwoFactorPolicy
already catches internally), and corrected two stale comments that
described mount points and MFA assumptions that no longer hold.
The reset button on the user edit page interpolated the user id into an
inline onclick. escapeHtml renders ' as ', which the HTML parser
decodes back to a bare quote before the JS parser sees the attribute — so
the escaping did not protect the handler. Passed via data-user-id instead.
Tests: real-SQLite coverage for the OAuth gate (6), the enrolment cache and
the middleware's htmx / /api/* / API-key branches (8), the issuer allow-list
(3), trailing-slash normalization (1), the edit-page self-heal (1), and the
reset panel's data attribute (4). E2E spec 108 covers API enforcement and
the trailing-slash disable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(e2e): repair 21 pre-existing E2E failures blocking PR #1059
Five distinct root causes, all pre-existing on the branch:
1. Hardcoded Origin (02c-otp-login, 02d-magic-link-auth)
Better Auth answers a POST from an untrusted Origin with 403
INVALID_ORIGIN and trusts the base URL the request arrived on. Both
specs pinned `Origin: http://localhost:9704`, so every request failed
against the CI preview deploy. Use TEST_ORIGIN, which tracks BASE_URL.
2. user-profiles Settings tab was dead code (106-two-factor-auth,
80-user-profile-code-config)
renderPluginSettingsPage gates the Settings tab on the plugin having
user-editable setting keys. user-profiles stores none — its panel is a
static description of the code-defined field model — so the tab, and
the renderUserProfilesSettingsContent behind it, could never render.
Opt such plugins in explicitly via STATIC_SETTINGS_PANEL_PLUGINS.
3. Ambiguous two-factor link (106-two-factor-auth)
The sidebar ships desktop and mobile copies of the /admin/two-factor
link, so an href-based locator matched three elements. Give the profile
security panel's own anchor an id and target that.
4. Role display names live in inputs (85-admin-panel-roles-naming)
They are rendered as <input value="..."> in the bulk-rename form, so
innerText never sees them. Read them out of `value` instead.
5. HTMX form never navigates (68-user-profile-document, 38-user-profile-edit)
The user form submits via hx-put, so waitForURL resolved immediately
and raced the save. Wait on the PUT response instead.
Also rewrites 38-user-profile-edit and 80-user-profile-code-config, whose
premises no longer matched the code: profile fields are declared in
my-sonicjs-app/src/user-profile.model.ts via defineUserProfile(), so the
"unconfigured" assertions and the website-URL-validation test (the field
is declared as plain text) described behavior that does not exist.
Adds unit coverage for the Settings-tab gate so the regression cannot
return silently.
Verified: 53 E2E tests pass locally across the seven touched specs;
packages/core type-check clean; full unit suite green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(graphql,e2e): unblock the two remaining CI failures
GraphQL document mutations were denied for every caller. requireWritePermission
called repo.isAllowed() with an empty typeSettings object, but base grants live
on the document type — with none supplied, isAllowed sees only per-document ACL
rows and denies update/publish/unpublish/delete even for an admin. Load the type
via DocumentTypeRegistry and pass its settings.
Two E2E corrections:
- 100-graphql: the type id is `blog_post`, not `blog_posts`. createDocument on a
type that does not exist surfaced as INTERNAL_SERVER_ERROR.
- 80-user-profile-code-config: a fresh deploy has no `plugins` row for
user-profiles, and #1075 made uninstalled plugins render the Info tab only, so
the guidance panel behind the Settings tab was absent. Install the plugin first
(no-op when already installed).
Verified: 100-graphql 5/5 and 80-user-profile-code-config 3/3 pass locally;
packages/core type-check clean; unit suite 1930 passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Lane Campbell <ldc0618@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Description
The legacy
forms/form_submissionstables started shipping via migration0004_forms.sql(added in #1022, 2026-08-11), which reactivatesadmin-forms.ts/public-forms.ts— routes that previously 404'd/503'd harmlessly on missing tables. Several pre-existing gaps in those routes are now reachable in production instead of moot. This PR closes them.Changes
display_name,description,category,name, and the?search=query param were rendered raw into HTML across the admin list/builder pages and the public form page — addedescapeHtml()(existingutils/sanitize.tshelper) at every site.formio_schema/settingsare embedded viaJSON.stringify(...)directly inside inline<script>tags; a</script>in any schema field value (e.g. a component label, which a form-builder admin controls) broke out of the tag. Added ajsonForScript()helper (<-escapes<) used at both embed sites.admin-forms.tswas gated byrequireAuth()only — any authenticated user, any role, had full form CRUD and submission-data read access. The sidebar nav's'forms:manage'permission gate is UI-only (requirePermissionis a no-op stub); addedrequireRole(['admin', 'editor'])as the actual server-side enforcement.is_publicbypass on submit:POST /:identifier/submitandGET /:identifier/turnstile-configcheckedis_activebut notis_public, unlike the render/schema GET routes — a form marked private was still directly reachable via a crafted request. Both now match.sub.submission_dataviaJSON.stringifyinto a<pre>with no escaping. The pre-existingsanitizeDeep()sanitizer only ever recursed into object values, never keys — and an anonymous public submitter controls both. Fixed at both layers:escapeHtml()at the render site, and key sanitization insanitizeDeep()at the root cause.Testing
Two independent fresh-context reviews (security/reachability/blast-radius) were run against this patch before opening it — both are reflected in the fixes above, not just the diff.
Unit Tests
is_public/RBAC/escaping fixes (the pre-existing mock-DB suite matches onsql.includes(...)and can't catch a dropped WHERE condition), plus a submissions-page XSS regression and asanitizeDeepkey-escaping regression. 44 forms-scoped tests (up from 5).tsc --noEmitclean.E2E Tests
display_nameXSS on the admin builder + public form page (the reachable UI path; see the spec file header for why theis_publicgate isn't included there).Screenshots/Videos
N/A — no visual/UI changes, only escaping and access-control.
Checklist