Issue 29377775: Issue 4916 - Request compression in CURL WebRequest. (Closed)
|
Created:
March 1, 2017, 10:44 p.m. by hub Modified:
March 16, 2017, 5:10 p.m. Base URL:
https://hg.adblockplus.org/libadblockplus/ Visibility:
Public. |
DescriptionIssue 4916 - Request compression in CURL WebRequest.
Patch Set 1 #
Total comments: 1
Patch Set 2 : Issue 4916 - Request compression in CURL WebRequest. (updated test) #
Total comments: 6
Patch Set 3 : Updated patch to fix test. #
MessagesTotal messages: 8
https://codereview.adblockplus.org/29377775/diff/29377776/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377775/diff/29377776/test/WebRequest.cpp... test/WebRequest.cpp:93: ASSERT_EQ("gzip", jsEngine->Evaluate("foo.responseHeaders['content-encoding'].substr(0, 10)")->AsString()); I think instead of 10 there should be 5.
On 2017/03/02 08:47:30, sergei wrote: > https://codereview.adblockplus.org/29377775/diff/29377776/test/WebRequest.cpp > File test/WebRequest.cpp (right): > > https://codereview.adblockplus.org/29377775/diff/29377776/test/WebRequest.cpp... > test/WebRequest.cpp:93: ASSERT_EQ("gzip", > jsEngine->Evaluate("foo.responseHeaders['content-encoding'].substr(0, > 10)")->AsString()); > I think instead of 10 there should be 5. Update with a new patchset changing this.
LGTM
https://codereview.adblockplus.org/29377775/diff/29377796/src/DefaultWebReque... File src/DefaultWebRequestCurl.cpp (right): https://codereview.adblockplus.org/29377775/diff/29377796/src/DefaultWebReque... src/DefaultWebRequestCurl.cpp:142: curl_easy_setopt(curl, CURLOPT_ACCEPT_ENCODING, ""); Won't it be possible to overwrite this by supplying the `Accept-Encoding` header in `requestHeaders`? It shouldn't be possible for XHR users to overwrite this, it's user agent specific after all. In issue 4916, I suggested to blacklist the headers not allowed in XHR's `setRequestHeader` function according to the spec, see: https://codereview.adblockplus.org/29377775/diff/29377796/test/WebRequest.cpp... I figure the easiest way is to reject those in lib/compat.js. I figure clients can still overwrite it if they really want to, but it'll be pretty obvious that this shouldn't be done. I'm not sure if that should be tackled in the same issue however, feel free to create a new one for that. I'm not all that worried about it. https://codereview.adblockplus.org/29377775/diff/29377796/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377775/diff/29377796/test/WebRequest.cpp... test/WebRequest.cpp:93: ASSERT_EQ("gzip", jsEngine->Evaluate("foo.responseHeaders['content-encoding'].substr(0, 5)")->AsString()); Nit: Given how "gzip" is a four character string, wouldn't this be `substr(0, 4)` then? I might be missing something though.
https://codereview.adblockplus.org/29377775/diff/29377796/src/DefaultWebReque... File src/DefaultWebRequestCurl.cpp (right): https://codereview.adblockplus.org/29377775/diff/29377796/src/DefaultWebReque... src/DefaultWebRequestCurl.cpp:142: curl_easy_setopt(curl, CURLOPT_ACCEPT_ENCODING, ""); On 2017/03/02 14:33:38, Felix Dahlke wrote: > Won't it be possible to overwrite this by supplying the `Accept-Encoding` header > in `requestHeaders`? It shouldn't be possible for XHR users to overwrite this, > it's user agent specific after all. > > In issue 4916, I suggested to blacklist the headers not allowed in XHR's > `setRequestHeader` function according to the spec, see: > https://codereview.adblockplus.org/29377775/diff/29377796/test/WebRequest.cpp... > > I figure the easiest way is to reject those in lib/compat.js. I figure clients > can still overwrite it if they really want to, but it'll be pretty obvious that > this shouldn't be done. > > I'm not sure if that should be tackled in the same issue however, feel free to > create a new one for that. I'm not all that worried about it. It is my understanding that passing "" will set the only valid values and make CURL do the proper decoding of the response: https://github.com/curl/curl/blob/a08db496429e09561c8b8d243e79b21ef04f98f2/do... An override will cause: 1. no change if the encoding is supported by CURL 2. an error, it the encoding is not supported by CURL Simply setting the Accept-Encoding header doesn't work because then we need to handle the decoding out of CURL. I think blacklisting is a different issue, because with or without this change it should be done. https://codereview.adblockplus.org/29377775/diff/29377796/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377775/diff/29377796/test/WebRequest.cpp... test/WebRequest.cpp:93: ASSERT_EQ("gzip", jsEngine->Evaluate("foo.responseHeaders['content-encoding'].substr(0, 5)")->AsString()); On 2017/03/02 14:33:39, Felix Dahlke wrote: > Nit: Given how "gzip" is a four character string, wouldn't this be `substr(0, > 4)` then? I might be missing something though. Acknowledged.
LGTM https://codereview.adblockplus.org/29377775/diff/29377796/src/DefaultWebReque... File src/DefaultWebRequestCurl.cpp (right): https://codereview.adblockplus.org/29377775/diff/29377796/src/DefaultWebReque... src/DefaultWebRequestCurl.cpp:142: curl_easy_setopt(curl, CURLOPT_ACCEPT_ENCODING, ""); On 2017/03/02 15:36:00, hub wrote: > On 2017/03/02 14:33:38, Felix Dahlke wrote: > > Won't it be possible to overwrite this by supplying the `Accept-Encoding` > header > > in `requestHeaders`? It shouldn't be possible for XHR users to overwrite this, > > it's user agent specific after all. > > > > In issue 4916, I suggested to blacklist the headers not allowed in XHR's > > `setRequestHeader` function according to the spec, see: > > > https://codereview.adblockplus.org/29377775/diff/29377796/test/WebRequest.cpp... > > > > I figure the easiest way is to reject those in lib/compat.js. I figure clients > > can still overwrite it if they really want to, but it'll be pretty obvious > that > > this shouldn't be done. > > > > I'm not sure if that should be tackled in the same issue however, feel free to > > create a new one for that. I'm not all that worried about it. > > It is my understanding that passing "" will set the only valid values and make > CURL do the proper decoding of the response: > https://github.com/curl/curl/blob/a08db496429e09561c8b8d243e79b21ef04f98f2/do... > > An override will cause: > 1. no change if the encoding is supported by CURL > 2. an error, it the encoding is not supported by CURL > > Simply setting the Accept-Encoding header doesn't work because then we need to > handle the decoding out of CURL. > > I think blacklisting is a different issue, because with or without this change > it should be done. Fair enough.
LGTM https://codereview.adblockplus.org/29377775/diff/29377796/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377775/diff/29377796/test/WebRequest.cpp... test/WebRequest.cpp:93: ASSERT_EQ("gzip", jsEngine->Evaluate("foo.responseHeaders['content-encoding'].substr(0, 5)")->AsString()); On 2017/03/02 14:33:39, Felix Dahlke wrote: > Nit: Given how "gzip" is a four character string, wouldn't this be `substr(0, > 4)` then? I might be missing something though. Sorry, my bad, off by one error. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
