Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2428103002:
Escape non-ASCII Request header values before attaching them as a NetLog string. (Closed)
DescriptionEscape non-ASCII Request header values before attaching them as a NetLog string.
This is a quick fix for a libfuzzer crash.
BUG=633069
Committed: https://crrev.com/396ba79a4314fd0be39f78e1e2ca467951a37cf4
Cr-Commit-Position: refs/heads/master@{#426038}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Address Matt's feedback #Messages
Total messages: 14 (7 generated)
eroman@chromium.org changed reviewers: + mmenke@chromium.org
LGTM, regardless of what you think of my comment. https://codereview.chromium.org/2428103002/diff/1/net/http/http_request_heade... File net/http/http_request_headers.cc (right): https://codereview.chromium.org/2428103002/diff/1/net/http/http_request_heade... net/http/http_request_headers.cc:198: std::string escaped_value = EscapeNonASCII(log_value); Would it make sense to use EscapeNonASCII on the output of base::StringPrintf instead? I'm fine either way, but not sure what sort of sanitizing we do on header names set by XHRs.
The CQ bit was checked by eroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2428103002/diff/1/net/http/http_request_heade... File net/http/http_request_headers.cc (right): https://codereview.chromium.org/2428103002/diff/1/net/http/http_request_heade... net/http/http_request_headers.cc:198: std::string escaped_value = EscapeNonASCII(log_value); On 2016/10/18 19:12:33, mmenke wrote: > Would it make sense to use EscapeNonASCII on the output of base::StringPrintf > instead? I'm fine either way, but not sure what sort of sanitizing we do on > header names set by XHRs. I was aiming for consistency with how HttpResponseHeaders does it: https://cs.chromium.org/chromium/src/net/http/http_response_headers.cc?rcl=0&... I went ahead and escaped the header name in the same manner for consistency, but I want to circle back on this, since it doesn't feel right. * I don't think our code should be generating/allowing for non-ASCII values in request headers. Certainly the Digest case doesn't feel right. Whether our XHR code from blink allows this is certainly something worth looking into. * I would like a more uniform approach to how we handle this in the netlogs, but will propose such a refactor separately so as not to conflate issues.
The CQ bit was unchecked by eroman@chromium.org
The CQ bit was checked by eroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2428103002/#ps20001 (title: "Address Matt's feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2428103002/diff/1/net/http/http_request_heade... File net/http/http_request_headers.cc (right): https://codereview.chromium.org/2428103002/diff/1/net/http/http_request_heade... net/http/http_request_headers.cc:198: std::string escaped_value = EscapeNonASCII(log_value); On 2016/10/18 19:21:52, eroman wrote: > On 2016/10/18 19:12:33, mmenke wrote: > > Would it make sense to use EscapeNonASCII on the output of base::StringPrintf > > instead? I'm fine either way, but not sure what sort of sanitizing we do on > > header names set by XHRs. > > I was aiming for consistency with how HttpResponseHeaders does it: > > https://cs.chromium.org/chromium/src/net/http/http_response_headers.cc?rcl=0&... > > I went ahead and escaped the header name in the same manner for consistency, but > I want to circle back on this, since it doesn't feel right. > > * I don't think our code should be generating/allowing for non-ASCII values in > request headers. Certainly the Digest case doesn't feel right. Whether our XHR > code from blink allows this is certainly something worth looking into. > > * I would like a more uniform approach to how we handle this in the netlogs, > but will propose such a refactor separately so as not to conflate issues. This all sounds reasonable to me.
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Escape non-ASCII Request header values before attaching them as a NetLog string. This is a quick fix for a libfuzzer crash. BUG=633069 ========== to ========== Escape non-ASCII Request header values before attaching them as a NetLog string. This is a quick fix for a libfuzzer crash. BUG=633069 Committed: https://crrev.com/396ba79a4314fd0be39f78e1e2ca467951a37cf4 Cr-Commit-Position: refs/heads/master@{#426038} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/396ba79a4314fd0be39f78e1e2ca467951a37cf4 Cr-Commit-Position: refs/heads/master@{#426038} |
