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 extra headers #5218

Merged
merged 4 commits into from
Nov 1, 2021

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Oct 19, 2021

Description
Fixes #4826

  • reset unneeded headers
  • reset unneeded configs

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

@MGatner
Copy link
Member

MGatner commented Oct 20, 2021

The code looks good but I would like it if someone else with more CURL experience took a look.

@WinterSilence
Copy link
Contributor

WinterSilence commented Oct 25, 2021

  1. I'm already wrote about direct access to global variables Dev: using filter_* functions instead direct access to global variables #4968
  2. Use getallheaders() polyfill to get headers by server request
  3. Add setter/getter for $unsharedHeaders
  4. Why Host, Accept-Encoding not in $unsharedHeaders?
    $this->removeHeader('Host');
  5. Why Referer, Location, Content-MD5 and other dynamic/generic headers not in $unsharedHeader?

P.S. In my opinion, better solution auto calculate dynamic/generic headers before sending request. List of headers to import better than $unsharedHeaders because we need import only small part of request headers.

$header = str_replace('_', ' ', strtolower($header));
$header = str_replace(' ', '-', ucwords($header));

if (in_array($header, $this->unsharedHeaders, true)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

$header = str_replace('_', '-', strtolower($header));
$titleHeader = ucwords($header, '-');
...
$this->headerMap[$header] = $titleHeader;

@kenjis
Copy link
Member Author

kenjis commented Oct 25, 2021

@WinterSilence Thank you for your comment!

Use getallheaders() polyfill to get headers by server request

Why? What is it? CI4 does not have it now.

Add setter/getter for $unsharedHeaders

Why do you need them?

Why Host, Accept-Encoding not in $unsharedHeaders?
Why Referer, Location, Content-MD5 and other dynamic/generic headers not in $unsharedHeader?

It is just because they are not considered yet. This PR only fixes the issue #4826.
I'm not sure what headers should be unshared.
I don't understand the current class design well.

P.S. In my opinion, better solution auto calculate dynamic/generic headers before sending request. List of headers to import better than $unsharedHeaders because we need import only small part of request headers.

Yeah, it is good design. But the current implementation is the opposite.
And once released, we have to keep backward compatibility as much as possible.

After all, I don't understand this class well enough to be able to create a list of headers to share.

@WinterSilence
Copy link
Contributor

WinterSilence commented Oct 25, 2021

@kenjis you can add my polyfill or ralouphie/getallheaders

Why do you need them?

because you can't auto detect all dynamic/generic headers to remove

@kenjis kenjis force-pushed the fix-curl-headers-sharing-bug branch from 6832cd5 to 4b51e01 Compare October 25, 2021 11:51
@kenjis kenjis marked this pull request as draft October 25, 2021 12:09
@kenjis
Copy link
Member Author

kenjis commented Oct 25, 2021

Why Host, Accept-Encoding not in $unsharedHeaders?

If they are in $unsharedHeaders, we can't set/use them.

$unsharedHeaders are removed in the top of request().
https://github.com/codeigniter4/CodeIgniter4/pull/5218/files#diff-c1b09d9175e885241fedeb4f406e7c19e3a78397538be28b70492647ae0c4bbaR121

@kenjis
Copy link
Member Author

kenjis commented Oct 26, 2021

@WinterSilence

List of headers to import better than $unsharedHeaders because we need import only small part of request headers.

Do you have the header list that you want to import?
To be honest, I don't really understand why we need to import.

@WinterSilence
Copy link
Contributor

WinterSilence commented Oct 26, 2021

@kenjis nope, it's my ugly English... You want use "allow all, deny $unsharedHeaders" list, I vote to use "deny all, allow $sharedHeaders" list. We need save only "cookie" and "auth" headers, other headers should be specified by the user himself.

@kenjis
Copy link
Member Author

kenjis commented Oct 28, 2021

I sent another PR: #5249
It is cleaner.

@kenjis
Copy link
Member Author

kenjis commented Oct 29, 2021

@WinterSilence

Content-MD5 is removed in RFC 7231 on June 2014.

The Content-MD5 header field has been removed because it was
inconsistently implemented with respect to partial responses.
https://www.rfc-editor.org/rfc/rfc7231

Is there any use case?

@WinterSilence
Copy link
Contributor

WinterSilence commented Oct 29, 2021

@kenjis Sorry, I miss link to my polyfill. Polyfill must be similar to original getallheaders().

@kenjis kenjis merged commit 4b51e01 into codeigniter4:develop Nov 1, 2021
@kenjis kenjis deleted the fix-curl-headers-sharing-bug branch November 1, 2021 00:44
@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Feb 19, 2022
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.

Bug: Extra Headers on CURLRequest
3 participants