Update Curl to respect case-insensitive headers #29274

pmzandbergen · 2020-07-26T08:31:32Z

According to RFC 2616 header names are case-insensitive: "Each header field consists of a name followed by a colon (":") and the field value. Field names are case-insensitive." - see https://tools.ietf.org/html/rfc2616#section-4.2

The "Set-Cookie" comparison in the current Curl client is case-sensitive and can cause problems with some (rare) HTTP servers.

Description (*)

According to RFC 2616 header names are case-insensitive: "Each header field consists of a name followed by a colon (":") and the field value. Field names are case-insensitive." - see https://tools.ietf.org/html/rfc2616#section-4.2

The "Set-Cookie" comparison in the previous version is case-sensitive and can cause problems with some (rare) HTTP servers.

Related Pull Requests

None

Fixed Issues (if relevant)

Did not create an issue

Manual testing scenarios (*)

Simulate a HTTP server to return a "set-cookie" or "SET-COOKIE" HTTP header
Test the original client if it respects those header names (it does not)
Test the new version (it does)

Questions or comments

Please note that I did not provide a unit test for this specific issue. If required please let me know.

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds are green)

Resolved issues:

resolves [Issue] Update Curl to respect case-insensitive headers #29524: Update Curl to respect case-insensitive headers

According to RFC 2616 header names are case-insensitive: "Each header field consists of a name followed by a colon (":") and the field value. Field names are case-insensitive." - see https://tools.ietf.org/html/rfc2616#section-4.2 The "Set-Cookie" comparison in the previous version is case-sensitive and can cause problems with some (rare) HTTP servers.

m2-assistant · 2020-07-26T08:31:34Z

Hi @pmzandbergen. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

rogyar · 2020-07-26T13:35:29Z

Hi @pmzandbergen.
Please, sign Adobe CLA. It's a required step for introducing your changes to Magento core. Thank you.

pmzandbergen · 2020-07-27T05:42:21Z

Hi @pmzandbergen.
Please, sign Adobe CLA. It's a required step for introducing your changes to Magento core. Thank you.

@rogyar tried to do so yesterday but the site gave a SSL error. Seems to be fixed, checking the CLA now.

m2-assistant · 2020-07-27T05:45:33Z

Hi @pmzandbergen, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

m2-assistant · 2020-07-27T05:45:37Z

Hi @pmzandbergen. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

ihor-sviziev

In general looks good. Would be good to have strict comparison and cover your change with some kind of test, for instance unit test.
Could you update your PR accordingly?

lib/internal/Magento/Framework/HTTP/Client/Curl.php

ihor-sviziev · 2020-07-29T03:21:20Z

@magento run all tests

Co-authored-by: Ihor Sviziev <[email protected]>

pmzandbergen · 2020-07-29T19:27:16Z

In general looks good. Would be good to have strict comparison

Done

and cover your change with some kind of test, for instance unit test.

There are no existing tests for the cookie handling of the client. Adding tests for the cookie handling in general would be nice.

Could you update your PR accordingly?

Done.

pmzandbergen · 2020-07-29T19:34:15Z

cover your change with some kind of test

I'll add tests next Friday.

…luding tests

…ndbergen/magento2 into patch-1 � Conflicts: � lib/internal/Magento/Framework/HTTP/Client/Curl.php

pmzandbergen · 2020-08-02T07:49:38Z

@ihor-sviziev added tests. Unfortunately the cUrl class needs to be extended for this test to allow mocking of the curl_exec function since it calls the protected parseHeaders method. The CurlMockTest class could be used for other tests requiring a response as well.

pmzandbergen · 2020-08-26T07:28:34Z

Hi @pmzandbergen.
Thank you for your collaboration.
Do you have some updates regarding test coverage?
If you need some help on it, please tell us.
Thank you.

Unfortunately there are only 24 hours in a day ;-) It's on my TODO list, updates will follow.

pmzandbergen · 2020-08-30T12:01:40Z

Unfortunately tests still failing. Could you try following option?
#29274 (comment)

Tests updated, removed the Curl mock and added tests using the reflection method instead.

ihor-sviziev · 2020-08-30T12:27:59Z

@magento run all tests

sidolov · 2020-08-31T20:36:32Z

Failed tests are not related to the PR changes

magento-engcom-team · 2020-08-31T20:37:44Z

Hi @sidolov, thank you for the review.
ENGCOM-8107 has been created to process this Pull Request

magento-engcom-team · 2020-08-31T20:37:46Z

@pmzandbergen thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

magento-engcom-team · 2020-08-31T20:39:30Z

Hi @ihor-sviziev, thank you for the review.
ENGCOM-8107 has been created to process this Pull Request

engcom-Alfa · 2020-09-17T06:40:19Z

✔️ QA Passed

Manual testing scenario:

Need to become a debugger to check the value on line:

 if ("Set-Cookie" == $name) {

Go to Admin -> Find Partners & Extensions (for example);

Before: ✖️ It doesn't respect those header names ("set-cookie" or "SET-COOKIE")

After: ✔️ It respects those header names

m2-assistant · 2020-09-18T01:48:20Z

Hi @pmzandbergen, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

magento-engcom-team added Component: HTTP Release Line: 2.4 labels Jul 26, 2020

ghost added the Progress: pending review label Jul 26, 2020

rogyar self-assigned this Jul 26, 2020

pmzandbergen closed this Jul 27, 2020

pmzandbergen reopened this Jul 27, 2020

ghost unassigned rogyar Jul 27, 2020

ihor-sviziev added Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests labels Jul 29, 2020

ihor-sviziev self-assigned this Jul 29, 2020

ihor-sviziev requested changes Jul 29, 2020

View reviewed changes

lib/internal/Magento/Framework/HTTP/Client/Curl.php Outdated Show resolved Hide resolved

ghost added Progress: needs update and removed Progress: pending review labels Jul 29, 2020

pmzandbergen and others added 2 commits July 29, 2020 21:12

Update lib/internal/Magento/Framework/HTTP/Client/Curl.php

ba6513f

Co-authored-by: Ihor Sviziev <[email protected]>

Merge branch '2.4-develop' into patch-1

730c495

pmzandbergen changed the title ~~Update Curl to respect case-insensitive headers~~ WIP: Update Curl to respect case-insensitive headers Jul 29, 2020

pmzandbergen added 2 commits August 2, 2020 09:41

Updated cUrl client to accept case-insensitive Set-Cookie headers inc…

552e0c1

…luding tests

Merge branches '2.4-develop' and 'patch-1' of https://github.com/pmza…

b5463b8

…ndbergen/magento2 into patch-1 � Conflicts: � lib/internal/Magento/Framework/HTTP/Client/Curl.php

pmzandbergen changed the title ~~WIP: Update Curl to respect case-insensitive headers~~ Update Curl to respect case-insensitive headers Aug 2, 2020

ihor-sviziev removed the Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests label Aug 2, 2020

pmzandbergen added 2 commits August 30, 2020 13:59

Updated tests

e912bdf

Merge remote-tracking branch 'origin/patch-1' into patch-1

d5c674d

pmzandbergen requested a review from ihor-sviziev August 30, 2020 12:00

ghost added Progress: pending review and removed Progress: needs update labels Aug 31, 2020

sidolov approved these changes Aug 31, 2020

View reviewed changes

ghost assigned sidolov Aug 31, 2020

ghost added Progress: ready for testing and removed Progress: pending review labels Aug 31, 2020

ihor-sviziev approved these changes Aug 31, 2020

View reviewed changes

engcom-Alfa self-assigned this Sep 17, 2020

ghost added Progress: testing in progress and removed Progress: ready for testing labels Sep 17, 2020

engcom-Alfa added the QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope label Sep 17, 2020

ghost added Progress: extended testing Progress: accept and removed Progress: testing in progress Progress: extended testing labels Sep 17, 2020

magento-engcom-team pushed a commit that referenced this pull request Sep 18, 2020

ENGCOM-8107: Update Curl to respect case-insensitive headers #29274

c6bbad2

magento-engcom-team merged commit 399f077 into magento:2.4-develop Sep 18, 2020

Update Curl to respect case-insensitive headers #29274

Update Curl to respect case-insensitive headers #29274

Uh oh!

Conversation

pmzandbergen commented Jul 26, 2020 • edited by m2-assistant bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

Resolved issues:

Uh oh!

m2-assistant bot commented Jul 26, 2020

Uh oh!

rogyar commented Jul 26, 2020

Uh oh!

pmzandbergen commented Jul 27, 2020

Uh oh!

m2-assistant bot commented Jul 27, 2020

Uh oh!

m2-assistant bot commented Jul 27, 2020

Uh oh!

ihor-sviziev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ihor-sviziev commented Jul 29, 2020

Uh oh!

pmzandbergen commented Jul 29, 2020

Uh oh!

pmzandbergen commented Jul 29, 2020

Uh oh!

pmzandbergen commented Aug 2, 2020

Uh oh!

pmzandbergen commented Aug 26, 2020

Uh oh!

pmzandbergen commented Aug 30, 2020

Uh oh!

ihor-sviziev commented Aug 30, 2020

Uh oh!

sidolov commented Aug 31, 2020

Uh oh!

magento-engcom-team commented Aug 31, 2020

Uh oh!

magento-engcom-team commented Aug 31, 2020

Uh oh!

magento-engcom-team commented Aug 31, 2020

Uh oh!

engcom-Alfa commented Sep 17, 2020

Uh oh!

m2-assistant bot commented Sep 18, 2020

Uh oh!

Uh oh!

pmzandbergen commented Jul 26, 2020 •

edited by m2-assistant bot

Loading