Fix: remove CURLRequest headers sharing from $_SERVER #5249

kenjis · 2021-10-28T04:49:32Z

Description
Alternative to #5218 (Inclueds #5218)
Fixes #4826

populateHeaders() which gets request headers from the browser and sets the same headers to the curl request and shares the headers between curl requests, is a bug. So remove populateHeaders() logic completely
add Config\CURLRequest::$shareOptions. The default is true
- if false,
  - all options are reset after sending a request
  - $defaultOptions which are passed in the constructor are applied to all requests
~~share $option['headers'] except $unsharedHeaders between curl requests~~
remove CURLRequestTest::testOptionsHeadersUsingPopulate()

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

Fixes codeigniter4#4826

If you reset it at the beginning of the request(), you will not be able to use setForm().

…er between CURL requests

MGatner · 2021-10-28T13:47:41Z

Initial review looks good but I will take a deeper look on desktop.

michalsn

I think that removing populateHeaders() call when the headers array is empty will solve most of the problems that users had. We will need an entry in the changelog for this - if it gonna be merged.

The real question is how big a problem for devs these changes will be... My point is that missing some headers that were populated before may cause some requests to not work. Personally, I have always specified my own headers, but it looks like a lot of people have not.

system/HTTP/CURLRequest.php

To override the values.

If user setHeader() that is defined in $unsharedHeaders, the header will be removed before sending request, and there is no way to send the header. The reason I didn't do the reset after sending is because a test failed. This time I modify the test, too.

kenjis · 2021-10-29T02:48:29Z

The real question is how big a problem for devs these changes will be... My point is that missing some headers that were populated before may cause some requests to not work. Personally, I have always specified my own headers, but it looks like a lot of people have not.

I guess it is very small. But I have no data.

kenjis · 2021-10-29T02:49:09Z

Added one fix and documentation.

user_guide_src/source/changelogs/v4.1.5.rst

user_guide_src/source/installation/upgrade_415.rst

system/HTTP/CURLRequest.php

$defaultOptions is options passed in the constructor. They are applied for all requests. All request headers are cleared after sending request, and $defaultOptions are set again.

kenjis · 2021-10-29T07:17:11Z

I removed $unsharedHeaders and added $defaultOptions.

$defaultOptions is the options passed in the constructor.
They are applied to all requests.

In other words, there are two kind of options. The default options
and the other options.

After sending a request, all headers are cleared. And $defaultOptions
are parsed again.

I wanted to reset all $config, too. But I'm not sure I can do it, so only
$this->config['multipart'] and $this->config['form_params'] are unset.

The explanation was not accurate. Co-authored-by: Michal Sniatala <[email protected]>

If $CURLRequestShareOptions is false, reset all config after sending a request. If true, keep the all config (the same as before).

kenjis · 2021-10-29T08:56:53Z

Added Config\App::$CURLRequestShareOptions. The default is true.
Only when set to false, all config are reset after sending a request,
and set the defaultOptions.

app/Config/App.php

WinterSilence · 2021-10-29T16:22:44Z

let's use other namespace to keep BC
at first need create class skeleton (architecture)
2.1. extract curl options in "config" class

michalsn

Nice work @kenjis, I think it looks really good.

The only important thing that is missing is the separate config file and some documentation for this new feature (shareOptions).

app/Config/App.php

system/HTTP/CURLRequest.php

michalsn · 2021-10-29T19:23:34Z

let's use other namespace to keep BC

at first need create class skeletons (architecture)
2.1. extract curl options in "config" class

@WinterSilence I'm not sure if I'm understanding you correctly, but if you're proposing to add a new implementation of this class because of BC, that's not going to happen. The possible BC break is accidental and is a derivative from a bug that is now fixed.

Technically speaking we are dealing with a bug here. However, we are aware that fixing it may lead to problems in some cases. Therefore, we raise this issue in the changelog and upgrade notes.

…uest::$shareOptions

kenjis · 2021-10-30T02:24:12Z

@WinterSilence I'm also not get what you want to say.

WinterSilence · 2021-10-30T08:12:48Z

@kenjis I want minimal changes in "behavior" of current classes i.e. save public part "as is". Next release is not new major version - BC not allowed. But some errors can't be solved without BC, so I suggest a temporary solution - similar classes with fixes - users will choose version to use. Old/deprecated classes will be removed in next major version.

michalsn

From my point of view, this is ready. Thanks!

kenjis · 2021-11-01T00:38:45Z

@WinterSilence I thought that old/new two classes.
If we have two classes, we have to maintain two classes.

Adding config for a new behavior is a bit better than maintaining two classes, I thought.
And users can use the new behavior, just set the config.

kenjis added 5 commits October 25, 2021 20:51

fix: reset unneeded headers to prevent request error

d1edaee

Fixes codeigniter4#4826

fix: reset unneeded configs

245dade

If you reset it at the beginning of the request(), you will not be able to use setForm().

test: add test for CURLRequest unshared headers

fa3620b

test: fix test method names

4b51e01

fix: remove populateHeaders() not to share request headers form brows…

e307ef5

…er between CURL requests

kenjis added the breaking change Pull requests that may break existing functionalities label Oct 28, 2021

kenjis changed the title ~~Fix: remove CURLRequest headers sharing~~ Fix: remove CURLRequest headers sharing from $_SERVER Oct 28, 2021

This was referenced Oct 28, 2021

Fix CURLRequest extra headers #5218

Merged

Bug: Extra Headers on CURLRequest #4826

Closed

kenjis requested review from MGatner and michalsn October 28, 2021 05:06

michalsn reviewed Oct 28, 2021

View reviewed changes

system/HTTP/CURLRequest.php Outdated Show resolved Hide resolved

system/HTTP/CURLRequest.php Outdated Show resolved Hide resolved

kenjis added 4 commits October 29, 2021 10:26

fix: make $unsharedHeaders protected

9cbe0f2

To override the values.

docs: improve changelogs/v4.1.5.rst format

d39543e

docs: add upgrading and changelog

9a6a0d3

michalsn reviewed Oct 29, 2021

View reviewed changes

user_guide_src/source/changelogs/v4.1.5.rst Outdated Show resolved Hide resolved

user_guide_src/source/installation/upgrade_415.rst Outdated Show resolved Hide resolved

system/HTTP/CURLRequest.php Outdated Show resolved Hide resolved

fix: remove $unsharedHeaders and add $defaultOptions

613b980

$defaultOptions is options passed in the constructor. They are applied for all requests. All request headers are cleared after sending request, and $defaultOptions are set again.

kenjis and others added 3 commits October 29, 2021 16:22

docs: fix changelogs/v4.1.5.rst

8b08642

The explanation was not accurate. Co-authored-by: Michal Sniatala <[email protected]>

docs: fix upgrade_415.rst

182c252

The explanation was not accurate. Co-authored-by: Michal Sniatala <[email protected]>

fix: add config $CURLRequestShareOptions for the CURLRequest

aed89d9

If $CURLRequestShareOptions is false, reset all config after sending a request. If true, keep the all config (the same as before).

kenjis added 2 commits October 29, 2021 18:42

test: add tests for $CURLRequestShareOptions = false

436013e

test: add tests for sharing options

9e88ec4

WinterSilence reviewed Oct 29, 2021

View reviewed changes

app/Config/App.php Outdated Show resolved Hide resolved

michalsn reviewed Oct 29, 2021

View reviewed changes

app/Config/App.php Outdated Show resolved Hide resolved

system/HTTP/CURLRequest.php Outdated Show resolved Hide resolved

system/HTTP/CURLRequest.php Outdated Show resolved Hide resolved

system/HTTP/CURLRequest.php Outdated Show resolved Hide resolved

kenjis added 6 commits October 30, 2021 08:58

refactor: remove unnecessary second param

a6c06cc

refactor: move Config\App::$CURLRequestShareOptions to Config\CURLReq…

f53a1da

…uest::$shareOptions

fix: change from private to protected

772a408

docs: add PHPDoc comments

3901496

config: add curlrequest.shareOptions

8a5973b

docs: add about CURLRequest $shareOptions

88963ba

michalsn approved these changes Oct 30, 2021

View reviewed changes

MGatner approved these changes Nov 1, 2021

View reviewed changes

kenjis merged commit dbbdd2d into codeigniter4:develop Nov 1, 2021

kenjis deleted the remove-curl-headers-sharing branch November 1, 2021 00:38

kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Feb 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: remove CURLRequest headers sharing from $_SERVER #5249

Fix: remove CURLRequest headers sharing from $_SERVER #5249

kenjis commented Oct 28, 2021 •

edited

Loading

MGatner commented Oct 28, 2021

michalsn left a comment

kenjis commented Oct 29, 2021

kenjis commented Oct 29, 2021

kenjis commented Oct 29, 2021

kenjis commented Oct 29, 2021

WinterSilence commented Oct 29, 2021 •

edited

Loading

michalsn left a comment

michalsn commented Oct 29, 2021

kenjis commented Oct 30, 2021

WinterSilence commented Oct 30, 2021

michalsn left a comment

kenjis commented Nov 1, 2021

Fix: remove CURLRequest headers sharing from $_SERVER #5249

Fix: remove CURLRequest headers sharing from $_SERVER #5249

Conversation

kenjis commented Oct 28, 2021 • edited Loading

MGatner commented Oct 28, 2021

michalsn left a comment

Choose a reason for hiding this comment

kenjis commented Oct 29, 2021

kenjis commented Oct 29, 2021

kenjis commented Oct 29, 2021

kenjis commented Oct 29, 2021

WinterSilence commented Oct 29, 2021 • edited Loading

michalsn left a comment

Choose a reason for hiding this comment

michalsn commented Oct 29, 2021

kenjis commented Oct 30, 2021

WinterSilence commented Oct 30, 2021

michalsn left a comment

Choose a reason for hiding this comment

kenjis commented Nov 1, 2021

kenjis commented Oct 28, 2021 •

edited

Loading

WinterSilence commented Oct 29, 2021 •

edited

Loading