feat: add Content-Security-Policy header and externalize theme-init#87
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (14)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughAdds a ChangesCSP Header and Script Externalization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app.py (2)
16-29: ⚡ Quick winConsider adding defense-in-depth CSP directives.
The policy covers the primary fetch directives, but adding the following would strengthen the security posture:
frame-ancestors 'none'— prevents clickjacking (similar to X-Frame-Options: DENY)base-uri 'self'— restricts<base>tag to prevent injection attacksform-action 'self'— limits form submission targetsobject-src 'none'— blocks plugins like FlashThese are not required for the current functionality but provide additional protection against common attack vectors.
🛡️ Proposed additions to CSP_POLICY
CSP_POLICY = "; ".join( [ "default-src 'self'", "script-src 'self' https://cdnjs.cloudflare.com", "style-src 'self' 'unsafe-inline' https://cdnjs.cloudflare.com", "img-src 'self' data:", "connect-src 'self'", "font-src 'self'", + "object-src 'none'", + "base-uri 'self'", + "form-action 'self'", + "frame-ancestors 'none'", ] )🤖 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 `@app.py` around lines 16 - 29, The CSP_POLICY variable in the list being joined is missing defense-in-depth security directives. Add the following four directives to the list of CSP policy strings in the join call: frame-ancestors 'none' to prevent clickjacking, base-uri 'self' to restrict base tag injection, form-action 'self' to limit form submission targets, and object-src 'none' to block plugins. Insert these as additional string elements in the list alongside the existing default-src, script-src, style-src, img-src, connect-src, and font-src directives.
16-29: Consider adding CSP reporting for production monitoring.The policy currently has no
report-uriorreport-todirective. Adding CSP violation reporting helps detect:
- Unexpected script/resource injections
- Policy misconfigurations
- Attack attempts
You can use a service like
report-uri.comor a self-hosted endpoint. Example:"report-uri https://your-domain.report-uri.com/r/d/csp/enforce"Or for newer browsers:
"report-to csp-endpoint"(Requires accompanying
Report-Toheader configuration)🤖 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 `@app.py` around lines 16 - 29, The CSP_POLICY constant is missing CSP violation reporting directives needed for production monitoring. Add a reporting capability to the policy by including either a report-uri directive with a URL to a reporting service (such as report-uri.com or a self-hosted endpoint), or a report-to directive for newer browsers (which also requires configuring a corresponding Report-To header). Include this new directive in the list being joined together in the CSP_POLICY definition.
🤖 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.
Nitpick comments:
In `@app.py`:
- Around line 16-29: The CSP_POLICY variable in the list being joined is missing
defense-in-depth security directives. Add the following four directives to the
list of CSP policy strings in the join call: frame-ancestors 'none' to prevent
clickjacking, base-uri 'self' to restrict base tag injection, form-action 'self'
to limit form submission targets, and object-src 'none' to block plugins. Insert
these as additional string elements in the list alongside the existing
default-src, script-src, style-src, img-src, connect-src, and font-src
directives.
- Around line 16-29: The CSP_POLICY constant is missing CSP violation reporting
directives needed for production monitoring. Add a reporting capability to the
policy by including either a report-uri directive with a URL to a reporting
service (such as report-uri.com or a self-hosted endpoint), or a report-to
directive for newer browsers (which also requires configuring a corresponding
Report-To header). Include this new directive in the list being joined together
in the CSP_POLICY definition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86761943-0383-43af-8f5a-7d8aeb518d70
📒 Files selected for processing (6)
app.pydocs/architecture.mdstatic/index.htmlstatic/js/hljs-theme-init.jsstatic/js/theme-init.jstests/test_api_integration.py
|
@CodeRabbit please check this PR in more depth |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Read all changed files for deep review
echo "=== app.py ===" && cat app.py
echo ""
echo "=== static/index.html ===" && cat static/index.html
echo ""
echo "=== static/js/theme-init.js ===" && cat static/js/theme-init.js
echo ""
echo "=== static/js/hljs-theme-init.js ===" && cat static/js/hljs-theme-init.js
echo ""
echo "=== static/js/app.js ===" && cat static/js/app.js
echo ""
echo "=== static/js/projects.js ===" && cat static/js/projects.js
echo ""
echo "=== static/js/search.js ===" && cat static/js/search.js
echo ""
echo "=== static/js/sessions.js ===" && cat static/js/sessions.js
echo ""
echo "=== tests/test_api_integration.py ===" && cat tests/test_api_integration.py
echo ""
echo "=== tests/test_hljs_theme_consistency.py ===" && cat tests/test_hljs_theme_consistency.py
echo ""
echo "=== docs/architecture.md ===" && cat docs/architecture.mdLength of output: 59827 🏁 Script executed: #!/bin/bash
# Check for any shared theme or export JS files referenced
echo "=== static/js/shared/theme.js ===" && cat static/js/shared/theme.js 2>/dev/null || echo "NOT FOUND"
echo ""
echo "=== static/js/export.js ===" && cat static/js/export.js 2>/dev/null || echo "NOT FOUND"
echo ""
# List all JS files
echo "=== All JS files ===" && fd -e js . static/
echo ""
# Check if there are any remaining inline event handlers in HTML
echo "=== Remaining inline handlers in index.html ===" && rg -n "on(click|keydown|load|submit)" static/index.html || echo "NONE"
echo ""
# Check for any unsafe-inline in CSP policy
echo "=== CSP policy definition ===" && rg -n "CSP_POLICY" app.pyLength of output: 8564 🏁 Script executed: #!/bin/bash
# Check test configuration / conftest
echo "=== tests/conftest.py ===" && cat tests/conftest.py 2>/dev/null || echo "NOT FOUND"
echo ""
# Check existing tests
echo "=== Full test_api_integration.py ===" && cat tests/test_api_integration.py
echo ""
# Check for vitest config or package.json
echo "=== package.json ===" && cat package.json 2>/dev/null || echo "NOT FOUND"
echo ""
# Look for any remaining window.* assignments in app.js
echo "=== window assignments in app.js ===" && rg -n "window\." static/js/app.js || echo "NONE"
echo ""
# Check for any SRI hashes in index.html
echo "=== SRI integrity attributes in index.html ===" && rg -n "integrity" static/index.html
echo ""
# Check whether hljs-theme-init.js SRI hash matches index.html
echo "=== SRI hash in hljs-theme-init.js ===" && rg -n "integrity" static/js/hljs-theme-init.jsLength of output: 8900
Overall ImpressionThis is a well-structured CSP implementation. The inline-script externalization is done correctly, SRI hashes are consistent across all three locations (verified), inline 🔴 Security — Missing
|
| Area | Note |
|---|---|
| SRI hash consistency | Dark hash matches between index.html and shared/theme.js; light hash matches between hljs-theme-init.js and shared/theme.js — the regression tests lock this invariant |
| Script load ordering | theme-init.js → hljs-theme-init.js → CDN scripts — correct for synchronous pre-paint execution |
integrity before href |
Both hljs-theme-init.js and applyHljsTheme() set integrity first, which is the correct browser behaviour |
| No residual inline handlers | Confirmed zero onclick/onkeydown attributes remaining in index.html |
base-uri 'self' + frame-ancestors 'none' |
These two are often forgotten; good to see both present |
| Export button wiring | bindProjectsExportButtons correctly defers event attachment until after smoothSet |
| Optional chaining | Consistently applied across projects.js, sessions.js, app.js for all newly bound handlers |
|
@CodeRabbit. all are addressed now? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== CSP_POLICY in app.py ===" && rg -n "CSP_POLICY|after_request|setdefault|headers.set" app.py
echo ""
echo "=== Full CSP_POLICY block ===" && sed -n '/^CSP_POLICY/,/^)/p' app.py
echo ""
echo "=== after_request hook ===" && rg -n -A5 "after_request" app.py
echo ""
echo "=== test_api_integration.py CSP tests ===" && rg -n "csp\|CSP\|api/projects" tests/test_api_integration.py
echo ""
echo "=== test_hljs_theme_consistency.py - APP_JS/THEME_JS ===" && rg -n "APP_JS\|THEME_JS" tests/test_hljs_theme_consistency.py
echo ""
echo "=== search.js focus line ===" && rg -n "focus" static/js/search.js
echo ""
echo "=== app.js loadingBar import ===" && rg -n "loadingBar" static/js/app.jsLength of output: 2243 🏁 Script executed: #!/bin/bash
echo "=== CSP test coverage in test_api_integration.py ===" && grep -n "CSP\|csp\|api/projects\|api_routes" tests/test_api_integration.py
echo ""
echo "=== THEME_JS / APP_JS in test_hljs_theme_consistency.py ===" && grep -n "APP_JS\|THEME_JS" tests/test_hljs_theme_consistency.pyLength of output: 1310
Everything looks good — the implementation is solid. 🐇 |
clean6378-max-it
force-pushed
the
feat/csp-header
branch
from
f94064f to
67ef87e
Compare
3 participants
Closes #79
Summary
Content-Security-Policyheader on every Flask response via@app.after_requestincreate_app(), using a module-levelCSP_POLICYconstant inapp.py.<script>blocks instatic/index.htmltostatic/js/theme-init.js(theme before first paint) andstatic/js/hljs-theme-init.js(initial light-mode hljs stylesheet), keepingscript-srcfree of'unsafe-inline'.test_root_sets_csp_headerintests/test_api_integration.pyassertingContent-Security-PolicyonGET /.docs/architecture.md.Test plan
python -m pytest tests/test_api_integration.py -qpython -m pytest -qnpm testcurl -I http://127.0.0.1:5000/showsContent-Security-Policywithdefault-src 'self'Summary by CodeRabbit
Security
Improvements
Documentation
Tests