Bug: Error on Google Cloud Functions

[keplersj]

When used on Google Cloud Functions an error is thrown due to req.originalURL === "". The error is thrown from these 3 lines handling originalPath.

The debugging to find this bug can be found in this tweet thread with @blakeembrey

[dougwilson]

Weird, that is not something that Express.js won't have set. Can you provide all the following?

1. Version of Express.js you are using.
2. Version of this module.
3. Version of Node.js
4. Complete code to reproduce the issue.
5. Complete steps to call the code to cause the issue.

Thanks!

[dougwilson]

Basically this imo is an issur somewhere outside this module. I would think an Express.js bug if that field is not being populated, so would like to take a look to fihure out what the root cause is.

[keplersj]

1. Express.js Version N/A. This bug occured in a Google Cloud Functions setup without Express.js. I was using compose-middleware to configure cookie-parser, express-session, and passport and return a Express.js middleware compatible with Google Cloud Functions. According to their documentation Google Cloud Functions provide Express.js Request and Response properties.
2. express-session version 1.15.4.
3. Node.js Version 6.11.1.
4. A basic sample of the code can be found in this gist.
5. The complete steps to reproduce this sadly involve deploying to a Google Cloud Function instance. I tried to reproduce this is a variety of local environments, including their own emulator, but could never get the error reproducible outside of production.

[dougwilson]

Ah, I didn't know what Google Cloud Functions even was. Are you sure the issue isn't just that their emulation simply had a bug? I suggest that because the parseurl module already has code to fallback to req.url if req.originalUrl is not populated and even uf we skipped it here, that would mean this module would just end up unconditionally calling next() and thus never working in your scenario.

[dougwilson]

Looks like there is definitely an issue in at least parseurl, because the following

$ node -pe 'require("parseurl").original({originalURL:""})'
undefined

Should be returning a URL object, not undefined. I'll get that fixed, but you'll end up just getting this module unconditionally skipped instead of an error.

[keplersj]

I suppose I should've filed an issue with parseurl instead of express-session I did look through parseurl when debugging which is how I found my temp solution of just setting req.originalUrl to "/" before running express-session

[dougwilson]

Oh, nevermind, I had the wrong capitalization for originalUrl , lol. It works properly from the information you provided:

$ node -pe 'require("parseurl").original({originalUrl:""})'
Url {
  protocol: null,
  slashes: null,
  auth: null,
  host: null,
  port: null,
  hostname: null,
  hash: null,
  search: null,
  query: null,
  pathname: null,
  path: null,
  href: '',
  _raw: '' }

[dougwilson]

So no error would be thrown on the three lines you highlighed even if req.originalUrl === ''. Can you paste the error you're getting on those lines?

[keplersj]

Sure thing. Let me remove the fix and get output of req real quick.

[dougwilson]

Wow, nevermind :) The .indexOf call would fail, duh!! Yes, I can get this fixed, though not sure what the fix should be. Without a URL we cannot perform the path check, so we could either always next() or just execute anyway, even if that will screw up the purpose of that condition, which is to stack multiple express-session modules on different paths... or always call next() possibly making this module useless for you.

[keplersj]

I totally recognize that I'm running express-session in an environment it wasn't designed to be run on and this probably breaks a few environment assumptions...

[dougwilson]

Well, thinking about it, probably this would be the fix:

If no pathname, then call next() if the cookie path is set to anything other than '/' (the default). Since / would cover the entire domain anyway, checking the path is not useful.
Otherwise, check the path and next() if not prefixing.

[keplersj]

Sounds good to me

[dougwilson]

I totally recognize that I'm running express-session in an environment it wasn't designed to be run on and this probably breaks a few environment assumptions...

I mean, we support various environments. I would think that if GCF if trying to provide an Express.js compatible interface then it is a bug if it is not actually doing so. This module supports connect framework as well, and even supports the plain-old http module in Node.js without Express.js nor connect.

What seems to be messing this up here is that req.originalUrl is set to an empty string. So this module should skip those requests. This module is A-OK if req.originalUrl is not even populated and will work fine, assuming that it is in an environment that doesn't provide that, but if there is an environment that is taking the effort to populate that property, we'll honor it, of course. If it's populating it wrong... I mean there's bigger problems there and you're really lucky that we just happened to error out here instead of silently not working and pulling your hair out without a single error... :)