Read the bare minimum of data from the WebAPK launch intent.

Read the bare minimum of data from the WebAPK launch intent.

The launch intent for WebappLauncherActivity may be sent by any app, not
necessarily Chrome or a WebAPK. For WebAPKs (unlike non-WebAPK web apps) the id
is predictable. For WebAPKs we verify that the package name in the intent
matches an installed WebAPK and that the start URL falls within the WebAPK's
scope. We do not validate any of the other data in the launch intent.

Prior to this CL, by customizing the launch intent a third party APK was able
to launch a WebAPK with an arbirtrary name and icon on the splash screen.
This CL reduces the amount of data we extract from the WebappLauncherActivity
launch intent to the minimum possible. We still read from the launch intent the
following data:
- WebAPK package name
- URL to navigate the WebAPK to. We cannot use the WebAPK's start URL because
the WebAPK can be launched at any URL within the WebAPK scope via deep linking.
We do verify that the URL in the intent is within the WebAPK scope.
- The reason the WebAPK got launched (e.g. launched from a notification). This
is used for UMA only.

BUG=651640
Committed: https://crrev.com/0b7c2979c1f64fbfb6ce5967a1424890b86579f1
Cr-Commit-Position: refs/heads/master@{#425093}

[pkotwicz, 2016-10-09 17:59:24]

Description was changed from

==========
Read the bare minimum of data from the WebAPK launch intent.

The WebappActivity launch intent may be

BUG=649440

Review-Url: https://codereview.chromium.org/2358223002
Cr-Commit-Position: refs/heads/master@{#422229}
==========

to

==========
Read the bare minimum of data from the WebAPK launch intent.

The launch intent for WebappLauncherActivity may be sent by any app, not
necessarily Chrome or a WebAPK. For WebAPKs (unlike non-WebAPK web apps) the id
is predictable. For WebAPKs we verify that the package name in the intent
matches an installed WebAPK and that the start URL falls within the WebAPK's
scope. We do not validate any of the other data in the launch intent.

Prior to this CL, by customizing the launch intent a third party APK was able to
launch a WebAPK with an arbirtrary name and icon on the splash screen.
This CL reduces the amount of data we extract from the WebappLauncherActivity
launch intent to the minimum possible. We still read from the launch intent the
following data:
- WebAPK package name
- URL to navigate the WebAPK to. We cannot use the WebAPK's start URL because
the WebAPK can be launched at any URL within the WebAPK scope via deep linking.
We do verify that the URL in the intent is within the WebAPK scope.
- The reason the WebAPK got launched (e.g. launched from a notification). This
is used for UMA only.

BUG=651640
==========

[pkotwicz, 2016-10-09 17:59:24]

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

[pkotwicz, 2016-10-09 18:00:25]

Dominick, can you please take a look?

[pkotwicz, 2016-10-09 18:18:07]

Patchset #1 (id:1) has been deleted

[dominickn, 2016-10-11 05:54:52]

Please reformat the CL description so that it's 74 characters wide.

https://codereview.chromium.org/2409483002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java
(right):

https://codereview.chromium.org/2409483002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java:76:
* @param name Name of the <meta-data> tag to extract the value from.
Missing @param for defaultValue

https://codereview.chromium.org/2409483002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java
(right):

https://codereview.chromium.org/2409483002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:83:
if (isValidWebApk || isValidMacForUrl(webappUrl, webappMac)
Make this if clause an else clause of the above if statement:

else if (!isValidMacForUrl(webappUrl, webappMac) &&
            !wasIntentFromChrome(intent)) {
  Log.E(TAG, "Shortcut opened in Chrome");
  // Other stuff from lines 93 - 101
  return;
}

// Lines 85 - 87 here.

That makes this a little tidier I think.

https://codereview.chromium.org/2409483002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java
(left):

https://codereview.chromium.org/2409483002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:227:
// EXTRA_WEBAPK_DISPLAY_MODE takes precedence over EXTRA_DISPLAY_MODE.
Strictly speaking, you could keep this test, and just remove the WebAPK
precedence stuff right?

https://codereview.chromium.org/2409483002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:258:
// EXTRA_WEBAPK_ORIENTATION takes precedence over EXTRA_ORIENTATION.
Ditto.

[pkotwicz, 2016-10-12 04:15:19]

Description was changed from

==========
Read the bare minimum of data from the WebAPK launch intent.

The launch intent for WebappLauncherActivity may be sent by any app, not
necessarily Chrome or a WebAPK. For WebAPKs (unlike non-WebAPK web apps) the id
is predictable. For WebAPKs we verify that the package name in the intent
matches an installed WebAPK and that the start URL falls within the WebAPK's
scope. We do not validate any of the other data in the launch intent.

Prior to this CL, by customizing the launch intent a third party APK was able to
launch a WebAPK with an arbirtrary name and icon on the splash screen.
This CL reduces the amount of data we extract from the WebappLauncherActivity
launch intent to the minimum possible. We still read from the launch intent the
following data:
- WebAPK package name
- URL to navigate the WebAPK to. We cannot use the WebAPK's start URL because
the WebAPK can be launched at any URL within the WebAPK scope via deep linking.
We do verify that the URL in the intent is within the WebAPK scope.
- The reason the WebAPK got launched (e.g. launched from a notification). This
is used for UMA only.

BUG=651640
==========

to

==========
Read the bare minimum of data from the WebAPK launch intent.

The launch intent for WebappLauncherActivity may be sent by any app, not
necessarily Chrome or a WebAPK. For WebAPKs (unlike non-WebAPK web apps) the id
is predictable. For WebAPKs we verify that the package name in the intent
matches an installed WebAPK and that the start URL falls within the WebAPK's
scope. We do not validate any of the other data in the launch intent.

Prior to this CL, by customizing the launch intent a third party APK was able
to launch a WebAPK with an arbirtrary name and icon on the splash screen.
This CL reduces the amount of data we extract from the WebappLauncherActivity
launch intent to the minimum possible. We still read from the launch intent the
following data:
- WebAPK package name
- URL to navigate the WebAPK to. We cannot use the WebAPK's start URL because
the WebAPK can be launched at any URL within the WebAPK scope via deep linking.
We do verify that the URL in the intent is within the WebAPK scope.
- The reason the WebAPK got launched (e.g. launched from a notification). This
is used for UMA only.

BUG=651640
==========

[pkotwicz, 2016-10-12 05:01:52]

Patchset #2 (id:40001) has been deleted

[pkotwicz, 2016-10-12 05:05:47]

Dominick, can you please take another look?

I thought the guideline was to wrap the CL description at 80 characters. (I was
told once during a code review over a year ago so I don't have a source for
this). I wrote a Chrome extension which reflows the CL description over the
weekend so now I don't have to do it manually anymore.

https://codereview.chromium.org/2409483002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java
(right):

https://codereview.chromium.org/2409483002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:83:
if (isValidWebApk || isValidMacForUrl(webappUrl, webappMac)
I don't understand. If the WebAPK is valid, this if() statement is also run. The
single line if on line 76 is deceiving

[dominickn, 2016-10-12 05:26:48]

lgtm

80 characters is fine - I think I had 74 in my head because it's the number
cited when you commit (or 72 or something)

https://codereview.chromium.org/2409483002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java
(right):

https://codereview.chromium.org/2409483002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:83:
if (isValidWebApk || isValidMacForUrl(webappUrl, webappMac)
I wrote out what I was suggesting, and it actually doesn't end up that much
cleaner (logged here for completeness). Feel free to ignore.

        if (isValidWebApk) {
            // {@link #isValidWebApk} checks whether the start URL sent in the
intent is in the
            // scope of a WebAPK but it does not check that the intent was sent
from Chrome. Unlike
            // non-WebAPK web apps, WebAPK ids are predictable. A malicious
actor may send an intent
            // with a valid start URL and arbitrary other data. Only use the
start URL, the package
            // name and the ShortcutSource from the launch intent and extract
the remaining data
            // from the <meta-data> in the WebAPK's Android manifest.
            webappInfo = WebApkMetaDataUtils.extractWebappInfoFromWebApk(
                    webApkPackageName, webappUrl, webappInfo.source());

            if (webappInfo == null) return;
        } else if (!isValidMacForUrl(webappUrl,
                           IntentUtils.safeGetStringExtra(intent,
ShortcutHelper.EXTRA_MAC))
                && !wasIntentFromChrome(intent)) {
            Log.e(TAG, "Shortcut (%s) opened in Chrome.", webappUrl);

            // The shortcut data doesn't match the current encoding.  Change the
intent action
            // launch the URL with a VIEW Intent in the regular browser.
            launchIntent = new Intent(Intent.ACTION_VIEW, Uri.parse(webappUrl));
            launchIntent.setClassName(getPackageName(),
ChromeLauncherActivity.class.getName());
           
launchIntent.putExtra(ShortcutHelper.REUSE_URL_MATCHING_TAB_ELSE_NEW_TAB, true);
            launchIntent.putExtra(ShortcutHelper.EXTRA_SOURCE, webappSource);
            launchIntent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK
                    | ApiCompatibilityUtils.getActivityNewDocumentFlag());

            startActivity(launchIntent);
            ApiCompatibilityUtils.finishAndRemoveTask(this);
            return;
        }

        LaunchMetrics.recordHomeScreenLaunchIntoStandaloneActivity(webappUrl,
webappSource);
        launchIntent = createWebappLaunchIntent(webappInfo, webappSource,
isValidWebApk);

        startActivity(launchIntent);
        ApiCompatibilityUtils.finishAndRemoveTask(this);
    }

[pkotwicz, 2016-10-12 18:52:57]

https://codereview.chromium.org/2409483002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java
(right):

https://codereview.chromium.org/2409483002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:83:
if (isValidWebApk || isValidMacForUrl(webappUrl, webappMac)
Ok, I see your suggestion now. I agree that it is not a lot cleaner.

[pkotwicz, 2016-10-12 18:53:02]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-10-12 18:53:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org
Link to the patchset: https://codereview.chromium.org/2409483002/#ps80001
(title: "Merge branch 'startup_crash0' into security")

[commit-bot: I haz the power, 2016-10-12 18:53:38]

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-10-12 19:05:15]

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

[commit-bot: I haz the power, 2016-10-12 19:05:16]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pkotwicz, 2016-10-12 20:58:51]

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

[pkotwicz, 2016-10-12 20:58:52]

dfalcantara@ for WebApkMetaDataUtils.java

[gone, 2016-10-13 18:07:31]

WebApkMetaDataUtils.java lgtm

https://chromiumcodereview.appspot.com/2409483002/diff/80001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java
(right):

https://chromiumcodereview.appspot.com/2409483002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java:59:
Resources resources;
Does it make sense to just combine these two try / catch blocks?  This function
returns null if either of them fails, so you'd be saving work.

[pkotwicz, 2016-10-13 18:12:19]

> Does it make sense to just combine these two try / catch blocks?  This
function
> returns null if either of them fails, so you'd be saving work.

I actually like having two try / catch blocks. It makes it obvious which code
can throw an exception

[pkotwicz, 2016-10-13 18:12:25]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-10-13 18:13:05]

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-10-13 18:20:34]

Description was changed from

==========
Read the bare minimum of data from the WebAPK launch intent.

The launch intent for WebappLauncherActivity may be sent by any app, not
necessarily Chrome or a WebAPK. For WebAPKs (unlike non-WebAPK web apps) the id
is predictable. For WebAPKs we verify that the package name in the intent
matches an installed WebAPK and that the start URL falls within the WebAPK's
scope. We do not validate any of the other data in the launch intent.

Prior to this CL, by customizing the launch intent a third party APK was able
to launch a WebAPK with an arbirtrary name and icon on the splash screen.
This CL reduces the amount of data we extract from the WebappLauncherActivity
launch intent to the minimum possible. We still read from the launch intent the
following data:
- WebAPK package name
- URL to navigate the WebAPK to. We cannot use the WebAPK's start URL because
the WebAPK can be launched at any URL within the WebAPK scope via deep linking.
We do verify that the URL in the intent is within the WebAPK scope.
- The reason the WebAPK got launched (e.g. launched from a notification). This
is used for UMA only.

BUG=651640
==========

to

==========
Read the bare minimum of data from the WebAPK launch intent.

The launch intent for WebappLauncherActivity may be sent by any app, not
necessarily Chrome or a WebAPK. For WebAPKs (unlike non-WebAPK web apps) the id
is predictable. For WebAPKs we verify that the package name in the intent
matches an installed WebAPK and that the start URL falls within the WebAPK's
scope. We do not validate any of the other data in the launch intent.

Prior to this CL, by customizing the launch intent a third party APK was able
to launch a WebAPK with an arbirtrary name and icon on the splash screen.
This CL reduces the amount of data we extract from the WebappLauncherActivity
launch intent to the minimum possible. We still read from the launch intent the
following data:
- WebAPK package name
- URL to navigate the WebAPK to. We cannot use the WebAPK's start URL because
the WebAPK can be launched at any URL within the WebAPK scope via deep linking.
We do verify that the URL in the intent is within the WebAPK scope.
- The reason the WebAPK got launched (e.g. launched from a notification). This
is used for UMA only.

BUG=651640
==========

[commit-bot: I haz the power, 2016-10-13 18:20:35]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-10-13 18:23:04]

Description was changed from

==========
Read the bare minimum of data from the WebAPK launch intent.

The launch intent for WebappLauncherActivity may be sent by any app, not
necessarily Chrome or a WebAPK. For WebAPKs (unlike non-WebAPK web apps) the id
is predictable. For WebAPKs we verify that the package name in the intent
matches an installed WebAPK and that the start URL falls within the WebAPK's
scope. We do not validate any of the other data in the launch intent.

Prior to this CL, by customizing the launch intent a third party APK was able
to launch a WebAPK with an arbirtrary name and icon on the splash screen.
This CL reduces the amount of data we extract from the WebappLauncherActivity
launch intent to the minimum possible. We still read from the launch intent the
following data:
- WebAPK package name
- URL to navigate the WebAPK to. We cannot use the WebAPK's start URL because
the WebAPK can be launched at any URL within the WebAPK scope via deep linking.
We do verify that the URL in the intent is within the WebAPK scope.
- The reason the WebAPK got launched (e.g. launched from a notification). This
is used for UMA only.

BUG=651640
==========

to

==========
Read the bare minimum of data from the WebAPK launch intent.

The launch intent for WebappLauncherActivity may be sent by any app, not
necessarily Chrome or a WebAPK. For WebAPKs (unlike non-WebAPK web apps) the id
is predictable. For WebAPKs we verify that the package name in the intent
matches an installed WebAPK and that the start URL falls within the WebAPK's
scope. We do not validate any of the other data in the launch intent.

Prior to this CL, by customizing the launch intent a third party APK was able
to launch a WebAPK with an arbirtrary name and icon on the splash screen.
This CL reduces the amount of data we extract from the WebappLauncherActivity
launch intent to the minimum possible. We still read from the launch intent the
following data:
- WebAPK package name
- URL to navigate the WebAPK to. We cannot use the WebAPK's start URL because
the WebAPK can be launched at any URL within the WebAPK scope via deep linking.
We do verify that the URL in the intent is within the WebAPK scope.
- The reason the WebAPK got launched (e.g. launched from a notification). This
is used for UMA only.

BUG=651640

Committed: https://crrev.com/0b7c2979c1f64fbfb6ce5967a1424890b86579f1
Cr-Commit-Position: refs/heads/master@{#425093}
==========

[commit-bot: I haz the power, 2016-10-13 18:23:06]

Patchset 3 (id:??) landed as
https://crrev.com/0b7c2979c1f64fbfb6ce5967a1424890b86579f1
Cr-Commit-Position: refs/heads/master@{#425093}