Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(11)

Issue 1996773002: Sanitize https:// URLs before sending them to PAC scripts. (Closed)

Created:
2 years, 5 months ago by eroman
Modified:
2 years, 5 months ago
Reviewers:
mmenke
CC:
chromium-reviews, cbentzel+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Sanitize https:// URLs before sending them to PAC scripts. This additionally strips the path and query components for https:// URL (embedded identity and reference fragment were already being stripped). For debugging purposes this behavior can be disabled with the command line flag --unsafe-pac-url. BUG=593759 R=mmenke@chromium.org Committed: https://chromium.googlesource.com/chromium/src/+/81357b39c643fc746517fd6ce5cb2076b7ddc3f4

Patch Set 1 #

Patch Set 2 : #

Total comments: 9

Patch Set 3 : Addressed Matt's feedback #

Total comments: 12

Patch Set 4 : Address Matt's comments #

Patch Set 5 : another ftp test with path #

Unified diffs Side-by-side diffs Delta from patch set Stats (+285 lines, -4 lines) Patch
M chrome/browser/net/proxy_service_factory.cc View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 chunk +5 lines, -0 lines 0 comments Download
M net/proxy/proxy_service.h View 1 2 3 4 chunks +27 lines, -0 lines 0 comments Download
M net/proxy/proxy_service.cc View 1 2 3 3 chunks +27 lines, -4 lines 0 comments Download
M net/proxy/proxy_service_unittest.cc View 1 2 3 4 1 chunk +220 lines, -0 lines 0 comments Download

Messages

Total messages: 21 (8 generated)
eroman
2 years, 5 months ago (2016-05-19 22:04:24 UTC) #2
mmenke
Quick comments. Haven't reviewed the tests, but I'm paranoid enough that I think that one ...
2 years, 5 months ago (2016-05-19 22:33:24 UTC) #3
eroman
Thanks for the feedback Matt! > but I'm paranoid enough that I think that one ...
2 years, 5 months ago (2016-05-19 23:26:05 UTC) #4
mmenke
On 2016/05/19 23:26:05, eroman wrote: > Thanks for the feedback Matt! > > > but ...
2 years, 5 months ago (2016-05-19 23:29:48 UTC) #5
mmenke
On 2016/05/19 23:29:48, mmenke wrote: > On 2016/05/19 23:26:05, eroman wrote: > > Thanks for ...
2 years, 5 months ago (2016-05-19 23:30:17 UTC) #6
mmenke
LGTM. A couple minor suggestions. https://codereview.chromium.org/1996773002/diff/40001/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/1996773002/diff/40001/net/proxy/proxy_service.cc#newcode1566 net/proxy/proxy_service.cc:1566: DCHECK(url.is_valid()); optional: Suggest a ...
2 years, 5 months ago (2016-05-20 17:40:23 UTC) #7
eroman
https://codereview.chromium.org/1996773002/diff/40001/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/1996773002/diff/40001/net/proxy/proxy_service.cc#newcode1566 net/proxy/proxy_service.cc:1566: DCHECK(url.is_valid()); On 2016/05/20 17:40:23, mmenke wrote: > optional: Suggest ...
2 years, 5 months ago (2016-05-20 21:23:53 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996773002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996773002/80001
2 years, 5 months ago (2016-05-20 21:25:11 UTC) #11
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/233285)
2 years, 5 months ago (2016-05-21 00:23:49 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996773002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996773002/80001
2 years, 5 months ago (2016-05-21 19:47:23 UTC) #15
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/233448)
2 years, 5 months ago (2016-05-21 22:18:41 UTC) #17
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/81357b39c643fc746517fd6ce5cb2076b7ddc3f4 Cr-Commit-Position: refs/heads/master@{#395266}
2 years, 5 months ago (2016-05-21 23:05:10 UTC) #19
eroman
2 years, 5 months ago (2016-05-21 23:05:55 UTC) #21
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as
81357b39c643fc746517fd6ce5cb2076b7ddc3f4 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698