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

Issue 387353003: Modify data_reduction_proxy_header to support tamper detection logic. (Closed)

Created:
6 years, 5 months ago by xingx
Modified:
6 years, 4 months ago
Reviewers:
xingx1, bengr, tonyg, bolian, Yaron
Base URL:
https://chromium.googlesource.com/chromium/src.git@work
Project:
chromium
Visibility:
Public.

Description

1. Adds action-parsing functionality for data reduction proxy header. 2. Adds a couple of functions to return different fingerprint value for tamper detection. 3. Adds a function to remove Chrome-Proxy header's fingerprint from its header values, and return the rest of header values. 4. Changes HasDataReductionProxyViaHeader to also tell whether data reduction proxy Via header occurs at the last or not. BUG=381907 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287561 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287855 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288272

Patch Set 1 #

Total comments: 14

Patch Set 2 : #

Total comments: 2

Patch Set 3 : Modify HasDataReductionProxyViaHeader to tell whether data reduction proxy occurs at last or not. #

Total comments: 17

Patch Set 4 : Adding constants to avoid change the same file in two CLs . #

Patch Set 5 : #

Patch Set 6 : after sync #

Total comments: 9

Patch Set 7 : #

Total comments: 24

Patch Set 8 : #

Patch Set 9 : merged with latest version #

Total comments: 8

Patch Set 10 : #

Patch Set 11 : #

Total comments: 4

Patch Set 12 : #

Total comments: 2

Patch Set 13 : #

Patch Set 14 : #

Patch Set 15 : #

Patch Set 16 : #

Patch Set 17 : #

Patch Set 18 : Fix timeout issue in Win7 test. #

Patch Set 19 : Fix timeout issue on Win7Test, add unittests. #

Patch Set 20 : after sync #

Unified diffs Side-by-side diffs Delta from patch set Stats (+563 lines, -21 lines) Patch
M chrome/browser/android/intercept_download_resource_throttle.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M chrome/renderer/page_load_histograms.cc View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -1 line 0 comments Download
M components/data_reduction_proxy/browser/data_reduction_proxy_metrics.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M components/data_reduction_proxy/common/data_reduction_proxy_headers.h View 1 2 3 4 5 6 7 8 2 chunks +39 lines, -2 lines 0 comments Download
M components/data_reduction_proxy/common/data_reduction_proxy_headers.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 8 chunks +122 lines, -12 lines 0 comments Download
M components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 5 chunks +398 lines, -4 lines 0 comments Download

Messages

Total messages: 60 (0 generated)
xingx
6 years, 5 months ago (2014-07-14 16:53:55 UTC) #1
bengr
https://codereview.chromium.org/387353003/diff/1/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/1/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc#newcode39 components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:39: *action_value = value.substr(action_prefix.size()); Before the loop, either DCHECK that ...
6 years, 5 months ago (2014-07-14 18:27:14 UTC) #2
bolian
https://codereview.chromium.org/387353003/diff/1/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/1/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc#newcode32 components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:32: std::string name = "chrome-proxy"; inline this in the func ...
6 years, 5 months ago (2014-07-14 19:23:59 UTC) #3
xingx
https://codereview.chromium.org/387353003/diff/1/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/1/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc#newcode32 components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:32: std::string name = "chrome-proxy"; On 2014/07/14 19:23:58, bolian wrote: ...
6 years, 5 months ago (2014-07-14 21:37:17 UTC) #4
bolian
https://codereview.chromium.org/387353003/diff/1/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/1/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc#newcode32 components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:32: std::string name = "chrome-proxy"; ? https://codereview.chromium.org/387353003/diff/40001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): ...
6 years, 5 months ago (2014-07-14 22:42:43 UTC) #5
xingx
https://codereview.chromium.org/387353003/diff/1/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/1/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc#newcode32 components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:32: std::string name = "chrome-proxy"; On 2014/07/14 22:42:43, bolian wrote: ...
6 years, 5 months ago (2014-07-14 22:51:09 UTC) #6
xingx
Changes HasDataReductionProxyViaHeader to also tell whether data reduction proxy Via header occurs at the last ...
6 years, 5 months ago (2014-07-14 23:34:33 UTC) #7
bengr
https://codereview.chromium.org/387353003/diff/110001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/110001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc#newcode27 components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:27: DCHECK(action_prefix.size()); DCHECK(!action_prefix.empty()); https://codereview.chromium.org/387353003/diff/110001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc#newcode30 components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:30: std::string name = "chrome-proxy"; Add ...
6 years, 5 months ago (2014-07-16 19:23:57 UTC) #8
xingx
https://codereview.chromium.org/387353003/diff/110001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/110001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc#newcode27 components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:27: DCHECK(action_prefix.size()); On 2014/07/16 19:23:56, bengr1 wrote: > DCHECK(!action_prefix.empty()); Done. ...
6 years, 5 months ago (2014-07-16 21:22:33 UTC) #9
bolian
Is this dead? Can you remove us from reviewers or delete this cl?
6 years, 5 months ago (2014-07-18 23:21:55 UTC) #10
xingx
Add kChromeProxyActionFingerprint* strings.
6 years, 5 months ago (2014-07-21 18:09:50 UTC) #11
bolian
https://codereview.chromium.org/387353003/diff/520001/chrome/renderer/page_load_histograms.cc File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/387353003/diff/520001/chrome/renderer/page_load_histograms.cc#newcode180 chrome/renderer/page_load_histograms.cc:180: return data_reduction_proxy::HasDataReductionProxyViaHeader(response_headers, new line before the first argument and ...
6 years, 4 months ago (2014-07-28 22:59:18 UTC) #12
xingx1
Addressed Bolian's comments. https://codereview.chromium.org/387353003/diff/520001/chrome/renderer/page_load_histograms.cc File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/387353003/diff/520001/chrome/renderer/page_load_histograms.cc#newcode180 chrome/renderer/page_load_histograms.cc:180: return data_reduction_proxy::HasDataReductionProxyViaHeader(response_headers, On 2014/07/28 22:59:18, bolian ...
6 years, 4 months ago (2014-07-29 00:47:40 UTC) #13
bolian
lgtm. Wait for bengr.
6 years, 4 months ago (2014-07-29 01:31:53 UTC) #14
bolian
https://codereview.chromium.org/387353003/diff/520001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/520001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc#newcode37 components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:37: DCHECK(action_prefix[action_prefix.size() - 1] != '='); I don't see this ...
6 years, 4 months ago (2014-07-29 01:36:21 UTC) #15
bolian
https://codereview.chromium.org/387353003/diff/580001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/580001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc#newcode61 components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:61: DCHECK(action_prefix[action_prefix.size() - 1] != '='); same here, you don't ...
6 years, 4 months ago (2014-07-29 17:03:50 UTC) #16
bengr
https://codereview.chromium.org/387353003/diff/580001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/580001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc#newcode30 components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:30: const char kChromeProxyActionFingerprintContentLength[] = "fcl"; I really don't like ...
6 years, 4 months ago (2014-07-29 17:50:28 UTC) #17
xingx1
https://codereview.chromium.org/387353003/diff/580001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/580001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc#newcode30 components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:30: const char kChromeProxyActionFingerprintContentLength[] = "fcl"; On 2014/07/29 17:50:28, bengr1 ...
6 years, 4 months ago (2014-07-30 03:32:56 UTC) #18
xingx1
Just merged with latest version. Changes I made to other codes (same to before): 1) ...
6 years, 4 months ago (2014-07-30 23:49:37 UTC) #19
bengr
https://codereview.chromium.org/387353003/diff/670001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/670001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc#newcode25 components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:25: const char kActionValueDelimiter[] = "="; Add a blank line ...
6 years, 4 months ago (2014-07-31 16:38:18 UTC) #20
xingx1
Added trybots. https://codereview.chromium.org/387353003/diff/670001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/670001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc#newcode25 components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:25: const char kActionValueDelimiter[] = "="; On 2014/07/31 ...
6 years, 4 months ago (2014-07-31 17:32:05 UTC) #21
bolian
https://codereview.chromium.org/387353003/diff/670001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/670001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc#newcode55 components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:55: DCHECK(action_prefix[action_prefix.size() - 1] != '='); I think you want ...
6 years, 4 months ago (2014-07-31 18:44:09 UTC) #22
xingx1
https://codereview.chromium.org/387353003/diff/670001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/670001/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc#newcode55 components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:55: DCHECK(action_prefix[action_prefix.size() - 1] != '='); On 2014/07/31 18:44:09, bolian ...
6 years, 4 months ago (2014-08-01 04:01:08 UTC) #23
bengr
lgtm with nits. Please fix the formatting of the description and add a BUG number ...
6 years, 4 months ago (2014-08-04 16:32:11 UTC) #24
xingx1
This one is ready. https://codereview.chromium.org/387353003/diff/710001/components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc File components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc (right): https://codereview.chromium.org/387353003/diff/710001/components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc#newcode294 components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:294: bool is_has_intermediary_null; On 2014/08/04 16:32:11, ...
6 years, 4 months ago (2014-08-04 17:24:32 UTC) #25
xingx1
The CQ bit was checked by xingx@google.com
6 years, 4 months ago (2014-08-04 17:31:53 UTC) #26
xingx1
The CQ bit was unchecked by xingx@google.com
6 years, 4 months ago (2014-08-04 17:32:11 UTC) #27
xingx1
The CQ bit was checked by xingx@google.com
6 years, 4 months ago (2014-08-04 18:12:16 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xingx@chromium.org/387353003/750001
6 years, 4 months ago (2014-08-04 18:14:17 UTC) #29
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium.linux ...
6 years, 4 months ago (2014-08-04 21:50:26 UTC) #30
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 4 months ago (2014-08-04 21:53:55 UTC) #31
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/1925)
6 years, 4 months ago (2014-08-04 21:53:56 UTC) #32
xingx1
Hi, Tony, Yaron, Please review a simple change of files: chrome/renderer/page_load_histograms.cc (for Tony) chrome/browser/android/intercept_download_resource_throttle.cc (for ...
6 years, 4 months ago (2014-08-04 22:48:56 UTC) #33
Yaron
lgtm
6 years, 4 months ago (2014-08-05 16:20:35 UTC) #34
tonyg
lgtm for page_load_histograms.cc. I didn't look at the data_reduction_proxy/* code closely.
6 years, 4 months ago (2014-08-05 17:56:13 UTC) #35
xingx1
The CQ bit was checked by xingx@google.com
6 years, 4 months ago (2014-08-05 17:57:39 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xingx@chromium.org/387353003/750001
6 years, 4 months ago (2014-08-05 17:59:42 UTC) #37
commit-bot: I haz the power
Change committed as 287561
6 years, 4 months ago (2014-08-05 18:04:39 UTC) #38
robliao
A revert of this CL has been created in https://codereview.chromium.org/444723003/ by robliao@chromium.org. The reason for ...
6 years, 4 months ago (2014-08-05 21:49:25 UTC) #39
robliao
On 2014/08/05 21:49:25, robliao wrote: > A revert of this CL has been created in ...
6 years, 4 months ago (2014-08-05 21:50:53 UTC) #40
bengr
https://codereview.chromium.org/387353003/diff/750001/components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc File components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc (right): https://codereview.chromium.org/387353003/diff/750001/components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc#newcode713 components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:713: struct { Make this const.
6 years, 4 months ago (2014-08-06 17:26:19 UTC) #41
xingx1
https://codereview.chromium.org/387353003/diff/750001/components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc File components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc (right): https://codereview.chromium.org/387353003/diff/750001/components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc#newcode713 components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:713: struct { On 2014/08/06 17:26:19, bengr1 wrote: > Make ...
6 years, 4 months ago (2014-08-06 17:35:22 UTC) #42
xingx1
The CQ bit was checked by xingx@google.com
6 years, 4 months ago (2014-08-06 17:38:02 UTC) #43
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xingx@chromium.org/387353003/770001
6 years, 4 months ago (2014-08-06 17:40:50 UTC) #44
commit-bot: I haz the power
Change committed as 287855
6 years, 4 months ago (2014-08-06 20:57:09 UTC) #45
xingx1
A revert of this CL has been created in https://codereview.chromium.org/447863004/ by xingx@google.com. The reason for ...
6 years, 4 months ago (2014-08-06 22:43:07 UTC) #46
xingx1
The CQ bit was checked by xingx@google.com
6 years, 4 months ago (2014-08-07 05:01:29 UTC) #47
xingx1
The CQ bit was unchecked by xingx@google.com
6 years, 4 months ago (2014-08-07 05:01:37 UTC) #48
xingx1
The CQ bit was checked by xingx@google.com
6 years, 4 months ago (2014-08-07 05:39:26 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xingx@chromium.org/387353003/930001
6 years, 4 months ago (2014-08-07 05:41:34 UTC) #50
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 4 months ago (2014-08-07 08:30:24 UTC) #51
commit-bot: I haz the power
Failed to apply patch for components/data_reduction_proxy/common/data_reduction_proxy_headers.cc: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
6 years, 4 months ago (2014-08-07 08:30:26 UTC) #52
xingx1
The CQ bit was checked by xingx@google.com
6 years, 4 months ago (2014-08-07 22:51:33 UTC) #53
xingx1
The CQ bit was unchecked by xingx@google.com
6 years, 4 months ago (2014-08-07 22:53:12 UTC) #54
xingx1
The CQ bit was checked by xingx@google.com
6 years, 4 months ago (2014-08-07 22:57:04 UTC) #55
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xingx@chromium.org/387353003/1030001
6 years, 4 months ago (2014-08-08 00:00:25 UTC) #56
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux ...
6 years, 4 months ago (2014-08-08 04:53:22 UTC) #57
xingx1
The CQ bit was checked by xingx@google.com
6 years, 4 months ago (2014-08-08 05:17:59 UTC) #58
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xingx@chromium.org/387353003/1030001
6 years, 4 months ago (2014-08-08 05:19:23 UTC) #59
commit-bot: I haz the power
6 years, 4 months ago (2014-08-08 09:16:29 UTC) #60
Message was sent while issue was closed.
Change committed as 288272

Powered by Google App Engine
This is Rietveld 408576698