gh-143916: Allow HTAB in wsgiref header values#144118
gh-143916: Allow HTAB in wsgiref header values#144118sethmlarson wants to merge 4 commits intopython:mainfrom
Conversation
|
See also PR gh-144371 which rejects control characters in |
Co-authored-by: Victor Stinner <vstinner@python.org>
|
Oh, multiple tests are failing: |
|
For some reason, my review doesn't show? But here's my idea: I guess that the reason for the failing checks is that here self._convert_string_type(v) is the name missing Suggestion: self._convert_string_type(v, name=False) |
|
Line 44 |
|
Fixed the last remaining |
|
LGTM! One suggestion: Maybe use raise ValueError("Control characters not allowed in headers and values") (new: "and values") or to be consistent with my PR raise ValueError("Control characters not allowed in headers, values and statuses") |
|
And @vstinner could you please merge this so that we can merge #144371 then as well, please? Thanks! At first I thought that (based upon the fact that status only uses the one with HTAB forbidden) we could already merge mine because this one is already included, but this doesn't work because it would produce further compatibility problems and also name is also included in my PR at the debug mode statement, so please merge this at first so that I can reference your regexes, thanks! |
| if _control_chars_re.search(value): | ||
| regex = (_name_disallowed_re if name else _value_disallowed_re) | ||
| if regex.search(value): | ||
| raise ValueError("Control characters not allowed in headers") |
There was a problem hiding this comment.
| raise ValueError("Control characters not allowed in headers") | |
| raise ValueError("Control characters not allowed in headers, values and statuses") |
There was a problem hiding this comment.
I don't get what are values and statuses. I prefer to just say "headers".
There was a problem hiding this comment.
Hi, sorry for the confusion, I was missing one word: I wanted to say "in header names, values and statuses" because there seems to be a differenciation between values and statuses and names (see in my PR) and that's why I added this to the error message and now I thought that this is now more consistent as it's kind of the same error message. And there's a difference in the handling between header names and values (see in the issue linked here), but I think that for the short error message it's not required to go into detail with the exact allowed and disallowed characters. But thanks for the information that I missed one word, hope that this is clear now! Thanks!
There was a problem hiding this comment.
| raise ValueError("Control characters not allowed in headers") | |
| raise ValueError("Control characters not allowed in header names, values and statuses") |
|
And one other question: How do we handle this concerning backports? Theoretically this is a bug fix, so it should be ported back to only a few supported versions (those recieving bug fixes and not those only recieving security updates) while this seems kind of interesting to me because the other patch (the denial of these chars) was a security update and for this reason applied backward also to those versions only getting security updates and this included this kind of "bug" and for this reason a "old version" now has a new bug through a new update which is not any more patched if we don't apply this to the ones only recieving security updates which is maybe a little bit a of a problem because those using older versions probably do this because they want them to be stable and in this case we "added a bug" / removed a feature and we won't apply it back to them. So will we also merge this to 3.10 and so on? |
In #143917 we were overzealous,
HTAB(0x09) is allowed in header values but not header names.