fix(security): harden sanitizers and regexes flagged by CodeQL#94
Merged
Conversation
Address CodeQL code-scanning alerts across the sanitization and string-handling surfaces: - svgSanitize (upload boundary): close-tag regexes now match `</script foo>` / `</script\t\n>` / `</script/>` (bad-tag-filter), and stripping iterates to a fixpoint instead of a fixed 2 passes so split-tag obfuscation (`<scr<script>ipt>`) cannot survive. Adds the first test for this boundary. - sanitize.ts fallback: same close-tag fix + fixpoint loop in the no-DOMPurify-runtime path; plain-text post-strip reuses it. - markdownDocument.decodeEntities: decode `&` last to stop the `&lt;` -> `<` double-unescape (js/double-escaping). - siteItemNames: drop dynamic RegExp for startsWith/slice. - scaleModule / dependencyResolver: replace -> replaceAll. - htmlPagePlan importer: harden script close-tag match. - arch gate: cms-handlers-capability-gated skips __tests__ dirs. Verified: bun run build, bun run lint, bun test (5741 pass). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL still flagged stripHtmlFallback (js/incomplete-multi-character- sanitization) because the generic `<[^>]*>` tag strip isn't credited with removing a residual `<script` / `<style` opener left by block removal. Mirror the proven server SVG sanitizer: each block regex is now paired with an explicit opener regex (`<script…>` / `<style…>`), making the dangerous substring provably gone — the same shape CodeQL already accepts in svgSanitize.ts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CodeQL's incomplete-multi-character-sanitization didn't credit the inline chained-replace loop body as a fixpoint, re-flagging the same lines. Mirror the exact structure CodeQL already accepts in svgSanitize.ts: extract the replace chain into stripHtmlOnce(), and loop `current = stripHtmlOnce(current)` to a fixpoint. Behavior unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CodeQL evaluates each .replace() independently and only credits a replace that is itself looped to a fixpoint — an outer loop around a chain, or the chain in a helper, left every individual replace flagged. Restructure stripHtmlFallback into three single-literal-regex do-while-until-stable loops (script block, style block, remaining tags), the exact shape CodeQL's qhelp documents as the complete fix. Behavior unchanged: script/style content is still removed (covered by the no-DOM server-runtime test). Co-Authored-By: Claude Opus 4.8 (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
2 participants
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.
What
Resolves CodeQL code-scanning alerts across the sanitization and string-handling surfaces.
Real security fix
server/handlers/cms/svgSanitize.ts(Remove AI credential authentication selector #30–34) — the media-upload SVG sanitizer. Close-tag regexes now match</script foo>,</script\t\n>, and</script/>(HTML parsers end a tag at the first>; the old<\/script\s*>missed these —js/bad-tag-filter). Stripping now iterates to a fixpoint instead of a fixed 2 passes, so split-tag obfuscation (<scr<script>ipt>) can't survive (js/incomplete-multi-character-sanitization). Adds__tests__/svgSanitize.test.ts— the first test on this boundary.Defense-in-depth
src/core/sanitize.ts(Add managed publishing MVP #1–5) — same close-tag fix + fixpoint loop in thestripHtmlFallbackpath (only used when no DOMPurify runtime exists); plain-text post-strip reuses it.Correctness
src/core/markdown/markdownDocument.ts(perf(canvas): kill per-node selector storms, per-frame DOM re-measures, and whole-site loop subscriptions #28) —decodeEntitiesdecodes&last, stopping the&lt;→<double-unescape (js/double-escaping).src/admin/.../siteItemNames.ts(docs(deployment): TRUSTED_PROXY_CIDRS and Railway/Render template hardening #13) — dynamicRegExp→startsWith/slice.src/core/framework/scaleModule.ts(fix(canvas): live mode shows the shared frame skeleton while hydrating #35) &server/publish/runtime/dependencyResolver.ts(Encrypt plugin secret settings at rest in a dedicated plugin_secrets table #12) —replace→replaceAll.src/core/siteImport/htmlPagePlan.ts(Remove inconsistent panel keyboard shortcuts from the rail #36) — importer script close-tag match hardened.Test infra
cms-handlers-capability-gated.test.tsskips__tests__dirs (test files aren't routes).Why
CodeQL flagged regex-based tag filtering and incomplete multi-character sanitization on the SVG upload boundary — the one path where these are genuinely security-relevant (uploads are served as
image/svg+xmland embedded in published pages). The rest are low-risk hardenings and correctness fixes.Not addressed (false positives — recommend dismissing in GitHub)
js/stack-trace-exposure(server/http.ts) — traced end-to-end: top-level catch returns a generic message, plugin routes return.messageonly, workerstackfields are log-only. No stack→response path exists.RichTextCell.tsx— preview is rendered as React text, which is HTML-escaped; not an injection sink.iframeCanvasQuery.ts— a test file. Content Outlet availability + toast layering; close outlet invariant holes #23scripts/bench/lib/report.ts— a benchmark script.Verification
bun run build✓bun run lint✓bun test✓ (5741 pass, 0 fail)🤖 Generated with Claude Code