Use correct origin when prompting for proxy authentication.

Use correct origin when prompting for proxy authentication.

Since M49, Chrome has been prompting for proxy authentication
credentials using the target origin instead of the origin of the proxy
server. Even if the proxy origin was displayed correctly, a mischievous
network operator could still spoof the proxy server origin. To mitigate
these problems, this CL:

* Fixes the origin used in the proxy authentication login prompt to use
  the origin of the proxy server.

* Indicate if the proxy server connection is insecure.

* Always throw up an interstitial and clear the omnibox when showing a
  proxy auth prompt.

* Use the correct origin when saving proxy authentication credentials.

BUG=613626, 620737
Committed: https://crrev.com/098c009df7a4ddc5c23d4d3c9dccf5eff1f24c98
Cr-Commit-Position: refs/heads/master@{#400247}

[asanka, 2016-06-14 18:29:42]

asanka@chromium.org changed reviewers:
+ meacer@chromium.org, vabr@chromium.org

[asanka, 2016-06-14 18:50:48]

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_handler.cc (left):

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_handler.cc:86: origin = std::string("http://") +
origin;
vabr: This logic doesn't distinguish between http and https proxies. I.e. a
password stored for an https proxy will be automatically provided to a http
proxy. In the new logic, we are passing up the origin of the proxy server up to
this layer. So we are using the correct origin here when populating
dialog_form.origin.

This will cause existing stored password entries for https proxies to become
orphaned and users will be prompted again after this change. I think this is OK
given that we really don't want to share passwords between secure and insecure
proxies. WDYT?

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_handler.cc:93: dialog_form.origin =
GURL(request_url.scheme() + "://" + host_and_port);
vabr: Note that host_and_port will always contain a port even if the port is
standard. Hence dialog_form.origin = GURL("http://example.com:80") when
authenticating against "http://example.com".

After this CL, dialog_form.origin would be GURL("http://example.com"). Since
GURL canonicalizes the URL there should be no observable change in behavior
before and after this change.

[asanka, 2016-06-14 18:51:26]

asanka@chromium.org changed reviewers:
+ brettw@chromium.org

[asanka, 2016-06-14 18:52:08]

Description was changed from

==========
Use correct origin when prompting for proxy authentication.

Since M49, Chrome has been prompting for proxy authentication
credentials using the target origin instead of the origin of the proxy
server. Even if the proxy origin was displayed correctly, a mischievous
network operator could still spoof the proxy server origin. To mitigate
these problems, this CL:

* Fixes the origin used in the proxy authentication login prompt to use
  the origin of the proxy server.

* Indicate if the proxy server connection is insecure.

* Always throw up an interstitial when showing a proxy auth prompt.

BUG=613626
==========

to

==========
Use correct origin when prompting for proxy authentication.

Since M49, Chrome has been prompting for proxy authentication
credentials using the target origin instead of the origin of the proxy
server. Even if the proxy origin was displayed correctly, a mischievous
network operator could still spoof the proxy server origin. To mitigate
these problems, this CL:

* Fixes the origin used in the proxy authentication login prompt to use
  the origin of the proxy server.

* Indicate if the proxy server connection is insecure.

* Always throw up an interstitial and clear the omnibox when showing a
  proxy auth prompt.

BUG=613626
==========

[asanka, 2016-06-14 18:58:38]

+brettw: for components/login_dialog_strings.grdp and url/origin.h

meacer: Everything. Also, I need to get a second opinion on the use of the
interstitial. I'm doing it in part to prevent the URL and lock icon that might
already be displayed in the omnibox from being abused to mislead users.

vabr: login_handler{.cc, _unittest.cc}

   I've noted the places where I'd like you to scrutinize. I'd like to make sure
we don't orphan any stored passwords unnecessarily.

[vabr (Chromium), 2016-06-15 08:50:06]

LGTM, thanks!
Vaclav

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_handler.cc (left):

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_handler.cc:86: origin = std::string("http://") +
origin;
On 2016/06/14 18:50:48, asanka wrote:
> vabr: This logic doesn't distinguish between http and https proxies. I.e. a
> password stored for an https proxy will be automatically provided to a http
> proxy. In the new logic, we are passing up the origin of the proxy server up
to
> this layer. So we are using the correct origin here when populating
> dialog_form.origin.
> 
> This will cause existing stored password entries for https proxies to become
> orphaned and users will be prompted again after this change. I think this is
OK
> given that we really don't want to share passwords between secure and insecure
> proxies. WDYT?

Thanks for the heads-up. I agree with your arguments. If we see a surge of bug
reports we will be able to direct users to chrome://passwords/settings to look
up the old passwords.

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_handler.cc:93: dialog_form.origin =
GURL(request_url.scheme() + "://" + host_and_port);
On 2016/06/14 18:50:48, asanka wrote:
> vabr: Note that host_and_port will always contain a port even if the port is
> standard. Hence dialog_form.origin = GURL("http://example.com:80") when
> authenticating against "http://example.com".
> 
> After this CL, dialog_form.origin would be GURL("http://example.com"). Since
> GURL canonicalizes the URL there should be no observable change in behavior
> before and after this change.

Acknowledged.

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_handler.cc (right):

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_handler.cc:494: // TODO(asanka): The string should
be different for proxies and servers.
nit: Could you please make this a TODO(crbug.com/XXX) where XXX is the number of
the bug having the details for what is the issue and plans to fix it?

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_handler.h (right):

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_handler.h:227: // The PasswordForm sent to the
PasswordManager. This is so we can refer to
nit: Seems like these two lines could have stayed as they were? Or at least
please reindent to use as much as possible of the available line character
limit.

[meacer, 2016-06-16 01:12:15]

Sorry for the delay. Could you also update the comment in
login_interstitial_delegate.h to mention proxies?

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_handler.cc (right):

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_handler.cc:463: } else if (auth_info.challenger !=
url::Origin(request_url)) {
Why not use !IsSameOrigin here? It sounds a bit heavy handed to add an overload
just for a single call.

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_handler.cc:587:
(parent_contents->ShowingInterstitialPage() || auth_info->is_proxy ||
nit: Can you update the comment above and add the proxy case?

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_handler.h (right):

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_handler.h:205: // directly in this callback. In
both cases, the response will be sent to
directly -> created directly

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_handler_unittest.cc (right):

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_handler_unittest.cc:23: struct TestCase {
const?

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_handler_unittest.cc:112: TEST(LoginHandlerTest,
Outputs) {
nit: rename to DialogStringsAndRealm?

[asanka, 2016-06-16 16:25:47]

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_handler.cc (right):

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_handler.cc:463: } else if (auth_info.challenger !=
url::Origin(request_url)) {
On 2016/06/16 at 01:12:14, Mustafa Emre Acer wrote:
> Why not use !IsSameOrigin here? It sounds a bit heavy handed to add an
overload just for a single call.

The addition of the operator follows the style guidelines to overload related
operators, which is why I didn't think much about it. However I'll remove it
from the CL since IsSameOriginWith() is clearer in its intent.

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_handler.cc:494: // TODO(asanka): The string should
be different for proxies and servers.
On 2016/06/15 at 08:50:06, vabr (Chromium) wrote:
> nit: Could you please make this a TODO(crbug.com/XXX) where XXX is the number
of the bug having the details for what is the issue and plans to fix it?

Added one. The plan is to pretty much fix it immediately. The string change is
not part of this CL because we may need to merge to the release branches and
such changes can't contain string changes.

I also removed the no-op change I was making to the .grdp file since that'll
probably be disconcerting during merge consideration.

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_handler.cc:587:
(parent_contents->ShowingInterstitialPage() || auth_info->is_proxy ||
On 2016/06/16 at 01:12:14, Mustafa Emre Acer wrote:
> nit: Can you update the comment above and add the proxy case?

Done. Thanks, missed the comment in the first pass.

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_handler.h (right):

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_handler.h:205: // directly in this callback. In
both cases, the response will be sent to
On 2016/06/16 at 01:12:15, Mustafa Emre Acer wrote:
> directly -> created directly

Done.

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_handler.h:227: // The PasswordForm sent to the
PasswordManager. This is so we can refer to
On 2016/06/15 at 08:50:06, vabr (Chromium) wrote:
> nit: Seems like these two lines could have stayed as they were? Or at least
please reindent to use as much as possible of the available line character
limit.

Reformatted.

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_handler_unittest.cc (right):

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_handler_unittest.cc:23: struct TestCase {
On 2016/06/16 at 01:12:15, Mustafa Emre Acer wrote:
> const?

Done.

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_handler_unittest.cc:112: TEST(LoginHandlerTest,
Outputs) {
On 2016/06/16 at 01:12:15, Mustafa Emre Acer wrote:
> nit: rename to DialogStringsAndRealm?

Done.

[asanka, 2016-06-16 16:26:21]

asanka@chromium.org changed reviewers:
- brettw@chromium.org

[asanka, 2016-06-16 16:26:22]

Also -brettw since both changes owned by them are no longer part of the CL.

[asanka, 2016-06-16 16:28:23]

Description was changed from

==========
Use correct origin when prompting for proxy authentication.

Since M49, Chrome has been prompting for proxy authentication
credentials using the target origin instead of the origin of the proxy
server. Even if the proxy origin was displayed correctly, a mischievous
network operator could still spoof the proxy server origin. To mitigate
these problems, this CL:

* Fixes the origin used in the proxy authentication login prompt to use
  the origin of the proxy server.

* Indicate if the proxy server connection is insecure.

* Always throw up an interstitial and clear the omnibox when showing a
  proxy auth prompt.

BUG=613626
==========

to

==========
Use correct origin when prompting for proxy authentication.

Since M49, Chrome has been prompting for proxy authentication
credentials using the target origin instead of the origin of the proxy
server. Even if the proxy origin was displayed correctly, a mischievous
network operator could still spoof the proxy server origin. To mitigate
these problems, this CL:

* Fixes the origin used in the proxy authentication login prompt to use
  the origin of the proxy server.

* Indicate if the proxy server connection is insecure.

* Always throw up an interstitial and clear the omnibox when showing a
  proxy auth prompt.

BUG=613626, 620737
==========

[vabr (Chromium), 2016-06-16 16:43:21]

Still LGTM, thanks!
Vaclav

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_handler.cc (right):

https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_handler.cc:494: // TODO(asanka): The string should
be different for proxies and servers.
On 2016/06/16 16:25:47, asanka wrote:
> On 2016/06/15 at 08:50:06, vabr (Chromium) wrote:
> > nit: Could you please make this a TODO(crbug.com/XXX) where XXX is the
number
> of the bug having the details for what is the issue and plans to fix it?
> 
> Added one. The plan is to pretty much fix it immediately. The string change is
> not part of this CL because we may need to merge to the release branches and
> such changes can't contain string changes.
> 
> I also removed the no-op change I was making to the .grdp file since that'll
> probably be disconcerting during merge consideration.

Acknowledged.

[meacer, 2016-06-16 17:26:58]

Thanks! LGTM.

[asanka, 2016-06-16 17:51:26]

Thanks everyone!

[asanka, 2016-06-16 17:52:23]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-16 17:53:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2067933002/20001

[asanka, 2016-06-16 17:58:13]

Description was changed from

==========
Use correct origin when prompting for proxy authentication.

Since M49, Chrome has been prompting for proxy authentication
credentials using the target origin instead of the origin of the proxy
server. Even if the proxy origin was displayed correctly, a mischievous
network operator could still spoof the proxy server origin. To mitigate
these problems, this CL:

* Fixes the origin used in the proxy authentication login prompt to use
  the origin of the proxy server.

* Indicate if the proxy server connection is insecure.

* Always throw up an interstitial and clear the omnibox when showing a
  proxy auth prompt.

BUG=613626, 620737
==========

to

==========
Use correct origin when prompting for proxy authentication.

Since M49, Chrome has been prompting for proxy authentication
credentials using the target origin instead of the origin of the proxy
server. Even if the proxy origin was displayed correctly, a mischievous
network operator could still spoof the proxy server origin. To mitigate
these problems, this CL:

* Fixes the origin used in the proxy authentication login prompt to use
  the origin of the proxy server.

* Indicate if the proxy server connection is insecure.

* Always throw up an interstitial and clear the omnibox when showing a
  proxy auth prompt.

* Use the correct origin when saving proxy authentication credentials.

BUG=613626, 620737
==========

[commit-bot: I haz the power, 2016-06-16 18:10:54]

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

[commit-bot: I haz the power, 2016-06-16 18:10:56]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[asanka, 2016-06-16 18:36:31]

asanka@chromium.org changed reviewers:
+ jam@chromium.org

[asanka, 2016-06-16 18:38:07]

+jam: for content/shell/browser/shell_login_dialog.cc

   The file was updated to derive a safe display string for the 'challenger'
field for the HTTP auth prompt.

[jam, 2016-06-16 19:58:07]

On 2016/06/16 18:38:07, asanka wrote:
> +jam: for content/shell/browser/shell_login_dialog.cc
> 
>    The file was updated to derive a safe display string for the 'challenger'
> field for the HTTP auth prompt.

lgtm

[asanka, 2016-06-16 20:11:56]

On 2016/06/16 at 19:58:07, jam wrote:
> On 2016/06/16 18:38:07, asanka wrote:
> > +jam: for content/shell/browser/shell_login_dialog.cc
> > 
> >    The file was updated to derive a safe display string for the 'challenger'
> > field for the HTTP auth prompt.
> 
> lgtm

Thanks!

[asanka, 2016-06-16 20:12:01]

The CQ bit was checked by asanka@chromium.org

[asanka, 2016-06-16 20:12:01]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2067933002/#ps40001
(title: "Fix content_shell build.")

[commit-bot: I haz the power, 2016-06-16 20:12:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2067933002/40001

[commit-bot: I haz the power, 2016-06-16 20:18:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-16 20:20:16]

Description was changed from

==========
Use correct origin when prompting for proxy authentication.

Since M49, Chrome has been prompting for proxy authentication
credentials using the target origin instead of the origin of the proxy
server. Even if the proxy origin was displayed correctly, a mischievous
network operator could still spoof the proxy server origin. To mitigate
these problems, this CL:

* Fixes the origin used in the proxy authentication login prompt to use
  the origin of the proxy server.

* Indicate if the proxy server connection is insecure.

* Always throw up an interstitial and clear the omnibox when showing a
  proxy auth prompt.

* Use the correct origin when saving proxy authentication credentials.

BUG=613626, 620737
==========

to

==========
Use correct origin when prompting for proxy authentication.

Since M49, Chrome has been prompting for proxy authentication
credentials using the target origin instead of the origin of the proxy
server. Even if the proxy origin was displayed correctly, a mischievous
network operator could still spoof the proxy server origin. To mitigate
these problems, this CL:

* Fixes the origin used in the proxy authentication login prompt to use
  the origin of the proxy server.

* Indicate if the proxy server connection is insecure.

* Always throw up an interstitial and clear the omnibox when showing a
  proxy auth prompt.

* Use the correct origin when saving proxy authentication credentials.

BUG=613626, 620737

Committed: https://crrev.com/098c009df7a4ddc5c23d4d3c9dccf5eff1f24c98
Cr-Commit-Position: refs/heads/master@{#400247}
==========

[commit-bot: I haz the power, 2016-06-16 20:20:17]

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