WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#13513 closed defect (bug) (invalid)

Processing HTTP Headers violates RFC 2616 - same header-name with multiple fields and values not normalized

Reported by: hakre Owned by: dd32
Milestone: Priority: normal
Severity: normal Version: 3.0
Component: HTTP API Keywords:
Focuses: Cc:

Description

Quote:

 Multiple message-header fields with the same field-name MAY be
   present in a message if and only if the entire field-value for that
   header field is defined as a comma-separated list [i.e., #(values)].
   It MUST be possible to combine the multiple header fields into one
   "field-name: field-value" pair, without changing the semantics of the
   message, by appending each subsequent field-value to the first, each
   separated by a comma. The order in which header fields with the same
   field-name are received is therefore significant to the
   interpretation of the combined field value, and thus a proxy MUST NOT
   change the order of these field values when a message is forwarded.

Summary:

  • Multiple message-header fields with the same field-name MAY be present
  • It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair

Currently the code is creating an array instead of a comma separated list.

Attachments (6)

13513.patch (3.2 KB) - added by hakre 5 years ago.
Concating values as List; response header overwrite protection; diverse formattings
13513.2.patch (3.1 KB) - added by hakre 5 years ago.
Updated: Concating values as List; diverse formattings
13513.3.patch (3.1 KB) - added by hakre 5 years ago.
Updated: Concating values as List; Better Status Line parsing; diverse formattings
13513.minor.patch (669 bytes) - added by hakre 5 years ago.
A little fix while in that file.
13513.4.patch (829 bytes) - added by hakre 4 years ago.
process multiple headers the same as a comma sperated list of values and handle such a list as an array in PHP
13513.minor.2.patch (697 bytes) - added by hakre 4 years ago.
13513.minor.patch

Download all attachments as: .zip

Change History (21)

comment:1 @hakre5 years ago

Related: #9395

comment:2 follow-up: @dd325 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 3.0 to Future Release

Can you post a reference for where the quote comes from?

Moving to Future Release pending patch.

comment:3 in reply to: ↑ 2 @nacin5 years ago

Replying to dd32:

Can you post a reference for where the quote comes from?

RFC 2616, 4.2 (Message Headers)

@hakre5 years ago

Concating values as List; response header overwrite protection; diverse formattings

comment:4 @hakre5 years ago

  • Version set to 3.0

A first patch. There is another issue regarding the response status code and message by injecting header not containing any colons. That is somehow already prevented by the "redirection" handling (which will only return the last status field and/or the injected one as I just see, need to update the patch).

I'll update the current patch to remove my handling of that again.

Additionally there is another issue with tolerant status line parsing we do not cover:

Clients SHOULD be tolerant in parsing the Status-Line [...]. In particular, they SHOULD accept any amount of SP or HT characters between fields, even though only a single SP is required.

Source: 19.3 Tolerant Applications / RFC 2616

We just do an explode on a single space.

@hakre5 years ago

Updated: Concating values as List; diverse formattings

@hakre5 years ago

Updated: Concating values as List; Better Status Line parsing; diverse formattings

comment:5 @hakre5 years ago

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

Updated the patch according to my previous notes. Un-needed check for setting the status line has been removed again and the better status line parsing has been implemented according RFC instead.

I gave it a testrun on my testbed and it looks good from my side. I also tested the preg_split code on example data.

@hakre5 years ago

A little fix while in that file.

comment:6 @dd325 years ago

  • Milestone changed from Future Release to 3.1
  • Status changed from new to accepted

Will handle this in 3.1

comment:7 @nacin5 years ago

  • Keywords tested removed
  • Milestone changed from Awaiting Triage to Future Release

comment:8 follow-ups: @dd324 years ago

  • Keywords close 2nd-opinion added

After reading the spec closer, I see no need to change anything here.

Specifically, the RFC quoted is specifying that the fields MAY be combined into a comma separated list(and that servers MUST present multiple headers in a form in which combining to a comma separated list will retain the original meaning), It is not dictating how a client must present the data given to a client (Excluding proxies, who must present the headers in the order given to them, and must not combine them, so as to preserve the meaning behind the message)

Note, That this is different from unfolding where a header is split over multiple lines, and must be re-combined before presenting the field to clients.

comment:9 in reply to: ↑ 8 @hakre4 years ago

Replying to dd32:

Note, That this is different from unfolding where a header is split over multiple lines, and must be re-combined before presenting the field to clients.

That has been already addressed in an earlier ticket and fixed: Related: #9395, [11351]

comment:10 in reply to: ↑ 8 @hakre4 years ago

  • Keywords close removed

Replying to dd32:

After reading the spec closer, I see no need to change anything here.

Specifically, the RFC quoted is specifying that the fields MAY be combined into a comma separated list(and that servers MUST present multiple headers in a form in which combining to a comma separated list will retain the original meaning), It is not dictating how a client must present the data given to a client (Excluding proxies, who must present the headers in the order given to them, and must not combine them, so as to preserve the meaning behind the message)

I see and I can follow that argumentation. What has been represented as a comma separated list within headers is then represented as an array list in our PHP. I'm fine with that approach, I think that's correct after re-reading.

However that does as well mean that multiple header-fields with the same header-name are only correct, if the specification allows a list.

Regardless of that fact, I'm against being too strict here and compare against a list (for the moment) which header-names have such a property (list values) specified or not.

Instead I suggest for those headers that provide multiple header-values to check those if they already contain commas per one array entry and normalize all array entries to values w/o commas so the values are normalized.

@hakre4 years ago

process multiple headers the same as a comma sperated list of values and handle such a list as an array in PHP

comment:11 @hakre4 years ago

And there was that minor patch left which is not in scope, but well, too less to open a ticket on it's own I suppose.

comment:12 @hakre4 years ago

  • Keywords 2nd-opinion removed

@hakre4 years ago

13513.minor.patch

comment:13 @dd324 years ago

  • Milestone Future Release deleted
  • Resolution set to invalid
  • Status changed from accepted to closed

no, Headers should be given as the server has presented it. If it offers a comma separated list, thats what should be passed back to the client.

As for the "minor patch", I prefer the more readable current form (Which is used in all the transports)

Closing as invalid due to not needing to implement it. (Please don't re-open this for the purpose of discussing the minor patch, as the purpose of the ticket is invalid anyway)

comment:14 @hakre4 years ago

  • Keywords has-patch removed
  • Summary changed from Processing HTTP Headers violates RFC 2616 to Processing HTTP Headers violates RFC 2616 - same header-name with multiple fields and values not normalized

comment:15 @hakre4 years ago

Note: 13513.3.patch has wrong order. First should not added behind next, but before.

Note: See TracTickets for help on using tickets.