[PWA] Revalidate PWA association metadata on launch to detect revocation [chromium/src : main]
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
m...@chromium.org is from context(googleclient/chrome/chromium_gwsq/chrome/browser/ui/web_applications/config.gwsq,googleclient/chrome/chromium_gwsq/chrome/browser/web_applications/config.gwsq)
Related details
- Marijn Kruisselbrink
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kunjan Patel (Gerrit)
Kunjan Patel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kunjan Patel
| Code-Review | +1 |
1 comment
// have scope extensions and migration sourcesnit: Nicely used 'or' in the CL description! Can we change 'and' to 'or' here to match that exact logic? What do you think?
Related details
- Dibyajyoti Pal
- Marijn Kruisselbrink
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. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
nit: Nicely used 'or' in the CL description! Can we change 'and' to 'or' here to match that exact logic? What do you think?
- Marijn Kruisselbrink
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marijn Kruisselbrink (Gerrit)
Marijn Kruisselbrink added 1 comment![]( "Open in Gerrit")
Things might have changed since the revalidation logic was originally implemented for IWAs, however one big concern we had with the logic at the time is that it would be not great if a temporary network/server issue would cause a valid scope extension to suddenly be no longer considered valid because revalidation happened at just the wrong time. If for example the user happens to be offline we'd throw away all perfectly valid origin associations and migration targets. So for those reasons we intentionally limited this to just IWAs at the time.
I think it is good to somehow extend revalidation to PWAs as well, but especially revalidation that can result in previously validated scope extensions/migration targets to no longer be considered valid probably needs more complicated logic than what is currently implemented in AddValidatedOriginAssociationsCommand?
Related details
- Dibyajyoti Pal
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. |
Marijn Kruisselbrink (Gerrit)
Marijn Kruisselbrink added 1 comment![]( "Open in Gerrit")
Things might have changed since the revalidation logic was originally implemented for IWAs, however one big concern we had with the logic at the time is that it would be not great if a temporary network/server issue would cause a valid scope extension to suddenly be no longer considered valid because revalidation happened at just the wrong time. If for example the user happens to be offline we'd throw away all perfectly valid origin associations and migration targets. So for those reasons we intentionally limited this to just IWAs at the time.
I think it is good to somehow extend revalidation to PWAs as well, but especially revalidation that can result in previously validated scope extensions/migration targets to no longer be considered valid probably needs more complicated logic than what is currently implemented in AddValidatedOriginAssociationsCommand?
That was also the reason why (until recently crrev.com/c/7845255 changed that) this command explicitly only added newly validated origins, and would never remove pre-existing validated ones (hence the name of the command).
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
Things might have changed since the revalidation logic was originally implemented for IWAs, however one big concern we had with the logic at the time is that it would be not great if a temporary network/server issue would cause a valid scope extension to suddenly be no longer considered valid because revalidation happened at just the wrong time. If for example the user happens to be offline we'd throw away all perfectly valid origin associations and migration targets. So for those reasons we intentionally limited this to just IWAs at the time.
I think it is good to somehow extend revalidation to PWAs as well, but especially revalidation that can result in previously validated scope extensions/migration targets to no longer be considered valid probably needs more complicated logic than what is currently implemented in AddValidatedOriginAssociationsCommand?
Hmmmm, do you have any ideas on how we can fix this? Some ideas off the top of the head:
1. If the user is offline, skip revalidation. Allow revalidation if there is an active network connection.
2. If there is a 404 error or the associations file doesn't show the manifest id of the web app, then we remove validation. Other HTTP errors (like a 5xx server error) doesn't revalidate.
3. Some sort of TTL (X days), where we only revalidate if more than X days have passed.
Option 1 is probably the easiest, wdyt?
Related details
- Marijn Kruisselbrink
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. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
Dibyajyoti PalThings might have changed since the revalidation logic was originally implemented for IWAs, however one big concern we had with the logic at the time is that it would be not great if a temporary network/server issue would cause a valid scope extension to suddenly be no longer considered valid because revalidation happened at just the wrong time. If for example the user happens to be offline we'd throw away all perfectly valid origin associations and migration targets. So for those reasons we intentionally limited this to just IWAs at the time.
I think it is good to somehow extend revalidation to PWAs as well, but especially revalidation that can result in previously validated scope extensions/migration targets to no longer be considered valid probably needs more complicated logic than what is currently implemented in AddValidatedOriginAssociationsCommand?
Hmmmm, do you have any ideas on how we can fix this? Some ideas off the top of the head:
1. If the user is offline, skip revalidation. Allow revalidation if there is an active network connection.
2. If there is a 404 error or the associations file doesn't show the manifest id of the web app, then we remove validation. Other HTTP errors (like a 5xx server error) doesn't revalidate.
3. Some sort of TTL (X days), where we only revalidate if more than X days have passed.Option 1 is probably the easiest, wdyt?
AI also had this idea: Asynchronous Background Sync Revalidation
Instead of running revalidation on the critical path of launching the web app (where failures immediately affect the launch environment):
- Decouple revalidation from LaunchWebAppCommand.
- Schedule revalidation via a background task runner or a background sync manager that runs periodically or when the device is detected to be online and idle.
- While the background revalidation is pending or failing, the active web app continues to use its last successfully validated associations cached in the registry.
Daniel Murphy (Gerrit)
Daniel Murphy added 1 comment![]( "Open in Gerrit")
Dibyajyoti PalThings might have changed since the revalidation logic was originally implemented for IWAs, however one big concern we had with the logic at the time is that it would be not great if a temporary network/server issue would cause a valid scope extension to suddenly be no longer considered valid because revalidation happened at just the wrong time. If for example the user happens to be offline we'd throw away all perfectly valid origin associations and migration targets. So for those reasons we intentionally limited this to just IWAs at the time.
I think it is good to somehow extend revalidation to PWAs as well, but especially revalidation that can result in previously validated scope extensions/migration targets to no longer be considered valid probably needs more complicated logic than what is currently implemented in AddValidatedOriginAssociationsCommand?
Dibyajyoti PalHmmmm, do you have any ideas on how we can fix this? Some ideas off the top of the head:
1. If the user is offline, skip revalidation. Allow revalidation if there is an active network connection.
2. If there is a 404 error or the associations file doesn't show the manifest id of the web app, then we remove validation. Other HTTP errors (like a 5xx server error) doesn't revalidate.
3. Some sort of TTL (X days), where we only revalidate if more than X days have passed.Option 1 is probably the easiest, wdyt?
AI also had this idea: Asynchronous Background Sync Revalidation
Instead of running revalidation on the critical path of launching the web app (where failures immediately affect the launch environment):
- Decouple revalidation from LaunchWebAppCommand.
- Schedule revalidation via a background task runner or a background sync manager that runs periodically or when the device is detected to be online and idle.
- While the background revalidation is pending or failing, the active web app continues to use its last successfully validated associations cached in the registry.
sooooo FYI this doesn't actually revoke? We only add new validated ones.
I would recommend
- a totally new command
- some leniency with failures that are due to network conditions (store failure #s and time of first failure?)
- can be run on launch and/or delayed on startup, after network is back?
I don't have opinions on best thing, but definitely a new command as the addvalidated is pretty specific to this one case (As it ended up being kinda complicated)
Related details
- Dibyajyoti Pal
- Marijn Kruisselbrink
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. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
Dibyajyoti PalThings might have changed since the revalidation logic was originally implemented for IWAs, however one big concern we had with the logic at the time is that it would be not great if a temporary network/server issue would cause a valid scope extension to suddenly be no longer considered valid because revalidation happened at just the wrong time. If for example the user happens to be offline we'd throw away all perfectly valid origin associations and migration targets. So for those reasons we intentionally limited this to just IWAs at the time.
I think it is good to somehow extend revalidation to PWAs as well, but especially revalidation that can result in previously validated scope extensions/migration targets to no longer be considered valid probably needs more complicated logic than what is currently implemented in AddValidatedOriginAssociationsCommand?
Dibyajyoti PalHmmmm, do you have any ideas on how we can fix this? Some ideas off the top of the head:
1. If the user is offline, skip revalidation. Allow revalidation if there is an active network connection.
2. If there is a 404 error or the associations file doesn't show the manifest id of the web app, then we remove validation. Other HTTP errors (like a 5xx server error) doesn't revalidate.
3. Some sort of TTL (X days), where we only revalidate if more than X days have passed.Option 1 is probably the easiest, wdyt?
Daniel MurphyAI also had this idea: Asynchronous Background Sync Revalidation
Instead of running revalidation on the critical path of launching the web app (where failures immediately affect the launch environment):
- Decouple revalidation from LaunchWebAppCommand.
- Schedule revalidation via a background task runner or a background sync manager that runs periodically or when the device is detected to be online and idle.
- While the background revalidation is pending or failing, the active web app continues to use its last successfully validated associations cached in the registry.
sooooo FYI this doesn't actually revoke? We only add new validated ones.
I would recommend
- a totally new command
- some leniency with failures that are due to network conditions (store failure #s and time of first failure?)
- can be run on launch and/or delayed on startup, after network is back?
I don't have opinions on best thing, but definitely a new command as the addvalidated is pretty specific to this one case (As it ended up being kinda complicated)
The command does revoke, ever since [this CL](https://chromium-review.git.corp.google.com/c/chromium/src/+/7845255) went in. The browser tests added here are proof that it revokes. Maybe we should rename the command to "UpdateValidatedOriginAssociations" or something like that?
Related details
- Daniel Murphy
- Marijn Kruisselbrink
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. |
Dibyajyoti Pal (Gerrit)
Offline discussion with Dan to resolve the issues highlighted here:
1. We'll rename the command to `UpdateValidatedOriginAssociations`.
2. Update the behavior so that the command doesn't run if the user is offline.
3. PWAs will be limited to one check per app for 10 days post launch.
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 2 comments![]( "Open in Gerrit")
Implemented as per discussion, resolving.
TEST_F(UpdateValidatedOriginAssociationsCommandTest, Offline) {
auto mock_network_change_notifier =
std::make_unique<net::test::ScopedMockNetworkChangeNotifier>();
mock_network_change_notifier->mock_network_change_notifier()
->SetConnectionType(net::NetworkChangeNotifier::CONNECTION_NONE);
GURL start_url("https://example.com/");
ScopeExtensionInfo extension = ScopeExtensionInfo::CreateForScope(
GURL("https://example.org/scope"), /*has_origin_wildcard=*/false);
webapps::AppId app_id = InstallApp(start_url, {extension});
base::HistogramTester tester;
base::test::TestFuture<UpdateValidatedOriginAssociationsResult> future;
provider().scheduler().UpdateValidatedOriginAssociations(
app_id, future.GetCallback());
ASSERT_EQ(UpdateValidatedOriginAssociationsResult::kOffline, future.Get());
tester.ExpectUniqueSample("WebApp.ValidatedOriginAssociations.Updated",
UpdateValidatedOriginAssociationsResult::kOffline,
1);
const WebApp* app = provider().registrar_unsafe().GetAppById(app_id);
EXPECT_TRUE(app->validated_scope_extensions().empty());
}Apart from this test, the whole file is a rename of the previous `add_<>_unittest.cc` file.
Related details
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. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
PTAL again! This CL is a bit big, but:
1. Most of it is renaming `Add_<X>` to `Update_<X>`, including the tests and metrics.
2. For the added behavior, the rename diff for `UpdateValidatedOriginAssociations` command. will have all of that information.
3. The behavior change being added is in the `LaunchWebAppCommand`.
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
PTAL again! This CL is a bit big, but:
1. Most of it is renaming `Add_<X>` to `Update_<X>`, including the tests and metrics.
2. For the added behavior, the rename diff for `UpdateValidatedOriginAssociations` command. will have all of that information.
3. The behavior change being added is in the `LaunchWebAppCommand`.
Happy to break it into chunks if that makes reviewing simpler.