Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sane default for cookie path #46

Closed
wants to merge 1 commit into from
Closed

Conversation

basz
Copy link
Contributor

@basz basz commented Jan 15, 2016

Browsers will start a new session for every uri without it ->withPath('/')

@Ocramius
Copy link
Member

@basz don't they start it for all sub-paths anyway if you set it to /?

@Ocramius Ocramius added this to the 1.0.3 milestone Jan 16, 2016
@basz
Copy link
Contributor Author

basz commented Jan 16, 2016

Yes

With '/' things work as expected, with null my browsers used multiple sessions cookies. Each valid for the distinct paths '/' and '/api/ping' (zend-expressive-skeleton).

http://tools.ietf.org/html/rfc6265#section-5.1.4

A request-path path-matches a given cookie-path if at least one of
   the following conditions holds:

   o  The cookie-path and the request-path are identical.

   o  The cookie-path is a prefix of the request-path, and the last
      character of the cookie-path is %x2F ("/").

   o  The cookie-path is a prefix of the request-path, and the first
      character of the request-path that is not included in the cookie-
      path is a %x2F ("/") character.

@Ocramius
Copy link
Member

with null my browsers used multiple sessions cookies

This is unclear to me: what was the behavior without the path? What was sent, and what was expected instead?

@basz
Copy link
Contributor Author

basz commented Jan 16, 2016

While testing psr7session I included it as pre routing middleware it into zend-expressive-skeleton.

I also wrote some middleware that increments a value, let's call it 'ssn', and stores it onto the session container.

The skeleton application has two actions and on both I pulled the ssn value from the session storage and passed it to the view. One in the footer and one in the Json payload and I was expecting to see that value increment on every request.

This worked in principle - a refresh will increment the displayed ssn value. The psr7session worked. Yeah! However when I went from the homepage action to the ping action the ssn value restarted with 0. Refreshing that page would successfully increment ssn. Going back to the homepage the value didn't reset to zero but continued increasing from its previous value.

In effect I was having two sessions each with it own ssn value.

Took me it took me a while before I realized the path option of the cookie was set to null which would make each path have its own cookie.

Setting the path option to '/' solved the issue. Now every request under '/' has the same cookie and thus session...

Clearer? I can setup a demo somewhere, can't explain it any better with words...

Op 16 jan. 2016 om 17:25 heeft Marco Pivetta [email protected] het volgende geschreven:

with null my browsers used multiple sessions cookies

This is unclear to me: what was the behavior without the path? What was sent, and what was expected instead?


Reply to this email directly or view it on GitHub.

@basz
Copy link
Contributor Author

basz commented Jan 16, 2016

Soo, this pr is not a bug fix for psr7session nor the cookie library. It is just a better default.

Op 16 jan. 2016 om 17:25 heeft Marco Pivetta [email protected] het volgende geschreven:

with null my browsers used multiple sessions cookies

This is unclear to me: what was the behavior without the path? What was sent, and what was expected instead?


Reply to this email directly or view it on GitHub.

@Ocramius
Copy link
Member

Ok, now that makes more sense :-)
On Jan 16, 2016 11:24 AM, "Bas Kamer" [email protected] wrote:

Soo, this pr is not a bug fix for psr7session nor the cookie library. It
is just a better default.

Op 16 jan. 2016 om 17:25 heeft Marco Pivetta [email protected]
het volgende geschreven:

with null my browsers used multiple sessions cookies

This is unclear to me: what was the behavior without the path? What was
sent, and what was expected instead?


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
#46 (comment).

@Ocramius
Copy link
Member

I think this should also be fixed inside the factories, but we'll need to release 2.0.0 for this fix to be merged, as it is a breaking change.

@Ocramius Ocramius added bug and removed enhancement labels Jan 16, 2016
@Ocramius Ocramius modified the milestones: 2.0.0, 1.0.3 Jan 16, 2016
@basz
Copy link
Contributor Author

basz commented Jan 16, 2016

auch..... indeed. however you could argue that because its behaviour was broken in the first place - although strictly speaking a breaking change - no one could be using it as it is at the moment?

@Ocramius
Copy link
Member

It is a BC break regardless. Bumping major version is not a tabu: if it's
needed, let's just do it.

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

On 16 January 2016 at 14:58, Bas Kamer [email protected] wrote:

auch..... indeed. however you could argue that because its behaviour was
broken in the first place - although strictly speaking a breaking change -
no one could be using it as it is at the moment?


Reply to this email directly or view it on GitHub
#46 (comment).

@Ocramius Ocramius modified the milestones: 2.0.0, 3.0.0 May 29, 2016
@Ocramius Ocramius assigned Ocramius and unassigned malukenho May 29, 2016
Ocramius added a commit that referenced this pull request May 29, 2016
Ocramius added a commit that referenced this pull request May 29, 2016
Ocramius added a commit that referenced this pull request May 29, 2016
Ocramius added a commit that referenced this pull request May 29, 2016
@Ocramius Ocramius closed this in 94bcd47 May 29, 2016
@Ocramius Ocramius changed the title Suggest sane default for cookie path Sane default for cookie path May 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants