fix: fix link handling in example preview iframe sandbox#701
fix: fix link handling in example preview iframe sandbox#701meshackm wants to merge 2 commits intonexu-io:mainfrom
Conversation
|
Hi @meshackm! 🎉 Thanks for making open-design better! |
lefarcen
left a comment
There was a problem hiding this comment.
Hi @meshackm, thanks for tackling this iframe navigation issue! The approach of adding a click interceptor is sound. However, I found 1 critical bug that would break the entire sandbox shim, plus 2 security/correctness issues:
Critical (P1):
apps/web/src/runtime/srcdoc.tsline 165: Invalid JavaScript syntax —href === ';is an unterminated string literal. This breaks the entire injected<script data-od-sandbox-shim>, so the existinglocalStorage/sessionStorageshims also stop working. Fix tohref === ''
Security/correctness (P2):
apps/web/src/runtime/srcdoc.tsline 166: Hash-link handler incomplete — doesn't updatelocation.hash, breaking templates using hash state,hashchangeevents, history/back, CSS:target, named anchors. Suggest:location.hash = href;apps/web/src/runtime/srcdoc.tsline 173:relattribute doesn't protectwindow.open(). Always pass'noopener,noreferrer'as features
Minor (P3):
apps/web/src/runtime/srcdoc.tsline 160:e.targetmay not be Element. Addinstanceofcheck beforeclosest()
Let me know if you need clarification on any of these!
lefarcen
left a comment
There was a problem hiding this comment.
Hi @meshackm! I need to correct my previous review — the P1 I mentioned (unterminated string literal) was a false positive; your code is syntactically correct on line 165. Apologies for the confusion!
However, the 2 security/correctness issues (P2) still remain:
P2 — Hash navigation incomplete:
- Line 166-170: The hash-link handler prevents default but only calls
scrollIntoView(). This doesn't updatelocation.hash, so templates using hash state,hashchangeevents, history/back, CSS:target, and named anchors will break. Native fragment navigation handles all of this automatically. - Fix: Use
location.hash = href;instead of manual scrolling, or allow native behavior for valid same-document hashes.
P2 — window.open opener leak:
- Line 173-178: The logic checks
rel === 'noopener noreferrer'(exact string match) and OMITS thenoopener,noreferrerfeatures when it matches. This is backwards — therelattribute does NOT protect a programmaticwindow.open()call. You must always pass'noopener,noreferrer'as the third argument. - Fix:
window.open(href, '_blank', 'noopener,noreferrer')for ALL_blanklinks, regardless ofrel.
P3 — Element check:
- Line 161:
e.target.closest('a[href]')assumes the target is anElement. Click targets can be text nodes, which would throw. Addinstanceof Elementcheck first.
Let me know if you need clarification on any of these!
|
I understand, let me look into it. There was indeed a missing single quote, I added it before your first review. |
Fix #660 Examples preview iframe link navigation bug
Fixes
href="#",href="#section") scroll correctly within the preview, smooth scrolling works if enabledtarget="_blank"are safely opened in a new tab, aided by adding the following iframe sandbox options:allow-popups allow-popups-to-escape-sandbox