Inherit and apply Document Policy for local-scheme dedicated workers [chromium/src : main]
Monica Chintala (Gerrit)
Monica Chintala voted and added 1 comment![]( "Open in Gerrit")
Votes added by Monica Chintala
| Commit-Queue | +1 |
1 comment
Would you please review the changes before opening for upstream?
Related details
- Liang Zhao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 3 comments![]( "Open in Gerrit")
response_url.ProtocolIsAbout() || response_url.ProtocolIsData() ||
response_url.ProtocolIs("blob") ||
response_url.ProtocolIs("filesystem")This is the same set of checks, should we calculate it once into a local variable and check the variable in both places?
Don't know whether the DP should always behave the same as csp though.
response_url.ProtocolIsAbout() || response_url.ProtocolIsData() ||WPT test only covers blob url, should we add some test coverage for other schemes, in unit test or end to end test? Not sure how we test it fo csp.
response_url.ProtocolIsAbout() || response_url.ProtocolIsData() ||In https://github.com/WICG/document-policy/pull/46, we stated `about:blank` but we only checks scheme is `about:`, and we didn't mention `filesystem:` in the spec. Is there any alignment needed?
Related details
- Monica Chintala
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Monica Chintala (Gerrit)
Monica Chintala added 3 comments![]( "Open in Gerrit")
Message
Addressed review comments: deduped the local-scheme check and added a local-scheme DP inheritance unit test. Replies inline.
3 comments
// existing policy-container inheritance behavior for such URLs.Done — extracted a single `is_local_scheme` used by both the CSP and Document-Policy branches. These inherit together by design: HTML's "initialize a worker global scope's policy container" clones the creator's entire policy container for local (non-blob) schemes and creates it from the fetch response otherwise; its members (CSP list, embedder policy, referrer policy, integrity policy) share that one local-scheme condition. The real partition is whether the response carries headers — a headerless local response can't deliver a `Document-Policy` header any more than a CSP one. This also matches existing code: `WorkerClassicScriptLoader::ProcessContentSecurityPolicy` and `ProcessDocumentPolicy` already inherit for the same headerless schemes. If CSP and DP ever need to diverge, re-splitting is a one-line change.
// the DP parsed from the response headers.Good catch on the wording. Two parts: (a) about:blank vs about: — there's no real code/spec mismatch. `ProtocolIsAbout()` matches all `about:` URLs, and Fetch's `local scheme` is defined by scheme being "about"; the spec note just uses `about:blank` as the canonical example. (b) filesystem: is a Chromium-specific scheme (`url::kFileSystemScheme`, still registered), headerless like blob:/file:, so it must inherit. `WorkerClassicScriptLoader::ProcessDocumentPolicy` (prior DP CL) already inherits DP for blob/file/filesystem, adjacent to the CSP block. Dropping it would make a filesystem: worker inherit CSP but get an empty DP. The spec intentionally describes only the standardized subset (about/blob/data); I've added a code comment noting filesystem: is a Chromium extension.
// the DP parsed from the response headers.Added a `DocumentPolicyInheritedForLocalSchemeWorker` unit test: it sets a non-default `js-profiling` policy on the creator, starts a blob: (local scheme) worker, and asserts the worker's DocumentPolicy reflects the inherited value (vs the default). This directly covers the local-scheme inheritance branch. Combined with the existing `DocumentPolicyInDedicatedWorker` test (network URL → response policy) and the blob:/data: WPT tests plus the network WPT from the prior CL, both branches of `is_local_scheme` are covered end-to-end. about:/filesystem: run the identical `is_local_scheme` path.
Related details
- Liang Zhao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 2 comments![]( "Open in Gerrit")
params->creator_document_policy.policy =Should we also set creator_document_policy.report_only_header? Should we add some test coverage for report only headers?
KURL("blob:http://fake.url/de305d54-75b4-431b-adb2-eb6b9e546014"),My original comment was more about test coverage for other schemes like data:. Is there already test coverage for those schemes for CSP inheritance?
Related details
- Monica Chintala
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Monica Chintala (Gerrit)
Monica Chintala added 2 comments![]( "Open in Gerrit")
params->creator_document_policy.policy =Should we also set creator_document_policy.report_only_header? Should we add some test coverage for report only headers?
Document-Policy violation reporting isn't wired up for workers. ReportDocumentPolicyViolation is a virtual with a no-op base on ExecutionContext and is only overridden on LocalDOMWindow — worker global scopes fall through to the base no-op. So a report-only DP installed on a worker (whether inherited here, or read from the Document-Policy-Report-Only header on the network path) never actually emits a violation report; it's inert.
This isn't DP-specific — Permissions Policy has the identical pattern (ReportPermissionsPolicyViolation is also LocalDOMWindow-only). Only CSP reports in workers, because it routes through ExecutionContextCSPDelegate, which branches on window vs. worker and queues to ReportingContext.
I'm going to leave report_only_header unset for workers for now.
KURL("blob:http://fake.url/de305d54-75b4-431b-adb2-eb6b9e546014"),My original comment was more about test coverage for other schemes like data:. Is there already test coverage for those schemes for CSP inheritance?
There is already [dedicated-worker-data.https.html](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/js-self-profiling/with-document-policy/dedicated-worker-data.https.html;l=1?q=dedicated-worker-data.https.html&sq=&ss=chromium%2Fchromium%2Fsrc) test for data: which was added in the previous CL.
For CSP parity: the closest CSP precedent is content-security-policy/connect-src/worker-from-guid.sub.html, which verifies a blob: worker inherits its creator's CSP — but only for blob: (enforce-only). Aklso CSP has no per-scheme worker-inheritance unit tests
Related details
- Liang Zhao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao voted and added 2 comments![]( "Open in Gerrit")
Votes added by Liang Zhao
| Code-Review | +1 |
2 comments
params->creator_document_policy.policy =Monica ChintalaShould we also set creator_document_policy.report_only_header? Should we add some test coverage for report only headers?
Document-Policy violation reporting isn't wired up for workers. ReportDocumentPolicyViolation is a virtual with a no-op base on ExecutionContext and is only overridden on LocalDOMWindow — worker global scopes fall through to the base no-op. So a report-only DP installed on a worker (whether inherited here, or read from the Document-Policy-Report-Only header on the network path) never actually emits a violation report; it's inert.
This isn't DP-specific — Permissions Policy has the identical pattern (ReportPermissionsPolicyViolation is also LocalDOMWindow-only). Only CSP reports in workers, because it routes through ExecutionContextCSPDelegate, which branches on window vs. worker and queues to ReportingContext.
I'm going to leave report_only_header unset for workers for now.
Is there anything in the spec that states report only policy is not applicable to workers? If not, then it is possible that we implements ReportDocumentPolicyViolation later for workers.
Anyway, let's add some comments about why we skipped report_only_header.
Related details
- Monica Chintala
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Monica Chintala (Gerrit)
Monica Chintala added 2 comments![]( "Open in Gerrit")
params->creator_document_policy.policy =Monica ChintalaShould we also set creator_document_policy.report_only_header? Should we add some test coverage for report only headers?
Liang ZhaoDocument-Policy violation reporting isn't wired up for workers. ReportDocumentPolicyViolation is a virtual with a no-op base on ExecutionContext and is only overridden on LocalDOMWindow — worker global scopes fall through to the base no-op. So a report-only DP installed on a worker (whether inherited here, or read from the Document-Policy-Report-Only header on the network path) never actually emits a violation report; it's inert.
This isn't DP-specific — Permissions Policy has the identical pattern (ReportPermissionsPolicyViolation is also LocalDOMWindow-only). Only CSP reports in workers, because it routes through ExecutionContextCSPDelegate, which branches on window vs. worker and queues to ReportingContext.
I'm going to leave report_only_header unset for workers for now.
Is there anything in the spec that states report only policy is not applicable to workers? If not, then it is possible that we implements ReportDocumentPolicyViolation later for workers.
Anyway, let's add some comments about why we skipped report_only_header.
Good question — no, the spec doesn't state report-only is inapplicable to workers; it's silent on it. So you're right that we could inherit it once ReportDocumentPolicyViolation() is implemented for worker global scopes. Added the comment.
Related details
- Ian Clelland
- Liang Zhao
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |