Block framebusts without a user gesture

Block framebusts without a user gesture

Intent to deprecate and remove: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Xi8-y4ySjA4

BUG=624061
Committed: https://crrev.com/b9aa142c91bea6d08117589fbcd00183fce21322
Cr-Commit-Position: refs/heads/master@{#417102}

[Nate Chapin, 2016-06-28 21:28:13]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-28 21:29:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 22:19:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-28 22:19:30]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Nate Chapin, 2016-08-22 23:52:23]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 23:52:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 00:25:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 00:25:44]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Nate Chapin, 2016-08-25 22:51:01]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 22:51:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 00:52:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 00:52:33]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2016-08-29 20:20:20]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 20:41:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 03:17:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 03:17:02]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Nate Chapin, 2016-08-30 20:00:16]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 20:00:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 20:05:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 20:05:18]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Nate Chapin, 2016-08-30 20:49:28]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 20:50:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 22:47:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 22:47:51]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2016-08-30 22:51:14]

japhet@chromium.org changed reviewers:
+ mkwst@chromium.org, ojan@chromium.org

[Nate Chapin, 2016-08-30 22:51:15]

mkwst, PTAL.

ojan, FYI.

Any opinions on the need for a flag for this? Given that this will hopefully be
on dev for a few weeks before a branch, there should be plenty of time to revert
if things go poorly?

[Nate Chapin, 2016-08-30 22:52:28]

Description was changed from

==========
Block framebusts without a user gesture

BUG=
==========

to

==========
Block framebusts without a user gesture

Intent to deprecate and remove:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Xi8-y4ySjA4

BUG=624061
==========

[ojan, 2016-08-30 23:36:42]

lgtm. ship it!

https://codereview.chromium.org/2092293002/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2092293002/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15207: +    <message
name="IDS_FLAGS_FONT_CACHE_SCALING_DESCRIPTION" desc="Description for the flag
to enable Framebusting restrictions">
I think you've got some issues in the diff here:
IDS_FLAGS_FONT_CACHE_SCALING_DESCRIPTION and FontCache.

[Mike West, 2016-08-31 13:14:48]

https://codereview.chromium.org/2092293002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/2092293002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/Frame.cpp:185: if
(!RuntimeEnabledFeatures::framebustingNeedsSameOriginOrUserGestureEnabled())
Is the "same origin" bit accurate? I don't see a same-origin check in the code,
nor do I think it makes sense to add one (that is, it's not any good to allow
`evil.com` to navigate to itself at the top-level from inside a frame; that's
the same as allowing it to navigate anywhere else).

[Nate Chapin, 2016-08-31 16:31:08]

On 2016/08/31 13:14:48, Mike West (OOO until 29th) wrote:
>
https://codereview.chromium.org/2092293002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/Frame.cpp (right):
> 
>
https://codereview.chromium.org/2092293002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/Frame.cpp:185: if
> (!RuntimeEnabledFeatures::framebustingNeedsSameOriginOrUserGestureEnabled())
> Is the "same origin" bit accurate? I don't see a same-origin check in the
code,
> nor do I think it makes sense to add one (that is, it's not any good to allow
> `evil.com` to navigate to itself at the top-level from inside a frame; that's
> the same as allowing it to navigate anywhere else).

Same-origin is more-or-less accurate, where the iframe has to be same-origin to
the existing content in the top frame.

I was using that as a shorthand for the check in
canNavigateWithoutFramebusting(). When an iframe tries to navigate its own top
and it isn't sandboxed, canNavigateWithoutFramebusting() basically devolves into
a SecurityOrigin::canAccess() check.

[ojan, 2016-08-31 16:58:28]

Mike, I worry about breaking too much legitimate content if we break same origin
as well. Why do you want to disallow same origin?

[Nate Chapin, 2016-08-31 17:11:24]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 17:12:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 19:07:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-31 19:07:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mike West, 2016-09-01 12:47:45]

On 2016/08/31 at 16:58:28, ojan wrote:
> Mike, I worry about breaking too much legitimate content if we break same
origin as well. Why do you want to disallow same origin?

I wanted to make sure that `good.com` -> `evil.com` didn't allow `evil.com` to
framebust itself to `evil.com`. I agree that allowing `good.com` -> `good.com`
to framebust to `good.com` is fine (I mean, the framed `good.com` can reach up
into its parent and poke at its properties directly; navigation is the least of
our worries there).

If that's what you mean too, Nate (and looking at the code again, I think it
is), LGTM. :)

[Nate Chapin, 2016-09-06 19:45:25]

japhet@chromium.org changed reviewers:
+ jochen@chromium.org

[Nate Chapin, 2016-09-06 19:45:26]

jochen, would you mind reviewing the content/ changes?

[jochen (gone - plz use gerrit), 2016-09-07 14:24:54]

lgtm

https://codereview.chromium.org/2092293002/diff/140001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2092293002/diff/140001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15213: +    <message
name="IDS_FLAGS_FRAMEBUSTING_NAME" desc="Title for the flag to enable FontCache
Framebusting restrictions">
please rephrase this description so that the translation team can derive the
meaning of the string to be translated

[Nate Chapin, 2016-09-07 20:59:45]

On 2016/09/07 14:24:54, jochen wrote:
> lgtm
> 
>
https://codereview.chromium.org/2092293002/diff/140001/chrome/app/generated_r...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/2092293002/diff/140001/chrome/app/generated_r...
> chrome/app/generated_resources.grd:15213: +    <message
> name="IDS_FLAGS_FRAMEBUSTING_NAME" desc="Title for the flag to enable
FontCache
> Framebusting restrictions">
> please rephrase this description so that the translation team can derive the
> meaning of the string to be translated

Done. I *think* I defined all of my terms.

[Nate Chapin, 2016-09-07 21:00:15]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2016-09-07 21:00:15]

The patchset sent to the CQ was uploaded after l-g-t-m from ojan@chromium.org,
mkwst@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2092293002/#ps160001
(title: "better flag description")

[commit-bot: I haz the power, 2016-09-07 21:01:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 22:41:29]

Description was changed from

==========
Block framebusts without a user gesture

Intent to deprecate and remove:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Xi8-y4ySjA4

BUG=624061
==========

to

==========
Block framebusts without a user gesture

Intent to deprecate and remove:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Xi8-y4ySjA4

BUG=624061
==========

[commit-bot: I haz the power, 2016-09-07 22:41:30]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-09-07 22:43:11]

Description was changed from

==========
Block framebusts without a user gesture

Intent to deprecate and remove:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Xi8-y4ySjA4

BUG=624061
==========

to

==========
Block framebusts without a user gesture

Intent to deprecate and remove:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Xi8-y4ySjA4

BUG=624061

Committed: https://crrev.com/b9aa142c91bea6d08117589fbcd00183fce21322
Cr-Commit-Position: refs/heads/master@{#417102}
==========

[commit-bot: I haz the power, 2016-09-07 22:43:12]

Patchset 9 (id:??) landed as
https://crrev.com/b9aa142c91bea6d08117589fbcd00183fce21322
Cr-Commit-Position: refs/heads/master@{#417102}