Skip to content

add parametrizable get/setcookie #420

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 3 commits into
base: master
Choose a base branch
from
Open

Conversation

caub
Copy link

@caub caub commented Jan 28, 2017

usage:

session({
	...
	getcookie({headers}, name, secrets) {
		if (headers.cookie) {
			var cookies = cookie.parse(headers.cookie);
			return signature.unsign(cookies[name] || '', secrets[0]);
		}
		else if (headers.authorization) {
			return signature.unsign(headers.authorization || '', secrets[0]);
		}
		return null;
	},
	setcookie(res, name, val, secret, options) {
		var signed = signature.sign(val, secret);
		var data = cookie.serialize(name, signed, options);

		var prev = res.getHeader('set-cookie') || [];
		var header = Array.isArray(prev) ? prev.concat(data) : [prev, data];

		res.setHeader('set-cookie', header);
		res.setHeader('authorization', data);
	}
})

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! I'm not sure fi you've read through all the discussions in this repo about the existing trials and possible solutions to using an Authorization header or not, but some quick comments on the PR:

  1. There are no tests, please add some when you can.
  2. There is no documentation, if you can add documentation when you have time, especially since your example in the opening comment doesn't even work, since the first argument to getcookie is req, not a headers object.

In addition, it seems like you're trying to make a PR to solve the using a different header problem, but it seems like it still misses on some of the points brought up in previous discussions, namely the concerns regarding a proper API to do this vs just quickly trying to PR in a hack to get it done. This solution provides a way to override private functions, and those functions' interfaces are probably not ideal for a public API and have changed over time. The setcookie for sure is an ugly API for public consumption, and even then mixing in cookie options set for the module with the intention for the function to do non-cookie things is not an ideal API.

@caub
Copy link
Author

caub commented Jan 28, 2017

you mean #158 surely, yes I've seen it, the goal is to have a fallback for non-cookie platforms.

That PR is fully compatible, I'll add a test using authorization, but I understand your point that those 2 parameters open dangerous doors.

The other solution to do it, is to have another middleware just before or after session, that will use the sessionStore, and then hack express-session, I don't feel that's better

	app.use(function(req, res, next){
		if (req.headers.cookie || !req.headers.authorization) return next();
		var key = signature.unsign(req.headers.authorization || '', sessionSecret);
		sessionStore.get(key , function(err, sess) {
			if (sess) {
				req.sessionID = key;
				req.sessionStore = sessionStore;
				sessionStore.createSession(req, sess);
			}
			next();
		});
	});

ps: note it's {headers}, (destructuring)

@dougwilson
Copy link
Contributor

Oh, sorry, I'm not familiar with a lot of ES6 syntax :) That makes sense, then. There are at least 5 different threads on this, and I was mainly referring to the current PR discussion on the most recent PR to try this, not #158

@dougwilson
Copy link
Contributor

#79 and #159 is where most of the discussion is at.

request(server)
.get('/admin')
.expect(function (res) {
console.log(res.headers)
Copy link
Author

@caub caub Jan 29, 2017

Choose a reason for hiding this comment

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

I'm not familiar with this testing, this console.log returns { 'set-cookie': [ 'connect.sid=s%3Ax2fXsI8gGwByJoPat0MFajRByCYqGLTl.%2BswySoAbDN7t5XAf25VvXOrCRLMi53vlreAQ5atKEzc; Path=/; Expires=Sun, 29 Jan 2017 15:03:19 GMT; HttpOnly' ], date: 'Sun, 29 Jan 2017 15:02:20 GMT', connection: 'close', 'content-length': '0' } I don't understand yet why it doesn't go in the custom setcookie function

@caub caub force-pushed the getcookie branch 7 times, most recently from 2f4eeae to 31d97b3 Compare January 29, 2017 20:00
@caub
Copy link
Author

caub commented Jan 29, 2017

I feel like it won't be merged, but I had read the other issues discussed, and I don't find decent solutions

The idea is to support a wide range of platforms: browser, chrome app (even if they are deprecated)/extensions, phonegap, electron in an uniform way.

I don't like this PR too, I need to see if document.cookie can be shimmed in platforms not having cookies, and how they will react to set-cookie, if it work, that would be a clean solution, but that's another topic

@caub caub force-pushed the getcookie branch 2 times, most recently from 9fccb96 to d2795c6 Compare January 30, 2017 15:52
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.

2 participants