[LNA] Make old LNA permission policy forward compatible [chromium/src : main]
Chris Thompson (Gerrit)
Chris Thompson added 1 comment![]( "Open in Gerrit")
hchao@ could you PTAL for initial review? Thanks!
Related details
- Hubert Chao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hubert Chao (Gerrit)
Hubert Chao added 4 comments![]( "Open in Gerrit")
// Permissions-Policy: local-network-access=(self)
// Permissions-Policy: loopback-network=()is it also true that
```
Permissions-Policy: local-network-access=()
Permissions-Policy: loopback-network=(self)
```
would be equivalent to
```
Permissions-Policy: local-network-access=()
Permissions-Policy: loopback-network=()
Permissions-Policy: local-network=()
```
?
Also, I thought you mentioned in the meeting that ordering here matters; does
```
Permissions-Policy: local-network-access=(self)
Permissions-Policy: loopback-network=()
```
still behave differently from
```
Permissions-Policy: loopback-network=()
Permissions-Policy: local-network-access=(self)
```
?
if (feature ==
network::mojom::PermissionsPolicyFeature::kLocalNetworkAccess) {
allow_lists_and_reporting_endpoints.allowlists_.emplace(
network::mojom::PermissionsPolicyFeature::kLocalNetwork,
Allowlist::FromDeclaration(parsed_declaration));
allow_lists_and_reporting_endpoints.allowlists_.emplace(
network::mojom::PermissionsPolicyFeature::kLoopbackNetwork,
Allowlist::FromDeclaration(parsed_declaration));
}should this be feature-flag guarded? (I think the answer is that its not necessary, but I'm not 100% sure, especially because they are flag-guarded in services/network/public/cpp/permissions_policy/permissions_policy_features.json5)
// allow="local-network-access; local-network 'none';"Is it worth calling out that
`allow="local-network-access none; local-network;"`
will still result in `local-network` being enabled?
(at least I think that's what would happen)
if (feature ==
network::mojom::PermissionsPolicyFeature::kLocalNetworkAccess) {
inherited_policies.Add(
network::mojom::PermissionsPolicyFeature::kLocalNetwork);
inherited_policies.Add(
network::mojom::PermissionsPolicyFeature::kLoopbackNetwork);
}same question as above (i suspect the answer is the same for both, whatever the answer is)
Related details
- Chris Thompson
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. |
Chris Thompson (Gerrit)
Chris Thompson added 4 comments![]( "Open in Gerrit")
// Permissions-Policy: local-network-access=(self)
// Permissions-Policy: loopback-network=()is it also true that
```
Permissions-Policy: local-network-access=()
Permissions-Policy: loopback-network=(self)
```
would be equivalent to
```
Permissions-Policy: local-network-access=()
Permissions-Policy: loopback-network=()
Permissions-Policy: local-network=()
```?
Also, I thought you mentioned in the meeting that ordering here matters; does
```
Permissions-Policy: local-network-access=(self)
Permissions-Policy: loopback-network=()
```still behave differently from
```
Permissions-Policy: loopback-network=()
Permissions-Policy: local-network-access=(self)
```
?
First one: Yes. The empty declaration gets copied to the key for "loopback-network" and "local-network" first. Then the explicit `loopback-network` declaration is processed and does not get added because the key is already set in the map.
Second one: Yes. The first would result in all three being set to self. The second would result in "loopback-network" being set to none, and "local-network-access" and "local-network" being set to self.
if (feature ==
network::mojom::PermissionsPolicyFeature::kLocalNetworkAccess) {
allow_lists_and_reporting_endpoints.allowlists_.emplace(
network::mojom::PermissionsPolicyFeature::kLocalNetwork,
Allowlist::FromDeclaration(parsed_declaration));
allow_lists_and_reporting_endpoints.allowlists_.emplace(
network::mojom::PermissionsPolicyFeature::kLoopbackNetwork,
Allowlist::FromDeclaration(parsed_declaration));
}should this be feature-flag guarded? (I think the answer is that its not necessary, but I'm not 100% sure, especially because they are flag-guarded in services/network/public/cpp/permissions_policy/permissions_policy_features.json5)
Yeah good idea to guard this (and the block in CreateFromParentPolicy()), since I can't convince myself that it isn't necessary either (these reference the Mojo enums which exist regardless of the depends_on state, but there might be some weird interaction if these are out of sync). Done.
Is it worth calling out that
`allow="local-network-access none; local-network;"`
will still result in `local-network` being enabled?
(at least I think that's what would happen)
Yeah that's a good idea, since it is a subtlety of iframe allowlists. (And I added another unit test for this case.)
if (feature ==
network::mojom::PermissionsPolicyFeature::kLocalNetworkAccess) {
inherited_policies.Add(
network::mojom::PermissionsPolicyFeature::kLocalNetwork);
inherited_policies.Add(
network::mojom::PermissionsPolicyFeature::kLoopbackNetwork);
}same question as above (i suspect the answer is the same for both, whatever the answer is)
- Hubert Chao
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. |
Hubert Chao (Gerrit)
Hubert Chao voted and added 1 comment![]( "Open in Gerrit")
Votes added by Hubert Chao
| Code-Review | +1 |
1 comment
// Permissions-Policy: local-network-access=(self)
// Permissions-Policy: loopback-network=()Chris Thompsonis it also true that
```
Permissions-Policy: local-network-access=()
Permissions-Policy: loopback-network=(self)
```
would be equivalent to
```
Permissions-Policy: local-network-access=()
Permissions-Policy: loopback-network=()
Permissions-Policy: local-network=()
```?
Also, I thought you mentioned in the meeting that ordering here matters; does
```
Permissions-Policy: local-network-access=(self)
Permissions-Policy: loopback-network=()
```still behave differently from
```
Permissions-Policy: loopback-network=()
Permissions-Policy: local-network-access=(self)
```
?
First one: Yes. The empty declaration gets copied to the key for "loopback-network" and "local-network" first. Then the explicit `loopback-network` declaration is processed and does not get added because the key is already set in the map.
Second one: Yes. The first would result in all three being set to self. The second would result in "loopback-network" being set to none, and "local-network-access" and "local-network" being set to self.
is it worth writing down these nuances somewhere (for both the header and the allow attribute)? I'd guess that no matter how much we say "don't use them together" someone is going to and then going to ask why something doesn't work, and writing these nuances might save us investigative time a few months down the line.
Related details
- Chris Thompson
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. |
Chris Thompson (Gerrit)
Chris Thompson added 2 comments![]( "Open in Gerrit")
// Permissions-Policy: local-network-access=(self)
// Permissions-Policy: loopback-network=()Chris Thompsonis it also true that
```
Permissions-Policy: local-network-access=()
Permissions-Policy: loopback-network=(self)
```
would be equivalent to
```
Permissions-Policy: local-network-access=()
Permissions-Policy: loopback-network=()
Permissions-Policy: local-network=()
```?
Also, I thought you mentioned in the meeting that ordering here matters; does
```
Permissions-Policy: local-network-access=(self)
Permissions-Policy: loopback-network=()
```still behave differently from
```
Permissions-Policy: loopback-network=()
Permissions-Policy: local-network-access=(self)
```
?
Hubert ChaoFirst one: Yes. The empty declaration gets copied to the key for "loopback-network" and "local-network" first. Then the explicit `loopback-network` declaration is processed and does not get added because the key is already set in the map.
Second one: Yes. The first would result in all three being set to self. The second would result in "loopback-network" being set to none, and "local-network-access" and "local-network" being set to self.
is it worth writing down these nuances somewhere (for both the header and the allow attribute)? I'd guess that no matter how much we say "don't use them together" someone is going to and then going to ask why something doesn't work, and writing these nuances might save us investigative time a few months down the line.
Fleshed out the comment with these additional examples, which is probably sufficient for posterity. We could also add test cases for these, but meh.
// allow="local-network-access; local-network 'none';"Chris ThompsonIs it worth calling out that
`allow="local-network-access none; local-network;"`
will still result in `local-network` being enabled?
(at least I think that's what would happen)
Yeah that's a good idea, since it is a subtlety of iframe allowlists. (And I added another unit test for this case.)
(And added a bit to the comment for this case as well.)
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Thompson (Gerrit)
Chris Thompson added 1 comment![]( "Open in Gerrit")
iclelland@ could you PTAL as permissions policy owner? This implements the proposed plan from go/split-lna-permission-dd. Thanks!
Related details
- Ian Clelland
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |