Skip to content

Implement custom cookie signature #337

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

roblabla
Copy link

@roblabla roblabla commented Aug 1, 2016

This is an updated version of #98, that applies properly on current master.

@gabeio gabeio added the pr label Aug 9, 2016
index.js Outdated
@@ -602,8 +606,8 @@ function issecure(req, trustProxy) {
* @private
*/

function setcookie(res, name, val, secret, options) {
var signed = 's:' + signature.sign(val, secret);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is not backwards-compatible, just as a FYI. It would also make the cookies emitted from here unreadable by other modules trying to decode them without them changing. What is the purpose of this change?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My purpose here was that I need to have full control over the cookie's content. (My end-game is to make express-session compatible with PHP's session management). That means having a "pass-through" signature function (AKA no signature at all).

I have a patch that should (in theory) turn back compatibility while still allowing me to achieve my goals.

@dougwilson
Copy link
Contributor

Looks like the PR is marked as failed because there are not enough tests to cover all the changes made in the PR. Please update the tests to try to cover all added code paths :)

README.md Outdated
Pass in your own cookie signing object/module here that implements the
`sign(value, secret)` and `unsign(value, secret)` functions.

The default value is the `node-cookie-signature` module.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo here, as this module uses the cookie-signature module, not node-cookie-signature.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably include a link to the npm page for the module as well.

@roblabla
Copy link
Author

roblabla commented Aug 16, 2016

Done with all your suggestions :). I think it should be compatible with previous versions now. The s: is only appended in the default signature. In my view, it's the job of the signature module to append it if it needs it (my patch doesn't need it). This allows express-session to be compatible with php's PHPSESSID without much trouble.

@dougwilson dougwilson self-assigned this Nov 17, 2016
@dougwilson dougwilson force-pushed the master branch 2 times, most recently from 9d2e29b to 408229e Compare January 28, 2024 20:24
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