Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(43)

Issue 2067933002: Use correct origin when prompting for proxy authentication. (Closed)

Created:
4 years, 6 months ago by asanka
Modified:
4 years, 6 months ago
Reviewers:
vabr (Chromium), jam, meacer
CC:
achuith+watch_chromium.org, cbentzel+watch_chromium.org, dzhioev+watch_chromium.org, Paweł Hajdan Jr., vabr+watchlistlogin_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Total comments: 19

Patch Set 2 : Address comments. Remove no-op string change and origin.h change. #

Patch Set 3 : Fix content_shell build. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+359 lines, -186 lines) Patch
M chrome/browser/ui/login/login_handler.h View 1 5 chunks +45 lines, -6 lines 0 comments Download
M chrome/browser/ui/login/login_handler.cc View 1 6 chunks +155 lines, -130 lines 0 comments Download
M chrome/browser/ui/login/login_handler_unittest.cc View 1 1 chunk +127 lines, -29 lines 0 comments Download
M content/shell/browser/shell_login_dialog.cc View 1 2 2 chunks +5 lines, -3 lines 0 comments Download
M net/base/auth.h View 2 chunks +2 lines, -2 lines 0 comments Download
M net/base/auth.cc View 1 chunk +1 line, -2 lines 0 comments Download
M net/http/http_auth_controller.cc View 1 chunk +1 line, -1 line 0 comments Download
M net/http/http_network_transaction_unittest.cc View 1 2 11 chunks +22 lines, -12 lines 0 comments Download
M net/url_request/url_request_ftp_job.cc View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 29 (12 generated)
asanka
https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/login_handler.cc File chrome/browser/ui/login/login_handler.cc (left): https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/login_handler.cc#oldcode86 chrome/browser/ui/login/login_handler.cc:86: origin = std::string("http://") + origin; vabr: This logic doesn't ...
4 years, 6 months ago (2016-06-14 18:50:48 UTC) #2
asanka
+brettw: for components/login_dialog_strings.grdp and url/origin.h meacer: Everything. Also, I need to get a second opinion ...
4 years, 6 months ago (2016-06-14 18:58:38 UTC) #5
vabr (Chromium)
LGTM, thanks! Vaclav https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/login_handler.cc File chrome/browser/ui/login/login_handler.cc (left): https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/login_handler.cc#oldcode86 chrome/browser/ui/login/login_handler.cc:86: origin = std::string("http://") + origin; On ...
4 years, 6 months ago (2016-06-15 08:50:06 UTC) #6
meacer
Sorry for the delay. Could you also update the comment in login_interstitial_delegate.h to mention proxies? ...
4 years, 6 months ago (2016-06-16 01:12:15 UTC) #7
asanka
https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/login_handler.cc File chrome/browser/ui/login/login_handler.cc (right): https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/login_handler.cc#newcode463 chrome/browser/ui/login/login_handler.cc:463: } else if (auth_info.challenger != url::Origin(request_url)) { On 2016/06/16 ...
4 years, 6 months ago (2016-06-16 16:25:47 UTC) #8
asanka
Also -brettw since both changes owned by them are no longer part of the CL.
4 years, 6 months ago (2016-06-16 16:26:22 UTC) #10
vabr (Chromium)
Still LGTM, thanks! Vaclav https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/login_handler.cc File chrome/browser/ui/login/login_handler.cc (right): https://codereview.chromium.org/2067933002/diff/1/chrome/browser/ui/login/login_handler.cc#newcode494 chrome/browser/ui/login/login_handler.cc:494: // TODO(asanka): The string should ...
4 years, 6 months ago (2016-06-16 16:43:21 UTC) #12
meacer
Thanks! LGTM.
4 years, 6 months ago (2016-06-16 17:26:58 UTC) #13
asanka
Thanks everyone!
4 years, 6 months ago (2016-06-16 17:51:26 UTC) #14
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2067933002/20001
4 years, 6 months ago (2016-06-16 17:53:05 UTC) #16
commit-bot: I haz the power
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_ng/builds/245457)
4 years, 6 months ago (2016-06-16 18:10:56 UTC) #19
asanka
+jam: for content/shell/browser/shell_login_dialog.cc The file was updated to derive a safe display string for the ...
4 years, 6 months ago (2016-06-16 18:38:07 UTC) #21
jam
On 2016/06/16 18:38:07, asanka wrote: > +jam: for content/shell/browser/shell_login_dialog.cc > > The file was updated ...
4 years, 6 months ago (2016-06-16 19:58:07 UTC) #22
asanka
On 2016/06/16 at 19:58:07, jam wrote: > On 2016/06/16 18:38:07, asanka wrote: > > +jam: ...
4 years, 6 months ago (2016-06-16 20:11:56 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2067933002/40001
4 years, 6 months ago (2016-06-16 20:12:59 UTC) #26
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 6 months ago (2016-06-16 20:18:58 UTC) #27
commit-bot: I haz the power
4 years, 6 months ago (2016-06-16 20:20:17 UTC) #29
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/098c009df7a4ddc5c23d4d3c9dccf5eff1f24c98
Cr-Commit-Position: refs/heads/master@{#400247}

Powered by Google App Engine
This is Rietveld 408576698