+dcheng and creis, who also ran into issues comparing unique origins with url::Origin and/or blink::SecurityOrigin.  Daniel had an idea that we could just maintain a GUID in the origin and make it survive the SecurityOrigin/url::Origin roundtrips.

Yeah, it's problematic that sending an origin from Blink to Chrome is lossy and loses information about the unique origin. This has also affected some code for media router (https://codereview.chromium.org/2627463003/#msg110).

My suggestion is just to union a GUID stored in binary form with the scheme/host/port/suborigin (?) tuple and any other random bits that are mutually exclusive with being a unique origin, but there might be some subtleties I'm missing =)

iclelland: Do we need to allow unique origins to appear in FP? Do you have any examples of cases where this is needed?

I kind of feel like we should avoid allowing delegation of features to data/about:blank/file URLs and that would actually be a nice feature to have. We're actively moving away from allowing these origins to have powerful capabilities. But maybe I'm missing a critical use case?

I don't know if there's a case for opaque origins to explicitly appear in a declared policy. We might need to be able to reference them implicitly: if a sandboxed document is delivered with a FP header which uses 'self', then that needs to refer to the unique sandbox origin.

I suppose it's a separate issue, whether sandboxed frames should be able to use feature like fullscreen or midi or geolocation. My understanding was that, even if it's odd by default, the embedder should be able to grant those frames any permissions that are removed by sandboxing.

Critically, I do think that on-by-default features will need to work in any frame, opaque origin or not. As soon as we use FP to implement something like document.write, then we'll need to ensure that the policy mechanism actually allows that to happen, and doesn't block it because the document's origin can't be same-origin with the one we stored in the policy.

We should probably move this discussion to the mailing list.

This sounds like something that might potentially need to block shipping of Strict Site Isolation to the stable channel.  Does that sound right?  Should we add the Proj-SiteIsolation-LaunchBlocking label?

Adding label out of caution, assuming that this affects an already launched feature.  (Is that correct?)

Sounds like a fix is on the way in  issue 794631 ?

This affects the ability to use the iframe 'allow' attribute to enable or disable feature-policy controlled features, like fullscreen, webusb, paymentrequest, synchronous XHR, and permissions (camera, mic, geolocation, midi, etc) in sandboxed frames. This is already launched, and currently working without site isolation, yes.

(The allowfullscreen and allowpaymentrequest attributes are not affected, since they implicitly grant access to *all* origins, including sandboxed ones)

And yes, the fix in that issue is also going to apply to this one.

There are two steps roughly for fixing this: The first part is getting the constructed policy in the new frame to recognize that the opaque origin mentioned matches its own origin. Since we don't even know that origin at the time that the container policy is constructed; only when the frame commits its initial navigation, we'll need to tag each declared allowlist with a "matches the opaque origin" bool. (Under development in https://crrev.com/c/963382)

This should fix everything except for navigations -- there are no same-origin navigations from an opaque frame, *except* that navigating forwards and then backwards should return you to the same origin you started at, even when opaque. To fix this, we'll actually need to tag each opaque origin with an identity, so that we can recognize when the frame has returned to a previously-authorized origin.

The design for fixing this is here: https://docs.google.com/document/d/1m3FYAKRjdz5wEclkT_qdZXSINIG4eK54jUo5L90LSDU/edit#heading=h.6xz05leuhwuj

iclelland@: Thanks for the update.  Will this be doable for M67?

This should be doable -- the current CL at https://crrev.com/c/963382 is the first part, and I'm hoping to land that soon.

With that, features in sandboxed frames will work much like sandbox flags do; they will work, and will apply to any document loaded in that frame. Navigations won't affect the status of the feature, as long as the allow attribute of the containing frame hasn't been changed.

The rest of the solution will give finer control over exactly which document is allowed to use the features, and will target M68, I think. This first part, however, should be sufficient to avoid regressions with site isolation.

Fantastic!  Thanks for the update, and for the extra effort to get this first part into M67.

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

commit 2c0351ed93162a0b1390324c9a4cd2bf14e95398
Author: Ian Clelland <iclelland@chromium.org>
Date: Fri Apr 13 01:44:38 2018

Allow feature policy to be used in opaque origins.

Currently, policy-controlled features do not work as expected in
frames with opaque origins, such as isolated sandboxes and data: URLs,
because the eventual opaque origin of the frame is not known when the
HTMLFrameOwnerElement builds the container policy, and so has no way
to tell the browser that a particular origin should be allowed.

This CL adds a new member to the ParsedFeaturePolicyDeclaration, which
indicates that the iframe policy is expected to apply to the origin of
the frame, and is used when that frame has an opaque origin. This can
be triggered with an iframe of the form

<iframe sandbox allow="feature">

or

<iframe sandbox allow="feature src">

This flag is checked when building the feature policy in the new frame,
and ensures that the new feature policy will allow the feature in that
origin.

This is the first part of the eventual solution -- currently this has
the effect of allowing the feature even if a sandboxed frame navigates
to a new page (causing a new opaque origin to be created for it).
Subsequent CLs will add a unique identified to each such origin, and
ensure that the generated policies are properly tied to the specific
origin of the frame.

Bug:  690520 
Change-Id: Ie18b9bc3c36be6550baf5a03e355871b9589fd40
Reviewed-on: https://chromium-review.googlesource.com/963382
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550463}
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/content/common/frame_messages.h
[add] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/content/test/data/page_with_data_iframe_and_allow.html
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/third_party/WebKit/LayoutTests/external/wpt/feature-policy/feature-policy-frame-policy-allowed-for-self.https.sub.html
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/third_party/WebKit/LayoutTests/external/wpt/feature-policy/resources/featurepolicy.js
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/third_party/blink/common/feature_policy/feature_policy.cc
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/third_party/blink/common/feature_policy/feature_policy_unittest.cc
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/third_party/blink/public/common/feature_policy/feature_policy.h
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/third_party/blink/renderer/core/html/html_iframe_element_test.cc
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/third_party/blink/renderer/platform/feature_policy/feature_policy.cc
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/third_party/blink/renderer/platform/feature_policy/feature_policy_test.cc

Thanks for landing r550463, Ian!  Looks like that narrowly missed the branch.  Can you confirm it when it gets to Canary and request merge?

Yeah, I was just looking at that -- I spent a couple of hours working up
some final tests, and that probably pushed it past the cutoff. I'll do a
merge request as soon as I've verified it in ToT.

Thanks,
Ian

Just missed the branch cutoff :(

Requesting merge back to M67

Verified in Canary 68.0.3397.0, has been stable on ToT since Apr 14.

Your change meets the bar and is auto-approved for M67. Please go ahead and merge the CL to branch 3396 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

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

Pls merge your change to M67 branch 3396 by 1:00 PM PT today, Tuesday (04/17/18) so we can pick it up for tomorrow's M67 dev release. Thank you.

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

commit 2c0351ed93162a0b1390324c9a4cd2bf14e95398
Author: Ian Clelland <iclelland@chromium.org>
Date: Fri Apr 13 01:44:38 2018

Allow feature policy to be used in opaque origins.

Currently, policy-controlled features do not work as expected in
frames with opaque origins, such as isolated sandboxes and data: URLs,
because the eventual opaque origin of the frame is not known when the
HTMLFrameOwnerElement builds the container policy, and so has no way
to tell the browser that a particular origin should be allowed.

This CL adds a new member to the ParsedFeaturePolicyDeclaration, which
indicates that the iframe policy is expected to apply to the origin of
the frame, and is used when that frame has an opaque origin. This can
be triggered with an iframe of the form

<iframe sandbox allow="feature">

or

<iframe sandbox allow="feature src">

This flag is checked when building the feature policy in the new frame,
and ensures that the new feature policy will allow the feature in that
origin.

This is the first part of the eventual solution -- currently this has
the effect of allowing the feature even if a sandboxed frame navigates
to a new page (causing a new opaque origin to be created for it).
Subsequent CLs will add a unique identified to each such origin, and
ensure that the generated policies are properly tied to the specific
origin of the frame.

Bug:  690520 
Change-Id: Ie18b9bc3c36be6550baf5a03e355871b9589fd40
Reviewed-on: https://chromium-review.googlesource.com/963382
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550463}
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/content/common/frame_messages.h
[add] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/content/test/data/page_with_data_iframe_and_allow.html
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/third_party/WebKit/LayoutTests/external/wpt/feature-policy/feature-policy-frame-policy-allowed-for-self.https.sub.html
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/third_party/WebKit/LayoutTests/external/wpt/feature-policy/resources/featurepolicy.js
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/third_party/blink/common/feature_policy/feature_policy.cc
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/third_party/blink/common/feature_policy/feature_policy_unittest.cc
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/third_party/blink/public/common/feature_policy/feature_policy.h
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/third_party/blink/renderer/core/html/html_iframe_element_test.cc
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/third_party/blink/renderer/platform/feature_policy/feature_policy.cc
[modify] https://crrev.com/2c0351ed93162a0b1390324c9a4cd2bf14e95398/third_party/blink/renderer/platform/feature_policy/feature_policy_test.cc

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

commit cdb72d4c103012aa903ab852ef1210d23b940c02
Author: Ian Clelland <iclelland@chromium.org>
Date: Tue Apr 17 20:31:56 2018

Allow feature policy to be used in opaque origins.

Currently, policy-controlled features do not work as expected in
frames with opaque origins, such as isolated sandboxes and data: URLs,
because the eventual opaque origin of the frame is not known when the
HTMLFrameOwnerElement builds the container policy, and so has no way
to tell the browser that a particular origin should be allowed.

This CL adds a new member to the ParsedFeaturePolicyDeclaration, which
indicates that the iframe policy is expected to apply to the origin of
the frame, and is used when that frame has an opaque origin. This can
be triggered with an iframe of the form

<iframe sandbox allow="feature">

or

<iframe sandbox allow="feature src">

This flag is checked when building the feature policy in the new frame,
and ensures that the new feature policy will allow the feature in that
origin.

This is the first part of the eventual solution -- currently this has
the effect of allowing the feature even if a sandboxed frame navigates
to a new page (causing a new opaque origin to be created for it).
Subsequent CLs will add a unique identified to each such origin, and
ensure that the generated policies are properly tied to the specific
origin of the frame.

TBR=iclelland@chromium.org

(cherry picked from commit 2c0351ed93162a0b1390324c9a4cd2bf14e95398)

Bug:  690520 
Change-Id: Ie18b9bc3c36be6550baf5a03e355871b9589fd40
Reviewed-on: https://chromium-review.googlesource.com/963382
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#550463}
Reviewed-on: https://chromium-review.googlesource.com/1015764
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#59}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/cdb72d4c103012aa903ab852ef1210d23b940c02/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/cdb72d4c103012aa903ab852ef1210d23b940c02/content/common/frame_messages.h
[add] https://crrev.com/cdb72d4c103012aa903ab852ef1210d23b940c02/content/test/data/page_with_data_iframe_and_allow.html
[modify] https://crrev.com/cdb72d4c103012aa903ab852ef1210d23b940c02/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process
[modify] https://crrev.com/cdb72d4c103012aa903ab852ef1210d23b940c02/third_party/WebKit/LayoutTests/external/wpt/feature-policy/feature-policy-frame-policy-allowed-for-self.https.sub.html
[modify] https://crrev.com/cdb72d4c103012aa903ab852ef1210d23b940c02/third_party/WebKit/LayoutTests/external/wpt/feature-policy/resources/featurepolicy.js
[modify] https://crrev.com/cdb72d4c103012aa903ab852ef1210d23b940c02/third_party/blink/common/feature_policy/feature_policy.cc
[modify] https://crrev.com/cdb72d4c103012aa903ab852ef1210d23b940c02/third_party/blink/common/feature_policy/feature_policy_unittest.cc
[modify] https://crrev.com/cdb72d4c103012aa903ab852ef1210d23b940c02/third_party/blink/public/common/feature_policy/feature_policy.h
[modify] https://crrev.com/cdb72d4c103012aa903ab852ef1210d23b940c02/third_party/blink/renderer/core/html/html_iframe_element_test.cc
[modify] https://crrev.com/cdb72d4c103012aa903ab852ef1210d23b940c02/third_party/blink/renderer/platform/feature_policy/feature_policy.cc
[modify] https://crrev.com/cdb72d4c103012aa903ab852ef1210d23b940c02/third_party/blink/renderer/platform/feature_policy/feature_policy_test.cc

Thanks for getting that merged, Ian!

Did you want to leave this open for the longer term fix, or close this and open a new one?  If we leave it open we can remove the LaunchBlocking label, but it might be cleaner (given the merge labels here) to open a new bug for the followup work.

SGTM; closing the launch blocker.

I was thinking about the merge labels earlier; it's too bad that they apply to the issue, and not to a particular commit or even a comment, but that's what we've got for now :) I'll move the blocked-ons to the new issue to keep it all clean.