FE-1500: Add oEmbed discovery and framing security - #9363
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
PR SummaryMedium Risk Overview A new
Reviewed by Cursor Bugbot for commit 85f0b57. Bugbot is set up for automated code reviews on this repo. Configure here. |
github-actions
Bot
added
area/infra
![semgrep-code-hashintel[bot]](https://avatars.githubusercontent.com/u/22116112?s=60&v=4){kind=small}
| value | ||
| .replaceAll("&", "&") | ||
| .replaceAll('"', """) | ||
| .replaceAll("'", "'") | ||
| .replaceAll("<", "<") | ||
| .replaceAll(">", ">"); |
There was a problem hiding this comment.
Semgrep identified an issue in your code:
Detected a call to replaceAll() in an attempt to HTML escape the string value .replaceAll("&", "&") .replaceAll('"', """) .replaceAll("'", "'") .replaceAll("<", "<"). Manually sanitizing input through a manually built list can be circumvented in many situations, and it's better to use a well known sanitization library such as sanitize-html or DOMPurify.
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by detect-replaceall-sanitization.
You can view more details about this finding in the Semgrep AppSec Platform.
|
Semgrep found 3 Detected a call to |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| value | ||
| .replaceAll("&", "&") |
| value | ||
| .replaceAll("&", "&") | ||
| .replaceAll('"', """) |
| value | ||
| .replaceAll("&", "&") | ||
| .replaceAll('"', """) | ||
| .replaceAll("'", "'") | ||
| .replaceAll("<", "<") |
| value | ||
| .replaceAll("&", "&") | ||
| .replaceAll('"', """) | ||
| .replaceAll("'", "'") | ||
| .replaceAll("<", "<") | ||
| .replaceAll(">", ">"); |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
6d3e470 to
8f9df3c
Compare
21dbd64 to
02e7685
Compare
8f9df3c to
23127ef
Compare
02e7685 to
798bd3d
Compare
23127ef to
27ef2b0
Compare
798bd3d to
91c9ccb
Compare
27ef2b0 to
43734e1
Compare
91c9ccb to
e371efe
Compare
github-actions
Bot
removed
area/deps
| } | ||
|
|
||
| return sanitized; | ||
| }; |
There was a problem hiding this comment.
oEmbed drops multi-item selection
Medium Severity
sanitizePreviewSearch forwards scenario, subnet, and the single-item itemType/itemId pair, but never copies the items query that the Viewer uses for multi-selection. Canonical URLs with two or more selected elements therefore lose that selection in the returned iframe.
Reviewed by Cursor Bugbot for commit 7836613. Configure here.
a48dcd5 to
3315bbb
Compare
7836613 to
28cb7c7
Compare
28cb7c7 to
929fc32
Compare
3315bbb to
5afc088
Compare
929fc32 to
ff0b3d3
Compare
5afc088 to
690195c
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ff0b3d3. Configure here.
| } | ||
|
|
||
| return sanitized; | ||
| }; |
There was a problem hiding this comment.
Multi-selection state dropped from embeds
Medium Severity
sanitizeEmbedSearch copies scenario, subnet, itemType, and itemId but never the items query key. Embed routes already treat items as the canonical multi-selection encoding, so an oEmbed iframe silently drops any multi-item selection from the source URL.
Reviewed by Cursor Bugbot for commit ff0b3d3. Configure here.
690195c to
2d06143
Compare
ff0b3d3 to
85f0b57
Compare
2 participants
🌟 What is the purpose of this PR?
Adds oEmbed support for the example pages: a
/api/oembedVercel function that turns a canonical example URL into a sandboxed iframe embed, runtime discovery links on the example pages, and site-wide framing security headers. Stacked on the embed-route PR.🔗 Related links
🔍 What does this change?
api/oembed.tsfetch handler: validates canonicalhttps://demo.petrinaut.org/examples/...URLs against catalog metadata, pins unversioned URLs to revision 1, strips query state down to the embed-supported scenario/subnet/selection keys, fits requested dimensions without upscaling, and returns arichoEmbed response whose iframe targets/embed/examples/...withsandbox="allow-scripts", no referrer, and lazy loading. The handler imports only catalog metadata so the function never bundles the browser React code; the embed-supported key set is duplicated there for that reason.application/json+oembeddiscovery<link>after mount (the site is a client-rendered SPA, so discovery works for consumers that execute JavaScript; this limitation is by design).vercel.jsonframing policy:frame-ancestors 'none'plusX-Frame-Options: DENYfor the whole site except/embed/examples/*, which allows all ancestors and addsnosniff./api/chatmiddleware to a table over/api/chatand/api/oembed, so development and production run the same handlers.Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
🛡 What tests cover this?
api/oembed.test.ts(23 cases: URL validation, revision pinning, state sanitization, dimension fitting, response headers) andoembed-discovery.test.ts.❓ How to test this?
yarn workspace @apps/petrinaut-website dev.curl "http://localhost:5173/api/oembed?url=https%3A%2F%2Fdemo.petrinaut.org%2Fexamples%2Fgases-1-pn&maxwidth=600".htmliframe targets/embed/examples/gases-1-pn/versions/1, and open an example page to see the discovery<link>indocument.head.