Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
34 changes: 34 additions & 0 deletions Lib/test/test_wsgiref.py
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,40 @@ def testExtras(self):
'\r\n'
)

validation_cases = (
Copy link
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'),
(b'InvalidName', 'ValidValue', AssertionError),
('ValidName', b'InvalidValue', AssertionError)
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to get a TypeError rather than an AssertionError.

)
Copy link
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):
Copy link
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([])
for name, value, *exception in self.validation_cases:
with self.subTest((name, value, exception)):
with self.assertRaises(exception[0]) if len(exception) else self.assertRaisesRegex(ValueError, 'Invalid header'):
h.add_header(name, value)

def test_initialize_validation(self):
for name, value, *exception in self.validation_cases:
with self.subTest((name, value, exception)):
with self.assertRaises(exception[0]) if len(exception) else self.assertRaisesRegex(ValueError, 'Invalid header'):
Headers([(name, value)])

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

Expand Down
20 changes: 14 additions & 6 deletions Lib/wsgiref/headers.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
import re
tspecials = re.compile(r'[ \(\)<>@,;:\\"/\[\]\?=]')

from http.client import _is_legal_header_name, _is_illegal_header_value

def _formatparam(param, value=None, quote=1):
"""Convenience function to format and return a key=value pair.

Expand All @@ -32,7 +34,9 @@ def __init__(self, headers=None):
headers = headers if headers is not None else []
if type(headers) is not list:
raise TypeError("Headers must be a list of name/value tuples")
self._headers = headers
self._headers = []
for header, value in headers:
self.add_header(header, value)
if __debug__:
for k, v in headers:
self._convert_string_type(k)
Expand All @@ -52,8 +56,7 @@ def __len__(self):
def __setitem__(self, name, val):
"""Set the value of a header."""
del self[name]
self._headers.append(
(self._convert_string_type(name), self._convert_string_type(val)))
self.add_header(name, val)

def __delitem__(self,name):
"""Delete all occurrences of a header, if present.
Expand Down Expand Up @@ -148,8 +151,7 @@ def setdefault(self,name,value):
and value 'value'."""
result = self.get(name)
if result is None:
self._headers.append((self._convert_string_type(name),
self._convert_string_type(value)))
self.add_header(name, value)
return value
else:
return result
Expand Down Expand Up @@ -181,4 +183,10 @@ def add_header(self, _name, _value, **_params):
else:
v = self._convert_string_type(v)
parts.append(_formatparam(k.replace('_', '-'), v))
self._headers.append((self._convert_string_type(_name), "; ".join(parts)))
header = self._convert_string_type(_name)
Copy link
Member

Choose a reason for hiding this comment

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

Can you rename the variable to header_name (or maybe name, I didn't check if it's already used)?

value = "; ".join(parts)
if not _is_legal_header_name(header.encode('ascii')):
raise ValueError('Invalid header name %r' % (header,))
if _is_illegal_header_value(value.encode('ascii')):
Copy link
Member

Choose a reason for hiding this comment

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

http/client.py encodes the value to Latin1, not ASCII

raise ValueError('Invalid header value %r' % (value,))
self._headers.append((header, value))
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add validation in wsgiref.headers.Headers to disallow invalid characters in header names and values. Patch by Ashwin Ramaswami, initial patch by Devin Cook.