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

Unexpected Cookies sent if persistent_handle_id is used #36

Closed
rcanavan opened this issue Apr 7, 2016 · 11 comments
Closed

Unexpected Cookies sent if persistent_handle_id is used #36

rcanavan opened this issue Apr 7, 2016 · 11 comments

Comments

@rcanavan
Copy link

rcanavan commented Apr 7, 2016

Given the following trivial test program that always sends a cookie "test" and, if Cookie "test" was received, the additional cookie "test2":

<?php if (!empty($_COOKIE["test"])) setcookie("test2", $_COOKIE["test"]); setcookie("test", "1"); echo "done";

Then ext-httpd 3.0.1 unexpectedly sends the "test" cookie in the second request of the test program below:

<?php                                                                                                                                                            
$url = 'http://192.168.56.1:14080/cookie.php';                                                                                                                   

foreach (array('a', 'B') as $id) {
    $client = new http\Client('curl', $id);
    $req1 = new  http\Client\Request('GET', $url);
    $client->enqueue($req1);
    $client->send();
    $response1 = $client->getResponse($req1);
    fprintf(STDERR, ">%s<\n", print_r($response1->getHeaders(), true));
    printf("curl: %s http: %s\n\n\n", $response1->getTransferInfo('curlcode'), $response1->getTransferInfo('response_code')); 
}

i.e. the output contains test2=1, although the persistent handle was different for each request, and both client and request were new objects:

[Set-Cookie] => Array
    (
        [0] => test2=1
        [1] => test=1
    )

This bahaviour is not present if no persistent handle id is used.

@m6w6
Copy link
Owner

m6w6 commented Apr 7, 2016

Ugh, ouch.

Here's a quick fix: persistent_request.txt

Thank you for the report!

@rcanavan
Copy link
Author

rcanavan commented Apr 8, 2016

Thanks, the quick fix actually fixes this problem for me.

@rcanavan
Copy link
Author

rcanavan commented Apr 8, 2016

I may have spoken too soon. I'd actually expect the test program not to send the Cookies even if the persistent_handle_id is identical for two requests. Replacing the array with e.g. array('a', 'a') returns to the faulty behaviour, even though var_dump($client->getCookies()); appears to indicate that no cookies are present.

Do I have to explicitly disable cookie handling in the request or client? So far I've tried various combinations and variants of $req1->setOptions(array('cookies' => array(), 'cookiestore' => NULL, 'cookiesession' => false)); without success.

@m6w6
Copy link
Owner

m6w6 commented Apr 8, 2016

No, if you don't want to share anything, don't use persistent handles, or use different ids.

http\Client::getCookies() just returns static cookies set by http\Client::setCookies().

More thorough cookie handling like flushing etc. is still on a TODO.

@rcanavan
Copy link
Author

rcanavan commented Apr 8, 2016

My plan was to use persistent handles to only recycle TCP and SSL Connections, but not other states, such as Cookies or e.g. the proxy configuration. Is any Documentation available or can you give hints how that could be achieved, or what features may be missing for that to work?

@m6w6
Copy link
Owner

m6w6 commented Apr 8, 2016

Since the v2.0 refactoring, we're missing support for CURLOPT_COOKIELIST, because curl_easy handles do not have any userland representation anymore.

If you need the recycling only per server request (i.e. single script runtime), then don't use persistent handles.

@m6w6
Copy link
Owner

m6w6 commented Apr 26, 2016

I'm currently working on a patch, which might work contrary to your liking, though.

I think the client should behave more like a client is supposed to, i.e. share more between it's requests.

The idea is to include a curl_share handle in the client, which enables cookie and TLS session sharing for all requests of this client.

@rcanavan
Copy link
Author

I'd appreciate if you could make this behavior switchable.

In my opinion, it's a useful feature to use pecl_http so to be used in "proxy mode", such that any secrets such as cookies are transparently passed between the user and the system pecl_http communicates with, so that they don't need to be stored on the PHP server and associated with a session there. Persistence still provides a significant performance advantage in this case.

As far as I'm aware, one cannot share any relevant TLS secrets between the connections between the user and PHP on one hand via pecl_http on the other hand - things like client certificates would typically be constant and shared for all pecl_http requests to a given sever.

@m6w6
Copy link
Owner

m6w6 commented May 24, 2016

So, I added curl_share support to the client, which means, that cookies and ssl session info is now shared in a meaningful way across all queued requests, unless you disable it:

$client->configure(["share_cookies" => false, "share_ssl" => false]);

Would you mind giving current HEAD a try?

Thank you.

@afflerbach
Copy link

On behalf of @rcanavan I tried 54059a4 and can confirm that the issue is now gone.
"share_cookies" => false is needed in our case and works as one would expect. Thank you!

@m6w6
Copy link
Owner

m6w6 commented Jun 1, 2016

Thanks!

@m6w6 m6w6 closed this as completed Jun 1, 2016
m6w6 added a commit that referenced this issue Oct 4, 2016
+ Added http\Client\Curl\User interface for userland event loops
+ Added http\Url::IGNORE_ERRORS, http\Url::SILENT_ERRORS and
http\Url::STDFLAGS
+ Added http\Client::setDebug(callable $debug)
+ Added http\Client\Curl\FEATURES constants and namespace
+ Added http\Client\Curl\VERSIONS constants and namespace
+ Added share_cookies and share_ssl (libcurl >= 7.23.0) options to
http\Client::configure()
+ http\Client uses curl_share handles to properly share cookies and
SSL/TLS sessions between requests
+ Improved configure checks for default CA bundles
+ Improved negotiation precision
* Fixed regression introduced by http\Params::PARSE_RFC5987: negotiation
using the params parser would receive param keys without the trailing
asterisk, stripped by http\Params::PARSE_RFC5987.
* Fix gh-issue #50: http\Client::dequeue() within
http\Client::setDebug() causes segfault (Mike, Maik Wagner)
* Fix gh-issue #47: http\Url: Null pointer deref in sanitize_value()
(Mike, @rc0r)
* Fix gh-issue #45: HTTP/2 response message parsing broken with libcurl
>= 7.49.1 (Mike)
* Fix gh-issue #43: Joining query with empty original variable in query
(Mike, Sander Backus)
* Fix gh-issue #42: fatal error when using punycode in URLs (Mike,
Sebastian Thielen)
* Fix gh-issue #41: Use curl_version_info_data.features when
initializing options (Mike)
* Fix gh-issue #40: determinde the SSL backend used by curl at runtime
(Mike, @rcanavan)
* Fix gh-issue #39: Notice: http\Client::enqueue(): Could not set option
proxy_service_name (Mike, @rcanavan)
* Fix gh-issue #38: Persistent curl handles: error code not properly
reset (Mike, @afflerbach)
* Fix gh-issue #36: Unexpected cookies sent if persistent_handle_id is
used (Mike, @rcanavan, @afflerbach)
* Fix gh-issue #34: allow setting multiple headers with the same name
(Mike, @rcanavan)
* Fix gh-issue #33: allow setting prodyhost request option to NULL
(Mike, @rcanavan)
* Fix gh-issue #31: add/improve configure checks for default CA
bundle/path (Mike, @rcanavan)

Changes from beta1:
* Fixed PHP-5.3 compatibility
* Fixed recursive calls to the event loop dispatcher

Changes from beta2:
* Fix bug #73055: crash in http\QueryString (Mike, @rc0r)
(CVE-2016-7398)
* Fix bug #73185: Buffer overflow in HTTP parse_hostinfo() (Mike, @rc0r)
* Fix HTTP/2 version parser for older libcurl versions (Mike)
m6w6 added a commit that referenced this issue Oct 4, 2016
+ Added http\Client\Curl\User interface for userland event loops
+ Added http\Url::IGNORE_ERRORS, http\Url::SILENT_ERRORS and
http\Url::STDFLAGS
+ Added http\Client::setDebug(callable $debug)
+ Added http\Client\Curl\FEATURES constants and namespace
+ Added http\Client\Curl\VERSIONS constants and namespace
+ Added share_cookies and share_ssl (libcurl >= 7.23.0) options to
http\Client::configure()
+ http\Client uses curl_share handles to properly share cookies and
SSL/TLS sessions between requests
+ Improved configure checks for default CA bundles
+ Improved negotiation precision
* Fixed regression introduced by http\Params::PARSE_RFC5987: negotiation
using the params parser would receive param keys without the trailing
asterisk, stripped by http\Params::PARSE_RFC5987.
* Fix gh-issue #50: http\Client::dequeue() within
http\Client::setDebug() causes segfault (Mike, Maik Wagner)
* Fix gh-issue #47: http\Url: Null pointer deref in sanitize_value()
(Mike, @rc0r)
* Fix gh-issue #45: HTTP/2 response message parsing broken with libcurl
>= 7.49.1 (Mike)
* Fix gh-issue #43: Joining query with empty original variable in query
(Mike, Sander Backus)
* Fix gh-issue #42: fatal error when using punycode in URLs (Mike,
Sebastian Thielen)
* Fix gh-issue #41: Use curl_version_info_data.features when
initializing options (Mike)
* Fix gh-issue #40: determinde the SSL backend used by curl at runtime
(Mike, @rcanavan)
* Fix gh-issue #39: Notice: http\Client::enqueue(): Could not set option
proxy_service_name (Mike, @rcanavan)
* Fix gh-issue #38: Persistent curl handles: error code not properly
reset (Mike, @afflerbach)
* Fix gh-issue #36: Unexpected cookies sent if persistent_handle_id is
used (Mike, @rcanavan, @afflerbach)
* Fix gh-issue #34: allow setting multiple headers with the same name
(Mike, @rcanavan)
* Fix gh-issue #33: allow setting prodyhost request option to NULL
(Mike, @rcanavan)
* Fix gh-issue #31: add/improve configure checks for default CA
bundle/path (Mike, @rcanavan)

Changes from beta1:
* Fixed recursive calls to the event loop dispatcher

Changes from beta2:
+ Improved configure checks for IDNA libraries (added
--with-http-libicu-dir, --with-http-libidnkit{,2}-dir,
--with-http-libidn2-dir)
* Fix bug #73055: crash in http\QueryString (Mike, @rc0r)
(CVE-2016-7398)
* Fix bug #73185: Buffer overflow in HTTP parse_hostinfo() (Mike, @rc0r)
* Fix HTTP/2 version parser for older libcurl versions (Mike)
* Fix gh-issue #52: Underscores in host names: libidn Failed to parse
IDN (Mike, @canavan)
m6w6 added a commit that referenced this issue Dec 12, 2016
+ Added http\Client\Curl\User interface for userland event loops
+ Added http\Url::IGNORE_ERRORS, http\Url::SILENT_ERRORS and
  http\Url::STDFLAGS
+ Added http\Client::setDebug(callable $debug)
+ Added http\Client\Curl\FEATURES constants and namespace
+ Added http\Client\Curl\VERSIONS constants and namespace
+ Added share_cookies and share_ssl (libcurl >= 7.23.0) options to
  http\Client::configure()
+ http\Client uses curl_share handles to properly share cookies and
  SSL/TLS sessions between requests
+ Improved configure checks for default CA bundles
+ Improved negotiation precision
* Fixed regression introduced by http\Params::PARSE_RFC5987: negotiation
  using the params parser would receive param keys without the trailing
  asterisk, stripped by http\Params::PARSE_RFC5987.
* Fix gh-issue #50: http\Client::dequeue() within
  http\Client::setDebug() causes segfault (Mike, Maik Wagner)
* Fix gh-issue #47: http\Url: Null pointer deref in sanitize_value()
  (Mike, @rc0r)
* Fix gh-issue #45: HTTP/2 response message parsing broken with
  libcurl >= 7.49.1 (Mike)
* Fix gh-issue #43: Joining query with empty original variable in query
  (Mike, Sander Backus)
* Fix gh-issue #42: fatal error when using punycode in URLs
  (Mike, Sebastian Thielen)
* Fix gh-issue #41: Use curl_version_info_data.features when
  initializing options (Mike)
* Fix gh-issue #40: determinde the SSL backend used by curl at runtime
  (Mike, @rcanavan)
* Fix gh-issue #39: Notice: http\Client::enqueue(): Could not set option
  proxy_service_name (Mike, @rcanavan)
* Fix gh-issue #38: Persistent curl handles: error code not properly
  reset (Mike, @afflerbach)
* Fix gh-issue #36: Unexpected cookies sent if persistent_handle_id is
  used (Mike, @rcanavan, @afflerbach)
* Fix gh-issue #34: allow setting multiple headers with the same name
  (Mike, @rcanavan)
* Fix gh-issue #33: allow setting prodyhost request option to NULL
  (Mike, @rcanavan)
* Fix gh-issue #31: add/improve configure checks for default
  CA bundle/path (Mike, @rcanavan)

Changes from beta1:
* Fixed PHP-5.3 compatibility
* Fixed recursive calls to the event loop dispatcher

Changes from beta2:
* Fix bug #73055: crash in http\QueryString (Mike, @rc0r)
  (CVE-2016-7398)
* Fix bug #73185: Buffer overflow in HTTP parse_hostinfo() (Mike, @rc0r)
  (CVE-2016-7961)
* Fix HTTP/2 version parser for older libcurl versions (Mike)
m6w6 added a commit that referenced this issue Dec 12, 2016
+ Added http\Client\Curl\User interface for userland event loops
+ Added http\Url::IGNORE_ERRORS, http\Url::SILENT_ERRORS and
  http\Url::STDFLAGS
+ Added http\Client::setDebug(callable $debug)
+ Added http\Client\Curl\FEATURES constants and namespace
+ Added http\Client\Curl\VERSIONS constants and namespace
+ Added share_cookies and share_ssl (libcurl >= 7.23.0) options to
  http\Client::configure()
+ http\Client uses curl_share handles to properly share cookies and
  SSL/TLS sessions between requests
+ Improved configure checks for default CA bundles
+ Improved negotiation precision
* Fixed regression introduced by http\Params::PARSE_RFC5987: negotiation
  using the params parser would receive param keys without the trailing
  asterisk, stripped by http\Params::PARSE_RFC5987.
* Fix gh-issue #50: http\Client::dequeue() within
  http\Client::setDebug() causes segfault (Mike, Maik Wagner)
* Fix gh-issue #47: http\Url: Null pointer deref in sanitize_value()
  (Mike, @rc0r)
* Fix gh-issue #45: HTTP/2 response message parsing broken with
  libcurl >= 7.49.1 (Mike)
* Fix gh-issue #43: Joining query with empty original variable in query
  (Mike, Sander Backus)
* Fix gh-issue #42: fatal error when using punycode in URLs
  (Mike, Sebastian Thielen)
* Fix gh-issue #41: Use curl_version_info_data.features when
  initializing options (Mike)
* Fix gh-issue #40: determinde the SSL backend used by curl at runtime
  (Mike, @rcanavan)
* Fix gh-issue #39: Notice: http\Client::enqueue(): Could not set option
  proxy_service_name (Mike, @rcanavan)
* Fix gh-issue #38: Persistent curl handles: error code not properly
  reset (Mike, @afflerbach)
* Fix gh-issue #36: Unexpected cookies sent if persistent_handle_id is
  used (Mike, @rcanavan, @afflerbach)
* Fix gh-issue #34: allow setting multiple headers with the same name
  (Mike, @rcanavan)
* Fix gh-issue #33: allow setting prodyhost request option to NULL
  (Mike, @rcanavan)
* Fix gh-issue #31: add/improve configure checks for default
  CA bundle/path (Mike, @rcanavan)

Changes from beta1:
* Fixed recursive calls to the event loop dispatcher

Changes from beta2:
+ Improved configure checks for IDNA libraries (added
  --with-http-libicu-dir, --with-http-libidnkit{,2}-dir,
  --with-http-libidn2-dir)
* Fix bug #73055: crash in http\QueryString (Mike, @rc0r)
  (CVE-2016-7398)
* Fix bug #73185: Buffer overflow in HTTP parse_hostinfo() (Mike, @rc0r)
  (CVE-2016-7961)
* Fix HTTP/2 version parser for older libcurl versions (Mike)
* Fix gh-issue #52: Underscores in host names: libidn Failed to parse
  IDN (Mike, @canavan)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants