Add support for plain asterisk route

[kasdimos]

When an OPTIONS request with a plain asterisk (*) as the URL is made to the server, the session object is not getting attached to the req object.

To replicate the issue:

JS code:

app.use(sessionMiddleware);
app.use(function(req, res, next) {
  if (req.session.uid) return next();
  res.sendStatus(401);
});

Client command: curl -X OPTIONS --request-target "*" example.com -i

Expected: req.session not being undefined regardless of the request Actual: req.session is undefined

[jonchurch]

Curious what usecase you have that you've run into this? I wouldn't expect session details to be relevant to an OPTIONS request regardless of the URL.

If we took this it would be to ensure consistency, but Im not certain it doesn't have knock on effects or vulnerabilities this could open up.

An options request with wildcard URL is an edgecase. It's valid under the spec, but is considered a noop and edgecase even by the spec:

link

An OPTIONS request with an asterisk ("*") as the request-target (Section 5.3 of [RFC7230]) applies to the server in general rather than to a specific resource. Since a server's communication options typically depend on the resource, the "*" request is only useful as a "ping" or "no-op" type of method; it does nothing beyond allowing the client to test the capabilities of the server. For example, this can be used to test a proxy for HTTP/1.1 conformance (or lack thereof).

[kasdimos]

Consistency is the reason I made the pull request. Right now the session object can be undefined inside a middleware so it is necessary to perform a check before accessing a session property. Currently the following code will throw an exception because it is not checking if the session object is initialized.

app.use(function(req, res, next) {
  if (req.session.uid) return next();
  res.sendStatus(401);
});

I checked my server's logs and found out that it had received some OPTIONS * requests.

[jonchurch]

Hmmm very interesting! At the least the codepath in the diff is not spec compliant in source today bc options requests have a special affordance that they can use a wildcard here.

But are wildcard options requests the only reason req.session could be undefined in your middleware?

[kasdimos]

Correct. As far as I know, wildcard options requests are the only reason req.session can be undefined in the middleware. The reason this issue arose is that the code was not initially spec compliant. Do you suggest to check also if it is an options request?

[jonchurch]

Actually this isn't a spec issue. The spec informs that this is a valid request, and Node lets it through because of that. But whether the request matches the path we've configured is up for debate. We're comparing the path option for cookies to the request's path.

The semantics of cookie path matching and wildcard requests don't map well to each other. Given a specific path like: cookieOptions.path = "/api/v1/" we are checking that we allow everything under it's subdirs such as /api/v1/foo || /api/v1/foo/bar/baz, while blocking /api/v2/foo. So I'd consider cookieOptions.path = /api/v1/ to not match * because our cookie is set granularly.

But the default is cookieOptions.path = "/" which covers all subdirs on the domain. I'd consider that equivalent to * and would expect the options wildcard request to pass under the default.

I don't think anyone else handles this, fastify session would also not match the path for example.

So if a change was going to be taken, it should be to treat a wildcard as semantically the same as when the cookie path is set to all subdirs. Because apparently HTTP has more than one way to express "all subdirs" by way of the options wildcard request-target. TIL

(note: should anyone bother with sessions on OPTIONS? also up for debate! but the reality is rn we would dress all options requests in sessions with the default settings, except these wildcard ones)

[jonchurch]

Just opened https://github.com/expressjs/session/pull/977 as a draft. Feel free to use it in your PR if you agree with it. Don't wanna snatch the glory from you if this does get taken.

It's a draft bc I didn't add a test for this or even test it ad hoc 😉