Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#21182 closed defect (bug) (fixed)

WP_HTTP_Cookie doesn't handle the default conditions for the 'domain' and 'path' field correctly.

Reported by: dd32 Owned by: dd32
Milestone: 3.7 Priority: low
Severity: minor Version:
Component: HTTP API Keywords: has-patch commit
Focuses: Cc:


WP_HTTP_Cookie currently handles "simple" cookies in exactly the same way they're received, that is, it only fills in the details that the set-cookie header contained.

However, some fields are optional, and as a result of this, the rfc for state management specifies a set of default values for certain fields:

4.3.1  Interpreting Set-Cookie

   The user agent keeps separate track of state information that arrives
   via Set-Cookie response headers from each origin server (as
   distinguished by name or IP address and port).  The user agent
   applies these defaults for optional attributes that are missing:

   VersionDefaults to "old cookie" behavior as originally specified by
          Netscape.  See the HISTORICAL section.

   Domain Defaults to the request-host.  (Note that there is no dot at
          the beginning of request-host.)

   Max-AgeThe default behavior is to discard the cookie when the user
          agent exits.

   Path   Defaults to the path of the request URL that generated the
          Set-Cookie response, up to, but not including, the
          right-most /.

   Secure If absent, the user agent may send the cookie over an
          insecure channel.

We currently don't do anything special for Secure cookies (From what I can see), but we also need to handle the 'domain' and 'path' field defaults better, as currently they remain at the default null if nothing is passed. This can result in domains passing the WP_HTTP_Cookie::test() method to a different domain or path than they were issued on.

Example cookie values (and WP_HTTP_Cookie representations) which can trigger this:

PHPSESSID=ros1liponkqip23k9le0hhmp31; path=/' (length=44)
test=1341632838; expires=Sat, 07-Jul-2012 04:47:18 GMT
array (size=2)
  0 => 
      public 'name' => string 'PHPSESSID' (length=9)
      public 'value' => string 'ros1liponkqip23k9le0hhmp31' (length=26)
      public 'expires' => null
      public 'path' => string '/' (length=1)
      public 'domain' => null
  1 => 
      public 'name' => string 'test' (length=4)
      public 'value' => string '1341632838' (length=10)
      public 'expires' => int 1341636438
      public 'path' => null
      public 'domain' => null

This is not a issue for WordPress core, but could affect plugins who do anything special with Cookies.

Attachments (4)

21182.diff (11.7 KB) - added by dd32 2 years ago.
21182.unit-tests.diff (1.9 KB) - added by dd32 2 years ago.
21182.2.diff (5.9 KB) - added by dd32 2 years ago.
21182.3.diff (5.1 KB) - added by dd32 2 years ago.
Just the cookie defaults

Download all attachments as: .zip

Change History (11)

comment:1 @dd322 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

@dd322 years ago

comment:2 @dd322 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

Attachment 21182.diff​ added

This builds off 17588.2.diff for #17588 and adds full Cookie Support to the HTTP API.

  • Adds the Defaults for the cookie
  • Adds support for "2nd generation cookies" (Set-Cookie2:..)
  • Passes cookies in redirect requests
  • Evaluates which cookies to pass to further requests
  • Fixes the handling of the 'port' parameter of cookie
  • Causes the API to return the cookies set by the request in addition to, the cookies which were passed to the request (previously it would only return cookies set, not used).

Still doesn't handle anything to do with the Secure parameter, or HTTPOnly parameter (not that the latter is of concern to us).

@dd322 years ago

@dd322 years ago

comment:3 @dd322 years ago

Attachment 21182.2.diff​ added

This is 21182.diff minus the code for #17588

Needs testing that cookies pass between requests correctly and that they overwrite/update each other correctly

comment:4 @dd322 years ago

  • Milestone changed from Future Release to 3.7

Moved passing cookies to further locations on redirects to #24987

@dd322 years ago

Just the cookie defaults

comment:5 @dd322 years ago

  • Keywords commit added; needs-testing removed
  • Owner set to dd32
  • Status changed from new to accepted

comment:6 @dd322 years ago

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

In 25044:

WP_HTTP: Cookies: Fill the defaults for the Cookie object based on the current requested URL. Fixes #21182

comment:7 @dd322 years ago

In 25176:

WP_HTTP: Make the new 2nd parameter to WP_HTP::processHeaders() as optional. See #21182. Fixes #25179

Note: See TracTickets for help on using tickets.