Prevent cross origin iframe content reading

[Igloczek]

Browsers like Firefox or IE11 throws errors while trying to read cross origin iframe content. Element type and hostname has to be checked before any content processing/reading.

Loading external iframes is common practise in every store, b/c services like Hotjar use this technique.

This errors are breaking checkout process, so it's not an low severity issue.

[Igloczek]

Looks like it's same as #7914, but I'm not using jQuery, b/c it's not necessary.

[OZZlE]

There is already a PR for this here: #7914

The integration test failure was probably a bug in that rather than my js version of the code

[OZZlE]

it's already implemented in jQuery so why not use it?

[Igloczek]

Because it's not necessary as you see. We should write as much of vanilla JS as we can, not a 100% jQuery based code, even just for sake of future upgrades and deprecations.

You soultion will not work if 3rd party iframe will have your main domain as a GET parameter i.e. Livechat make it this way.

I already mentioned your PR in secod comment 😉

[orlangur]

I would prefer #7914 over this one (if they work equally good).

@Igloczek,

Because it's not necessary as you see. We should write as much of vanilla JS as we can, not a 100% jQuery based code, even just for sake of future upgrades and deprecations.

jQuery is NEVER necessary. There is NO valid reason to not use it in a file where it is already defined. You simply create a mess with all these wordy getElementById mixed with jQuery calls.

lookup($(el).find('body'));

There is jQuery call here, isn't it? There is jQuery call one line after you code:

$(element).contents().each(function (index, el) {

So, is it really necessary to force any developer reading this code scratch their heads?

Does it work as intended, by the way? I didn't try your implementation in action but tried a small snippet in browser console:

$($('html').get(0)).find('body').nodeName
undefined
$($('html').get(0)).find('body').get(0).nodeName
"BODY"

[Igloczek]

@orlangur
For sure we can swap Vanilla JS to jQuery here, but for me it looks werid and using Vanilla JS for simple things is much better than basing on any framework.

jQuery replacment:

var el = $('<a></a>');
el.prop('href', 'http://magento.com/test&page=test.com');
console.log(el[0].hostname);

About lookup($(el).find('body')); - I just stick with default code, don't want to make unecessary changes.

As I mention before #7914 dosen't cover option while URL contains base page URL as GET param (as in exapmple above) and thread this as "own" iframe, which cause same errors as currently, that's why we need to check hostname of url, not just run indexOf on it.

BTW. Yes it works, we have it on production stores already.

[orlangur]

@Igloczek, I was trying to say that you (probably) changed behavior of code trying to use Vanilla JS.

lookup($(el).find('body')); should look as lookup($(el).find('body').get(0)); to work with your code I believe (BUT, didn't check). Please confirm I'm wrong.

I'm perfectly fine with VanillaJS-only (for instance, in small templates with KnockoutJS bindings).
I'm fine with jQuery in places where we stick to it (inside old school widgets, in most places we have even jQuery+jQueryUI).
Problems start to appear when we cross the borders within one file. So, jQuery is not so bad, inconsistency is worser.

[orlangur]

Please confirm I'm wrong.

BTW. Yes it works, we have it on production stores already.

Ah, ok then. All the rest is just a matter of taste :)

[vrann]

@Igloczek can you please merge with the latest mainline?

[Igloczek]

@vrann Conflicts solved.

[vrann]

@Igloczek After running the tests for JS had one failure:

iframe from other host returns empty Array

Expected 1 to equal 0.

which is related to this test dev/tests/js/jasmine/tests/app/code/Magento/PageCache/frontend/js/page-cache.test.js

As far as we already have merged #7914, should we just close this one? Seems like the exactly same solution. Haven't dig deep into why the test fails, though.

[orlangur]

@vrann I didn't check by myself, but from #8005 (comment) it looks like this fix is wider.

[Igloczek]

@vrann Weird... Returning an empty array is the thing in this PR. I run tests locally and everything is fine. Do you use this test somewhere in CI pipeline to check non-local results?

I pushed updated tests, b/c the solution from #7914 not cover a case when origin hostname is passed as a GET param and assume that can read iframe content, breaking everything.

[vrann]

@Igloczek ok, let me try to run it again. The covered use case makes sense

[vrann]

@Igloczek I run this test again in CI pipeline and it fails with the same result on latest commit. Can that be related to any environment setting?

[orlangur]

which is related to this test dev/tests/js/jasmine/tests/app/code/Magento/PageCache/frontend/js/page-cache.test.js

Returning an empty array is the thing in this PR. I run tests locally and everything is fine. Do you use this test somewhere in CI pipeline to check non-local results?

@Igloczek are you sure running those Jasmine-based JavaScript tests locally? They are not a part of Travis CI build currently.

So, if you say "Returning an empty array is the thing in this PR" and you observe it locally, corresponding JavaScript unit test needs to be adopted to a changed logic.

OR this test is passing on your local env?

[Igloczek]

I found why it was failing and now I'm wondering how it works in my stores, b/c it shouldn't...

The problem was related to reading jQuery object in vanilla JS, so I just switched to jQuery solution instead of addding [0] everywhere.

Also for some reason I probably was checking some cached / wrong version of my files, that's why my tests were green all the time.

BTW. Do you have any plans to run Jasmine on Travis too? It should be easy to do.

[orlangur]

The problem was related to reading jQuery object in vanilla JS

That's exactly what I was referring in #8005 (comment) 😄 Good that we found it before merging to mainline.

[vrann]

@Igloczek thanks for all the updates! Continue merging.

[vrann]

@Igloczek accepted! Pleasure working with you.

[viebel]

@vrann I'm on magento 2.1.7. How can I apply this fix?

[TommyKolkman]

@viebel Same question, did you already find out?

[hostep]

@TommyKolkman: this fix was very recently backported to the 2.1-develop branch, I suspect it might get released in version 2.1.8 or 2.1.9, but you'll have to ask the Magento devs for the exact timeframe.

[peterjaap]

Not fixed in 2.1.8; https://github.com/magento/magento2/blob/2.1.8/app/code/Magento/PageCache/view/frontend/web/js/page-cache.js

But is fixed in 2.2 RC: https://github.com/magento/magento2/blob/2.2.0-rc3.0/app/code/Magento/PageCache/view/frontend/web/js/page-cache.js#L47

[hostep]

@peterjaap: It was too late to get included in 2.1.8 as far as I understood it, so it will probably be released in 2.1.10.
2.1.9 should get released today but is a security release / USPS change only, maybe some highly critical bugfixes are going to be included but probably not this one. But I'm just guessing here :)

[korostii]

@hostep yes, seems to be present on 2.1.10-preview:
https://github.com/magento/magento2/blob/2.1.10-preview/app/code/Magento/PageCache/view/frontend/web/js/page-cache.js#L30

[DaimPiek]

There is still an error in the fix mentioned above (in 2.1.10).

It should be 'el' not 'element'.
`

    // prevent cross origin iframe content reading
    if ($(el).prop('tagName') === 'IFRAME') {
      iframeHostName = $('<a>').prop('href', $(el).prop('src'))
        .prop('hostname');

      if (window.location.hostname !== iframeHostName) {
        return [];
      }
    }`

[OZZlE]

@DaimPiek doesn't seem to be incorrect in this PR at least..

[DaimPiek]

@OZZlE
I seem to have made a mistake there. I've overriden the file in our Magento 2.1.7 install and placed the code segment in the each loop. Where the variable name is 'el' instead of 'element', my bad.

[behnamshayani]

released in 2.1.10

[OZZlE]

@behnamshayani great! Good thai today 🥇 :D

[Baracuda011]

I still have this issue in 2.1.11.