bbangert/beaker

Setting `session.domain` loses `secure`, `httponly` and `path` cookie attributes

mgedmin opened this issue · 1 comments

If you set session.domain in a request that has an existing session (i.e. one where SessionMiddleware found an existing Cookie header with a session cookie), Beaker emits a Set-Cookie header that includes Domain= but forgets all about HttpOnly, Secure and Path attributes.

Here's a failing test case: A failing test case is available in pull request #113.

diff --git a/tests/test_cookie_domain_only.py b/tests/test_cookie_domain_only.py
index 96d4241..4c510f4 100644
--- a/tests/test_cookie_domain_only.py
+++ b/tests/test_cookie_domain_only.py
@@ -47,6 +47,22 @@ def test_increment():
     assert 'current value is: 2' in res
 
 
+def test_cookie_attributes_are_preserved():
+    options = {'session.type': 'memory',
+               'session.httponly': True,
+               'session.secure': True,
+               'session.cookie_path': '/app',
+               'session.cookie_domain': 'localhost'}
+    app = TestApp(SessionMiddleware(simple_app, **options))
+    res = app.get('/app', extra_environ=dict(
+        HTTP_COOKIE='beaker.session.id=oldsessid', domain='.hoop.com'))
+    cookie = res.headers['Set-Cookie']
+    assert 'Domain=.hoop.com' in cookie
+    assert 'Path=/app' in cookie
+    assert 'secure' in cookie
+    assert 'httponly' in cookie
+
+
 if __name__ == '__main__':
     from paste import httpserver
     wsgi_app = SessionMiddleware(simple_app, {})

My attempt of fixing this is in PR #114.

amol- commented

Thanks for the effort in debugging this and providing a test!
I felt btw that the core of the issue lied in the fact that whenever a new cookie was generate the values weren't always updated. So more than setting them on load it was a better approach to set them whenever the cookie was generated which lead to 59964e1 which includes the test you provided as insurance that it tackled your problem.