feat: frontend security hardening -- ESLint XSS ban + MotionConfig CSP nonce#926
feat: frontend security hardening -- ESLint XSS ban + MotionConfig CSP nonce#926
Conversation
…P nonce Ban dangerouslySetInnerHTML via ESLint no-restricted-syntax rule to catch accidental XSS vectors at write time. Add <MotionConfig nonce> wrapper in App.tsx so Framer Motion's PopChild style injection is CSP-compliant when nonce-based style-src is enabled. Nonce is read from a <meta> tag injected by nginx; gracefully no-ops when absent (current state). Closes #924 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fix getCspNonce caching: use Symbol sentinel so absent-nonce results are also cached (previously re-queried DOM on every call). Trim whitespace from nonce content. Add tests for whitespace-only content and DOM re-query prevention. Improve index.html comment with cross-references and include rule name in ESLint escape hatch message. Pre-reviewed by 5 agents, 4 findings addressed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🧰 Additional context used🧠 Learnings (7)📚 Learning: 2026-03-19T07:12:14.508ZApplied to files:
📚 Learning: 2026-03-15T21:20:09.993ZApplied to files:
📚 Learning: 2026-03-30T10:09:41.160ZApplied to files:
📚 Learning: 2026-03-30T10:09:41.160ZApplied to files:
📚 Learning: 2026-03-15T18:17:43.675ZApplied to files:
📚 Learning: 2026-03-14T15:43:05.601ZApplied to files:
📚 Learning: 2026-03-14T15:43:05.601ZApplied to files:
🔇 Additional comments (1)
WalkthroughThis pull request adds frontend CSP hardening: an ESLint rule that flags uses of dangerouslySetInnerHTML, a new 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches. Re-running this action after a short time may resolve the issue. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
There was a problem hiding this comment.
Code Review
This pull request introduces a CSP nonce handling mechanism, including a utility to retrieve the nonce from a meta tag and integration with framer-motion via MotionConfig. It also adds an ESLint rule to ban dangerouslySetInnerHTML. The review feedback suggests improving the robustness of the getCspNonce utility by guarding against non-browser environments (SSR) and moving the nonce retrieval inside the App component to avoid module-level side effects.
| const nonce = getCspNonce() | ||
|
|
||
| export default function App() { | ||
| return <AppRouter /> | ||
| return ( | ||
| <MotionConfig nonce={nonce}> | ||
| <AppRouter /> | ||
| </MotionConfig> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Calling getCspNonce() at the module level introduces a side effect that depends on the document object. While this works for client-side rendering, it's generally better to avoid such side effects at the module level. It can make the code harder to test, can behave unexpectedly with Hot Module Replacement (HMR), and prevents the code from being easily adapted for Server-Side Rendering (SSR) in the future.
A better practice is to retrieve the nonce within the component's lifecycle. This can be done lazily and only once using useState or useMemo.
| const nonce = getCspNonce() | |
| export default function App() { | |
| return <AppRouter /> | |
| return ( | |
| <MotionConfig nonce={nonce}> | |
| <AppRouter /> | |
| </MotionConfig> | |
| ) | |
| } | |
| import { useState } from 'react' | |
| export default function App() { | |
| const [nonce] = useState(getCspNonce) | |
| return ( | |
| <MotionConfig nonce={nonce}> | |
| <AppRouter /> | |
| </MotionConfig> | |
| ) | |
| } |
web/src/__tests__/lib/csp.test.ts
Outdated
| // querySelector called once during import-time init + once for first call | ||
| // Second call should hit cache, not query DOM again |
There was a problem hiding this comment.
This comment is slightly inaccurate. querySelector is not called at import time, but rather on the first call to getCspNonce(). The test logic itself is correct, but the comment could be clarified to avoid confusion for future readers.
| // querySelector called once during import-time init + once for first call | |
| // Second call should hit cache, not query DOM again | |
| // querySelector is only called on the first invocation of getCspNonce. | |
| // Subsequent calls should hit the cache and not re-query the DOM. |
| export function getCspNonce(): string | undefined { | ||
| if (cached !== UNREAD) return cached as string | undefined | ||
|
|
||
| const meta = document.querySelector<HTMLMetaElement>( | ||
| 'meta[name="csp-nonce"]', | ||
| ) | ||
| cached = meta?.content?.trim() || undefined | ||
| return cached | ||
| } |
There was a problem hiding this comment.
The current implementation of getCspNonce directly accesses the document object, which will cause a crash if this code is ever run in a non-browser environment, such as during Server-Side Rendering (SSR). To make this utility more robust and future-proof, it's good practice to guard against document being undefined.
| export function getCspNonce(): string | undefined { | |
| if (cached !== UNREAD) return cached as string | undefined | |
| const meta = document.querySelector<HTMLMetaElement>( | |
| 'meta[name="csp-nonce"]', | |
| ) | |
| cached = meta?.content?.trim() || undefined | |
| return cached | |
| } | |
| export function getCspNonce(): string | undefined { | |
| if (cached !== UNREAD) return cached as string | undefined | |
| if (typeof document === 'undefined') { | |
| cached = undefined | |
| return cached | |
| } | |
| const meta = document.querySelector<HTMLMetaElement>( | |
| 'meta[name="csp-nonce"]', | |
| ) | |
| cached = meta?.content?.trim() || undefined | |
| return cached | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/index.html`:
- Around line 7-10: Update the inline comment near the commented meta tag "<meta
name="csp-nonce" content="__CSP_NONCE__" />" to explain the required nginx and
header changes: state that enabling the nonce requires (1) uncommenting this
meta tag, (2) adding a sub_filter directive and sub_filter_once off to
nginx.conf to replace __CSP_NONCE__ per-request, and (3) changing
security-headers.conf to use 'nonce-__CSP_NONCE__' in the style-src policy; also
add a pointer to MotionConfig and lib/csp.ts for runtime nonce handling so
future developers know all three pieces must be updated together.
In `@web/src/__tests__/lib/csp.test.ts`:
- Around line 63-64: The comment is misleading about import-time behavior;
update the test comment near getCspNonce and querySelector to state that
getCspNonce() is lazy and only queries the DOM on the first invocation (then
uses a cache on subsequent calls), so the DOM is queried once during the first
call and once more for the initial import-time query mentioned should be removed
— clarify that the assertion toHaveLength(1) verifies the first call hit and
subsequent calls use the cached value.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8805ea85-5110-408a-a881-e374897311ce
📒 Files selected for processing (5)
web/eslint.config.jsweb/index.htmlweb/src/App.tsxweb/src/__tests__/lib/csp.test.tsweb/src/lib/csp.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Dashboard Test
- GitHub Check: Build Backend
- GitHub Check: Build Web
- GitHub Check: Build Sandbox
- GitHub Check: Dependency Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (2)
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web/src/**/*.{ts,tsx}: React component reuse: ALWAYS reuse existing components fromweb/src/components/ui/before creating new ones. The design system inventory covers all common patterns.
Design tokens: use Tailwind semantic classes (text-foreground,bg-card,text-accent,text-success,bg-danger, etc.) or CSS variables (var(--so-accent)). NEVER hardcode hex values in.tsx/.tsfiles.
Typography in web code: usefont-sansorfont-mono(maps to Geist tokens). NEVER setfontFamilydirectly.
Spacing in web code: use density-aware tokens (p-card,gap-section-gap,gap-grid-gap) or standard Tailwind spacing. NEVER hardcode pixel values for layout spacing.
Shadows/borders in web code: use token variables (var(--so-shadow-card-hover),border-border,border-bright).
Usecn()from@/lib/utilsfor conditional class merging in React components.
Do NOT recreate status dots inline - use<StatusBadge>component.
Do NOT build card-with-header layouts from scratch - use<SectionCard>component.
Do NOT create metric displays withtext-metric font-bold- use<MetricCard>component.
Do NOT render initials circles manually - use<Avatar>component.
Do NOT create complex (>8 line) JSX inside.map()- extract to a shared component.
Do NOT usergba()with hardcoded values - use design token variables.
React: TypeScript 6.0 hasnoUncheckedSideEffectImportsdefaulting to true - CSS side-effect imports need type declarations (Vite's/// <reference types='vite/client' />covers this).
React/Web: ESLint zero warnings enforced onweb/src/**/*.{ts,tsx}files.
React/Web: use React 19, react-router for routing, shadcn/ui + Radix UI for components, Tailwind CSS 4 for styling.
React/Web: use Zustand for state management and@tanstack/react-queryfor server state.
Files:
web/src/App.tsxweb/src/lib/csp.tsweb/src/__tests__/lib/csp.test.ts
web/src/**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
React/Web: use Vitest for unit testing with coverage scoped to files changed vs origin/main.
Files:
web/src/__tests__/lib/csp.test.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T12:00:18.113Z
Learning: Commits: <type>: <description> — types: feat, fix, refactor, docs, test, chore, perf, ci. Enforced by commitizen (commit-msg hook). Signed commits: required on main via branch protection — all commits must be GPG/SSH signed.
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-30T06:54:37.737Z
Learning: Applies to web/src/**/*.{ts,tsx} : React/Web: ESLint zero warnings enforced on `web/src/**/*.{ts,tsx}` files.
📚 Learning: 2026-03-30T06:54:37.737Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-30T06:54:37.737Z
Learning: Applies to web/src/**/*.{ts,tsx} : React/Web: use React 19, react-router for routing, shadcn/ui + Radix UI for components, Tailwind CSS 4 for styling.
Applied to files:
web/src/App.tsx
📚 Learning: 2026-03-30T06:54:37.737Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-30T06:54:37.737Z
Learning: Applies to web/src/**/*.{ts,tsx} : React: TypeScript 6.0 has `noUncheckedSideEffectImports` defaulting to true - CSS side-effect imports need type declarations (Vite's `/// <reference types='vite/client' />` covers this).
Applied to files:
web/src/App.tsxweb/eslint.config.js
📚 Learning: 2026-03-30T06:54:37.737Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-30T06:54:37.737Z
Learning: Applies to web/src/**/*.{ts,tsx} : Use `cn()` from `@/lib/utils` for conditional class merging in React components.
Applied to files:
web/src/App.tsx
📚 Learning: 2026-03-30T06:54:37.737Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-30T06:54:37.737Z
Learning: Applies to web/src/**/*.{ts,tsx} : React/Web: use Zustand for state management and tanstack/react-query for server state.
Applied to files:
web/src/App.tsx
📚 Learning: 2026-03-30T06:54:37.737Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-30T06:54:37.737Z
Learning: Applies to web/src/**/*.{ts,tsx} : React/Web: ESLint zero warnings enforced on `web/src/**/*.{ts,tsx}` files.
Applied to files:
web/eslint.config.js
📚 Learning: 2026-03-30T06:54:37.737Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-30T06:54:37.737Z
Learning: Applies to web/tsconfig.json : React: TypeScript 6.0 makes `esModuleInterop` always true - cannot be set to false; remove explicit declaration to avoid deprecation warning.
Applied to files:
web/eslint.config.js
📚 Learning: 2026-03-30T06:54:37.737Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-30T06:54:37.737Z
Learning: Applies to web/src/**/__tests__/**/*.{ts,tsx} : React/Web: use Vitest for unit testing with coverage scoped to files changed vs origin/main.
Applied to files:
web/src/__tests__/lib/csp.test.ts
📚 Learning: 2026-03-20T08:28:32.845Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T08:28:32.845Z
Learning: Applies to web/src/__tests__/**/*.{ts,js} : Dashboard testing: Vitest unit tests organized by feature under `web/src/__tests__/`. Use fast-check for property-based testing (`fc.assert` + `fc.property`).
Applied to files:
web/src/__tests__/lib/csp.test.ts
🔇 Additional comments (4)
web/src/lib/csp.ts (1)
1-23: Well-designed caching utility.The implementation correctly addresses several edge cases:
- Symbol sentinel (
UNREAD) properly distinguishes "never queried" from "queried but absent" — avoiding repeated DOM queries when no nonce exists.- Trimming whitespace and converting empty strings to
undefinedprevents invalid nonce values from propagating.- Module-level caching ensures single DOM query per page lifecycle.
The type assertion on line 16 is safe given the preceding guard.
web/eslint.config.js (1)
26-34: Effective XSS prevention rule.The
no-restricted-syntaxconfiguration correctly:
- Uses the appropriate AST selector to catch
dangerouslySetInnerHTMLin JSX attributes.- Sets error severity to enforce the ban at write time.
- Provides a clear message with alternatives (text content, sanitization library) and documents the escape hatch for justified exceptions.
This aligns with the PR objective and the learning that ESLint zero warnings are enforced.
web/src/__tests__/lib/csp.test.ts (1)
1-71: Comprehensive test coverage.The test suite thoroughly validates
getCspNonce():
- Covers missing, empty, whitespace-only, and valid nonce scenarios.
- Verifies caching behavior for both present and absent results.
- Uses
vi.resetModules()for proper isolation between tests.- Correctly uses dynamic imports to get fresh module instances.
This satisfies the PR requirement for test coverage of the caching fix.
web/src/App.tsx (1)
1-13: Clean integration of CSP nonce with Framer Motion.The implementation correctly:
- Reads the nonce once at module scope (before first render, when the DOM is already available).
- Wraps the entire app in
MotionConfigto provide the nonce to all motion components.- Handles
undefinedgracefully when no nonce meta tag exists (local dev).This satisfies the PR objective of making Framer Motion's style injection CSP-compliant.
- Add __CSP_NONCE__ placeholder guard in getCspNonce() (security) - Move JSDoc to getCspNonce(), add threat model note and activation status - Remove unnecessary type assertion in getCspNonce() (TS narrows correctly) - Expand index.html comment with full 3-step nonce activation checklist - Add nonce activation TODO in nginx.conf with cross-references - Fix spy leak risk: move vi.restoreAllMocks() to afterEach - Use querySelectorAll in test cleanup for robustness - Fix misleading "import-time init" comment in querySelector spy test - Add tests: placeholder rejection, whitespace trimming to valid nonce - Update CLAUDE.md lib/ description (add motion.ts, csp.ts) - Add Frontend Security section to docs/security.md (XSS ban, CSP nonce) - Expand CSP header row with nonce readiness note - Add csp.ts to ux-guidelines.md Reference Materials table Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🤖 I have created a release *beep* *boop* --- ## [0.5.1](v0.5.0...v0.5.1) (2026-03-30) ### Features * add linear variant to ProgressGauge component ([#927](#927)) ([89bf8d0](89bf8d0)) * frontend security hardening -- ESLint XSS ban + MotionConfig CSP nonce ([#926](#926)) ([6592ed0](6592ed0)) * set up MSW for Storybook API mocking ([#930](#930)) ([214078c](214078c)) ### Refactoring * **web:** replace Sidebar tablet overlay with shared Drawer component ([#928](#928)) ([ad5451d](ad5451d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Summary
dangerouslySetInnerHTMLvia ESLintno-restricted-syntaxrule to catch accidental XSS vectors at write time (zero existing usages)<MotionConfig nonce>wrapper inApp.tsxso Framer Motion'sPopChildstyle injection is CSP-compliant when nonce-basedstyle-srcis enabledgetCspNonce()utility reads nonce from<meta name="csp-nonce">tag (injected by nginxsub_filter); gracefully returnsundefinedwhen absent (current state)index.htmldocuments the nginx integration contractTest plan
npm --prefix web run lint-- passes, zero violations (confirms no existingdangerouslySetInnerHTMLusage)npm --prefix web run type-check-- cleannpm --prefix web run test-- 180 files, 2151 tests pass (6 new tests forgetCspNonce: absent meta, valid nonce, caching, empty content, whitespace-only, DOM re-query prevention)npm --prefix web run devand navigate between pages to verify AnimatePresence transitions still work with MotionConfig wrapperReview coverage
Pre-reviewed by 5 agents (docs-consistency, security-reviewer, frontend-reviewer, test-quality-reviewer, issue-resolution-verifier). 4 findings addressed: caching bug fix (Symbol sentinel), whitespace trim, test coverage gaps, comment clarity improvements.
Closes #924
🤖 Generated with Claude Code