fix(design): allow cross-origin PNG screenshots from chain - #80
Conversation
Public gallery <img> tags should hit chain.joinbase.ai directly so Vercel does not proxy large screenshot bytes. Set CORP cross-origin on PNG view responses while keeping the HTML lockdown floor for non-PNG paths.
|
Warning Review limit reached
Next review available in: 38 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds a separate header policy for PNG screenshots. The gateway detects PNG viewer paths, applies cross-origin screenshot headers, removes cookies and stale HTML headers, and documents direct gateway URL usage. ChangesPNG viewer header policy
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Rename similar bindings and use Path::extension for case-insensitive .png detection so -D warnings CI can merge the CORP cross-origin fix.
![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 `@crates/gateway/src/proxy.rs`:
- Around line 247-258: The apply_view_lockdown function currently selects
screenshot_headers based only on the request path, allowing non-PNG upstream
content to bypass the sandbox CSP. Inspect the upstream response Content-Type
and use screenshot_headers only when it is image/png; otherwise retain
viewer_headers, and add an integration test covering an index.png response with
text/html that verifies the sandbox CSP remains present.
In `@docs/DESIGN_CHALLENGE.md`:
- Around line 231-236: Add the http language identifier to the fenced code block
containing the security response headers, while leaving the block’s contents
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cf848523-3c29-447f-9cb2-294e7c202ec2
📒 Files selected for processing (6)
crates/design-http/src/api.rscrates/design-sanitize/src/lib.rscrates/gateway/src/proxy.rscrates/gateway/tests/proxy_view_lockdown.rsdocs/DESIGN_CHALLENGE.mddocs/SITE_API.md
| fn apply_view_lockdown(resp: &mut Response, frame_ancestors: &str, rest: &str) { | ||
| let headers = resp.headers_mut(); | ||
| headers.remove(header::SET_COOKIE); | ||
| for (k, v) in design_sanitize::viewer_headers(frame_ancestors) { | ||
| let floor = if is_view_png_path(rest) { | ||
| // Drop HTML-only lockdown if a stale hop set them on a PNG response. | ||
| headers.remove(header::CONTENT_SECURITY_POLICY); | ||
| headers.remove(HeaderName::from_static("cross-origin-opener-policy")); | ||
| headers.remove(HeaderName::from_static("permissions-policy")); | ||
| design_sanitize::screenshot_headers() | ||
| } else { | ||
| design_sanitize::viewer_headers(frame_ancestors) | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep the sandbox header unless the upstream response is declared image/png.
Line 250 classifies the response from the request path only. An upstream can return Content-Type: text/html for an index.png request. This branch then removes CSP and applies the cross-origin screenshot policy, so miner-controlled HTML can execute without the non-PNG sandbox floor.
Check the upstream Content-Type before selecting screenshot_headers(). If the type is not image/png, retain viewer_headers(). Add an integration test where index.png returns text/html and assert that the sandbox CSP remains present.
🧰 Tools
🪛 GitHub Actions: ci / 1_fmt · clippy · test · deny · xtask.txt
[error] 247-247: Clippy similar-names: bindings resp and rest are too similar. Rename one binding or explicitly allow the lint. Failed command: cargo clippy --workspace --all-targets -- -D warnings.
🪛 GitHub Actions: ci / fmt · clippy · test · deny · xtask
[error] 247-247: Clippy similar_names: binding rest is too similar to the existing binding resp. Rename one of the bindings or explicitly allow the lint. Failed under cargo clippy --workspace --all-targets -- -D warnings.
🤖 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 `@crates/gateway/src/proxy.rs` around lines 247 - 258, The apply_view_lockdown
function currently selects screenshot_headers based only on the request path,
allowing non-PNG upstream content to bypass the sandbox CSP. Inspect the
upstream response Content-Type and use screenshot_headers only when it is
image/png; otherwise retain viewer_headers, and add an integration test covering
an index.png response with text/html that verifies the sandbox CSP remains
present.
| ``` | ||
| X-Content-Type-Options: nosniff | ||
| Referrer-Policy: no-referrer | ||
| Cross-Origin-Resource-Policy: cross-origin | ||
| Cache-Control: private, no-store | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the fenced block.
Static analysis reports MD040 at Line 231. Mark this block as http so Markdown linting accepts it.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 231-231: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/DESIGN_CHALLENGE.md` around lines 231 - 236, Add the http language
identifier to the fenced code block containing the security response headers,
while leaving the block’s contents unchanged.
Source: Linters/SAST tools
1 participant
Summary
/v1/view/*/index.pngresponses now useCross-Origin-Resource-Policy: cross-origin(gateway + design-http) so joinbase.ai can load screenshots with a direct absolute URL.CORP: same-origin, CSP sandbox).<img src>should hithttps://chain.joinbase.ai/challenge/design/v1/view/{runId}/index.png;/gbase-apimay remain for JSON only.Test plan
cargo test -p design-sanitize screenshot_headerscargo test -p gateway --lib png_viewcargo test -p gateway --test proxy_view_lockdown png_viewcargo test -p design-http view_page_serves_screenshots_onlycargo run -p xtask -- design-check<img src>at chain — otherwise CORP still blocks cross-origin images on current prod.Summary by CodeRabbit
New Features
Bug Fixes
Documentation