Strengthen CSP by removing unsafe-inline and unsafe-eval from script-src#393
Open
tracygardner wants to merge 4 commits intomainfrom
Open
Strengthen CSP by removing unsafe-inline and unsafe-eval from script-src#393tracygardner wants to merge 4 commits intomainfrom
tracygardner wants to merge 4 commits intomainfrom
Conversation
- Move Google Analytics init from inline <script> to ga-init.js served from self, eliminating the only inline script in index.html - Remove new Function(code) syntax check from validateCode(); the AST-based validateUserCodeAST() already provides a stricter check - Drop 'unsafe-inline' and 'unsafe-eval' from script-src in both the Vite header policy and the index.html meta fallback - 'wasm-unsafe-eval' is retained for Draco/Manifold WASM compilation - style-src 'unsafe-inline' is retained (Blockly dynamic inline styles) - The sandbox iframe keeps its own internal CSP with unsafe-eval; that document is separate from the parent page's policy - Update docs/CSP_POLICY.md to reflect the tightened policy https://claude.ai/code/session_01538UMQMjCETL6HohuTeaDC
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughTightens Content-Security-Policy (removes inline/eval script allowances, adds Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Menu as AccessibleFlyoutMenu
participant Modal as AboutModal
participant Doc as Document
User->>Menu: activate main menu (click/keyboard)
Menu->>Menu: toggleMainMenu() / manage focus
Menu-->>Doc: set aria-expanded / show menu
User->>Menu: navigate items (Arrow keys)
Menu->>Menu: focusMenuItem() / handleMenuItemKeydown()
User->>Menu: activate "About" item
Menu->>Modal: open modal (trap focus)
Modal->>Doc: apply modal state (no-scroll, aria-hidden others)
User->>Modal: Tab / Shift+Tab
Modal->>Modal: cycle focus inside modal
User->>Modal: press Escape or close
Modal->>Menu: close modal / restore focus
Modal-->>Doc: remove modal state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/CSP_POLICY.md`:
- Around line 60-63: The security rationale incorrectly states that `script-src`
no longer requires `'unsafe-inline'` because only GA was moved to `ga-init.js`;
update the CSP_POLICY.md text to accurately reflect that other inline
scripts/handlers still exist in `index.html` (so `'unsafe-inline'` may still be
required) or explicitly state that full externalization is pending; reference
the moved GA script (`ga-init.js`), the remaining inline handlers in
`index.html`, and the removed `new Function(code)` check in `validateCode()` vs
the AST check in `validateUserCodeAST()` so readers understand which changes
affected CSP and which did not.
In `@index.html`:
- Line 7: The meta Content-Security-Policy currently blocks inline
scripts/handlers used in this document (see inline scripts at lines ~160 and
~430 and an inline handler at ~997); to fix, either fully externalize those
inline scripts/handlers and remove inline usage from the page, or add a
temporary migration exception in the meta CSP's content string (the shown
"script-src ...") by including 'unsafe-inline' or, preferably, by computing and
adding sha256 hashes for each inline script or using a nonce approach (add a
nonce attribute to each inline script/handler and include the same nonce token
in the CSP) so the existing inline code continues to run until you finish
externalizing it.
In `@vite.config.mjs`:
- Line 13: The CSP_META_POLICY constant removal of 'unsafe-inline' in the CSP
string will block existing inline scripts and inline event handlers referenced
in index.html (e.g., the inline bootstraps and handlers noted), so either
temporarily restore the script-src 'unsafe-inline' token in CSP_META_POLICY to
avoid breaking menu/modal/debug/bootstrap flows, or preferably move all inline
JavaScript and inline handlers into external JS files (or replace them with
nonce- or sha256-based CSP entries) and update the CSP_META_POLICY accordingly;
locate and modify the CSP_META_POLICY declaration and the inline
scripts/handlers in index.html to implement the chosen fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e0048da-6603-4803-b124-388c6d3ab347
📒 Files selected for processing (5)
docs/CSP_POLICY.mdflock.jsga-init.jsindex.htmlvite.config.mjs
💤 Files with no reviewable changes (1)
- flock.js
All inline JS in index.html has now been eliminated so that 'unsafe-inline' can be removed from script-src: - Extract touch-debug overlay IIFE → touch-debug.js (sync, early) - Extract menu/modal/About JS → menu.js (defer + DOMContentLoaded) - Adds let previouslyFocused declaration (was implicit global) - Replace onclick="selectShape(...)" on shape buttons with data-shape attributes; delegate clicks in menu.js via #shape-row event listener calling window.selectShape - Register touch-debug.js and menu.js in viteStaticCopy so they appear in dist/ on build After this commit index.html contains no inline <script> blocks and no inline event handlers (onclick/onerror), satisfying the script-src 'self' policy without 'unsafe-inline'. https://claude.ai/code/session_01538UMQMjCETL6HohuTeaDC
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
index.html (1)
398-398: Script placement is valid but unconventional.The
<script>tag placed inside a<div>is valid HTML5, though typically scripts are placed at document level. Withdefer, the script location doesn't affect execution timing. TheDOMContentLoadedwrapper inmenu.jsis slightly redundant sincedeferalready guarantees DOM readiness, but it's harmless.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.html` at line 398, Move the <script src="menu.js" defer></script> out of the <div> and place it at document level (preferably just before </body> or in <head>) to follow conventional script placement; then open menu.js and remove the redundant DOMContentLoaded wrapper (the "DOMContentLoaded" event listener) because defer already guarantees DOM readiness, leaving the module's initialization code to run directly.menu.js (2)
2-7: Consider adding null checks for DOM element lookups.If any of these elements are missing from the DOM (e.g., during testing or if IDs change), the code will throw. The
AccessibleFlyoutMenuconstructor immediately callsquerySelectorAllonmenuDropdown(line 14), which will fail ifmenuDropdownis null.🛡️ Proposed defensive check
const menuBtn = document.getElementById("menuBtn"); const menuDropdown = document.getElementById("menuDropdown"); + if (!menuBtn || !menuDropdown) { + console.warn('Menu elements not found, skipping menu initialization'); + return; + } const openAbout = document.getElementById("about-menu-item");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@menu.js` around lines 2 - 7, The DOM lookups for menuBtn, menuDropdown, openAbout, hubMenuItem, infoModal, and closeInfoModal must be null-checked before use: ensure you verify each const (especially menuDropdown) is non-null before calling methods like querySelectorAll or instantiating AccessibleFlyoutMenu; if any required element is missing, either return early or throw/log a clear error mentioning the missing element(s) so the constructor is not called on null. Update the code paths around the AccessibleFlyoutMenu creation and any subsequent calls that assume these elements exist to guard against null and handle the fallback behavior gracefully.
366-372: Duplicate Escape key handling.This document-level Escape handler duplicates functionality already present in the
AccessibleFlyoutMenuclass (lines 82-87). While both handlers produce the same effect (close menu, focus button), consider consolidating to avoid redundancy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@menu.js` around lines 366 - 372, There’s a duplicate Escape key handler: the document-level listener that checks e.key === "Escape" and manipulates menuDropdown/menuBtn replicates logic already implemented in the AccessibleFlyoutMenu class (the handler around lines 82-87); remove the redundant document.addEventListener("keydown", ...) block and instead ensure the AccessibleFlyoutMenu instance handles Escape for the same elements (or hook the instance’s close/focus method into a single keydown listener) so only AccessibleFlyoutMenu manages closing the menu and focusing menuBtn.touch-debug.js (1)
1-12: Potential timing issue:document.body.appendChildcalled before DOM is fully parsed.The script runs immediately as an IIFE. While placed early in
<body>(line 160 of index.html),document.bodyexists at that point, but if the script were ever moved to<head>, this would fail. Consider adding a defensive check or deferring the DOM mutation.🛡️ Optional: Add defensive check
- document.body.appendChild(overlay); + (document.body || document.documentElement).appendChild(overlay);Or wrap in a DOMContentLoaded listener if the overlay can be deferred:
+ const attach = () => document.body.appendChild(overlay); + if (document.body) attach(); + else document.addEventListener('DOMContentLoaded', attach);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@touch-debug.js` around lines 1 - 12, The IIFE currently calls document.body.appendChild(overlay) immediately which can fail if run before DOM is parsed; modify the IIFE to defer the DOM mutation by checking for document.body and, if absent, registering a DOMContentLoaded listener that appends overlay and registers the touchstart handler; reference the existing overlay variable, the touchstart listener registration, and the document.body.appendChild call so you only move/guard the append and listener setup (do not change the overlay styling or event logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@menu.js`:
- Around line 249-258: The declaration const submenu in the 'ArrowLeft' switch
case is leaking scope across other cases and triggers static-analysis warnings;
fix it by wrapping the entire 'ArrowLeft' case body in its own block (add { ...
} after case 'ArrowLeft':) so that submenu is block-scoped, keeping the existing
calls to e.preventDefault(), e.stopPropagation(), parentItem.focus(), the
submenu.hidden assignment and parentItem.setAttribute('aria-expanded', 'false')
inside that block.
---
Nitpick comments:
In `@index.html`:
- Line 398: Move the <script src="menu.js" defer></script> out of the <div> and
place it at document level (preferably just before </body> or in <head>) to
follow conventional script placement; then open menu.js and remove the redundant
DOMContentLoaded wrapper (the "DOMContentLoaded" event listener) because defer
already guarantees DOM readiness, leaving the module's initialization code to
run directly.
In `@menu.js`:
- Around line 2-7: The DOM lookups for menuBtn, menuDropdown, openAbout,
hubMenuItem, infoModal, and closeInfoModal must be null-checked before use:
ensure you verify each const (especially menuDropdown) is non-null before
calling methods like querySelectorAll or instantiating AccessibleFlyoutMenu; if
any required element is missing, either return early or throw/log a clear error
mentioning the missing element(s) so the constructor is not called on null.
Update the code paths around the AccessibleFlyoutMenu creation and any
subsequent calls that assume these elements exist to guard against null and
handle the fallback behavior gracefully.
- Around line 366-372: There’s a duplicate Escape key handler: the
document-level listener that checks e.key === "Escape" and manipulates
menuDropdown/menuBtn replicates logic already implemented in the
AccessibleFlyoutMenu class (the handler around lines 82-87); remove the
redundant document.addEventListener("keydown", ...) block and instead ensure the
AccessibleFlyoutMenu instance handles Escape for the same elements (or hook the
instance’s close/focus method into a single keydown listener) so only
AccessibleFlyoutMenu manages closing the menu and focusing menuBtn.
In `@touch-debug.js`:
- Around line 1-12: The IIFE currently calls document.body.appendChild(overlay)
immediately which can fail if run before DOM is parsed; modify the IIFE to defer
the DOM mutation by checking for document.body and, if absent, registering a
DOMContentLoaded listener that appends overlay and registers the touchstart
handler; reference the existing overlay variable, the touchstart listener
registration, and the document.body.appendChild call so you only move/guard the
append and listener setup (do not change the overlay styling or event logic).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c9fbe00-4488-4b26-a9e9-637683a9eec2
📒 Files selected for processing (4)
index.htmlmenu.jstouch-debug.jsvite.config.mjs
Comment on lines
+249
to
+258
| case 'ArrowLeft': | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| parentItem.focus(); | ||
| const submenu = parentItem.querySelector('.submenu'); | ||
| if (submenu) { | ||
| submenu.hidden = true; | ||
| parentItem.setAttribute('aria-expanded', 'false'); | ||
| } | ||
| break; |
There was a problem hiding this comment.
Wrap const submenu declaration in a block to fix static analysis warning.
The const submenu declaration on line 253 is accessible from other switch clauses, which can lead to unexpected behavior. Biome correctly flags this.
🔧 Proposed fix
case 'ArrowLeft':
+ {
e.preventDefault();
e.stopPropagation();
parentItem.focus();
const submenu = parentItem.querySelector('.submenu');
if (submenu) {
submenu.hidden = true;
parentItem.setAttribute('aria-expanded', 'false');
}
break;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case 'ArrowLeft': | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| parentItem.focus(); | |
| const submenu = parentItem.querySelector('.submenu'); | |
| if (submenu) { | |
| submenu.hidden = true; | |
| parentItem.setAttribute('aria-expanded', 'false'); | |
| } | |
| break; | |
| case 'ArrowLeft': | |
| { | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| parentItem.focus(); | |
| const submenu = parentItem.querySelector('.submenu'); | |
| if (submenu) { | |
| submenu.hidden = true; | |
| parentItem.setAttribute('aria-expanded', 'false'); | |
| } | |
| break; | |
| } |
🧰 Tools
🪛 Biome (2.4.6)
[error] 253-253: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@menu.js` around lines 249 - 258, The declaration const submenu in the
'ArrowLeft' switch case is leaking scope across other cases and triggers
static-analysis warnings; fix it by wrapping the entire 'ArrowLeft' case body in
its own block (add { ... } after case 'ArrowLeft':) so that submenu is
block-scoped, keeping the existing calls to e.preventDefault(),
e.stopPropagation(), parentItem.focus(), the submenu.hidden assignment and
parentItem.setAttribute('aria-expanded', 'false') inside that block.
The sandbox iframe uses srcdoc + allow-same-origin, which causes the parent page's CSP to be enforced on scripts injected into the iframe. The parent's script-src no longer includes 'unsafe-inline', so the previous pattern of fetching lockdown.umd.min.js and setting sesScript.text = <content> was being blocked. Replace with sesScript.src pointing to the already-served vendor/ses/lockdown.umd.min.js file (same origin → allowed by script-src 'self'). The script element now loads via URL, so win.lockdown() is called in the onload callback via await. Also disable Vite's build.modulePreload.polyfill to prevent an additional inline <script> from being injected into the built HTML. https://claude.ai/code/session_01538UMQMjCETL6HohuTeaDC
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flock.js`:
- Around line 666-683: The iframe Content-Security-Policy used when creating the
iframe must include 'self' in its script-src so the externally-loaded SES script
(sesScript.src -> "vendor/ses/lockdown.umd.min.js" resolved via
document.baseURI) is allowed; update the CSP string/attribute that sets
script-src (where you currently have "script-src 'unsafe-inline' 'unsafe-eval'")
to include 'self' (e.g., "script-src 'self' 'unsafe-inline' 'unsafe-eval'"), and
verify the vendor/ses/lockdown.umd.min.js file is present in the build output so
the dynamic load via doc.createElement("script") succeeds.
Comment on lines
+666
to
+683
| // Load SES lockdown via src= so the parent's script-src 'self' policy | ||
| // is satisfied — injecting inline text would require 'unsafe-inline'. | ||
| await new Promise((resolve, reject) => { | ||
| const sesScript = doc.createElement("script"); | ||
| sesScript.type = "text/javascript"; | ||
| sesScript.onload = resolve; | ||
| sesScript.onerror = () => | ||
| reject( | ||
| new Error( | ||
| "Failed to load SES lockdown", | ||
| ), | ||
| ); | ||
| sesScript.src = new URL( | ||
| "vendor/ses/lockdown.umd.min.js", | ||
| document.baseURI, | ||
| ).href; | ||
| doc.head.appendChild(sesScript); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify iframe CSP and SES external loading code paths
rg -n -C2 "const csp =|script-src|sesScript\\.src|lockdown\\.umd\\.min\\.js" flock.jsRepository: flipcomputing/flock
Length of output: 1005
🏁 Script executed:
#!/bin/bash
# Check CSP application context and SES script location
echo "=== Checking CSP context (around line 1176) ==="
sed -n '1170,1185p' flock.js
echo -e "\n=== Checking if vendor/ses/lockdown.umd.min.js exists ==="
find . -name "lockdown.umd.min.js" 2>/dev/null || echo "File not found in current search"
echo -e "\n=== Checking SES loading code context (around line 666) ==="
sed -n '660,690p' flock.jsRepository: flipcomputing/flock
Length of output: 2720
🏁 Script executed:
#!/bin/bash
# Check vendor directory and SES files
echo "=== Checking for vendor directory ==="
ls -la vendor/ 2>/dev/null || echo "vendor/ directory does not exist"
echo -e "\n=== Searching for any SES-related files ==="
find . -iname "*ses*" -o -iname "*lockdown*" 2>/dev/null | head -20
echo -e "\n=== Checking package.json for SES dependency ==="
grep -i "ses" package.json 2>/dev/null || echo "No SES mention in package.json"
echo -e "\n=== Checking if this code path is executed/tested ==="
rg -n "replaceSandboxIframe\|lockdown\(\)" flock.js | head -10Repository: flipcomputing/flock
Length of output: 316
🏁 Script executed:
#!/bin/bash
# Check where replaceSandboxIframe is defined and called
echo "=== Finding replaceSandboxIframe definition ==="
rg -n "replaceSandboxIframe\s*[=:(]" flock.js | head -5
echo -e "\n=== Checking SES npm package installation ==="
ls -la node_modules/ses 2>/dev/null | head -5 || echo "SES not in node_modules"
echo -e "\n=== Checking actual SES file locations ==="
find node_modules/ses -name "lockdown*" 2>/dev/null | head -10
echo -e "\n=== Checking if vendor is built/created elsewhere ==="
rg -n "vendor.*ses\|lockdown.*umd" --type js | head -10Repository: flipcomputing/flock
Length of output: 382
CSP blocks the SES script load — add 'self' to script-src.
The iframe CSP at line 1176 is script-src 'unsafe-inline' 'unsafe-eval', which blocks external scripts. Line 666-683 loads SES via sesScript.src, requiring the 'self' directive to allow same-origin external scripts. The promise will reject with a CSP violation.
Add 'self' to the script-src policy:
Minimal fix
-const csp = `default-src 'none'; style-src 'unsafe-inline'; script-src 'unsafe-inline' 'unsafe-eval'`;
+const csp = `default-src 'none'; style-src 'unsafe-inline'; script-src 'self' 'unsafe-inline' 'unsafe-eval'`;Note: Verify the SES file path vendor/ses/lockdown.umd.min.js exists in your build output, as the vendor directory is not in the source repo. The SES package is listed as a dependency ("ses": "^1.14.0").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flock.js` around lines 666 - 683, The iframe Content-Security-Policy used
when creating the iframe must include 'self' in its script-src so the
externally-loaded SES script (sesScript.src -> "vendor/ses/lockdown.umd.min.js"
resolved via document.baseURI) is allowed; update the CSP string/attribute that
sets script-src (where you currently have "script-src 'unsafe-inline'
'unsafe-eval'") to include 'self' (e.g., "script-src 'self' 'unsafe-inline'
'unsafe-eval'"), and verify the vendor/ses/lockdown.umd.min.js file is present
in the build output so the dynamic load via doc.createElement("script")
succeeds.
InitializeCSG2Async calls _LoadScriptModuleAsync with an inline ES module string that imports from unpkg.com. This creates a <script type="module"> with inline textContent which violates script-src 'unsafe-inline'. Fix: import manifold-3d directly and call ManifoldInit() ourselves, then pass manifoldInstance and manifoldMeshInstance to InitializeCSG2Async. This takes the localOptions.manifoldInstance branch in BabylonJS's code, which skips _LoadScriptModuleAsync entirely. locateFile redirects WASM lookup to ./wasm/manifold.wasm, matching the path where viteStaticCopy places the file in the build output. https://claude.ai/code/session_01538UMQMjCETL6HohuTeaDC
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.
Summary
This PR significantly strengthens the Content Security Policy (CSP) by removing
'unsafe-inline'and'unsafe-eval'fromscript-src, while maintaining all required functionality. The changes eliminate inline scripts and replace runtime code evaluation with safer alternatives.Key Changes
Removed CSP directives: Eliminated
'unsafe-inline'and'unsafe-eval'fromscript-srcacross all configuration files (index.html,vite.config.mjs, andCSP_POLICY.md)Externalized Google Analytics: Moved inline GA initialization script from
<script>block inindex.htmlto a new external filega-init.js, served fromselfReplaced code validation: Removed the
new Function(code)syntax check inflock.jsthat relied on'unsafe-eval'. The existing AST-basedvalidateUserCodeAST()already provides stricter validation before code reaches the sandboxUpdated CSP documentation: Clarified that:
'unsafe-inline'instyle-srcremains necessary for Blockly's runtime-injected styles'wasm-unsafe-eval'alonescript-src 'unsafe-inline' 'unsafe-eval') for user-code executionBuild configuration: Added
ga-init.jsto the Vite static copy targets to ensure it's included in the build outputImplementation Details
The removal of
'unsafe-eval'is safe because:eval-like behavior) is explicitly allowed via'wasm-unsafe-eval'This change reduces the application's attack surface while maintaining full functionality.
https://claude.ai/code/session_01538UMQMjCETL6HohuTeaDC
Summary by CodeRabbit
Security
New Features
Improvements