fix(security): shorten JWT lifetime to 24h + add web-UI CSP (#657)#709
Conversation
Defense-in-depth for the localStorage-stored auth JWT (audit finding M3). Takes the acceptance criteria's "lifetime shortened + CSP added" branch rather than the invasive HttpOnly-cookie refactor (which would break the SSE/WS ?token= design). - auth/manager.py: JWT_LIFETIME_SECONDS default 604800 (7d) -> 86400 (24h); env override unchanged. Tests read the constant dynamically. - web-ui/security-headers.js: CSP + hardening headers. connect-src is built from the same build-time NEXT_PUBLIC_API_URL/WS_URL the app dials, so it matches the real backend; img-src allows only the GitHub avatar host; object-src/base-uri/frame-ancestors locked down. Wired via next.config.js headers(). - Test pins the exfil-relevant directives against silent widening. Known ceiling: script-src keeps 'unsafe-inline'/'unsafe-eval' (Next.js App Router needs them absent a nonce middleware); exfil containment comes from connect-src/img-src/object-src. No refresh token, so re-login after 24h.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReduces the JWT default lifetime from 7 days to 24 hours via a changed ChangesDefense-in-depth: JWT lifetime reduction and CSP hardening
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 |
|
Code Review for PR 709: JWT lifetime plus CSP hardening. Overall LGTM with minor feedback. The implementation is clean, well-scoped, and the tradeoffs are clearly documented. See full review in the PR thread. |
Code Review: PR 709 - JWT lifetime + CSP hardeningOverall: LGTM with minor feedback. The implementation is clean, well-scoped, and the tradeoffs are clearly documented. The acceptance criteria are met. What this does well
IssuesMinor — typo in comment (security-headers.js:44) The comment reads // ponytail: — should be // NOTE: or // Caveat:. The content is clear and useful; this is just a cosmetic artifact. Minor — missing Permissions-Policy header The hardening set ships CSP, X-Content-Type-Options, X-Frame-Options, and Referrer-Policy, but omits Permissions-Policy. Locking down browser features (camera, microphone, geolocation, payment) is a standard companion header. Not a blocker — outside acceptance criteria scope — but worth a follow-up ticket. Minor — no Strict-Transport-Security HSTS is absent. Fine for local-dev defaults, but for production self-hosted deployments it should be present. At minimum, worth documenting alongside the CSP note in CLAUDE.md. Not a blocker. Test coverageThe 4 new tests cover the key security guarantees well:
One gap for a follow-up: form-action self is not explicitly asserted. Less critical than connect-src for the JWT exfil threat, but it is a meaningful directive and the test pattern already exists in the file. Backend changeJWT_LIFETIME_SECONDS change is minimal and correct. The constant evaluates at module import time (existing behavior, unchanged). The 7x window reduction is meaningful defense-in-depth. Summary
The typo is the only thing worth fixing before merging; everything else is follow-up scope. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@web-ui/__tests__/security-headers.test.js`:
- Around line 8-12: Convert the test file from CommonJS to TypeScript with ES6
imports. Rename security-headers.test.js to security-headers.test.ts, and
replace the CommonJS require() statement with an ES6 import statement to import
the functions buildCsp, buildConnectSrc, and securityHeaders from the
security-headers module. Ensure the import path and destructuring syntax match
the ES6 import patterns used in other test files within the project.
In `@web-ui/security-headers.js`:
- Around line 21-25: The buildConnectSrc function unconditionally adds
DEFAULT_WS_URL (which is localhost:8000) to the connect-src policy when wsUrl is
not explicitly provided, creating an unnecessary security risk in production by
exposing a local exfil destination. Fix this by modifying the line that adds
wsUrl to only include it when explicitly provided, removing the fallback to
DEFAULT_WS_URL. Apply the same fix to the similar pattern mentioned at lines
29-32 to ensure no default localhost addresses are shipped in production
security headers.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c49fd9e-c139-4bb2-b217-508722436667
📒 Files selected for processing (5)
CLAUDE.mdcodeframe/auth/manager.pyweb-ui/__tests__/security-headers.test.jsweb-ui/next.config.jsweb-ui/security-headers.js
| const { | ||
| buildCsp, | ||
| buildConnectSrc, | ||
| securityHeaders, | ||
| } = require('../security-headers'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and verify the content at lines 8-12
cd web-ui && find . -name "security-headers.test.js" -type fRepository: frankbria/codeframe
Length of output: 100
🏁 Script executed:
# Check the project structure and find eslint/typescript config
cd web-ui && ls -la && fd -e ".eslintrc*" -e "eslint.config.*" -e "tsconfig.json" | head -20Repository: frankbria/codeframe
Length of output: 1490
🏁 Script executed:
# Search for `@typescript-eslint/no-require-imports` in config files
rg "no-require-imports" --type javascript --type json --type yamlRepository: frankbria/codeframe
Length of output: 98
🏁 Script executed:
# Read the test file to verify the import statement
cat -n web-ui/__tests__/security-headers.test.js | head -20Repository: frankbria/codeframe
Length of output: 906
🏁 Script executed:
# Check the eslint configuration
cat web-ui/eslint.config.mjsRepository: frankbria/codeframe
Length of output: 979
🏁 Script executed:
# Search for no-require-imports configuration in all config files
rg "no-require-imports" web-ui/Repository: frankbria/codeframe
Length of output: 45
🏁 Script executed:
# Check what files are being ignored in eslint config
cat web-ui/eslint.config.mjs | grep -A 5 "ignores"Repository: frankbria/codeframe
Length of output: 244
🏁 Script executed:
# Check if the test file is using .js extension and might be excluded
file web-ui/__tests__/security-headers.test.jsRepository: frankbria/codeframe
Length of output: 108
🏁 Script executed:
# Search for typescript-eslint configurations and their defaults
rg "tseslint.configs" web-ui/eslint.config.mjs -A 2 -B 2Repository: frankbria/codeframe
Length of output: 229
🏁 Script executed:
# Check if there are any other eslint configs or package.json eslint settings
cat web-ui/package.json | grep -A 10 "eslint"Repository: frankbria/codeframe
Length of output: 1183
🏁 Script executed:
# Check the source file being imported
cat -n web-ui/security-headers.js | head -30Repository: frankbria/codeframe
Length of output: 1460
🏁 Script executed:
# Check if there's a TypeScript version of the security headers file
ls -la web-ui/security-headers* | head -10Repository: frankbria/codeframe
Length of output: 139
🏁 Script executed:
# Verify the exact eslint file patterns - look at the full config more carefully
cat web-ui/eslint.config.mjsRepository: frankbria/codeframe
Length of output: 979
🏁 Script executed:
# Check if the __tests__ directory has any TypeScript files
find web-ui/__tests__ -type f -name "*.ts" -o -name "*.tsx" | head -10Repository: frankbria/codeframe
Length of output: 607
🏁 Script executed:
# Verify: does the eslint actually run on .js test files by checking the default typescript-eslint recommended rules
rg "`@typescript-eslint/no-require-imports`" web-ui/ || echo "Rule not explicitly configured in repo"Repository: frankbria/codeframe
Length of output: 102
🏁 Script executed:
# Check what the typescript-eslint default behavior is - look at package versions
grep -A 5 "typescript-eslint" web-ui/package.jsonRepository: frankbria/codeframe
Length of output: 104
🏁 Script executed:
# Confirm: list all test files to see the pattern - are there .js test files?
find web-ui/__tests__ -type f \( -name "*.js" -o -name "*.ts" -o -name "*.tsx" \) | sortRepository: frankbria/codeframe
Length of output: 2196
Convert this file to TypeScript (.test.ts) and use ES6 import syntax for consistency with the rest of the codebase.
The file currently uses CommonJS require() syntax, which is inconsistent with the project's other test files (all using .test.tsx or .test.ts). All other files in __tests__/ use ES6 imports and TypeScript. Converting this file to TypeScript and adopting ES6 import syntax would align it with the project's patterns. Example:
Suggested refactor
+import securityHeadersModule from '../security-headers';
+
+const { buildCsp, buildConnectSrc, securityHeaders } = securityHeadersModule;🧰 Tools
🪛 GitHub Actions: Test Suite (Unit + E2E) / 6_Code Quality (Lint + Type Check).txt
[error] 12-12: ESLint (rule: @typescript-eslint/no-require-imports): A require() style import is forbidden.
🪛 GitHub Actions: Test Suite (Unit + E2E) / Code Quality (Lint + Type Check)
[error] 12-12: ESLint (@typescript-eslint/no-require-imports): A require() style import is forbidden.
🪛 GitHub Check: Code Quality (Lint + Type Check)
[failure] 12-12:
A require() style import is forbidden
🤖 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 `@web-ui/__tests__/security-headers.test.js` around lines 8 - 12, Convert the
test file from CommonJS to TypeScript with ES6 imports. Rename
security-headers.test.js to security-headers.test.ts, and replace the CommonJS
require() statement with an ES6 import statement to import the functions
buildCsp, buildConnectSrc, and securityHeaders from the security-headers module.
Ensure the import path and destructuring syntax match the ES6 import patterns
used in other test files within the project.
Sources: Linters/SAST tools, Pipeline failures
| function buildConnectSrc({ apiUrl, wsUrl } = {}) { | ||
| const sources = new Set(["'self'"]); | ||
| if (apiUrl) sources.add(apiUrl); | ||
| sources.add(wsUrl || DEFAULT_WS_URL); | ||
| return Array.from(sources).join(' '); |
There was a problem hiding this comment.
Avoid shipping localhost in connect-src for production defaults.
Line 24 currently adds ws://localhost:8000 whenever NEXT_PUBLIC_WS_URL is unset, and this policy is then applied globally. In production, that unnecessarily opens an extra exfil destination (any local listener) if XSS occurs.
Suggested hardening
function buildConnectSrc({ apiUrl, wsUrl } = {}) {
const sources = new Set(["'self'"]);
if (apiUrl) sources.add(apiUrl);
- sources.add(wsUrl || DEFAULT_WS_URL);
+ if (wsUrl) {
+ sources.add(wsUrl);
+ } else if (process.env.NODE_ENV !== 'production') {
+ sources.add(DEFAULT_WS_URL);
+ }
return Array.from(sources).join(' ');
}Also applies to: 29-32
🤖 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 `@web-ui/security-headers.js` around lines 21 - 25, The buildConnectSrc
function unconditionally adds DEFAULT_WS_URL (which is localhost:8000) to the
connect-src policy when wsUrl is not explicitly provided, creating an
unnecessary security risk in production by exposing a local exfil destination.
Fix this by modifying the line that adds wsUrl to only include it when
explicitly provided, removing the fallback to DEFAULT_WS_URL. Apply the same fix
to the similar pattern mentioned at lines 29-32 to ensure no default localhost
addresses are shipped in production security headers.
|
Follow-up review — latest commit (1117cdd) Picking up from my previous review (summary table above). The latest commit touched only Test file update The test file now uses ES6 import syntax — that addresses the spirit of CodeRabbit's CommonJS-to-ES-module comment. Keeping the Still outstanding from prior review Typo in CodeRabbit's DEFAULT_WS_URL concern CodeRabbit flagged that
Suggest addressing this as a follow-up rather than blocking merge, but a comment in the code making the production requirement explicit would help. Verdict Same as before: fix the |
|
Follow-up review — post-merge-from-main (5cecd05) Building on my two previous reviews. No new concerns from the merge-from-main commit — it's a clean rebase with no conflict resolutions that touch the security surface. Still open
Everything else from prior reviews remains follow-up scope:
Verdict: Fix the |
1 participant
Closes #657
What
Defense-in-depth for the localStorage-stored auth JWT (release-readiness audit finding M3). Implements the acceptance criteria's "lifetime shortened + CSP added" branch rather than the invasive HttpOnly-cookie refactor (which would break the SSE/WS
?token=design and touch axios, both WS hooks, and the backend transport for a Phase-7/Future hardening item).Changes
codeframe/auth/manager.py—JWT_LIFETIME_SECONDSdefault604800(7d) →86400(24h). Env override unchanged; existing tests read the constant dynamically so they're unaffected.web-ui/security-headers.js(new) — CSP + hardening headers.connect-srcis built at build time from the sameNEXT_PUBLIC_API_URL/NEXT_PUBLIC_WS_URLthe app dials, so it matches the real backend without hardcoding a deploy URL.img-srcallows only the GitHub avatar host;object-src/base-uri/frame-ancestors/form-actionlocked down. Wired vianext.config.jsheaders().web-ui/__tests__/security-headers.test.js(new) — pins the exfil-relevant directives so a future edit can't silently widen them (e.g. toconnect-src *).CLAUDE.md— documentsJWT_LIFETIME_SECONDS.Acceptance criteria
✅ Lifetime shortened — default 7d → 24h (7× window reduction), env-overridable.
✅ CSP added — contains XSS-based token exfiltration;
connect-src/img-src/object-srcblock sending the token to an attacker origin.Demo evidence (live, from
npm run build && npm start)Tests
web-ui: 1024 jest tests pass (incl. 4 new);npm run buildsucceeds.tests/ui/test_v2_auth_enforcement.py+tests/api/— 69 pass;ruffclean.Known limitations
script-srckeeps'unsafe-inline'/'unsafe-eval'— the Next.js App Router needs them without a per-request nonce middleware (a much larger change). Exfil containment comes fromconnect-src/img-src/object-src, notscript-src./loginon 401.Summary by CodeRabbit
New Features
Security Updates
Documentation
Tests