Update "Is feature enabled in document for origin?" behavior #499
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aarongable
pushed a commit
to chromium/chromium
that referenced
this pull request
Dec 20, 2022
…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}
aarongable
pushed a commit
to chromium/chromium
that referenced
this pull request
Dec 29, 2022
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}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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