search

bbangert / beaker

WSGI middleware for sessions and caching
https://beaker.readthedocs.org/
Other
524 stars 143 forks source link

Fix path for CookieSession #171

Closed FredrikAppelros closed 5 years ago

FredrikAppelros commented 5 years ago

CookieSession does not properly apply the cookie_path configuration option, causing the resulting cookie to be set for the default path instead.

This happens directly after initialization since this does not set the internal dict item associated with the _path key. It also happens after calling delete since this will clear the dict items.

The attribute _path should be used in _create_cookie instead as is done in _set_cookie_values.

amol- commented 5 years ago

I'm not confident this is the right fix. Checking the code it seems that path is read/written from the Session used as a dict, but _path is read/written from the session used as an object.

See https://github.com/bbangert/beaker/blob/master/beaker/session.py#L668-L674 vs https://github.com/bbangert/beaker/blob/master/beaker/session.py#L594

There self['_path'] was read, but the init function set self._path and the two are two different things and looks like there is some confusion in the code about which one should be used.

FredrikAppelros commented 5 years ago

I have tried myself to find the reason for why the code looks the way it does and I came to the conclusion that the idea behind it could be the following.

Properties are stored in dict items when you need to access them from the server. The documentation states:

The session object provided by Beaker’s SessionMiddleware implements a dict-style interface with a few additional object methods.

Properties that are used internally by the session backend are instead stored in object attributes. E.g. there are properties inherent to cookies such as domain and path which define the cookie itself and hence should not be seen as part of the session values. These need to be available to the backend even if the session values are cleared.

It seems that somewhere along the way the implementation has diverged from this. Like you are pointing out there seems to be some mix up about when to use the different properties.

I will take another look at this and try to correct it. I have also found another issue related to when a session is loaded from a cookie that is still broken. So I will try to include a test and a fix for that as well.

FredrikAppelros commented 5 years ago

Seems like third time's the charm. I figured out that both storage locations are used in the implementation and that we need to make sure that the inherent properties are stored in the session state if we are to be able to reconstruct the session on load.

amol- commented 5 years ago

What I don't get is: why we can't just get rid of self._path and only ever use self['_path']?

FredrikAppelros commented 5 years ago

We could, however I would argue that is out of scope here as I am only trying to solve the issues mentioned in this PR.

I believe you can refactor this later to only use a single storage location.

FredrikAppelros commented 5 years ago

Are there any plans to create a new release of Beaker which will feature this fix any time soon?

amol- commented 5 years ago

I'm working on a branch to refactor _paths and _domain dicothomy. As soon as that's done I'll make a release

FredrikAppelros commented 5 years ago

Okay, great. Thanks for the update!

FredrikAppelros commented 5 years ago

Any update on how the refactoring is going or when a new release will be available?

amol- commented 5 years ago

I'll release 1.11 this weekend. It was already done

amol- commented 5 years ago

Released