-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[6.1] - Fix JHTTP socket transport http version #43130
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
base: 6.1-dev
Are you sure you want to change the base?
Conversation
|
@wilsonge the PR solves the issue of unterminated connections caused by the missing header, however I don’t see how it fixes the missing support for the chunked transfer encoding? |
|
Can you explain what you think is broken with the chunked encoding? As far as I can see after this PR I get a fully normal json response. there's a bug in the redirect handling too - but trying to do one thing at a time - but I can get responses to longer documents like joomla.org too |
|
@wilsonge I've created a demo file that's always going to output a chunked response. Now, call that file with a HTTP client of choice (i.e. Postman) and the socket driver. Expected result: Actual result: |
|
I'm struggling to test this but found an old docs page on an old PHP Manual archive for the PECL HTTP Package with some sample polyfill code and reworked that into something that I think works. I still am struggling to reproduce - but found this https://jigsaw.w3.org/HTTP/ChunkedScript and I think this makes it work? I'm not sure if I'm stripping line breaks improperly or not after. But it definitely removes the extra characters. |
| while (($pos < $len) | ||
| && ($chunkLenHex = substr($chunk, $pos, ($newlineAt = strpos($chunk, "\n", $pos + 1)) - $pos))) { | ||
| if (!static::is_hex(rtrim($chunkLenHex))) { | ||
| trigger_error('Value is not properly chunk encoded', E_USER_WARNING); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that should be an exception, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't sure. When it's hard to find examples of when this might fail - hard to know if it can ever not be a hex in genuine page output.
|
This pull request has been automatically rebased to 5.2-dev. |
|
This pull request has been automatically rebased to 5.3-dev. |
|
This pull request has been automatically rebased to 6.0-dev. |
|
This pull request has been automatically rebased to 6.1-dev. |
Pull Request for Issue #38963 and #42973 and an alternative fix to #43002
Summary of Changes
In #35568 a change was merged into the JHTTP socket driver, increasing the accepted HTTP version for the client from 1.0 to 1.1.
That change introduce the issue described in #38963: HTTP 1.1 defines the chunked transfer mode which is mandatory for all clients implementing HTTP 1.1 - as our socket-based client however does not support chunked responses, a chunked response causes an infinite loop.
The fix (as used in other similar http libraries such as guzzle is to simply ensure the connection is closed at the end of the request). This is now implemented. Additionally we now default the stream library to http 1.1 as well - as this currently would use HTTP 1.0 in < PHP 8 and HTTP 1.1 in PHP >= 8.0 and I believe would hit similar issues.
I've already fixed this in the framework too but it needs someone there to do a release - see joomla-framework/http@3.0.1...3.x-dev
Testing Instructions
$http = \Joomla\CMS\Http\HttpFactory::getHttp([], 'socket'); $response = $http->get('https://update.joomla.org/cms/root.json');Actual result BEFORE applying this Pull Request
Expected result AFTER applying this Pull Request
Link to documentations
Please select:
Documentation link for docs.joomla.org:
No documentation changes for docs.joomla.org needed
Pull Request link for manual.joomla.org:
No documentation changes for manual.joomla.org needed