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

Fix bogus sig problem add overwrite-flag #19

Merged
merged 4 commits into from
Nov 26, 2012

Conversation

mtkopone
Copy link
Contributor

cookie.get('cookie-with-bogus-sig') caused a cookie named 'cookie-with-bogus-sig.sig.sig' to get created.

overwrite-flag allows clearing out previous 'Set-Cookie'-declaration which set a cookie with the same name.

See individual commits for more info.

Mikko Koponen added 3 commits November 13, 2012 11:18
This requires that the cookie header array is filtered when this flag is set. Since connect/express have custom handling for setHeader('Set-Cookie'), we need to bypass that to be able to fully control which cookies get written to the response. Hence the crazy prototype setup, to find a "working" setHeader without connect/express magic.

Caveat: Currently overwrites all cookies with the same name, regardless of path or domain. This is why I didn't make this global behaviour, so now the default is to behave as previously, but give the user the option to clear out any duplicates out of the request.

Reasoning for the whole commit: we can have a situation where one of our signed authentication cookies is corrupt (e.g. bad sig) but it's "parent" cookie is OK so we can regenerate the corrupt cookie by checking the parent cookie with a SSO-service. In this case, the cookies.get('child') tries to clear the 'child.sig'-cookie, with a 'Set-Cookie' with an empty value. Later, the SSO-check rewrites a correct 'child' cookie with it's sig. Without the overwrite flag, the .sig cookie gets duplicated into the 'Set-Cookie' header.
@jed
Copy link
Contributor

jed commented Nov 14, 2012

i'm a little leery of that magical-lookiing __proto__ chain... would you mind refactoring these commits more logically, one for tests, and one for changing the the setHeader logic, and one to add the overwrite option?

@mtkopone
Copy link
Contributor Author

Hi, had a busy week.

I figured out an easier and less brittle way to use the OutgoingMessage.setHeader, and hence bypass the connect-patch which is causing all the problems. See latest commit: e3b1247

As for the rest, the first two stand on their own, and...
762a836 has the logic for adding the overwrite-flag. It alone requires the changes to the way getHeader is getting called, the pushCookie usage, and the way setHeader needs to be called. They're going to work wrong as individual pieces.

jed added a commit that referenced this pull request Nov 26, 2012
…rrides

Fix bogus sig problem add overwrite-flag
@jed jed merged commit 51a7014 into pillarjs:master Nov 26, 2012
@jed
Copy link
Contributor

jed commented Nov 26, 2012

thanks for your help, @mtkopone.

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.

3 participants