`http` uses `email` header parser but they have slightly different rules about whitespace

[bentasker]

Bug report

The function header_source_parse only strips leading whitespace from header values, any trailing whitespace before the CRLF is left intact:

>>> b = header_source_parse(["content-length:15 "])
>>> b[1]
'15 '

This can cause issues further up the callchain (for example: pallets/werkzeug#2734).

Although part of Email, this header parsing functionality is also used by http (call chain starts here), so the HTTP RFC's are also relevant

RFC 7231 notes

A field value might be preceded and/or followed by optional
whitespace (OWS); a single SP preceding the field-value is preferred
for consistent readability by humans. The field value does not
include any leading or trailing whitespace: OWS occurring before the
first non-whitespace octet of the field value or after the last
non-whitespace octet of the field value ought to be excluded by
parsers when extracting the field value from a header field.

Unfortunately, the Email RFC (RFC 5322) conflicts with this, expecting headers of the from name: value<CRLF>.

This means that the Email header parser's response, under some circumstances, is not compatible with the HTTP spec.

Options:

1. Change header_source_parse() to use strip() instead of lstrip()
2. Do subsequent processing in http to check for and strip trailing whitespace
3. Make changes to the entire callchain to allow header_source_parse() to be told whether to strip trailing whitespace

Option 1 sounds inherently dangerous - although I can't think of a usage that would require preservation of trailing whitespace, it's permitted by RFC5322 so the workflow will exist somewhere.

Option 2 seems better than 3 - for 3) every single caller would need to be identified and updated.

Linked PRs

• gh-105973: Ensure that http header parser returns RFC 7230 compliant header values #105994

[gaogaotiantian]

Could you elaborate on where the email RFC specifies "no space" in headers? From your link, I read

A field body may be composed of printable US-ASCII characters
as well as the space (SP, ASCII value 32) and horizontal tab (HTAB,
ASCII value 9) characters (together known as the white space
characters, WSP).

It seems like space could be part of the field body of the headers, and I did not see any rules for trailing spaces.

[bentasker]

Sorry, I was probably a little unclear in that bit.

The email RFC allows spaces and doesn't make special allowances for trailing spaces, so from: ben is valid, and the value is ben (i.e. with trailing space preserved).

However, when passed via HTTP that same value needs to become ben (no trailing space).

It seems like space could be part of the field body of the headers, and I did not see any rules for trailing spaces.

Yes, exactly this. That's why I think option 1, though easy to implement, is too risky a change.

I was thinking of putting together a PR for 2. as it probably makes the most sense to implement http specific requirements within http

[gaogaotiantian]

Okay so it's not an issue with the email module - I think the behavior is correct and we should definitely preserve that. Maybe you could come up with a more detailed description about how it would fail in http? I have trouble imagining how an email header becomes an http header.

[bentasker]

Okay so it's not an issue with the email module

Yeah, on reflection, it's an issue with http rather than email - the rules being enforced are correct for email, it's just that the function is getting reused by other modules subject to different rules.

have trouble imagining how an email header becomes an http header.

It happens because the http module doesn't re-implement header parsing, it imports email and has that do it.

So, if you (for example) stand up a Flask debug server, it uses http.server.

When you place a request against that server BaseHTTPRequestHandler:parse_request() calls http.client.parse_headers().

parse_headers() calls http.client._parse_header_lines().

The client imports email and the headers (at this stage just a single string) are passed to email.parser.Parser.parsestr().

So, although header_source_pass was intended to deal with email headers (From, To etc), it's also ended up dealing with HTTP headers which have slightly different rules applied to them in terms of what's considered valid.

I think, if I put a PR, the way I'd approach it would be to sanitise them here.

[gaogaotiantian]

Ah, that makes sense now. You are getting a Message object in http.client._parse_header_lines() right? So it might take some effort to clean it, which makes it a non-trivial change and the code owner's opinion is needed.

You are more than welcome to work on the PR draft now, but I don't know if the approach will be accepted. You can also wait for the core dev's opinion. In the meantime, I'll edit your issue title to reflect the actual issue.

[gaogaotiantian]

@gpshead I think you've been working on this method recently, could you share some thoughts on this?

[bentasker]

Awesome, thanks for the help!

I probably won't get started on the PR tonight anyway, so will hold off til we hear back

[gpshead]

go ahead with a PR, I can review it. for this kind of thing I find it helpful to future maintainers to link to relevant RFC sections from comments in code even if trivial. (and obviously, include regression tests). agreed that option-1 sounds undesirable. A rstrip() transformation of the field value from the email code within the http code perhaps as option-2?

[bentasker]

Thanks!

I'm hoping I should have some time this evening to look at putting something together.

A rstrip() transformation of the field value from the email code within the http code perhaps as option-2?

Yep, that was more or less my thinking - move the call to email up in the function then iterate through the response and rstrip() the values before returning the result.

for this kind of thing I find it helpful to future maintainers to link to relevant RFC sections from comments in code even if trivial.

Noted