Repository: aio-libs/aiohttp-session

Issue:
Error 500 when switching from EncryptedCookieStorage to NaClCookieStorage
```Traceback (most recent call last):
  File "[..]/site-packages/aiohttp/web_protocol.py", line 390, in start
    resp = await self._request_handler(request)
  File "[..]/site-packages/aiohttp/web_app.py", line 366, in _handle
    resp = await handler(request)
  File "[..]/site-packages/aiohttp/web_middlewares.py", line 106, in impl
    return await handler(request)
  File "[..]/site-packages/aiohttp_session/__init__.py", line 149, in factory
    response = await handler(request)
  File "/Users/az02096/dev/nestor/elasticsearch_authorizations/elasticsearch_authorizations/app.py", line 122, in index_handler
    session = await get_session(request)
  File "[..]/site-packages/aiohttp_session/__init__.py", line 113, in get_session
    session = await storage.load_session(request)
  File "[..]/site-packages/aiohttp_session/nacl_storage.py", line 32, in load_session
    encoder=Base64Encoder).decode('utf-8')
  File "[..]/site-packages/nacl/secret.py", line 112, in decrypt
    ciphertext = encoder.decode(ciphertext)
  File "[..]/site-packages/nacl/encoding.py", line 73, in decode
    return base64.b64decode(data)
  File "[..]/base64.py", line 87, in b64decode
    return binascii.a2b_base64(s)
binascii.Error: Incorrect padding
INFO:aiohttp.access:::1 [10/Sep/2018:14:27:23 +0000] "GET / HTTP/1.1" 500 330 "http://localhost:8000/" 
```

Same issue when changing the secret.

probably we need to port this patch to NaClCookieStorage:
https://github.com/aio-libs/aiohttp-session/commit/0741f5a5380094ff6a970194870b2c41451747cf

Not sure what is the benefit of NaClCookieStorage over  EncryptedCookieStorage.


Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/aio-libs/aiohttp-session/pull/317?src=pr&el=h1) Report
> Merging [#317](https://codecov.io/gh/aio-libs/aiohttp-session/pull/317?src=pr&el=desc) into [master](https://codecov.io/gh/aio-libs/aiohttp-session/commit/cecdc7282e3c2bdd0fd4931ce2a562c8812a82a6?src=pr&el=desc) will **increase** coverage by `0.12%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/aio-libs/aiohttp-session/pull/317/graphs/tree.svg?width=650&token=M1jTmwOZ5C&height=150&src=pr)](https://codecov.io/gh/aio-libs/aiohttp-session/pull/317?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##           master    #317      +/-   ##
=========================================
+ Coverage   97.08%   97.2%   +0.12%     
=========================================
  Files           4       4              
  Lines         206     215       +9     
  Branches       25      25              
=========================================
+ Hits          200     209       +9     
  Misses          4       4              
  Partials        2       2
```


| [Impacted Files](https://codecov.io/gh/aio-libs/aiohttp-session/pull/317?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [aiohttp\_session/nacl\_storage.py](https://codecov.io/gh/aio-libs/aiohttp-session/pull/317/diff?src=pr&el=tree#diff-YWlvaHR0cF9zZXNzaW9uL25hY2xfc3RvcmFnZS5weQ==) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/aio-libs/aiohttp-session/pull/317?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/aio-libs/aiohttp-session/pull/317?src=pr&el=footer). Last update [cecdc72...7240128](https://codecov.io/gh/aio-libs/aiohttp-session/pull/317?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/aiohttp_session/nacl_storage.py b/aiohttp_session/nacl_storage.py
index eb7105d..c855ff8 100644
--- a/aiohttp_session/nacl_storage.py
+++ b/aiohttp_session/nacl_storage.py
@@ -1,10 +1,13 @@
+import binascii
 import json
 
 import nacl.secret
 import nacl.utils
+import nacl.exceptions
 from nacl.encoding import Base64Encoder
 
 from . import AbstractStorage, Session
+from .log import log
 
 
 class NaClCookieStorage(AbstractStorage):
@@ -22,16 +25,26 @@ class NaClCookieStorage(AbstractStorage):
 
         self._secretbox = nacl.secret.SecretBox(secret_key)
 
+    def empty_session(self):
+        return Session(None, data=None, new=True, max_age=self.max_age)
+
     async def load_session(self, request):
         cookie = self.load_cookie(request)
         if cookie is None:
-            return Session(None, data=None, new=True, max_age=self.max_age)
+            return self.empty_session()
         else:
-            data = self._decoder(
-                self._secretbox.decrypt(cookie.encode('utf-8'),
-                                        encoder=Base64Encoder).decode('utf-8')
-            )
-            return Session(None, data=data, new=False, max_age=self.max_age)
+            try:
+                data = self._decoder(
+                    self._secretbox.decrypt(
+                        cookie.encode('utf-8'),
+                        encoder=Base64Encoder).decode('utf-8')
+                )
+                return Session(None, data=data, new=False,
+                               max_age=self.max_age)
+            except (binascii.Error, nacl.exceptions.CryptoError):
+                log.warning("Cannot decrypt cookie value, "
+                            "create a new fresh session")
+                return self.empty_session()
 
     async def save_session(self, request, response, session):
         if session.empty:
diff --git a/docs/reference.rst b/docs/reference.rst
index 267c882..a22970b 100644
--- a/docs/reference.rst
+++ b/docs/reference.rst
@@ -348,7 +348,7 @@ To use the storage you should push it into
 :func:`~aiohttp_session.session_middleware`::
 
    app = aiohttp.web.Application(middlewares=[
-       aiohttp_session.cookie_storage.NaClCookieStorage(
+       aiohttp_session.nacl_storage.NaClCookieStorage(
            b'Thirty  two  length  bytes  key.'])
 
 .. class:: NaClCookieStorage(secret_key, *, \


Test patch:
diff --git a/tests/test_nacl_storage.py b/tests/test_nacl_storage.py
index 66bd814..836bfe6 100644
--- a/tests/test_nacl_storage.py
+++ b/tests/test_nacl_storage.py
@@ -179,3 +179,36 @@ async def test_load_session_dont_load_expired_session(aiohttp_client,
 
     resp = await client.get('/?exp=yes')
     assert resp.status == 200
+
+
+async def test_load_corrupted_session(aiohttp_client, key):
+
+    async def handler(request):
+        session = await get_session(request)
+        assert isinstance(session, Session)
+        assert session.new
+        assert {} == session
+        return web.Response(body=b'OK')
+
+    client = await aiohttp_client(create_app(handler, key))
+    client.session.cookie_jar.update_cookies({'AIOHTTP_SESSION': 'bad key'})
+    resp = await client.get('/')
+    assert resp.status == 200
+
+
+async def test_load_session_different_key(aiohttp_client, key):
+
+    async def handler(request):
+        session = await get_session(request)
+        assert isinstance(session, Session)
+        assert session.new
+        assert {} == session
+        return web.Response(body=b'OK')
+
+    client = await aiohttp_client(create_app(handler, key))
+    # create another box with another key
+    key = nacl.utils.random(nacl.secret.SecretBox.KEY_SIZE)
+    secretbox = nacl.secret.SecretBox(key)
+    make_cookie(client, secretbox, {'a': 1, 'b': 12})
+    resp = await client.get('/')
+    assert resp.status == 200