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.
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