Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add disallowdocumentaccess iframe attribute to force a creation of a new agent cluster map #4606

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dtapuska
Copy link
Contributor

@dtapuska dtapuska commented May 9, 2019

Add a disallowdocumentaccess iframe attribute to prevent access from one document to another.

Add steps that cause a newly created browsing context to either inherit the agent cluster map or
create a new one.

Given a nested browsing context (or child) with the disallowdocumentaccess parameter and an
iframe outside of the tree, the two browsing contexts will not be able to share data because
they will have different Agents (even if they are same-origin-domain).

Fixes #4435

(See WHATWG Working Mode: Changes for more details.)


💥 Error: Wattsi server error 💥

PR Preview failed to build. (Last tried on Jan 15, 2021, 7:59 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.

🔗 Related URL

<html>
<head><title>504 Gateway Time-out</title></head>
<body bgcolor="white">
<center><h1>504 Gateway Time-out</h1></center>
<hr><center>nginx/1.10.3</center>
</body>
</html>

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

@dtapuska
Copy link
Contributor Author

dtapuska commented May 9, 2019

@annevk @domenic
If this approach is ok. I'll continue iterating with tests and other implementation bugs.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a minor nit on my part. Otherwise, this seems to make sense to me, although @annevk is the expert.

I think the remaining "problem" is a preexisting one: that we only declaratively say "these agents are in the same cluster." For a long time @annevk and I have wanted to move to a model that explicitly allocates agents and agent clusters when new documents are created. That would make it clearer that at agent creation time, we consult the feature policy header/iframe's allow attribute, and use that to influence which agent cluster the agent goes in.

But, I see no problems with introducing this on top of the current paradigm. There is nothing introduced here that will make that future clarification harder to do.

This does mean we're closing the door on being able to deliver feature policy via in-document mechanisms like <meta>, since by that time it'll be too late (the agent will already be in its cluster). I suspect we're already pretty committed to that path, but /cc @clelland to make sure.

source Outdated
@@ -78127,6 +78130,8 @@ console.assert(iframeWindow.frameElement === null);
<h5><dfn>IsPlatformObjectSameOrigin</dfn> ( <var>O</var> )</h5>

<ol>
<li><p>If the <span>current global object</span> and <var>O</var> are not
<span>same-agent Window objects</span> return false.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work if O is a Location object. I think you want to check O's relevant global object.

Also, note how prevailing style is , then return X, not return X.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the primitives you're building this on are ready.

source Outdated
<li><p>If either <var>A</var>'s <span>relevant settings object</span>'s
<span>responsible document</span> or <var>B</var>'s <span>relevant settings object</span>'s
<span>responsible document</span> are not <span>allowed to use</span> the
"<code data-x="document-access">document-access</code>" feature, then return false.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feature Policy is being redesigned, so this doesn't quite fit I think, given https://github.com/w3c/webappsec-feature-policy/issues/282#issuecomment-486267212. This would be a sandboxing policy of sorts, but the exact framework for that isn't designed yet as per w3c/webappsec-permissions-policy#300.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this feature fit the permission-style model? I don't think it requires opt-in from an embedded page, as that page could have been loaded in isolation anyway, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It only fits if it's not allowed by default, which doesn't make sense? There's also no user involved which makes me rather wary as well.

@annevk
Copy link
Member

annevk commented May 10, 2019

Explicitly allocating agent clusters is tracked by #4361. All help welcome!

@clelland
Copy link
Contributor

Re: declaring feature policy within HTML markup -- the one strong call we've had for this is for Client Hints. See @yoavweiss's and @eeeps's comments here. I'm not sure how late we can possibly defer the agent cluster allocation, by the time we're pre-scanning the document, can we still make that decision?

@annevk
Copy link
Member

annevk commented May 13, 2019

In theory and with sufficient refactoring of the HTML Standard, but I really don't think we should be designing for servers that cannot set headers anymore. We end up with a large number of edge cases that go undertested and underdefined and make it really hard to figure out what's going on.

@domenic domenic mentioned this pull request Jun 1, 2019
3 tasks
@yoavweiss
Copy link
Contributor

yoavweiss commented Jun 5, 2019

Apologies for not commenting here sooner.
I don't think this is just limited to Client Hints. Talking to e.g. analytics vendors about trying out feature policies for their customers in reporting-only mode, this is an industry-wide problem.

For deployment purposes, ideally we'd be able to turn on feature policies from script (e.g. by dynamically adding Feature Policy meta tags). That would mean that users that add <script src=fooanalytics.js> can magically also get reports regarding the feature policies that they violate, and those reports can evolve over time.

If the above is not feasible, having an HTML option would enable motivated front end devs to change their markup and add the required policies. Foo Analytics would also be able to add that piece to their published markup snippet, so that future devs will have that by default (even if that would tend to freeze in time the kind of FPs that they are tracking).

Finally, if only headers are supported, that means that only highly motivated developers with access to their backend code will be able to turn on FPs. So while making it easiest to specify and potentially implement (I haven't looked), it will be the worst option for the wide-scale deployment that we want Feature Policies to have.

@eeeps
Copy link
Contributor

eeeps commented Jun 5, 2019

developers with access to their backend code

As Feature-Policies generally control front-end behavior, this seems especially worrying.

My biggest take away from the fact that 98% of devs, when given a choice, enabled Client Hints using HTML rather than HTTP, was not that Client Hints need HTML delivery, but rather that front-end-developers do.

aarongable pushed a commit to chromium/chromium that referenced this pull request Jul 26, 2019
This fundamentally should not change anything currently. All windows that
have access to each other share the same agent so this is just an
additional constant value to the string.

When the agent is different (in a yet to be implemented change) this
will fail the token checks even though the sites are the same origin.

This matches what is proposed in the whatwg/html#4606

BUG=961448

Change-Id: Ic0be1e1ac5f231589924eab94d409d8a37c5f081
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1720712
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#681362}
@dtapuska
Copy link
Contributor Author

I've rebased this on top of the refactoring I've done for feature policy, and agent cluster definition (squashed commit of #4617) so hopefully this makes a whole lot more sense.

This does not address the request to specify this separation via meta tags which I believe is fundamentally impossible because the DOM is already created by then and this needs to be done before the Javascript global is created. I intend to proceed with an Intent to Implement and TAG review based on this current direction.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 30, 2019
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 30, 2019
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 30, 2019
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 1, 2019
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 1, 2019
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 2, 2019
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 12, 2019
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 15, 2019
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 20, 2019
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 21, 2019
aarongable pushed a commit to chromium/chromium that referenced this pull request Aug 21, 2019
…olicy

Intent to Implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/Cibo-GNPs7Y/RznlX7WKDAAJ
Spec: whatwg/html#4606

BUG=961448

Change-Id: I3c2ff129a71a8ccb5a0015661770adc7ff22d14b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1726055
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688994}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 21, 2019
…olicy

Intent to Implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/Cibo-GNPs7Y/RznlX7WKDAAJ
Spec: whatwg/html#4606

BUG=961448

Change-Id: I3c2ff129a71a8ccb5a0015661770adc7ff22d14b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1726055
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688994}
foolip pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 28, 2019
…olicy

Intent to Implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/Cibo-GNPs7Y/RznlX7WKDAAJ
Spec: whatwg/html#4606

BUG=961448

Change-Id: I3c2ff129a71a8ccb5a0015661770adc7ff22d14b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1726055
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688994}
Hexcles pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 28, 2019
…olicy

Intent to Implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/Cibo-GNPs7Y/RznlX7WKDAAJ
Spec: whatwg/html#4606

BUG=961448

Change-Id: I3c2ff129a71a8ccb5a0015661770adc7ff22d14b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1726055
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688994}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 3, 2019
…via document-access feature policy, a=testonly

Automatic update from web-platform-tests
Add ability to block same-origin access via document-access feature policy

Intent to Implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/Cibo-GNPs7Y/RznlX7WKDAAJ
Spec: whatwg/html#4606

BUG=961448

Change-Id: I3c2ff129a71a8ccb5a0015661770adc7ff22d14b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1726055
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688994}

--

wpt-commits: 0221225c4e1863839c1505b7efe1230ff1d6ff34
wpt-pr: 18174
dbaron pushed a commit to dbaron/gecko that referenced this pull request Sep 4, 2019
…via document-access feature policy, a=testonly

Automatic update from web-platform-tests
Add ability to block same-origin access via document-access feature policy

Intent to Implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/Cibo-GNPs7Y/RznlX7WKDAAJ
Spec: whatwg/html#4606

BUG=961448

Change-Id: I3c2ff129a71a8ccb5a0015661770adc7ff22d14b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1726055
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688994}

--

wpt-commits: 0221225c4e1863839c1505b7efe1230ff1d6ff34
wpt-pr: 18174
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 4, 2019
…via document-access feature policy, a=testonly

Automatic update from web-platform-tests
Add ability to block same-origin access via document-access feature policy

Intent to Implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/Cibo-GNPs7Y/RznlX7WKDAAJ
Spec: whatwg/html#4606

BUG=961448

Change-Id: I3c2ff129a71a8ccb5a0015661770adc7ff22d14b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1726055
Commit-Queue: Dave Tapuska <dtapuskachromium.org>
Reviewed-by: Ken Buchanan <kenrbchromium.org>
Reviewed-by: Yuki Shiino <yukishiinochromium.org>
Reviewed-by: Ian Clelland <iclellandchromium.org>
Reviewed-by: Kentaro Hara <harakenchromium.org>
Cr-Commit-Position: refs/heads/master{#688994}

--

wpt-commits: 0221225c4e1863839c1505b7efe1230ff1d6ff34
wpt-pr: 18174

UltraBlame original commit: 1e3822b202e3dd7622fc90c4638980d9dc037a46
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 4, 2019
…via document-access feature policy, a=testonly

Automatic update from web-platform-tests
Add ability to block same-origin access via document-access feature policy

Intent to Implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/Cibo-GNPs7Y/RznlX7WKDAAJ
Spec: whatwg/html#4606

BUG=961448

Change-Id: I3c2ff129a71a8ccb5a0015661770adc7ff22d14b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1726055
Commit-Queue: Dave Tapuska <dtapuskachromium.org>
Reviewed-by: Ken Buchanan <kenrbchromium.org>
Reviewed-by: Yuki Shiino <yukishiinochromium.org>
Reviewed-by: Ian Clelland <iclellandchromium.org>
Reviewed-by: Kentaro Hara <harakenchromium.org>
Cr-Commit-Position: refs/heads/master{#688994}

--

wpt-commits: 0221225c4e1863839c1505b7efe1230ff1d6ff34
wpt-pr: 18174

UltraBlame original commit: 1e3822b202e3dd7622fc90c4638980d9dc037a46
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 4, 2019
…via document-access feature policy, a=testonly

Automatic update from web-platform-tests
Add ability to block same-origin access via document-access feature policy

Intent to Implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/Cibo-GNPs7Y/RznlX7WKDAAJ
Spec: whatwg/html#4606

BUG=961448

Change-Id: I3c2ff129a71a8ccb5a0015661770adc7ff22d14b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1726055
Commit-Queue: Dave Tapuska <dtapuskachromium.org>
Reviewed-by: Ken Buchanan <kenrbchromium.org>
Reviewed-by: Yuki Shiino <yukishiinochromium.org>
Reviewed-by: Ian Clelland <iclellandchromium.org>
Reviewed-by: Kentaro Hara <harakenchromium.org>
Cr-Commit-Position: refs/heads/master{#688994}

--

wpt-commits: 0221225c4e1863839c1505b7efe1230ff1d6ff34
wpt-pr: 18174

UltraBlame original commit: 1e3822b202e3dd7622fc90c4638980d9dc037a46
@dtapuska dtapuska changed the title Add a document-access feature policy to prevent access from one Add disallowdocumentaccess iframe attribute to force a creation of a new *nested* browsing context group Feb 11, 2020
@dtapuska
Copy link
Contributor Author

We've pivoted a bit on this feature and are looking for it to be an iframe attribute instead. This causes a nested browsing context group to be created at the iframe itself and then causes all further nested browsing context groups to be in different agents and then they can't possibly share data only via 'postMessage'

@annevk
Copy link
Member

annevk commented Feb 11, 2020

I don't really understand how this works. A browsing context group exclusively holds top-level browsing contexts. Browsing contexts also cannot be same origin-domain or some such.

@dtapuska
Copy link
Contributor Author

I don't really understand how this works. A browsing context group exclusively holds top-level browsing contexts. Browsing contexts also cannot be same origin-domain or some such.

What I am thinking is that the browsing context group doesn't hold a set of top-level browsing contexts but aset of browsing contexts. If you set the iframe attribute it will create a new nested browsing context group. This maps kind of what we have in Chrome with SiteInstance.

The alternate is to change the AgentClusterMap to be on the browser context itself instead of the group. Then when we create a nested browser context we can either inherit the agent cluster map or not.

@annevk
Copy link
Member

annevk commented Feb 11, 2020

From its name a SiteInstance pretty much has to be an agent cluster, I think. A browsing context holds a sequence of documents that can have different origins from each other.

@dtapuska
Copy link
Contributor Author

From its name a SiteInstance pretty much has to be an agent cluster, I think. A browsing context holds a sequence of documents that can have different origins from each other.

Alright placed the agent cluster map on the browsingContext. Hopefully this makes sense then.

@annevk
Copy link
Member

annevk commented Feb 12, 2020

I don't really see how I'm afraid. If A1 popups A2, how would they share an agent cluster across two browsing contexts with this setup?

@dtapuska dtapuska changed the title Add disallowdocumentaccess iframe attribute to force a creation of a new *nested* browsing context group Add disallowdocumentaccess iframe attribute to force a creation of a new agent cluster map Feb 13, 2020
@dtapuska
Copy link
Contributor Author

I don't really see how I'm afraid. If A1 popups A2, how would they share an agent cluster across two browsing contexts with this setup?

As discussed on IRC the browsing context inherits the same reference of the agent cluster map (I'm not sure exactly how to detail this in the spec).

After explanation these were your comments:
"I guess that makes sense; I'm still not a 100% on why we want to be doing this and am somewhat concerned that you can frame the top-level again and it being sandboxed from itself
Maybe we should couple this to #5273 somehow?
So I understand the effects of your proposal, it's just not clear to me it's worth doing on its own and if it's okay to do without opt-in of the affected sites"

document to another.

Add steps that cause a newly created browsing context group at the iframe
boundary.

Fixes whatwg#4435
@annevk
Copy link
Member

annevk commented Jul 6, 2023

@dtapuska I suspect this can be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Prevent cross document access via policy
6 participants