Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

always send an "Accept-Encoding" header with HTTP requests #17009

Merged
merged 4 commits into from Jan 1, 2021

Conversation

Findus23
Copy link
Member

This fixes a fun issue reported in https://forum.matomo.org/t/error-array-offset-on-bool-in-securityinfo-matomo-4-1-0/39986

I originally blamed safe_unserialize to be broken, but it turned out that even a Http::sendHttpRequest('https://php.net/releases/?json=1&version=7', $timeout); was only returning garbled output.

It turns out that whatever server the PHP team uses, assumes that when you use Firefox, you are always able to uncompress gzip-encoded content, even if it was not requested. (and Matomo forwards the User Agent of the user for its HTTP requests).

~ curl 'https://www.php.net/releases/?json=1&version=7' 
{"announcement":true,"tags":[],"date":"26 Nov 2020","source":[{"filename":"php-7.4.13.tar.gz","name":"PHP 7.4.13 (tar.gz)","sha256":"0865cff41e7210de2537bcd5750377cfe09a9312b9b44c1a166cf372d5204b8f","date":"26 Nov 2020"},{"filename":"php-7.4.13.tar.bz2","name":"PHP 7.4.13 (tar.bz2)","sha256":"15a339857e11c92eb47fddcd0dfe8aaa951a9be7c57ab7230ccd497465a31fda","date":"26 Nov 2020"},{"filename":"php-7.4.13.tar.xz","name":"PHP 7.4.13 (tar.xz)","sha256":"aead303e3abac23106529560547baebbedba0bb2943b91d5aa08fff1f41680f4","date":"26 Nov 2020"}],"version":"7.4.13"}%
➜  ~ curl 'https://www.php.net/releases/?json=1&version=7' -H 'User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:84.0) Gecko/20100101 Firefox/84.0'                                                          
Warning: Binary output can mess up your terminal. Use "--output -" to tell 
Warning: curl to output it to your terminal anyway, or consider "--output 
Warning: <FILE>" to save to a file.
➜  ~ curl 'https://www.php.net/releases/?json=1&version=7' -H 'User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:84.0) Gecko/20100101 Firefox/84.0' -H "Accept-Encoding: idendity"                           
{"announcement":true,"tags":[],"date":"26 Nov 2020","source":[{"filename":"php-7.4.13.tar.gz","name":"PHP 7.4.13 (tar.gz)","sha256":"0865cff41e7210de2537bcd5750377cfe09a9312b9b44c1a166cf372d5204b8f","date":"26 Nov 2020"},{"filename":"php-7.4.13.tar.bz2","name":"PHP 7.4.13 (tar.bz2)","sha256":"15a339857e11c92eb47fddcd0dfe8aaa951a9be7c57ab7230ccd497465a31fda","date":"26 Nov 2020"},{"filename":"php-7.4.13.tar.xz","name":"PHP 7.4.13 (tar.xz)","sha256":"aead303e3abac23106529560547baebbedba0bb2943b91d5aa08fff1f41680f4","date":"26 Nov 2020"}],"version":"7.4.13"}% 

So PHP curl recieved a gziped response even though it didn't request it and therefore didn't automatically unpack it. Adding CURLOPT_ENCODING => "" tells curl to always request all encodings it supports and therefore be able to handle any gzipped response.

CURLOPT_ENCODING The contents of the "Accept-Encoding: " header. This enables decoding of the response. Supported encodings are "identity", "deflate", and "gzip". If an empty string, "", is set, a header containing all supported encoding types is sent. Added in cURL 7.10.

https://www.php.net/manual/en/function.curl-setopt.php

Review

  • Functional review done
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@diosmosis
Copy link
Member

@Findus23 for some reason, this setting is interfering w/ CURLOPT_RANGE causing at least one test to fail. Looked a bit for a reason but couldn't find any. Worst case I guess we can avoid setting this header if we also have to set CURLOPT_RANGE.

@diosmosis
Copy link
Member

@Findus23 i couldn't quickly find a reason as to why it didn't work w/ CURLOPT_RANGE, so I modified the PR to only apply the option if not also applying CURLOPT_RANGE. If you ever find the reason, feel free to make another PR.

@diosmosis diosmosis merged commit bd58cc8 into 4.x-dev Jan 1, 2021
@diosmosis diosmosis deleted the accept-encoding-http branch January 1, 2021 00:13
@Findus23
Copy link
Member Author

Findus23 commented Jan 1, 2021

Sorry, I forgot to respond. This is fine by me. I'm not even sure if there is any code in current Matomo that even does range http requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants