Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
361053002:
Elide data reduction proxy credentials from NetLog (Closed)
DescriptionElide data reduction proxy credentials from NetLog
BUG=345907
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281117
Patch Set 1 #Patch Set 2 : Addressed comment #
Total comments: 10
Patch Set 3 : Addressed comments #
Messages
Total messages: 13 (0 generated)
I'm open to how to do a better string replacement with const_interators.
On 2014/07/01 20:31:00, bengr1 wrote: > I'm open to how to do a better string replacement with const_interators. I'm assuming there's a reason for not just hiding the whole thing. We also have a unit test for this that you need to update.
On 2014/07/01 20:37:04, mmenke wrote: > On 2014/07/01 20:31:00, bengr1 wrote: > > I'm open to how to do a better string replacement with const_interators. > > I'm assuming there's a reason for not just hiding the whole thing. > > We also have a unit test for this that you need to update. Ok, I've updated the unit test and changed the redaction logic to emit the same message as other cases.
On 2014/07/01 23:25:52, bengr1 wrote: > On 2014/07/01 20:37:04, mmenke wrote: > > On 2014/07/01 20:31:00, bengr1 wrote: > > > I'm open to how to do a better string replacement with const_interators. > > > > I'm assuming there's a reason for not just hiding the whole thing. > > > > We also have a unit test for this that you need to update. > > Ok, I've updated the unit test and changed the redaction logic to emit the same > message as other cases. How important is this behavior, anyways? If we're really that concerned, we may want to consider a full integration test, if we already have integration tests for flywheel. That would protect against this sort of regression in the future - check the log for the mock key used by tests.
On 2014/07/01 23:53:01, mmenke wrote: > On 2014/07/01 23:25:52, bengr1 wrote: > > On 2014/07/01 20:37:04, mmenke wrote: > > > On 2014/07/01 20:31:00, bengr1 wrote: > > > > I'm open to how to do a better string replacement with const_interators. > > > > > > I'm assuming there's a reason for not just hiding the whole thing. > > > > > > We also have a unit test for this that you need to update. > > > > Ok, I've updated the unit test and changed the redaction logic to emit the > same > > message as other cases. > > How important is this behavior, anyways? If we're really that concerned, we may > want to consider a full integration test, if we already have integration tests > for flywheel. That would protect against this sort of regression in the future > - check the log for the mock key used by tests. Oh, and I'll finish reviewing this tomorrow - not holding off signing off on the CL on your response to that question.
On 2014/07/01 23:53:01, mmenke wrote: > On 2014/07/01 23:25:52, bengr1 wrote: > > On 2014/07/01 20:37:04, mmenke wrote: > > > On 2014/07/01 20:31:00, bengr1 wrote: > > > > I'm open to how to do a better string replacement with const_interators. > > > > > > I'm assuming there's a reason for not just hiding the whole thing. > > > > > > We also have a unit test for this that you need to update. > > > > Ok, I've updated the unit test and changed the redaction logic to emit the > same > > message as other cases. > > How important is this behavior, anyways? If we're really that concerned, we may > want to consider a full integration test, if we already have integration tests > for flywheel. That would protect against this sort of regression in the future > - check the log for the mock key used by tests. There are already some integration tests that use telemetry, and we can add this to the list. The behavior is important.
https://codereview.chromium.org/361053002/diff/40001/net/http/http_log_util.cc File net/http/http_log_util.cc (right): https://codereview.chromium.org/361053002/diff/40001/net/http/http_log_util.c... net/http/http_log_util.cc:69: if (!unconditionally_redacted) { Think you can get rid of this bool, and just use "if (redact_begin != redact_end)" here (If they're the same, you aren't redacting anything, anyways). Optionally, could combine the two ifs... if (redact_begin != redact_end && log_level >= NetLog::LOG_STRIP_PRIVATE_DATA) Then there are only two return statements, which I think is a little cleaner. https://codereview.chromium.org/361053002/diff/40001/net/http/http_log_util_u... File net/http/http_log_util_unittest.cc (left): https://codereview.chromium.org/361053002/diff/40001/net/http/http_log_util_u... net/http/http_log_util_unittest.cc:57: #if !defined(SPDY_PROXY_AUTH_ORIGIN) These !defined cases shouldn't be deleted, but just always enabled, no? https://codereview.chromium.org/361053002/diff/40001/net/http/http_log_util_u... File net/http/http_log_util_unittest.cc (right): https://codereview.chromium.org/361053002/diff/40001/net/http/http_log_util_u... net/http/http_log_util_unittest.cc:13: net::NetLog::LOG_STRIP_PRIVATE_DATA, "Cookie", "name=value")); Hrm...while you're here, mind removing all the net::'s? https://codereview.chromium.org/361053002/diff/40001/net/http/http_log_util_u... net/http/http_log_util_unittest.cc:54: "Chrome-Proxy", "ps=123, sid=456, c=foo, v=bar")); The indentation here is weird. Suggest indenting 4 more than ElideHeaderValueForNetLog. https://codereview.chromium.org/361053002/diff/40001/net/http/http_log_util_u... net/http/http_log_util_unittest.cc:54: "Chrome-Proxy", "ps=123, sid=456, c=foo, v=bar")); Just for the sake of completeness, suggest tests where the sid param is first, and where it's last.
https://codereview.chromium.org/361053002/diff/40001/net/http/http_log_util.cc File net/http/http_log_util.cc (right): https://codereview.chromium.org/361053002/diff/40001/net/http/http_log_util.c... net/http/http_log_util.cc:69: if (!unconditionally_redacted) { On 2014/07/02 18:34:25, mmenke wrote: > Think you can get rid of this bool, and just use "if (redact_begin != > redact_end)" here (If they're the same, you aren't redacting anything, anyways). > > Optionally, could combine the two ifs... > > if (redact_begin != redact_end && log_level >= NetLog::LOG_STRIP_PRIVATE_DATA) > > Then there are only two return statements, which I think is a little cleaner. Done. https://codereview.chromium.org/361053002/diff/40001/net/http/http_log_util_u... File net/http/http_log_util_unittest.cc (left): https://codereview.chromium.org/361053002/diff/40001/net/http/http_log_util_u... net/http/http_log_util_unittest.cc:57: #if !defined(SPDY_PROXY_AUTH_ORIGIN) On 2014/07/02 18:34:25, mmenke wrote: > These !defined cases shouldn't be deleted, but just always enabled, no? Done. https://codereview.chromium.org/361053002/diff/40001/net/http/http_log_util_u... File net/http/http_log_util_unittest.cc (right): https://codereview.chromium.org/361053002/diff/40001/net/http/http_log_util_u... net/http/http_log_util_unittest.cc:13: net::NetLog::LOG_STRIP_PRIVATE_DATA, "Cookie", "name=value")); On 2014/07/02 18:34:25, mmenke wrote: > Hrm...while you're here, mind removing all the net::'s? Done. https://codereview.chromium.org/361053002/diff/40001/net/http/http_log_util_u... net/http/http_log_util_unittest.cc:54: "Chrome-Proxy", "ps=123, sid=456, c=foo, v=bar")); On 2014/07/02 18:34:25, mmenke wrote: > Just for the sake of completeness, suggest tests where the sid param is first, > and where it's last. Done. https://codereview.chromium.org/361053002/diff/40001/net/http/http_log_util_u... net/http/http_log_util_unittest.cc:54: "Chrome-Proxy", "ps=123, sid=456, c=foo, v=bar")); On 2014/07/02 18:34:25, mmenke wrote: > The indentation here is weird. Suggest indenting 4 more than > ElideHeaderValueForNetLog. Done.
LGTM
The CQ bit was checked by bengr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bengr@chromium.org/361053002/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 281117 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
