Do not install WebAPKs with web manifests with invalid URL components

Do not install WebAPKs with web manifests with invalid URL components

This CL disallows generating WebAPKs for pages with Web Manifests with
passwords and user name URL components.

The WebAPK server caches web manifests for WebAPKs that it has previously
generated. We do not want to store usernames and passwords on the WebAPK server.

BUG=622465
Committed: https://crrev.com/6981e68516d341354372d174c557fb85653adcd7
Cr-Commit-Position: refs/heads/master@{#414907}

[pkotwicz, 2016-08-17 03:02:36]

pkotwicz@chromium.org changed reviewers:
+ hanxi@chromium.org

[pkotwicz, 2016-08-17 03:02:37]

Xi can you please take a look?

[Xi Han, 2016-08-17 15:39:00]

lgtm

[pkotwicz, 2016-08-17 15:49:15]

pkotwicz@chromium.org changed reviewers:
+ dominickn@chromium.org

[pkotwicz, 2016-08-17 15:49:16]

Dominick, can you please take a look?

[dominickn, 2016-08-17 22:54:43]

https://codereview.chromium.org/2248293002/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_logging.cc (right):

https://codereview.chromium.org/2248293002/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_logging.cc:37: static const char
kUrlUsernameAndPasswordNotSupportedMessage[] =
Move to the end to match the order in the enum. Also add "for WebAPKs" to the
string.

https://codereview.chromium.org/2248293002/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_logging.h (right):

https://codereview.chromium.org/2248293002/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_logging.h:29:
URL_USERNAME_AND_PASSWORD_NOT_SUPPORTED,
Add this to the end prior to MAX_ERROR_CODE. This enum is append-only since it's
used for UMA.

https://codereview.chromium.org/2248293002/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_manager.cc (right):

https://codereview.chromium.org/2248293002/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:141: if
(DoesManifestHaveUrlWithUsernameOrPassword(manifest) ||
I think this check should belong in WebAPK-specific code, not in
InstallableManager. You can check the URL once InstallableManager gets back to
you. This restriction doesn't really make sense for other InstallableManager
clients like banners.

If you're thinking about efficiency, you could even split the InstallableManager
call into two: one to get the manifest first, check it's URL, and then a second
call to fetch everything else. App banners do this to support native apps (which
don't need a SW or app icon in the manifest).

[pkotwicz, 2016-08-18 01:11:41]

https://codereview.chromium.org/2248293002/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_manager.cc (right):

https://codereview.chromium.org/2248293002/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:141: if
(DoesManifestHaveUrlWithUsernameOrPassword(manifest) ||
The reason I put this code here is that there are at least two places which will
need to do this check:
- ShortcutHelper::AddToLauncherWithSkBitmap() (from both banners and
add-to-homescreen-dialog)
- ManifestUpgradeDetectorFetcher in order to determine whether an updated Web
Manifest is still WebAPK-able

Should I create a static only class to determine whether an installable
WebManifest is WebAPK-able?
WebApkManifestValidator::IsInstallableWebManifestValidForWebAPK() perhaps?

For the sake of this milestone we are going to consider non-WebAPKable Web
Manifests as non-installable (because it is simpler to implement)
For instance, when a user has enabled WebAPKs via chrome://flags, the app-banner
will only show if the Web Manifest is WebAPK-able

[dominickn, 2016-08-18 01:19:16]

https://codereview.chromium.org/2248293002/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_manager.cc (right):

https://codereview.chromium.org/2248293002/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager.cc:141: if
(DoesManifestHaveUrlWithUsernameOrPassword(manifest) ||
On 2016/08/18 01:11:41, pkotwicz wrote:
> The reason I put this code here is that there are at least two places which
will
> need to do this check:
> - ShortcutHelper::AddToLauncherWithSkBitmap() (from both banners and
> add-to-homescreen-dialog)
> - ManifestUpgradeDetectorFetcher in order to determine whether an updated Web
> Manifest is still WebAPK-able
> 
> Should I create a static only class to determine whether an installable
> WebManifest is WebAPK-able?
> WebApkManifestValidator::IsInstallableWebManifestValidForWebAPK() perhaps?
> 
> For the sake of this milestone we are going to consider non-WebAPKable Web
> Manifests as non-installable (because it is simpler to implement)
> For instance, when a user has enabled WebAPKs via chrome://flags, the
app-banner
> will only show if the Web Manifest is WebAPK-able
> 

I think a separate WebApkManifestValidator is the right way to go, since right
now we know that WebAPKs have more stringent requirements than banners and add
to homescreen (though the base requirements are the same). You could probably
put it into ShortcutHelper for now to be honest.

That way, we can isolate WebAPK specific behaviour for now, and in the future
look to merge things into InstallableManager if we decide that banners and add
to homescreen should also have the more stringent WebAPK behaviour. Is that
reasonable?

[pkotwicz, 2016-08-23 21:57:17]

Dominick can you please take another look?

I have moved the URL-with-password-component check out of InstallableManager and
into a utility file as you have suggested. This CL does not apply the check to
the add-to-homescreen flow. I will deal with the add-to-homescreen flow once
https://codereview.chromium.org/2244223002/ is approved and landed

[pkotwicz, 2016-08-23 22:06:51]

Description was changed from

==========
Do not install WebAPKs with web manifests with passwords in URL

BUG=622465
==========

to

==========
Do not install WebAPKs with web manifests with invalid URL components

This CL disallows generating WebAPKs for pages with Web Manifests with:
- Passwords and user name URL components.
    The WebAPK server caches web manifests for WebAPKs that it has previously
    generated. We do not want to store usernames and passwords on the WebAPK
    server.
- Explictly set port in a URL.
    This is for the sake of simplicity. We would need additional logic to
    conditionally set the port on the <intent-filter> in the WebAPK's Android
    Manifest.

BUG=622465
==========

[dominickn, 2016-08-24 03:27:11]

This is much cleaner, thanks! Mostly nits - I should be back up to full review
speed now.

https://codereview.chromium.org/2248293002/diff/20001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/2248293002/diff/20001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager_android.cc:163:
ReportStatus(web_contents(), error_code);
When CheckWebManifestUrlsWebApkCompatible returns a boolean, you can inline the
error code in ReportStatus(web_contents(), URL_NOT_SUPPORTED_FOR_WEBAPK);

https://codereview.chromium.org/2248293002/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc (right):

https://codereview.chromium.org/2248293002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:140:
NO_ERROR_DETECTED) {
Do you still want to check data.installable here?

https://codereview.chromium.org/2248293002/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_web_manifest_checker.h (right):

https://codereview.chromium.org/2248293002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_web_manifest_checker.h:17:
InstallableStatusCode CheckWebManifestUrlsWebApkCompatible(
As per only having one status code for this method, make this return a bool, and
remove the installable_logging.h include.

https://codereview.chromium.org/2248293002/diff/20001/chrome/browser/installa...
File chrome/browser/installable/installable_logging.h (right):

https://codereview.chromium.org/2248293002/diff/20001/chrome/browser/installa...
chrome/browser/installable/installable_logging.h:47:
URL_USERNAME_AND_PASSWORD_NOT_SUPPORTED_FOR_WEBAPK,
I think you can just have one error here - URL_NOT_SUPPORTED_FOR_WEBAPK. In the
reason string, you can then have "A URL in the manifest contains a username,
password, or port". That makes the checking method simpler because it can return
a boolean rather than an error code. Having just one error code related to
WebAPK URL satisfaction makes sense to me because it's super obvious to the
developer what's gone wrong - they don't need a separate error for the port.

[pkotwicz, 2016-08-24 14:57:08]

Patchset #3 (id:40001) has been deleted

[pkotwicz, 2016-08-24 15:01:10]

Patchset #3 (id:60001) has been deleted

[pkotwicz, 2016-08-24 15:04:56]

Dominick can you please take another look?

https://codereview.chromium.org/2248293002/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc (right):

https://codereview.chromium.org/2248293002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:140:
NO_ERROR_DETECTED) {
Yes I do. Thank you for catching this bug!

https://codereview.chromium.org/2248293002/diff/20001/chrome/browser/installa...
File chrome/browser/installable/installable_logging.h (right):

https://codereview.chromium.org/2248293002/diff/20001/chrome/browser/installa...
chrome/browser/installable/installable_logging.h:47:
URL_USERNAME_AND_PASSWORD_NOT_SUPPORTED_FOR_WEBAPK,
I now use just one error code. You are right that it simplifies things a lot.

[dominickn, 2016-08-24 15:36:54]

lgtm % minor nits. Thanks!

https://codereview.chromium.org/2248293002/diff/80001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_web_manifest_checker_unittest.cc
(right):

https://codereview.chromium.org/2248293002/diff/80001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_web_manifest_checker_unittest.cc:53:
TEST(WebApkWebManifestCheckerTest, CompatibleURLHasNoPort) {
Minor nit - you should test manifest.scope as well.

https://codereview.chromium.org/2248293002/diff/80001/chrome/browser/installa...
File chrome/browser/installable/installable_logging.cc (right):

https://codereview.chromium.org/2248293002/diff/80001/chrome/browser/installa...
chrome/browser/installable/installable_logging.cc:63: "A URL in the web manifest
contains a username, password, or port";
Nit: lowercase "A" (sorry, my fault!) to match the other strings.

[pkotwicz, 2016-08-25 19:22:06]

Patchset #4 (id:100001) has been deleted

[pkotwicz, 2016-08-25 19:36:43]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-08-25 19:36:44]

The patchset sent to the CQ was uploaded after l-g-t-m from hanxi@chromium.org,
dominickn@chromium.org
Link to the patchset: https://codereview.chromium.org/2248293002/#ps140001
(title: "Merge branch 'master' into webapk_installable_checker")

[commit-bot: I haz the power, 2016-08-25 19:38:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 21:13:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 21:13:33]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[pkotwicz, 2016-08-26 23:36:08]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-08-26 23:36:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, hanxi@chromium.org
Link to the patchset: https://codereview.chromium.org/2248293002/#ps160001
(title: "Merge branch 'master' into webapk_installable_checker")

[pkotwicz, 2016-08-26 23:36:18]

The CQ bit was unchecked by pkotwicz@chromium.org

[pkotwicz, 2016-08-26 23:37:31]

Description was changed from

==========
Do not install WebAPKs with web manifests with invalid URL components

This CL disallows generating WebAPKs for pages with Web Manifests with:
- Passwords and user name URL components.
    The WebAPK server caches web manifests for WebAPKs that it has previously
    generated. We do not want to store usernames and passwords on the WebAPK
    server.
- Explictly set port in a URL.
    This is for the sake of simplicity. We would need additional logic to
    conditionally set the port on the <intent-filter> in the WebAPK's Android
    Manifest.

BUG=622465
==========

to

==========
Do not install WebAPKs with web manifests with invalid URL components

This CL disallows generating WebAPKs for pages with Web Manifests with
passwords and user name URL components.

The WebAPK server caches web manifests for WebAPKs that it has previously
generated. We do not want to store usernames and passwords on the WebAPK server.

BUG=622465
==========

[pkotwicz, 2016-08-26 23:39:46]

I removed the checking for an explicitly set port in
AreWebManifestUrlsWebApkCompatible() because EmbeddedTestServer::GetURL()
returns a URL with an explicitly set port

[pkotwicz, 2016-08-26 23:39:50]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-08-26 23:40:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-27 10:53:28]

Description was changed from

==========
Do not install WebAPKs with web manifests with invalid URL components

This CL disallows generating WebAPKs for pages with Web Manifests with
passwords and user name URL components.

The WebAPK server caches web manifests for WebAPKs that it has previously
generated. We do not want to store usernames and passwords on the WebAPK server.

BUG=622465
==========

to

==========
Do not install WebAPKs with web manifests with invalid URL components

This CL disallows generating WebAPKs for pages with Web Manifests with
passwords and user name URL components.

The WebAPK server caches web manifests for WebAPKs that it has previously
generated. We do not want to store usernames and passwords on the WebAPK server.

BUG=622465
==========

[commit-bot: I haz the power, 2016-08-27 10:53:29]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2016-08-27 10:57:08]

Description was changed from

==========
Do not install WebAPKs with web manifests with invalid URL components

This CL disallows generating WebAPKs for pages with Web Manifests with
passwords and user name URL components.

The WebAPK server caches web manifests for WebAPKs that it has previously
generated. We do not want to store usernames and passwords on the WebAPK server.

BUG=622465
==========

to

==========
Do not install WebAPKs with web manifests with invalid URL components

This CL disallows generating WebAPKs for pages with Web Manifests with
passwords and user name URL components.

The WebAPK server caches web manifests for WebAPKs that it has previously
generated. We do not want to store usernames and passwords on the WebAPK server.

BUG=622465

Committed: https://crrev.com/6981e68516d341354372d174c557fb85653adcd7
Cr-Commit-Position: refs/heads/master@{#414907}
==========

[commit-bot: I haz the power, 2016-08-27 10:57:09]

Patchset 6 (id:??) landed as
https://crrev.com/6981e68516d341354372d174c557fb85653adcd7
Cr-Commit-Position: refs/heads/master@{#414907}

[dominickn, 2016-08-29 00:15:41]

https://codereview.chromium.org/2248293002/diff/160001/chrome/browser/install...
File chrome/browser/installable/installable_logging.cc (right):

https://codereview.chromium.org/2248293002/diff/160001/chrome/browser/install...
chrome/browser/installable/installable_logging.cc:62: static const char
kUrlNotSupportedForWebApkMessage[] =
Nit: you removed checking the username for a port, but the error message still
mentions a port!