Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1996773002:
Sanitize https:// URLs before sending them to PAC scripts. (Closed)
DescriptionSanitize 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 #
Messages
Total messages: 21 (8 generated)
eroman@chromium.org changed reviewers: + mmenke@chromium.org
Quick comments. Haven't reviewed the tests, but I'm paranoid enough that I think that one of the tests that actually uses a PAC script should check that the URLs it gets really are sanitized. https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service... net/proxy/proxy_service.cc:364: } An alternative approach would be to do: if (policy == SanitizeUrlForPacScriptPolicy::SAFE && url.SchemeIsCryptographic()) { return url.GetOrigin(); } GURL::Replacements replacements; ... Nice thing about that approach is it emphasizes this is indeed just an origin. Downside is it de-emphasizes just what the differences are between the two paths. https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service... net/proxy/proxy_service.cc:1074: SanitizeUrlForPacScript(raw_url, sanitize_url_for_pac_script_policy_); Should we only do this when we create the PacRequest? Seems weird to call it SanitizeForPac, and then send it to TryToCompleteSynchronously, which doesn't actually run a PAC script. https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service.h File net/proxy/proxy_service.h (right): https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service... net/proxy/proxy_service.h:69: NET_EXPORT GURL SanitizeUrlForPacScript(const GURL& url, Do you need to include gurl.h when returning a GURL? https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service... net/proxy/proxy_service.h:70: SanitizeUrlForPacScriptPolicy policy); Not a big fan of bonus enums and methods hanging out in a class's header file. Can we just move these into ProxyService? https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service... net/proxy/proxy_service.h:494: SanitizeUrlForPacScriptPolicy::SAFE; nit: Seems weird to me to mix inline simple initialization with initialization in the constructor. I'm fine with either pattern, just don't like mixing-and-matching.
Thanks for the feedback Matt! > but I'm paranoid enough that I think that one of the tests > that actually uses a PAC script should check that > the URLs it gets really are sanitized. A full functional test for proxy resolving is a pretty involved, and we don't really have an existing framework for that. There are several more layers of goop around proxy resolving if we wanted to measure final delivery to a PAC script. - MultiThreadedProxyResolver wrapping a platform implementation Or - TracingProxyResolverV8 wrapping v8 proxy resolver Or - Mojo proxy resolver wrapping either an in-process implementation or out-of-process forwarder. Because we instantiate all these different modes (chrome side and embedder side) the biggest bang for your buck is testing the interface between ProxyService --> ProxyResolver. You do have a valid point though. Minimally I will do some manual tests. I would like a better story for end-to-end tests in the future. https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service... net/proxy/proxy_service.cc:364: } On 2016/05/19 22:33:23, mmenke wrote: > An alternative approach would be to do: > > if (policy == SanitizeUrlForPacScriptPolicy::SAFE && > url.SchemeIsCryptographic()) { > return url.GetOrigin(); > } > > GURL::Replacements replacements; > ... > > Nice thing about that approach is it emphasizes this is indeed just an origin. > Downside is it de-emphasizes just what the differences are between the two > paths. I added a TODO to explore that, will follow-up. There are some differences I would like to better understand between the two approaches -- GetOrigin() short circuits on non-standard URL schemes and completely clears the URL. I am not sure if that would come into play here, but would like to play it safe by using the existing pattern of code. https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service... net/proxy/proxy_service.cc:1074: SanitizeUrlForPacScript(raw_url, sanitize_url_for_pac_script_policy_); On 2016/05/19 22:33:23, mmenke wrote: > Should we only do this when we create the PacRequest? Seems weird to call it > SanitizeForPac, and then send it to TryToCompleteSynchronously, which doesn't > actually run a PAC script. I hope to merge this CL to M52 so don't want to change the existing code organization too much. I have ideas on improving this but will not be tackling them here. (There are several refactoring opportunities around layering issues in this file -- too many PAC concepts leak into ProxyService. Also the distinction between ProxyService, ProxyConfigService, and ProxyResolver is not great.) https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service.h File net/proxy/proxy_service.h (right): https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service... net/proxy/proxy_service.h:69: NET_EXPORT GURL SanitizeUrlForPacScript(const GURL& url, On 2016/05/19 22:33:23, mmenke wrote: > Do you need to include gurl.h when returning a GURL? Done. https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service... net/proxy/proxy_service.h:70: SanitizeUrlForPacScriptPolicy policy); On 2016/05/19 22:33:24, mmenke wrote: > Not a big fan of bonus enums and methods hanging out in a class's header file. > Can we just move these into ProxyService? Done.
On 2016/05/19 23:26:05, eroman wrote: > Thanks for the feedback Matt! > > > but I'm paranoid enough that I think that one of the tests > > that actually uses a PAC script should check that > > the URLs it gets really are sanitized. > > A full functional test for proxy resolving is a pretty involved, and we don't > really have an existing framework for that. > > There are several more layers of goop around proxy resolving if we wanted to > measure final delivery to a PAC script. > > - MultiThreadedProxyResolver wrapping a platform implementation > Or > - TracingProxyResolverV8 wrapping v8 proxy resolver > Or > - Mojo proxy resolver wrapping either an in-process implementation or > out-of-process forwarder. > > Because we instantiate all these different modes (chrome side and embedder side) > the biggest bang for your buck is testing the interface between ProxyService --> > ProxyResolver. I'd be fine with just a TracingProxyResolverV8 test that uses a PAC script that returns a different value based on whether the URL has any bonus bits attached, or similar. I agree that it would be great if we had something more unity, too, but I think a full on integration test is good enough, for now. > You do have a valid point though. Minimally I will do some manual tests. > > I would like a better story for end-to-end tests in the future. > > https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service.cc > File net/proxy/proxy_service.cc (right): > > https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service... > net/proxy/proxy_service.cc:364: } > On 2016/05/19 22:33:23, mmenke wrote: > > An alternative approach would be to do: > > > > if (policy == SanitizeUrlForPacScriptPolicy::SAFE && > > url.SchemeIsCryptographic()) { > > return url.GetOrigin(); > > } > > > > GURL::Replacements replacements; > > ... > > > > Nice thing about that approach is it emphasizes this is indeed just an origin. > > > Downside is it de-emphasizes just what the differences are between the two > > paths. > > I added a TODO to explore that, will follow-up. > > There are some differences I would like to better understand between the two > approaches -- GetOrigin() short circuits on non-standard URL schemes and > completely clears the URL. I am not sure if that would come into play here, but > would like to play it safe by using the existing pattern of code. > > https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service... > net/proxy/proxy_service.cc:1074: SanitizeUrlForPacScript(raw_url, > sanitize_url_for_pac_script_policy_); > On 2016/05/19 22:33:23, mmenke wrote: > > Should we only do this when we create the PacRequest? Seems weird to call it > > SanitizeForPac, and then send it to TryToCompleteSynchronously, which doesn't > > actually run a PAC script. > > I hope to merge this CL to M52 so don't want to change the existing code > organization too much. > > I have ideas on improving this but will not be tackling them here. > > (There are several refactoring opportunities around layering issues in this file > -- too many PAC concepts leak into ProxyService. Also the distinction between > ProxyService, ProxyConfigService, and ProxyResolver is not great.) > > https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service.h > File net/proxy/proxy_service.h (right): > > https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service... > net/proxy/proxy_service.h:69: NET_EXPORT GURL SanitizeUrlForPacScript(const > GURL& url, > On 2016/05/19 22:33:23, mmenke wrote: > > Do you need to include gurl.h when returning a GURL? > > Done. > > https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service... > net/proxy/proxy_service.h:70: SanitizeUrlForPacScriptPolicy policy); > On 2016/05/19 22:33:24, mmenke wrote: > > Not a big fan of bonus enums and methods hanging out in a class's header file. > > > Can we just move these into ProxyService? > > Done.
On 2016/05/19 23:29:48, mmenke wrote: > On 2016/05/19 23:26:05, eroman wrote: > > Thanks for the feedback Matt! > > > > > but I'm paranoid enough that I think that one of the tests > > > that actually uses a PAC script should check that > > > the URLs it gets really are sanitized. > > > > A full functional test for proxy resolving is a pretty involved, and we don't > > really have an existing framework for that. > > > > There are several more layers of goop around proxy resolving if we wanted to > > measure final delivery to a PAC script. > > > > - MultiThreadedProxyResolver wrapping a platform implementation > > Or > > - TracingProxyResolverV8 wrapping v8 proxy resolver > > Or > > - Mojo proxy resolver wrapping either an in-process implementation or > > out-of-process forwarder. > > > > Because we instantiate all these different modes (chrome side and embedder > side) > > the biggest bang for your buck is testing the interface between ProxyService > --> > > ProxyResolver. > > I'd be fine with just a TracingProxyResolverV8 test that uses a PAC script that > returns a different value based on whether the URL has any bonus bits attached, > or similar. I agree that it would be great if we had something more unity, too, > but I think a full on integration test is good enough, for now. Erm...unity = unit-y > > > You do have a valid point though. Minimally I will do some manual tests. > > > > I would like a better story for end-to-end tests in the future. > > > > > https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service.cc > > File net/proxy/proxy_service.cc (right): > > > > > https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service... > > net/proxy/proxy_service.cc:364: } > > On 2016/05/19 22:33:23, mmenke wrote: > > > An alternative approach would be to do: > > > > > > if (policy == SanitizeUrlForPacScriptPolicy::SAFE && > > > url.SchemeIsCryptographic()) { > > > return url.GetOrigin(); > > > } > > > > > > GURL::Replacements replacements; > > > ... > > > > > > Nice thing about that approach is it emphasizes this is indeed just an > origin. > > > > > Downside is it de-emphasizes just what the differences are between the two > > > paths. > > > > I added a TODO to explore that, will follow-up. > > > > There are some differences I would like to better understand between the two > > approaches -- GetOrigin() short circuits on non-standard URL schemes and > > completely clears the URL. I am not sure if that would come into play here, > but > > would like to play it safe by using the existing pattern of code. > > > > > https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service... > > net/proxy/proxy_service.cc:1074: SanitizeUrlForPacScript(raw_url, > > sanitize_url_for_pac_script_policy_); > > On 2016/05/19 22:33:23, mmenke wrote: > > > Should we only do this when we create the PacRequest? Seems weird to call > it > > > SanitizeForPac, and then send it to TryToCompleteSynchronously, which > doesn't > > > actually run a PAC script. > > > > I hope to merge this CL to M52 so don't want to change the existing code > > organization too much. > > > > I have ideas on improving this but will not be tackling them here. > > > > (There are several refactoring opportunities around layering issues in this > file > > -- too many PAC concepts leak into ProxyService. Also the distinction between > > ProxyService, ProxyConfigService, and ProxyResolver is not great.) > > > > > https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service.h > > File net/proxy/proxy_service.h (right): > > > > > https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service... > > net/proxy/proxy_service.h:69: NET_EXPORT GURL SanitizeUrlForPacScript(const > > GURL& url, > > On 2016/05/19 22:33:23, mmenke wrote: > > > Do you need to include gurl.h when returning a GURL? > > > > Done. > > > > > https://codereview.chromium.org/1996773002/diff/20001/net/proxy/proxy_service... > > net/proxy/proxy_service.h:70: SanitizeUrlForPacScriptPolicy policy); > > On 2016/05/19 22:33:24, mmenke wrote: > > > Not a big fan of bonus enums and methods hanging out in a class's header > file. > > > > > Can we just move these into ProxyService? > > > > Done.
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... net/proxy/proxy_service.cc:1566: DCHECK(url.is_valid()); optional: Suggest a blank line between input sanity checks and function logic. https://codereview.chromium.org/1996773002/diff/40001/net/proxy/proxy_service.h File net/proxy/proxy_service.h (right): https://codereview.chromium.org/1996773002/diff/40001/net/proxy/proxy_service... net/proxy/proxy_service.h:326: NET_EXPORT static GURL SanitizeUrl(const GURL& url, SanitizeUrlPolicy policy); Should this be NET_EXPORT_PRIVATE? Not really concerned about external use, just not sure we really expect it. https://codereview.chromium.org/1996773002/diff/40001/net/proxy/proxy_service... File net/proxy/proxy_service_unittest.cc (right): https://codereview.chromium.org/1996773002/diff/40001/net/proxy/proxy_service... net/proxy/proxy_service_unittest.cc:3485: TEST_F(ProxyServiceTest, SanitizeUrlsBeforeResolving) { Didn't realize you had this sort of test as well, yesterday - just assumed that ProxyService::SanitizeUrl's existence as a public method meant you were only testing at that level. Could consider making it private / moving it to an anonymous namespace, and make all tests go through the ConfigService, so you're only testing its public interface, rather than its implementation details. Could either add a helper method, or just put the request2 / request3 in a loop. Up to you, I just have a personal preference to avoid adding to the public API just for tests if it can reasonably be avoided, and think this is little enough boilerplate that it falls into that category. https://codereview.chromium.org/1996773002/diff/40001/net/proxy/proxy_service... net/proxy/proxy_service_unittest.cc:3593: "ftp://foo:bar@example.com/", "ftp://example.com/", Seems like we should have a test with an FTP URL with a path. https://codereview.chromium.org/1996773002/diff/40001/net/proxy/proxy_service... net/proxy/proxy_service_unittest.cc:3661: "https://example.com/foo/bar/baz?hello", "https://example.com/", Suggest no path on this one (Well...a path of "/", to be technical). Next test checks path + query. While GURL no doubt has plenty of testing, feel that no path here gives us marginally more test coverage here. https://codereview.chromium.org/1996773002/diff/40001/net/proxy/proxy_service... net/proxy/proxy_service_unittest.cc:3673: }, Suggest also having a wss test. Could add a ws test above as well, though I'm less concerned about that.
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... net/proxy/proxy_service.cc:1566: DCHECK(url.is_valid()); On 2016/05/20 17:40:23, mmenke wrote: > optional: Suggest a blank line between input sanity checks and function logic. Done. https://codereview.chromium.org/1996773002/diff/40001/net/proxy/proxy_service.h File net/proxy/proxy_service.h (right): https://codereview.chromium.org/1996773002/diff/40001/net/proxy/proxy_service... net/proxy/proxy_service.h:326: NET_EXPORT static GURL SanitizeUrl(const GURL& url, SanitizeUrlPolicy policy); On 2016/05/20 17:40:23, mmenke wrote: > Should this be NET_EXPORT_PRIVATE? Not really concerned about external use, > just not sure we really expect it. No longer applicable, as I moved the function to be internal-only. That said, I am not a fan of NET_EXPORT_PRIVATE for the reasons given in https://bugs.chromium.org/p/chromium/issues/detail?id=552248 (My pessimistic view is that its value erodes with time as consumers invariably depend on it, making it little more than a confusing out-of-date comment). https://codereview.chromium.org/1996773002/diff/40001/net/proxy/proxy_service... File net/proxy/proxy_service_unittest.cc (right): https://codereview.chromium.org/1996773002/diff/40001/net/proxy/proxy_service... net/proxy/proxy_service_unittest.cc:3485: TEST_F(ProxyServiceTest, SanitizeUrlsBeforeResolving) { On 2016/05/20 17:40:23, mmenke wrote: > Didn't realize you had this sort of test as well, yesterday - just assumed that > ProxyService::SanitizeUrl's existence as a public method meant you were only > testing at that level. > > Could consider making it private / moving it to an anonymous namespace, and make > all tests go through the ConfigService, so you're only testing its public > interface, rather than its implementation details. Could either add a helper > method, or just put the request2 / request3 in a loop. Up to you, I just have a > personal preference to avoid adding to the public API just for tests if it can > reasonably be avoided, and think this is little enough boilerplate that it falls > into that category. Done. I agree that is a better approach. All of the testing is now going through ProxyService. https://codereview.chromium.org/1996773002/diff/40001/net/proxy/proxy_service... net/proxy/proxy_service_unittest.cc:3593: "ftp://foo:bar@example.com/", "ftp://example.com/", On 2016/05/20 17:40:23, mmenke wrote: > Seems like we should have a test with an FTP URL with a path. Done. https://codereview.chromium.org/1996773002/diff/40001/net/proxy/proxy_service... net/proxy/proxy_service_unittest.cc:3661: "https://example.com/foo/bar/baz?hello", "https://example.com/", On 2016/05/20 17:40:23, mmenke wrote: > Suggest no path on this one (Well...a path of "/", to be technical). Next test > checks path + query. While GURL no doubt has plenty of testing, feel that no > path here gives us marginally more test coverage here. Done. https://codereview.chromium.org/1996773002/diff/40001/net/proxy/proxy_service... net/proxy/proxy_service_unittest.cc:3673: }, On 2016/05/20 17:40:23, mmenke wrote: > Suggest also having a wss test. Could add a ws test above as well, though I'm > less concerned about that. Done.
The CQ bit was checked by eroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1996773002/#ps80001 (title: "another ftp test with path")
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
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by eroman@chromium.org
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
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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://crrev.com/81357b39c643fc746517fd6ce5cb2076b7ddc3f4 Cr-Commit-Position: refs/heads/master@{#395266} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/81357b39c643fc746517fd6ce5cb2076b7ddc3f4 Cr-Commit-Position: refs/heads/master@{#395266}
Message was sent while issue was closed.
Description was changed from ========== 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://crrev.com/81357b39c643fc746517fd6ce5cb2076b7ddc3f4 Cr-Commit-Position: refs/heads/master@{#395266} ========== to ========== 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/+/81357b39c643fc746517fd6ce5cb... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 81357b39c643fc746517fd6ce5cb2076b7ddc3f4 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
