[PATCH] middleware: use secure cookies over secure connections

[Mads Kiilerich]

On 03/04/2015 11:58 PM, Andrew Shadura wrote:
> # HG changeset patch
> # User Andrew Shadura <andrew at shadura.me>
> # Date 1425509877 -3600
> #      Wed Mar 04 23:57:57 2015 +0100
> # Node ID 6e0cecf00cdad2d108caca97c4cd9dece0333d73
> # Parent  599fba9967a4981e8c401c39521a52635dd93380
> middleware: use secure cookies over secure connections
>
> Use our own wrapper around Beaker's SessionMiddleware class to
> give secure cookies over HTTPS connections.

It would also be nice to have a clear statement of what problem this is 
solving. There was no way to flag cookies as "secure" when using https? 
In which scenarios would that be a problem ... and how big?

I assume it would be better to use plain Beaker for this ... but I 
assume you have tried and researched and concluded that this was the 
best way to do it? Please you share your findings - perhaps as a comment 
in sessionmiddleware.py or in the commit message. Is it a bug or 
not-yet-implemented feature or philosophical disagreement?

> diff --git a/kallithea/config/middleware.py b/kallithea/config/middleware.py
> --- a/kallithea/config/middleware.py
> +++ b/kallithea/config/middleware.py
> @@ -15,7 +15,6 @@
>       Pylons middleware initialization
>   """
>   
> -from beaker.middleware import SessionMiddleware
>   from routes.middleware import RoutesMiddleware
>   from paste.cascade import Cascade
>   from paste.registry import RegistryManager
> @@ -29,6 +28,7 @@ from pylons.wsgiapp import PylonsApp
>   from kallithea.lib.middleware.simplehg import SimpleHg
>   from kallithea.lib.middleware.simplegit import SimpleGit
>   from kallithea.lib.middleware.https_fixup import HttpsFixup
> +from kallithea.lib.middleware.sessionmiddleware import SessionMiddleware
>   from kallithea.config.environment import load_environment
>   from kallithea.lib.middleware.wrapper import RequestWrapper
>   
> diff --git a/kallithea/lib/middleware/sessionmiddleware.py b/kallithea/lib/middleware/sessionmiddleware.py
> new file mode 100644
> --- /dev/null
> +++ b/kallithea/lib/middleware/sessionmiddleware.py
> @@ -0,0 +1,78 @@
> +# -*- coding: utf-8 -*-
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +"""
> +kallithea.lib.middleware.sessionmiddleware
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +session management middleware
> +
> +This file overrides Beaker's built-in SessionMiddleware
> +class to automagically use secure cookies over HTTPS.
> +
> +Original Beaker SessionMiddleware class written by Ben Bangert
> +
> +:created_on: March 04, 2015
> +:author: andrewsh
> +:copyright: (c) 2015 Andrew Shadura
> +:license: GPLv3, see LICENSE.md for more details.
> +"""

Unless it already has been fixed / contributed upstream, the license of 
this could perhaps be made compatible with upstream so they can take it 
back?

> +from beaker.session import SessionObject
> +from beaker.middleware import SessionMiddleware as BeakerSessionMiddleware
> +
> +class SessionMiddleware(BeakerSessionMiddleware):
> +    def __init__(self, wrap_app, config=None, environ_key='beaker.session',
> +                **kwargs):
> +        """
> +        Initialise the session middleware
> +
> +        Call Beaker's original constructor to set the options, then
> +        unset secure option as we're handling that on our own and don't
> +        want Beaker to interfere.
> +        """
> +        super(SessionMiddleware, self).__init__(wrap_app, config,
> +            environ_key, **kwargs)
> +        self.options["secure"] = False
> +        # self.options["httponly"] = True

I guess this either should be removed or have a comment to explain what 
the purpose is and when it can be useful?

> +
> +    def __call__(self, environ, start_response):
> +        """
> +        This function's implementation is taken directly from Beaker,
> +        with HTTPS detection added. When accessed over HTTPS, force
> +        setting cookie's secure flag.
> +        """
> +        session = SessionObject(environ, **self.options)
> +        if environ.get('paste.registry'):
> +            if environ['paste.registry'].reglist:
> +                environ['paste.registry'].register(self.session, session)
> +        environ[self.environ_key] = session
> +        environ['beaker.get_session'] = self._get_session
> +
> +        if 'paste.testing_variables' in environ and 'webtest_varname' in self.options:
> +            environ['paste.testing_variables'][self.options['webtest_varname']] = session
> +
> +        is_ssl = environ['wsgi.url_scheme'] == 'https'
> +
> +        def session_start_response(status, headers, exc_info=None):
> +            if session.accessed():
> +                session.persist()
> +                if session.__dict__['_headers']['set_cookie']:
> +                    cookie = session.__dict__['_headers']['cookie_out']
> +
> +                    if is_ssl:
> +                        cookie += "; secure"
> +                    if cookie:
> +                        headers.append(('Set-cookie', cookie))
> +            return start_response(status, headers, exc_info)
> +        return self.wrap_app(environ, session_start_response)
> _______________________________________________
> kallithea-general mailing list
> kallithea-general at sfconservancy.org
> http://lists.sfconservancy.org/mailman/listinfo/kallithea-general