Skip to content

Implement custom cookie signature#337

Closed
roblabla wants to merge 4 commits into
expressjs:masterfrom
roblabla:feature-signatureOption
Closed

Implement custom cookie signature#337
roblabla wants to merge 4 commits into
expressjs:masterfrom
roblabla:feature-signatureOption

Conversation

@roblabla

@roblabla roblabla commented Aug 1, 2016

Copy link
Copy Markdown

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

@gabeio gabeio added the pr label Aug 9, 2016
Comment thread index.js
*/

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 :)

Comment thread 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
Copy Markdown
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
Copy Markdown
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

roblabla commented Aug 16, 2016

Copy link
Copy Markdown
Author

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.

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.

4 participants