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

[5.3] - Fix JHTTP socket transport http version #43130

Open
wants to merge 13 commits into
base: 5.3-dev
Choose a base branch
from

Conversation

wilsonge
Copy link
Contributor

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

  • Create a socket-based JHTTP request to a server responding with a chunked response, i.e. by using this code block:
    $http = \Joomla\CMS\Http\HttpFactory::getHttp([], 'socket'); $response = $http->get('https://update.joomla.org/cms/root.json');

Actual result BEFORE applying this Pull Request

  • HTTP 1.0 request but working.

Expected result AFTER applying this Pull Request

  • Response returned

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

@SniperSister
Copy link
Contributor

@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?

@wilsonge
Copy link
Contributor Author

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

@SniperSister
Copy link
Contributor

@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:

This is Chunk #0.This is Chunk #1.This is Chunk #2.This is Chunk #3.This is Chunk #4.This is Chunk #5.This is Chunk #6.This is Chunk #7.This is Chunk #8.This is Chunk #9.

Actual result:

11
This is Chunk #0.
11
This is Chunk #1.
11
This is Chunk #2.
11
This is Chunk #3.
11
This is Chunk #4.
11
This is Chunk #5.
11
This is Chunk #6.
11
This is Chunk #7.
11
This is Chunk #8.
11
This is Chunk #9.
0

@wilsonge
Copy link
Contributor Author

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);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@HLeithner
Copy link
Member

This pull request has been automatically rebased to 5.2-dev.

@HLeithner HLeithner changed the title [5.1] - Fix JHTTP socket transport http version [5.2] - Fix JHTTP socket transport http version Apr 24, 2024
@HLeithner HLeithner changed the base branch from 5.2-dev to 5.3-dev September 2, 2024 08:51
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 5.3-dev.

@HLeithner HLeithner changed the title [5.2] - Fix JHTTP socket transport http version [5.3] - Fix JHTTP socket transport http version Sep 2, 2024
@Hackwar Hackwar removed the PR-5.2-dev label Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants