Ensure/document that the first parameter to the native messaging host is the origin

Ensure/document that the first parameter to the native messaging host is the origin

This reflects the current implementation, and allows native messaging
hosts to verify their caller when they have authorized multiple
(extension) origins to invoke the native messaging host.

On Windows, the first argument was --parent-window=integer,
but to make the use of the origin parameter easier, the origin parameter
is first, just like other platforms (Linux/OS X).

BUG=651496
R=sergeyu@chromium.org

Committed: https://crrev.com/f048ff5342719462f88ff146b8ac01346f320da8
Cr-Commit-Position: refs/heads/master@{#423169}

[robwu, 2016-08-24 07:26:39]

Hi Sergey,

I still had to submit this documentation fix following our conversation on the
11th of May.

[Sergey Ulanov, 2016-08-31 23:53:04]

lgtm

[Sergey Ulanov, 2016-09-06 18:07:56]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2016-09-06 18:08:25]

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-09-06 18:16:05]

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

[commit-bot: I haz the power, 2016-09-06 18:16:07]

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...)

[robwu, 2016-09-07 04:52:50]

rob@robwu.nl changed reviewers:
+ rdevlin.cronin@chromium.org

[robwu, 2016-09-07 04:52:51]

OWNER stamp please :)

[Devlin, 2016-09-07 15:01:34]

Do we already test this?  I didn't immediately see anything in the native
messaging tests from a quick skim.

[Sergey Ulanov, 2016-09-07 19:47:39]

On 2016/09/07 15:01:34, Devlin wrote:
> Do we already test this?  I didn't immediately see anything in the native
> messaging tests from a quick skim.

The test verifies that the is parameter specified here:
https://codesearch.chromium.org/chromium/src/chrome/test/data/native_messagin...
Currently it just verifies that the parameter is there, but not that the value
is correct.

[Devlin, 2016-09-07 20:08:50]

On 2016/09/07 19:47:39, Sergey Ulanov wrote:
> On 2016/09/07 15:01:34, Devlin wrote:
> > Do we already test this?  I didn't immediately see anything in the native
> > messaging tests from a quick skim.
> 
> The test verifies that the is parameter specified here:
>
https://codesearch.chromium.org/chromium/src/chrome/test/data/native_messagin...
> Currently it just verifies that the parameter is there, but not that the value
> is correct.

Seems like an easy enough fix, so I'd like to do that in this CL as well.

[robwu, 2016-09-24 21:13:41]

On 2016/09/07 20:08:50, Devlin (OOO until sep 26) wrote:
> On 2016/09/07 19:47:39, Sergey Ulanov wrote:
> > On 2016/09/07 15:01:34, Devlin wrote:
> > > Do we already test this?  I didn't immediately see anything in the native
> > > messaging tests from a quick skim.
> > 
> > The test verifies that the is parameter specified here:
> >
>
https://codesearch.chromium.org/chromium/src/chrome/test/data/native_messagin...
> > Currently it just verifies that the parameter is there, but not that the
value
> > is correct.
> 
> Seems like an easy enough fix, so I'd like to do that in this CL as well.

The value is also verified:
https://chromium.googlesource.com/chromium/src/+/0906cac26ecd6cfd5c98ee5ab296...
However a later change weakened the test to the extent that it's not guaranteed
that the first argument is the origin:
https://chromium.googlesource.com/chromium/src/+/4c69465afd402fc6db73ac8fcfb5...

I have updated the CL to make a stronger assertion that the first parameter is
the origin.

[robwu, 2016-09-24 21:14:28]

The CQ bit was checked by rob@robwu.nl to run a CQ dry run

[commit-bot: I haz the power, 2016-09-24 21:14:35]

Dry run: 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-09-24 22:26:25]

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

[commit-bot: I haz the power, 2016-09-24 22:26:26]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[robwu, 2016-09-25 11:03:12]

The CQ bit was checked by rob@robwu.nl to run a CQ dry run

[commit-bot: I haz the power, 2016-09-25 11:03:19]

Dry run: 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-09-25 12:38:23]

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

[commit-bot: I haz the power, 2016-09-25 12:38:24]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Devlin, 2016-09-26 15:53:59]

Awesome, thanks Rob!

lgtm with nits

https://codereview.chromium.org/2271143002/diff/40001/chrome/common/extension...
File chrome/common/extensions/docs/templates/articles/nativeMessaging.html
(right):

https://codereview.chromium.org/2271143002/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/templates/articles/nativeMessaging.html:133:
usually <code>chrome-extension://[extension ID of whitelisted extension]</code>.
nit: s/extension ID of whitelisted extension/ID of the whitelisted extension

https://codereview.chromium.org/2271143002/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/templates/articles/nativeMessaging.html:137: <a
href="#native-messaging-host-manifest">native messaging host manifest</a>.
This sentence reads a little funny.  Maybe

"This allows native messaging hosts to identify the source of the message when
multiple extensions are specified in the <code>allowed_origins</code> key in the
<a href="#native-messaging-host-manifest">host manifest</a>."

WDYT?

[robwu, 2016-09-28 09:03:50]

Description was changed from

==========
Document that the first parameter to the native messaging host is the origin

This reflects the current implementation, and allows native messaging
hosts to verify their caller when they have authorized multiple
(extension) origins to invoke the native messaging host.

R=sergeyu@chromium.org
==========

to

==========
Document that the first parameter to the native messaging host is the origin

This reflects the current implementation, and allows native messaging
hosts to verify their caller when they have authorized multiple
(extension) origins to invoke the native messaging host.

On Windows, the first argument was --parent-window=integer,
but to make the use of the origin parameter easier, the origin parameter
is first, just like other platforms (Linux/OS X).

R=sergeyu@chromium.org
==========

[robwu, 2016-09-28 09:07:09]

Devlin - do note that I did not only change the documentation, but also the
behavior on Windows (as described in the updated commit message).

If you were aware of this and agree, please tick the commit box.

https://codereview.chromium.org/2271143002/diff/40001/chrome/common/extension...
File chrome/common/extensions/docs/templates/articles/nativeMessaging.html
(right):

https://codereview.chromium.org/2271143002/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/templates/articles/nativeMessaging.html:133:
usually <code>chrome-extension://[extension ID of whitelisted extension]</code>.
On 2016/09/26 15:53:59, Devlin (OOO until Thursday) wrote:
> nit: s/extension ID of whitelisted extension/ID of the whitelisted extension

Done.

https://codereview.chromium.org/2271143002/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/templates/articles/nativeMessaging.html:137: <a
href="#native-messaging-host-manifest">native messaging host manifest</a>.
On 2016/09/26 15:53:59, Devlin (OOO until Thursday) wrote:
> This sentence reads a little funny.  Maybe
> 
> "This allows native messaging hosts to identify the source of the message when
> multiple extensions are specified in the <code>allowed_origins</code> key in
the
> <a href="#native-messaging-host-manifest">host manifest</a>."
> 
> WDYT?

SGTM. I used "native messaging host manifest" instead of "host manifest" to
emphasize that it is not manifest.json but the manifest of the NMH.

[Devlin, 2016-09-29 15:14:51]

On 2016/09/28 09:07:09, robwu wrote:
> Devlin - do note that I did not only change the documentation, but also the
> behavior on Windows (as described in the updated commit message).
> 
> If you were aware of this and agree, please tick the commit box.

Thanks for drawing my attention to this; I didn't notice it at first.

Given that a) the order of arguments is currently undocumented and b) this is
providing parity between windows and linux/mac, I think this is fine.  But
let's:
- File an associated bug ("origin argument of native messaging is inconsistent
on different platforms" or similar) for future discussions, if any, and link it
here.
- Send an FYI email to chromium-extensions@chromium.org notifying that this
change is happening and linking to the bug.
- Refrain from merging this back and just let it go through normal release
cycles so that developers have plenty of time to update, if they are relying on
it (even though it's undocumented).

https://codereview.chromium.org/2271143002/diff/60001/chrome/common/extension...
File chrome/common/extensions/docs/templates/articles/nativeMessaging.html
(right):

https://codereview.chromium.org/2271143002/diff/60001/chrome/common/extension...
chrome/common/extensions/docs/templates/articles/nativeMessaging.html:150: On
Windows, the native messaging host gets passed a command line argument with
nit: let's "s/gets/is also" to make it clear that this is in addition to the
origin.

[robwu, 2016-09-29 17:17:24]

Description was changed from

==========
Document that the first parameter to the native messaging host is the origin

This reflects the current implementation, and allows native messaging
hosts to verify their caller when they have authorized multiple
(extension) origins to invoke the native messaging host.

On Windows, the first argument was --parent-window=integer,
but to make the use of the origin parameter easier, the origin parameter
is first, just like other platforms (Linux/OS X).

R=sergeyu@chromium.org
==========

to

==========
Ensure/document that the first parameter to the native messaging host is the
origin

This reflects the current implementation, and allows native messaging
hosts to verify their caller when they have authorized multiple
(extension) origins to invoke the native messaging host.

On Windows, the first argument was --parent-window=integer,
but to make the use of the origin parameter easier, the origin parameter
is first, just like other platforms (Linux/OS X).

BUG=651496
R=sergeyu@chromium.org
==========

[robwu, 2016-09-29 17:38:23]

On 2016/09/29 15:14:51, Devlin (catching up) wrote:
> On 2016/09/28 09:07:09, robwu wrote:
> > Devlin - do note that I did not only change the documentation, but also the
> > behavior on Windows (as described in the updated commit message).
> > 
> > If you were aware of this and agree, please tick the commit box.
> 
> Thanks for drawing my attention to this; I didn't notice it at first.
> 
> Given that a) the order of arguments is currently undocumented and b) this is
> providing parity between windows and linux/mac, I think this is fine.  But
> let's:
> - File an associated bug ("origin argument of native messaging is inconsistent
> on different platforms" or similar) for future discussions, if any, and link
it
> here.
> - Send an FYI email to mailto:chromium-extensions@chromium.org notifying that
this
> change is happening and linking to the bug.
> - Refrain from merging this back and just let it go through normal release
> cycles so that developers have plenty of time to update, if they are relying
on
> it (even though it's undocumented).

Done, see bug in description for the link to the announcement.

https://codereview.chromium.org/2271143002/diff/60001/chrome/common/extension...
File chrome/common/extensions/docs/templates/articles/nativeMessaging.html
(right):

https://codereview.chromium.org/2271143002/diff/60001/chrome/common/extension...
chrome/common/extensions/docs/templates/articles/nativeMessaging.html:150: On
Windows, the native messaging host gets passed a command line argument with
On 2016/09/29 15:14:51, Devlin (catching up) wrote:
> nit: let's "s/gets/is also" to make it clear that this is in addition to the
> origin.

Done. Also added an extra line in the docs to mention the change in order on
Windows.

[robwu, 2016-10-05 14:55:13]

The CQ bit was checked by rob@robwu.nl

[robwu, 2016-10-05 14:55:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2271143002/#ps80001
(title: "Clarify parameters on Windows")

[commit-bot: I haz the power, 2016-10-05 14:55:37]

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-05 15:52:22]

Description was changed from

==========
Ensure/document that the first parameter to the native messaging host is the
origin

This reflects the current implementation, and allows native messaging
hosts to verify their caller when they have authorized multiple
(extension) origins to invoke the native messaging host.

On Windows, the first argument was --parent-window=integer,
but to make the use of the origin parameter easier, the origin parameter
is first, just like other platforms (Linux/OS X).

BUG=651496
R=sergeyu@chromium.org
==========

to

==========
Ensure/document that the first parameter to the native messaging host is the
origin

This reflects the current implementation, and allows native messaging
hosts to verify their caller when they have authorized multiple
(extension) origins to invoke the native messaging host.

On Windows, the first argument was --parent-window=integer,
but to make the use of the origin parameter easier, the origin parameter
is first, just like other platforms (Linux/OS X).

BUG=651496
R=sergeyu@chromium.org
==========

[commit-bot: I haz the power, 2016-10-05 15:52:23]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-05 15:55:33]

Description was changed from

==========
Ensure/document that the first parameter to the native messaging host is the
origin

This reflects the current implementation, and allows native messaging
hosts to verify their caller when they have authorized multiple
(extension) origins to invoke the native messaging host.

On Windows, the first argument was --parent-window=integer,
but to make the use of the origin parameter easier, the origin parameter
is first, just like other platforms (Linux/OS X).

BUG=651496
R=sergeyu@chromium.org
==========

to

==========
Ensure/document that the first parameter to the native messaging host is the
origin

This reflects the current implementation, and allows native messaging
hosts to verify their caller when they have authorized multiple
(extension) origins to invoke the native messaging host.

On Windows, the first argument was --parent-window=integer,
but to make the use of the origin parameter easier, the origin parameter
is first, just like other platforms (Linux/OS X).

BUG=651496
R=sergeyu@chromium.org

Committed: https://crrev.com/f048ff5342719462f88ff146b8ac01346f320da8
Cr-Commit-Position: refs/heads/master@{#423169}
==========

[commit-bot: I haz the power, 2016-10-05 15:55:34]

Patchset 5 (id:??) landed as
https://crrev.com/f048ff5342719462f88ff146b8ac01346f320da8
Cr-Commit-Position: refs/heads/master@{#423169}