feat: DOMPurify XSS hardening + ES module split for static JS#36
Merged
Conversation
… app.js into modules Sanitize all session markdown before innerHTML assignment via a single renderMarkdown() wrapper in shared/markdown.js that applies DOMPurify.sanitize(marked.parse(...)). DOMPurify 3.2.7 loaded from CDN with SRI and crossorigin="anonymous" (issue #295). Split the 955-line app.js monolith into six route/view modules: sessions.js, projects.js, search.js, export.js, four shared modules (state, utils, markdown, theme), and a thin app.js bootstrap with window registrations for inline handlers. Entry switched to <script type="module"> (no build step). Add tests/test_xss_sanitization.py (8 source-level regression tests) and update test_hljs_theme_consistency.py for the HLJS_THEME_SHEETS move to shared/theme.js. All 229 tests pass.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughConverts the frontend to ES modules, adds shared state/utils/theme and a safe markdown renderer (DOMPurify), implements Projects/Search/Sessions/Export client routes, updates index.html to load DOMPurify and module app entry, adds Projects header CSS, and introduces tests enforcing theme and XSS sanitization. ChangesFrontend feature cohort
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
static/js/app.js (1)
19-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard route decoding against malformed hash input.
Line 23 and Line 34 call
decodeURIComponentdirectly; malformed hash fragments can throw and break navigation.Proposed fix
+function safeDecode(value) { + try { return decodeURIComponent(value); } + catch { return null; } +} + function handleRoute() { @@ - const project = decodeURIComponent(parts.slice(0, slashIdx)); + const project = safeDecode(parts.slice(0, slashIdx)); + if (project === null) { showProjects(); return; } @@ - showWorkspace(decodeURIComponent(parts)); + const project = safeDecode(parts); + if (project === null) { showProjects(); return; } + showWorkspace(project);🤖 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 `@static/js/app.js` around lines 19 - 35, The route parsing calls decodeURIComponent on untrusted hash fragments (in the project branch where parts are decoded for project and in the else branch) which can throw on malformed input; wrap the decode steps in a safe decoder and handle failures by falling back to a sane value or showing an error/redirect. Specifically, replace direct decodeURIComponent usages around parts.slice(0, slashIdx) and parts (used by showWorkspace and loadSession) with a try/catch or a helper like safeDecode(str) that returns null or a sanitized string on URIError, and then branch: if decoding fails, call showWorkspace with a fallback or abort navigation, otherwise proceed to call loadSession(project, sessionId) or showWorkspace(project).
🧹 Nitpick comments (1)
static/js/search.js (1)
41-43: ⚡ Quick winHandle non-2xx responses explicitly before parsing JSON.
Without an
res.okcheck, backend failures often surface as generic parse/type errors instead of actionable messages.🛠️ Suggested fix
try { const res = await fetch(`/api/search?q=${encodeURIComponent(query)}&limit=50`); + if (!res.ok) { + throw new Error(`Search failed (${res.status})`); + } const results = await res.json();🤖 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 `@static/js/search.js` around lines 41 - 43, The fetch result is parsed without checking HTTP status, causing opaque JSON parse errors on backend failures; update the fetch handling around the const res = await fetch(...) / const results = await res.json() block to first check res.ok and, if false, read the response text or status (e.g. await res.text() or res.statusText) and throw a descriptive Error so callers can handle it, otherwise proceed to await res.json() to obtain results.
🤖 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 `@static/js/export.js`:
- Around line 74-76: The current code calls handle.createWritable(), then awaits
writable.write(blob) and writable.close(), but if writable.write() (or close)
throws the writable stays in a bad state; wrap the writable usage in
try/catch/finally: call handle.createWritable() to get writable, perform await
writable.write(blob) and await writable.close() inside try, and in the catch
block call await writable.abort() (or await writable.close() if abort is
unavailable) to discard partial writes, rethrow or handle the error as needed;
reference the functions handle.createWritable(), writable.write(),
writable.close(), and writable.abort() when making the change.
- Around line 52-53: The fetch call building the download URL encodes project
but not sessionId, so update the URL construction in the fetch invocation (the
fetch(...) that references project and sessionId) to wrap sessionId with
encodeURIComponent as well (i.e., encodeURIComponent(sessionId)) so path
parameters are consistently encoded and reserved characters in sessionId won't
break routing.
In `@static/js/projects.js`:
- Around line 23-33: The code parses projRes.json() unconditionally which can
turn a backend error into an empty projects array and show the wrong UI; change
the flow to check projRes.ok first (after the Promise.all) and handle non-2xx
responses by reading the response body (try json then fallback to text), call
loadingBar.done(), and render an error state via smoothSet (including status and
error message), then return; only if projRes.ok proceed to await projRes.json()
and continue with the existing empty-state check. Ensure you reference projRes,
stateRes, projects, smoothSet and loadingBar.done in the fix.
- Around line 47-49: The server-derived value sessionCount (used when building
lastExportHtml) is interpolated directly into HTML and needs to be sanitized:
coerce sessionCount to a safe numeric value (e.g. parseInt/Number and fallback
to 0 if NaN, and clamp to a non-negative integer) before using it, or avoid
string interpolation into innerHTML by building the element and inserting the
count via textContent; update the code around exportState, sessionCount and
lastExportHtml to apply the coercion/escape so only a validated numeric string
is injected.
In `@static/js/search.js`:
- Around line 47-49: The onclick string builds a hash with r.session_id
unescaped which allows injection; update the inline handler in
static/js/search.js where results are iterated (variable r) so that the session
id is encoded before interpolation (use encodeURIComponent on r.session_id,
matching the existing encodeURIComponent(r.project)) and ensure the final string
remains wrapped in the existing quotes to avoid breaking the attribute.
In `@static/js/sessions.js`:
- Line 99: The hash route and fetch URL are using raw sessionId which can
contain characters that break routing; update the places that build the route
and API call (the window.location.hash assignment and the fetch that references
sessionId) to wrap sessionId with encodeURIComponent(sessionId) (i.e., use
encodeURIComponent for the sessionId segment when constructing
`window.location.hash =
\`#project/${encodeURIComponent(projectName)}/${encodeURIComponent(sessionId)}\``
and when composing the fetch path) so all unsafe characters are percent-encoded.
- Line 57: The inline onclick handlers build a JS string using HTML-escaped
values (esc(projectName) and esc(s.id)) which is unsafe because HTML-escaping
does not escape JS-quote characters; update the sidebar markup generation in
static/js/sessions.js to stop passing raw escaped strings into onclick. Instead
attach data attributes (e.g., data-project-name and data-session-id on the
button using proper HTML-escaping) and bind selectSession via addEventListener
(or use JSON.stringify(...) when inlining JS literals) when creating the
element; update the code paths that build the button (the template building code
that includes onclick="selectSession(...)" and the similar occurrence around
lines 154-155) to use the data-attribute + event listener approach (or
JSON.stringify) and call selectSession(element.dataset.projectName,
element.dataset.sessionId) from the listener.
- Around line 395-398: The copyAll() handler is querying the wrong DOM selector
('.messages-container') that this module never renders, so the Copy button does
nothing; update copyAll() to target the actual session DOM used here
('#session-content' or its child '.session-content-inner'), e.g. locate the
container by document.querySelector('#session-content') or
document.querySelector('.session-content-inner'), fall back to the alternative
selector if needed, read its innerText, and then call
navigator.clipboard.writeText(text). Also adjust any variable names (msgs ->
sessionEl or similar) inside copyAll() to reflect the new selector and keep the
existing showToast('Copied to clipboard', 'success') behavior.
In `@static/js/shared/markdown.js`:
- Around line 39-45: Remove the unsanitized early return when DOMPurify is
missing: in the block that checks "if (typeof DOMPurify !== 'undefined')" do not
console.warn then "return parsed"; instead throw an error (e.g. throw new
Error('DOMPurify not available')) so execution falls into the existing catch
handler and the code uses the escaped fallback. Reference DOMPurify and the
local variable parsed (inside the renderMarkdown flow) so you modify that branch
to throw rather than return unsanitized output.
In `@static/js/shared/utils.js`:
- Around line 58-63: The showToast function currently injects unescaped message
into toast.innerHTML which permits XSS; update showToast (and the similar code
block around lines 77-83) to stop using innerHTML for user content: create the
toast container and its child elements (icon span, text span, close button,
progress div) via document.createElement and set the message using textContent
(or otherwise escape the message) instead of interpolating into innerHTML,
preserving existing class names (toast, toast-icon, toast-text, toast-close,
toast-progress) and behavior.
- Around line 39-41: The function formatSize incorrectly treats 0 as missing
because it uses a falsy check; change the guard in formatSize to only treat
null/undefined as unknown (e.g., use bytes == null or bytes === null || bytes
=== undefined) so that 0 is preserved and returns "0 B"; keep the rest of the
logic (bytes < 1024 -> bytes + ' B') unchanged and reference the function name
formatSize when making the change.
- Around line 13-37: formatTs and formatDate silently produce "Invalid Date"/NaN
output because new Date(ts) can be invalid without throwing; update both
functions to validate the Date after construction (e.g., check
isNaN(d.getTime()) or Number.isNaN(d.getTime())) and return the intended
fallback (for formatTs return the original ts, for formatDate return ts ?
ts.slice(0,10) : '') when the date is invalid; keep the existing try/catch but
perform the explicit invalid-date check at the start of formatTs and formatDate
to avoid rendering NaN values.
In `@tests/test_xss_sanitization.py`:
- Line 117: The string expression uses an unnecessary f-string prefix (F541):
change the expression f"shared/markdown.js:\n " + "\n ".join(violations) to a
plain string "shared/markdown.js:\n " + "\n ".join(violations) (i.e., remove
the leading 'f') so the code is a regular concatenation with the violations
variable.
---
Outside diff comments:
In `@static/js/app.js`:
- Around line 19-35: The route parsing calls decodeURIComponent on untrusted
hash fragments (in the project branch where parts are decoded for project and in
the else branch) which can throw on malformed input; wrap the decode steps in a
safe decoder and handle failures by falling back to a sane value or showing an
error/redirect. Specifically, replace direct decodeURIComponent usages around
parts.slice(0, slashIdx) and parts (used by showWorkspace and loadSession) with
a try/catch or a helper like safeDecode(str) that returns null or a sanitized
string on URIError, and then branch: if decoding fails, call showWorkspace with
a fallback or abort navigation, otherwise proceed to call loadSession(project,
sessionId) or showWorkspace(project).
---
Nitpick comments:
In `@static/js/search.js`:
- Around line 41-43: The fetch result is parsed without checking HTTP status,
causing opaque JSON parse errors on backend failures; update the fetch handling
around the const res = await fetch(...) / const results = await res.json() block
to first check res.ok and, if false, read the response text or status (e.g.
await res.text() or res.statusText) and throw a descriptive Error so callers can
handle it, otherwise proceed to await res.json() to obtain results.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce9915e4-7db1-4ef7-807e-2c26feb750a1
📒 Files selected for processing (12)
static/index.htmlstatic/js/app.jsstatic/js/export.jsstatic/js/projects.jsstatic/js/search.jsstatic/js/sessions.jsstatic/js/shared/markdown.jsstatic/js/shared/state.jsstatic/js/shared/theme.jsstatic/js/shared/utils.jstests/test_hljs_theme_consistency.pytests/test_xss_sanitization.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@static/js/search.js`:
- Around line 31-64: The doSearch function can be overwritten by out-of-order
async responses; add a request token guard: introduce a module-scoped increasing
counter (e.g., lastSearchRequestId) that you increment at the start of doSearch
and capture as localRequestId, then after each await (both successful fetch/json
and in the catch) check that localRequestId === lastSearchRequestId before
updating the DOM (container.innerHTML or smoothSet). Reference doSearch, the
fetch/res.json handling and the catch block to apply the guard so stale
responses are ignored.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91670e0c-29fc-4ed5-af85-27f7fcb82590
📒 Files selected for processing (8)
static/js/app.jsstatic/js/export.jsstatic/js/projects.jsstatic/js/search.jsstatic/js/sessions.jsstatic/js/shared/markdown.jsstatic/js/shared/utils.jstests/test_xss_sanitization.py
🚧 Files skipped from review as they are similar to previous changes (6)
- static/js/export.js
- tests/test_xss_sanitization.py
- static/js/projects.js
- static/js/sessions.js
- static/js/app.js
- static/js/shared/utils.js
10 tasks
timon0305
approved these changes
May 15, 2026
Collaborator
is there any way to show in same line?
|
wpak-ai
approved these changes
May 15, 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
3 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.
feat(web): DOMPurify session markdown + split static JS into ES modules
Session HTML was built with
marked.parse()and assigned viainnerHTMLwithout sanitizing model or user content, which is an XSS risk. This change loads DOMPurify from cdnjs (same SRI +crossorigin="anonymous"pattern as marked and highlight.js) and routes all session markdown throughstatic/js/shared/markdown.js, whererenderMarkdown()returnsDOMPurify.sanitize(marked.parse(text, { breaks: true, gfm: true }))so injected markup cannot run scripts or handlers.The former ~950-line
static/js/app.jsis split into ES modules with no build step: a thinapp.jsentry (<script type="module">) handles routing and registerswindowglobals for existing inlineonclickhandlers;projects.js,sessions.js,search.js, andexport.jsown the primary views;shared/state.js,shared/utils.js,shared/markdown.js, andshared/theme.jshold cross-cutting state, DOM helpers, sanitized markdown, and highlight.js theme swapping (HLJS_THEME_SHEETSmoved here so it stays paired withindex.htmlSRI viatests/test_hljs_theme_consistency.py).New
tests/test_xss_sanitization.pyasserts DOMPurify is present inindex.htmlwith integrity and crossorigin, that onlyshared/markdown.jscallsmarked.parse, and that no file assigns a rawmarked.parse(...)intoinnerHTML. A small follow-up fix importssetHamburgerVisiblefromshared/utils.jsinprojects.jsandsearch.js(it was incorrectly imported fromtheme.js).Test plan: run
pytest(full suite). Manually open the app, load projects, open a session with markdown, code blocks, thinking, and tool output, run search and zero-hit search, exercise export and session download, and spot-check that obvious XSS snippets in message text render safely without executing.Summary by CodeRabbit
New Features
Bug Fixes
Tests