[WebInstallAPI] 2/n: Add manifest URL fetcher [chromium/src : main]
Daniel Murphy (Gerrit)
Daniel Murphy voted and added 5 comments![]( "Open in Gerrit")
Votes added by Daniel Murphy
| Code-Review | +1 |
5 comments
// Manifest URLs that redirect are not supported. Abort the downloadside note: I could see us allowing same-origin redirect, to help with versioning (e.g. app.com/manifest.json -> app.com/manifest_v1.2.json), but IDK if that conflicts with restrictions you have planned.
EXPECT_THAT(future.Take(), ValueIs(Eq(kValidManifestJson)));nit: ASSERT_TRUE(future.Wait());, then expect_that
this makes it so it doesn't crash if it is a timeout
here and elsewhere
std::unique_ptr<WebInstallManifestFetcher> manifest_fetcher_;forward declar
#include "chrome/browser/web_applications/web_install_manifest_fetcher.h"prefer to have a dedicated file for the result, instead of an inner class. you can likely put it in the model/ subdir
if (!install_target.SchemeIsHTTPOrHTTPS()) {Lia HiscockWhy is HTTP ok?
Lu Huanghmm...so for the install_url flow, HTTP gets excluded because WebAppDataRetriever [sets `check_eligibility = true`](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/web_contents/web_app_data_retriever.cc;l=181?q=web_app_data_retriever.cc&ss=chromium%2Fchromium%2Fsrc) when it asks the InstallableManager for the page manifest, which traces to [InstallableEvaluator::CheckEligibility](https://source.chromium.org/chromium/chromium/src/+/main:components/webapps/browser/installable/installable_evaluator.cc;l=270)
but I'm realizing since we're not using the data retriever, we actually do need to exclude HTTP on our own.
**Updated so we only allow HTTPS or http://localhost. Also added service_impl unittest coverage**
Lia HiscockAck - but why allow localhost? Is there a similar example elsewhere?
Lu Huangyeah it's explicitly treated as trustworthy by the web app [`installable_evaluator`](https://source.chromium.org/chromium/chromium/src/+/main:components/webapps/browser/installable/installable_evaluator.cc;l=317-323?q=installable_evaluator.cc&ss=chromium). This enables devs to do local testing, also pretty sure browsertests use localhost to install test apps?
Here's the spec text too on [secure contexts - 5.2. localhost](https://www.w3.org/TR/secure-contexts/#localhost)
Daniel MurphyI'll leave the choice here to Dan.
Sorry I was more pointing at the http:// part of the http://localhost. In browser tests we do use https test servers when necessary so I don't know if testing is a good enough reason to special case http://localhost. If we require https, whether it's localhost then doesn't matter.
:shrug: seems fine to allow http on localhost, it matches our existing behavior I believe.
Related details
- Lia Hiscock
- Mike West
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. |
Lia Hiscock (Gerrit)
Lia Hiscock voted and added 5 comments![]( "Open in Gerrit")
Votes added by Lia Hiscock
| Commit-Queue | +1 |
5 comments
// Manifest URLs that redirect are not supported. Abort the downloadside note: I could see us allowing same-origin redirect, to help with versioning (e.g. app.com/manifest.json -> app.com/manifest_v1.2.json), but IDK if that conflicts with restrictions you have planned.
Acknowledged - no known conflicts ATM. Tracking as a followup for simplicity - https://issues.chromium.org/issues/526696765
EXPECT_THAT(future.Take(), ValueIs(Eq(kValidManifestJson)));nit: ASSERT_TRUE(future.Wait());, then expect_that
this makes it so it doesn't crash if it is a timeout
here and elsewhere
Acknowledged
std::unique_ptr<WebInstallManifestFetcher> manifest_fetcher_;Lia Hiscockforward declar
Done
#include "chrome/browser/web_applications/web_install_manifest_fetcher.h"prefer to have a dedicated file for the result, instead of an inner class. you can likely put it in the model/ subdir
Done
if (!install_target.SchemeIsHTTPOrHTTPS()) {Lia HiscockWhy is HTTP ok?
Lu Huanghmm...so for the install_url flow, HTTP gets excluded because WebAppDataRetriever [sets `check_eligibility = true`](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/web_contents/web_app_data_retriever.cc;l=181?q=web_app_data_retriever.cc&ss=chromium%2Fchromium%2Fsrc) when it asks the InstallableManager for the page manifest, which traces to [InstallableEvaluator::CheckEligibility](https://source.chromium.org/chromium/chromium/src/+/main:components/webapps/browser/installable/installable_evaluator.cc;l=270)
but I'm realizing since we're not using the data retriever, we actually do need to exclude HTTP on our own.
**Updated so we only allow HTTPS or http://localhost. Also added service_impl unittest coverage**
Lia HiscockAck - but why allow localhost? Is there a similar example elsewhere?
Lu Huangyeah it's explicitly treated as trustworthy by the web app [`installable_evaluator`](https://source.chromium.org/chromium/chromium/src/+/main:components/webapps/browser/installable/installable_evaluator.cc;l=317-323?q=installable_evaluator.cc&ss=chromium). This enables devs to do local testing, also pretty sure browsertests use localhost to install test apps?
Here's the spec text too on [secure contexts - 5.2. localhost](https://www.w3.org/TR/secure-contexts/#localhost)
Daniel MurphyI'll leave the choice here to Dan.
Sorry I was more pointing at the http:// part of the http://localhost. In browser tests we do use https test servers when necessary so I don't know if testing is a good enough reason to special case http://localhost. If we require https, whether it's localhost then doesn't matter.
:shrug: seems fine to allow http on localhost, it matches our existing behavior I believe.
ACK - leaving as is then. And yes, per the [installable_evaluator](https://source.chromium.org/chromium/chromium/src/+/main:components/webapps/browser/installable/installable_evaluator.cc;l=317-323?q=installable_evaluator.cc&ss=chromium) -
```
bool InstallableEvaluator::IsOriginConsideredSecure(const GURL& url) {
auto origin = url::Origin::Create(url);
auto* webapps_client = webapps::WebappsClient::Get();
return (webapps_client && webapps_client->IsOriginConsideredSecure(origin)) ||
--> net::IsLocalhost(url) ||
network::SecureOriginAllowlist::GetInstance().IsOriginAllowlisted(
origin);
```
Related details
- Daniel Murphy
- Lu Huang
- Mike West
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. |
Lia Hiscock (Gerrit)
Lia Hiscock added 1 comment![]( "Open in Gerrit")
Lia HiscockLooks good for more reviewers. Thanks!
Done
Related details
- Daniel Murphy
- Lu Huang
- Mike West
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mike West
| Code-Review | +1 |
1 comment
annotations LGTM. I skimmed the rest and it looked reasonable, but I'm mostly relying on Dan's review of //c/b/web_applications
Related details
- Daniel Murphy
- Lia Hiscock
- Lu Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Murphy (Gerrit)
Daniel Murphy voted Code-Review+1![]( "Open in Gerrit")
- Lia Hiscock
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lia Hiscock (Gerrit)
Lia Hiscock voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
17 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
[WebInstallAPI] 2/n: Add manifest URL fetcher
Introduce WebInstallManifestFetcher, a class that downloads web app
manifest JSON from a URL using SimpleURLLoader. This supports the
manifest-first install flow where a page provides a manifest URL via
navigator.install() or an <install> element.
The fetcher:
- Downloads manifest content up to a 5MB size limit
- Rejects redirecting manifest URLs
- Retries up to 3 times on 5xx errors and network changes
- Omits cookies from requests
- Returns raw manifest content without parsing
Wire the fetcher into WebInstallServiceImpl::InstallFromManifest,
introducing InstallFromManifestInternal (parallel of InstallInternal)
which wraps the mojo callback and auto-releases the install-in-progress
guard on every exit path. Non-HTTPS URLs (excluding localhost) are
rejected early with kDataError. Fetch failures also return kDataError.
Manifest parsing will be 3/n.
Dev design doc -
https://docs.google.com/document/d/1rGvLhD4SR8Y9M1wVmqgyesPNkbZGU7HOqlttjEFJ5Vo/edit?tab=t.eiywj6swkfs6#bookmark=id.g6hjyl26llpy
Explainer - aka.ms/WebInstall
- M chrome/browser/web_applications/BUILD.gn
- A chrome/browser/web_applications/model/web_install_manifest_fetch_error.h
- A chrome/browser/web_applications/web_install_manifest_fetcher.cc
- A chrome/browser/web_applications/web_install_manifest_fetcher.h
- A chrome/browser/web_applications/web_install_manifest_fetcher_unittest.cc
- M chrome/browser/web_applications/web_install_service_impl.cc
- M chrome/browser/web_applications/web_install_service_impl.h
- M chrome/browser/web_applications/web_install_service_impl_unittest.cc
- M tools/traffic_annotation/summary/annotations.xml
- M tools/traffic_annotation/summary/grouping.xml
Code-Review: +1 by Daniel Murphy, +1 by Mike West
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |