559131 - [l10n_site] make session cookie httponly

Status: VERIFIED FIXED

(Webtools Graveyard :: Elmo, defect, P1)

Description

[Axel Hecht [:Pike]]

There's an upstream ticket to make the session cookie httponly, http://code.djangoproject.com/ticket/3304. There's also a snippet to add this as middleware, http://www.djangosnippets.org/snippets/1983/

Comment 1

[Axel Hecht [:Pike]]

Would playdoh give us this by default?

Comment 2

[Fred Wenzel [:wenzel]]

Yes. Playdoh sets all cookies with httponly and secure flags (if served via HTTPS) by default, unless you opt out.

(for reference:
https://github.com/jsocol/commonware/blob/master/commonware/response/cookies/monkeypatch.py )

Comment 3

[Peter Bengtsson [:peterbe]]

Having moved elmo into Playdoh, this will mean that once we make the switch on the staging and production site people's old insecure cookies will be expired thus, I guess, leaving them to just log in again.

Comment 4

[Fred Wenzel [:wenzel]]

To be future-proof, it looks like you should also set

SESSION_COOKIE_SECURE and SESSION_COOKIE_HTTPONLY in settings.

https://github.com/django/django/blob/master/django/contrib/sessions/middleware.py#L41

Comment 5

[Peter Bengtsson [:peterbe]]

(In reply to comment #4)
> To be future-proof, it looks like you should also set
> 
> SESSION_COOKIE_SECURE and SESSION_COOKIE_HTTPONLY in settings.
> 
> https://github.com/django/django/blob/master/django/contrib/sessions/middleware.py#L41

The secure one is only possible if the whole site is on HTTPS.
I'm putting a hint for that in settings/local.py-dist so local devs can use sessions without having to run https://localhost:8000/

Comment 6

[Peter Bengtsson [:peterbe]]

Attachment: Secure session cookies only

New default that session cookies are all secure and http-only. Tests there to prove it. 

Important: now settings/base.py demands that all session cookies are secure it becomes impossible to log in when running via http://localhost:8000/. Solution is to run https locally or to set SESSION_COOKIE_SECURE=False in settings/local.py

Comment 7

[Peter Bengtsson [:peterbe]]

Comment on attachment 529996 [details] [diff] [review]
Secure session cookies only

dragging stas into it too.

Comment 8

[Axel Hecht [:Pike]]

Comment on attachment 529996 [details] [diff] [review]
Secure session cookies only

Review of attachment 529996 [details] [diff] [review]:

::: apps/homepage/tests.py
@@ +72,5 @@
+        self.assertEqual(response.status_code, 302)
+        self.assertTrue(response['Location'].endswith('/foo'))
+
+        # if this fails it's because settings.SESSION_COOKIE_SECURE isn't true
+        assert settings.SESSION_COOKIE_SECURE

I don't think I like the mix between unittest testing and plain python asserts here. I'd rather see this do an assertTrue or something.

::: settings/base.py
@@ +253,4 @@
 ## Tests
 #TEST_RUNNER = 'test_utils.runner.RadicalTestSuiteRunner'
 #TEST_RUNNER = 'django.test.simple.DjangoTestSuiteRunner'
 TEST_RUNNER = 'test_runner.PatchedNoseTestSuiteRunner'

The patching of the test_runner is in a different patch?

Comment 9

[Peter Bengtsson [:peterbe]]

(In reply to comment #8)
> Comment on attachment 529996 [details] [diff] [review] [review]
> Secure session cookies only
> 
> Review of attachment 529996 [details] [diff] [review] [review]:
> 
> ::: apps/homepage/tests.py
> @@ +72,5 @@
> +        self.assertEqual(response.status_code, 302)
> +        self.assertTrue(response['Location'].endswith('/foo'))
> +
> +        # if this fails it's because settings.SESSION_COOKIE_SECURE isn't true
> +        assert settings.SESSION_COOKIE_SECURE
> 
> I don't think I like the mix between unittest testing and plain python asserts
> here. I'd rather see this do an assertTrue or something.
> 

I see your point. However, the plain assert is there merely to tell the reader that the tests expect certain things to be set up accordingly. It's not a test of functionality really. SESSION_COOKIE_SECURE is always set to True in the testcase's setUp()


> ::: settings/base.py
> @@ +253,4 @@
>  ## Tests
>  #TEST_RUNNER = 'test_utils.runner.RadicalTestSuiteRunner'
>  #TEST_RUNNER = 'django.test.simple.DjangoTestSuiteRunner'
>  TEST_RUNNER = 'test_runner.PatchedNoseTestSuiteRunner'
> 
> The patching of the test_runner is in a different patch?

I have no idea why that became part of changeset. It has always been PatchedNoseTestSuiteRunner.

Comment 10

[Staś Małolepszy :stas]

Comment on attachment 529996 [details] [diff] [review]
Secure session cookies only

Review of attachment 529996 [details] [diff] [review]:

::: apps/homepage/tests.py
@@ +72,5 @@
+        self.assertEqual(response.status_code, 302)
+        self.assertTrue(response['Location'].endswith('/foo'))
+
+        # if this fails it's because settings.SESSION_COOKIE_SECURE isn't true
+        assert settings.SESSION_COOKIE_SECURE

I agree with Peter that this isn't a part of the tast case, and thus the assert makes sense to me.

::: settings/local.py-dist
@@ +33,5 @@
 DEBUG = TEMPLATE_DEBUG = True
 #TEMPLATE_STRING_IF_INVALID = '!{ %s }'
 #REPOSITORY_BASE = '/home/peterbe/dev/MOZILLA/ELMO/repos'
+
+SESSION_COOKIE_SECURE = False # so I can use http://localhost:8000/

Can you group this and DEBUG under a single comment saying that these settings are to be used only for development purposes and should be removed from local.py on production?

Comment 11

[Staś Małolepszy :stas]

Comment on attachment 529996 [details] [diff] [review]
Secure session cookies only

Review of attachment 529996 [details] [diff] [review]:

I meant to r+ this in my previous comment.  r=me with the nit about the comment in local.py-dist.

Comment 12

[Axel Hecht [:Pike]]

Comment on attachment 529996 [details] [diff] [review]
Secure session cookies only

yep, can land with stas' comments.

Comment 13

[Peter Bengtsson [:peterbe]]

This is now merged into origin/develop. Thanks guys.

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

I've verified the sessionid is for "Encrypted connections only", but how do I verify that they're http-only?  I'm using https://l10n-dev-sj.mozilla.org/

Comment 15

[Peter Bengtsson [:peterbe]]

(In reply to comment #14)
> I've verified the sessionid is for "Encrypted connections only", but how do
> I verify that they're http-only?  I'm using https://l10n-dev-sj.mozilla.org/

Open Firebug, click "Login", type in username + password and check out the headers of the POST. It should look something like this::

 Set-Cookie	sessionid=414...ff; expires=Thu, 07-Jul-2011 18:24:08 GMT; httponly; Max-Age=1209600; Path=/; secure

Comment 16

[Stephen Donner [:stephend] Not actively reading bugmail]

Ah, yes; thanks:

Location:https://l10n-dev-sj.mozilla.org/accounts/user.htmlServer:Apache/2.2.14 (Ubuntu)Set-Cookie:sessionid=[nulledOut]; expires=Thu, 07-Jul-2011 19:35:07 GMT; httponly; Max-Age=1209600; Path=/; secure

Verified FIXED!