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

fix(CURLRequest): multiple header sections after redirects #9426

Conversation

ducng99
Copy link
Contributor

@ducng99 ducng99 commented Jan 20, 2025

Description
Fixes an issue where multiple header sections are returned if there are redirects. The original code would only parse the first header section, and treated the rest as body. For example:

With output:

HTTP/2 308
content-type: text/html; charset=utf-8
content-length: 211
location: http://example.com
date: Mon, 20 Jan 2025 11:46:34 GMT
server: nginx/1.21.6
vary: Origin

HTTP/2 200
content-type: text/html; charset=utf-8
content-length: 211
date: Mon, 20 Jan 2025 11:46:34 GMT
server: nginx/1.21.6
vary: Origin

Hello world

$response->getBody() returns:

HTTP/2 200
content-type: text/html; charset=utf-8
content-length: 211
date: Mon, 20 Jan 2025 11:46:34 GMT
server: nginx/1.21.6
vary: Origin

Hello world

With these changes, the redirect header section is stripped out, only the latest header section is parsed (the 200 one in this example), and body is Hello world.

Not sure if this issue has been discussed before but I couldn't find an issue related to this.

My machine details:
Ubuntu 24.04
PHP 8.2.27
libcurl 7.88.1

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a changelog entry here: https://github.com/codeigniter4/CodeIgniter4/blob/develop/user_guide_src/source/changelogs/v4.6.1.rst under the "Bugs Fixed" section. Please follow the style from the previous changelogs.

system/HTTP/CURLRequest.php Outdated Show resolved Hide resolved
@paulbalandan paulbalandan added the bug Verified issues on the current code behavior or pull requests that will fix them label Jan 20, 2025
@ducng99 ducng99 force-pushed the fix/curl_multiple_header_sections_after_redirects branch from ae7688f to 72c5158 Compare January 20, 2025 20:49
@ducng99 ducng99 marked this pull request as draft January 20, 2025 20:54
@ducng99
Copy link
Contributor Author

ducng99 commented Jan 20, 2025

Change to draft as I need to add checks for Location: header as well.
In cases where a 3xx code is returned but no Location header so curl won't redirect but we strip out the only header section.

@ducng99 ducng99 force-pushed the fix/curl_multiple_header_sections_after_redirects branch from 6e2f2a6 to 4722795 Compare January 20, 2025 21:19
@ducng99 ducng99 marked this pull request as ready for review January 20, 2025 21:39
@paulbalandan paulbalandan requested a review from michalsn January 21, 2025 06:34
Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you.

@samsonasik
Copy link
Member

please check phpstan and rector notice

@ducng99
Copy link
Contributor Author

ducng99 commented Jan 21, 2025

@samsonasik Can you double check those notices please? I think I'm missing something here, they are about other files that have nothing to do with my changes. Thanks

Also composer analyze returns OK on my local machine

@neznaika0
Copy link
Contributor

@ducng99 ducng99 force-pushed the fix/curl_multiple_header_sections_after_redirects branch from 5defc18 to cc521d9 Compare January 22, 2025 21:30
@ducng99
Copy link
Contributor Author

ducng99 commented Jan 22, 2025

@neznaika0 @paulbalandan
An update to phpstan is causing errors in development branch, as the errors are not part of this PR, can you check them please?

As for the rector notice, the file is also not part of this PR, the reported line system/HTTP/Exceptions/RedirectException.php:75 had a pass before, see Rector check with changes for 190f720
May be rector was also updated?

In any case, these two files are not part of this PR so I can't push fixes for them here

@paulbalandan
Copy link
Member

Hi @ducng99 I see that the failing tests are not related to your changed files, so you can ignore that.

@paulbalandan paulbalandan merged commit 909e9b0 into codeigniter4:develop Jan 23, 2025
94 of 98 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants