fix(security): admit the platform asset origins the renderer emits - #3411
fix(security): admit the platform asset origins the renderer emits#3411kwakayama wants to merge 3 commits into
Conversation
The default CSP was 'self'-only for scripts and images while the renderer writes React from the ESM CDN and optimized image URLs from the platform image service into every document it serves. The policy therefore forbade the same response's own assets: no hosted page hydrated and every optimized image was refused. Platform origins now live in one list beside the existing Studio origin allowlist, so the policy and the emitters cannot drift apart. Project-owned external origins stay out -- those belong in project security.csp.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Warning Review limit reached
Next review available in: 34 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 (2)
📝 WalkthroughWalkthroughThe change adds shared platform asset origin constants and includes them in the default production CSP. Tests validate approved script and image origins while preserving restrictions on other CSP directives. ChangesPlatform asset CSP
Estimated code review effort: 3 (Moderate) | ~20 minutes 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/response/security-handler.test.ts`:
- Around line 314-327: Update the CSP assertions in the security-handler test to
explicitly verify that both media-src and worker-src contain no remote hosts,
alongside the existing host-free directive checks. Keep the platform allowlist
validation for applicable asset directives unchanged.
🪄 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: b427de33-c2b7-4dee-a71f-0e70f7214486
📒 Files selected for processing (3)
src/security/http/platform-asset-origins.tssrc/security/http/response/security-handler.test.tssrc/security/http/response/security-handler.ts
The loop accepted a platform host in every directive it checked, so adding one to media-src or worker-src would have passed. Only script-src and img-src carry a platform asset; every other directive is now asserted exactly host-free, and a directive added to the policy later is covered without anyone remembering to list it.
deno task docs. The import and comments added to security-handler.ts moved the exported declarations down a few lines, so the generated source links went stale; no symbols changed.
|
Superseded by #3417, which consolidates this with the other queued render-path fixes plus the v0.1.1206 bump so one queue pass lands the set. Dequeued from the merge queue to avoid this merging underneath the consolidated branch and leaving it empty or conflicting. Please do not re-queue — merge #3417 instead. Closing once that lands. |
1 participant
The default CSP forbade the assets the renderer itself emits. No hosted page hydrated, and every optimized image was blocked.
The contradiction
One response, sent by one server:
and, in the body it was attached to:
images.veryfront.comimg-src 'self' data:esm.sh(React, react-dom, jsx-runtime)script-src 'self' 'nonce-...'The renderer writes 291 URLs pointing at the platform's own image service, then sends a policy refusing that origin. React is imported from the ESM CDN by the framework's own hydration, then refused the same way. Browser console on production:
This is framework-level. It affects every hosted project that renders an image or hydrates.
Fix
Platform-owned origins now live in one list,
src/security/http/platform-asset-origins.ts, beside the existingHOSTED_STUDIO_ORIGINSthat already feedsframe-ancestors.script-srcandimg-srcconsume it, so the policy and the emitters cannot drift apart silently.Deliberately not widened
fonts.googleapis.comstays blocked. It is Google's origin, referenced by the project's layout, not platform infrastructure -- projects declare their own external origins throughsecurity.csp. Granting it by default would hand every hosted site an origin it never asked for.connect-src,font-src,default-src,frame-src,media-src,worker-src: unchanged, and now explicitly asserted host-free.The invariant is narrowed, not removed
default CSP admits no remote hosts or broad network schemesfailed on this change withscript-src must not hardcode a remote host. That stance could not hold -- it made the policy block its own response -- so it is rewritten to the property that does hold: nothing outside the platform allowlist gets in. The directives that gain nothing from a remote host are still pinned to exactly their old values.A positive test pins the fix:
Red before, green after.
Proof
Compiled binary, real production release, environment read off the live deployment:
Suite: 431 passed / 5497 steps across
security,modules,handlers,html,transforms.deno fmtclean.Tradeoff worth reviewing
This permits scripts from
esm.shon every hosted page. That is a real widening, and I want it seen rather than buried. The cleaner long-term fix is serving React same-origin from/_vf_modules-- that path exists and works (/_vf_modules/_veryfront/react/runtime/core.jsreturns 200 once #3409 lands). Changing React delivery has caching and performance implications well beyond this defect, so this change matches the renderer's current behaviour instead of redesigning it. If you would rather close the gap by moving React same-origin, this PR is the wrong shape and I will write that one instead.Related
Independent of #3409 (module pin conflict). I verified they are separate: with modules serving correctly, the document still carries all 6
esm.shreferences. Both are needed for a hosted page to work.Summary by CodeRabbit