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.

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

fix(react-dom): access iframe contentWindow instead of contentDocument #15099

Merged
merged 1 commit into from
Mar 20, 2019

Conversation

renanvalentin
Copy link
Contributor

@renanvalentin renanvalentin commented Mar 13, 2019

MDN has a list of methods for obtaining the window reference of an
iframe:

https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage#Syntax

Closes #14002

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@gaearon
Copy link
Collaborator

gaearon commented Mar 13, 2019

How did you test this change? Specifically, how do you know this doesn't regress the use cases for which this logic was added?

@renanvalentin
Copy link
Contributor Author

Is there any specific use case that should be tested?

Both solutions are trying to get a reference from the window object but the current one has a security violation:

try {
 win = element.contentDocument.defaultView 
//             ^ contentDocument will violate the same origin policy if the iframe is in a different domain
} catch (err) {
 return element; // the catch block will not be executed because we had a security violation
}

@philipp-spiess
Copy link
Contributor

If you have a same origin policy, does it really matter which API you use? I'd expect that the browser makes sure that no API allows access to the frame contents in that case.


Is there any specific use case that should be tested?

We need to test this change across a variety of browser to ensure we do not regress one (especially since the previous comment was pointing out issues with the API that you're now using).

Does this change make it work for Safari on iOS for you?

@renanvalentin
Copy link
Contributor Author

renanvalentin commented Mar 13, 2019

If you have a same origin policy, does it really matter which API you use? I'd expect that the browser makes sure that no API allows access to the frame contents in that case.

In this case it matters because the iframe has a different origin from the parent page, thus it cannot access the content document.

For cross origin access you can use the contentWindow api to reference the window object (which has limited access. This link has the complete list of properties

Does this change make it work for Safari on iOS for you?

Applying this changes fixed the problem on Safari. The application that I'm currently working is creating each credit card input in different iframe and relies on post message. This approach didn't work on iOS and the console is bloated by security error messages. A workaround for this issue was to replace some post message logic by directly calling global objects on the window object from each iframe (which introduced some complexity).

Steps to reproduce:
1 - Open Safari's developer tools
2 - Visit the code sandbox sample at https://codesandbox.io/s/vv8w1zwm90
3 - Click on the input field hosted on codepen
4 - Click on the console tab on code sandbox
5 - It should log Blocked a frame with origin "https://codesandbox.io" from accessing a frame with origin "https://vv8w1zwm90.codesandbox.io". Protocols, domains, and ports must match

The step 5 doesn't occur on Chrome 72 and Firefox 65.0.1. Unfortunately codesanbox doesn't support IE 11 but I can provide some video if needed.

@philipp-spiess
Copy link
Contributor

@renanvalentin Thanks for explaining!

Interestingly if I use your example and select the iframe manually to call .contentWindow on it, Safari will also log the error:

Screenshot 2019-03-13 at 23 58 00

In your initial ticket you mention that this is "breaking the post message flow". What exactly do you mean by that? As far as I can see this is only about an error logged in the browser.

@renanvalentin
Copy link
Contributor Author

renanvalentin commented Mar 13, 2019

It seems that Safari doesn't like to access the iframe using $0:
Screenshot 2019-03-14 at 00 09 27

Or using $0.contentDocument:
Screenshot 2019-03-14 at 00 08 05

But works with $0.contentWindow:
Screenshot 2019-03-14 at 00 07 43

The same behaviour happens on IE 11:

Using $0.contentDocument.defaultView:
Screenshot 2019-03-14 at 09 32 31

Using $0.contentWindow:
Screenshot 2019-03-14 at 09 32 54

In your initial ticket you mention that this is "breaking the post message flow". What exactly do you mean by that? As far as I can see this is only about an error logged in the browser.

I'm sending a post message to the parent page when the input is focused or other event related. It seems that when this error occurs the MessageEvent is not dispatched to the parent page or other iframe. (This is just a gut feeling)

@philipp-spiess
Copy link
Contributor

@renanvalentin You're right I think I made a mistake in selecting the wrong element as $0. I could reproduce it the same way as in your screenshots.

In #12037 we also added a new DOM fixture - This is an example that you can build locally by checking out the React repo, running yarn build, then going to fixtures/dom, and yarn && yarn start there. Can you verify that your change does not break anything by playing around with this? If you've asserted a certain baseline of browsers (whatever you have available would be super helpful) we can help and test out some of the not-so-easy-to-get ones like older IE versions. A list of supported browsers can be found at the landing page of the DOM fixture.

After your explanation I feel confident about your proposed change but we need to make sure that this is not causing any regressions on other browsers (e.g. maybe some older IE versions don't implement contentWindow). We also have to check out which other code paths rely on this helper function and play around with these as well.

Thank you! We really appreciate your help here.

@renanvalentin
Copy link
Contributor Author

@p-jackson I've just found an issue and now the selection behaviour is broken:

  while (element instanceof win.HTMLIFrameElement) {
    try {
      win = element.contentWindow;
    } catch (e) {
      return element;
    }
    element = getActiveElement(win.document); // Can't access the document here
  }

I'm going to run more tests and get back to you.

@sizebot
Copy link

sizebot commented Mar 14, 2019

ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.1%

Details of bundled changes.

Comparing: 103378b...07346f6

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% +0.1% 779.14 KB 779.71 KB 177.56 KB 177.79 KB UMD_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.1% 105.11 KB 105.16 KB 34.11 KB 34.14 KB UMD_PROD
react-dom.profiling.min.js 0.0% +0.1% 108.11 KB 108.17 KB 34.77 KB 34.79 KB UMD_PROFILING
react-dom.development.js +0.1% +0.1% 773.53 KB 774.1 KB 176.06 KB 176.27 KB NODE_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.1% 105.09 KB 105.14 KB 33.6 KB 33.62 KB NODE_PROD
react-dom.profiling.min.js 0.0% +0.1% 108.2 KB 108.26 KB 34.21 KB 34.23 KB NODE_PROFILING
ReactDOM-dev.js +0.1% +0.1% 796.53 KB 797.1 KB 177.21 KB 177.43 KB FB_WWW_DEV
ReactDOM-prod.js 0.0% 0.0% 322.89 KB 323.04 KB 59.14 KB 59.17 KB FB_WWW_PROD
ReactDOM-profiling.js 0.0% +0.1% 328.82 KB 328.98 KB 60.43 KB 60.47 KB FB_WWW_PROFILING
react-dom-unstable-fire.development.js +0.1% +0.1% 779.49 KB 780.05 KB 177.7 KB 177.93 KB UMD_DEV
react-dom-unstable-fire.production.min.js 🔺+0.1% 🔺+0.1% 105.12 KB 105.18 KB 34.12 KB 34.15 KB UMD_PROD
react-dom-unstable-fire.profiling.min.js 0.0% +0.1% 108.13 KB 108.18 KB 34.78 KB 34.8 KB UMD_PROFILING
react-dom-unstable-fire.development.js +0.1% +0.1% 773.88 KB 774.44 KB 176.2 KB 176.41 KB NODE_DEV
react-dom-unstable-fire.production.min.js 🔺+0.1% 🔺+0.1% 105.1 KB 105.16 KB 33.61 KB 33.63 KB NODE_PROD
react-dom-unstable-fire.profiling.min.js 0.0% +0.1% 108.22 KB 108.27 KB 34.22 KB 34.24 KB NODE_PROFILING
ReactFire-dev.js +0.1% +0.1% 795.74 KB 796.31 KB 177.16 KB 177.37 KB FB_WWW_DEV
ReactFire-prod.js 0.0% 🔺+0.1% 311.43 KB 311.58 KB 56.77 KB 56.8 KB FB_WWW_PROD
ReactFire-profiling.js 0.0% +0.1% 317.45 KB 317.61 KB 58.08 KB 58.11 KB FB_WWW_PROFILING
react-dom-test-utils.development.js 0.0% 0.0% 47.06 KB 47.06 KB 12.99 KB 12.99 KB UMD_DEV
react-dom-test-utils.development.js 0.0% 0.0% 46.78 KB 46.78 KB 12.92 KB 12.92 KB NODE_DEV
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.61 KB 60.61 KB 15.92 KB 15.92 KB UMD_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% 🔺+0.1% 11.01 KB 11.01 KB 3.81 KB 3.81 KB UMD_PROD
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.28 KB 60.28 KB 15.79 KB 15.79 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% 🔺+0.1% 10.75 KB 10.75 KB 3.7 KB 3.71 KB NODE_PROD
react-dom-server.browser.development.js 0.0% 0.0% 129.86 KB 129.86 KB 34.6 KB 34.6 KB UMD_DEV
react-dom-server.browser.development.js 0.0% 0.0% 125.99 KB 125.99 KB 33.68 KB 33.68 KB NODE_DEV
ReactDOMServer-dev.js 0.0% 0.0% 127.04 KB 127.04 KB 33.09 KB 33.09 KB FB_WWW_DEV
ReactDOMServer-prod.js 0.0% -0.0% 45.62 KB 45.62 KB 10.6 KB 10.6 KB FB_WWW_PROD
react-dom-server.node.production.min.js 0.0% 0.0% 19.99 KB 19.99 KB 7.58 KB 7.59 KB NODE_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.1% 1.21 KB 1.21 KB 706 B 705 B UMD_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.2% 1.05 KB 1.05 KB 636 B 637 B NODE_PROD
react-dom-unstable-fizz.node.development.js 0.0% +0.1% 3.7 KB 3.7 KB 1.42 KB 1.42 KB NODE_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% 🔺+0.2% 1.1 KB 1.1 KB 666 B 667 B NODE_PROD

Generated by 🚫 dangerJS

@renanvalentin
Copy link
Contributor Author

@philipp-spiess now it is working fine and I think that I have a better understanding of this issue now.

Safari will throw an uncatchable error when the access results in "Blocked a frame with origin". e.g:

Screenshot 2019-03-15 at 11 25 41

A better way is to access one of the cross origin properties: Window or Location
Which might result in "SecurityError" DOM Exception and it is compatible to Safari.

Screenshot 2019-03-15 at 11 26 03

Also, evaluating the iframe using console.log will result in "Blocked a frame with origin". e.g:

export default function getActiveElement(doc: ?Document): ?Element {
  doc = doc || (typeof document !== 'undefined' ? document : undefined);
  if (typeof doc === 'undefined') {
    return null;
  }
  try {
   console.log(doc.activeElement) // throws an error on console when it is an iframe
    return doc.activeElement || doc.body;
  } catch (e) {
    return doc.body;
  }
}

// > Blocked a frame with origin X from accessing a frame with origin Y. Protocols, domains, and ports must match.

Copy link
Contributor

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the change looks good but I found out that it doesn’t work for older versions of Safari Desktop. There, it still behaves the same as before the change.

I conducted the following tests using a new fixture that I wrote (we can merge it into this PR if it makes sense) and the existing selection fixtures:

  • Chrome - Desktop: 49 ✅, Latest ✅
  • Chrome - Android: Latest ✅
  • Firefox Desktop: ESR ✅, Latest ✅
  • Internet Explorer: 9 ✅, 10 ✅, 11 ✅
  • Microsoft Edge: 14 ⚠️, 15 ✅, Latest ✅
  • Safari - Desktop: 7 ❌, 8 ❌, 9 ❌, 10 ✅, 11 ✅,Latest ✅
  • Safari - iOS: 7 ✅, Latest ✅

⚠️ Edge 14 is no longer on BrowserStack so I skipped it.

I think a unit test for this would not be worth it as we’d have to mock so much that we end up having no margin to change the implementation without the test failing. The good comment should help to prevent accidental regression.


I have two smaller nits inline. Thank you a ton for finding this workaround!

try {
// Accessing the contentDocument of a HTMLIframeElement can cause the browser
// to throw, e.g. if it has a cross-origin src attribute.
// Safari will throw an uncatchable error when the access results in "Blocked a frame with origin". e.g:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the term "uncatchable error" is misleading since the error can be caught, it's just that in addition to the error, Safari will also show an error in the console.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason I had this problem of having the catch block not being executed but I couldn't reproduce it anymore.

I changed the comment to Safari will show an error in the console when the access results in "Blocked a frame with origin"

// https://html.spec.whatwg.org/multipage/browsers.html#integration-with-idl

const canReadHrefAttribute = iframe.contentWindow.location.href;
return canReadHrefAttribute != null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to find out wether this returns null or undefined but it should always be a string from what I can see so maybe we check for this instead?

return typeof iframe.contentWindow.location.href === 'string';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated to use your suggestion. I wasn't sure how to express this code, something like this also came up to my mind:

(iframe.contentWindow.location.href) // trying to evaluate the expression
return true;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was thinking the same but I'm not sure if GCC would strip this out so I think the typeof there makes more sense. Thanks for the update!

@renanvalentin renanvalentin force-pushed the fix-cross-domain-issue branch 2 times, most recently from 5df7ff7 to 8e367f1 Compare March 20, 2019 09:55
MDN has a list of methods for obtaining the window reference of an
iframe:

https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage#Syntax

fix(react-dom): check if iframe belongs to the same origin

Accessing the contentDocument of a HTMLIframeElement can cause the browser
to throw, e.g. if it has a cross-origin src attribute.
Safari will show an error in the console when the access results in "Blocked a frame with origin". e.g:

```javascript
try {
 $0.contentDocument.defaultView
} catch (err) {
  console.log('err', err)
}

> Blocked a frame with origin X from accessing a frame with origin Y. Protocols, domains, and ports must match.
> err – TypeError: null is not an object (evaluating '$0.contentDocument.defaultView')
```

A safety way is to access one of the cross origin properties: Window or Location
Which might result in "SecurityError" DOM Exception and it is compatible to Safari.

```javascript
try {
 $0.contentWindow.location.href
} catch (err) {
 console.log('err', err)
}

> err – SecurityError: Blocked a frame with origin "http://localhost:3001" from accessing a cross-origin frame. Protocols, domains, and ports must match.
```

https://html.spec.whatwg.org/multipage/browsers.html#integration-with-idl
@renanvalentin
Copy link
Contributor Author

@philipp-spiess Thanks for sharing those tests results, this issue had more impact on mobile than desktop.

We can also merge the fixture into this PR since it will help to reproduce this issue in the future.

Copy link
Contributor

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff looks good to me. Thank you!

@gaearon do you want to take a quick look? Browser tests can be found here. There are some problems with older versions of Desktop Safari but given that a lot of people ran into this already I think it still makes sense to get this in.

@gaearon
Copy link
Collaborator

gaearon commented Mar 20, 2019

I trust you this works. It's been an annoying issue so let's get it in. Thank you both.

@gaearon gaearon merged commit 061d6ce into facebook:master Mar 20, 2019
gaearon pushed a commit to gaearon/react that referenced this pull request Mar 27, 2019
facebook#15099)

MDN has a list of methods for obtaining the window reference of an
iframe:

https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage#Syntax

fix(react-dom): check if iframe belongs to the same origin

Accessing the contentDocument of a HTMLIframeElement can cause the browser
to throw, e.g. if it has a cross-origin src attribute.
Safari will show an error in the console when the access results in "Blocked a frame with origin". e.g:

```javascript
try {
 $0.contentDocument.defaultView
} catch (err) {
  console.log('err', err)
}

> Blocked a frame with origin X from accessing a frame with origin Y. Protocols, domains, and ports must match.
> err – TypeError: null is not an object (evaluating '$0.contentDocument.defaultView')
```

A safety way is to access one of the cross origin properties: Window or Location
Which might result in "SecurityError" DOM Exception and it is compatible to Safari.

```javascript
try {
 $0.contentWindow.location.href
} catch (err) {
 console.log('err', err)
}

> err – SecurityError: Blocked a frame with origin "http://localhost:3001" from accessing a cross-origin frame. Protocols, domains, and ports must match.
```

https://html.spec.whatwg.org/multipage/browsers.html#integration-with-idl
@gaearon gaearon mentioned this pull request Mar 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants