Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1126413006:
Temporary fix to prevent the DRP from being activated improperly. (Closed)
DescriptionTemporary fix to prevent the DRP from being activated improperly.
This change prevents any *.googlezip.net Data Reduction Proxy from being
configured as part of the ProxyService's ProxyConfig via the
proxy pref.
Specifying a DRP in the proxy rules is not a supported means of
activating the DRP, and could cause requests to be sent to the DRP
without the appropriate authentication headers and without using any of
the DRP bypass logic.
This is a fix for http://crbug.com/476610, to be merged into M43.
BUG=476610
Committed: https://crrev.com/7e18b89c1fe52bfc7868dc3ded772e81f404b3c4
Cr-Commit-Position: refs/heads/master@{#329349}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Addressed comments and added bypass logic enforcement if the DRP via header is present #
Total comments: 4
Patch Set 3 : Addressed comments #Patch Set 4 : Moved workaround to pref_proxy_config_tracker_impl, and split out drp_bypass_protocol change to a d… #Patch Set 5 : Fixed comment #
Total comments: 10
Patch Set 6 : Addressed comments #
Total comments: 4
Patch Set 7 : Addressed comments #
Messages
Total messages: 25 (8 generated)
sclittle@chromium.org changed reviewers: + bengr@chromium.org, eroman@chromium.org, mmenke@chromium.org
PTAL - I'd like to land this ASAP in time for the last M43 beta cut on Tuesday. Thanks in advance!
We might also want to support bypass instructions on non-incognito even when Chrome doesn't recognize the proxy connection as being to the DRP. We could double check that the Via header is there of we want to be a little more paranoid. https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc#... net/proxy/proxy_service.cc:357: // Removes any Data Reduction Proxies like *.googlezip.net from |proxy_list|. Consider limiting all of this logic to android and ios, with #if defined(OS_ANDROID) and #if defined(OS_IOS). We don't see this problem on desktop, and desktop use of proxies is much more extensive than mobile. https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc#... net/proxy/proxy_service.cc:358: void RemoveGooglezipDataReductionProxies(ProxyList* proxy_list) { Add a TODO to add UMA to track how often this is called and how often it actually removes a googlezip.net proxy, and to remove this code when the latter is zero. https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc#... net/proxy/proxy_service.cc:359: if (proxy_list->IsEmpty()) #include "net/proxy/proxy_list.h" https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc#... net/proxy/proxy_service.cc:366: replacement_list.AddProxyServer(proxy); add an else clause that sets found_googlezip_proxy to true https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc#... net/proxy/proxy_service.cc:371: replacement_list.AddProxyServer(ProxyServer::Direct()); Is this the contents of a list when there's no configured proxy? https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc#... net/proxy/proxy_service.cc:372: *proxy_list = replacement_list; replace only if found_googlezip_proxy is true. https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc#... net/proxy/proxy_service.cc:378: RemoveGooglezipDataReductionProxies(&proxy_rules->fallback_proxies); #include "net/proxy/proxy_config.h" https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc#... net/proxy/proxy_service.cc:380: RemoveGooglezipDataReductionProxies(&proxy_rules->proxies_for_http); We only ever set HTTP. I can understand checking fallback and single proxies, but why check ftp and https? https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc#... net/proxy/proxy_service.cc:1204: RemoveGooglezipDataReductionProxiesFromRules(&config_.proxy_rules()); Please explain when you expect this to be called. For example, if I change the system proxy, will this be called again? If I toggle the DRP setting off/on, will this be called?
kundaji@chromium.org changed reviewers: + kundaji@chromium.org
https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc#... net/proxy/proxy_service.cc:357: // Removes any Data Reduction Proxies like *.googlezip.net from |proxy_list|. On 2015/05/10 16:49:34, bengr wrote: > Consider limiting all of this logic to android and ios, with #if > defined(OS_ANDROID) and #if defined(OS_IOS). We don't see this problem on > desktop, and desktop use of proxies is much more extensive than mobile. Can we verify that this does not affect desktop? If it does, we can call this behind a check for pref::kDataReductionProxyWasEnabledBefore. https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc#... net/proxy/proxy_service.cc:371: replacement_list.AddProxyServer(ProxyServer::Direct()); On 2015/05/10 16:49:34, bengr wrote: > Is this the contents of a list when there's no configured proxy? Instead of setting ProxyServer::Direct(), you should clear prefs::kProxy pref when there are no proxies. If this pref is not set, Chrome will use ProxyPrefs::MODE_SYSTEM as the default.
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc#... net/proxy/proxy_service.cc:357: // Removes any Data Reduction Proxies like *.googlezip.net from |proxy_list|. On 2015/05/10 16:49:34, bengr wrote: > Consider limiting all of this logic to android and ios, with #if > defined(OS_ANDROID) and #if defined(OS_IOS). We don't see this problem on > desktop, and desktop use of proxies is much more extensive than mobile. But either way, this code would only remove data reduction proxies that were added by something other than OnProxyResolved in DRPNetworkDelegate, so using these proxies could thus cause users to see 502 responses from that proxy. We should prevent a data reduction proxy from being used in an unsupported way, regardless of platform. We actually do see this issue on desktop, but much less often than on mobile. https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc#... net/proxy/proxy_service.cc:358: void RemoveGooglezipDataReductionProxies(ProxyList* proxy_list) { On 2015/05/10 16:49:34, bengr wrote: > Add a TODO to add UMA to track how often this is called and how often it > actually removes a http://googlezip.net proxy, and to remove this code when the latter > is zero. Done. https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc#... net/proxy/proxy_service.cc:359: if (proxy_list->IsEmpty()) On 2015/05/10 16:49:34, bengr wrote: > #include "net/proxy/proxy_list.h" Done. https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc#... net/proxy/proxy_service.cc:366: replacement_list.AddProxyServer(proxy); On 2015/05/10 16:49:33, bengr wrote: > add an else clause that sets found_googlezip_proxy to true Instead of using a bool variable, I just check if the size of the replacement list is different. https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc#... net/proxy/proxy_service.cc:371: replacement_list.AddProxyServer(ProxyServer::Direct()); On 2015/05/11 17:24:09, kundaji wrote: > On 2015/05/10 16:49:34, bengr wrote: > > Is this the contents of a list when there's no configured proxy? > > Instead of setting ProxyServer::Direct(), you should clear prefs::kProxy pref > when there are no proxies. > > If this pref is not set, Chrome will use ProxyPrefs::MODE_SYSTEM as the > default. Re bengr: It's possible that the DRP was configured as a mandatory proxy, if all the proxies in the list were DRPs. This check is just to prevent the user from seeing ERR_NO_SUPPORTED_PROXIES errors all the time. Re kundaji: I'm trying to avoid assuming anything about the pref at this point, otherwise I'd expect this other method to solve the problem: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net... This method doesn't just apply to the proxy pref, it applies to any proxy configuration set in the proxy service, e.g. proxy pref, PAC fetch, "--proxy-server" command line flag, etc. Also, this is called after that pref is read, so adding in Direct() would still have to happen here. https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc#... net/proxy/proxy_service.cc:372: *proxy_list = replacement_list; On 2015/05/10 16:49:34, bengr wrote: > replace only if found_googlezip_proxy is true. Done, but instead of using a found_googlezip_proxy variable I just check if the replacement list is a different size. https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc#... net/proxy/proxy_service.cc:378: RemoveGooglezipDataReductionProxies(&proxy_rules->fallback_proxies); On 2015/05/10 16:49:34, bengr wrote: > #include "net/proxy/proxy_config.h" Done. https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc#... net/proxy/proxy_service.cc:380: RemoveGooglezipDataReductionProxies(&proxy_rules->proxies_for_http); On 2015/05/10 16:49:34, bengr wrote: > We only ever set HTTP. I can understand checking fallback and single proxies, > but why check ftp and https? For thoroughness; there's no harm in checking the others. The DRP is definitely not supported for FTP, and the alternative DRP configuration has an HTTPS tunnel proxy. https://codereview.chromium.org/1126413006/diff/1/net/proxy/proxy_service.cc#... net/proxy/proxy_service.cc:1204: RemoveGooglezipDataReductionProxiesFromRules(&config_.proxy_rules()); On 2015/05/10 16:49:34, bengr wrote: > Please explain when you expect this to be called. For example, if I change the > system proxy, will this be called again? If I toggle the DRP setting off/on, > will this be called? Added comment. This will be called whenever SetReady is called, which is when the ProxyService decides on a ProxyConfig. It has nothing to do with toggling the DRP on or off.
lgtm modulo nits. https://codereview.chromium.org/1126413006/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc (right): https://codereview.chromium.org/1126413006/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc:101: if (HasDataReductionProxyViaHeader(response_headers, nullptr)) { Negate this and return false early. https://codereview.chromium.org/1126413006/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc:105: // TODO(sclittle): Add UMA to occur how often this occurs, and remove this Fix "occur".
https://codereview.chromium.org/1126413006/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc (right): https://codereview.chromium.org/1126413006/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc:101: if (HasDataReductionProxyViaHeader(response_headers, nullptr)) { On 2015/05/11 20:51:35, bengr wrote: > Negate this and return false early. Done. https://codereview.chromium.org/1126413006/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc:105: // TODO(sclittle): Add UMA to occur how often this occurs, and remove this On 2015/05/11 20:51:35, bengr wrote: > Fix "occur". Done.
The CQ bit was checked by sclittle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org Link to the patchset: https://codereview.chromium.org/1126413006/#ps60001 (title: "Addressed comments")
The CQ bit was unchecked by sclittle@chromium.org
On 2015/05/11 20:56:37, sclittle wrote: > https://codereview.chromium.org/1126413006/diff/40001/components/data_reducti... > File > components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc > (right): > > https://codereview.chromium.org/1126413006/diff/40001/components/data_reducti... > components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc:101: > if (HasDataReductionProxyViaHeader(response_headers, nullptr)) { > On 2015/05/11 20:51:35, bengr wrote: > > Negate this and return false early. > > Done. > > https://codereview.chromium.org/1126413006/diff/40001/components/data_reducti... > components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc:105: > // TODO(sclittle): Add UMA to occur how often this occurs, and remove this > On 2015/05/11 20:51:35, bengr wrote: > > Fix "occur". > > Done. Sorry, I misclicked when adding trybots; I haven't sent this to the CQ yet.
This layering is wrong, why are we checking for .googlezip.net in //net? I am also not sympathetic to rushing things in just before branch points. Can you summarize the issue this is trying to fix? (476610 has a lot of back and forth which I didn't read all of). Is there something that can be rolled back instead?
FTR we discussed an alternate place to put this fix being in src/chrome/browser/net/pref_proxy_config_tracker_impl.cc, which I think is more in line with where the problematic prefs originates from. Perhaps simply in the existing function: PrefProxyConfigTrackerImpl::GetEffectiveProxyConfig sclittle is looking into it if this covers all the cases they are interested in.
I've moved the fix to src/chrome/browser/net/pref_proxy_config_tracker_impl.cc. PTAL
https://codereview.chromium.org/1126413006/diff/100001/chrome/browser/net/pre... File chrome/browser/net/pref_proxy_config_tracker_impl.cc (right): https://codereview.chromium.org/1126413006/diff/100001/chrome/browser/net/pre... chrome/browser/net/pref_proxy_config_tracker_impl.cc:26: // Removes any Data Reduction Proxies like *.googlezip.net from |proxy_list|. Please add more context on why this is being done, along with a link to the bug. https://codereview.chromium.org/1126413006/diff/100001/chrome/browser/net/pre... chrome/browser/net/pref_proxy_config_tracker_impl.cc:31: net::ProxyList replacement_list; Please do this in two phases: (1) Scan the list to see if it contains at least 1 proxy ending in ".googlezip.net" (2) If it does then build the replacement list and swap. The reason I would rather do it this way, is so there is less changes to the normal codeflow (i.e. .googlezip.net is no in there). https://codereview.chromium.org/1126413006/diff/100001/chrome/browser/net/pre... chrome/browser/net/pref_proxy_config_tracker_impl.cc:41: if (replacement_list.size() != proxy_list->size()) This doesn't look correct. What if the list were: ["http://foo.googlezip.net] In that case we will rewrite it to: ["direct://"] Because of the line above. In which case sizes will be unchanged (1) however replacement is in fact needed. See my comment above which I think will also address this (do it in two phases). https://codereview.chromium.org/1126413006/diff/100001/chrome/browser/net/pre... chrome/browser/net/pref_proxy_config_tracker_impl.cc:46: // TODO(sclittle): Add UMA to record how often this method is called, and how When to do you plan on doing this? If we are going to be adding this hack to M43 it should also have appropriate measurements. https://codereview.chromium.org/1126413006/diff/100001/chrome/browser/net/pre... chrome/browser/net/pref_proxy_config_tracker_impl.cc:228: } else if (system_availability == net::ProxyConfigService::CONFIG_UNSET) { OK. alternately you could have extracted it to a helper function and kept the "return" short-circuits.
https://codereview.chromium.org/1126413006/diff/100001/chrome/browser/net/pre... File chrome/browser/net/pref_proxy_config_tracker_impl.cc (right): https://codereview.chromium.org/1126413006/diff/100001/chrome/browser/net/pre... chrome/browser/net/pref_proxy_config_tracker_impl.cc:26: // Removes any Data Reduction Proxies like *.googlezip.net from |proxy_list|. On 2015/05/12 00:43:01, eroman wrote: > Please add more context on why this is being done, along with a link to the bug. Done, added to the comment of the RemoveGooglezipDataReductionProxies function below. https://codereview.chromium.org/1126413006/diff/100001/chrome/browser/net/pre... chrome/browser/net/pref_proxy_config_tracker_impl.cc:31: net::ProxyList replacement_list; On 2015/05/12 00:43:01, eroman wrote: > Please do this in two phases: > (1) Scan the list to see if it contains at least 1 proxy ending in > ".googlezip.net" > (2) If it does then build the replacement list and swap. > > The reason I would rather do it this way, is so there is less changes to the > normal codeflow (i.e. .googlezip.net is no in there). Done. https://codereview.chromium.org/1126413006/diff/100001/chrome/browser/net/pre... chrome/browser/net/pref_proxy_config_tracker_impl.cc:41: if (replacement_list.size() != proxy_list->size()) On 2015/05/12 00:43:01, eroman wrote: > This doesn't look correct. What if the list were: > > ["http://foo.googlezip.net] > > In that case we will rewrite it to: > > ["direct://"] > > Because of the line above. In which case sizes will be unchanged (1) however > replacement is in fact needed. > > See my comment above which I think will also address this (do it in two phases). Thanks, I just noticed this as well when testing now. https://codereview.chromium.org/1126413006/diff/100001/chrome/browser/net/pre... chrome/browser/net/pref_proxy_config_tracker_impl.cc:46: // TODO(sclittle): Add UMA to record how often this method is called, and how On 2015/05/12 00:43:01, eroman wrote: > When to do you plan on doing this? If we are going to be adding this hack to M43 > it should also have appropriate measurements. I'm planning on adding the measurements in M44. I'm trying to minimize the code that gets merged into M43 here. It seems about as useful for us to measure these stats in M44, since the full fix for this will likely also be in M44. https://codereview.chromium.org/1126413006/diff/100001/chrome/browser/net/pre... chrome/browser/net/pref_proxy_config_tracker_impl.cc:228: } else if (system_availability == net::ProxyConfigService::CONFIG_UNSET) { On 2015/05/12 00:43:01, eroman wrote: > OK. alternately you could have extracted it to a helper function and kept the > "return" short-circuits. Acknowledged. I'll leave it this way unless you'd prefer it as a separate helper function.
LGTM. Although not sure I understand the meaning of "hotfix" in the description. https://codereview.chromium.org/1126413006/diff/120001/chrome/browser/net/pre... File chrome/browser/net/pref_proxy_config_tracker_impl.cc (right): https://codereview.chromium.org/1126413006/diff/120001/chrome/browser/net/pre... chrome/browser/net/pref_proxy_config_tracker_impl.cc:48: for (const net::ProxyServer& proxy : proxy_list->GetAll()) can you put curlies around this? https://codereview.chromium.org/1126413006/diff/120001/chrome/browser/net/pre... chrome/browser/net/pref_proxy_config_tracker_impl.cc:264: // TODO(sclittle): This is a temporary hotfix for http://crbug.com/476610, and not sure what "hotfix" is supposed to mean. In my mind a hotfix is a patch you apply to a live system, which doesn't seem relevant here. How about just calling this either a "fix" or a "hack"
I've replaced my usage of the term "hotfix" with "temporary fix". Thanks a ton for reviewing this, sorry for bothering you. https://codereview.chromium.org/1126413006/diff/120001/chrome/browser/net/pre... File chrome/browser/net/pref_proxy_config_tracker_impl.cc (right): https://codereview.chromium.org/1126413006/diff/120001/chrome/browser/net/pre... chrome/browser/net/pref_proxy_config_tracker_impl.cc:48: for (const net::ProxyServer& proxy : proxy_list->GetAll()) On 2015/05/12 03:13:53, eroman wrote: > can you put curlies around this? Done. https://codereview.chromium.org/1126413006/diff/120001/chrome/browser/net/pre... chrome/browser/net/pref_proxy_config_tracker_impl.cc:264: // TODO(sclittle): This is a temporary hotfix for http://crbug.com/476610, and On 2015/05/12 03:13:53, eroman wrote: > not sure what "hotfix" is supposed to mean. In my mind a hotfix is a patch you > apply to a live system, which doesn't seem relevant here. How about just calling > this either a "fix" or a "hack" Done.
The CQ bit was checked by sclittle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org, eroman@chromium.org Link to the patchset: https://codereview.chromium.org/1126413006/#ps140001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1126413006/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7e18b89c1fe52bfc7868dc3ded772e81f404b3c4 Cr-Commit-Position: refs/heads/master@{#329349} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
