New issue
Advanced search Search tips

Issue 901896 link

Starred by 1 user
Status: Untriaged
Owner: ----
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 652385


Sign in to add a comment

Tests should not rely on proxying localhost

Project Member Reported by eroman@chromium.org, Nov 5 
We have tests that test proxy behaviors by requesting URLs to localhost and routing those through the proxy (in some cases using the same host or URL between the proxy and target server).

Except for cases where the test is explicitly trying to validate an ability to proxy localhost addresses, tests are better written using a totally different URL. This makes them more robust (if the proxying is not setup the URL fetch will definitely fail), and it also ensures that tests use a standard proxying configuration (proxying localhost is not a default behavior on macOS and Windows and needs a special proxy bypass rule).

Comment 1 by eroman@chromium.org, Nov 6

Blockedon: 652385
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 6 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d31efeca7c0554367d4ba3a293a0a81b6fe872e0

commit d31efeca7c0554367d4ba3a293a0a81b6fe872e0
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Tue Nov 06 22:22:42 2018

Roll src/third_party/catapult 0273c5e3c23a..70df4ea4898e (2 commits)

https://chromium.googlesource.com/catapult.git/+log/0273c5e3c23a..70df4ea4898e


git log 0273c5e3c23a..70df4ea4898e --date=short --no-merges --format='%ad %ae %s'
2018-11-06 dpranke@chromium.org Address review comments from https://crrev.com/c/1171862.
2018-11-06 eroman@chromium.org Add --proxy-bypass-list parameter to explicitly allow proxying of localhost.


Created with:
  gclient setdep -r src/third_party/catapult@70df4ea4898e

The AutoRoll server is located here: https://autoroll.skia.org/r/catapult-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG= chromium:835690 ,chromium:901896
TBR=sullivan@chromium.org

Change-Id: Icea4759cfbfe703010fc8136b82755bd9be74c18
Reviewed-on: https://chromium-review.googlesource.com/c/1320614
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#605847}
[modify] https://crrev.com/d31efeca7c0554367d4ba3a293a0a81b6fe872e0/DEPS
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 7 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/da790f920bbc169a6805a4fb83b4c2ab09532d91

commit da790f920bbc169a6805a4fb83b4c2ab09532d91
Author: Eric Roman <eroman@chromium.org>
Date: Wed Nov 07 19:17:15 2018

Implicitly bypass localhost when proxying requests.

This aligns Chrome's behavior with the Windows and macOS proxy resolvers (but not Firefox).

Concretely:
 * localhost names (as determined by net::IsLocalhost) now implicitly bypass the proxy
 * link-local IP addresses implicitly bypass the proxy

The implicit rules are handled by ProxyBypassRules, and it is possible to override them when manually configuring proxy settings (but not when using PAC or auto-detect).

This change also adds support for the "<-loopback>" proxy bypass rule, with similar semantics as it has on Windows (removes the implicit bypass rules for localhost and link-local).

The compatibility risk of this change should be low as proxying through localhost was not universally supported. It is however an idiom used in testing (a number of our own tests had such a dependency). Impacted users can use the "<-loopback>" bypass rule as a workaround.

Bug:  413511 , 899126, 901896
Change-Id: I263ca21ef9f12d4759a20cb4751dc3261bda6ac0
Reviewed-on: https://chromium-review.googlesource.com/c/1303626
Commit-Queue: Eric Roman <eroman@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606112}
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/chrome/browser/net/proxy_browsertest.cc
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/headless/lib/headless_devtools_client_browsertest.cc
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/net/http/http_auth_filter.cc
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/net/http/http_auth_filter.h
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/net/http/http_network_transaction_unittest.cc
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/net/proxy_resolution/parse_proxy_bypass_rules_fuzzer.cc
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/net/proxy_resolution/proxy_bypass_rules.cc
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/net/proxy_resolution/proxy_bypass_rules.h
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/net/proxy_resolution/proxy_bypass_rules_unittest.cc
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/net/proxy_resolution/proxy_config.cc
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/net/proxy_resolution/proxy_config.h
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/net/proxy_resolution/proxy_config_service_linux.cc
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/net/proxy_resolution/proxy_config_service_linux.h
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/net/proxy_resolution/proxy_config_service_linux_unittest.cc
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/net/proxy_resolution/proxy_resolution_service.cc
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/net/proxy_resolution/proxy_resolution_service.h
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/net/proxy_resolution/proxy_resolution_service_unittest.cc
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/net/url_request/url_fetcher_impl_unittest.cc
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/net/url_request/url_request_unittest.cc
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/net/websockets/websocket_end_to_end_test.cc
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/services/network/public/cpp/proxy_config_mojom_traits_unittest.cc
[modify] https://crrev.com/da790f920bbc169a6805a4fb83b4c2ab09532d91/services/network/tls_client_socket_unittest.cc
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 28 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/70e4265a5a0376b0dc886b75108c32a9fe199188

commit 70e4265a5a0376b0dc886b75108c32a9fe199188
Author: Eric Roman <eroman@chromium.org>
Date: Wed Nov 28 21:28:38 2018

Fix LoadTimingBrowserTest.Proxy so it actually tests the proxy case.

After I changed the bypass policy for localhost, this test stopped routing through proxy.

Bug: 901896
Change-Id: I5f604865b60837143e7b2bc00c3dcbc9971c70c3
Reviewed-on: https://chromium-review.googlesource.com/c/1351145
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Eric Roman <eroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611874}
[modify] https://crrev.com/70e4265a5a0376b0dc886b75108c32a9fe199188/chrome/browser/net/load_timing_browsertest.cc
Project Member

Comment 6 by bugdroid1@chromium.org, Nov 28 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/210da2fb3dc1c6e88dfdefdbea264daf1c8dfe43

commit 210da2fb3dc1c6e88dfdefdbea264daf1c8dfe43
Author: Eric Roman <eroman@chromium.org>
Date: Wed Nov 28 23:05:02 2018

Update some test expectations which relied on localhost not bypassing PAC.

Bug: 901896
Change-Id: I932d2a34b63c803f1455b9d908539072ce350cd8
Reviewed-on: https://chromium-review.googlesource.com/c/1351820
Commit-Queue: Eric Roman <eroman@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611919}
[modify] https://crrev.com/210da2fb3dc1c6e88dfdefdbea264daf1c8dfe43/chrome/browser/net/network_context_configuration_browsertest.cc
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 29 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e4a2d0f51a91fffbb0c59e6627f3595d9d19b156

commit e4a2d0f51a91fffbb0c59e6627f3595d9d19b156
Author: Eric Roman <eroman@chromium.org>
Date: Thu Nov 29 02:33:48 2018

Update a test to explicitly allow proxying localhost.

This preserves its previous behavior (localhost now bypasses the proxy).

Bug: 901896
Change-Id: I2d651b6ff8828d3f344fed0aecf4fb4ae4c31eb8
Reviewed-on: https://chromium-review.googlesource.com/c/1354646
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Eric Roman <eroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611993}
[modify] https://crrev.com/e4a2d0f51a91fffbb0c59e6627f3595d9d19b156/services/network/network_context_unittest.cc
Project Member

Comment 8 by bugdroid1@chromium.org, Nov 29 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9ac411d40a22426f11547c5e000ca82bc0ae6339

commit 9ac411d40a22426f11547c5e000ca82bc0ae6339
Author: Eric Roman <eroman@chromium.org>
Date: Thu Nov 29 18:37:10 2018

Update some extension tests to use an unresolvable URL when using proxy.

This ensures that the request must go through the proxy to pass.

Bug: 901896
Change-Id: I4c54d438ab2689b082eefc544b1c64f3866bb2c9
Reviewed-on: https://chromium-review.googlesource.com/c/1354324
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Eric Roman <eroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612287}
[modify] https://crrev.com/9ac411d40a22426f11547c5e000ca82bc0ae6339/chrome/browser/extensions/api/declarative_net_request/declarative_net_request_browsertest.cc
[modify] https://crrev.com/9ac411d40a22426f11547c5e000ca82bc0ae6339/chrome/browser/extensions/api/web_request/web_request_apitest.cc
►

Sign in to add a comment