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

Change default cookie name from 'dancer.session' to 'dancer2.session'. #1034

Closed
wants to merge 1 commit into from

Conversation

cromedome
Copy link
Contributor

I'm not sure this is a bug per se, but since so many other things were
renamed to be Dancer2, the cookie name seemed like an oversight. For the
sake of being consistent, I changed the name.

This discussion prompted the change:
http://paste.perldancer.org/1kccHcbfIsYNX

In this case, there was a Dancer1 and Dancer2 app on the same host using
the default cookie, and inadvertently ended up sharing the same cookies
(not desirable). Racke posted a solid workaround, but this seemed like
something we should also fix.

I'm not sure this is a bug per se, but since so many other things were
renamed to be Dancer2, the cookie name seemed like an oversight. For the
sake of being consistent, I changed the name.

This discussion prompted the change:
http://paste.perldancer.org/1kccHcbfIsYNX

In this case, there was a Dancer1 and Dancer2 app on the same host using
the default cookie, and inadvertently ended up sharing the same cookies
(not desirable). Racke posted a solid workaround, but this seemed like
something we should also fix.
@xsawyerx
Copy link
Member

Agreed here.

@xsawyerx
Copy link
Member

Merged, thanks! 👍
And thanks to pokki and @racke!

@xsawyerx xsawyerx closed this Dec 16, 2015
xsawyerx added a commit that referenced this pull request Dec 16, 2015
    [ DOCUMENTATION ]
    * Update core team members and contributors list. (Russell Jenkins)
    * GH #1066: Fix typo in Cookbook. (gertvanoss)
    * Correct typo. It's "query_parameters", not "request_parameters".
      Thanks to mst for letting me know and making sure I fix it!
      (Sawyer X)

    [ BUG FIXES ]
    * GH #1040: Forward with a post body no longer tries to re-read body
      filehandle. (Bas Bloemsaat)
    * GH #1042: Add Diggest::SHA as explicit prequisite for installs on
      perl < v5.9.3. (Russell Jenkins)
    * GH #1071, #1070: HTML escape the message in the default error page.
      (Peter Mottram)
    * GH #1062, #1063: Command line interface didn't support
      "-s SKELETON_DIRECTORY" in any order.
      (Nuno Carvalho)
    * GH #1052, #1053: Always call before_serializer hook when serializer
      is set.
      (Mickey Nasriachi)
    * GH #1034: Correctly use different session cookie name for Dancer2.
      (Jason A. Crome)
    * GH #1060: Remove trailing slashes when providing skeleton
      directory.
      (Gabor Szabo)

    [ ENHANCEMENTS ]
    * Use Plack 1.0035 to make sure you only have HTTP::Headers::Fast
      in the Plack::Request object internally.
    * GH #951 #1037: Dancer2::Template::TemplateToolkit no longer sets TT2
      INCLUDE_PATH directive, allowing `views` setting to be non-absolute
      paths. (Russell Jenkins)
    * GH #1032 #1043: Add .dancer file to new app scaffolding.
      (Jason A. Crome)
    * GH #1045: Small cleanups to Request class. (Russell Jenkins)
    * GH #1033: strict && warnings in Dancer2::CLI. (Mohammad S Anwar)
    * GH #1052, #1053: Allow before_serializer hook to change the content
      using @_.
      (Mickey Nasriachi)
    * GH #1060: Ignore .git directory when using an external skeleton
      directory.
      (Gabor Szabo)
    * GH #1060: Support more asset file extensions. (Gabor Szabo)
    * GH #1072: Add request->is_options(). (Theo van Hoesel)
@racke
Copy link
Member

racke commented Dec 17, 2015

It seems to me that this change would drop existing sessions if you upgrade Dancer2, which would
be a bad surprise.

@racke racke reopened this Dec 17, 2015
@xsawyerx
Copy link
Member

Hmm... tricky. I thought of this more of as a security fix, but you're right. I'll patch it back and release quickly. How do you suggest fixing it?

@racke
Copy link
Member

racke commented Dec 17, 2015

Good question, what about setting session_name in the skeleton to the correct value and add a note on startup for existing applications without session_name setting?

@xsawyerx
Copy link
Member

We could do that and start throwing warnings on the old behavior. The problem is that it will force everyone to change the configuration until that configuration is default.

@racke
Copy link
Member

racke commented Dec 17, 2015

Yeah. Maybe just recommending dancer2_session as best practice?

@SysPete
Copy link
Member

SysPete commented Dec 17, 2015

First of all pokki would have had (almost) the same problem if running two independent D2 apps rather than one D1 and on D2 even if the default cookie name was changed. Agree that adding session_name to skel is a good idea but IMO best practice is to name the session cookie something app-specific or completely generic. For a single app domain I generally set it to 'session' and when running multiple apps will use things like 'session-admin' and 'session-shop'.

I agree with @racke that changing the default is now a bad idea. I'll create a PR for skel changes with some text so we have something to discuss.

@xsawyerx
Copy link
Member

I've released a version that revert this change. Considering we see this change, as this PR presents it, to be the wrong approach, should we close this PR?

@SysPete
Copy link
Member

SysPete commented Dec 17, 2015

@xsawyerx now #1074 exists I think this one can be closed.

@racke
Copy link
Member

racke commented Dec 17, 2015

On 12/17/2015 10:25 AM, Sawyer X wrote:

I've released a version that revert this change. Considering we see this change, as this PR presents it, to be the wrong approach, should we close this PR?

Yes, let's close it.

Regards
Racke

Perl and Dancer Development

Visit our Perl::Dancer conference 2015.
More information on https://www.perl.dance.

@fgabolde
Copy link
Contributor

First of all pokki would have had (almost) the same problem if running two independent D2 apps rather than one D1 and on D2 even if the default cookie name was changed.

(I'm pokki.)

"almost" the same problem is right: since D1 and D2 use different session ID formats, D2 (or some D2 session engines? it's been a while) actually throws an exception when it receives a D1 cookie. This is how I first noticed it.

Anyway, thanks everybody for the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants