Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
338483002:
Chrome Participated Tamper Detect (Closed)
|
Created:
6 years, 6 months ago by xingx Modified:
6 years, 4 months ago Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionImplement client side tamper detection logic.
The Chrome data reduction feature relies on HTTP headers to
work correctly and efficiently with the data reduction
proxy. This include both standard HTTP headers (like Via)
and custom headers (like Chrome-Proxy). Tampering on these
headers could lead to miserable user experience, taking 10s
to load some pages, for example.
In the past, we have seen such headers being stripped by
middle box proxies (for example, the WWW-Authenticate header
was stripped by some carrier). It has been known that mobile
carriers are doing HTTP traffic optimizations. We also want
to know whether mobile carriers are trying to "optimize" the
already optimized data reduction proxy response body, which
might lead to higher cost to users.
We propose a mechanism in Chromium to enable us to learn the
scale and the types of such tampers. In short, the mechanism
will check whether a predefined set of HTTP response headers
and the response body have been changed in a way that could
affect the data reduction proxy. It will detect such changes
by using pre-calculated header (and probably content) hashes
sent by the server. Chromium will report through UMA the
count of each tamper types has happened. This will only be
enabled for a fraction of the data reduction proxy users.
BUG=381907
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288492
Patch Set 1 : move all UMA reports to main; rewrite unittest, with comments added. #Patch Set 2 : #
Total comments: 9
Patch Set 3 : address Bolian's comments #
Total comments: 38
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #
Total comments: 80
Patch Set 9 : #Patch Set 10 : addressed most of comments #
Total comments: 62
Patch Set 11 : Expose only 1 public function of the class. #Patch Set 12 : unittest modified. #Patch Set 13 : #
Total comments: 26
Patch Set 14 : fixed array size issue. #Patch Set 15 : #Patch Set 16 : #
Total comments: 161
Patch Set 17 : Move modification on _headers.h/cc to another CL. #
Total comments: 82
Patch Set 18 : #
Total comments: 72
Patch Set 19 : #
Total comments: 27
Patch Set 20 : #
Total comments: 7
Patch Set 21 : #
Total comments: 4
Patch Set 22 : #
Total comments: 6
Patch Set 23 : #
Total comments: 154
Patch Set 24 : #Patch Set 25 : #
Total comments: 38
Patch Set 26 : #
Total comments: 2
Patch Set 27 : #
Total comments: 42
Patch Set 28 : #
Total comments: 9
Patch Set 29 : #
Total comments: 8
Patch Set 30 : #Patch Set 31 : #Patch Set 32 : #Patch Set 33 : #Patch Set 34 : #Patch Set 35 : #Patch Set 36 : #Patch Set 37 : #Patch Set 38 : #Patch Set 39 : #Patch Set 40 : #Patch Set 41 : fix size_t issue #Patch Set 42 : #Patch Set 43 : fix histograms #Messages
Total messages: 53 (0 generated)
https://codereview.chromium.org/338483002/diff/430001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/430001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:77: "DataReductionProxy.TamperDetectHTTPSChromeProxy" : The histogram name should be clear whether it has been tampered and what headers, lets call them DataReuctionProxy.HTTPSHeaderTampered_ChromeProxy, DataReuctionProxy.HTTPSHeaderTampered_Via, DataReuctionProxy.HTTPSHeaderTampered_ContentLength, DataReuctionProxy.HTTPSHeaderTampered_Other, Add similar set for HTTP case. https://codereview.chromium.org/338483002/diff/430001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/430001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:11: bool HandleF1(const std::string, const net::HttpResponseHeaders*, Have more meaningful func names, CheckChromeProxyHeader,etc, for example. https://codereview.chromium.org/338483002/diff/430001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:11: bool HandleF1(const std::string, const net::HttpResponseHeaders*, Put these funcs in data_reduction_proxy namespace, and comment "exported for unit testing". https://codereview.chromium.org/338483002/diff/430001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc (right): https://codereview.chromium.org/338483002/diff/430001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:37: const char* raw_headers; comment the fields and the test strategy used here. Why not have a set of headers from flywheel, and another set of headers received by the browser? https://codereview.chromium.org/338483002/diff/430001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:38: std::string f1_fw; first don't use the word 'flywheel', the public name is 'data reduction proxy', second, write it out not abbr. https://codereview.chromium.org/338483002/diff/430001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:86: "HTTP/1.1 202 Accepted \n" Consider add a one line comment for each case. https://codereview.chromium.org/338483002/diff/430001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:502: TEST(DataReductionProxyTamperDetectTest, f3) { f3 : Camel case the test name. https://codereview.chromium.org/338483002/diff/430001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:993: TEST(DataReductionProxyTamperDetectTest, parsing) { s/parsing/Parsing.
https://codereview.chromium.org/338483002/diff/560001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/560001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:50: bool CheckHeaderChromeProxy(const std::string f1, I think it is better to return true in case of positive (tamper detected). The code reading flows better that way. https://codereview.chromium.org/338483002/diff/560001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/560001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:13: // Eported for unittest s/Eported/Exported https://codereview.chromium.org/338483002/diff/560001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:14: bool CheckHeaderChromeProxy(const std::string, Comment public funcs. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... in particular and http://www.chromium.org/developers/coding-style in general. https://codereview.chromium.org/338483002/diff/560001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc (right): https://codereview.chromium.org/338483002/diff/560001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:39: bool expected_result; What does the expected_result mean? Tampered or not? Name it expected_tampered or something like. Variable names should be self-explaining. If names are not enough, add comments. https://codereview.chromium.org/338483002/diff/560001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:99: "aaxxx=xxx,aut=aauutthh,bbbloc=1,bbbypas=0,", Can you address my previous comment about why not have two sets of headers? the fp values can be computed inside the test, right?
https://codereview.chromium.org/338483002/diff/560001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/560001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:50: bool CheckHeaderChromeProxy(const std::string f1, On 2014/06/25 18:55:56, bolian wrote: > I think it is better to return true in case of positive (tamper detected). The > code reading flows better that way. Done. https://codereview.chromium.org/338483002/diff/560001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/560001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:13: // Eported for unittest On 2014/06/25 18:55:56, bolian wrote: > s/Eported/Exported Done. https://codereview.chromium.org/338483002/diff/560001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:14: bool CheckHeaderChromeProxy(const std::string, On 2014/06/25 18:55:56, bolian wrote: > Comment public funcs. See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > in particular and http://www.chromium.org/developers/coding-style in general. Done. https://codereview.chromium.org/338483002/diff/560001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc (right): https://codereview.chromium.org/338483002/diff/560001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:99: "aaxxx=xxx,aut=aauutthh,bbbloc=1,bbbypas=0,", On 2014/06/25 18:55:56, bolian wrote: > Can you address my previous comment about why not have two sets of headers? the > fp values can be computed inside the test, right? Right now the fp value is manually input, then calculate MD5 on that manually input value if necessary. Compare to input header and generate fp in unittest, the good part is we don't need to implement fp generating process again in Chromium unittest, this has following advantages: 1. no need to write code for generating fp, of course :) 2. no need to worry about the correctness of generating fp process 3. can check edge cases, which means we don't assume the fp Chromium received makes sense, it can be anything, say a wrongly formatted one due to following reasons: a. FW's generating fp process is wrong b. Middlebox changed the fp in above cases, the unittest needs to guarantee that the program doesn't crash We can discuss more about this.
So far for now, on-boarding flight. I will have to look at the test in detail later. For next round, please add bengr@chromium.org https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:45: return std::string((char*)new_digest.a, 16); Is this constant defined somewhere? https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:52: // I call fingerprint from FW, received_fingerprint; the fingerprint Don't mention FW. I think readers want to see more comments about the mechanism used here. You can have a header doc in the cc file as high level description, and then func doc about each one here. Be concise in both cases. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:56: LOG(WARNING) << "Xing f1 base64 decode fails"; // remove later It is time to remove now. :) https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:60: // need to get all the values of Chrome-Proxy, remove value fp=xxx, Capitalize the first letter and add period at the end of the sentence. Here and everywhere else. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:74: return received_fingerprint.compare(actual_fingerprint) != 0; return received_fingerprint != actual_fingerprint; https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:93: // exist_chrome = true; Use HasDataReductionProxyViaHeader in components/data_reduction_proxy/common/data_reduction_proxy_headers.h. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:110: std::string(it.value_begin(), it.value_end()), it.value()? https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:117: std::string header_values = ""; Remove the empty string literal. That is the default value. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:144: if (!base::StringToInt(actual_content_length_, &actual_content_length)) You can just compare the strings. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:161: std::vector<std::string> values = GetHeaderValues(headers, "Chrome-Proxy"); I think we should have a class that wraps these intermediate result so that you don't parse it again, and also wraps the helper funcs as private. Use FRIEND_TEST_ALL_PREFIXES to expose them to test. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:166: if (values[i].find("fp=") == 0) { define fp= and other keywords as constants. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:175: // delimiter "|", separate fp= string: fp=f1|f2|f3|f4 What is the assumption here? All four values must be appear and in this order? Do you always have three delimiters even if some values are missing? I think that is hard to maintain, why not make each field a name-value pair? https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:175: // delimiter "|", separate fp= string: fp=f1|f2|f3|f4 I think it might be better to encode the whole value of fp and add some basic integrity check. If fx value is tampered somehow, we can know it. Otherwise, we will report false positive for x header. WDYT? https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:176: net::HttpUtil::ValuesIterator it(values[fingerprint_index].begin() + 3, compute 3 from the keyword "fp=" you defined. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:186: "DataReductionProxy.HTTPSHeaderTampereDetected" : The name sounds like tamper happened. How about ...HeaderTamperDetection https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:201: LOG(WARNING)<<"Xing f1 not equal"; Time to remove all these. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:19: bool CheckHeaderChromeProxy(const std::string, const std::string& here and others. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc (right): https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:20: base::MD5Final(&new_digest, &context); use GetMD5 in your cc file. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:59: EXPECT_EQ(test.expected_result, checkFuncs[fingerprintNumber]( use enum instead of int for func index.
https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:92: if (vias[i].find("Chrome") != std::string::npos) { Use the real header value. Expose that from components/data_reduction_proxy/common/data_reduction_proxy_headers.cc https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc (right): https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:37: const char* raw_header; s/const char*/std::string/ ? https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:39: bool expected_result; the var name is not clear, whether you expect tampered or not tampered. Rename it expect_tampered. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:54: CheckHeader checkFuncs[] = {&data_reduction_proxy::CheckHeaderChromeProxy, this table should be outside the func. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:76: "HTTP/1.1 202 Accepted \n" why 202? https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:77: "Chrome-Proxy: aut=aauutthh,fp=123,bbbypas=0,aaxxx=xxx,bbbloc=1\n", Either 1) Make it real. replace 123 with the value computed in your test. You can use %s here and sprintf. or 2) Comment that the fp value here is not the one being tested because ... https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:292: test[i].received_fingerprint = don't rewrite the test field. Rename the field and pass the fingerprint to your test helper. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:304: "Via: a, b, c, Chrome Proxy\n", use the actual data reduction proxy via header value. Expose the value from components/data_reduction_proxy/common/data_reduction_proxy_headers.cc and use it here.
https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:74: return received_fingerprint.compare(actual_fingerprint) != 0; On 2014/06/27 00:49:02, bolian wrote: > return received_fingerprint != actual_fingerprint; Done. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:110: std::string(it.value_begin(), it.value_end()), On 2014/06/27 00:49:02, bolian wrote: > it.value()? Done. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:117: std::string header_values = ""; On 2014/06/27 00:49:02, bolian wrote: > Remove the empty string literal. That is the default value. Done. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:166: if (values[i].find("fp=") == 0) { On 2014/06/27 00:49:02, bolian wrote: > define fp= and other keywords as constants. Done. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:176: net::HttpUtil::ValuesIterator it(values[fingerprint_index].begin() + 3, On 2014/06/27 00:49:02, bolian wrote: > compute 3 from the keyword "fp=" you defined. Done. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:186: "DataReductionProxy.HTTPSHeaderTampereDetected" : On 2014/06/27 00:49:02, bolian wrote: > The name sounds like tamper happened. How about ...HeaderTamperDetection Done. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:19: bool CheckHeaderChromeProxy(const std::string, On 2014/06/27 00:49:02, bolian wrote: > const std::string& here and others. Done. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc (right): https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:37: const char* raw_header; On 2014/06/27 02:16:58, bolian wrote: > s/const char*/std::string/ ? Done. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:39: bool expected_result; On 2014/06/27 02:16:58, bolian wrote: > the var name is not clear, whether you expect tampered or not tampered. Rename > it expect_tampered. Done. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:54: CheckHeader checkFuncs[] = {&data_reduction_proxy::CheckHeaderChromeProxy, On 2014/06/27 02:16:58, bolian wrote: > this table should be outside the func. Done. https://codereview.chromium.org/338483002/diff/580001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:59: EXPECT_EQ(test.expected_result, checkFuncs[fingerprintNumber]( On 2014/06/27 00:49:02, bolian wrote: > use enum instead of int for func index. Done.
Here are a few comments to get you started. https://codereview.chromium.org/338483002/diff/800001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/338483002/diff/800001/components/components_t... components/components_tests.gyp:78: 'data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc', alphabetize https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... File components/data_reduction_proxy.gypi (right): https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy.gypi:39: 'data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc', alphabetize https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:7: // which maybe break correct communication between Chrome and data reduction and the data... https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:71: std::string GetMD5(const std::string &input) { Look at https://code.google.com/p/chromium/codesearch#chromium/src/components/data_re... for how we use MD5. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:117: values.erase(values.begin() + (i--)); Separate out the decrement and explain why it is needed. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:119: else if (values[i].find(kTamperDetectFingerprint) == 0) Add curly braces. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:225: if ((this->*funcs.check_tamper_func)(fingerprint)) use base::Callback https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:288: const std::string& fingerprint) { indent 4. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:303: std::vector<std::string> values = GetHeaderValues(response_headers, move GetHeaderValues to next line and indent 4. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:349: LOG(WARNING) << "xing can't convert"; remove. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:394: remove blank line. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:10: #include "net/http/http_response_headers.h" add: namespace net { class HttpResponseHeaders; } https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:14: // There are two fingerprints will be added to Chrome-Proxy header. Remove "There are" https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:22: // In fingerprint starts with |kTamperDetectFingerprint|, it contains multiple "In" --> "If |kTamperDetectFingerprint| contains https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:24: // value. Currently we have 3 of fingerprints and thus 3 tags, defined below. Currently --> Three fingerprints and their respective tags are defined below. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:25: const char kTamperDetectFingerprintVia[] = "via"; Are these needed outside the class? If not, defined them in an anonymous namespace in the .cc https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:29: // Macro for UMA report. Why is this in the .h? https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:35: #define UMA_REPORT(is_secure_scheme, HTTP_histogram, HTTPS_histogram, mcc_mnc) \ Unless you've seen this pattern in other UMA reporting, I'd name these http_histogram, and https_histogram. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:37: if (is_secure_scheme) { \ Is the scheme always https if |is_secure_scheme| is true? If so, rename this |scheme_is_https|. Otherwise, add a comment that explains the exceptions. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:48: std::string GetMD5(const std::string& input); I don't think you should expose such a function to the namespace. Can this be a virtual protected method of the class? https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:53: std::vector<std::string> GetHeaderValues( Do you really need this? HttpResponseHeaders parses the headers into basically the same thing. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:64: bool ContainsTamperDetectFingerprints(std::vector<std::string>& values, Don't use non-const references. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:68: // The main function for detecting tamper. Fill out comments to the 80-char limit. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:100: bool CheckHeaderChromeProxy(const std::string&); Can this function be const? What about others below? https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:102: // For Via header tamper detection... Use complete sentences in comments. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:107: bool CheckHeaderVia(const std::string&); What does this function do? https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:143: typedef bool (DataReductionProxyTamperDetect::*CheckTamper)( Why do you need function pointers? https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:152: CheckTamper check_tamper_func; variable names should not be abbreviated. E.g., this should be check_tamper_function. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:158: const net::HttpResponseHeaders* response_headers; Add a blank line after each variable. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc (right): https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:126: "HTTP/1.1 200 OK \n" indent 2, not 4, here and below. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:357: GetEncoded(test[i].received_fingerprint); move up a line. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:414: remove blank line here and in all other cases. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.h (right): https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.h:18: const char kDataReductionProxyViaValue[] = "Chrome-Compression-Proxy"; The definition should not be in the header. See switches for examples. Also note that the data reduction proxy uses the via header "Chrome Compression Proxy" so you should accept both.
https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:102: bool ContainsTamperDetectFingerprints(std::vector<std::string>& values, Don't repeat func doc from the .h file. For exposed APIs, the .h tells are meant for the users of an API. The .cc doc should be used to note implementation details if anything worth noting. Don't document something that is obvious, e.g. anything readers can tell from the func name or the parameter name - which also means use self-explaining func name and parameter name if possible). Apply this comment to all other func doc here. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:134: // Get all the values of Chrome-Proxy header. rm doc here. This is obvious from reading the code. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:153: // Initialize tamper detect object. rm. Useless comment. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:189: // Constructor of DataReductionProxyTamperDetect class. delete "// Constructor of DataReductionProxyTamperDetect class." https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:39: UMA_HISTOGRAM_SPARSE_SLOWLY(HTTPS_histogram "_Total", 0); \ Should use UMA_HISTOGRAM_COUNTS here. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:42: UMA_HISTOGRAM_SPARSE_SLOWLY(HTTP_histogram "_Total", 0); \ same here, UMA_HISTOGRAM_COUNTS https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:94: // For Chrome-Proxy header tamper detection... Let's simplify and reformat the doc of this func and you can apply it to other similar func doc. How about // Returns true if Chrome-Proxy has been tampered. bool CheckHeaderChromeProxy(const std::string& received_fingerprint) const; I think this covers everything you want to say. The named parameter should be clear what the it is.
https://codereview.chromium.org/338483002/diff/800001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/338483002/diff/800001/components/components_t... components/components_tests.gyp:78: 'data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc', On 2014/07/02 17:30:59, bengr1 wrote: > alphabetize Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... File components/data_reduction_proxy.gypi (right): https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy.gypi:39: 'data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc', On 2014/07/02 17:30:59, bengr1 wrote: > alphabetize Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:7: // which maybe break correct communication between Chrome and data reduction On 2014/07/02 17:30:59, bengr1 wrote: > and the data... Not sure if I get it right, change to "break correct communication and data transfer"... https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:71: std::string GetMD5(const std::string &input) { On 2014/07/02 17:30:59, bengr1 wrote: > Look at > https://code.google.com/p/chromium/codesearch#chromium/src/components/data_re... > for how we use MD5. We can discuss about this, right now I'm using base64 so comparison is different, I'm comparing raw binary string. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:102: bool ContainsTamperDetectFingerprints(std::vector<std::string>& values, On 2014/07/02 23:47:37, bolian wrote: > Don't repeat func doc from the .h file. For exposed APIs, the .h tells are > meant for the users of an API. The .cc doc should be used to note implementation > details if anything worth noting. Don't document something that is obvious, e.g. > anything readers can tell from the func name or the parameter name - which also > means use self-explaining func name and parameter name if possible). > > Apply this comment to all other func doc here. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:117: values.erase(values.begin() + (i--)); On 2014/07/02 17:30:59, bengr1 wrote: > Separate out the decrement and explain why it is needed. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:119: else if (values[i].find(kTamperDetectFingerprint) == 0) On 2014/07/02 17:30:59, bengr1 wrote: > Add curly braces. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:134: // Get all the values of Chrome-Proxy header. On 2014/07/02 23:47:37, bolian wrote: > rm doc here. This is obvious from reading the code. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:153: // Initialize tamper detect object. On 2014/07/02 23:47:37, bolian wrote: > rm. Useless comment. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:189: // Constructor of DataReductionProxyTamperDetect class. On 2014/07/02 23:47:37, bolian wrote: > delete "// Constructor of DataReductionProxyTamperDetect class." Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:225: if ((this->*funcs.check_tamper_func)(fingerprint)) On 2014/07/02 17:30:59, bengr1 wrote: > use base::Callback Changed to switch / enum. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:288: const std::string& fingerprint) { On 2014/07/02 17:30:59, bengr1 wrote: > indent 4. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:303: std::vector<std::string> values = GetHeaderValues(response_headers, On 2014/07/02 17:30:59, bengr1 wrote: > move GetHeaderValues to next line and indent 4. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:349: LOG(WARNING) << "xing can't convert"; On 2014/07/02 17:30:59, bengr1 wrote: > remove. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:394: On 2014/07/02 17:30:59, bengr1 wrote: > remove blank line. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:10: #include "net/http/http_response_headers.h" On 2014/07/02 17:31:01, bengr1 wrote: > add: > namespace net { > class HttpResponseHeaders; > } Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:14: // There are two fingerprints will be added to Chrome-Proxy header. On 2014/07/02 17:31:00, bengr1 wrote: > Remove "There are" Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:22: // In fingerprint starts with |kTamperDetectFingerprint|, it contains multiple On 2014/07/02 17:31:00, bengr1 wrote: > "In" --> "If |kTamperDetectFingerprint| contains Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:24: // value. Currently we have 3 of fingerprints and thus 3 tags, defined below. On 2014/07/02 17:30:59, bengr1 wrote: > Currently --> Three fingerprints and their respective tags are defined below. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:25: const char kTamperDetectFingerprintVia[] = "via"; On 2014/07/02 17:31:00, bengr1 wrote: > Are these needed outside the class? If not, defined them in an anonymous > namespace in the .cc Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:29: // Macro for UMA report. On 2014/07/02 17:30:59, bengr1 wrote: > Why is this in the .h? Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:35: #define UMA_REPORT(is_secure_scheme, HTTP_histogram, HTTPS_histogram, mcc_mnc) \ On 2014/07/02 17:31:00, bengr1 wrote: > Unless you've seen this pattern in other UMA reporting, I'd name these > http_histogram, and https_histogram. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:37: if (is_secure_scheme) { \ On 2014/07/02 17:31:00, bengr1 wrote: > Is the scheme always https if |is_secure_scheme| is true? If so, rename this > |scheme_is_https|. Otherwise, add a comment that explains the exceptions. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:39: UMA_HISTOGRAM_SPARSE_SLOWLY(HTTPS_histogram "_Total", 0); \ On 2014/07/02 23:47:37, bolian wrote: > Should use UMA_HISTOGRAM_COUNTS here. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:42: UMA_HISTOGRAM_SPARSE_SLOWLY(HTTP_histogram "_Total", 0); \ On 2014/07/02 23:47:37, bolian wrote: > same here, UMA_HISTOGRAM_COUNTS Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:48: std::string GetMD5(const std::string& input); On 2014/07/02 17:31:00, bengr1 wrote: > I don't think you should expose such a function to the namespace. Can this be a > virtual protected method of the class? Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:53: std::vector<std::string> GetHeaderValues( On 2014/07/02 17:31:00, bengr1 wrote: > Do you really need this? HttpResponseHeaders parses the headers into basically > the same thing. Discussed with you, reason is I need to sort the values. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:64: bool ContainsTamperDetectFingerprints(std::vector<std::string>& values, The function checks whether there is chrome-proxy fingerprint field, if there is, remove it and save other values for future usage to avoid parse the header values twice. Let me know if such design is OK, if not, I'll figure out another way. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:68: // The main function for detecting tamper. On 2014/07/02 17:31:00, bengr1 wrote: > Fill out comments to the 80-char limit. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:94: // For Chrome-Proxy header tamper detection... On 2014/07/02 23:47:37, bolian wrote: > Let's simplify and reformat the doc of this func and you can apply it to other > similar func doc. > How about > > // Returns true if Chrome-Proxy has been tampered. > bool CheckHeaderChromeProxy(const std::string& received_fingerprint) const; > > > I think this covers everything you want to say. The named parameter should be > clear what the it is. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:100: bool CheckHeaderChromeProxy(const std::string&); On 2014/07/02 17:31:00, bengr1 wrote: > Can this function be const? What about others below? Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:102: // For Via header tamper detection... On 2014/07/02 17:31:00, bengr1 wrote: > Use complete sentences in comments. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:107: bool CheckHeaderVia(const std::string&); On 2014/07/02 17:30:59, bengr1 wrote: > What does this function do? Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:143: typedef bool (DataReductionProxyTamperDetect::*CheckTamper)( On 2014/07/02 17:31:00, bengr1 wrote: > Why do you need function pointers? removed. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:152: CheckTamper check_tamper_func; On 2014/07/02 17:31:00, bengr1 wrote: > variable names should not be abbreviated. E.g., this should be > check_tamper_function. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:158: const net::HttpResponseHeaders* response_headers; On 2014/07/02 17:31:00, bengr1 wrote: > Add a blank line after each variable. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc (right): https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:126: "HTTP/1.1 200 OK \n" On 2014/07/02 17:31:01, bengr1 wrote: > indent 2, not 4, here and below. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:357: GetEncoded(test[i].received_fingerprint); On 2014/07/02 17:31:01, bengr1 wrote: > move up a line. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:414: On 2014/07/02 17:31:01, bengr1 wrote: > remove blank line here and in all other cases. Done. https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.h (right): https://codereview.chromium.org/338483002/diff/800001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.h:18: const char kDataReductionProxyViaValue[] = "Chrome-Compression-Proxy"; On 2014/07/02 17:31:01, bengr1 wrote: > The definition should not be in the header. See switches for examples. > > Also note that the data reduction proxy uses the via header "Chrome Compression > Proxy" so you should accept both. Done.
https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:14: //namespace net { Remove dead code. Though I think you need this for below. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:20: // Two fingerprints will be added to Chrome-Proxy header. Fill the comment out to 80 characters on each line. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:22: // fingerprint for Chrome-Proxy header. for the https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:31: extern const char kTamperDetectFingerprintVia[]; Do these really need to be visible to the entire namespace? It looks like you only need these in the .cc and in your tests. I'd suggest putting these in an anonymous namespace in the .cc and not using these directly from your tests. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:38: // If there is, return true, and save Chrome-Proxy header's fingerprint to If there is --> If it does https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:42: // Return false if there is no Chrome-Proxy header's fingerprint found. header's -> header https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:43: bool ContainsTamperDetectFingerprints(std::vector<std::string>& values, Make this a private or protected static member and friend your tests. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:44: std::string& chrome_proxy_fingerprint, Do not use non-const references. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:47: // The main function for detecting tamper. It takes two parameters as input, tamper -> tampering. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:49: // 2. a boolean variable indicates whether the connection variable indicates -> variable that indicates https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:51: // For such response, the function checks whether there is a tamper detect What is "such response"? Be clearer https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:57: enum FingerprintCode { CHROMEPROXY, VIA, OTHERHEADERS, Put each value on a separate line. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:60: // The class for detecting tamper. tampering. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:61: // It wraps up the functionalities for tamper detection. Remove this line. It doesn't add anything. Try to add a more descriptive comment. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:70: DataReductionProxyTamperDetect(const net::HttpResponseHeaders*, const bool, provide variable names. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:71: const unsigned, std::vector<std::string>*); Don't make the bool and the unsigned const. I don't see how that helps. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:74: // Returns true if Chrome-Proxy has been tampered. if Chrome-Proxy --> if the Chrome-Proxy header https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:74: // Returns true if Chrome-Proxy has been tampered. what is the parameter? https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:75: bool CheckHeaderChromeProxy(const std::string&) const; variable name https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:75: bool CheckHeaderChromeProxy(const std::string&) const; Rename as IsChromeProxyHeaderModified(const std::string& header_value) or similar, e.g., IsChromeProxyHeaderTampered() https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:77: // Returns true if Via has been tampered. if Via --> if the Via header https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:78: bool CheckHeaderVia(const std::string&) const; variable name https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:78: bool CheckHeaderVia(const std::string&) const; rename as IsViaHeaderModified(const std::string& header_value) https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:80: void ReportHeaderVia() const; suggest ReportViaHeaderTamperedUMA() https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:83: bool CheckHeaderOtherHeaders(const std::string&) const; What is the parameter? The concatenation of all other headers? That's weird. Also, suggest: AreOtherHeadersModified() https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:85: void ReportHeaderOtherHeaders() const; ReportOtherHeadersTamperedUMA() https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:90: void ReportHeaderContentLength() const; ReportContentLengthHeaderTamperedUMA() https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:93: static std::string ValuesToSortedString(std::vector<std::string> &values); Why does this (and many of these functions) need to be public? https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:95: // Return MD5 hash value for a given string |input|. Return --> Returns Add to the comment why you can't just use the MD5 hash method directly. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:104: FingerprintCode GetFingerprintCode(const std::string&); provide a variable name here and everywhere.
https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:14: //namespace net { On 2014/07/07 17:01:33, bengr1 wrote: > Remove dead code. Though I think you need this for below. Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:20: // Two fingerprints will be added to Chrome-Proxy header. On 2014/07/07 17:01:34, bengr1 wrote: > Fill the comment out to 80 characters on each line. Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:22: // fingerprint for Chrome-Proxy header. On 2014/07/07 17:01:33, bengr1 wrote: > for the Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:31: extern const char kTamperDetectFingerprintVia[]; On 2014/07/07 17:01:35, bengr1 wrote: > Do these really need to be visible to the entire namespace? It looks like you > only need these in the .cc and in your tests. I'd suggest putting these in an > anonymous namespace in the .cc and not using these directly from your tests. Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:38: // If there is, return true, and save Chrome-Proxy header's fingerprint to On 2014/07/07 17:01:33, bengr1 wrote: > If there is --> If it does Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:42: // Return false if there is no Chrome-Proxy header's fingerprint found. On 2014/07/07 17:01:34, bengr1 wrote: > header's -> header Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:44: std::string& chrome_proxy_fingerprint, On 2014/07/07 17:01:34, bengr1 wrote: > Do not use non-const references. Will discuss with you. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:47: // The main function for detecting tamper. It takes two parameters as input, On 2014/07/07 17:01:33, bengr1 wrote: > tamper -> tampering. Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:49: // 2. a boolean variable indicates whether the connection On 2014/07/07 17:01:34, bengr1 wrote: > variable indicates -> variable that indicates Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:51: // For such response, the function checks whether there is a tamper detect On 2014/07/07 17:01:34, bengr1 wrote: > What is "such response"? Be clearer Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:57: enum FingerprintCode { CHROMEPROXY, VIA, OTHERHEADERS, On 2014/07/07 17:01:33, bengr1 wrote: > Put each value on a separate line. Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:60: // The class for detecting tamper. On 2014/07/07 17:01:34, bengr1 wrote: > tampering. Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:61: // It wraps up the functionalities for tamper detection. On 2014/07/07 17:01:35, bengr1 wrote: > Remove this line. It doesn't add anything. Try to add a more descriptive > comment. Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:70: DataReductionProxyTamperDetect(const net::HttpResponseHeaders*, const bool, On 2014/07/07 17:01:33, bengr1 wrote: > provide variable names. Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:71: const unsigned, std::vector<std::string>*); On 2014/07/07 17:01:34, bengr1 wrote: > Don't make the bool and the unsigned const. I don't see how that helps. Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:74: // Returns true if Chrome-Proxy has been tampered. On 2014/07/07 17:01:33, bengr1 wrote: > what is the parameter? Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:74: // Returns true if Chrome-Proxy has been tampered. On 2014/07/07 17:01:33, bengr1 wrote: > what is the parameter? Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:74: // Returns true if Chrome-Proxy has been tampered. On 2014/07/07 17:01:33, bengr1 wrote: > what is the parameter? Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:74: // Returns true if Chrome-Proxy has been tampered. On 2014/07/07 17:01:34, bengr1 wrote: > if Chrome-Proxy --> if the Chrome-Proxy header Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:75: bool CheckHeaderChromeProxy(const std::string&) const; On 2014/07/07 17:01:35, bengr1 wrote: > variable name Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:75: bool CheckHeaderChromeProxy(const std::string&) const; On 2014/07/07 17:01:35, bengr1 wrote: > variable name Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:77: // Returns true if Via has been tampered. On 2014/07/07 17:01:34, bengr1 wrote: > if Via --> if the Via header Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:78: bool CheckHeaderVia(const std::string&) const; On 2014/07/07 17:01:34, bengr1 wrote: > rename as IsViaHeaderModified(const std::string& header_value) Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:78: bool CheckHeaderVia(const std::string&) const; On 2014/07/07 17:01:35, bengr1 wrote: > variable name Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:78: bool CheckHeaderVia(const std::string&) const; On 2014/07/07 17:01:34, bengr1 wrote: > rename as IsViaHeaderModified(const std::string& header_value) Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:80: void ReportHeaderVia() const; On 2014/07/07 17:01:34, bengr1 wrote: > suggest ReportViaHeaderTamperedUMA() Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:83: bool CheckHeaderOtherHeaders(const std::string&) const; On 2014/07/07 17:01:34, bengr1 wrote: > What is the parameter? The concatenation of all other headers? That's weird. > > Also, suggest: AreOtherHeadersModified() Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:85: void ReportHeaderOtherHeaders() const; On 2014/07/07 17:01:35, bengr1 wrote: > ReportOtherHeadersTamperedUMA() Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:90: void ReportHeaderContentLength() const; On 2014/07/07 17:01:34, bengr1 wrote: > ReportContentLengthHeaderTamperedUMA() Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:93: static std::string ValuesToSortedString(std::vector<std::string> &values); On 2014/07/07 17:01:33, bengr1 wrote: > Why does this (and many of these functions) need to be public? Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:95: // Return MD5 hash value for a given string |input|. On 2014/07/07 17:01:34, bengr1 wrote: > Return --> Returns > > Add to the comment why you can't just use the MD5 hash method directly. Done. https://codereview.chromium.org/338483002/diff/840001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:104: FingerprintCode GetFingerprintCode(const std::string&); On 2014/07/07 17:01:33, bengr1 wrote: > provide a variable name here and everywhere. Done.
https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:162: case CHROMEPROXY: Replace the last two cases with case default: NOTREACHED(); break; https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:198: if (!base::Base64Decode(fingerprint, &received_fingerprint)) This should be treated as tampered, right? https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:220: // TODO(xingx): change 2 to array size of such constant array. why not do it now? https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:257: if (!(it.GetNext() && Will the server always send "oh=" even if the header list is empty? Base64Decode should always succeeded right? Assert that with NOTREACHED. https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:368: bool DataReductionProxyTamperDetect::ContainsTamperDetectFingerprints( Rename this. The name sounds like it won't change |values|, but it does. https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:374: if ((*values)[i].find(data_reduction_proxy:: s/data_reduction_proxy::// This is in the same namespace. https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:384: --i; Don't change the vector while iterating. Do it outside. https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:37: static void CheckResponseFingerprint(const net::HttpResponseHeaders*, bool); Name the parameters. https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:68: // Report UMA for tampering of the Via header. s/Report/Reports/ here and below. https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:71: // Returns true if other headers (a list of headers) have been tampered. How about // Returns true if a list of server defined headers have been tampered. https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:81: // Return fingerprint code (enum) for the given fingerprint tag. Returns https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:109: const bool is_secure_scheme; // If true, the connection to the data reduction proxy is over HTTPS. https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:109: const bool is_secure_scheme; Add trailing "_" to all private member vars.
https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:162: case CHROMEPROXY: On 2014/07/09 22:51:06, bolian wrote: > Replace the last two cases with > case default: > NOTREACHED(); > break; Done. https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:198: if (!base::Base64Decode(fingerprint, &received_fingerprint)) On 2014/07/09 22:51:05, bolian wrote: > This should be treated as tampered, right? That's right, my mistake. https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:220: // TODO(xingx): change 2 to array size of such constant array. On 2014/07/09 22:51:06, bolian wrote: > why not do it now? Need to figure it out... https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:257: if (!(it.GetNext() && On 2014/07/09 22:51:06, bolian wrote: > Will the server always send "oh=" even if the header list is empty? > Base64Decode should always succeeded right? Assert that with NOTREACHED. Yes. Done. https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:368: bool DataReductionProxyTamperDetect::ContainsTamperDetectFingerprints( On 2014/07/09 22:51:06, bolian wrote: > Rename this. The name sounds like it won't change |values|, but it does. Done. https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:374: if ((*values)[i].find(data_reduction_proxy:: On 2014/07/09 22:51:06, bolian wrote: > s/data_reduction_proxy::// > This is in the same namespace. Done. https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:384: --i; On 2014/07/09 22:51:06, bolian wrote: > Don't change the vector while iterating. Do it outside. Done. https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:37: static void CheckResponseFingerprint(const net::HttpResponseHeaders*, bool); On 2014/07/09 22:51:06, bolian wrote: > Name the parameters. Done. https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:68: // Report UMA for tampering of the Via header. On 2014/07/09 22:51:06, bolian wrote: > s/Report/Reports/ here and below. Done. https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:71: // Returns true if other headers (a list of headers) have been tampered. On 2014/07/09 22:51:06, bolian wrote: > How about > > // Returns true if a list of server defined headers have been tampered. Done. https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:81: // Return fingerprint code (enum) for the given fingerprint tag. On 2014/07/09 22:51:06, bolian wrote: > Returns Done. https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:109: const bool is_secure_scheme; On 2014/07/09 22:51:06, bolian wrote: > // If true, the connection to the data reduction proxy is over HTTPS. Done. https://codereview.chromium.org/338483002/diff/1020001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:109: const bool is_secure_scheme; On 2014/07/09 22:51:06, bolian wrote: > // If true, the connection to the data reduction proxy is over HTTPS. Done.
Here are some more comments. I haven't finished but am tired. This is too large a change for one CL. Please do not create such large CLs in the future. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:5: // This file implements the tamper detection logic, where we want to detect This comment should be moved to the .h. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:11: // 1. Data reduction proxy selects the requests we want to detect tamper; The data... https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:12: // for the selected ones, data reduction proxy generates a series of the data reduction... https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:13: // fingerprints of the response, and append them to the Chrome-Proxy header; of -> for appends it https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:14: // 2. At Chrome client side, once it sees such fingerprints, it uses the When Chrome sees such fingerprints in response headers, ... https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:15: // same method of data reduction proxy to generate the fingerprints on of --> as the remove "on the response it receives" compare it --> and compares them to the fingerprints in the response, to see if ... https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:21: // Right now we have 4 fingerprints (listed below). Chrome first check the Right now we --> Four fingerprints are defined (listed below). checks of Chrome-Proxy -> of the Chrome-Proxy If Chrome-Proxy -> If the Chrome-Proxy https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:23: // tampered, then other fingerprints would not be checked; if not, Chrome This is Chromium code too. In general you don't have to say Chrome is doing anything. That's implied by this being part of the Chromium code base. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:24: // parses the rest of the fingerprints and check whether there is tampering checks https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:37: // Chrome reports tamper or not information for each fingerprint to UMA. In Up above you seem to suggest that UMA is only collected when the response has been tampered with, which is different from what you are saying here. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:46: #include <algorithm> alphabetize https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:55: remove the blank line https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:60: // Macro for UMA report. First report to either |https_histogram| or report -> reporting https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:61: // |http_histogram| depends on |scheme_is_https|, with carrier ID as bucket depends -> depending https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:62: // |mcc_mnc_|. Then report to http(s)_histogram_Total, which counts the total carrier_id_ https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:62: // |mcc_mnc_|. Then report to http(s)_histogram_Total, which counts the total Is that the name of the histogram? Clarify. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:64: #define UMA_REPORT(scheme_is_https, http_histogram, https_histogram, mcc_mnc) \ Why is this a macro. Make this a function. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:77: namespace { I see no reason for this anonymous namespace to be inside the data_reduction_proxy namespace. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:78: // Two fingerprints will be added to Chrome-Proxy header. One starts with I thought you were checking 4 things, not 2. Explain. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:94: void DataReductionProxyTamperDetect::CheckResponseFingerprint( add: "// static" above this line. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:98: std::vector<std::string> values = DataReductionProxyTamperDetect:: You don't need the DataReductionProxyTamperDetect:: https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:106: // Check if there are fingerprints (and thus need to detect tamper). tampering https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:107: if (!GetTamperDetectFingerprints(&values, rename: GetTamperDetectionFingerprints, or just GetFingerprints() https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:112: // Found tamper detect request field. detection https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:114: unsigned mcc_mnc = 0; carrier_id https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:115: base::StringToUint(net::android::GetTelephonyNetworkOperator(), &mcc_mnc); What do you do on other platforms? Will this even compile on other platforms? Also, this is missing an #include above. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:117: DataReductionProxyTamperDetect tamper_detect(headers, is_secure_scheme, put each param on its own line if they don't all fit on one. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:120: // Check if Chrome-Proxy header has been tampered. if the Chrome-Proxy ... tampered with. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:121: if (tamper_detect.IsChromeProxyHeaderTampered(chrome_proxy_fingerprint)) { rename tamper_detect as tamper_detection https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:127: } else add curly braces https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:134: net::HttpUtil::ValuesIterator it(other_fingerprints.begin(), put all the parameters on one line https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:137: // For each fingerprint, get its name |key| and the fingerprint value |value| I think DataReductionProxyHeaders should have a function that takes an instruction name and return's the corresponding value in the Chrome-Proxy header. If not, please add such a function there. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:139: // detect and corresponding UMA report. detect --> detection and report the corresponding UMA https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:141: while (it.GetNext()) { can't you use the same logic as in data_reduction_proxy_headers.cc to find each chrome-proxy instruction? https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:187: // Check whether Chrome-Proxy header has been tampered. tampered with. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:187: // Check whether Chrome-Proxy header has been tampered. Checks whether the https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:188: // |fingerprint| is the fingerprint Chrome received from data reduction proxy, remove "Chrome" https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:189: // which is Base64 encoded. Decode it first. Calculate the hash value of Calculates... https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:193: // Compare calculated fingerprint to the fingerprint from data reduction proxy Compares... https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:196: const std::string& fingerprint) const { indent only 4. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:202: ValuesToSortedString(*clean_chrome_proxy_header_values_)); Where is memory allocated for this vector? https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:207: // Check whether there are proxies/middleboxes between Chrome Checks... https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:207: // Check whether there are proxies/middleboxes between Chrome Remove the first sentence and the word "Concretely" https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:208: // and data reduction proxy. Concretely, it checks whether there are other and the... https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:211: const std::string& fingerprint) const { indent only 4 https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:213: std::vector<std::string> vias = GetHeaderValues(response_headers_, "via"); vias -> via_header_values https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:215: // If there is no tag, then data reduction proxy's tag have been removed. then the https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:215: // If there is no tag, then data reduction proxy's tag have been removed. have -> has https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:216: if (vias.size() == 0) return true; move the return to the next line. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:218: // Check whether the last proxy/middlebox is data reduction proxy or not. is the https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:219: bool seen_data_reduction_proxy = false; seen_data_reduction_proxy -> in_via_header https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:228: return !seen_data_reduction_proxy; In general, you shouldn't do any header processing here. It should all be in data_reduction_proxy_headers. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:267: while (it.GetNext()) { This will start with the second fingerprint, because the first one was retrieved in 259, right? Is that intentional? Add a comment to explain. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:281: // Report other headers tamper detected. Reports... https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:290: // For Content-Length tamper detection... Use complete sentences. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:291: // Check whether the Content-Length value is different from what Checks... https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:293: // have been modified. Fill comments out to 80 characters. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:297: const std::string& fingerprint) const { indent only 4 https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:300: // it cannot be converted to integer, pass. to an https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:303: // If there is Content-Length at Chrome client side is not available, pass. We are on the client side. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:316: // Report Content-Length tamper detected. Reports... https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:317: // Get MIME type of the response and report to different UMA histogram. Gets... https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:10: #include "net/http/http_response_headers.h" #include <string> #include <vector> https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:11: namespace net { class HttpResponseHeaders; } https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:14: // The class for detecting tampering. Explain what that means. E.g., "This class that detects if response header information sent by the data reduction proxy has been modified or deleted by intermediaries on the Web. Specifically, it detects these forms of tampering: ..." https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:22: DataReductionProxyTamperDetect( suggest: DataReductionProxyTamperDetection https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:22: DataReductionProxyTamperDetect( Add a comment. What are all these parameters? How are they used? https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:25: unsigned mcc_mnc, rename: carrier_id. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:31: // tamper detect request (i.e., contains fingerprints added by data reduction Be clearer, e.g.: "Checks if the response contains tamper detection fingerprints added by the data reduction proxy, and determines if the response had been tampered with if so. Results are reported to UMA. HTTP and..." https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:35: static void CheckResponseFingerprint(const net::HttpResponseHeaders* header, Move this static method above the constructor. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:55: // Enum for fingerprint type. Put each value on its own line and add a comment for each. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:63: bool IsChromeProxyHeaderTampered(const std::string& fingerprint) const; Where's the reporting function fo this one? https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:84: // Check whether values of a Chrome-Proxy header contains fingerprints added contain https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:85: // by data reduction proxy. If it does, return true, and save Chrome-Proxy the data reduction If they do and save the https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:90: static bool GetTamperDetectFingerprints(std::vector<std::string>* values, Move this first param to a new line or align the subsequent ones with it. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:94: // Utility function. Return string of sorted values of |values|. Returns And no need to call it a utility function. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:95: static std::string ValuesToSortedString(std::vector<std::string> &values); Do not use non-const references. And the & should be next to the type, not the name. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:97: // Utility function. Return MD5 hash value for a given string |input|. Remove "Utility function" Returns the https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:98: // We need raw MD5 hash value so it's different to base::MD5String which is Remove "We". I.e., don't personify code or comment on the goals of the programmer. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:114: const unsigned mcc_mnc_; Don't use abbreviations in variable names. Can you just call the variable carrier_id_? https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:117: // Save it as temporary result so we don't need to parse Chrome-Proxy header as a parse the https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:23: const char* kDataReductionProxyViaValues[] = {"Chrome-Compression-Proxy", I don't know why you added this here, but make these: const char* const kDataReductionProxyViaValues[] = { "Chrome-Compression-Proxy", "Chrome Compression Proxy" }; https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:23: const char* kDataReductionProxyViaValues[] = {"Chrome-Compression-Proxy", Also, move this into an anonymous namespace. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:100: "1.1 Chrome Compression Proxy"; This should be changed too. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... File components/data_reduction_proxy/common/data_reduction_proxy_headers.h (right): https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/common/data_reduction_proxy_headers.h:18: extern const char* kDataReductionProxyViaValues[2]; I wouldn't expose these here. Can you have accessor methods instead?
Addressing comments. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:5: // This file implements the tamper detection logic, where we want to detect On 2014/07/11 18:22:45, bengr1 wrote: > This comment should be moved to the .h. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:11: // 1. Data reduction proxy selects the requests we want to detect tamper; On 2014/07/11 18:22:46, bengr1 wrote: > The data... Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:12: // for the selected ones, data reduction proxy generates a series of On 2014/07/11 18:22:47, bengr1 wrote: > the data reduction... Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:13: // fingerprints of the response, and append them to the Chrome-Proxy header; On 2014/07/11 18:22:46, bengr1 wrote: > of -> for > > appends it Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:14: // 2. At Chrome client side, once it sees such fingerprints, it uses the On 2014/07/11 18:22:46, bengr1 wrote: > When Chrome sees such fingerprints in response headers, ... Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:15: // same method of data reduction proxy to generate the fingerprints on On 2014/07/11 18:22:44, bengr1 wrote: > of --> as the > > remove "on the response it receives" > > compare it --> and compares them to the fingerprints in the response, to see if > ... Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:21: // Right now we have 4 fingerprints (listed below). Chrome first check the On 2014/07/11 18:22:47, bengr1 wrote: > Right now we --> Four fingerprints are defined (listed below). > > checks > > of Chrome-Proxy -> of the Chrome-Proxy > If Chrome-Proxy -> If the Chrome-Proxy Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:23: // tampered, then other fingerprints would not be checked; if not, Chrome On 2014/07/11 18:22:46, bengr1 wrote: > This is Chromium code too. > > In general you don't have to say Chrome is doing anything. That's implied by > this being part of the Chromium code base. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:24: // parses the rest of the fingerprints and check whether there is tampering On 2014/07/11 18:22:47, bengr1 wrote: > checks Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:37: // Chrome reports tamper or not information for each fingerprint to UMA. In On 2014/07/11 18:22:46, bengr1 wrote: > Up above you seem to suggest that UMA is only collected when the response has > been tampered with, which is different from what you are saying here. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:46: #include <algorithm> On 2014/07/11 18:22:47, bengr1 wrote: > alphabetize Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:55: On 2014/07/11 18:22:44, bengr1 wrote: > remove the blank line Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:60: // Macro for UMA report. First report to either |https_histogram| or On 2014/07/11 18:22:45, bengr1 wrote: > report -> reporting Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:61: // |http_histogram| depends on |scheme_is_https|, with carrier ID as bucket On 2014/07/11 18:22:46, bengr1 wrote: > depends -> depending Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:62: // |mcc_mnc_|. Then report to http(s)_histogram_Total, which counts the total On 2014/07/11 18:22:45, bengr1 wrote: > carrier_id_ Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:64: #define UMA_REPORT(scheme_is_https, http_histogram, https_histogram, mcc_mnc) \ On 2014/07/11 18:22:44, bengr1 wrote: > Why is this a macro. Make this a function. Because it is only 1 line of code and occurs a lot of times. I think it's faster to do it with macro (otherwise it's a function with only 1 IF statement). macro is easier for UMA histogram name also， I have a pair of UMA histogram, for example DataReductionProxy.HTTPSHeaderTamperDetection's x-axis is carrier ID, then DataReductionProxy.HTTPSHeaderTamperDetection_Total is the total number, with no x-axis (only 1 column), the way to handle "_Total" is easier. I think macro is more appropriate, we can change it to inline. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:77: namespace { On 2014/07/11 18:22:45, bengr1 wrote: > I see no reason for this anonymous namespace to be inside the > data_reduction_proxy namespace. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:78: // Two fingerprints will be added to Chrome-Proxy header. One starts with On 2014/07/11 18:22:47, bengr1 wrote: > I thought you were checking 4 things, not 2. Explain. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:94: void DataReductionProxyTamperDetect::CheckResponseFingerprint( On 2014/07/11 18:22:47, bengr1 wrote: > add: "// static" above this line. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:98: std::vector<std::string> values = DataReductionProxyTamperDetect:: On 2014/07/11 18:22:45, bengr1 wrote: > You don't need the DataReductionProxyTamperDetect:: Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:106: // Check if there are fingerprints (and thus need to detect tamper). On 2014/07/11 18:22:47, bengr1 wrote: > tampering Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:107: if (!GetTamperDetectFingerprints(&values, On 2014/07/11 18:22:46, bengr1 wrote: > rename: GetTamperDetectionFingerprints, or just GetFingerprints() Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:112: // Found tamper detect request field. On 2014/07/11 18:22:46, bengr1 wrote: > detection Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:114: unsigned mcc_mnc = 0; On 2014/07/11 18:22:46, bengr1 wrote: > carrier_id Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:117: DataReductionProxyTamperDetect tamper_detect(headers, is_secure_scheme, On 2014/07/11 18:22:45, bengr1 wrote: > put each param on its own line if they don't all fit on one. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:120: // Check if Chrome-Proxy header has been tampered. On 2014/07/11 18:22:46, bengr1 wrote: > if the Chrome-Proxy ... tampered with. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:121: if (tamper_detect.IsChromeProxyHeaderTampered(chrome_proxy_fingerprint)) { On 2014/07/11 18:22:46, bengr1 wrote: > rename tamper_detect as tamper_detection Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:127: } else On 2014/07/11 18:22:45, bengr1 wrote: > add curly braces Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:134: net::HttpUtil::ValuesIterator it(other_fingerprints.begin(), On 2014/07/11 18:22:45, bengr1 wrote: > put all the parameters on one line Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:139: // detect and corresponding UMA report. On 2014/07/11 18:22:47, bengr1 wrote: > detect --> detection > > and report the corresponding UMA Removed that two lines, codes explain it pretty much. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:187: // Check whether Chrome-Proxy header has been tampered. On 2014/07/11 18:22:46, bengr1 wrote: > tampered with. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:187: // Check whether Chrome-Proxy header has been tampered. On 2014/07/11 18:22:46, bengr1 wrote: > tampered with. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:188: // |fingerprint| is the fingerprint Chrome received from data reduction proxy, On 2014/07/11 18:22:45, bengr1 wrote: > remove "Chrome" Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:189: // which is Base64 encoded. Decode it first. Calculate the hash value of On 2014/07/11 18:22:47, bengr1 wrote: > Calculates... > Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:193: // Compare calculated fingerprint to the fingerprint from data reduction proxy On 2014/07/11 18:22:46, bengr1 wrote: > Compares... Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:196: const std::string& fingerprint) const { On 2014/07/11 18:22:46, bengr1 wrote: > indent only 4. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:207: // Check whether there are proxies/middleboxes between Chrome On 2014/07/11 18:22:44, bengr1 wrote: > Remove the first sentence and the word "Concretely" Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:207: // Check whether there are proxies/middleboxes between Chrome On 2014/07/11 18:22:45, bengr1 wrote: > Checks... Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:208: // and data reduction proxy. Concretely, it checks whether there are other On 2014/07/11 18:22:47, bengr1 wrote: > and the... removed. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:211: const std::string& fingerprint) const { On 2014/07/11 18:22:45, bengr1 wrote: > indent only 4 Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:213: std::vector<std::string> vias = GetHeaderValues(response_headers_, "via"); On 2014/07/11 18:22:45, bengr1 wrote: > vias -> via_header_values Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:215: // If there is no tag, then data reduction proxy's tag have been removed. On 2014/07/11 18:22:44, bengr1 wrote: > have -> has Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:215: // If there is no tag, then data reduction proxy's tag have been removed. On 2014/07/11 18:22:47, bengr1 wrote: > then the Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:216: if (vias.size() == 0) return true; On 2014/07/11 18:22:46, bengr1 wrote: > move the return to the next line. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:218: // Check whether the last proxy/middlebox is data reduction proxy or not. On 2014/07/11 18:22:45, bengr1 wrote: > is the Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:219: bool seen_data_reduction_proxy = false; On 2014/07/11 18:22:44, bengr1 wrote: > seen_data_reduction_proxy -> in_via_header Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:267: while (it.GetNext()) { On 2014/07/11 18:22:47, bengr1 wrote: > This will start with the second fingerprint, because the first one was retrieved > in 259, right? Is that intentional? Add a comment to explain. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:281: // Report other headers tamper detected. On 2014/07/11 18:22:46, bengr1 wrote: > Reports... Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:290: // For Content-Length tamper detection... On 2014/07/11 18:22:45, bengr1 wrote: > Use complete sentences. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:291: // Check whether the Content-Length value is different from what On 2014/07/11 18:22:47, bengr1 wrote: > Checks... Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:293: // have been modified. On 2014/07/11 18:22:47, bengr1 wrote: > Fill comments out to 80 characters. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:297: const std::string& fingerprint) const { On 2014/07/11 18:22:44, bengr1 wrote: > indent only 4 Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:300: // it cannot be converted to integer, pass. On 2014/07/11 18:22:45, bengr1 wrote: > to an Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:303: // If there is Content-Length at Chrome client side is not available, pass. On 2014/07/11 18:22:46, bengr1 wrote: > We are on the client side. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:316: // Report Content-Length tamper detected. On 2014/07/11 18:22:47, bengr1 wrote: > Reports... Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:317: // Get MIME type of the response and report to different UMA histogram. On 2014/07/11 18:22:45, bengr1 wrote: > Gets... Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:10: #include "net/http/http_response_headers.h" On 2014/07/11 18:22:48, bengr1 wrote: > #include <string> > #include <vector> Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:11: On 2014/07/11 18:22:47, bengr1 wrote: > namespace net { > class HttpResponseHeaders; > } Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:14: // The class for detecting tampering. On 2014/07/11 18:22:48, bengr1 wrote: > Explain what that means. E.g., "This class that detects if response header > information sent by the data reduction proxy has been modified or deleted by > intermediaries on the Web. Specifically, it detects these forms of tampering: > ..." Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:22: DataReductionProxyTamperDetect( On 2014/07/11 18:22:48, bengr1 wrote: > suggest: DataReductionProxyTamperDetection Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:22: DataReductionProxyTamperDetect( On 2014/07/11 18:22:48, bengr1 wrote: > Add a comment. What are all these parameters? How are they used? Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:25: unsigned mcc_mnc, On 2014/07/11 18:22:49, bengr1 wrote: > rename: carrier_id. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:31: // tamper detect request (i.e., contains fingerprints added by data reduction On 2014/07/11 18:22:48, bengr1 wrote: > Be clearer, e.g.: > > "Checks if the response contains tamper detection fingerprints added by the data > reduction proxy, and determines if the response had been tampered with if so. > Results are reported to UMA. HTTP and..." Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:35: static void CheckResponseFingerprint(const net::HttpResponseHeaders* header, On 2014/07/11 18:22:48, bengr1 wrote: > Move this static method above the constructor. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:55: // Enum for fingerprint type. On 2014/07/11 18:22:48, bengr1 wrote: > Put each value on its own line and add a comment for each. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:63: bool IsChromeProxyHeaderTampered(const std::string& fingerprint) const; On 2014/07/11 18:22:48, bengr1 wrote: > Where's the reporting function fo this one? Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:84: // Check whether values of a Chrome-Proxy header contains fingerprints added On 2014/07/11 18:22:48, bengr1 wrote: > contain Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:85: // by data reduction proxy. If it does, return true, and save Chrome-Proxy On 2014/07/11 18:22:48, bengr1 wrote: > the data reduction > > If they do > > and save the Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:90: static bool GetTamperDetectFingerprints(std::vector<std::string>* values, On 2014/07/11 18:22:48, bengr1 wrote: > Move this first param to a new line or align the subsequent ones with it. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:94: // Utility function. Return string of sorted values of |values|. On 2014/07/11 18:22:48, bengr1 wrote: > Returns > > And no need to call it a utility function. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:95: static std::string ValuesToSortedString(std::vector<std::string> &values); On 2014/07/11 18:22:48, bengr1 wrote: > Do not use non-const references. > And the & should be next to the type, not the name. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:97: // Utility function. Return MD5 hash value for a given string |input|. On 2014/07/11 18:22:48, bengr1 wrote: > Remove "Utility function" > > Returns the Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:98: // We need raw MD5 hash value so it's different to base::MD5String which is On 2014/07/11 18:22:48, bengr1 wrote: > Remove "We". I.e., don't personify code or comment on the goals of the > programmer. Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:114: const unsigned mcc_mnc_; On 2014/07/11 18:22:48, bengr1 wrote: > Don't use abbreviations in variable names. Can you just call the variable > carrier_id_? Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:117: // Save it as temporary result so we don't need to parse Chrome-Proxy header On 2014/07/11 18:22:49, bengr1 wrote: > as a > > parse the Done. https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... File components/data_reduction_proxy/common/data_reduction_proxy_headers.h (right): https://codereview.chromium.org/338483002/diff/1160001/components/data_reduct... components/data_reduction_proxy/common/data_reduction_proxy_headers.h:18: extern const char* kDataReductionProxyViaValues[2]; On 2014/07/11 18:22:49, bengr1 wrote: > I wouldn't expose these here. Can you have accessor methods instead? With modified HasDataReductionProxyViaHeader, I don't need these outside _header.cc/h
Move modification on _headers.h/cc to another CL. This CL uses two functions added in https://codereview.chromium.org/387353003/ I was trying to use as many common functions as possible, and also making functions more common and can be added to _header.h. But the function I need may not be that common: 1) I need to parse 2 actions (fingerprints) in 1 parse; 2) I need to sort values of one header.
https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:41: const char kTamperDetectFingerprints[] = "fp"; To avoid name conflicts, all chrome-proxy actions should be defined in data_reduction_proxy_headers.{cc|h} https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:50: } // namespace Add a space before // https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:56: const net::HttpResponseHeaders* headers, check that headers is non-NULL. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:57: const bool is_secure_scheme) can this function be const? https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:58: { Move the curly up a line. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:59: // If Chrome-Proxy header fingerprint absents, quits tamper detection ASAP. If the Chrome-Proxy header fingerprint is absent, abort tamper detection. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:66: std::vector<std::string> values = GetHeaderValues(headers, "Chrome-Proxy"); I still don't understand why you need this. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:66: std::vector<std::string> values = GetHeaderValues(headers, "Chrome-Proxy"); values -> chrome_proxy_header_values https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:76: return; I would add curly braces around this. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:80: #if defined(OS_ANDROID) Is this busted on other platforms? https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:102: other_fingerprints.begin(), other_fingerprints.end(), '|'); Are fingerprints guaranteed not to contain '|'. Say so in the comment. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:104: // For each fingerprint, get its name |key| and the fingerprint |value| |value|. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:111: std::string value = it.value().substr(delimiter_pos + 1); Make each fingerprint a Chrome-Proxy action starting with 'f' and declared in data_reduction_proxy_header.h https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:137: const net::HttpResponseHeaders* headers, DCHECK that headers is non-NULL https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:177: UMA_REPORT(is_secure_scheme_, I'd rename the macro to be less general sounding. E.g., REPORT_TAMPER_DETECTION_UMA https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:190: // Via header of the data reductoin proxy is missing. spelling https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:241: std::vector<std::string> values = rename as response_header_values https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:244: header_values += ValuesToSortedString(&values) + ";"; are the values guaranteed not to contain ';'? Add a comment explaining. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:273: &actual_content_length_)) { indentation. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:335: // Enumerates the values of Chrome-Proxy header and checks if there are Move method comments to the .h. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:341: std::vector<std::string>* values, DCHECK that values is non-NULL. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:6: // whether there are middleboxes and whether they are tampering the response with the response https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:7: // which maybe break correct communication and data transfer between Chrome Chrome -> a Chromium client https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:11: // 1. The data reduction proxy selects the requests we want to detect tamper; detect tamper -> analyze https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:13: // fingerprints for the response , and appends it to the Chrome-Proxy header; response , --> response, https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:14: // 2. At Chrome client side, when Chrome sees such fingerprints, it uses the At Chrome client side, when Chrome sees such fingerprints --> When the Chromium client sees such fingerprints https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:15: // same method as the data reduction proxy to generate the fingerprints, and generate --> re-generate https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:17: // tamper detected. there is any tamper detected -> the response has been tampered with. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:19: // Four fingerprints are defined (listed below). Chrome first checks the Chrome first checks --> The fingerprint of the Chrome-Proxy header is checked first. In general, stop using "Chrome". This is Chromium code. Chrome is one application that can be built with this code. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:21: // tampered, then other fingerprints would not be checked; if not, Chrome tampered with https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:23: // on each of them. Tamper is a strange word. You might want to stop using it and instead use what you mean, namely, that you are checking if a middlebox modified or deleted information. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:35: // Chrome reports tampered information for each fingerprint to UMA. In general, Again, not Chrome, and use of tampered is awkward. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:39: // dimension, MIME types, Chrome reports the tamper on different MIME type You can use phrases like "tamper detection", "tampering", and "tamper with," but "the tamper" is incorrect. For this one, I'd suggest "Reports of tampering are separated by MIME type" https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:45: #include <string> #include <map> https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:60: remove blank line https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:63: // with if so. Results are reported to UMA. HTTP and HTTPS traffic would be would be -> are https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:68: // Tamper detection checks the response |respose_headers|. |is_secure_scheme| checks the response |response_headers| --> checks |response_headers|. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:69: // and |carrier_id| are parameters specify correct UMA histogram to report. I don't understand this sentence. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:72: // which is an temporary result saved to use later for avoiding parsing the a temporary Also, I don't understand this sentence. The header is already parsed by HttpResponseHeaders. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:101: CHROMEPROXY, // 1. Code of fingerprint for Chrome-Proxy header. Give these explicit values, e.g.,: CHROMEPROXY = 1, /* Code of ... */ VIA = 2, /* ...
- Addressed all the comments. - Fixed grammar: - about *tamper* - using fingerprint *of* some header instead *for* & *of* - using Chrome-Proxy header, not header*s* (I realized that in other files header*s* sometimes mean with multiple values) - one thing we can do: in _detect.cc, line 88, to use SWITCH(fingerprint_code), we need to initialize the name_to_code map, put names into a array and do a FOR; alternatively, we can just do 3 IFs, which is simpler. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:41: const char kTamperDetectFingerprints[] = "fp"; On 2014/07/16 21:24:48, bengr1 wrote: > To avoid name conflicts, all chrome-proxy actions should be defined in > data_reduction_proxy_headers.{cc|h} Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:50: } // namespace On 2014/07/16 21:24:48, bengr1 wrote: > Add a space before // Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:56: const net::HttpResponseHeaders* headers, On 2014/07/16 21:24:47, bengr1 wrote: > check that headers is non-NULL. Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:57: const bool is_secure_scheme) On 2014/07/16 21:24:48, bengr1 wrote: > can this function be const? Right now the function is static, so can't be const, any suggestion? https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:58: { On 2014/07/16 21:24:48, bengr1 wrote: > Move the curly up a line. Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:59: // If Chrome-Proxy header fingerprint absents, quits tamper detection ASAP. On 2014/07/16 21:24:48, bengr1 wrote: > If the Chrome-Proxy header fingerprint is absent, abort tamper detection. Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:66: std::vector<std::string> values = GetHeaderValues(headers, "Chrome-Proxy"); On 2014/07/16 21:24:47, bengr1 wrote: > I still don't understand why you need this. Acknowledged. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:66: std::vector<std::string> values = GetHeaderValues(headers, "Chrome-Proxy"); On 2014/07/16 21:24:47, bengr1 wrote: > I still don't understand why you need this. Acknowledged. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:76: return; On 2014/07/16 21:24:48, bengr1 wrote: > I would add curly braces around this. Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:80: #if defined(OS_ANDROID) On 2014/07/16 21:24:47, bengr1 wrote: > Is this busted on other platforms? Discussed with Bolian, right now for other platforms the carrier_id is 0 (WiFi is 0 as well). https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:102: other_fingerprints.begin(), other_fingerprints.end(), '|'); On 2014/07/16 21:24:48, bengr1 wrote: > Are fingerprints guaranteed not to contain '|'. Say so in the comment. Modified the design. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:104: // For each fingerprint, get its name |key| and the fingerprint |value| On 2014/07/16 21:24:48, bengr1 wrote: > |value|. Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:111: std::string value = it.value().substr(delimiter_pos + 1); On 2014/07/16 21:24:48, bengr1 wrote: > Make each fingerprint a Chrome-Proxy action starting with 'f' and declared in > data_reduction_proxy_header.h Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:137: const net::HttpResponseHeaders* headers, On 2014/07/16 21:24:48, bengr1 wrote: > DCHECK that headers is non-NULL Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:177: UMA_REPORT(is_secure_scheme_, On 2014/07/16 21:24:47, bengr1 wrote: > I'd rename the macro to be less general sounding. E.g., > REPORT_TAMPER_DETECTION_UMA Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:190: // Via header of the data reductoin proxy is missing. On 2014/07/16 21:24:47, bengr1 wrote: > spelling Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:241: std::vector<std::string> values = On 2014/07/16 21:24:48, bengr1 wrote: > rename as response_header_values Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:244: header_values += ValuesToSortedString(&values) + ";"; On 2014/07/16 21:24:47, bengr1 wrote: > are the values guaranteed not to contain ';'? Add a comment explaining. Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:273: &actual_content_length_)) { On 2014/07/16 21:24:47, bengr1 wrote: > indentation. Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:335: // Enumerates the values of Chrome-Proxy header and checks if there are On 2014/07/16 21:24:48, bengr1 wrote: > Move method comments to the .h. Function removed. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:341: std::vector<std::string>* values, On 2014/07/16 21:24:47, bengr1 wrote: > DCHECK that values is non-NULL. Function removed. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:6: // whether there are middleboxes and whether they are tampering the response On 2014/07/16 21:24:49, bengr1 wrote: > with the response Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:7: // which maybe break correct communication and data transfer between Chrome On 2014/07/16 21:24:48, bengr1 wrote: > Chrome -> a Chromium client Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:11: // 1. The data reduction proxy selects the requests we want to detect tamper; On 2014/07/16 21:24:49, bengr1 wrote: > detect tamper -> analyze Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:13: // fingerprints for the response , and appends it to the Chrome-Proxy header; On 2014/07/16 21:24:49, bengr1 wrote: > response , --> response, Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:14: // 2. At Chrome client side, when Chrome sees such fingerprints, it uses the On 2014/07/16 21:24:48, bengr1 wrote: > At Chrome client side, when Chrome sees such fingerprints --> When the Chromium > client sees such fingerprints Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:14: // 2. At Chrome client side, when Chrome sees such fingerprints, it uses the On 2014/07/16 21:24:48, bengr1 wrote: > At Chrome client side, when Chrome sees such fingerprints --> When the Chromium > client sees such fingerprints Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:15: // same method as the data reduction proxy to generate the fingerprints, and On 2014/07/16 21:24:49, bengr1 wrote: > generate --> re-generate Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:17: // tamper detected. On 2014/07/16 21:24:49, bengr1 wrote: > there is any tamper detected -> the response has been tampered with. Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:19: // Four fingerprints are defined (listed below). Chrome first checks the On 2014/07/16 21:24:49, bengr1 wrote: > Chrome first checks --> The fingerprint of the Chrome-Proxy header is checked > first. > > In general, stop using "Chrome". This is Chromium code. Chrome is one > application that can be built with this code. Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:21: // tampered, then other fingerprints would not be checked; if not, Chrome On 2014/07/16 21:24:49, bengr1 wrote: > tampered with Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:23: // on each of them. On 2014/07/16 21:24:49, bengr1 wrote: > Tamper is a strange word. You might want to stop using it and instead use what > you mean, namely, that you are checking if a middlebox modified or deleted > information. Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:35: // Chrome reports tampered information for each fingerprint to UMA. In general, On 2014/07/16 21:24:48, bengr1 wrote: > Again, not Chrome, and use of tampered is awkward. Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:39: // dimension, MIME types, Chrome reports the tamper on different MIME type On 2014/07/16 21:24:49, bengr1 wrote: > You can use phrases like "tamper detection", "tampering", and "tamper with," but > "the tamper" is incorrect. For this one, I'd suggest "Reports of tampering are > separated by MIME type" > Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:45: #include <string> On 2014/07/16 21:24:48, bengr1 wrote: > #include <map> Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:60: On 2014/07/16 21:24:49, bengr1 wrote: > remove blank line Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:63: // with if so. Results are reported to UMA. HTTP and HTTPS traffic would be On 2014/07/16 21:24:49, bengr1 wrote: > would be -> are Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:68: // Tamper detection checks the response |respose_headers|. |is_secure_scheme| On 2014/07/16 21:24:49, bengr1 wrote: > checks the response |response_headers| --> checks |response_headers|. Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:69: // and |carrier_id| are parameters specify correct UMA histogram to report. On 2014/07/16 21:24:49, bengr1 wrote: > I don't understand this sentence. Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:72: // which is an temporary result saved to use later for avoiding parsing the On 2014/07/16 21:24:49, bengr1 wrote: > a temporary > > Also, I don't understand this sentence. The header is already parsed by > HttpResponseHeaders. Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:101: CHROMEPROXY, // 1. Code of fingerprint for Chrome-Proxy header. On 2014/07/16 21:24:49, bengr1 wrote: > Give these explicit values, e.g.,: > CHROMEPROXY = 1, /* Code of ... */ > VIA = 2, /* ... Done. https://codereview.chromium.org/338483002/diff/1260001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:101: CHROMEPROXY, // 1. Code of fingerprint for Chrome-Proxy header. On 2014/07/16 21:24:49, bengr1 wrote: > Give these explicit values, e.g.,: > CHROMEPROXY = 1, /* Code of ... */ > VIA = 2, /* ... Done.
https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:46: // If fingerprint of Chrome-Proxy header is absent, abort tamper detection. If the fingerprint of the Chrome-Proxy https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:58: // Removes Chrome-Proxy header's fingerprint for generating the fingerprint Remove the Chrome-Proxy https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:59: // of received Chrome-Proxy header later. I don't understand this comment. Do you mean: // The Chrome-Proxy header's fingerprint is computed over all instructions within the header except the fingerprint, so remove that fingerprint before verifying the Chrome-Proxy header's integrity. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:90: data_reduction_proxy::tamper_detection_fingerprint_names:: Call this kChromeProxyActionFingerprintVia. Don't create a separate namespace for this. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:105: FingerprintCode fingerprint_code = tamper_detection. move tamper_detection to the next line. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:128: // Constructor initializes fingerprint name to code map. // Constructor initializes the map of fingerprint name to code. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:207: // The format for |fingerprint| is: I'm confused by this. I would expect an explanation like the following: We construct a canonical representation of the headers so that reordered headers will produce the same fingerprint. The fingerprint is constructed as follows: 1) sort... 2) merge into a string with a ':' delimeter. 3) etc https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:208: // [base64encoded_fingerprint]:header_name1:header_namer2:... Verify that the delimiter is legal according to RFC2616, and that it does not appear in a base64 encoding, so '+' and '/' are out. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:240: header_values += ValuesToSortedString(&response_header_values) + ";"; The end of the list will be a ';'. Is that ok? https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:243: // Calculates MD5 hash value on the concatenated string. the MD5 hash of https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:338: return it == fingerprint_name_code_map_.end() ? NONEXIST : it->second; This is confusing. How about: if (!fingerprint_name_code_map_.end()) return it->second; return NONEXIST; https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:344: std::vector<std::string>* values) { Check that values is not NULL https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:348: size_t size = values->size(); Move values->size() into the loop declaration. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:362: size_t size = values->size(); Move values->size() into the loop declaration. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:363: for (size_t i = 0; i < size; ++i) add curly brace https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:21: // 1. Fingerprint of Chrome-Proxy header, which is designed to check whether of the https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:23: // 2. Fingerprint of Via header, which is designed to check whether there are of the https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:27: // have been modified or not; modified or deleted? https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:30: // Content-Length value indicates different response body). Should we be making this assumption? Content-Length headers often don't match the content length. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:32: // At Chromium client side, the fingerprint of Chrome-Proxy header would be At Chromium client side -> At the Chromium client of Chrome-Proxy --> of the Chrome-Proxy would be --> will be https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:33: // checked first. If such fingerprint indicates that the Chrome-Proxy header such -> the https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:34: // has not been modified, then other fingerprints are reliable and would be other -> the other are -> will be considered to be would be -> will be possible that other --> possible that the other thus would -> thus they will https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:39: // fingerprint, Chromium client reports the number of responses that have been Chromium -> the Chromium https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:40: // tampered with for different carriers. For the fingerprint of Content-Length of -> of the https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:72: // |carrier_id| is used to specify the bucket for histograms. Change this sentence to: Histogram events are reported by |carrier_id|. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:75: // which is a temporary result saved to use later to avoid parsing the header header -> header again. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:138: // Removes the fingerprint of Chrome-Proxy header from Chrome-Proxy header's "the Chrome-Proxy" every time in this comment. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... File components/data_reduction_proxy/common/data_reduction_proxy_headers.h (right): https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/common/data_reduction_proxy_headers.h:17: namespace fingerprint_actions { remove this namespace and use names like: kChromeProxyActionFingerprintChromeProxy
https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:20: // Macro for UMA reporting. First reports to either |https_histogram| or Can we rewrite the comment shorter. Reading the comments, I am confused. The code itself is clearer. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:39: void DataReductionProxyTamperDetection::CheckResponseFingerprint( can we rename this to DetectAndReport? https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:50: tamper_detection_fingerprint_names::kFingerprintChromeProxy, why add a new namespace just for this? https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:59: // of received Chrome-Proxy header later. +1. Better document why than what. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:78: } else { get rid of else. you have returned in 'if'. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:89: const char* fingerprint_names[] = { You don't need this array. Just iterate the name->code map that you already have. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:125: return; rm https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:188: if (!has_chrome_proxy) { I would move this to the report function and return the missing case in a param. This will also allow you unit test such a case. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:315: else if (mime_type.find("image") == 0) { image/
Addressed comments. Fixed grammar: - mainly focusing on adding "the" Move kChromeProxyAction* to the other CL (headers.cc/h). https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:20: // Macro for UMA reporting. First reports to either |https_histogram| or On 2014/07/19 00:15:24, bolian wrote: > Can we rewrite the comment shorter. Reading the comments, I am confused. The > code itself is clearer. Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:39: void DataReductionProxyTamperDetection::CheckResponseFingerprint( On 2014/07/19 00:15:24, bolian wrote: > can we rename this to DetectAndReport? Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:46: // If fingerprint of Chrome-Proxy header is absent, abort tamper detection. On 2014/07/18 22:47:54, bengr1 wrote: > If the fingerprint of the Chrome-Proxy Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:50: tamper_detection_fingerprint_names::kFingerprintChromeProxy, On 2014/07/19 00:15:24, bolian wrote: > why add a new namespace just for this? Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:58: // Removes Chrome-Proxy header's fingerprint for generating the fingerprint On 2014/07/18 22:47:54, bengr1 wrote: > Remove the Chrome-Proxy Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:78: } else { On 2014/07/19 00:15:24, bolian wrote: > get rid of else. you have returned in 'if'. Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:89: const char* fingerprint_names[] = { On 2014/07/19 00:15:24, bolian wrote: > You don't need this array. Just iterate the name->code map that you already > have. Done. Good point. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:90: data_reduction_proxy::tamper_detection_fingerprint_names:: On 2014/07/18 22:47:54, bengr1 wrote: > Call this kChromeProxyActionFingerprintVia. Don't create a separate namespace > for this. Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:105: FingerprintCode fingerprint_code = tamper_detection. On 2014/07/18 22:47:54, bengr1 wrote: > move tamper_detection to the next line. Removed. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:125: return; On 2014/07/19 00:15:24, bolian wrote: > rm Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:128: // Constructor initializes fingerprint name to code map. On 2014/07/18 22:47:54, bengr1 wrote: > // Constructor initializes the map of fingerprint name to code. Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:188: if (!has_chrome_proxy) { On 2014/07/19 00:15:24, bolian wrote: > I would move this to the report function and return the missing case in a param. > This will also allow you unit test such a case. Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:207: // The format for |fingerprint| is: On 2014/07/18 22:47:53, bengr1 wrote: > I'm confused by this. I would expect an explanation like the following: > > We construct a canonical representation of the headers so that reordered headers > will produce the same fingerprint. The fingerprint is constructed as follows: > 1) sort... > 2) merge into a string with a ':' delimeter. > 3) etc Done. the function to generate "canonical representation of A header" is ValuesToSortedString. Here for each header, we need to call ValuesToSortedString to generate a string, concatenates the string and then calculate hash value on it. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:208: // [base64encoded_fingerprint]:header_name1:header_namer2:... On 2014/07/18 22:47:54, bengr1 wrote: > Verify that the delimiter is legal according to RFC2616, and that it does not > appear in a base64 encoding, so '+' and '/' are out. Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:240: header_values += ValuesToSortedString(&response_header_values) + ";"; On 2014/07/18 22:47:54, bengr1 wrote: > The end of the list will be a ';'. Is that ok? That's expected, so a non-exist header is different to a header contains an empty value: a header contains an empty value: "," a header does not exist: "" https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:243: // Calculates MD5 hash value on the concatenated string. On 2014/07/18 22:47:54, bengr1 wrote: > the MD5 hash of Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:315: else if (mime_type.find("image") == 0) { On 2014/07/19 00:15:24, bolian wrote: > image/ Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:338: return it == fingerprint_name_code_map_.end() ? NONEXIST : it->second; On 2014/07/18 22:47:53, bengr1 wrote: > This is confusing. How about: > if (!fingerprint_name_code_map_.end()) > return it->second; > return NONEXIST; Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:344: std::vector<std::string>* values) { On 2014/07/18 22:47:54, bengr1 wrote: > Check that values is not NULL Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:348: size_t size = values->size(); On 2014/07/18 22:47:54, bengr1 wrote: > Move values->size() into the loop declaration. Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:362: size_t size = values->size(); On 2014/07/18 22:47:53, bengr1 wrote: > Move values->size() into the loop declaration. Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:363: for (size_t i = 0; i < size; ++i) On 2014/07/18 22:47:54, bengr1 wrote: > add curly brace Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:21: // 1. Fingerprint of Chrome-Proxy header, which is designed to check whether On 2014/07/18 22:47:54, bengr1 wrote: > of the Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:23: // 2. Fingerprint of Via header, which is designed to check whether there are On 2014/07/18 22:47:55, bengr1 wrote: > of the Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:27: // have been modified or not; On 2014/07/18 22:47:54, bengr1 wrote: > modified or deleted? Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:30: // Content-Length value indicates different response body). On 2014/07/18 22:47:55, bengr1 wrote: > Should we be making this assumption? Content-Length headers often don't match > the content length. No modification made, need to discuss. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:32: // At Chromium client side, the fingerprint of Chrome-Proxy header would be On 2014/07/18 22:47:55, bengr1 wrote: > At Chromium client side -> At the Chromium client > of Chrome-Proxy --> of the Chrome-Proxy > would be --> will be Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:33: // checked first. If such fingerprint indicates that the Chrome-Proxy header On 2014/07/18 22:47:55, bengr1 wrote: > such -> the Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:34: // has not been modified, then other fingerprints are reliable and would be On 2014/07/18 22:47:55, bengr1 wrote: > other -> the other > are -> will be considered to be > would be -> will be > possible that other --> possible that the other > thus would -> thus they will Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:39: // fingerprint, Chromium client reports the number of responses that have been On 2014/07/18 22:47:55, bengr1 wrote: > Chromium -> the Chromium Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:40: // tampered with for different carriers. For the fingerprint of Content-Length On 2014/07/18 22:47:55, bengr1 wrote: > of -> of the Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:72: // |carrier_id| is used to specify the bucket for histograms. On 2014/07/18 22:47:55, bengr1 wrote: > Change this sentence to: > Histogram events are reported by |carrier_id|. Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:75: // which is a temporary result saved to use later to avoid parsing the header On 2014/07/18 22:47:55, bengr1 wrote: > header -> header again. Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:138: // Removes the fingerprint of Chrome-Proxy header from Chrome-Proxy header's On 2014/07/18 22:47:55, bengr1 wrote: > "the Chrome-Proxy" every time in this comment. Done. https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... File components/data_reduction_proxy/common/data_reduction_proxy_headers.h (right): https://codereview.chromium.org/338483002/diff/1380001/components/data_reduct... components/data_reduction_proxy/common/data_reduction_proxy_headers.h:17: namespace fingerprint_actions { On 2014/07/18 22:47:55, bengr1 wrote: > remove this namespace and use names like: > kChromeProxyActionFingerprintChromeProxy Done and moved to the other CL.
https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:390: std::vector<std::string> DataReductionProxyTamperDetection::GetHeaderValues( Should this belong to components/data_reduction_proxy/common/data_reduction_proxy_headers https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc (right): https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:31: static std::string GetEncoded(const std::string& input) { s/static// https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:57: struct TestCaseCheckingFingerprint { rename it to TamperDetectionTestCase? https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:80: DataReductionProxyTamperDetection::FingerprintCode fingerprint) { s/fingerprint/fingerprint_code/ https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:83: scoped_refptr<net::HttpResponseHeaders> headers( why scoped_refptr? Will a stack var of net::HttpResponseHeaders work? https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:97: bool tampered; set a default value. https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:119: break; not setting tampered in this case? https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:128: // Checks sorting values and decoding. This is the case that server does not ask for detection,right? https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:193: // Chrome-Proxy header different to its fingerprint. This comment does not make sense. Do you mean // Missing header values. or // Changed header values. or // Additional header values. or all of them? https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:196: "Chrome-Proxy: aut=aauutthh,bbbypas=2,aaxxx=xxx,bbbloc=1\n", can you minimize you headers to test exact this case? You have too many values that don't matter, which makes it not easy to read. Apply this to all other cases. https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:201: // Chrome-Proxy header different to its fingerprint. How is this different than above (your comment is the same)? https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:391: test[i].received_fingerprint = You need to test the case that fcp value encoding is wrong. You can pre-compute the encoding before setting up the tests. https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:394: DataReductionProxyTamperDetectionTest::TestFingerprintCommon(test[i], If you one case fails, can you easily tell which one it is from the error message?
Addressed Ben and Bolian's comments for .h/.cc. Will address comments for unittest later today. https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:390: std::vector<std::string> DataReductionProxyTamperDetection::GetHeaderValues( On 2014/07/21 23:43:04, bolian wrote: > Should this belong to > components/data_reduction_proxy/common/data_reduction_proxy_headers I think it would only be used in tamper detection: it serves the purpose of "getting values -> remove one value -> sort values -> form a string". "remove one value" is too.. special, which is only for tamper detection.
Addressed Bolian's comments on unittest, minimized it. https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc (right): https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:31: static std::string GetEncoded(const std::string& input) { On 2014/07/21 23:43:05, bolian wrote: > s/static// Done. https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:57: struct TestCaseCheckingFingerprint { On 2014/07/21 23:43:05, bolian wrote: > rename it to TamperDetectionTestCase? Done. https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:80: DataReductionProxyTamperDetection::FingerprintCode fingerprint) { On 2014/07/21 23:43:05, bolian wrote: > s/fingerprint/fingerprint_code/ Done. https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:83: scoped_refptr<net::HttpResponseHeaders> headers( On 2014/07/21 23:43:05, bolian wrote: > why scoped_refptr? Will a stack var of net::HttpResponseHeaders work? Done. https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:97: bool tampered; On 2014/07/21 23:43:05, bolian wrote: > set a default value. Done. https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:119: break; On 2014/07/21 23:43:04, bolian wrote: > not setting tampered in this case? It should not reach here. https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:128: // Checks sorting values and decoding. On 2014/07/21 23:43:05, bolian wrote: > This is the case that server does not ask for detection,right? No, this is the case we check ChromeProxy's fingerprint. We have the fingerprint, we have the header; but at header, I removed Chrome-Proxy's fingerprint. https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:193: // Chrome-Proxy header different to its fingerprint. On 2014/07/21 23:43:05, bolian wrote: > This comment does not make sense. Do you mean > // Missing header values. > or > // Changed header values. > or > // Additional header values. > or > all of them? Done. https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:196: "Chrome-Proxy: aut=aauutthh,bbbypas=2,aaxxx=xxx,bbbloc=1\n", On 2014/07/21 23:43:04, bolian wrote: > can you minimize you headers to test exact this case? You have too many values > that don't matter, which makes it not easy to read. Apply this to all other > cases. Done. https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:201: // Chrome-Proxy header different to its fingerprint. On 2014/07/21 23:43:05, bolian wrote: > How is this different than above (your comment is the same)? Done. https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:391: test[i].received_fingerprint = On 2014/07/21 23:43:05, bolian wrote: > You need to test the case that fcp value encoding is wrong. You can pre-compute > the encoding before setting up the tests. Done. https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:394: DataReductionProxyTamperDetectionTest::TestFingerprintCommon(test[i], On 2014/07/21 23:43:04, bolian wrote: > If you one case fails, can you easily tell which one it is from the error > message? Done.
https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc (right): https://codereview.chromium.org/338483002/diff/1440001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:83: scoped_refptr<net::HttpResponseHeaders> headers( You are leaking memory. Just net::HttpResponseHeaders headers(raw_headers); https://codereview.chromium.org/338483002/diff/1460001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc (right): https://codereview.chromium.org/338483002/diff/1460001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:121: has_chrome_proxy_via_header)<<test.label; space before and after "<<". everywhere. https://codereview.chromium.org/338483002/diff/1460001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:143: // proxy, instead, the base64 encoded field is in plain text (within "[]") I think I now understand you tests, but probably because I have read it many times. I think part of the problem is you add a fcp and then removed it. Can you setup tests as follows? TamperDetectionTestCase test[] = { { "reordering", string("Chrome-Proxy: b=2,a=1,c=3,fcp=") + GetEncoded("a=1,b=2,c=3), false, }, } This way, you can also test the case when fcp value cannot be decoded. WDYT? https://codereview.chromium.org/338483002/diff/1460001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:148: "HTTP/1.1 200 OK\n" Do you need the 200 OK line? You are not enforcing that, right? It is a condition in chrome_network_delegate.
Addressed Bolian's comments. In unittest, modify ReplaceWithEncodedString to support nested case: [[abc]def], so right now I can convert a string: "Chrome-Proxy: foh=[need_to_encode], fvia=0, fcp=[foh=[need_to_encode], fvia=0] https://codereview.chromium.org/338483002/diff/1460001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc (right): https://codereview.chromium.org/338483002/diff/1460001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:121: has_chrome_proxy_via_header)<<test.label; On 2014/07/23 23:22:57, bolian wrote: > space before and after "<<". everywhere. Done. https://codereview.chromium.org/338483002/diff/1460001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:143: // proxy, instead, the base64 encoded field is in plain text (within "[]") On 2014/07/23 23:22:57, bolian wrote: > I think I now understand you tests, but probably because I have read it many > times. I think part of the problem is you add a fcp and then removed it. Can you > setup tests as follows? > > TamperDetectionTestCase test[] = { > { > "reordering", > string("Chrome-Proxy: b=2,a=1,c=3,fcp=") + > GetEncoded("a=1,b=2,c=3), > false, > }, > } > > This way, you can also test the case when fcp value cannot be decoded. > WDYT? Done. But a little bit tricky, Chrome-Proxy fingerprint contains OtherHeader fingerprint, which is nested encoding. Modified ReplaceWithEncodedString to support nested case like [[abc]def]. https://codereview.chromium.org/338483002/diff/1460001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:148: "HTTP/1.1 200 OK\n" On 2014/07/23 23:22:57, bolian wrote: > Do you need the 200 OK line? You are not enforcing that, right? It is a > condition in chrome_network_delegate. I need that to parse it as a HTTP header, otherwise I can't form httpresponseheader.
Thanks, I think it is almost there to me. We need to change the histogram.xml. https://codereview.chromium.org/338483002/diff/1460001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc (right): https://codereview.chromium.org/338483002/diff/1460001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:140: // Checks function IsChromeProxyHeaderTampered. s/Checks/Tests. And in below cases as well. https://codereview.chromium.org/338483002/diff/1480001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc (right): https://codereview.chromium.org/338483002/diff/1480001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:772: "Check the case response matches the fingerprint completely.", Is this the same the one above? https://codereview.chromium.org/338483002/diff/1480001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/338483002/diff/1480001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:2985: + Middleboxes may tamper response sent by data reduction proxy. Tamper detect No need to explain the background here. Just say what it is accurately. For this one, it will be <summary> The total number of HTTP responses that have been checked for tampering. This assumes a data reduction proxy injected fingerprint is not tampered. Only data reduction proxy responses with 200 OK response code are checked. <summary> Please double check if this is faithful to your implementation and apply wording for all the others.
https://codereview.chromium.org/338483002/diff/1480001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc (right): https://codereview.chromium.org/338483002/diff/1480001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:772: "Check the case response matches the fingerprint completely.", On 2014/07/24 07:11:58, bolian wrote: > Is this the same the one above? Done. https://codereview.chromium.org/338483002/diff/1480001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/338483002/diff/1480001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:2985: + Middleboxes may tamper response sent by data reduction proxy. Tamper detect On 2014/07/24 07:11:58, bolian wrote: > No need to explain the background here. Just say what it is accurately. For this > one, it will be > > <summary> > The total number of HTTP responses that have been checked for tampering. This > assumes a data reduction proxy injected fingerprint is not tampered. Only data > reduction proxy responses with 200 OK response code are checked. > <summary> > > Please double check if this is faithful to your implementation and apply wording > for all the others. Done.
lgtm Thanks! Only minor issues from my point of view. Please wait for Ben's comments. https://codereview.chromium.org/338483002/diff/1520001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/1520001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:20: // Macro for UMA reporting. Depending on |scheme_is_https|, first reports to s/first reports to/it first reports/ https://codereview.chromium.org/338483002/diff/1520001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/1520001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:68: // reported separately, specified by |is_secure_scheme|. document the return value. https://codereview.chromium.org/338483002/diff/1520001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/338483002/diff/1520001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:2986: + for tampering. This assumes data reduction proxy injected fingerprints are add "the" before data reduction proxy in both occurrences. We need the last two sentences in all names that they apply for completion.
Oops, didn't received comments notification yesterday.. Done with minor changes. Waiting for Ben to review. https://codereview.chromium.org/338483002/diff/1520001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/1520001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:20: // Macro for UMA reporting. Depending on |scheme_is_https|, first reports to On 2014/07/24 21:00:15, bolian wrote: > s/first reports to/it first reports/ Done. https://codereview.chromium.org/338483002/diff/1520001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/1520001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:68: // reported separately, specified by |is_secure_scheme|. On 2014/07/24 21:00:15, bolian wrote: > document the return value. Done. https://codereview.chromium.org/338483002/diff/1520001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/338483002/diff/1520001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:2986: + for tampering. This assumes data reduction proxy injected fingerprints are On 2014/07/24 21:00:15, bolian wrote: > add "the" before data reduction proxy in both occurrences. > > We need the last two sentences in all names that they apply for completion. Done.
https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... File components/data_reduction_proxy.gypi (right): https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy.gypi:39: 'data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc', Rename these as data_reduction_proxy_tamper_detection.{cc|h} and rename the class as DataReductionProxyTamperDetection. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:23: // Macro for UMA reporting. Depending on |scheme_is_https|, it first reports to I can't parse "it first reports to histograms events". Do you mean it reports either to https_histogram or http_histogram depending on the scheme and adds events by carrier id. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:42: const bool is_secure_scheme) { scheme_is_https https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:43: DCHECK(headers); are headers ever NULL? When does that happen? https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:47: // If the fingerprint of the Chrome-Proxy header is absent, abort tamper So a middlebox that strips fingerprints won't be detected, right? Add a comment in the .h that says this. It would be better if the client could determine if a response should have fingerprints, but that might be hard. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:83: // number of responses that other fingerprints will be checked. I don't understand. Please clarify. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:127: return tampered; might be better to return a status than a bool. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:130: // Constructor initializes the map of fingerprint name to code. names to codes https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:133: const bool is_secure, scheme_is_https https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:141: fingerprint_name_code_map_ = std::map<std::string, FingerprintCode>(); Why do you need this? https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:180: // Checks whether there are other proxies/middleboxes' name after the data name -> named https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:216: // values of a list headers. The fingerprint is constructed as follows: list headers -> list of headers https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:217: // 1) for each header, gets the string representation of its values (same to same to -> same as OR "using" if that is true https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:219: // 2) concatenates all header's string representation with a ";" delimiter, representation -> representations with a ';' delimiter -> using ";" as a delimiter https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:220: // respect to the order of the header list; what does this mean? "keeping the headers in the order in which they were received" https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:226: // To check whether such fingerprint matches the response that the Chromium such a fingerprint https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:227: // client receives, the Chromium client firstly extracts the header names. For the Chromium client -> the client https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:229: // concatenates them and calculates the MD5 hash value. Compares such hash such --> the https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:231: bool DataReductionProxyTamperDetection::AreOtherHeadersTampered( ValidateOtherHeaders https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:234: DCHECK(fingerprint.size()); DCHECK(!fingerprint.empty()); https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:238: fingerprint.end(), '|'); can you fit all of these params on one line? E.g.,. net::HttpUtil::ValuesIterator it( fingerprint.begin(), fingerprint.end(), '|'); https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:241: // following values are the header names included in fingerprint calculation. in -> in the https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:242: // Make sure there is [base64fingerprint] and it can be decoded. Add a note that you ignore the first value. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:257: header_values += ValuesToSortedString(&response_header_values) + ";"; Why not have ValuesToSortedSTring add the ";" to the end? https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:261: std::string actual_fingerprint = GetMD5(header_values); you could avoid a copy by passing a pointer to the result into GetMD5. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:276: // decoded as an integer at both ends and such two numbers are not equal. I don't understand this last sentence. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:280: // If Content-Length value from data reduction proxy does not exist or it from the data reduction... https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:284: // If there is no Content-Length header received, abort. You can move the "abort" first, here and elsewhere. E.g., "Abort if no Content-Length header was received." https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:360: // Removes the Chrome-Proxy header's fingerprint (action name What if there is no such fingerprint? If you expect there to always be such a fingerprint, verify that there is. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:381: // 1) sorts the values; 1) sort the values https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:382: // 2) concatenates sorted values with a "," delimiter. concatenate https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:385: std::string concatenated_values; Why not make this a parameter. E.g., ValuesToSortedSTring(std::vector..., std::string* concatenated_values); https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:401: return std::string((char*)digest.a, ARRAYSIZE_UNSAFE(digest.a)); Likewise, the output could be a parameter of the method. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:7: // which maybe break correct communication and data transfer between the maybe --> may https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:10: // A high-level description of the tamper detection process works in two steps: Reword: "At a high level, the tamper detection process works in two steps:" https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:11: // 1. The data reduction proxy selects a fraction of responses to analyze; 1. The data reduction proxy selects a fraction of responses to analyze, generates a series of fingerprints for each, and appends them to Chrome-Proxy response headers. 2. The client re-generate the fingerprints using the same method as the proxy, compares them to the fingerprints in the response, and generates UMA. A response is considered to have been tampered with if the fingerprints do not match. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:21: // Four fingerprints are generated at the data reduction proxy side: Four fingerprints are generated by the proxy: https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:30: // whether the response body has been modified or not (assume that different assume --> the code assumes value indicates --> values indicate body -> bodies https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:33: // At the Chromium client side, the fingerprint of the Chrome-Proxy header will At the Chromium client side -> On the client https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:39: // Detected tampering information will be reported to UMA. In general, for each Do you also report when response have not been tampered with? That would be useful as a baseline. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:40: // fingerprint, the Chromium client reports the number of responses that have Chromium client -> client https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:61: // This class detects if the response sent by the data reduction proxy has been This class detects -> Detects the intermediaries -> intermediaries https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:70: static bool DetectAndReport(const net::HttpResponseHeaders* header, header -> headers https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:71: bool is_secure_scheme); broken identation. This should be aligned with the "const" above. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:76: // points to the vector contains the values of the Chrome-Proxy header, but contains -> containing https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:82: unsigned carrier_id, Explain what a carrier id is. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:85: virtual ~DataReductionProxyTamperDetection(); Constructors and destructors should come before other public methods. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:88: friend class DataReductionProxyTamperDetectionTest; Why do you need this? Why isn't friending the specific tests enough? https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:110: CHROMEPROXY = 1, /* Code of fingerprint of the Chrome-Proxy header */ prefix all of these so there is no confusion. E.g., FINGERPRINT_CHROMEPROXY. Also remove "Code of fingerprint of" from all comments. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:114: NONEXIST = 5, What does NONEEXIST mean? Also, rename these as FINGERPRINT_CHROME_PROXY, FINGERPRINT_VIA, FINGERPRINT_OTHER_HEADERS, FINGERPRINT_CONTENT_LENGTH, and FINGERPRINT_NONE_EXIST. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:117: // Returns true if the Chrome-Proxy header has been tampered with. Change the comment here and below if you change the name. It should be something like: Returns the result of validating the Chrome-Proxy header. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:118: bool IsChromeProxyHeaderTampered(const std::string& fingerprint) const; This function and all similar ones need new names. One option is to have a function like: TamperDetectionResult ValidateChromeProxyHeader(); This is better if you expect returns to have more than two values. E.g., the return can be a status and you could pass in a pointer to write more information for deteected tampering. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:120: void ReportChromeProxyHeaderTamperedUMA() const; rename as ReportUMAforChromeProxyHeaderValidation(); Do the same below. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:145: // fingerprint to the Chrome-Proxy header, so at the Chromium client side, at the Chromium client --> on the client https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:148: static void RemoveChromeProxyFingerprint(std::vector<std::string>* values); can this method be const? https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:151: static std::string ValuesToSortedString(std::vector<std::string>* values); can this method be const? https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:155: static std::string GetMD5(const std::string& input); can this method be const? https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:167: const bool is_secure_scheme_; can you just be more explicit and rename this scheme_is_https_ ? https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:173: // header value removed. Save it as a temporary result to avoid parsing the I don't understand this comment. Do you mean: The memoized Chrome-Proxy header with the fingerprint of the Chrome-Proxy header removed. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:177: // Maps a fingerprint name (string) to a fingerprint code (enum). Why? What uses this? https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:180: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc (right): https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:29: void HeadersToRaw(std::string* headers) { This seems to be used in a few places by the data reduction proxy. Make this a method in the headers file. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:36: std::string GetEncoded(const std::string& input) { Why do you need this in the tests? Add the answer to the comment. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:47: void ReplaceWithEncodedString(std::string* input) { Why is this needed? https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:68: std::vector<std::string> StringsToVector(std::string* input) { Why is this needed? Here and above, I would have thought that you'd be testing this logic, not recreating it. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:71: if (input[i].size()) { i = 0 https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:153: // Tests function IsChromeProxyHeaderTampered. Be sure to update comments like this one when you rename the functions. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:290: for (size_t i=0; i<ARRAYSIZE_UNSAFE(test); ++i) { i = 0 https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:293: DataReductionProxyTamperDetectionTest::TestFingerprintCommon(test[i], move test[i] to the beginning of the next line. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:401: for (size_t i=0; i<ARRAYSIZE_UNSAFE(test); ++i) { i = 0 https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:529: for (size_t i=0; i<ARRAYSIZE_UNSAFE(test); ++i) { i = 0 https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:531: DataReductionProxyTamperDetectionTest::TestFingerprintCommon(test[i], Move test[i] to the next line. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:667: for (size_t i=0; i<ARRAYSIZE_UNSAFE(test); ++i) { i = 0 https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:671: std::string output_string = DataReductionProxyTamperDetection:: Can DataReduction.. fit on the next line? https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:717: for (size_t i=0; i<ARRAYSIZE_UNSAFE(test); ++i) { i = 0 https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:796: for (size_t i=0; i<ARRAYSIZE_UNSAFE(test); ++i) { i = 0 https://codereview.chromium.org/338483002/diff/1600001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/338483002/diff/1600001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:3168: + are not tampered. Only the data reduction proxy responses with 200 OK are not tampered --> have not been tampered with. Here and below.
https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... File components/data_reduction_proxy.gypi (right): https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy.gypi:39: 'data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc', On 2014/07/28 21:56:47, bengr1 wrote: > Rename these as data_reduction_proxy_tamper_detection.{cc|h} and rename the > class as DataReductionProxyTamperDetection. Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc (right): https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:23: // Macro for UMA reporting. Depending on |scheme_is_https|, it first reports to On 2014/07/28 21:56:47, bengr1 wrote: > I can't parse "it first reports to histograms events". Do you mean it reports > either to https_histogram or http_histogram depending on the scheme and adds > events by carrier id. Changed. Two things: 1) report to |http_histogram| by |carrier_id| and then report total number to |http_histogram|_Total; 2) HTTP and HTTPS responses are reported to different histograms separately. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:42: const bool is_secure_scheme) { On 2014/07/28 21:56:48, bengr1 wrote: > scheme_is_https Done. Also with .h file https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:43: DCHECK(headers); On 2014/07/28 21:56:48, bengr1 wrote: > are headers ever NULL? When does that happen? It should not happen. Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:47: // If the fingerprint of the Chrome-Proxy header is absent, abort tamper On 2014/07/28 21:56:49, bengr1 wrote: > So a middlebox that strips fingerprints won't be detected, right? Add a comment > in the .h that says this. It would be better if the client could determine if a > response should have fingerprints, but that might be hard. Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:83: // number of responses that other fingerprints will be checked. On 2014/07/28 21:56:47, bengr1 wrote: > I don't understand. Please clarify. Changed. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:127: return tampered; On 2014/07/28 21:56:48, bengr1 wrote: > might be better to return a status than a bool. Acknowledged. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:130: // Constructor initializes the map of fingerprint name to code. On 2014/07/28 21:56:49, bengr1 wrote: > names to codes Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:133: const bool is_secure, On 2014/07/28 21:56:47, bengr1 wrote: > scheme_is_https Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:141: fingerprint_name_code_map_ = std::map<std::string, FingerprintCode>(); On 2014/07/28 21:56:47, bengr1 wrote: > Why do you need this? Previously need this for "SWITCH", now I changed to "IF", so removed. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:180: // Checks whether there are other proxies/middleboxes' name after the data On 2014/07/28 21:56:47, bengr1 wrote: > name -> named Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:216: // values of a list headers. The fingerprint is constructed as follows: On 2014/07/28 21:56:47, bengr1 wrote: > list headers -> list of headers Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:217: // 1) for each header, gets the string representation of its values (same to On 2014/07/28 21:56:49, bengr1 wrote: > same to -> same as OR "using" if that is true Done. I'm explaining fingerprint generation, which happens at Flywheel side. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:219: // 2) concatenates all header's string representation with a ";" delimiter, On 2014/07/28 21:56:48, bengr1 wrote: > representation -> representations > with a ';' delimiter -> using ";" as a delimiter Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:220: // respect to the order of the header list; On 2014/07/28 21:56:48, bengr1 wrote: > what does this mean? "keeping the headers in the order in which they were > received" I mean the order of that header list. Header list is: header_a, header_b, header_c. Then when we concatenate the string representation of each header, we need to concatenate them according to header list's order, i.e., string representation of header_a first, then header_b, then header_c. But I think it's clear so I removed the sentence. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:226: // To check whether such fingerprint matches the response that the Chromium On 2014/07/28 21:56:49, bengr1 wrote: > such a fingerprint Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:227: // client receives, the Chromium client firstly extracts the header names. For On 2014/07/28 21:56:47, bengr1 wrote: > the Chromium client -> the client Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:229: // concatenates them and calculates the MD5 hash value. Compares such hash On 2014/07/28 21:56:47, bengr1 wrote: > such --> the Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:231: bool DataReductionProxyTamperDetection::AreOtherHeadersTampered( On 2014/07/28 21:56:48, bengr1 wrote: > ValidateOtherHeaders Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:234: DCHECK(fingerprint.size()); On 2014/07/28 21:56:48, bengr1 wrote: > DCHECK(!fingerprint.empty()); Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:238: fingerprint.end(), '|'); On 2014/07/28 21:56:48, bengr1 wrote: > can you fit all of these params on one line? E.g.,. > > net::HttpUtil::ValuesIterator it( > fingerprint.begin(), fingerprint.end(), '|'); Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:241: // following values are the header names included in fingerprint calculation. On 2014/07/28 21:56:47, bengr1 wrote: > in -> in the Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:242: // Make sure there is [base64fingerprint] and it can be decoded. On 2014/07/28 21:56:47, bengr1 wrote: > Add a note that you ignore the first value. Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:257: header_values += ValuesToSortedString(&response_header_values) + ";"; On 2014/07/28 21:56:48, bengr1 wrote: > Why not have ValuesToSortedSTring add the ";" to the end? ValuesToSortedString generates a representation of a header. In this ValiadateOtherHeaders function, we are dealing with multiple headers, each header's string is generated by ValuesToSortedString, we use delimiter ";" to concatenate strings of multiple headers: [representation of header A];[representation of header B];[representation of header C]; I think right now it's clearer, because we have representation for each header, and the delimiter. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:261: std::string actual_fingerprint = GetMD5(header_values); On 2014/07/28 21:56:47, bengr1 wrote: > you could avoid a copy by passing a pointer to the result into GetMD5. Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:276: // decoded as an integer at both ends and such two numbers are not equal. On 2014/07/28 21:56:48, bengr1 wrote: > I don't understand this last sentence. Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:280: // If Content-Length value from data reduction proxy does not exist or it On 2014/07/28 21:56:47, bengr1 wrote: > from the data reduction... Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:284: // If there is no Content-Length header received, abort. On 2014/07/28 21:56:48, bengr1 wrote: > You can move the "abort" first, here and elsewhere. E.g., "Abort if no > Content-Length header was received." Done. all other places. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:360: // Removes the Chrome-Proxy header's fingerprint (action name On 2014/07/28 21:56:48, bengr1 wrote: > What if there is no such fingerprint? If you expect there to always be such a > fingerprint, verify that there is. Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:381: // 1) sorts the values; On 2014/07/28 21:56:49, bengr1 wrote: > 1) sort the values Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:382: // 2) concatenates sorted values with a "," delimiter. On 2014/07/28 21:56:48, bengr1 wrote: > concatenate Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:385: std::string concatenated_values; On 2014/07/28 21:56:49, bengr1 wrote: > Why not make this a parameter. E.g., ValuesToSortedSTring(std::vector..., > std::string* concatenated_values); Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc:401: return std::string((char*)digest.a, ARRAYSIZE_UNSAFE(digest.a)); On 2014/07/28 21:56:48, bengr1 wrote: > Likewise, the output could be a parameter of the method. To avoid return value copying, right? Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h (right): https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:7: // which maybe break correct communication and data transfer between the On 2014/07/28 21:56:49, bengr1 wrote: > maybe --> may Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:10: // A high-level description of the tamper detection process works in two steps: On 2014/07/28 21:56:50, bengr1 wrote: > Reword: > > "At a high level, the tamper detection process works in two steps:" Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:11: // 1. The data reduction proxy selects a fraction of responses to analyze; On 2014/07/28 21:56:51, bengr1 wrote: > 1. The data reduction proxy selects a fraction of responses to analyze, > generates a series of fingerprints for each, and appends them to Chrome-Proxy > response headers. > > 2. The client re-generate the fingerprints using the same method as the proxy, > compares them to the fingerprints in the response, and generates UMA. A response > is considered to have been tampered with if the fingerprints do not match. > Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:21: // Four fingerprints are generated at the data reduction proxy side: On 2014/07/28 21:56:49, bengr1 wrote: > Four fingerprints are generated by the proxy: Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:30: // whether the response body has been modified or not (assume that different On 2014/07/28 21:56:49, bengr1 wrote: > assume --> the code assumes > value indicates --> values indicate > body -> bodies Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:33: // At the Chromium client side, the fingerprint of the Chrome-Proxy header will On 2014/07/28 21:56:51, bengr1 wrote: > At the Chromium client side -> On the client Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:39: // Detected tampering information will be reported to UMA. In general, for each On 2014/07/28 21:56:50, bengr1 wrote: > Do you also report when response have not been tampered with? That would be > useful as a baseline. Right now, no. But we have total number of "checked" responses, so we know the number of responses that have not been tampered with. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:40: // fingerprint, the Chromium client reports the number of responses that have On 2014/07/28 21:56:49, bengr1 wrote: > Chromium client -> client Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:61: // This class detects if the response sent by the data reduction proxy has been On 2014/07/28 21:56:51, bengr1 wrote: > This class detects -> Detects > the intermediaries -> intermediaries Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:70: static bool DetectAndReport(const net::HttpResponseHeaders* header, On 2014/07/28 21:56:51, bengr1 wrote: > header -> headers Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:71: bool is_secure_scheme); On 2014/07/28 21:56:49, bengr1 wrote: > broken identation. This should be aligned with the "const" above. Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:76: // points to the vector contains the values of the Chrome-Proxy header, but On 2014/07/28 21:56:50, bengr1 wrote: > contains -> containing Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:82: unsigned carrier_id, On 2014/07/28 21:56:50, bengr1 wrote: > Explain what a carrier id is. Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:85: virtual ~DataReductionProxyTamperDetection(); On 2014/07/28 21:56:50, bengr1 wrote: > Constructors and destructors should come before other public methods. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:88: friend class DataReductionProxyTamperDetectionTest; On 2014/07/28 21:56:50, bengr1 wrote: > Why do you need this? Why isn't friending the specific tests enough? Previously I have a common function for testing all fingerprints-checking functions. So I need to "friend" that function. Right now I removed that function, each fingerprint checking unittest becomes longer (since I removed common checking function). Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:110: CHROMEPROXY = 1, /* Code of fingerprint of the Chrome-Proxy header */ On 2014/07/28 21:56:51, bengr1 wrote: > prefix all of these so there is no confusion. E.g., FINGERPRINT_CHROMEPROXY. > Also remove "Code of fingerprint of" from all comments. Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:114: NONEXIST = 5, On 2014/07/28 21:56:50, bengr1 wrote: > What does NONEEXIST mean? Also, rename these as FINGERPRINT_CHROME_PROXY, > FINGERPRINT_VIA, FINGERPRINT_OTHER_HEADERS, FINGERPRINT_CONTENT_LENGTH, and > FINGERPRINT_NONE_EXIST. Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:117: // Returns true if the Chrome-Proxy header has been tampered with. On 2014/07/28 21:56:50, bengr1 wrote: > Change the comment here and below if you change the name. It should be something > like: > > Returns the result of validating the Chrome-Proxy header. Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:118: bool IsChromeProxyHeaderTampered(const std::string& fingerprint) const; On 2014/07/28 21:56:50, bengr1 wrote: > This function and all similar ones need new names. One option is to have a > function like: > > TamperDetectionResult ValidateChromeProxyHeader(); > > This is better if you expect returns to have more than two values. E.g., the > return can be a status and you could pass in a pointer to write more information > for deteected tampering. Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:120: void ReportChromeProxyHeaderTamperedUMA() const; On 2014/07/28 21:56:50, bengr1 wrote: > rename as ReportUMAforChromeProxyHeaderValidation(); Do the same below. Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:145: // fingerprint to the Chrome-Proxy header, so at the Chromium client side, On 2014/07/28 21:56:49, bengr1 wrote: > at the Chromium client --> on the client Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:148: static void RemoveChromeProxyFingerprint(std::vector<std::string>* values); On 2014/07/28 21:56:51, bengr1 wrote: > can this method be const? Acknowledged. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:151: static std::string ValuesToSortedString(std::vector<std::string>* values); On 2014/07/28 21:56:50, bengr1 wrote: > can this method be const? Acknowledged. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:155: static std::string GetMD5(const std::string& input); On 2014/07/28 21:56:50, bengr1 wrote: > can this method be const? Acknowledged. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:167: const bool is_secure_scheme_; On 2014/07/28 21:56:51, bengr1 wrote: > can you just be more explicit and rename this scheme_is_https_ ? Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:173: // header value removed. Save it as a temporary result to avoid parsing the On 2014/07/28 21:56:49, bengr1 wrote: > I don't understand this comment. Do you mean: > > The memoized Chrome-Proxy header with the fingerprint of the Chrome-Proxy header > removed. Exactly！ Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:177: // Maps a fingerprint name (string) to a fingerprint code (enum). On 2014/07/28 21:56:50, bengr1 wrote: > Why? What uses this? Removed. Right now using three IF instead of SWITCH. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h:180: On 2014/07/28 21:56:49, bengr1 wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc (right): https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:29: void HeadersToRaw(std::string* headers) { On 2014/07/28 21:56:52, bengr1 wrote: > This seems to be used in a few places by the data reduction proxy. Make this a > method in the headers file. I think this function is more like an utility function for unittest. So I'm not sure if we want to put it into real codes. It's also in http_response_unittest. WDYT? But let me know if you want to move this into headers, I can help :) https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:36: std::string GetEncoded(const std::string& input) { On 2014/07/28 21:56:52, bengr1 wrote: > Why do you need this in the tests? Add the answer to the comment. Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:47: void ReplaceWithEncodedString(std::string* input) { On 2014/07/28 21:56:52, bengr1 wrote: > Why is this needed? Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:68: std::vector<std::string> StringsToVector(std::string* input) { On 2014/07/28 21:56:52, bengr1 wrote: > Why is this needed? Here and above, I would have thought that you'd be testing > this logic, not recreating it. Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:71: if (input[i].size()) { On 2014/07/28 21:56:52, bengr1 wrote: > i = 0 Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:153: // Tests function IsChromeProxyHeaderTampered. On 2014/07/28 21:56:52, bengr1 wrote: > Be sure to update comments like this one when you rename the functions. Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:290: for (size_t i=0; i<ARRAYSIZE_UNSAFE(test); ++i) { On 2014/07/28 21:56:52, bengr1 wrote: > i = 0 Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:293: DataReductionProxyTamperDetectionTest::TestFingerprintCommon(test[i], On 2014/07/28 21:56:52, bengr1 wrote: > move test[i] to the beginning of the next line. Function removed. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:401: for (size_t i=0; i<ARRAYSIZE_UNSAFE(test); ++i) { On 2014/07/28 21:56:52, bengr1 wrote: > i = 0 Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:529: for (size_t i=0; i<ARRAYSIZE_UNSAFE(test); ++i) { On 2014/07/28 21:56:52, bengr1 wrote: > i = 0 Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:531: DataReductionProxyTamperDetectionTest::TestFingerprintCommon(test[i], On 2014/07/28 21:56:52, bengr1 wrote: > Move test[i] to the next line. Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:667: for (size_t i=0; i<ARRAYSIZE_UNSAFE(test); ++i) { On 2014/07/28 21:56:53, bengr1 wrote: > i = 0 Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:671: std::string output_string = DataReductionProxyTamperDetection:: On 2014/07/28 21:56:52, bengr1 wrote: > Can DataReduction.. fit on the next line? Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:717: for (size_t i=0; i<ARRAYSIZE_UNSAFE(test); ++i) { On 2014/07/28 21:56:52, bengr1 wrote: > i = 0 Done. https://codereview.chromium.org/338483002/diff/1600001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect_unittest.cc:796: for (size_t i=0; i<ARRAYSIZE_UNSAFE(test); ++i) { On 2014/07/28 21:56:52, bengr1 wrote: > i = 0 Done. https://codereview.chromium.org/338483002/diff/1600001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/338483002/diff/1600001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:3168: + are not tampered. Only the data reduction proxy responses with 200 OK On 2014/07/28 21:56:53, bengr1 wrote: > are not tampered --> have not been tampered with. > > Here and below. Done.
https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc (right): https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:50: return false; add curly braces https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:59: headers, put on one line https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:211: // "|" delimiter would not occur in base64 as well as header names. Say: According to RFC 2616, "|" is not a valid character in a header name, so there is not ambiguity in using it as a delimiter. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:229: // Sorts the values and concatenate them, with delimiter ";". ";" would not Fix the commment because ';' can occur in header values. ";" may occur in a header value and thus two different sets of header values could map to the same representation. This should be very rare. Add a TODO(xingx) to find an unambiguous representation. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:269: Remove blank line. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.h (right): https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.h:106: // Returns the result of validating Chrome-Proxy header.. .. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.h:114: bool* has_chrome_proxy_via_header) const; indentation https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.h:120: // Reports UMA for tampering of values of the list of headers. add blank line above https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.h:125: // Reports UMA for tampering of the Content-Length header. add blank line above https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc (right): https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:13: #include "base/md5.h" #include "base/memory/scoped_ptr.h" https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:237: net::HttpResponseHeaders* headers = scoped_ptr<net::HttpResponseHeaders> headers(new ...); https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:243: test[i].received_fingerprint); indentation https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:362: new net::HttpResponseHeaders(raw_headers); scoped_ptr<net::HttpResponseHeaders> headers(new ...); https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:495: new net::HttpResponseHeaders(raw_headers); scoped_ptr<net::HttpResponseHeaders> headers(new ...); https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:571: new net::HttpResponseHeaders(raw_headers); scoped_ptr<net::HttpResponseHeaders> headers(new ...); https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:609: remove blank line https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:612: remove blank line https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:660: new net::HttpResponseHeaders(raw_headers); scoped_ptr<net::HttpResponseHeaders> headers(new ...); https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:740: new net::HttpResponseHeaders(raw_headers); scoped_ptr<net::HttpResponseHeaders> headers(new ...);
https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc (right): https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:50: return false; On 2014/08/04 16:54:21, bengr1 wrote: > add curly braces Done. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:59: headers, On 2014/08/04 16:54:21, bengr1 wrote: > put on one line Done. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:211: // "|" delimiter would not occur in base64 as well as header names. On 2014/08/04 16:54:21, bengr1 wrote: > Say: According to RFC 2616, "|" is not a valid character in a header name, so > there is not ambiguity in using it as a delimiter. Done. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:229: // Sorts the values and concatenate them, with delimiter ";". ";" would not On 2014/08/04 16:54:21, bengr1 wrote: > Fix the commment because ';' can occur in header values. > > ";" may occur in a header value and thus two different sets of header values > could map to the same representation. This should be very rare. > > Add a TODO(xingx) to find an unambiguous representation. Done. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:269: On 2014/08/04 16:54:21, bengr1 wrote: > Remove blank line. Done. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.h (right): https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.h:106: // Returns the result of validating Chrome-Proxy header.. On 2014/08/04 16:54:21, bengr1 wrote: > .. Done. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.h:114: bool* has_chrome_proxy_via_header) const; On 2014/08/04 16:54:21, bengr1 wrote: > indentation Done. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.h:120: // Reports UMA for tampering of values of the list of headers. On 2014/08/04 16:54:21, bengr1 wrote: > add blank line above Done. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.h:125: // Reports UMA for tampering of the Content-Length header. On 2014/08/04 16:54:21, bengr1 wrote: > add blank line above Done. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc (right): https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:13: #include "base/md5.h" On 2014/08/04 16:54:21, bengr1 wrote: > #include "base/memory/scoped_ptr.h" Done. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:237: net::HttpResponseHeaders* headers = On 2014/08/04 16:54:21, bengr1 wrote: > scoped_ptr<net::HttpResponseHeaders> headers(new ...); Done. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:243: test[i].received_fingerprint); On 2014/08/04 16:54:21, bengr1 wrote: > indentation Done. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:362: new net::HttpResponseHeaders(raw_headers); On 2014/08/04 16:54:22, bengr1 wrote: > scoped_ptr<net::HttpResponseHeaders> headers(new ...); Done. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:495: new net::HttpResponseHeaders(raw_headers); On 2014/08/04 16:54:22, bengr1 wrote: > scoped_ptr<net::HttpResponseHeaders> headers(new ...); Done. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:571: new net::HttpResponseHeaders(raw_headers); On 2014/08/04 16:54:21, bengr1 wrote: > scoped_ptr<net::HttpResponseHeaders> headers(new ...); Done. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:609: On 2014/08/04 16:54:22, bengr1 wrote: > remove blank line Done. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:612: On 2014/08/04 16:54:22, bengr1 wrote: > remove blank line Done. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:660: new net::HttpResponseHeaders(raw_headers); On 2014/08/04 16:54:21, bengr1 wrote: > scoped_ptr<net::HttpResponseHeaders> headers(new ...); Done. https://codereview.chromium.org/338483002/diff/1660001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:740: new net::HttpResponseHeaders(raw_headers); On 2014/08/04 16:54:21, bengr1 wrote: > scoped_ptr<net::HttpResponseHeaders> headers(new ...); Done.
Hi, Alexei, Could you please review my UMA changes: /tools/metrics/histograms/histograms.xml Codes of reporting: components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc, line 27 Thanks, Xing xingx@google.com xingx@chromium.org
https://codereview.chromium.org/338483002/diff/1680001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/338483002/diff/1680001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:3679: +<histogram name="DataReductionProxy.HTTPSHeaderTampered_Via_Total"> Can you use a histogram-suffixes tag to avoid duplication of these histograms? It allows you to specify a base histogram prefix (e.g. DataReductionProxy.HTTPSHeaderTampered) and a set of suffixes that added for specializations of that histogram.
Hi Alexei, I've changed to using suffix, please take another look. Thanks, Xing https://codereview.chromium.org/338483002/diff/1680001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/338483002/diff/1680001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:3679: +<histogram name="DataReductionProxy.HTTPSHeaderTampered_Via_Total"> On 2014/08/04 18:48:56, Alexei Svitkine wrote: > Can you use a histogram-suffixes tag to avoid duplication of these histograms? > It allows you to specify a base histogram prefix (e.g. > DataReductionProxy.HTTPSHeaderTampered) and a set of suffixes that added for > specializations of that histogram. Done.
On 2014/08/04 19:23:40, xingx1 wrote: > Hi Alexei, > > I've changed to using suffix, please take another look. > > Thanks, > Xing > > https://codereview.chromium.org/338483002/diff/1680001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/338483002/diff/1680001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:3679: +<histogram > name="DataReductionProxy.HTTPSHeaderTampered_Via_Total"> > On 2014/08/04 18:48:56, Alexei Svitkine wrote: > > Can you use a histogram-suffixes tag to avoid duplication of these histograms? > > It allows you to specify a base histogram prefix (e.g. > > DataReductionProxy.HTTPSHeaderTampered) and a set of suffixes that added for > > specializations of that histogram. > > Done. lgtm
https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc (right): https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:27: #define REPORT_TAMPER_DETECTION_UMA(scheme_is_https, http_histogram, https_histogram, carrier_id) \ Nit: This goes over the 80 character limit. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:74: "DataReductionProxy.HTTPHeaderTamperDetection", Nit: I think it would be better to name these: DataReductionProxy.HeaderTamperDetectionHTTP and DataReductionProxy.HeaderTamperDetectionHTTPS I think it makes it more readable since people will be looking more at the last part of the name (near the suffix). https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:117: DCHECK(headers); Why not take them as a const ref? Comment applies throughout for any const pointers that you don't expect to be null. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:118: }; Nit: No ; needed https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:126: // not. Comment should be in the header, not here. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:159: const std::string& fingerprint, bool* has_chrome_proxy_via_header) const { Nit: 1 param per line in the function signature if first param is on a new line. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:207: std::string received_fingerprint; Nit: MOve this closer to where it's used https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:218: base::Base64Decode(it.value(), &received_fingerprint))) { Nit: !(a && b) -> !a || !b https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:262: if (base::StringToInt(fingerprint, &received_content_length_fingerprint)) { Nit: Make it an early return to avoid excessive nesting. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:304: else if (mime_type.compare("text/css") == 0) { Nit: Put else on the same line as the } above. You can move the comment into the block or just omit it. Please fix throughout. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:352: *output = std::string((char*)digest.a, ARRAYSIZE_UNSAFE(digest.a)); Nit: C-style casts are not allowed. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:356: const net::HttpResponseHeaders* headers, const std::string& header_name) { Nit: 1 param per line https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc (right): https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:56: temp = input->find("[", start + 1); Nit: Indent is wrong. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:72: std::vector<std::string> StringsToVector(std::string& values) { Nit: Pass by const ref? https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:76: size_t now = -1, next; Nit: 1 per line. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:77: while((next = values.find(",", now+1)) != std::string::npos) Nit: Spaces around + and after while. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:78: { Nit: Put { on previous line. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:79: ret.push_back(values.substr(now+1, next-now-1)); Nit: spaces around operator https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:81: }; Nit: No ; https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:491: ReplaceWithEncodedString(&(test[i].received_fingerprint)); Nit: Is inner set of parens needed? https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:731: net::android::RegisterNetworkLibrary(env); Is this safe to do multiple times? gtest allows multiple runs of a test (via a param). Can you check that this doesn't break in that case?
https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc (right): https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:27: #define REPORT_TAMPER_DETECTION_UMA(scheme_is_https, http_histogram, https_histogram, carrier_id) \ On 2014/08/04 21:07:51, Alexei Svitkine wrote: > Nit: This goes over the 80 character limit. Done. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:74: "DataReductionProxy.HTTPHeaderTamperDetection", On 2014/08/04 21:07:51, Alexei Svitkine wrote: > Nit: I think it would be better to name these: > > DataReductionProxy.HeaderTamperDetectionHTTP > and > DataReductionProxy.HeaderTamperDetectionHTTPS > > I think it makes it more readable since people will be looking more at the last > part of the name (near the suffix). Done. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:117: DCHECK(headers); On 2014/08/04 21:07:51, Alexei Svitkine wrote: > Why not take them as a const ref? Comment applies throughout for any const > pointers that you don't expect to be null. Ack. To make it consistent with other files in data_reduction_proxy folder, I'm leaving it as is :) https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:118: }; On 2014/08/04 21:07:51, Alexei Svitkine wrote: > Nit: No ; needed Done. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:126: // not. On 2014/08/04 21:07:52, Alexei Svitkine wrote: > Comment should be in the header, not here. Removed general description, leave detailed implementation comments here in .cc file. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:159: const std::string& fingerprint, bool* has_chrome_proxy_via_header) const { On 2014/08/04 21:07:51, Alexei Svitkine wrote: > Nit: 1 param per line in the function signature if first param is on a new line. Done. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:207: std::string received_fingerprint; On 2014/08/04 21:07:51, Alexei Svitkine wrote: > Nit: MOve this closer to where it's used Done. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:218: base::Base64Decode(it.value(), &received_fingerprint))) { On 2014/08/04 21:07:51, Alexei Svitkine wrote: > Nit: !(a && b) -> !a || !b Done. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:262: if (base::StringToInt(fingerprint, &received_content_length_fingerprint)) { On 2014/08/04 21:07:51, Alexei Svitkine wrote: > Nit: Make it an early return to avoid excessive nesting. Done. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:304: else if (mime_type.compare("text/css") == 0) { On 2014/08/04 21:07:51, Alexei Svitkine wrote: > Nit: Put else on the same line as the } above. You can move the comment into the > block or just omit it. Please fix throughout. Done. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:352: *output = std::string((char*)digest.a, ARRAYSIZE_UNSAFE(digest.a)); On 2014/08/04 21:07:51, Alexei Svitkine wrote: > Nit: C-style casts are not allowed. Done. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection.cc:356: const net::HttpResponseHeaders* headers, const std::string& header_name) { On 2014/08/04 21:07:51, Alexei Svitkine wrote: > Nit: 1 param per line Done. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc (right): https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:56: temp = input->find("[", start + 1); On 2014/08/04 21:07:52, Alexei Svitkine wrote: > Nit: Indent is wrong. Done. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:72: std::vector<std::string> StringsToVector(std::string& values) { On 2014/08/04 21:07:52, Alexei Svitkine wrote: > Nit: Pass by const ref? Done. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:76: size_t now = -1, next; On 2014/08/04 21:07:52, Alexei Svitkine wrote: > Nit: 1 per line. Done. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:77: while((next = values.find(",", now+1)) != std::string::npos) On 2014/08/04 21:07:52, Alexei Svitkine wrote: > Nit: Spaces around + and after while. Done. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:78: { On 2014/08/04 21:07:52, Alexei Svitkine wrote: > Nit: Put { on previous line. Done. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:79: ret.push_back(values.substr(now+1, next-now-1)); On 2014/08/04 21:07:52, Alexei Svitkine wrote: > Nit: spaces around operator Done. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:81: }; On 2014/08/04 21:07:52, Alexei Svitkine wrote: > Nit: No ; Done. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:491: ReplaceWithEncodedString(&(test[i].received_fingerprint)); On 2014/08/04 21:07:52, Alexei Svitkine wrote: > Nit: Is inner set of parens needed? Done. https://codereview.chromium.org/338483002/diff/1700001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:731: net::android::RegisterNetworkLibrary(env); On 2014/08/04 21:07:52, Alexei Svitkine wrote: > Is this safe to do multiple times? gtest allows multiple runs of a test (via a > param). Can you check that this doesn't break in that case? Didn't figure out the parameter of running times. But I changed it to InitEnv (copied from https://code.google.com/p/chromium/codesearch#chromium/src/net/android/keysto...)
General comment: This is adding a lot of new histograms. Can you provide an estimate of how much more data will be included in each UMA report as a result of this? https://codereview.chromium.org/338483002/diff/1720001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc (right): https://codereview.chromium.org/338483002/diff/1720001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:60: else Nit: No else needed after break https://codereview.chromium.org/338483002/diff/1720001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/338483002/diff/1720001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:3209: + For each carrier, the total number of HTTP responses that have been checked Can you add an enum definition to histograms.xml documenting a set of known carrier ids and reference it from this histogram (and every other "For each carrier" one too). https://codereview.chromium.org/338483002/diff/1720001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:3240: +<histogram name="DataReductionProxy.HeaderTamperDetectionHTTPS_Total"> Perhaps the _Total versions of the histograms can be specified via a suffix list too? https://codereview.chromium.org/338483002/diff/1720001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:49719: + <suffix name="ContentLength_Other_Total" If you add a separate histogram_suffixes tag for _Total, then you can simply list DataReductionProxy.HeaderTamperedHTTP_ContentLength there and same for others, rather than having everything here.
Q: General comment: This is adding a lot of new histograms. Can you provide an estimate of how much more data will be included in each UMA report as a result of this? A: each UMA report will have at most 20 integers. But for most of the cases, there will not be any changes on newly added histograms (only a small fraction of responses will go through the process, the fraction is controlled by the data reduction proxy). https://codereview.chromium.org/338483002/diff/1720001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc (right): https://codereview.chromium.org/338483002/diff/1720001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:60: else On 2014/08/05 15:24:10, Alexei Svitkine wrote: > Nit: No else needed after break Done. https://codereview.chromium.org/338483002/diff/1720001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/338483002/diff/1720001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:3209: + For each carrier, the total number of HTTP responses that have been checked On 2014/08/05 15:24:10, Alexei Svitkine wrote: > Can you add an enum definition to histograms.xml documenting a set of known > carrier ids and reference it from this histogram (and every other "For each > carrier" one too). There are hundreds (or maybe more :) ) of them (http://en.wikipedia.org/wiki/Mobile_country_code). And there exist histograms use the same buckets (check "NCN.NetworkOperatorMCCMNC") I'm leaving it as is for now. https://codereview.chromium.org/338483002/diff/1720001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:3240: +<histogram name="DataReductionProxy.HeaderTamperDetectionHTTPS_Total"> On 2014/08/05 15:24:10, Alexei Svitkine wrote: > Perhaps the _Total versions of the histograms can be specified via a suffix list > too? Done. https://codereview.chromium.org/338483002/diff/1720001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:49719: + <suffix name="ContentLength_Other_Total" On 2014/08/05 15:24:10, Alexei Svitkine wrote: > If you add a separate histogram_suffixes tag for _Total, then you can simply > list DataReductionProxy.HeaderTamperedHTTP_ContentLength there and same for > others, rather than having everything here. Done.
LGTM % remaining comments. Thanks! https://codereview.chromium.org/338483002/diff/1720001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/338483002/diff/1720001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:3209: + For each carrier, the total number of HTTP responses that have been checked On 2014/08/05 18:08:50, xingx1 wrote: > On 2014/08/05 15:24:10, Alexei Svitkine wrote: > > Can you add an enum definition to histograms.xml documenting a set of known > > carrier ids and reference it from this histogram (and every other "For each > > carrier" one too). > > There are hundreds (or maybe more :) ) of them > (http://en.wikipedia.org/wiki/Mobile_country_code). > > And there exist histograms use the same buckets (check > "NCN.NetworkOperatorMCCMNC") > > I'm leaving it as is for now. Okay, please add a sentence to each description pointing at that URL, so people know how to decode them. https://codereview.chromium.org/338483002/diff/1740001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc (right): https://codereview.chromium.org/338483002/diff/1740001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:74: if (values.size() == 0) nit: .empty() https://codereview.chromium.org/338483002/diff/1740001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:85: JNIEnv* InitEnv() { I suspect this probably needs to live in an ifdef android block. Also, you don't seem to be using the return value, so I suggest omitting it. https://codereview.chromium.org/338483002/diff/1740001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:99: class DataReductionProxyTamperDetectionTest : public testing::Test { nit: instead, do this: typedef testing::Test DataReductionProxyTamperDetectionTest; https://codereview.chromium.org/338483002/diff/1740001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/338483002/diff/1740001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:52594: +<histogram_suffixes name="TotalNumber" separator="_"> Nit: Name this element something more meaningful - e.g. DataReductionProxy_TamperingTotal.
https://codereview.chromium.org/338483002/diff/1740001/components/data_reduct... File components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc (right): https://codereview.chromium.org/338483002/diff/1740001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:74: if (values.size() == 0) On 2014/08/05 19:41:42, Alexei Svitkine wrote: > nit: .empty() Done. https://codereview.chromium.org/338483002/diff/1740001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:85: JNIEnv* InitEnv() { On 2014/08/05 19:41:42, Alexei Svitkine wrote: > I suspect this probably needs to live in an ifdef android block. > > Also, you don't seem to be using the return value, so I suggest omitting it. Done. https://codereview.chromium.org/338483002/diff/1740001/components/data_reduct... components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detection_unittest.cc:99: class DataReductionProxyTamperDetectionTest : public testing::Test { On 2014/08/05 19:41:42, Alexei Svitkine wrote: > nit: instead, do this: > > typedef testing::Test DataReductionProxyTamperDetectionTest; Acknowledged. https://codereview.chromium.org/338483002/diff/1740001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/338483002/diff/1740001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:52594: +<histogram_suffixes name="TotalNumber" separator="_"> On 2014/08/05 19:41:42, Alexei Svitkine wrote: > Nit: Name this element something more meaningful - e.g. > DataReductionProxy_TamperingTotal. Done.
The CQ bit was checked by xingx@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xingx@chromium.org/338483002/2020001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
Message was sent while issue was closed.
Change committed as 288492 |
