Update "Is feature enabled in document for origin?" behavior #499
Conversation
…Origin() PR: w3c/webappsec-permissions-policy#499 Bug: 1401473 Change-Id: Ie08f5304d09241e09604f615e05790c14f6ebbcc Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4111962 Commit-Queue: Yao Xiao <[email protected]> Reviewed-by: Ari Chivukula <[email protected]> Cr-Commit-Position: refs/heads/main@{#1085663}
The permissions policy should follow the algorithm in https://www.w3.org/TR/permissions-policy/#algo-is-feature-enabled plus the proposed update in w3c/webappsec-permissions-policy#499. This CL: - Removes the renderer side check that checks the policy against request initiator context's origin. (I misunderstood the spec before). This also means that on permissions error, we won’t throw an exception in any case, but will just skip adding the topics header. - Calls IsFeatureEnabledForSubresourceRequest() to align with the proposed update. This has no behavior difference for now as the default feature state is `EnableForAll`, but this would fix the behavior had the default state been switched to `EnableForSelf`. The distinction was already tested in PermissionsPolicyTest.ProposedTestIsBrowsingTopicsFeatureEnabledForSubresourceRequest, thus no test for this change / we cannot easily override the default feature state outside blink. - Enforce resource_request.browsing_topics at mojom boundary at BrowsingTopicsURLLoaderService::CreateLoaderAndStart(). Bug: 1401483 Change-Id: Ibea86eb1d63997955d9ce9de8a81762c2c047318 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4111963 Reviewed-by: Josh Karlin <[email protected]> Reviewed-by: Daniel Cheng <[email protected]> Commit-Queue: Yao Xiao <[email protected]> Cr-Commit-Position: refs/heads/main@{#1087639}
|
Can you modify this to support x-site javascript calls in addition to requests? For instance, joinAdInterestGroup() allows you to set a x-site owner origin, so the interest group data will be written to the x-site's data. I believe this x-site owner should be checked in a similar way to your patch here for requests. That is, there should be some, It seems like you could use this as for resource requests as well? That is, instead of adding all of the opted-in types, just add the opt-in type for the feature being checked. |
|
@jkarlin : On the point of adding opt-in types, that was my initial thought (as seen in this earlier commit), but I shifted to align with Ari's suggestion in the code: "This feels a little dangerous as a boolean that's passed in might be better to pass in a set of opt_in features ..." @arichiv : |
|
I personally find the list to be confusing when we're only ever talking about checking a single feature at a time and prefer the boolean. Looking again, I think your existing change (or ideally preferably with the boolean), is likely sufficient for my case, but it seems like it would be good to specify when the boolean should be set to true. You already have that text for requests in your proposal. How would we write it for javascript methods that are x-origin in nature? |
|
Maybe 'could policy inherited from this frame allow access for origin'? That covers the concept of 'future iframes' while making clear it's not counting the future Allowlist or frame policy. |
Rationale: if a page can create an <iframe src=… allow="featureX"> and can delegate permissions to the iframe, then the request like fetch(url, {allowFeatureX: true}) should be allowed for the permission as well, otherwise, they can just create an iframe similarly to gain the permission.
For a feature that does not have an equivalent opt-in flag, this rationale isn’t 100% applicable, so I’ll leave the spec unchanged for now. But for feature that does have it (e.g. fetch(url, {browsingTopics: true}) for the Topics API), we should be able to skip the last few steps that check the default allowlist, and can return "Enabled" directly. This way, the behavior will be the same with the Define an inherited policy for feature in container at origin algorithm with a container policy that allows the feature.
Preview | Diff