-
Notifications
You must be signed in to change notification settings - Fork 223
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
Signed cookie value is assigning with false when cookie secret is updated #108
Comments
This is not a bug, the behavior is as described in the documentation |
@dougwilson Thanks for checking and providing your feedback. Somehow, I missed to read this specific part in the module documentation. I have one query and its related to encryption of the cookies. As of now, I can see Is this feature already in draft or if anyone has been already assigned? In case if not, could you assign this work to me so that I will work towards it and contribute to this module. Let me know if this new feature works for you? |
I no longer work on this project. Just do what you want to contribute and someone will get back to you. |
I am not sure. But you don't need to tag people to interact with a project, as those who are working on it will see your issues and stuff. |
Sure, thanks for the information. I will start working on the new feature. |
Issue in detail:
Let's say a signed cookie is created with
['abc']
secret usingcookie-parser
module and save on the browser's cookie storage. Later on, due to the reason of cookie secret compromise, the user needs to change the cookie secret for e.g.,['xyz']
on the express server.Now, the server is re-started with the updated cookie secret
['xyz']
and the user tries to fire an HTTP request method via browser on the same domain on another API endpoint of the server. As the signed cookie created with['abc']
secret already exists on the browser's cookie storage (Assume the cookie is not expired), the same signed cookie is included by the browser under thereq
object and express server receives the same underreq.headers.cookie
.In this case, as the cookie secret is updated, the
cookie-parser
module's middleware function will not be able to parse that signed cookie. I have gone throughsignedCookies(obj, secret){}
function defined under the same module and found that in this particular case, the function is populatingreq.signedCookies
object with keycookieName
and valuefalse
.Since that signed cookie is unable to parse with the updated secret, the
signedCookies(obj, secret){}
function should not include this scenario-type signed cookies underreq.signedCookies
object as they become invalid post cookie secret updation.The bug is reproducible by the below steps included with screenshots:
cookieParser()
with secret'abc'
usingconst cookieParser = require('cookie-parser');
const express = require('express');
const app = express();
app.use(cookieParser('abc'));
return res.cookie('csrf', 'csrfToken', { expires: new Date(Date.now() + 30 * 60 * 1000), httpOnly: true, signed: true });
'xyz'
usingapp.use(cookieParser('xyz'));
cookieParser(req, res, next)
middleware ofcookie-parser
module.signedCookies (obj, secret){}
, this function does not handle possible values ofvar dec
forundefined
andfalse
returned bysignedCookie(val, secret){}
function.false
value is returned bysignedCookie(val, secret){}
function ascookie-signature
module is unable to unsign that signed cookie with the updated secret and thus returnsfalse
. As the validation forundefined
andfalse
values are not implemented, thereq.signedCookies
object is populated with that signed cookie consequently having cookie name ascsrf
and cookie value asfalse
.Fixing the bug:
Handle the validation of afore-mentioned
undefined
andfalse
values in such a way that all signed cookies possessing the scenario described above can be ignored and should neither be included inreq.cookies
nor inreq.signedCookies
objects as they become invalid post cookie secret updation.I have already worked to fix this bug and will soon create a pull request tagging this issue no. as reference.
@dougwilson Kindly check on my careful observations and provide your meaning inputs on the same. If my work looks correct and appreciable, please merge my pull request which I will be creating after posting this issue.
The text was updated successfully, but these errors were encountered: