Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cross-origin frame error #581

Closed
outoftime opened this issue Nov 29, 2016 · 10 comments · Fixed by #1213
Closed

Cross-origin frame error #581

outoftime opened this issue Nov 29, 2016 · 10 comments · Fixed by #1213
Labels
bug help wanted

Comments

@outoftime
Copy link
Contributor

outoftime commented Nov 29, 2016 •
edited

No idea what to make of this, but this project throws an uncaught error when you click the button:

Uncaught DOMException: Failed to read the 'contentDocument' property
from 'HTMLIFrameElement': Blocked a frame with origin "null" from
accessing a cross-origin frame.

Original bug report

Submitter: mekhi
this does not work in the preview:

https://gist.github.com/anonymous/8549724468c89747b455b4f7312092fd
@outoftime outoftime changed the title User Feedback Cross-origin frame error May 28, 2017
@outoftime outoftime added the bug label May 28, 2017
@jwang1919
Copy link
Contributor

Able to reproduce with the following this snapshot.

Interestingly enough, using inline styles seems to work though,

@jwang1919
Copy link
Contributor

jwang1919 commented Nov 14, 2017 •
edited

Found a temporary fix.
Issue appears to stem from using an element tag selector to hide elements and then showing them.

@catrope
Copy link
Contributor

catrope commented Nov 20, 2017

It only fails when hiding with an element selector, not with any other selector. So if you have CSS like h1 { display: none; }, then calling $('#myheading').show(); causes an error, but if instead you have CSS like #myheading { display: none; } or .heading { display: none; } it doesn't.

This is because of the complicated way in which jQuery's .show() method works (see unminified source). In the showHide function, it first tries to show the element by clearing the .style.display property, then checks if that worked. In our case, the element is still hidden, because there's a CSS rule hiding it. To deal with this, jQuery tries to set .style.display to the browser default for the display property for that element.

Figuring out what that default is happens in the defaultDisplay() function. It first tries to just create another <h1>, append it to the <body>, and read its display property. If we used an element/class-based selector, this would return 'block', we'd be done, and not get an error. However, because we used an element selector, this returns 'none', so jQuery gets desperate. It creates an iframe, appends it to the document, creates an <h1> inside of that, and reads its display properties.

The iframe creation step fails in the normal popcode interface (where this iframe is inside the preview iframe), but not when the preview is opened a separate tab (in that case the iframe isn't a nested iframe). The error happens because the preview iframe is sandboxed, which gives it a unique origin and disallows it from accessing any other iframes. Unfortunately this includes iframes //inside// the sandboxed iframe as well (even if they'd be treated as same-origin in absence of the sandbox), which is kind of silly.

We could fix this by adding allow-same-origin to the sandbox attribute. However, MDN warns that:

When the embedded document has the same origin as the main page, it is strongly discouraged to use both allow-scripts and allow-same-origin at the same time, as that allows the embedded document to programmatically remove the sandbox attribute. Although it is accepted, this case is no more secure than not using the sandbox attribute.

My impression is that Popcode sandboxes the iframe to prevent it from messing things up accidentally, and isn't designed to guard against someone deliberately attacking it. On the other hand, we probably don't want people to be able to trick people into clicking on snapshot links that then do malicious things. On the other other hand, I don't know that there's many malicious things one could possibly do.

@outoftime
Copy link
Contributor Author

Fantastic investigation and writeup @catrope!

As it happens you correctly anticipated the reasoning behind the frame sandbox—the concern about a malicious snapshot or gist import link having access to the application runtime. The biggest practical concern would be gaining access to the user’s authed GitHub session.

I have to say, I’m quite surprised that jQuery has this problem, given its ubiquity and maturity. Did you come across any open issues related to this? I guess in most cases it would be pretty easy for devs to just work around it by avoiding the exact combination of selectors that triggers it. Not so for us, though…

Other than turning off sandbox (which I have to pretty much rule out), any ideas how we might work around this…? I will let it simmer as well.

@outoftime
Copy link
Contributor Author

Hmm, would display: initial work????

@outoftime
Copy link
Contributor Author

(Update: If I naïvely change .show() in the offending code to .css('display', 'initial') it seems to do the right thing!)

@catrope
Copy link
Contributor

catrope commented Nov 20, 2017

I tried that too, but display: initial turns out to behave like display: inline, which is not what we want for <h1>s (see this snapshot). From reading MDN I think that's because every property can have only one initial value, which does not depend on the kind of element it applies to.

display: unset; doesn't seem to work correctly either (also results in inline), but display: inherit does result in block, and getComputedStyle( h1Element ).style also returns 'block'. Unless, that is, the <body> itself has display: none on it, but in that case it arguably matters less what you do.

I have a meeting in a few minutes but afterwards I'll see if this issue has already been reported against jQuery. If we don't want to change Popcode's iframe sandboxing, we'd have to change/fork jQuery's show/hide code (could be as simple as trying the display: inherit trick above before trying an iframe). It might also be a good idea to file a bug against the WHATWG spec for iframe origins asking for sandboxed iframes to be allowed to access their child iframes, but that's much more of a long game; and in my previous experience trying to get a spec bug fixed, the forces of inertia ("but all browsers have done it this way for years and we don't want to break them") can be very strong.

@catrope
Copy link
Contributor

catrope commented Nov 20, 2017

TL;DR: Upgrading to jQuery 3 would fix this. There are a number of breaking changes in v3 though, so upgrading isn't trivial.

I dug into the jQuery history and found a number of confusing things, but it looks like the iframe has been removed completely in jQuery 3. Initially they were talking about making .show() refuse to work on things that were hidden from a stylesheet, and even semi-announced this as a breaking change. Then they decided that (and other changes they had made) were too intrusive, and changed some things back. In the end, it looks like they decided to undo this breaking change, but not bring the iframe back either. In cases where jQuery 2 creates an iframe to figure out what the default display value is for an element type, jQuery 3 just gives up and assumes that it's 'block'. (See this comment and this comment for the jQuery community's somewhat shocked initial reaction to this approach.)

Amusingly, I can claim a tiny bit of credit for this being fixed in jQuery 3, because one of the impetuses for rewriting .show()/.hide() was a performance issue in those functions that was discovered during a performance audit of a project that I was working on at the time, and which I implemented workarounds for. Small world, huh? 😄

(For reference: the change that first removed the iframe, people complaining, jQuery deciding to revert to the old behavior, and the change that did that without reintroducing the iframe.)

@outoftime
Copy link
Contributor Author

Thanks so much for the continued detailed investigation @catrope! I’m going to look through the 3.0 breaking changes now, but my guess is that it’s fairly unlikely that there is anything that would affect the very basic usage of jQuery that our students do. Also, it’s generally a project goal to stay as up-to-date as possible with the student-facing libraries we ship with (I always forget to upgrade these when I’m upgrading actual project dependencies).

Funny that this isn’t your first contact with this issue!

@outoftime outoftime mentioned this issue Nov 21, 2017
Merged
@catrope catrope mentioned this issue Nov 21, 2017
Closed
@jwang1919
Copy link
Contributor

Thanks again @catrope for the in-depth analysis on the bug. I wouldn't have figured it would be a jQuery issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug help wanted
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Upgrade to jQuery 3.2.1 outoftime/popcode
3 participants
@outoftime @catrope @jwang1919