Enable Document-Policy in Dedicated Workers [chromium/src : main]
Monica Chintala (Gerrit)
Monica Chintala added 1 comment![]( "Open in Gerrit")
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")
std::move(referrer_policy),Why do we remove the `std::move`? And if we follow the existing pattern, we might actually want to also do std::move for the new document policy. Not that as this is passing parameter to a callback. Forwarding ownership is more reliable than passing reference to an object, which might be released when the callback is invoked.
class DedicatedWorkerObjectProxyForTest finalNot necessarily in this file, but can we add some test for the new code?
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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by Monica Chintala
| Commit-Queue | +1 |
2 comments
std::move(referrer_policy),Why do we remove the `std::move`? And if we follow the existing pattern, we might actually want to also do std::move for the new document policy. Not that as this is passing parameter to a callback. Forwarding ownership is more reliable than passing reference to an object, which might be released when the callback is invoked.
That wasn't intentional. Also moved the document policy
Not necessarily in this file, but can we add some test for the new code?
- 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")
const DocumentPolicy::ParsedDocumentPolicy& response_document_policy,If we are using std::move, we probably should not use reference as parameter. The caller would not expect the parameter to be moved if the paramter is const reference.
document_policy(document_policy),The referrer_policy is copied, but others are using std::move, which one is better?
const DocumentPolicy::ParsedDocumentPolicy& GetDocumentPolicy() const {
return document_policy_;
}If we return reference, and the other code does a std::move on it, would it make the policy not valid anymore afterwards?
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")
const DocumentPolicy::ParsedDocumentPolicy& response_document_policy,If we are using std::move, we probably should not use reference as parameter. The caller would not expect the parameter to be moved if the paramter is const reference.
Done
The referrer_policy is copied, but others are using std::move, which one is better?
Forwarding is better than passing a reference for DP. Removed the reference and moved where ever is necessary.
const DocumentPolicy::ParsedDocumentPolicy& GetDocumentPolicy() const {
return document_policy_;
}If we return reference, and the other code does a std::move on it, would it make the policy not valid anymore afterwards?
- 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")
security_init.ApplyDocumentPolicy(response_document_policy, String());This seems to be for report_only_document_policy_header. Are we ignoring report only policies, or there is a TODO to add that support?
DocumentPolicy::ParsedDocumentPolicy GetDocumentPolicy() const {
return document_policy_;
}Just a note, with the current design, we are returning a copy of the policy every time this function is called, whether the caller needs a new copy or not. The previous version returns a reference, the caller could use it as a reference or assign the returned reference to a value if it want to use a copy of the value.
It seems that the current caller needs a copy, so not much a difference.
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")
security_init.ApplyDocumentPolicy(response_document_policy, String());This seems to be for report_only_document_policy_header. Are we ignoring report only policies, or there is a TODO to add that support?
Missed it, added the support now.
Added a struct DocumentPolicyBundle that has DocumentPolicy and report only header value. Can you please take a look?
DocumentPolicy::ParsedDocumentPolicy GetDocumentPolicy() const {
return document_policy_;
}Just a note, with the current design, we are returning a copy of the policy every time this function is called, whether the caller needs a new copy or not. The previous version returns a reference, the caller could use it as a reference or assign the returned reference to a value if it want to use a copy of the value.
It seems that the current caller needs a copy, so not much a difference.
- 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 added 2 comments![]( "Open in Gerrit")
I see that you have spec change prepared. We probably should also add WPT tests for the feature. Are we going to add those right after this CL?
security_init.ApplyDocumentPolicy(response_document_policy, String());Monica ChintalaThis seems to be for report_only_document_policy_header. Are we ignoring report only policies, or there is a TODO to add that support?
Missed it, added the support now.
Added a struct DocumentPolicyBundle that has DocumentPolicy and report only header value. Can you please take a look?
The change seems OK to me. The owners might have different opinion.
It seems odd that we don't have something for both DP and report only DP already.
Related details
- Monica Chintala
Monica Chintala (Gerrit)
Monica Chintala added 2 comments![]( "Open in Gerrit")
I see that you have spec change prepared. We probably should also add WPT tests for the feature. Are we going to add those right after this CL?
Yes, I've added in the upcoming CL https://chromium-review.googlesource.com/c/chromium/src/+/7270269/1
security_init.ApplyDocumentPolicy(response_document_policy, String());Monica ChintalaThis seems to be for report_only_document_policy_header. Are we ignoring report only policies, or there is a TODO to add that support?
Liang ZhaoMissed it, added the support now.
Added a struct DocumentPolicyBundle that has DocumentPolicy and report only header value. Can you please take a look?
The change seems OK to me. The owners might have different opinion.
It seems odd that we don't have something for both DP and report only DP already.
Thanks, I'll open the CL for further review from upstream folks.
Related details
- Liang Zhao
Monica Chintala (Gerrit)
Monica Chintala added 1 comment![]( "Open in Gerrit")
- Hiroshige Hayashizaki
- Ian Clelland
- 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. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 4 comments![]( "Open in Gerrit")
Basically I'd like to wait for the spec PR to settle, i.e. https://github.com/whatwg/html/pull/12026, particularly annevk's input.
Also, could you create a chromestatus entry for visibility? (You already have sufficient content, e.g. explainer, spec PR, etc.)
See https://www.chromium.org/blink/launching-features/.
Maybe we need some Intent-to-*, but I defer to relevant feature owners (Ian for Document Policy, Yoshisato for Workers).
script_data->GetContentSecurityPolicyResponseHeaders()),`// TODO(crbug.com/450845903): Plumb Document Policy in Service Workers.`
DocumentPolicy::DocumentPolicyBundle{},We need a TODO comment for support module workers, and mention the lack of this support in the commit message.
Are you planning implementing this in subsequent CLs before shipping this feature?
script_data->GetContentSecurityPolicyResponseHeaders()),`// TODO(crbug.com/450845903): Plumb Document Policy in Service Workers.`
Related details
- Ian Clelland
- Liang Zhao
- Monica Chintala
Monica Chintala (Gerrit)
Monica Chintala voted and added 4 comments![]( "Open in Gerrit")
Votes added by Monica Chintala
| Commit-Queue | +1 |
Basically I'd like to wait for the spec PR to settle, i.e. https://github.com/whatwg/html/pull/12026, particularly annevk's input.
Also, could you create a chromestatus entry for visibility? (You already have sufficient content, e.g. explainer, spec PR, etc.)
See https://www.chromium.org/blink/launching-features/.
Maybe we need some Intent-to-*, but I defer to relevant feature owners (Ian for Document Policy, Yoshisato for Workers).
Here is the chromestatus entry - https://chromestatus.com/feature/6242687411945472 for the DP in dedicated worker feature
iclelland@ yyanagisawa@ Do we need intent to prototype as we already started impl work on this?
script_data->GetContentSecurityPolicyResponseHeaders()),`// TODO(crbug.com/450845903): Plumb Document Policy in Service Workers.`
Done
DocumentPolicy::DocumentPolicyBundle{},We need a TODO comment for support module workers, and mention the lack of this support in the commit message.
Are you planning implementing this in subsequent CLs before shipping this feature?
Added a TODO (using the same crbug) to track Document Policy implementation across all worker types, including module workers.
We plan to ship this per worker type, since some customers need the Dedicated Worker support immediately. So created the chromestatus entry accordingly. Let me know if this process works.
If so, do I need to create separate crbugs for each of the worker impl instead of using one crbug.com/450845903
script_data->GetContentSecurityPolicyResponseHeaders()),`// TODO(crbug.com/450845903): Plumb Document Policy in Service Workers.`
- Hiroshige Hayashizaki
- Ian Clelland
- Liang Zhao
- Yoshisato Yanagisawa
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. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 1 comment![]( "Open in Gerrit")
Monica ChintalaBasically I'd like to wait for the spec PR to settle, i.e. https://github.com/whatwg/html/pull/12026, particularly annevk's input.
Also, could you create a chromestatus entry for visibility? (You already have sufficient content, e.g. explainer, spec PR, etc.)
See https://www.chromium.org/blink/launching-features/.
Maybe we need some Intent-to-*, but I defer to relevant feature owners (Ian for Document Policy, Yoshisato for Workers).
Here is the chromestatus entry - https://chromestatus.com/feature/6242687411945472 for the DP in dedicated worker feature
iclelland@ yyanagisawa@ Do we need intent to prototype as we already started impl work on this?
Yes, I suggest to send Intent to Prototype to gather attentions from web developers and Chromium developers if possible. I would like to know others' thought on this Document-Policy inheritance.
Related details
- Hiroshige Hayashizaki
- Ian Clelland
- Liang Zhao
- Monica Chintala
Monica Chintala (Gerrit)
Monica Chintala added 1 comment![]( "Open in Gerrit")
Monica ChintalaBasically I'd like to wait for the spec PR to settle, i.e. https://github.com/whatwg/html/pull/12026, particularly annevk's input.
Also, could you create a chromestatus entry for visibility? (You already have sufficient content, e.g. explainer, spec PR, etc.)
See https://www.chromium.org/blink/launching-features/.
Maybe we need some Intent-to-*, but I defer to relevant feature owners (Ian for Document Policy, Yoshisato for Workers).
Yoshisato YanagisawaHere is the chromestatus entry - https://chromestatus.com/feature/6242687411945472 for the DP in dedicated worker feature
iclelland@ yyanagisawa@ Do we need intent to prototype as we already started impl work on this?
Yes, I suggest to send Intent to Prototype to gather attentions from web developers and Chromium developers if possible. I would like to know others' thought on this Document-Policy inheritance.
Created I2P - https://groups.google.com/a/chromium.org/g/blink-dev/c/l-3DhX_uCco/m/Q3ZGgqg-AQAJ
hiroshige@ iclelland@ yyanagisawa@ I’m still waiting to hear folk's thoughts on linking the DP WICG resources from HTML. So far, there doesn’t seem to be strong opposition to inheriting Document Policy via the Policy Container.
If the CL here looks okay, what do you think about getting this merged?
My current thinking is:
- If the HTML changes are acceptable without requiring significant changes to the WG, we can proceed as-is.
- If linking to WICG content from HTML isn’t acceptable unless DP moves to a WG, then we can instead look at adding worker inheritance semantics directly to the Document Policy spec.
Related details
- Hiroshige Hayashizaki
- Ian Clelland
- Liang Zhao
- Yoshisato Yanagisawa
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. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 2 comments![]( "Open in Gerrit")
Monica ChintalaBasically I'd like to wait for the spec PR to settle, i.e. https://github.com/whatwg/html/pull/12026, particularly annevk's input.
Also, could you create a chromestatus entry for visibility? (You already have sufficient content, e.g. explainer, spec PR, etc.)
See https://www.chromium.org/blink/launching-features/.
Maybe we need some Intent-to-*, but I defer to relevant feature owners (Ian for Document Policy, Yoshisato for Workers).
Yoshisato YanagisawaHere is the chromestatus entry - https://chromestatus.com/feature/6242687411945472 for the DP in dedicated worker feature
iclelland@ yyanagisawa@ Do we need intent to prototype as we already started impl work on this?
Monica ChintalaYes, I suggest to send Intent to Prototype to gather attentions from web developers and Chromium developers if possible. I would like to know others' thought on this Document-Policy inheritance.
Created I2P - https://groups.google.com/a/chromium.org/g/blink-dev/c/l-3DhX_uCco/m/Q3ZGgqg-AQAJ
hiroshige@ iclelland@ yyanagisawa@ I’m still waiting to hear folk's thoughts on linking the DP WICG resources from HTML. So far, there doesn’t seem to be strong opposition to inheriting Document Policy via the Policy Container.
If the CL here looks okay, what do you think about getting this merged?My current thinking is:
- If the HTML changes are acceptable without requiring significant changes to the WG, we can proceed as-is.
- If linking to WICG content from HTML isn’t acceptable unless DP moves to a WG, then we can instead look at adding worker inheritance semantics directly to the Document Policy spec.
Thank you for the I2P.
I expect we go through the procedure (https://www.chromium.org/blink/launching-features/#new-feature-process). Also, please ensure the feature kill switch works.
Looks good to me other than them. I would like to listen to other reviewers thought.
!response.CurrentRequestUrl().ProtocolIs("filesystem")) {Don't we need the flag check either of setting or returning?
Related details
- Hiroshige Hayashizaki
- Ian Clelland
- Liang Zhao
- Monica Chintala
Monica Chintala (Gerrit)
Monica Chintala added 2 comments![]( "Open in Gerrit")
Monica ChintalaBasically I'd like to wait for the spec PR to settle, i.e. https://github.com/whatwg/html/pull/12026, particularly annevk's input.
Also, could you create a chromestatus entry for visibility? (You already have sufficient content, e.g. explainer, spec PR, etc.)
See https://www.chromium.org/blink/launching-features/.
Maybe we need some Intent-to-*, but I defer to relevant feature owners (Ian for Document Policy, Yoshisato for Workers).
Yoshisato YanagisawaHere is the chromestatus entry - https://chromestatus.com/feature/6242687411945472 for the DP in dedicated worker feature
iclelland@ yyanagisawa@ Do we need intent to prototype as we already started impl work on this?
Monica ChintalaYes, I suggest to send Intent to Prototype to gather attentions from web developers and Chromium developers if possible. I would like to know others' thought on this Document-Policy inheritance.
Yoshisato YanagisawaCreated I2P - https://groups.google.com/a/chromium.org/g/blink-dev/c/l-3DhX_uCco/m/Q3ZGgqg-AQAJ
hiroshige@ iclelland@ yyanagisawa@ I’m still waiting to hear folk's thoughts on linking the DP WICG resources from HTML. So far, there doesn’t seem to be strong opposition to inheriting Document Policy via the Policy Container.
If the CL here looks okay, what do you think about getting this merged?My current thinking is:
- If the HTML changes are acceptable without requiring significant changes to the WG, we can proceed as-is.
- If linking to WICG content from HTML isn’t acceptable unless DP moves to a WG, then we can instead look at adding worker inheritance semantics directly to the Document Policy spec.
Thank you for the I2P.
I expect we go through the procedure (https://www.chromium.org/blink/launching-features/#new-feature-process). Also, please ensure the feature kill switch works.
Looks good to me other than them. I would like to listen to other reviewers thought.
For DP testing when feature is on, I've added a wpt test in the following CL https://chromium-review.googlesource.com/c/chromium/src/+/7546208/10/third_party/blink/web_tests/external/wpt/js-self-profiling/with-document-policy/dedicated-worker.https.html that checks DP exists and working! Do you think it will be sufficient?
From the https://www.chromium.org/blink/launching-features/#new-feature-process, most of the things are done/in-progess
1. Explainer
2. I2P and Chromestatus
3. Spec changes PR is open and under review
3. Feature is behind a flag
Are there any other things that I am missing other than spec PR which hasn't approved/merged. But yes, I'll wait to hear from other folks.
!response.CurrentRequestUrl().ProtocolIs("filesystem")) {Don't we need the flag check either of setting or returning?
Guarded with feature flag when we set the DP in dedicated worker global scope and also here in script parsing
Related details
- Hiroshige Hayashizaki
- Ian Clelland
- Liang Zhao
- Yoshisato Yanagisawa
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. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 1 comment![]( "Open in Gerrit")
I took a look at the linked WPT, and felt it does not cover all cases for Dedicated Workers described in the explainer and the spec PR. To ensure the implementation follows the 'Response Headers as Authoritative' model and the defined inheritance rules, I believe the following three cases should be verified:
1. **Network script with `Document-Policy header`**: The effective policy is derived from the response header. (Currently covered by crrev.com/c/7546208)
2. **Network script without `Document-Policy` header**: The effective policy should be the default (i.e. disabled), even if the creator document has the policy enabled. This is crucial to verify that no unintentional inheritance occurs for network workers.
3. **Local scheme script (`blob:`, `data:`)**: The policy should be inherited from the creator document. It is written as TODO of this CL, but it would be good to have a test case (even if it's currently failing/expecting default) to track its implementation.
Currently, the WPT seems to only focus on Case 1. You may need to implement Case 2 and Case 3 in addition.
For the feature process, I do not come up with anything else. If I have to add, maybe having the test to ensure that the kill switch actually works.
Related details
- Hiroshige Hayashizaki
- Ian Clelland
- Liang Zhao
- Monica Chintala
Monica Chintala (Gerrit)
Monica Chintala added 1 comment![]( "Open in Gerrit")
yyanagisawa@ Thanks for taking a look. I added tests for other cases in the next CL. Does this look good now?
In the current CL I updated DedicatedWorkerDocumentPolicyTest.DocumentPolicyInDedicatedWorker test to verify when DP works when feature enabled/disabled (also ensures kill switch works)
Related details
- Hiroshige Hayashizaki
- Ian Clelland
- Liang Zhao
- Yoshisato Yanagisawa
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. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yoshisato Yanagisawa
| Code-Review | +1 |
- Hiroshige Hayashizaki
- Ian Clelland
- Liang Zhao
- Monica Chintala
Monica Chintala (Gerrit)
Monica Chintala added 1 comment![]( "Open in Gerrit")
hiroshige@ iclelland@ The feature is behind a flag and will not be enabled by default until the spec merge. Can I get your votes on landing this prototype CL if it looks good?
Related details
- Hiroshige Hayashizaki
- Ian Clelland
- Liang Zhao
- Yoshisato Yanagisawa