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

Signed cookie value is assigning with false when cookie secret is updated #108

Open
sarraf1996 opened this issue Sep 26, 2024 · 5 comments
Labels

Comments

@sarraf1996
Copy link

sarraf1996 commented Sep 26, 2024

Issue in detail:

Let's say a signed cookie is created with ['abc'] secret using cookie-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 the req object and express server receives the same under req.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 through signedCookies(obj, secret){} function defined under the same module and found that in this particular case, the function is populating req.signedCookies object with key cookieName and value false.

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 under req.signedCookies object as they become invalid post cookie secret updation.

The bug is reproducible by the below steps included with screenshots:

  1. Initialize cookieParser() with secret 'abc' using
    const cookieParser = require('cookie-parser');
    const express = require('express');
    const app = express();
    app.use(cookieParser('abc'));
  2. Create a signed cookie and return the response using
    return res.cookie('csrf', 'csrfToken', { expires: new Date(Date.now() + 30 * 60 * 1000), httpOnly: true, signed: true });
  3. Update and save new cookie secret 'xyz' using
    app.use(cookieParser('xyz'));
  4. Restart the express server.
  5. Use browser to send HTTP request on the same server's domain on another API endpoint of the server.
  6. The server gets the browser request and the request goes through cookieParser(req, res, next) middleware of cookie-parser module.
  7. Execution flow comes under the module function signedCookies (obj, secret){}, this function does not handle possible values of var dec for undefined and false returned by signedCookie(val, secret){} function.
    Existing_Code
  8. In this context, false value is returned by signedCookie(val, secret){} function as cookie-signature module is unable to unsign that signed cookie with the updated secret and thus returns false. As the validation for undefined and false values are not implemented, the req.signedCookies object is populated with that signed cookie consequently having cookie name as csrf and cookie value as false.
    Issue_Proof

Fixing the bug:

Handle the validation of afore-mentioned undefined and false values in such a way that all signed cookies possessing the scenario described above can be ignored and should neither be included in req.cookies nor in req.signedCookies objects as they become invalid post cookie secret updation.
New_Code
Issue_Fix_Proof

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.

@dougwilson
Copy link
Contributor

This is not a bug, the behavior is as described in the documentation

@sarraf1996
Copy link
Author

sarraf1996 commented Sep 28, 2024

@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 cookie-parser module is using encoding techniques to sign a cookie using cookie-signature module. We can implement cookie encryption as a new feature which will provide a more secured way of cookie creation and transmission from server to client and vice versa.

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?

@dougwilson
Copy link
Contributor

dougwilson commented Sep 28, 2024

I no longer work on this project. Just do what you want to contribute and someone will get back to you.

@dougwilson
Copy link
Contributor

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.

@sarraf1996
Copy link
Author

Sure, thanks for the information. I will start working on the new feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants