Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow horizontal tabs in header values #3451

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dave-shawley
Copy link

Addresses #3450

Copy link
Member

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! You're right that tabs should be allowed here, you're just the first to ask for it.

tornado/web.py Outdated
@@ -399,7 +399,7 @@ def clear_header(self, name: str) -> None:
if name in self._headers:
del self._headers[name]

_INVALID_HEADER_CHAR_RE = re.compile(r"[\x00-\x1f]")
_INVALID_HEADER_CHAR_RE = re.compile(r"[\x00-\x08\x0a-\x1f]")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're here: I went back to the RFCs to make sure that this change is correct, and found that \x7f (DEL) is also invalid. So let's add that to the list too: \x00-\x08\x0a-\x1f\x7f. Or for a slightly larger code change we could invert it to define valid chars (like the RFCs do) instead of invalid ones: \x09\x20-\x7e\x80-0xff.

raise

self.set_header("X-Foo", "foo\tX-Bar: baz")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nitpicky, but my first thought on seeing this test case is "why does it look like another header value after the tab? Is that some sort of injection attack I haven't seen?". Header injection is the reason \r\n are banned, but if we're expanding this handler to be the general test of set_header's enforcement of valid header characters we should probably rename it and use examples that look less like injection attacks.

Also, there should probably be a check in test_header_injection to assert that the valid header is readable from the client side.

@dave-shawley
Copy link
Author

@bdarnell I force pushed over my branch to clean up the commit history. I took your advice and added a new test case instead of reusing the one from test_header_injection. I also added an exclusion for DEL.

This makes the expression match what is in the RFC. I also added a test
for empty header values since I messed up the RE the first time around.
@dave-shawley
Copy link
Author

Since inverting the sense of the regular expression required switching from re.search to re.fullmatch, should I implement the requirement that the first and last characters in a header are printable characters?

  field-value    = *field-content
  field-content  = field-vchar [ 1*( SP / HTAB / field-vchar ) field-vchar ]
  field-vchar    = VCHAR / obs-text
  obs-text       = %x80-FF

I think that changing the RE from [\x09\x20-\x7e\x80-\xff]* to [\x21-\x7e\x80-\xff]([\x09\x20-\x7e\x80-\xff]*[\x21-\x7e\x80-\xff])* would do the job. I'm not sure if increasing the RE complexity is worth it or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants