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

'TypeError: cookie required' when setting custom id #103

Closed
djtregear opened this issue Dec 2, 2014 · 15 comments
Closed

'TypeError: cookie required' when setting custom id #103

djtregear opened this issue Dec 2, 2014 · 15 comments
Labels

Comments

@djtregear
Copy link

I'm trying to use a session ID from my mongo database as the sessionID within express-session and have these details stored within a connect-redis session store.

I keep getting 'TypeError: cookie required'

This appears to be a bug, but could somebody check my logic here please:

I've stripped the code down to it's bare bones and removed anything that's not required to create the issue.

'use strict';


var express       = require('express'),
    session       = require('express-session'),
    redisStore    = require('connect-redis')(session),
    bodyParser    = require('body-parser'),
    app           = express();


app.use(session({
  name: 'test',
  genid: function(req) {
    if (typeof req.sessionID != 'undefined') return req.sessionID;
  },
  cookie: {
    httpOnly: false,
    path: '/',
    maxAge: null
  },
  resave: false,
  saveUninitialized: false,
  secret: 'finbarboobar',
  store: new redisStore({
    host: 'localhost',
    port: 6379,
    prefix: 'kTest:'
  })
}));


app.use(bodyParser.urlencoded({ extended: false }));


app.route(['/'])
  .get(function(req, res) {
    if (req.session.email) return res.redirect('/admin');
    res.send('<h1>Login</h1><form action="/login" method="POST" accept-charset="UTF-8" ><input placeholder="Enter Email" name="email" type="email" autofocus="autofocus"><input type="submit" value="sign in"></form>');

  });


app.route(['/login'])
  .post(function(req, res) {
    req.sessionID = 1;
    req.session.regenerate(function(error) {
      if (error) {
        console.log(error);
      } else {
        req.session.email = req.body.email;
        res.redirect('/admin');
      }
    });
});


app.get('/admin', function(req, res) {
  if (req.session.email) {
    res.send('<h1>Hello ' + req.session.email + '</h1><a href="/logout">logout</a>');
  } else {
    res.redirect('/');
  }
});


app.get('/logout', function(req, res) {
  req.session.destroy(function(error) {
    if (error) {
      console.log(error);
    } else {
      res.redirect('/');
    }
  })
});


app.listen(3001);
@dougwilson
Copy link
Contributor

Can you post the full stack trace for that error, please?

@djtregear
Copy link
Author

I hope this is what you're after, if not, let me know what you need me to do. Thanks.

TypeError: cookie required
    at Object.exports.sign (/Users/djtregear/Documents/Foundry/testSession/node_modules/express-session/node_modules/cookie-signature/index.js:17:37)
    at setcookie (/Users/djtregear/Documents/Foundry/testSession/node_modules/express-session/index.js:537:33)
    at ServerResponse.<anonymous> (/Users/djtregear/Documents/Foundry/testSession/node_modules/express-session/index.js:196:7)
    at ServerResponse.writeHead (/Users/djtregear/Documents/Foundry/testSession/node_modules/express-session/node_modules/on-headers/index.js:44:16)
    at ServerResponse._implicitHeader (http.js:1133:8)
    at ServerResponse.OutgoingMessage.write (http.js:841:10)
    at writetop (/Users/djtregear/Documents/Foundry/testSession/node_modules/express-session/index.js:244:26)
    at ServerResponse.end (/Users/djtregear/Documents/Foundry/testSession/node_modules/express-session/index.js:289:16)
    at ServerResponse.redirect (/Users/djtregear/Documents/Foundry/testSession/node_modules/express/lib/response.js:846:8)
    at /Users/djtregear/Documents/Foundry/testSession/index.js:62:13

@dougwilson
Copy link
Contributor

Thank you, that is exactly what I was looking for :)

@dougwilson
Copy link
Contributor

So, really, the only way to that that error is if you are not actually providing the name option to the session constructor. You are in the example you posted above, but is that the literal code you are using when you get the error?

Also, remove that genid, because it's wrong, but I don't think it's causing this issue, but it still shouldn't be there as-is.

@dougwilson
Copy link
Contributor

Also, can you give the output of npm ls? It doesn't seem like you're using the latest version of this library, and I'm just trying to look into the issue on whatever version you're using.

@djtregear
Copy link
Author

Yes this is the literal code (I've created a reduced test case to ease confusion). I thought I was providing the name in the session constructor (name: 'test').

I've removed the genid and the problem is the same, only now the sessionID is the default code, what I'm trying to achieve is to have my own session ID in the Redis database (and therefore in the cookie). If you can help with this that would be great.

As requested, here's the result of an npm ls (hope it helps):

apiDiddly@0.0.2 /Users/djtregear/Documents/Foundry/testSession
├─┬ body-parser@1.9.3
│ ├── bytes@1.0.0
│ ├── depd@1.0.0
│ ├── iconv-lite@0.4.5
│ ├── media-typer@0.3.0
│ ├─┬ on-finished@2.1.1
│ │ └── ee-first@1.1.0
│ ├── qs@2.3.3
│ ├── raw-body@1.3.1
│ └─┬ type-is@1.5.3
│   └─┬ mime-types@2.0.3
│     └── mime-db@1.2.0
├─┬ connect-redis@2.1.0
│ ├─┬ debug@1.0.4
│ │ └── ms@0.6.2
│ └── redis@0.12.1
├─┬ cookie-parser@1.3.3
│ ├── cookie@0.1.2
│ └── cookie-signature@1.0.5
├─┬ express@4.10.4
│ ├─┬ accepts@1.1.3
│ │ ├─┬ mime-types@2.0.3
│ │ │ └── mime-db@1.2.0
│ │ └── negotiator@0.4.9
│ ├── content-disposition@0.5.0
│ ├── cookie@0.1.2
│ ├── cookie-signature@1.0.5
│ ├─┬ debug@2.1.0
│ │ └── ms@0.6.2
│ ├── depd@1.0.0
│ ├── escape-html@1.0.1
│ ├─┬ etag@1.5.1
│ │ └── crc@3.2.1
│ ├── finalhandler@0.3.2
│ ├── fresh@0.2.4
│ ├── media-typer@0.3.0
│ ├── merge-descriptors@0.0.2
│ ├── methods@1.1.0
│ ├─┬ on-finished@2.1.1
│ │ └── ee-first@1.1.0
│ ├── parseurl@1.3.0
│ ├── path-to-regexp@0.1.3
│ ├─┬ proxy-addr@1.0.4
│ │ ├── forwarded@0.1.0
│ │ └── ipaddr.js@0.1.5
│ ├── qs@2.3.3
│ ├── range-parser@1.0.2
│ ├─┬ send@0.10.1
│ │ ├── destroy@1.0.3
│ │ ├── mime@1.2.11
│ │ └── ms@0.6.2
│ ├── serve-static@1.7.1
│ ├─┬ type-is@1.5.3
│ │ └─┬ mime-types@2.0.3
│ │   └── mime-db@1.2.0
│ ├── utils-merge@1.0.0
│ └── vary@1.0.0
└─┬ express-session@1.9.2
  ├── cookie@0.1.2
  ├── cookie-signature@1.0.5
  ├── crc@3.2.1
  ├─┬ debug@2.1.0
  │ └── ms@0.6.2
  ├── depd@1.0.0
  ├── on-headers@1.0.0
  ├── parseurl@1.3.0
  ├─┬ uid-safe@1.0.1
  │ ├── base64-url@1.0.0
  │ └─┬ mz@1.1.0
  │   └── native-or-bluebird@1.1.2
  └── utils-merge@1.0.0

@dougwilson
Copy link
Contributor

Just to humor me real quick, can you do the following: npm cache clean and then npm install express-session in your example? It just didn't seem the line numbers in your stack trace aligned with the 1.9.2 code is all.

what I'm trying to achieve is to have my own session ID in the Redis database (and therefore in the cookie). If you can help with this that would be great.

No problem. The issue with what you had there is there must always be a return value; your code sometimes would return undefined, which is a no-no for the API. You can use it, but it needs to always return a value.

Yes this is the literal code (I've created a reduced test case to ease confusion). I thought I was providing the name in the session constructor (name: 'test').

me too... so I can run your app for sure, but can you provide me the steps i need to take to reproduce the error? is just loading http://localhost:3001 enough, or do i have to do something else?

@djtregear
Copy link
Author

OK, I've completed the npm cache clean and then the npm install express-session and the error is still there. The alignment issues with the line numbers was probably my half-baked attempt at debugging the issue with some console.logs in the express-session code.

Here's the new stack trace (hope this one lines up):

    at Object.exports.sign (/Users/djtregear/Documents/Foundry/testSession/node_modules/express-session/node_modules/cookie-signature/index.js:17:37)
    at setcookie (/Users/djtregear/Documents/Foundry/testSession/node_modules/express-session/index.js:528:33)
    at ServerResponse.<anonymous> (/Users/djtregear/Documents/Foundry/testSession/node_modules/express-session/index.js:187:7)
    at ServerResponse.writeHead (/Users/djtregear/Documents/Foundry/testSession/node_modules/express-session/node_modules/on-headers/index.js:44:16)
    at ServerResponse._implicitHeader (http.js:1133:8)
    at ServerResponse.OutgoingMessage.write (http.js:841:10)
    at writetop (/Users/djtregear/Documents/Foundry/testSession/node_modules/express-session/index.js:235:26)
    at ServerResponse.end (/Users/djtregear/Documents/Foundry/testSession/node_modules/express-session/index.js:280:16)
    at ServerResponse.redirect (/Users/djtregear/Documents/Foundry/testSession/node_modules/express/lib/response.js:846:8)
    at /Users/djtregear/Documents/Foundry/testSession/index.js:62:13

me too... so I can run your app for sure, but can you provide me the steps i need to take to reproduce the error? is just loading http://localhost:3001 enough, or do i have to do something else?

navigate to http://localhost:3001/ then enter an email address into the form field and click submit, it should fail at this point.

Thanks for your help

@dougwilson
Copy link
Contributor

I was able to reproduce :) Now the bug can be squashed!

@dougwilson
Copy link
Contributor

Ok, the root cause is because req.sessionID is a number and not a string.

@dougwilson
Copy link
Contributor

i.e. req.sessionID = 1; in the /login route is causing this issue. The error when that happens need to be much better, and I have to think about a good way to deal with weird issues like this, but the solution right now is you need to set req.sessionID to a string :)

@dougwilson dougwilson added bug and removed investigate labels Dec 2, 2014
@djtregear
Copy link
Author

Brilliant Doug, I revisited my original implementation (as I knew I wasn't using a number there). I was passing a mongo ._id object as the sessionID. I think you'll need to check the typeof whatever is passed as sessionID and perhaps see if you can call a .toString() on any object that arrives (or to keep it light just throw an error saying "sessionID must be string". I'm glad it wasn't just me.

Could you just check this isn't a mad thing to do. I'm going to set the sessionID to a string and then call regenerate and have the genid routine do this:

  genid: function(req) {
    return req.sessionID;
  }

Is this what you'd recommend?

Thanks for your help and time on this one.

David

@dougwilson
Copy link
Contributor

Is this what you'd recommend?

As far as your strategy here, to give a good answer, I need to know more about where this session ID is actually coming from. So it seems to me what you're trying to do is create a document in Mongo and then use that document's automatically-generated ID (basically similar to an auto-increment key in a RDBMS) and use that as the session ID, correct?

If that is the case, there really is no good answer, at least not until #52 is addressed.

@djtregear
Copy link
Author

Hi Doug, just thought I'd say thanks, I hope saying thanks doesn't cause you more work (i.e. reopens this ticket).
FYI: I am effectively creating a mongoDB session, but only for authenticated users and only when they sign in, so this shouldn't cause the race conditions set out in #52. Thanks for your help, you're a star.

David

@dougwilson
Copy link
Contributor

Hey, so the actual contents of what #52 says is a little misleading for what it's about, but the essence is that you can make the call into mongo and then return the session ID, etc. all from genid async.

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