WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 22 months ago

Last modified 21 months ago

#14191 closed defect (bug) (fixed)

The test() function of the cookie class does not allow session cookies

Reported by: mailnew2ster Owned by: dd32
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.0
Component: HTTP API Keywords: has-patch needs-testing
Focuses: Cc:

Description

http://core.trac.wordpress.org/browser/trunk/wp-includes/class-http.php#L1777

Because $this->expires is not defined for session cookies, test() always returns false.

Attachments (1)

.diff (1.1 KB) - added by mailnew2ster 4 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 mailnew2ster4 years ago

Patch also improves the "new cookie from array" code, by making it cleaner and allowing unknown values such as httponly (like the "new cookie from string" code does).

mailnew2ster4 years ago

comment:2 dd324 years ago

  • Keywords has-patch needs-testing added
  • Version set to 3.0

comment:3 nacin3 years ago

  • Milestone changed from Awaiting Review to 3.1

Would this allow more than necessary though?

The isset() should be enough to fix the issue as we set this->expires to null.

comment:4 nacin3 years ago

  • Milestone changed from 3.1 to Future Release

comment:5 dd3222 months ago

  • Milestone changed from Future Release to 3.5
  • Owner set to dd32
  • Status changed from new to accepted

While testing this, I've just noticed that WP_Http_Cookie::test() is never actually used by core, or WP_HTTP itself.

The following code works fine at present without this change:

$one = wp_remote_get( 'http://tools.dd32.id.au/wordpress/cookie-test.php' );
$two = wp_remote_get( 'http://tools.dd32.id.au/wordpress/cookie-test.php',
                       array( 'cookies' => $one['cookies'] ) );
var_dump( $one, $two );

The 2nd request is sent with the cookies returned from request 1, However, If you wish to validate the cookies can be sent to the 2nd/new destination, then calling $cookie->test() would still fail.

Looking at testing it, and the attached patch, nacin was right that only the 2nd chunk was required, While the first chunk of diff is nice to "clean up" the existing code, there's some bugs that need fixing which would be harder with the cleanup.

comment:6 dd3222 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In [21226]:

WP_HTTP: Respect that some cookies do not have an expiration time, this is a valid use-case that WP_HTTP_Cookie::test() should not discard, a non-existant expiration date simply means to let it expire at the end of the session. Props mailnew2ster. Fixes #14191

comment:7 mailnew2ster21 months ago

While the first chunk of diff is nice to "clean up" the existing code, there's some bugs that need fixing which would be harder with the cleanup.

It's not only a "nice clean up".
I quote my first comment:

Patch also improves the "new cookie from array" code, by making it cleaner and allowing unknown values such as httponly (like the "new cookie from string" code does).

It improves the function, making it:

  1. Accept values different than name, value, path, domain and expires (e.g. httponly).
  2. Case-insensitive (e.g. accept name, Name, or NAME).

I understand it's unrelated to the current ticket title, I just wanted to clarify the purpose of my change.

Note: See TracTickets for help on using tickets.