Add header documentation with mobile preview #418
Conversation
docs/mobile-embed-frame.md
Outdated
| const replacement = document.createElement('script'); | ||
| replacement.innerHTML = script.innerHTML; | ||
| script.parentNode.replaceChild(replacement, script); | ||
| }); |
There was a problem hiding this comment.
I know your original comment mentioned this PR should raise some security eyebrows... just wanted to confirm yes eyebrows raised
There was a problem hiding this comment.
Yeah, on this code in particular, it's strange that browsers don't evaluate dynamically inserted script tags, while seemingly having no issue with inserting something equally exploitable like <img onload="somejavascript">.
An alternative approach could be to have just a standalone HTML page that already has the component markup in it, though that might end up with a lot more duplication, especially if we wanted to use it for more components.
I might time-box and see if I can put something together for that and what it ends up looking like. I tried in an earlier iteration and had some trouble, but I think I was fighting more with Jekyll than the approach itself.
There was a problem hiding this comment.
For what it's worth, I've implemented something like this in the past and had previously spent a lot of time verifying the concept as safe, even with untrusted content in a secure context.
Although now that I write this, I probably need to harden this a bit more for the implementation to be truly safe (specifically iframe sandbox attribute). I'll see where I end up with my timeboxed experimentation whether this is an approach we go with.
There was a problem hiding this comment.
Updated in ace14d0 to avoid the postMessage communication and dynamic script insertion, and instead using a simple static page for the frame.
Co-authored-by: Zach Margolis <[email protected]>
aduth
force-pushed
the
aduth-header-preview
branch
from
f78266c
to
ace14d0
Compare
Avoid inconsistencies causing issues like what was fixed in c9d9eb2
|
Visual regression failure is expected, since we're adding a new link to the sidebar navigation. |
🛠 Summary of changes
This adds a new documentation page preview for the Header component, in response to a bug which surfaced in 18F/identity-dashboard#748 (comment) related to active hover states for dropdown navigation links.
This doesn't actually fix the issue addressed in the comment above, and instead is limited to just adding a page to the internal documentation.
Notably, this includes a live, simulated mobile preview
using anEdit: After ace14d0, this was updated to use a simple static page as the frame source.iframereceiver communicated usingpostMessage. For the security-minded, this should probably raise some hairs, though (a) there is protection to ensure messages are only received from the same domain, (b) this site is not publicly hosted anywhere at the moment, and (c) the site does not store any valuable data which would be worth targeting.📜 Testing Plan
npm start👀 Screenshots
Example mobile preview: