Repository: kumar303/mohawk

Issue:
Commas in header values break parsing
The parser tries to split the entire header `Hawk id="something", key="value"` on commas to separate the fields, thus any field with comma in it will cause breakage.

``` python
def test_ext_with_all_valid_characters(self):
    valid_characters = "!#$%&'()*+,-./:;<=>?@[]^_`{|}~ azAZ09"
    sender = self.Sender(ext=valid_characters)
    self.receive(sender.request_header)
    parsed = parse_authorization_header(sender.request_header)
    eq_(parsed['ext'], valid_characters)
```

Discovered when working on #33 


Discussion:
kumar303: Hmm. I think I agree with https://github.com/hueniverse/hawk/issues/151 i.e. raise an error if an ext contains a backslash. I guess we could do the same thing for escaped quotes? I can't think of any realistic scenarios for using those characters.
thusoy: Yeah, I can’t see any use for it either. I’ll modify this to raise an error on invalid content in the header.


Have a splendid day!

Tarjei



> On Feb 9, 2017, at 08:39, Kumar McMillan <notifications@github.com> wrote:
> 
> Hmm. I think I agree with hueniverse/hawk#151 i.e. raise an error if an ext contains a backslash. I guess we could do the same thing for escaped quotes? I can't think of any realistic scenarios for using those characters.
> 
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub, or mute the thread.
> 


thusoy: Okay, disallowed escaping and introduced a test for duplicate values. Should be good to go now.
thusoy: Yes, yes and yes on cleanup, adding to the changelog and releasing as 1.0.0. See my comments for some discussion points though.
kumar303: Whoops, you noticed the backslash handling for bewits in that other patch (which I forgot about). Could you make backslashes raise an exception there too for consistency? The test for it is here: https://github.com/kumar303/mohawk/blob/master/mohawk/tests.py#L687 I guess both of the `ext` processor functions should be sharing code but if that's a hassle to refactor don't worry about it.
thusoy: Couldn't come up in any other ways to structure the code for clarity, but reworded the comments, let me know if it's still muddy. Didn't have time to look at the bewit stuff this time around, maybe in a couple of days. While the incoming headers are now validated for no escapes I think we still allow it outgoing? Should probably be symmetric.
thusoy: I also added the unparsed header as part of the args to the parse exception raised. I think generally we should aim to keep the failing property in the args and not inside the error message, since it's pretty much inaccessible to developers there, much more useful stuff can be done if it can be extracted, and it's convention in python to add the exceptional value to the exception args. It would still be logged as part of the traceback, and it would be possible to extract programmatically.
kumar303: > While the incoming headers are now validated for no escapes I think we still allow it outgoing? Should probably be symmetric.

I agree, can you file an issue?
kumar303: Don't worry about changing the bewit code in this patch. I filed https://github.com/kumar303/mohawk/issues/41 so that it can be done in a different patch.
thusoy: @kumar303 I added a test case for the exception message, and found an untested path that was failing due to some variables that had been renamed.
thusoy: Make that two untested paths. Both have tests now.
kumar303: Thanks. Looks like this unittest code needs slight adjustments for python 2.6
thusoy: Bah, 2.6... Fixed (hopefully). I'll squash them when you're satisfied to make review easier.
thusoy: @kumar303 We're all green.
thusoy: Rebased and stopped doing anything about backslashes in bewits. Currently we just ignore it (no tests for invalid values), but ideally there should probably be tests and explicit failures for invalid values.
kumar303: Hi. Sorry for the delay. I've been traveling for work but I should be able to do another review shortly.

Patch:
diff --git a/docs/index.rst b/docs/index.rst
index 89c0753..c2970a5 100644
--- a/docs/index.rst
+++ b/docs/index.rst
@@ -106,6 +106,11 @@ TODO
 Changelog
 ---------
 
+- **UNRELEASED**
+  - **Breaking**: Escape characters in header values are no longer allowed,
+    potentially breaking clients that depend on this behavior.
+  - Introduced max limit of 4096 characters in the Authorization header
+
 - **0.3.4** (2017-01-07)
 
   - Fixed ``AttributeError`` exception
diff --git a/mohawk/base.py b/mohawk/base.py
index 1ce1c2b..2c0a9bf 100644
--- a/mohawk/base.py
+++ b/mohawk/base.py
@@ -152,6 +152,7 @@ class Resource:
 
     def __init__(self, **kw):
         self.credentials = kw.pop('credentials')
+        self.credentials['id'] = prepare_header_val(self.credentials['id'])
         self.method = kw.pop('method').upper()
         self.content = kw.pop('content', None)
         self.content_type = kw.pop('content_type', None)
diff --git a/mohawk/bewit.py b/mohawk/bewit.py
index d6dff2c..ed8c3fe 100644
--- a/mohawk/bewit.py
+++ b/mohawk/bewit.py
@@ -7,7 +7,8 @@ import six
 
 from .base import Resource
 from .util import (calculate_mac,
-                   utc_now)
+                   utc_now,
+                   validate_header_attr)
 from .exc import (CredentialsLookupError,
                   InvalidBewit,
                   MacMismatch,
@@ -40,25 +41,15 @@ def get_bewit(resource):
     if resource.ext is None:
         ext = ''
     else:
+        validate_header_attr(resource.ext, name='ext')
         ext = resource.ext
 
-    # Strip out \ from the client id
-    # since that can break parsing the response
-    # NB that the canonical implementation does not do this as of
-    # Oct 28, 2015, so this could break compat.
-    # We can leave \ in ext since validators can limit how many \ they split
-    # on (although again, the canonical implementation does not do this)
-    client_id = six.text_type(resource.credentials['id'])
-    if "\\" in client_id:
-        log.warning("Stripping backslash character(s) '\\' from client_id")
-        client_id = client_id.replace("\\", "")
-
     # b64encode works only with bytes in python3, but all of our parameters are
     # in unicode, so we need to encode them. The cleanest way to do this that
     # works in both python 2 and 3 is to use string formatting to get a
     # unicode string, and then explicitly encode it to bytes.
     inner_bewit = u"{id}\\{exp}\\{mac}\\{ext}".format(
-        id=client_id,
+        id=resource.credentials['id'],
         exp=resource.timestamp,
         mac=mac,
         ext=ext,
@@ -83,10 +74,10 @@ def parse_bewit(bewit):
     :type bewit: str
     """
     decoded_bewit = b64decode(bewit).decode('ascii')
-    bewit_parts = decoded_bewit.split("\\", 3)
+    bewit_parts = decoded_bewit.split("\\")
     if len(bewit_parts) != 4:
         raise InvalidBewit('Expected 4 parts to bewit: %s' % decoded_bewit)
-    return bewittuple(*decoded_bewit.split("\\", 3))
+    return bewittuple(*bewit_parts)
 
 
 def strip_bewit(url):
diff --git a/mohawk/util.py b/mohawk/util.py
index 46a28e9..bb8a14e 100644
--- a/mohawk/util.py
+++ b/mohawk/util.py
@@ -19,6 +19,8 @@ from .exc import (
 
 
 HAWK_VER = 1
+HAWK_HEADER_RE = re.compile(r'(?P<key>\w+)=\"(?P<value>[^\"\\]*)\"\s*(?:,\s*|$)')
+MAX_LENGTH = 4096
 log = logging.getLogger(__name__)
 allowable_header_keys = set(['id', 'ts', 'tsm', 'nonce', 'hash',
                              'error', 'ext', 'mac', 'app', 'dlg'])
@@ -149,45 +151,42 @@ def parse_authorization_header(auth_header):
         'Hawk id="dh37fgj492je", ts="1367076201", nonce="NPHgnG", ext="and
         welcome!", mac="CeWHy4d9kbLGhDlkyw2Nh3PJ7SDOdZDa267KH4ZaNMY="'
     """
-    attributes = {}
+    if len(auth_header) > MAX_LENGTH:
+        raise BadHeaderValue('Header exceeds maximum length of {max_length}'.format(
+            max_length=MAX_LENGTH))
 
     # Make sure we have a unicode object for consistency.
     if isinstance(auth_header, six.binary_type):
         auth_header = auth_header.decode('utf8')
 
-    parts = auth_header.split(',')
-    auth_scheme_parts = parts[0].split(' ')
-    if 'hawk' != auth_scheme_parts[0].lower():
+    scheme, attributes_string = auth_header.split(' ', 1)
+
+    if scheme.lower() != 'hawk':
         raise HawkFail("Unknown scheme '{scheme}' when parsing header"
-                       .format(scheme=auth_scheme_parts[0].lower()))
+                       .format(scheme=scheme))
+
 
-    # Replace 'Hawk key: value' with 'key: value'
-    # which matches the rest of parts
-    parts[0] = auth_scheme_parts[1]
+    attributes = {}
 
-    for part in parts:
-        attr_parts = part.split('=')
-        key = attr_parts[0].strip()
+    def replace_attribute(match):
+        """Extract the next key="value"-pair in the header."""
+        key = match.group('key')
+        value = match.group('value')
         if key not in allowable_header_keys:
             raise HawkFail("Unknown Hawk key '{key}' when parsing header"
                            .format(key=key))
-
-        if len(attr_parts) > 2:
-            attr_parts[1] = '='.join(attr_parts[1:])
-
-        # Chop of quotation marks
-        value = attr_parts[1]
-
-        if attr_parts[1].find('"') == 0:
-            value = attr_parts[1][1:]
-
-        if value.find('"') > -1:
-            value = value[0:-1]
-
         validate_header_attr(value, name=key)
-        value = unescape_header_attr(value)
+        if key in attributes:
+            raise BadHeaderValue('Duplicate key in header: {key}'.format(key=key))
         attributes[key] = value
 
+    # Iterate over all the key="value"-pairs in the header, replace them with
+    # an empty string, and store the extracted attribute in the attributes
+    # dict. Correctly formed headers will then leave nothing unparsed ('').
+    unparsed_header = HAWK_HEADER_RE.sub(replace_attribute, attributes_string)
+    if unparsed_header != '':
+        raise BadHeaderValue("Couldn't parse Hawk header", unparsed_header)
+
     log.debug('parsed Hawk header: {header} into: \n{parsed}'
               .format(header=auth_header, parsed=pprint.pformat(attributes)))
     return attributes
@@ -222,7 +221,7 @@ def utc_now(offset_in_seconds=0.0):
 # Allowed value characters:
 # !#$%&'()*+,-./:;<=>?@[]^_`{|}~ and space, a-z, A-Z, 0-9, \, "
 _header_attribute_chars = re.compile(
-    r"^[ a-zA-Z0-9_\!#\$%&'\(\)\*\+,\-\./\:;<\=>\?@\[\]\^`\{\|\}~\"\\]*$")
+    r"^[ a-zA-Z0-9_\!#\$%&'\(\)\*\+,\-\./\:;<\=>\?@\[\]\^`\{\|\}~]*$")
 
 
 def validate_header_attr(val, name=None):
@@ -232,36 +231,14 @@ def validate_header_attr(val, name=None):
                              .format(name=name or '?', val=repr(val)))
 
 
-def escape_header_attr(val):
-
-    # Ensure we are working with Unicode for consistency.
-    if isinstance(val, six.binary_type):
-        val = val.decode('utf8')
-
-    # Escape quotes and slash like the hawk reference code.
-    val = val.replace('\\', '\\\\')
-    val = val.replace('"', '\\"')
-    val = val.replace('\n', '\\n')
-    return val
-
-
-def unescape_header_attr(val):
-    # Un-do the hawk escaping.
-    val = val.replace('\\n', '\n')
-    val = val.replace('\\\\', '\\').replace('\\"', '"')
-    return val
-
-
 def prepare_header_val(val):
-    val = escape_header_attr(val)
+    if isinstance(val, six.binary_type):
+        val = val.decode('utf-8')
     validate_header_attr(val)
     return val
 
 
 def normalize_header_attr(val):
-    if not val:
-        val = ''
-
-    # Normalize like the hawk reference code.
-    val = escape_header_attr(val)
+    if isinstance(val, six.binary_type):
+        return val.decode('utf-8')
     return val


Test patch:
diff --git a/mohawk/tests.py b/mohawk/tests.py
index eedafd8..52b5226 100644
--- a/mohawk/tests.py
+++ b/mohawk/tests.py
@@ -12,6 +12,7 @@ from .base import Resource
 from .exc import (AlreadyProcessed,
                   BadHeaderValue,
                   CredentialsLookupError,
+                  HawkFail,
                   InvalidCredentials,
                   MacMismatch,
                   MisComputedContentHash,
@@ -313,17 +314,21 @@ class TestSender(Base):
         header = sn.request_header.replace('my external data', 'TAMPERED')
         self.receive(header)
 
+    @raises(BadHeaderValue)
+    def test_duplicate_keys(self):
+        sn = self.Sender(ext='someext')
+        header = sn.request_header + ', ext="otherext"'
+        self.receive(header)
+
+    @raises(BadHeaderValue)
     def test_ext_with_quotes(self):
         sn = self.Sender(ext='quotes=""')
         self.receive(sn.request_header)
-        parsed = parse_authorization_header(sn.request_header)
-        eq_(parsed['ext'], 'quotes=""')
 
+    @raises(BadHeaderValue)
     def test_ext_with_new_line(self):
         sn = self.Sender(ext="new line \n in the middle")
         self.receive(sn.request_header)
-        parsed = parse_authorization_header(sn.request_header)
-        eq_(parsed['ext'], "new line \n in the middle")
 
     def test_ext_with_equality_sign(self):
         sn = self.Sender(ext="foo=bar&foo2=bar2;foo3=bar3")
@@ -331,19 +336,47 @@ class TestSender(Base):
         parsed = parse_authorization_header(sn.request_header)
         eq_(parsed['ext'], "foo=bar&foo2=bar2;foo3=bar3")
 
+    @raises(HawkFail)
+    def test_non_hawk_scheme(self):
+        parse_authorization_header('Basic user:base64pw')
+
+    @raises(HawkFail)
+    def test_invalid_key(self):
+        parse_authorization_header('Hawk mac="validmac" unknownkey="value"')
+
+    def test_ext_with_all_valid_characters(self):
+        valid_characters = "!#$%&'()*+,-./:;<=>?@[]^_`{|}~ azAZ09_"
+        sender = self.Sender(ext=valid_characters)
+        parsed = parse_authorization_header(sender.request_header)
+        eq_(parsed['ext'], valid_characters)
+
     @raises(BadHeaderValue)
     def test_ext_with_illegal_chars(self):
         self.Sender(ext="something like \t is illegal")
 
+    def test_unparseable_header(self):
+        try:
+            parse_authorization_header('Hawk mac="somemac", unparseable')
+        except BadHeaderValue as exc:
+            error_msg = str(exc)
+            self.assertTrue("Couldn't parse Hawk header" in error_msg)
+            self.assertTrue("unparseable" in error_msg)
+        else:
+            self.fail('should raise')
+
     @raises(BadHeaderValue)
     def test_ext_with_illegal_unicode(self):
         self.Sender(ext=u'Ivan Kristi\u0107')
 
+    @raises(BadHeaderValue)
+    def test_too_long_header(self):
+        sn = self.Sender(ext='a'*5000)
+        self.receive(sn.request_header)
+
     @raises(BadHeaderValue)
     def test_ext_with_illegal_utf8(self):
         # This isn't allowed because the escaped byte chars are out of
-        # range. It's a little odd but this is what the Node lib does
-        # implicitly with its regex.
+        # range.
         self.Sender(ext=u'Ivan Kristi\u0107'.encode('utf8'))
 
     def test_app_ok(self):
@@ -689,19 +722,24 @@ class TestBewit(Base):
         expected = '123456\\1356420707\\kscxwNR2tJpP1T1zDLNPbB5UiKIU9tOSJXTUdG7X9h8=\\xandyandz'
         eq_(b64decode(bewit).decode('ascii'), expected)
 
-    def test_bewit_with_ext_and_backslashes(self):
-        credentials = self.credentials
-        credentials['id'] = '123\\456'
+    @raises(BadHeaderValue)
+    def test_bewit_with_invalid_ext(self):
         res = Resource(url='https://example.com/somewhere/over/the/rainbow',
                        method='GET', credentials=self.credentials,
                        timestamp=1356420407 + 300,
                        nonce='',
-                       ext='xand\\yandz'
-                       )
-        bewit = get_bewit(res)
+                       ext='xand\\yandz')
+        get_bewit(res)
 
-        expected = '123456\\1356420707\\b82LLIxG5UDkaChLU953mC+SMrbniV1sb8KiZi9cSsc=\\xand\\yandz'
-        eq_(b64decode(bewit).decode('ascii'), expected)
+    @raises(BadHeaderValue)
+    def test_bewit_with_backslashes_in_id(self):
+        credentials = self.credentials
+        credentials['id'] = '123\\456'
+        res = Resource(url='https://example.com/somewhere/over/the/rainbow',
+                       method='GET', credentials=self.credentials,
+                       timestamp=1356420407 + 300,
+                       nonce='')
+        get_bewit(res)
 
     def test_bewit_with_port(self):
         res = Resource(url='https://example.com:8080/somewhere/over/the/rainbow',
@@ -759,14 +797,11 @@ class TestBewit(Base):
         self.assertEqual(bewit.mac, 'IGYmLgIqLrCe8CxvKPs4JlWIA+UjWJJouwgARiVhCAg=')
         self.assertEqual(bewit.ext, 'xandyandz')
 
+    @raises(InvalidBewit)
     def test_parse_bewit_with_ext_and_backslashes(self):
         bewit = b'123456\\1356420707\\IGYmLgIqLrCe8CxvKPs4JlWIA+UjWJJouwgARiVhCAg=\\xand\\yandz'
         bewit = urlsafe_b64encode(bewit).decode('ascii')
-        bewit = parse_bewit(bewit)
-        self.assertEqual(bewit.id, '123456')
-        self.assertEqual(bewit.expiration, '1356420707')
-        self.assertEqual(bewit.mac, 'IGYmLgIqLrCe8CxvKPs4JlWIA+UjWJJouwgARiVhCAg=')
-        self.assertEqual(bewit.ext, 'xand\\yandz')
+        parse_bewit(bewit)
 
     @raises(InvalidBewit)
     def test_parse_invalid_bewit_with_only_one_part(self):
@@ -798,6 +833,7 @@ class TestBewit(Base):
         })
         self.assertTrue(check_bewit(url, credential_lookup=credential_lookup, now=1356420407 + 10))
 
+    @raises(InvalidBewit)
     def test_validate_bewit_with_ext_and_backslashes(self):
         bewit = b'123456\\1356420707\\b82LLIxG5UDkaChLU953mC+SMrbniV1sb8KiZi9cSsc=\\xand\\yandz'
         bewit = urlsafe_b64encode(bewit).decode('ascii')
@@ -805,7 +841,7 @@ class TestBewit(Base):
         credential_lookup = self.make_credential_lookup({
             self.credentials['id']: self.credentials,
         })
-        self.assertTrue(check_bewit(url, credential_lookup=credential_lookup, now=1356420407 + 10))
+        check_bewit(url, credential_lookup=credential_lookup, now=1356420407 + 10)
 
     @raises(TokenExpired)
     def test_validate_expired_bewit(self):