New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 662506 link

Starred by 6 users
Status: Fixed
Owner:
█ binlu@chromium.org
Last visit > 30 days ago
Closed: Apr 2017
Cc:
█ mkwst@chromium.org
ojan@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Blocking top-level navigation w/o user gesture in sandboxed iframe

Project Member Reported by bi...@google.com, Nov 4 2016 
UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.71 Safari/537.36

Steps to reproduce the problem:

What is the expected behavior?
With the "allow-top-navigation" flag, sandboxed iframe should allow top-level navigation only when there is/has even been user gesture on the sandboxed iframe.

This change would enable a lot of those use cases of allowing top navigation while blocking malicious auto-redirecting from third-party resources (e.g, ads).

What went wrong?
When adding untrusted third-party content (e.g, ads) on the site, web developer could choose to put them inside sandboxed iframe to limit what they can do. 

But currently, sandboxed iframe either blocks all top-level navigation or allow all kinds of top-level navigation with "allow-top-navigation" including auto-redirecting w/o user gesture.

This blocks many use cases of sandboxing in iframe.

Did this work before? No 

Does this work in other browsers? No

Chrome version: 54.0.2840.71  Channel: n/a
OS Version: OS X 10.11.6
Flash Version: Shockwave Flash 23.0 r0

Comment 1 by krajshree@chromium.org, Nov 7 2016

Cc: krajshree@chromium.org
Labels: Needs-Feedback
binlu@ - Could you please provide a sample URL to test this issue.

Thanks...!!

Comment 2 by bi...@google.com, Nov 9 2016 

Hi krajshree@, Could you assign this to mkwst@ (who is planning to take a look at this)?
Project Member

Comment 3 by sheriffbot@chromium.org, Nov 17 2016

Labels: -Needs-Feedback Needs-Review
Owner: krajshree@chromium.org
Thank you for providing more feedback. Adding requester "krajshree@chromium.org" for another review and adding "Needs-Review" label for tracking.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 4 by mkwst@chromium.org, Nov 17 2016

Cc: -krajshree@chromium.org ojan@chromium.org
Components: Blink>HTML>IFrame
Labels: -Needs-Review -OS-Mac OS-All
Owner: mkwst@chromium.org
Status: Assigned (was: Unconfirmed)
Project Member

Comment 5 by bugdroid1@chromium.org, Jan 6 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3180362167493d3261e31d0cb4a7fa1f5180683f

commit 3180362167493d3261e31d0cb4a7fa1f5180683f
Author: binlu <binlu@google.com>
Date: Fri Jan 06 22:19:27 2017

Add UserCounter for Top Navigation in sandbox

To tell us the % of page views that do top navigations in sandboxed
iframes w/ or w/o 'allow-top-navigation' and user gesture.

This is for this intent to implement/ship feature:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Flt2IixYQK4

BUG= 662506 

Review-Url: https://codereview.chromium.org/2614073004
Cr-Commit-Position: refs/heads/master@{#442072}

[modify] https://crrev.com/3180362167493d3261e31d0cb4a7fa1f5180683f/third_party/WebKit/Source/core/frame/Frame.cpp
[modify] https://crrev.com/3180362167493d3261e31d0cb4a7fa1f5180683f/third_party/WebKit/Source/core/frame/UseCounter.h
[modify] https://crrev.com/3180362167493d3261e31d0cb4a7fa1f5180683f/tools/metrics/histograms/histograms.xml

Comment 6 by bi...@google.com, Jan 9 2017 

Hi Release Manager,

Could we merge this above CL which collects metrics for top-navigation usage in sandbox into M56? It's for an important feature which will enable us to sandbox more ads to prevent malicious behaviors like auto-redirecting users to phishing/malware web sites. We'd like to ship this feature asap which is a P0 for this quarter. Previously, this has been hold back because we were hoping another effort could save us from doing this, but it's got postponed (https://bugs.chromium.org/p/chromium/issues/detail?id=678328). Therefore, we'd like to merge this CL to M56 to collect metrics, hoping this will save us a release cycle.

Please let me know if more info is needed!

Thanks,
Bin

Comment 7 by mkwst@chromium.org, Jan 10 2017

Labels: Merge-Request-56
Adding the merge-request label for Bin, as per comment #6.
Project Member

Comment 8 by sheriffbot@chromium.org, Jan 10 2017

Labels: -Merge-Request-56 Hotlist-Merge-Approved Merge-Approved-56
Your change meets the bar and is auto-approved for M56. Please go ahead and merge the CL manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), gkihumba@(cros), bustamante@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 9 by bugdroid1@chromium.org, Jan 10 2017

Labels: -merge-approved-56 merge-merged-2924
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5f34cba74251583df913a4fdd8fd0f4529c96adf

commit 5f34cba74251583df913a4fdd8fd0f4529c96adf
Author: gogerald <gogerald@google.com>
Date: Tue Jan 10 16:22:23 2017

Add UserCounter for Top Navigation in sandbox

To tell us the % of page views that do top navigations in sandboxed
iframes w/ or w/o 'allow-top-navigation' and user gesture.

This is for this intent to implement/ship feature:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Flt2IixYQK4

BUG= 662506 

Review-Url: https://codereview.chromium.org/2614073004
Cr-Commit-Position: refs/heads/master@{#442072}
(cherry picked from commit 3180362167493d3261e31d0cb4a7fa1f5180683f)

Review-Url: https://codereview.chromium.org/2621043002 .
Cr-Commit-Position: refs/branch-heads/2924@{#715}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/5f34cba74251583df913a4fdd8fd0f4529c96adf/third_party/WebKit/Source/core/frame/Frame.cpp
[modify] https://crrev.com/5f34cba74251583df913a4fdd8fd0f4529c96adf/third_party/WebKit/Source/core/frame/UseCounter.h
[modify] https://crrev.com/5f34cba74251583df913a4fdd8fd0f4529c96adf/tools/metrics/histograms/histograms.xml
Project Member

Comment 10 by bugdroid1@chromium.org, Jan 19 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7a2bf4d888a500a9db0772ce02a88adbb7be6aee

commit 7a2bf4d888a500a9db0772ce02a88adbb7be6aee
Author: binlu <binlu@google.com>
Date: Thu Jan 19 09:20:40 2017

Add an 'allow-top-navigation-with-user-interaction' sandbox flag.

This is a new flag for `<iframe sandbox="...">` which will allow a
sandboxed document to navigate top-level page only with a user activation (aka. gesture). This will allow, for example, a third-party advertisement to be safely sandboxed without breaking existing sandboxed contents.

Intent to Implement & Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/Flt2IixYQK4/RKMfll65AgAJ

BUG= 662506 

Review-Url: https://codereview.chromium.org/2645733002
Cr-Commit-Position: refs/heads/master@{#444687}

[add] https://crrev.com/7a2bf4d888a500a9db0772ce02a88adbb7be6aee/third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/resources/iframe-that-performs-top-navigation-without-user-gesture-failed.html
[add] https://crrev.com/7a2bf4d888a500a9db0772ce02a88adbb7be6aee/third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/sandbox-ALLOWED-top-navigation-with-user-gesture-expected.txt
[add] https://crrev.com/7a2bf4d888a500a9db0772ce02a88adbb7be6aee/third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/sandbox-ALLOWED-top-navigation-with-user-gesture.html
[add] https://crrev.com/7a2bf4d888a500a9db0772ce02a88adbb7be6aee/third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/sandbox-DENIED-top-navigation-without-user-gesture-expected.txt
[add] https://crrev.com/7a2bf4d888a500a9db0772ce02a88adbb7be6aee/third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/sandbox-DENIED-top-navigation-without-user-gesture.html
[modify] https://crrev.com/7a2bf4d888a500a9db0772ce02a88adbb7be6aee/third_party/WebKit/Source/core/dom/SandboxFlags.cpp
[modify] https://crrev.com/7a2bf4d888a500a9db0772ce02a88adbb7be6aee/third_party/WebKit/Source/core/dom/SandboxFlags.h
[modify] https://crrev.com/7a2bf4d888a500a9db0772ce02a88adbb7be6aee/third_party/WebKit/Source/core/frame/Frame.cpp
[modify] https://crrev.com/7a2bf4d888a500a9db0772ce02a88adbb7be6aee/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
Project Member

Comment 11 by bugdroid1@chromium.org, Feb 23 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8926e98b4c2426be840b2deb8401e501b971f045

commit 8926e98b4c2426be840b2deb8401e501b971f045
Author: binlu <binlu@google.com>
Date: Thu Feb 23 03:12:24 2017

Rename the flag of 'allow-top-navigation-with-user-activation' to 'allow-top-navigation-by-user-activation' to be consistent with the spec proposal: http://github.com/whatwg/html/pull/2292.

Also changed the tests to be testharness-based for the merging into web-platform-tests.

BUG= 662506 

Review-Url: https://codereview.chromium.org/2696703010
Cr-Commit-Position: refs/heads/master@{#452368}

[add] https://crrev.com/8926e98b4c2426be840b2deb8401e501b971f045/third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/resources/iframe-that-performs-top-navigation-succeeded.html
[add] https://crrev.com/8926e98b4c2426be840b2deb8401e501b971f045/third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/sandbox-ALLOWED-top-navigation-with-two-flags-expected.txt
[add] https://crrev.com/8926e98b4c2426be840b2deb8401e501b971f045/third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/sandbox-ALLOWED-top-navigation-with-two-flags.html
[modify] https://crrev.com/8926e98b4c2426be840b2deb8401e501b971f045/third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/sandbox-ALLOWED-top-navigation-with-user-gesture.html
[delete] https://crrev.com/26664a7d9d2e37eeaaf5f565cb651a4f37c4e438/third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/sandbox-DENIED-top-navigation-without-user-gesture-expected.txt
[modify] https://crrev.com/8926e98b4c2426be840b2deb8401e501b971f045/third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/sandbox-DENIED-top-navigation-without-user-gesture.html
[modify] https://crrev.com/8926e98b4c2426be840b2deb8401e501b971f045/third_party/WebKit/Source/core/dom/SandboxFlags.cpp
[modify] https://crrev.com/8926e98b4c2426be840b2deb8401e501b971f045/third_party/WebKit/Source/core/dom/SandboxFlags.h
[modify] https://crrev.com/8926e98b4c2426be840b2deb8401e501b971f045/third_party/WebKit/Source/core/frame/Frame.cpp
[modify] https://crrev.com/8926e98b4c2426be840b2deb8401e501b971f045/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5
Project Member

Comment 12 by bugdroid1@chromium.org, Feb 23 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/290032568531b964b232611699a358900bdb789f

commit 290032568531b964b232611699a358900bdb789f
Author: binlu <binlu@google.com>
Date: Thu Feb 23 20:35:47 2017

WPT tests for allow-top-navigation-by-user-activation for iframe@sandbox.

Move the test files for disallowing top navigation given no gesture to WPT, and also adding a manual test for allowing top navigation with user gesture.

BUG= 662506 

Review-Url: https://codereview.chromium.org/2711073002
Cr-Commit-Position: refs/heads/master@{#452609}

[modify] https://crrev.com/290032568531b964b232611699a358900bdb789f/third_party/WebKit/LayoutTests/external/wpt/MANIFEST.json
[add] https://crrev.com/290032568531b964b232611699a358900bdb789f/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation_by_user_activation-manual.html
[rename] https://crrev.com/290032568531b964b232611699a358900bdb789f/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation_by_user_activation_without_user_gesture.html
[rename] https://crrev.com/290032568531b964b232611699a358900bdb789f/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-iframe-element/support/iframe-that-performs-top-navigation-without-user-gesture-failed.html
[add] https://crrev.com/290032568531b964b232611699a358900bdb789f/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-iframe-element/support/iframe-that-performs-top-navigation.html
[add] https://crrev.com/290032568531b964b232611699a358900bdb789f/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-iframe-element/support/navigation-changed-iframe.html
Project Member

Comment 13 by bugdroid1@chromium.org, Feb 27 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/edeee45ff630ec55e37944197900c3b5846631c7

commit edeee45ff630ec55e37944197900c3b5846631c7
Author: binlu <binlu@google.com>
Date: Mon Feb 27 20:50:39 2017

Turn on the flag for 'allow-top-navigation-by-user-activation'.

Change the feature of TopNavByUserActivationInSandbox from "experimental" to "stable".
Refine the words in the WPT test for 'allow-top-navigation-by-user-activation'.

Intent to ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/El0ueyoNgRE

BUG= 662506 

Review-Url: https://codereview.chromium.org/2713283002
Cr-Commit-Position: refs/heads/master@{#453327}

[modify] https://crrev.com/edeee45ff630ec55e37944197900c3b5846631c7/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation_by_user_activation-manual.html
[modify] https://crrev.com/edeee45ff630ec55e37944197900c3b5846631c7/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-iframe-element/support/iframe-that-performs-top-navigation.html
[modify] https://crrev.com/edeee45ff630ec55e37944197900c3b5846631c7/third_party/WebKit/LayoutTests/fast/frames/sandboxed-iframe-close-top-noclose-expected.txt
[modify] https://crrev.com/edeee45ff630ec55e37944197900c3b5846631c7/third_party/WebKit/LayoutTests/fast/frames/sandboxed-iframe-navigation-top-denied-expected.txt
[modify] https://crrev.com/edeee45ff630ec55e37944197900c3b5846631c7/third_party/WebKit/LayoutTests/http/tests/navigation/new-window-sandboxed-iframe-expected.txt
[modify] https://crrev.com/edeee45ff630ec55e37944197900c3b5846631c7/third_party/WebKit/LayoutTests/http/tests/security/no-popup-from-sandbox-top-expected.txt
[modify] https://crrev.com/edeee45ff630ec55e37944197900c3b5846631c7/third_party/WebKit/LayoutTests/http/tests/security/sandboxed-iframe-form-top-expected.txt
[modify] https://crrev.com/edeee45ff630ec55e37944197900c3b5846631c7/third_party/WebKit/Source/core/frame/Frame.cpp
[modify] https://crrev.com/edeee45ff630ec55e37944197900c3b5846631c7/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5

Comment 14 by bi...@google.com, Apr 10 2017

Cc: mkwst@chromium.org
Owner: binlu@chromium.org
Status: Fixed (was: Assigned)
Marking this as fixed since it's officially in M58.
Project Member

Comment 15 by bugdroid1@chromium.org, Mar 16 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/28620663e5ff743874e295c24950c4d244522a20

commit 28620663e5ff743874e295c24950c4d244522a20
Author: Bin Lu <binlu@chromium.org>
Date: Fri Mar 16 17:04:05 2018

Add missing flags to the iframe sandbox supported list.

Add missing flags to the iframe sandbox supported list, and add tests.
Also remove the runtime config for 'allow-top-navigation-by-user-activation' since the feature has shipped a long time ago.

Bug:  662506 
Change-Id: I458210f6133ddd5e30b507214b003ff0fb596761
Reviewed-on: https://chromium-review.googlesource.com/965843
Commit-Queue: Bin Lu <binlu@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543733}
[modify] https://crrev.com/28620663e5ff743874e295c24950c4d244522a20/third_party/WebKit/LayoutTests/fast/dom/HTMLIFrameElement/sandbox-feature-detection.html
[modify] https://crrev.com/28620663e5ff743874e295c24950c4d244522a20/third_party/WebKit/Source/core/html/HTMLIFrameElementSandbox.cpp

Comment 16 by binlu@chromium.org, Mar 16 2018

Labels: Merge-Request-66
Adding the merge-request label for the above CL:
https://chromium.googlesource.com/chromium/src.git/+/28620663e5ff743874e295c24950c4d244522a20.

It was forgot in Chrome M58, and is blocking AMP's usage of iframe sandbox on <amp-ad> to block malicious ads. The request merge will save us a release cycle.

Please let me know if more info is needed. Thanks.

Comment 17 by abdulsyed@google.com, Mar 16 2018 

Let's wait until this has been verified in Canary and we can review it on Monday if changes look good.

Comment 18 by binlu@chromium.org, Mar 16 2018 

SG. Just let me know if there is any problem. Thanks Abdul.
Project Member

Comment 19 by sheriffbot@chromium.org, Mar 17 2018

Labels: -Merge-Request-66 Merge-Review-66 Hotlist-Merge-Review
This bug requires manual review: M66 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 20 by abdulsyed@google.com, Mar 19 2018 

How does it look in canary?

Comment 21 by binlu@chromium.org, Mar 19 2018 

Just tested on Canary build for Mac, and it works as expected. 

The script used is attached below:
<script>
var iframe = document.createElement('iframe');
if (iframe.sandbox.supports('allow-top-navigation-by-user-activation')) {
  alert('support tp');
}
</script>

Comment 22 by abdulsyed@google.com, Mar 20 2018

Labels: -Merge-Review-66 Merge-Approved-66
Approving merge for M66. Branch:3359
Project Member

Comment 23 by bugdroid1@chromium.org, Mar 20 2018

Labels: -merge-approved-66 merge-merged-3359
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aa16224b6dd03cf35c5167cc5a2ce11b533b63b1

commit aa16224b6dd03cf35c5167cc5a2ce11b533b63b1
Author: gogerald <gogerald@google.com>
Date: Tue Mar 20 17:16:31 2018

Add missing flags to the iframe sandbox supported list.

Add missing flags to the iframe sandbox supported list, and add tests.
Also remove the runtime config for 'allow-top-navigation-by-user-activation' since the feature has shipped a long time ago.

TBR=binlu@chromium.org

(cherry picked from commit 28620663e5ff743874e295c24950c4d244522a20)

Bug:  662506 
Change-Id: I458210f6133ddd5e30b507214b003ff0fb596761
Reviewed-on: https://chromium-review.googlesource.com/965843
Commit-Queue: Bin Lu <binlu@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#543733}
Reviewed-on: https://chromium-review.googlesource.com/971243
Reviewed-by: Ganggui Tang <gogerald@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#347}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/aa16224b6dd03cf35c5167cc5a2ce11b533b63b1/third_party/WebKit/LayoutTests/fast/dom/HTMLIFrameElement/sandbox-feature-detection.html
[modify] https://crrev.com/aa16224b6dd03cf35c5167cc5a2ce11b533b63b1/third_party/WebKit/Source/core/html/HTMLIFrameElementSandbox.cpp
►

Sign in to add a comment