2.2.0 verifyAndDecodeToken Bugs

[Phylodome]

Looking at the code published in 2.2.0, I noticed a few issues.

The previous header retrieval logic took advantage of short-circuiting to avoid issues of attempting to access properties of an undefined object:

  !req ||
  !req.headers ||
  (!req.headers.authorization && !req.headers.Authorization)

At each stage of this predicate, the || ensures we're not attempting to access fields of an undefined object.

But as of 2.2.0:

(!req ||
  !req.headers ||
  (!req.headers.authorization && !req.headers.Authorization)) &&
(!req.cookies && !req.cookies.token))

There are actually 2 bugs in just these 4 lines of code, whereby we may attempt to access the fields of an intermediate undefined object.

First, let's say that the initial !req evaluates to true. Because the initial logic is now adjoined with (!req.cookies && !req.cookies.token)) we will now attempt to evaluate !req.cookies, but req is undefined, and we blow up.

Second, the same logic applies within (!req.cookies && !req.cookies.token) itself, if req.cookies is undefined: !req.cookies evaluates to true, so we attempt to execute !req.cookies.token, and again blow up trying to access a property on an undefined object.

Because these are inside an if statement and not a try/catch, these cases will propagate to the GraphQL error response, which I assume is not the intent.

In the first case, one might argue that a missing req is violating the package's API, and thus no guarantees are provided / blowing up is fine. I'd tend to disagree, favoring the friendlier developer ergonomics of catching such cases and providing a useful error message.

In the second case, it's perfectly reasonable to not have a req.cookies object. In fact this is the default in express, absent a cookie-parsing middleware, which is how I ran into this issue when trying to upgrade.