Skip to content

bpo-11671: add header validation from http.client to wsgiref.headers.Headers #15299

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.

Sign up for GitHub

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

Closed
epicfaace wants to merge 8 commits into from
Closed

bpo-11671: add header validation from http.client to wsgiref.headers.Headers #15299

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
221779a
apply devin's patch from bpo-11671
epicfaace Aug 15, 2019
501ac89
spacing
epicfaace Aug 15, 2019
6928b31
fix: use regex's from http.client to validate header names and values…
epicfaace Aug 15, 2019
6cd271a
test: update tests to be more consistent with http.client tests
epicfaace Aug 15, 2019
8446cfd
use add_header in constructor
epicfaace Aug 19, 2019
71d5819
test: test invalid types
epicfaace Aug 19, 2019
d172a8e
📜🤖 Added by blurb_it.
blurb-it[bot] Aug 27, 2019
db98184
Update 2019-08-27-01-08-25.bpo-11671.wfJY-D.rst
epicfaace Aug 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
test: update tests to be more consistent with http.client tests
  • Loading branch information
@epicfaace
epicfaace committed Aug 15, 2019
commit 6cd271ae2f0ff5c4962bd0808eda2d8bf4464408
39 changes: 30 additions & 9 deletions Lib/test/test_wsgiref.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,16 +525,37 @@ def testExtras(self):
'\r\n'
)

def testValidateHeaders(self):
validation_cases = (

@vstinner vstinner Aug 27, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add a comment mentioning bpo-11671.

Rather than an hardcoded list, why not building such list using a few invalid characters. For example for header names:

invalid_names = []
invalid_name_chars = ['\r', '\n', '\r\n', ':']
for invalid_chars in invalid_name_chars:
  invalid_names.append(f'{invalid_chars}InvalidName')
  invalid_names.append(f'Invalid{invalid_chars}Name')
  invalid_names.append(f'InvalidName{invalid_chars}')
invalid_names.append(' InvalidName')

Are whitespaces (" ") and colon (":") only invalid at the beginning of the name?

('Invalid\r\nName', 'ValidValue'),
('Invalid\rName', 'ValidValue'),
('Invalid\nName', 'ValidValue'),
('\r\nInvalidName', 'ValidValue'),
('\rInvalidName', 'ValidValue'),
('\nInvalidName', 'ValidValue'),
(' InvalidName', 'ValidValue'),
('\tInvalidName', 'ValidValue'),
('Invalid:Name', 'ValidValue'),
(':InvalidName', 'ValidValue'),
('ValidName', 'Invalid\r\nValue'),
('ValidName', 'Invalid\rValue'),
('ValidName', 'Invalid\nValue'),
('ValidName', 'InvalidValue\r\n'),
('ValidName', 'InvalidValue\r'),
('ValidName', 'InvalidValue\n')
)

@vstinner vstinner Aug 27, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are there already tests for header names which cannot be encoded to ASCII? And tests for values not encodable to Latin1?


def test_add_header_validation(self):

@vstinner vstinner Aug 27, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suggest to merge the two tests a single test, to factorize code.

I suggest to remove the b'InvalidName' and b'InvalidValue' from validation_cases. Maybe create a second list, maybe put it directly in the test method to avoid with self.assertRaises(exception[0]) if len(exception) else self.assertRaisesRegex(ValueError, 'Invalid header'): complexity.

h = Headers([])
self.assertRaises(ValueError, h.add_header, 'foo', 'bar\rbaz: bat')
self.assertRaises(ValueError, h.add_header, 'foo', 'bar\nbaz: bat')
self.assertRaises(ValueError, h.add_header, 'foo: bar', 'baz')
self.assertRaises(ValueError, h.add_header, 'foo:\rbar', 'baz')
self.assertRaises(ValueError, h.add_header, 'foo:\nbar', 'baz')
self.assertRaises(ValueError, h.add_header, 'foo', 'bar\nbaz: bat')
self.assertRaises(ValueError, h.add_header, 'foo', 'bar', baz='bat\rqux: spam')
self.assertRaises(ValueError, h.add_header, 'foo', 'bar', baz='bat\nqux: spam')
for name, value in self.validation_cases:
with self.subTest((name, value)):
with self.assertRaisesRegex(ValueError, 'Invalid header'):
h.add_header(name, value)

def test_initialize_validation(self):
Comment thread
epicfaace marked this conversation as resolved.
for name, value in self.validation_cases:
with self.subTest((name, value)):
with self.assertRaisesRegex(ValueError, 'Invalid header'):
Headers([(name, value)])

class ErrorHandler(BaseCGIHandler):
"""Simple handler subclass for testing BaseHandler"""
Expand Down