search

expressjs / csurf

CSRF token middleware
MIT License
2.3k stars 217 forks source link

Adding in Support for Flexible Session Keys #67

Closed rdegges closed 9 years ago

rdegges commented 9 years ago

This commit should fix #66.

Basically, I added a new option sessionKey, which allows the developer to specify the name of the session key used.

I added in README docs, a working implementation, and tests which support this.

Happy to make changes if needed.

rdegges commented 9 years ago

Hmm, so apparently client-sessions won't work with node 0.8. Not sure how to get around this for testing, since express-sessions don't allow for arbitrary session request keys. Any suggestions? Other than this, I'm sure things will pass.

dougwilson commented 9 years ago

We do need the tests to work on Node.js 0.8, and I don't forsee us dropping Node.js 0.8 from this module anytime soon. My only suggestion is perhaps just add a custom mini session module as a fixture in this source code instead of using another library?

rdegges commented 9 years ago

Thanks @dougwilson I'll get this fixed today ^^

rdegges commented 9 years ago

Woot, fixed. Apparently switching from ^ -> ~ was all that was required -- client-sessions does actually support 0.8.x.

Looks like we won't need any mocking libraries after all! Whew! Anything else need fixing, or does this look good to you?

dougwilson commented 9 years ago

Hi @rdegges , sorry for the delay. In general it's good. I'm just thinking over an idea that would make it even more flexible (just taking a function that will determine where the key will come from), but thinking that's probably better left for the next major to try and consolidate all these different options, rather than changing this PR :)

rdegges commented 9 years ago

Hey @dougwilson -- no problem! If you think that's a better option, I'm happy to accommodate. I do agree that passing a function in would be better for a future release though (or potentially even restructuring the options completely -- I have some ideas for that as well).

But not sure if you want in increment breaking changes atm or not.

dougwilson commented 9 years ago

Right. For now, I'm just going to take this as-is so people can put the session object at any property they choose :)

rdegges commented 9 years ago

Sounds good! I'll resolve those merge conflicts from the doc updates so you can merge this in then. Appreciate you taking the time to look this over ^^

dougwilson commented 9 years ago

I'll resolve those merge conflicts from the doc updates so you can merge this in then.

Cool :) It's not necessary, but I certainly welcome it. I'll have it merged in either way today, haha

rdegges commented 9 years ago

All fixed! Also: happy to help. I maintain a ton of projects myself, so I know it can get super crazy with PRs and merge conflicts and it takes a ton of time to sort stuff out.

Thanks for being so responsive! Has been a pleasure contributing tot his project ^^

dougwilson commented 9 years ago

haha, thanks :) I don't use the GitHub merge butt anyway, FWIW :)

rdegges commented 9 years ago

Thanks for merging this! Any chance you want to cut a release with this included? ^^

dougwilson commented 9 years ago

I will :) I just like to wait just a bit to reflect on it in case I realize there is some obvious issue. If you want to test it to at least confirm on that side, that would help :) Otherwise it'll likely be releases by default tomorrow.

dougwilson commented 9 years ago

@rdegges just published this to npm as version 1.8.0

rdegges commented 9 years ago

Thanks! I'm traveling so am slow to respond atm, but I really appreciate it! <33