feat(security): derive passive CSP origins from a project's own source - #3461
Conversation
Groundwork for making `security.csp` an override rather than a prerequisite. Pure function only -- nothing consumes it yet, so this changes no served policy. The platform floor lists what the platform emits, so anything a project loads from elsewhere has to be admitted by hand. A fleet audit found ~100 projects loading blocked assets and two that had declared anything, which is a default that breaks working sites and tells no one. Two decisions carry the design, both settled against real released source rather than from first principles. Only passive directives are derived: img-src, media-src, font-src. script-src and connect-src are never contributed to. Deriving those would mean anyone able to influence source -- a compromised dependency, a merged PR, CMS-authored MDX -- could grant themselves execution or an exfiltration endpoint, collapsing two independent barriers into one. Every project in the audit was fixed by passive directives alone, so the stricter line costs nothing measurable. Matching is syntax-blind rather than position-aware. The first version matched `<img src>`, `url()` and `poster`, reasoning that knowing which element made a reference would place each origin precisely. Measured against a real project it recovered about a third: the same site reaches its CDN through an src attribute, an `imageSrc` prop, YAML frontmatter, markdown ``, a bare string in an array, a `srcFallback` key, and an href on a favicon link. Partial derivation is worse than none -- the site still breaks, but now it looks configured. With the passive/active line held firmly, recall is what protects the project and precision is not what protects the visitor. Origins are ranked by reference count before the cap applies. Running over a real article produced its CDN plus five hosts that appear only as prose links; across a content site those one-off links vastly outnumber asset hosts, so an arbitrary cut could drop the origin the site depends on and silently reintroduce the breakage. An asset host recurs on every page that uses it. Verified on production data: given `pages/blog/articles/introduction-graphql.mdx` from codersociety's deployed release, this recovers `https://cdn.codersociety.com` -- the origin whose absence took that site's images and video down. 28 steps, 0 failed. Wiring into buildCSP as a layer between the floor and `security.csp` follows separately.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a module that derives up to 32 normalized public HTTPS origins from release source files. The origins populate only passive CSP directives. Tests cover extraction, filtering, ranking, scan limits, deterministic output, and immutability. ChangesCSP origin derivation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SourceFiles
participant Derivation
participant Normalization
participant CSPDirectives
SourceFiles->>Derivation: provide release source files
Derivation->>Normalization: extract and normalize HTTPS URLs
Normalization-->>Derivation: return valid origins
Derivation->>CSPDirectives: assign capped frozen origins
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
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 `@src/security/http/derived-csp-origins.ts`:
- Around line 110-113: Update scanContent to enforce MAX_SCANNED_BYTES_PER_FILE
using UTF-8 byte length rather than content.length, truncating at a valid UTF-8
code point boundary before URL matching. Add a regression test using multibyte
filler followed by a late URL to verify content beyond the 512 KiB byte limit is
ignored.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: be263c7d-d960-4328-b009-243e6240f858
📒 Files selected for processing (2)
src/security/http/derived-csp-origins.test.tssrc/security/http/derived-csp-origins.ts
| function scanContent(content: string, into: Map<string, number>): void { | ||
| const scanned = content.length > MAX_SCANNED_BYTES_PER_FILE | ||
| ? content.slice(0, MAX_SCANNED_BYTES_PER_FILE) | ||
| : content; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Enforce the limit in UTF-8 bytes.
content.length counts UTF-16 code units, not bytes. A file with 256 KiB of € characters is 768 KiB in UTF-8, but this code scans all of it and can derive a URL after the documented 512 KiB boundary.
Truncate on UTF-8 code point boundaries before matching. Add a regression test with multibyte filler before a late URL.
Proposed fix
+function truncateToUtf8ByteLimit(content: string): string {
+ let bytes = 0;
+ let end = 0;
+
+ for (const character of content) {
+ const codePoint = character.codePointAt(0)!;
+ const characterBytes = codePoint <= 0x7f
+ ? 1
+ : codePoint <= 0x7ff
+ ? 2
+ : codePoint <= 0xffff
+ ? 3
+ : 4;
+ if (bytes + characterBytes > MAX_SCANNED_BYTES_PER_FILE) break;
+ bytes += characterBytes;
+ end += character.length;
+ }
+
+ return content.slice(0, end);
+}
+
function scanContent(content: string, into: Map<string, number>): void {
- const scanned = content.length > MAX_SCANNED_BYTES_PER_FILE
- ? content.slice(0, MAX_SCANNED_BYTES_PER_FILE)
- : content;
+ const scanned = truncateToUtf8ByteLimit(content);📝 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.
| function scanContent(content: string, into: Map<string, number>): void { | |
| const scanned = content.length > MAX_SCANNED_BYTES_PER_FILE | |
| ? content.slice(0, MAX_SCANNED_BYTES_PER_FILE) | |
| : content; | |
| function truncateToUtf8ByteLimit(content: string): string { | |
| let bytes = 0; | |
| let end = 0; | |
| for (const character of content) { | |
| const codePoint = character.codePointAt(0)!; | |
| const characterBytes = codePoint <= 0x7f | |
| ? 1 | |
| : codePoint <= 0x7ff | |
| ? 2 | |
| : codePoint <= 0xffff | |
| ? 3 | |
| : 4; | |
| if (bytes + characterBytes > MAX_SCANNED_BYTES_PER_FILE) break; | |
| bytes += characterBytes; | |
| end += character.length; | |
| } | |
| return content.slice(0, end); | |
| } | |
| function scanContent(content: string, into: Map<string, number>): void { | |
| const scanned = truncateToUtf8ByteLimit(content); |
🤖 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 `@src/security/http/derived-csp-origins.ts` around lines 110 - 113, Update
scanContent to enforce MAX_SCANNED_BYTES_PER_FILE using UTF-8 byte length rather
than content.length, truncating at a valid UTF-8 code point boundary before URL
matching. Add a regression test using multibyte filler followed by a late URL to
verify content beyond the 512 KiB byte limit is ignored.
Review catch: `MAX_SCANNED_BYTES_PER_FILE` was compared against `content.length`, which counts UTF-16 code units, not bytes. The two diverge by up to 3-4x on multibyte content -- 1000 `€` characters are 1000 code units and 3000 UTF-8 bytes -- so a file could be scanned well past the documented boundary. The remedy is the name, not the comparison. The budget exists to bound scanning work, and the regex engine steps through code units: 512K code units costs the same whether they are ASCII or CJK, so there is no amplification to defend against. Denominating the cap in UTF-8 bytes would read more naturally while tracking the cost less accurately, and computing it means walking the string to decide how much of the string to walk. Renamed to `MAX_SCANNED_CHARS_PER_FILE` and documented why that unit. A regression test pins the behaviour from both sides: multibyte filler under the budget still yields a following URL, and over it does not. 29 steps, 0 failed.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flagged the array `.includes` as substring sanitization. The receiver is an array, so it was already exact equality -- but asserting through a Set says so unambiguously and matches how security-handler.test.ts states the same contract. Matching "https://cdn.example.com" as a substring would also pass for a hostile "https://cdn.example.com.evil.test".
2 participants
Groundwork for making
security.cspan override rather than a prerequisite. Pure function only — nothing consumes it yet, so this changes no served policy. Wiring it intobuildCSPas a layer between the floor andsecurity.cspfollows separately.Why
The platform floor lists what the platform emits, so anything a project loads from elsewhere has to be admitted by hand via
security.csp. A fleet audit found ~100 projects loading blocked assets and two that had declared anything — including veryfront's own marketing site. A default that breaks working sites and tells no one is a bad default however principled the allowlist argument is.Two decisions, both settled against real released source
Only passive directives are derived —
img-src,media-src,font-src.script-srcandconnect-srcare never contributed to.Deriving those would mean anyone able to influence source — a compromised dependency, a merged PR, CMS-authored MDX — could grant themselves code execution or an exfiltration endpoint, collapsing two independent barriers into one. Every project in the audit was fixed by passive directives alone, so the stricter line costs nothing measurable. A host referenced from a
<script>tag is still discovered, but can only ever land in the passive set.Matching is syntax-blind, not position-aware. The first version matched
<img src>,url()andposter, reasoning that knowing which element made a reference would place each origin precisely. Measured against a real project it recovered about a third. The same site reaches its CDN through:<img src="https://cdn…/a.png" />srcpropimageSrc: 'https://cdn…/b.png'image: "https://cdn…/hero.png"https://cdn…/manifest.m3u8srcFallback: "https://cdn…/720p.mp4"href="https://cdn…/favicon.ico"Partial derivation is worse than none — the site still breaks, but now it looks configured. With the passive/active line held firmly, recall is what protects the project and precision is not what protects the visitor.
Origins are ranked by reference count before the cap applies. Running over a real article produced its CDN plus five hosts appearing only as prose links. Across a content site those one-off links vastly outnumber asset hosts, so an arbitrary cut could drop the origin the site depends on and silently reintroduce the breakage. An asset host recurs on every page that uses it; a body-copy link appears once.
Verified against production data
Given
pages/blog/articles/introduction-graphql.mdxfrom codersociety's deployed release, this recovershttps://cdn.codersociety.com— the origin whose absence took that site's images and video down.It also picks up
github.com,graphql.organd three other prose-link hosts from the article body. Those get passive access only: they can serve an image, and cannot execute code or receive data. That is the recall/precision trade-off made explicit, and it is the one worth taking.Known boundary
A URL assembled at runtime — template literal, CMS field, environment variable — is invisible to static analysis. Those projects still declare
security.csp, which is exactly what "override" means.Verification
28 steps, 0 failed.
deno check,lint,fmtclean.Summary by CodeRabbit
New Features
Tests