Create and parse a "*" pseudofeature for permisions policy reporting [chromium/src : main]
Ian Clelland (Gerrit)
Attention is currently required from: Ari Chivukula.
Ian Clelland would like Ari Chivukula to review this change.
Create and parse a "*" pseudofeature for permisions policy reporting
Change-Id: Ib44a7b9ca438b2daf88d9087827bc56589307974
---
M chrome/browser/web_applications/test/web_app_test_utils.cc
M third_party/blink/common/permissions_policy/permissions_policy.cc
M third_party/blink/public/common/permissions_policy/permissions_policy.h
M third_party/blink/public/devtools_protocol/browser_protocol.pdl
M third_party/blink/public/mojom/permissions_policy/permissions_policy_feature.mojom
M third_party/blink/renderer/build/scripts/make_policy_helper.py
M third_party/blink/renderer/core/permissions_policy/dom_feature_policy.cc
M third_party/blink/renderer/core/permissions_policy/permissions_policy_features.json5
M third_party/blink/renderer/core/permissions_policy/permissions_policy_parser.cc
M third_party/blink/renderer/core/permissions_policy/permissions_policy_test.cc
A third_party/blink/web_tests/external/wpt/permissions-policy/reporting/report-only-and-enforce.https.sub.html
A third_party/blink/web_tests/external/wpt/permissions-policy/reporting/report-only-and-enforce.https.sub.html.sub.headers
A third_party/blink/web_tests/external/wpt/permissions-policy/reporting/report-only-single-endpoint.https.sub.html
A third_party/blink/web_tests/external/wpt/permissions-policy/reporting/report-only-single-endpoint.https.sub.html.sub.headers
A third_party/blink/web_tests/external/wpt/permissions-policy/reporting/report-to-default-endpoint.https.sub.html
A third_party/blink/web_tests/external/wpt/permissions-policy/reporting/report-to-default-endpoint.https.sub.html.sub.headers
A third_party/blink/web_tests/external/wpt/permissions-policy/reporting/report-to-specific-endpoint.https.sub.html
A third_party/blink/web_tests/external/wpt/permissions-policy/reporting/report-to-specific-endpoint.https.sub.html.sub.headers
M tools/metrics/histograms/enums.xml
19 files changed, 239 insertions(+), 15 deletions(-)
To view, visit change 4573511. To unsubscribe, or for help writing mail filters, visit settings.
Ian Clelland (Gerrit)
Attention is currently required from: Ari Chivukula.
To view, visit change 4573511. To unsubscribe, or for help writing mail filters, visit settings.
Blink W3C Test Autoroller (Gerrit)
Attention is currently required from: Ari Chivukula.
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/42343.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Ari Chivukula (Gerrit)
Attention is currently required from: Ian Clelland.
1 comment:
Patchset:
Okay so my feedback is more on the design/syntax level and not really about this code. If the design concerns are resolved then I can review the code more in depth, but at least my initial skimming did not raise any significant issues with it.
My feedback is that the pesudofeature * is a confusing approach to setting the default endpoint.
**First, I want to write out my understanding of how the syntax works so if I'm wrong my misconception can be corrected.**
If the http header looks like:
`
Permissions-Policy: *;report-to=default-endpoint
Reporting-Endpoints: default-endpoint="SOME_URL"
`
Then reporting for any policy violations will be directed toward SOME_URL.
If the http header looks like:
`
Permissions-Policy-Report-Only: *;report-to=default-endpoint
Reporting-Endpoints: default-endpoint="SOME_URL"
`
Then reporting for any prospective policy violations (were the policy to have been enforced) will be directed toward SOME_URL.**Second, I want to raise two concerns.**
*My first concern is that this syntax will confuse developers.*
Since in the general the following two headers are the same:
`
Permissions-Policy: camera
Permissions-Policy: camera=(self)
`
This would lead developers to assume that the following two are the same as well:
`
Permissions-Policy: *
Permissions-Policy: *=(self)
`
Implying some sort of blanket enable for all features on self.*My second concern is with the potential future need for syntax similar to this that really does impact the allowlists of all features.*
I don't think the following will ever be needed:
`
Permissions-Policy: *=*
Permissions-Policy: *=(self)
Permissions-Policy: *=(SOME_ORIGIN)
`
But, I think it's possible we would want to enable something like:
`
Permissions-Policy: *=()
`
For sites which wanted to disable all permissions in some context, maybe with the idea that they could combine the blanket disallow with some specific enables:
`
Permissions-Policy: *=(),camera=(self)
`**Third, I want to propose an alternative that resolves the two above concerns without committing to any future syntax extensions beyond what you need.**
Instead of:
`
Permissions-Policy: ...,*;report-to=default-endpoint
Reporting-Endpoints: default-endpoint="SOME_URL"
`
What about just:
`
Permissions-Policy: ...
Reporting-Endpoints: default="SOME_URL"
`
Where `default` is turned into a reserved endpoint name that cannot be listed explicitly in a `report-to` parameter in a Permissions Policy. If a developer tried something like:
`
Permissions-Policy: camera=();report-to=default
Reporting-Endpoints: default="SOME_URL"
`
The developer would get a warning that `default` is a reserved name and reports for all feature violations will go to it, so listing it under `camera` is redundant and/or deceptive.There probably needs to be a second reserved name so that:
`
Permissions-Policy-Report-Only: ...,*;report-to=default-endpoint
Reporting-Endpoints: default-endpoint="SOME_URL"
`
Could be replaced with:
`
Permissions-Policy-Report-Only: ...
Reporting-Endpoints: default-report-to="SOME_URL"
`
So that if a given page wanted reports for the active policy and report-only policy they could have different endpoints for them.Let me know what you think.
To view, visit change 4573511. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Patchset:
Okay so my feedback is more on the design/syntax level and not really about this code. […]
Noticed one error with my wall of text above, the second reserved reporting endpoint name should have been `default-report-only` instead of `default-report-to`.
To view, visit change 4573511. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
- Ian Clelland
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Clelland (Gerrit)
Ian Clelland abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |