Fix Privacy Side-Channel in Device Posture API via Background Tabs [chromium/src : main]
Alvin Ji (Gerrit)
Alvin Ji added 1 comment![]( "Open in Gerrit")
Related details
- Matt Reynolds
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matt Reynolds (Gerrit)
Matt Reynolds added 2 comments![]( "Open in Gerrit")
class FakeDevicePostureClient : public blink::mojom::DevicePostureClient {Let's make this a mock instead of a test fake so we can mock OnPostureChanged in tests.
EXPECT_TRUE(base::test::RunUntil([&]() {
return client.last_posture() == blink::mojom::DevicePostureType::kFolded;
}));We shouldn't need to use RunUntil to poll here since the client (which we faked) is notified when the posture changed. Instead of keeping internal state in the test fake and polling for that state, it would be better to register a callback with the fake that gets called with the current posture whenever OnPostureChanged is called. That's basically just a mock so I recommend switching the test fake to a mock.
Related details
- Alvin Ji
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. |
Alvin Ji (Gerrit)
Alvin Ji added 2 comments![]( "Open in Gerrit")
class FakeDevicePostureClient : public blink::mojom::DevicePostureClient {Let's make this a mock instead of a test fake so we can mock OnPostureChanged in tests.
Done
EXPECT_TRUE(base::test::RunUntil([&]() {
return client.last_posture() == blink::mojom::DevicePostureType::kFolded;
}));We shouldn't need to use RunUntil to poll here since the client (which we faked) is notified when the posture changed. Instead of keeping internal state in the test fake and polling for that state, it would be better to register a callback with the fake that gets called with the current posture whenever OnPostureChanged is called. That's basically just a mock so I recommend switching the test fake to a mock.
- Matt Reynolds
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matt Reynolds (Gerrit)
Matt Reynolds added 4 comments![]( "Open in Gerrit")
(is_posture_emulated_ && emulated_posture_)
? *emulated_posture_
: platform_provider_->GetDevicePosture();This logic is duplicated below in `OnVisibilityChanged`, please factor it out into a helper function.
if (visibility == Visibility::HIDDEN) {`visibility` can have three states: VISIBLE, HIDDEN, or OCCLUDED. Here we're treating OCCLUDED the same as VISIBLE, which seems unintended. If the tab is covered by other windows, should it be counted as visible? Is there any reason an occluded tab would need to know the device posture?
: platform_provider_->GetDevicePosture();Before calling platform_provider_->GetDevicePosture(), check if there are any posture_clients_. If there are no clients we can skip checking the posture.
test_web_contents()->WasShown();Please add a test with `WasOccluded`
Related details
- Alvin Ji
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. |
Alvin Ji (Gerrit)
Alvin Ji voted and added 4 comments![]( "Open in Gerrit")
Votes added by Alvin Ji
| Commit-Queue | +1 |
4 comments
(is_posture_emulated_ && emulated_posture_)
? *emulated_posture_
: platform_provider_->GetDevicePosture();This logic is duplicated below in `OnVisibilityChanged`, please factor it out into a helper function.
I added `GetCurrentPosture` to reuse the code here. Thanks!
`visibility` can have three states: VISIBLE, HIDDEN, or OCCLUDED. Here we're treating OCCLUDED the same as VISIBLE, which seems unintended. If the tab is covered by other windows, should it be counted as visible? Is there any reason an occluded tab would need to know the device posture?
Thanks for pointing this out. I think we should treat OCCLUDED as HIDDEN and only passing update when page is VISIBLE. I updated the visibility check now only allow the update in VISIBLE case.
Before calling platform_provider_->GetDevicePosture(), check if there are any posture_clients_. If there are no clients we can skip checking the posture.
Done
Please add a test with `WasOccluded`
- Matt Reynolds
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |