launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-improve-framing-checks into lp:launchpad.

Commit message:
Improve macaroon framing checks.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-improve-framing-checks/+merge/296312

Improve macaroon framing checks.

This fixes a couple of problems brought up in passing in https://github.com/ubuntu-core/snapcraft/pull/532.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-improve-framing-checks into lp:launchpad.

=== modified file 'lib/lp/snappy/model/snapstoreclient.py'
--- lib/lp/snappy/model/snapstoreclient.py	2016-05-31 10:54:32 +0000
+++ lib/lp/snappy/model/snapstoreclient.py	2016-06-02 11:22:22 +0000
@@ -52,6 +52,10 @@
         return data
 
 
+class InvalidStoreSecretsError(Exception):
+    pass
+
+
 class MacaroonAuth(requests.auth.AuthBase):
     """Attaches macaroon authentication to a given Request object."""
 
@@ -65,8 +69,12 @@
     @classmethod
     def _makeAuthParam(cls, key, value):
         # Check framing.
-        assert set(key).issubset(cls.allowed_chars)
-        assert set(value).issubset(cls.allowed_chars)
+        if not set(key).issubset(cls.allowed_chars):
+            raise InvalidStoreSecretsError(
+                "Key contains unsafe characters: %r" % key)
+        if not set(value).issubset(cls.allowed_chars):
+            raise InvalidStoreSecretsError(
+                "Value contains unsafe characters: %r" % key)
         return '%s="%s"' % (key, value)
 
     @property

=== modified file 'lib/lp/snappy/tests/test_snapstoreclient.py'
--- lib/lp/snappy/tests/test_snapstoreclient.py	2016-05-31 10:54:32 +0000
+++ lib/lp/snappy/tests/test_snapstoreclient.py	2016-06-02 11:22:22 +0000
@@ -45,7 +45,10 @@
     BadRequestPackageUploadResponse,
     ISnapStoreClient,
     )
-from lp.snappy.model.snapstoreclient import MacaroonAuth
+from lp.snappy.model.snapstoreclient import (
+    InvalidStoreSecretsError,
+    MacaroonAuth,
+    )
 from lp.testing import (
     TestCase,
     TestCaseWithFactory,
@@ -95,7 +98,16 @@
 
     def test_bad_framing(self):
         r = Request()
-        self.assertRaises(AssertionError, MacaroonAuth('ev"il', 'wic"ked'), r)
+        self.assertRaises(
+            InvalidStoreSecretsError, MacaroonAuth('ev"il', 'wic"ked'), r)
+        # Test _makeAuthParam's behaviour directly in case somebody somehow
+        # convinces Macaroon.serialize to emit data that breaks framing.
+        self.assertRaises(
+            InvalidStoreSecretsError, MacaroonAuth._makeAuthParam,
+            'ev"il', 'good')
+        self.assertRaises(
+            InvalidStoreSecretsError, MacaroonAuth._makeAuthParam,
+            'good', 'ev"il')
 
 
 class RequestMatches(Matcher):