Prevent cross origin iframe content reading #32244

ihor-sviziev · 2021-02-22T14:52:51Z

Description (*)

Issue described in #8005, #7914 will message like this reproducing randomly (shown as console error):

Seems like it's happening because domain checking only on the exact element, not on all listed

The message looks like this:

[2021-02-22 04:30:25] [ERROR] SecurityError: Blocked a frame with origin "https://example.com" from accessing a cross-origin frame.
ConsoleOutputHandler.show @ console-output-handler.min.js:2
(anonymous) @ logger.min.js:6
Logger.processOutput_ @ logger.min.js:6
Logger.log_ @ logger.min.js:3
Logger.error @ logger.min.js:3
(anonymous) @ page-cache.min.js:4
map @ jquery.min.js:35
contents @ page-cache.min.js:4
lookup @ page-cache.min.js:4
(anonymous) @ page-cache.min.js:4
each @ jquery.min.js:28
lookup @ page-cache.min.js:4
(anonymous) @ page-cache.min.js:4
each @ jquery.min.js:28
lookup @ page-cache.min.js:4
$.fn.comments @ page-cache.min.js:4
_create @ page-cache.min.js:6
(anonymous) @ widget.min.js:15
_createWidget @ widget.min.js:21
$.<computed>.<computed> @ widget.min.js:14
$.<computed>.<computed> @ widget.min.js:13
(anonymous) @ main.min.js:2
execCb @ require.min.js:112
check @ require.min.js:56
(anonymous) @ require.min.js:72
(anonymous) @ require.min.js:11
(anonymous) @ require.min.js:74
each @ require.min.js:3
emit @ require.min.js:74
check @ require.min.js:61
enable @ require.min.js:73
init @ require.min.js:54
callGetModule @ require.min.js:74
completeLoad @ require.min.js:103
onScriptLoad @ require.min.js:112
load (async)
req.load @ require.min.js:119
load @ require.min.js:112
load @ require.min.js:55
fetch @ require.min.js:55
check @ require.min.js:56
enable @ require.min.js:73
enable @ require.min.js:101
(anonymous) @ require.min.js:73
(anonymous) @ require.min.js:11
each @ require.min.js:3
enable @ require.min.js:71
init @ require.min.js:54
(anonymous) @ require.min.js:97
setTimeout (async)
req.nextTick @ require.min.js:117
localRequire @ require.min.js:97
defContext.require @ mixins.min.js:11
requirejs @ require.min.js:117
init @ main.min.js:1
(anonymous) @ main.min.js:6
_.each._.forEach @ underscore.min.js:7
(anonymous) @ main.min.js:5
apply @ main.min.js:5
fire @ jquery.min.js:212
add @ jquery.min.js:215
jQuery.fn.ready @ jquery.min.js:223
jQuery.fn.init @ jquery.min.js:200
jQuery.fn.init @ jquery-migrate.min.js:27
jQuery @ jquery.min.js:16
(anonymous) @ bootstrap.min.js:1
execCb @ require.min.js:112
check @ require.min.js:56
(anonymous) @ require.min.js:72
(anonymous) @ require.min.js:11
(anonymous) @ require.min.js:74
each @ require.min.js:3
emit @ require.min.js:74
check @ require.min.js:61
(anonymous) @ require.min.js:72
(anonymous) @ require.min.js:11
(anonymous) @ require.min.js:74
each @ require.min.js:3
emit @ require.min.js:74
check @ require.min.js:61
enable @ require.min.js:73
init @ require.min.js:54
(anonymous) @ require.min.js:66
(anonymous) @ require.min.js:11
(anonymous) @ mixins.min.js:8
execCb @ require.min.js:112
check @ require.min.js:56
(anonymous) @ require.min.js:72
(anonymous) @ require.min.js:11
(anonymous) @ require.min.js:74
each @ require.min.js:3
emit @ require.min.js:74
check @ require.min.js:61
(anonymous) @ require.min.js:72
(anonymous) @ require.min.js:11
(anonymous) @ require.min.js:74
each @ require.min.js:3
emit @ require.min.js:74
check @ require.min.js:61
enable @ require.min.js:73
init @ require.min.js:54
(anonymous) @ require.min.js:66
(anonymous) @ require.min.js:11
(anonymous) @ mixins.min.js:8
execCb @ require.min.js:112
check @ require.min.js:56
(anonymous) @ require.min.js:72
(anonymous) @ require.min.js:11
(anonymous) @ require.min.js:74
each @ require.min.js:3
emit @ require.min.js:74
check @ require.min.js:61
(anonymous) @ require.min.js:72
(anonymous) @ require.min.js:11
(anonymous) @ require.min.js:74
each @ require.min.js:3
emit @ require.min.js:74
check @ require.min.js:61
(anonymous) @ require.min.js:72
(anonymous) @ require.min.js:11
(anonymous) @ require.min.js:74
each @ require.min.js:3
emit @ require.min.js:74
check @ require.min.js:61
enable @ require.min.js:73
init @ require.min.js:54
(anonymous) @ require.min.js:66
(anonymous) @ require.min.js:11
(anonymous) @ mixins.min.js:8
execCb @ require.min.js:112
check @ require.min.js:56
(anonymous) @ require.min.js:72
(anonymous) @ require.min.js:11
(anonymous) @ require.min.js:74
each @ require.min.js:3
emit @ require.min.js:74
check @ require.min.js:61
(anonymous) @ require.min.js:72
(anonymous) @ require.min.js:11
(anonymous) @ require.min.js:74
each @ require.min.js:3
emit @ require.min.js:74
check @ require.min.js:61
enable @ require.min.js:73
init @ require.min.js:54
callGetModule @ require.min.js:74
completeLoad @ require.min.js:104
onScriptLoad @ require.min.js:112
load (async)
req.load @ require.min.js:119
load @ require.min.js:112
load @ require.min.js:55
(anonymous) @ require.min.js:55
(anonymous) @ require.min.js:11
execCb @ require.min.js:112
check @ require.min.js:56
enable @ require.min.js:73
init @ require.min.js:54
(anonymous) @ require.min.js:97
setTimeout (async)
req.nextTick @ require.min.js:117
localRequire @ require.min.js:97
fetch @ require.min.js:55
check @ require.min.js:56
enable @ require.min.js:73
enable @ require.min.js:101
(anonymous) @ require.min.js:73
(anonymous) @ require.min.js:11
each @ require.min.js:3
enable @ require.min.js:71
init @ require.min.js:54
callGetModule @ require.min.js:74
completeLoad @ require.min.js:103
onScriptLoad @ require.min.js:112
load (async)
req.load @ require.min.js:119
load @ require.min.js:112
load @ require.min.js:55
fetch @ require.min.js:55
check @ require.min.js:56
enable @ require.min.js:73
enable @ require.min.js:101
(anonymous) @ require.min.js:73
(anonymous) @ require.min.js:11
each @ require.min.js:3
enable @ require.min.js:71
init @ require.min.js:54
(anonymous) @ require.min.js:97
setTimeout (async)
req.nextTick @ require.min.js:117
localRequire @ require.min.js:97
load @ mixins.min.js:8
(anonymous) @ require.min.js:71
(anonymous) @ require.min.js:11
on @ require.min.js:40
callPlugin @ require.min.js:61
fetch @ require.min.js:55
check @ require.min.js:56
enable @ require.min.js:73
enable @ require.min.js:101
(anonymous) @ require.min.js:73
(anonymous) @ require.min.js:11
each @ require.min.js:3
enable @ require.min.js:71
init @ require.min.js:54
callGetModule @ require.min.js:74
completeLoad @ require.min.js:103
onScriptLoad @ require.min.js:112
load (async)
req.load @ require.min.js:119
load @ require.min.js:112
load @ require.min.js:55
fetch @ require.min.js:55
check @ require.min.js:56
enable @ require.min.js:73
enable @ require.min.js:101
(anonymous) @ require.min.js:73
(anonymous) @ require.min.js:11
each @ require.min.js:3
enable @ require.min.js:71
init @ require.min.js:54
callGetModule @ require.min.js:74
completeLoad @ require.min.js:103
onScriptLoad @ require.min.js:112
load (async)
req.load @ require.min.js:119
load @ require.min.js:112
load @ require.min.js:55
fetch @ require.min.js:55
check @ require.min.js:56
enable @ require.min.js:73
enable @ require.min.js:101
(anonymous) @ require.min.js:73
(anonymous) @ require.min.js:11
each @ require.min.js:3
enable @ require.min.js:71
init @ require.min.js:54
(anonymous) @ require.min.js:97
setTimeout (async)
req.nextTick @ require.min.js:117
localRequire @ require.min.js:97
load @ mixins.min.js:8
(anonymous) @ require.min.js:71
(anonymous) @ require.min.js:11
on @ require.min.js:40
callPlugin @ require.min.js:61
fetch @ require.min.js:55
check @ require.min.js:56
enable @ require.min.js:73
enable @ require.min.js:101
(anonymous) @ require.min.js:73
(anonymous) @ require.min.js:11
each @ require.min.js:3
enable @ require.min.js:71
init @ require.min.js:54
callGetModule @ require.min.js:74
completeLoad @ require.min.js:103
onScriptLoad @ require.min.js:112
load (async)
req.load @ require.min.js:119
load @ require.min.js:112
load @ require.min.js:55
fetch @ require.min.js:55
check @ require.min.js:56
enable @ require.min.js:73
enable @ require.min.js:101
(anonymous) @ require.min.js:73
(anonymous) @ require.min.js:11
each @ require.min.js:3
enable @ require.min.js:71
init @ require.min.js:54
(anonymous) @ require.min.js:97
setTimeout (async)
req.nextTick @ require.min.js:117
localRequire @ require.min.js:97
load @ mixins.min.js:8
(anonymous) @ require.min.js:71
(anonymous) @ require.min.js:11
on @ require.min.js:40
callPlugin @ require.min.js:61
fetch @ require.min.js:55
check @ require.min.js:56
enable @ require.min.js:73
enable @ require.min.js:101
(anonymous) @ require.min.js:73
(anonymous) @ require.min.js:11
each @ require.min.js:3
enable @ require.min.js:71
init @ require.min.js:54
callGetModule @ require.min.js:74
completeLoad @ require.min.js:103
onScriptLoad @ require.min.js:112
load (async)
req.load @ require.min.js:119
load @ require.min.js:112
load @ require.min.js:55
fetch @ require.min.js:55
check @ require.min.js:56
enable @ require.min.js:73
enable @ require.min.js:101
(anonymous) @ require.min.js:73
(anonymous) @ require.min.js:11
each @ require.min.js:3
enable @ require.min.js:71
init @ require.min.js:54
callGetModule @ require.min.js:74
completeLoad @ require.min.js:103
onScriptLoad @ require.min.js:112
load (async)
req.load @ require.min.js:119
load @ require.min.js:112
load @ require.min.js:55
fetch @ require.min.js:55
check @ require.min.js:56
enable @ require.min.js:73
enable @ require.min.js:101
(anonymous) @ require.min.js:73
(anonymous) @ require.min.js:11
each @ require.min.js:3
enable @ require.min.js:71
init @ require.min.js:54
(anonymous) @ require.min.js:97
setTimeout (async)
req.nextTick @ require.min.js:117
localRequire @ require.min.js:97
configure @ require.min.js:86
requirejs @ require.min.js:116
req.config @ require.min.js:117
(anonymous) @ requirejs-config.min.js:1
(anonymous) @ requirejs-config.min.js:1
(anonymous) @ requirejs-config.min.js:2
Show 103 more frames

Related Pull Requests

Fixed Issues (if relevant)

Fixes magento/magento2#<issue_number>

Manual testing scenarios (*)

Magento 2.3.6-p2
Add some iframe, browse website, you'll randomly see this error in the browser console

Questions or comments

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds are green)

Resolved issues:

resolves [Issue] Prevent cross origin iframe content reading #32264: Prevent cross origin iframe content reading

m2-assistant · 2021-02-22T14:52:54Z

Hi @ihor-sviziev. Thank you for your contribution
Here are some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests
Semantic Version Checker

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

Fix static test

ihor-sviziev · 2021-02-23T07:22:48Z

@magento run all tests

gabrieldagama · 2021-02-23T17:39:26Z

@magento create issue

ihor-sviziev · 2021-02-24T07:01:15Z

@magento run Functional Tests B2B, Functional Tests EE, Static Tests

ihor-sviziev · 2021-02-26T08:37:16Z

@magento run Functional Tests B2B, Functional Tests EE, Static Tests

ihor-sviziev · 2021-03-03T08:14:07Z

@magento run Functional Tests B2B, Functional Tests EE, Static Tests

ihor-sviziev · 2021-03-03T14:32:22Z

@magento run Functional Tests EE

Den4ik · 2021-05-26T13:28:12Z

@magento run all tests

magento-automated-testing · 2021-05-26T13:28:21Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

ihor-sviziev · 2021-05-26T13:28:35Z

@magento run all tests

magento-automated-testing · 2021-05-26T13:28:39Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

Den4ik · 2021-05-26T18:50:18Z

@magento run Unit Tests, Functional Tests B2B

magento-automated-testing · 2021-05-26T18:50:24Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

Den4ik

✅ Approve

Researched and discussed with @ihor-sviziev

Changes make since if $(element) will be array of 2+ elements. I did't found steps for reproducing issue.
But I admit that element could be selector string by some external libraries like FB pixel, twitter etc. Moving code block prevent issue.

magento-engcom-team · 2021-05-26T21:54:42Z

Hi @Den4ik, thank you for the review.
ENGCOM-9101 has been created to process this Pull Request
✳️ @Den4ik, could you please add one of the following labels to the Pull Request?

Label	Description
`Auto-Tests: Covered`	All changes in Pull Request is covered by auto-tests
`Auto-Tests: Not Covered`	Changes in Pull Request requires coverage by auto-tests
`Auto-Tests: Not Required`	Changes in Pull Request does not require coverage by auto-tests

sfritzsche · 2022-03-20T16:21:15Z

Hi @ihor-sviziev,
Hi @Den4ik,

we also encountered this problem today.
We want to add some missing things to your code changes:

if you look at the implementation of Magento's "$.nodeName" you will see that there is already a "lowerCase" comparison. So if ($.nodeName(element, "iframe")) { should be sufficient.
you don't check if the src attribute is empty.
the technique of dynamically creating a <a> tag, setting the href property (iframe SRC) and then reading the hostname did not work in our tests.A query via the js native new URL() seems to be more stable here.

Full patch:

Index: vendor/magento/module-page-cache/view/frontend/web/js/page-cache.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/vendor/magento/module-page-cache/view/frontend/web/js/page-cache.js b/vendor/magento/module-page-cache/view/frontend/web/js/page-cache.js
--- a/vendor/magento/module-page-cache/view/frontend/web/js/page-cache.js	
+++ b/vendor/magento/module-page-cache/view/frontend/web/js/page-cache.js	(date 1647797357355)
@@ -45,18 +45,6 @@
          * @param {jQuery} element - Comment holder
          */
         (function lookup(element) {
-            var iframeHostName;
-
-            // prevent cross origin iframe content reading
-            if ($(element).prop('tagName') === 'IFRAME') {
-                iframeHostName = $('<a>').prop('href', $(element).prop('src'))
-                    .prop('hostname');
-
-                if (window.location.hostname !== iframeHostName) {
-                    return [];
-                }
-            }
-
             /**
              * Rewrite jQuery contents().
              *
@@ -64,8 +52,21 @@
              */
             contents = function (elem) {
                 return $.map(elem, function (el) {
+                    let isIframe = $.nodeName(element, "iframe");
+                    if (isIframe) {
+                        let iframeSource = $(element).prop('src');
+                        if(iframeSource.length) {
+                            let iframeDomain = (new URL(iframeSource));
+                            if (window.location.hostname !== iframeDomain.hostname) {
+                                return []; // src not origin
+                            }
+                        } else {
+                            return []; // src is emtpy
+                        }
+                    }
+                    
                     try {
-                        return $.nodeName(el, 'iframe') ?
+                        return isIframe ?
                             el.contentDocument || (el.contentWindow ? el.contentWindow.document : []) :
                             $.merge([], el.childNodes);
                     } catch (e) {

Note: I didn't read the note on your conversation "$(element) will be array of 2+ elements" until now. So it seems to make sense to put this check inside the contents function. (Updated full patch)

Could you check this once ?

engcom-Bravo · 2024-04-15T07:13:19Z

Hi @ihor-sviziev,

As per this #32264 (comment) We are closing this PR.

Please feel free to reopen the PR.

Thanks.

Prevent cross origin iframe content reading

64306eb

magento-engcom-team added Area: Frontend Component: PageCache Release Line: 2.4 labels Feb 22, 2021

ihor-sviziev marked this pull request as ready for review February 22, 2021 15:04

m2-community-project bot added the Progress: pending review label Feb 22, 2021

Prevent cross origin iframe content reading

73ed0e5

Fix static test

gabrieldagama added the Priority: P3 May be fixed according to the position in the backlog. label Feb 23, 2021

m2-assistant bot mentioned this pull request Feb 23, 2021

[Issue] Prevent cross origin iframe content reading #32264

Closed

4 tasks

Den4ik self-assigned this May 26, 2021

m2-community-project bot added Progress: review and removed Progress: pending review labels May 26, 2021

ihor-sviziev added 2 commits May 26, 2021 16:24

Merge branch '2.4-develop' into patch-7

bb3d5ad

Prevent cross origin iframe content reading

a531184

Den4ik approved these changes May 26, 2021

View reviewed changes

m2-community-project bot added Progress: ready for testing and removed Progress: review labels May 26, 2021

ihor-sviziev mentioned this pull request Aug 18, 2021

Improve requirejs load files #33483

Closed

5 tasks

engcom-Bravo self-assigned this Apr 15, 2024

engcom-Bravo closed this Apr 15, 2024

m2-community-project bot removed the Progress: ready for testing label Apr 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent cross origin iframe content reading #32244

Prevent cross origin iframe content reading #32244

ihor-sviziev commented Feb 22, 2021 •

edited by m2-assistant bot

Loading

m2-assistant bot commented Feb 22, 2021

ihor-sviziev commented Feb 23, 2021

gabrieldagama commented Feb 23, 2021

ihor-sviziev commented Feb 24, 2021

ihor-sviziev commented Feb 26, 2021

ihor-sviziev commented Mar 3, 2021

ihor-sviziev commented Mar 3, 2021

Den4ik commented May 26, 2021

magento-automated-testing bot commented May 26, 2021

ihor-sviziev commented May 26, 2021

magento-automated-testing bot commented May 26, 2021

Den4ik commented May 26, 2021

magento-automated-testing bot commented May 26, 2021

Den4ik left a comment

magento-engcom-team commented May 26, 2021

sfritzsche commented Mar 20, 2022 •

edited

Loading

engcom-Bravo commented Apr 15, 2024

Prevent cross origin iframe content reading #32244

Prevent cross origin iframe content reading #32244

Conversation

ihor-sviziev commented Feb 22, 2021 • edited by m2-assistant bot Loading

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

Resolved issues:

m2-assistant bot commented Feb 22, 2021

ihor-sviziev commented Feb 23, 2021

gabrieldagama commented Feb 23, 2021

ihor-sviziev commented Feb 24, 2021

ihor-sviziev commented Feb 26, 2021

ihor-sviziev commented Mar 3, 2021

ihor-sviziev commented Mar 3, 2021

Den4ik commented May 26, 2021

magento-automated-testing bot commented May 26, 2021

ihor-sviziev commented May 26, 2021

magento-automated-testing bot commented May 26, 2021

Den4ik commented May 26, 2021

magento-automated-testing bot commented May 26, 2021

Den4ik left a comment

Choose a reason for hiding this comment

magento-engcom-team commented May 26, 2021

sfritzsche commented Mar 20, 2022 • edited Loading

engcom-Bravo commented Apr 15, 2024

ihor-sviziev commented Feb 22, 2021 •

edited by m2-assistant bot

Loading

sfritzsche commented Mar 20, 2022 •

edited

Loading