Should the Passport strategy support HTTP HEAD requests, as well?

[delfuego]

I've been seeing the occasional HTTP 500 response sent from my Passport+OIDC-protected web app, and looking into it, they're all HTTP HEAD requests to my /login route, which is the route that is backed by a passport.authenticate call to my OIDC strategy. The HTTP 500 is returned when the strategy hits line 332 of client.js, and the switch doesn't have a case for an HTTP HEAD method. But in delving into both the expectations of how servers handle HTTP HEAD requests, and what the strategy is really doing, there shouldn't be any problem for the strategy just handling a HEAD the same way it handles GET requests — e.g., just replacing lines 333-334 in BaseClient.callbackParams:

        case 'GET':
          return pickCb(url.parse(input.url, true).query);

with

        case 'GET':
        case 'HEAD':
          return pickCb(url.parse(input.url, true).query);

I'm happy to submit a PR for this, if folks think it's reasonable — it really does meet the expectations for how the Passport strategy would respond to HEAD requests, and prevents the HTTP 500 internal server errors that shouldn't be happening.

[delfuego]

@panva Do you have any thoughts on this one? I'm happy to submit a PR, since I'm probably going to fork this, add HEAD support, and then use the fork in one of my projects anyway. (It's a project that is under a lot of security scrutiny internally, and our organization's internal app security scanner looks to include a bunch of tests that involve HEAD requests; when these hit my /login route, they generate HTTP 500s that then trigger app performance monitor alerts, etc.)

[panva]

I cannot imagine a scenario where a proper user-agent redirect would trigger a HEAD request. I suggest you dig deeper to find out why that's happening before entertaining this in a fork.

[delfuego]

It's not a user-agent redirect, it's the user agent specifically and natively making a HEAD request. A bunch of security scanners do it (hoping to trigger some sort of insecurity in login endpoints, I would presume), and a bunch of search crawlers do it (since it's the best way to see if the last-modified timestamp of content has changed since the search engine previously indexed it without having to also download the full HTTP body). And realistically, I can't think of why one wouldn't want to support it, or any downsides; the HTTP HEAD request semantics are identical to HTTP GET with the exception of explicitly not sending a body. And then ExpressJS explicitly calls the app.get() function for all HTTP HEAD requests to an endpoint unless the app defines an app.head() function for that endpoint prior to any app.get() function. (It's this latter part that caused me to notice this happen in my logs.) Ultimately, it seems like handling the HEAD request properly, rather than throwing an exception, would be better.

Looking over the other top Passport auth strategies (passport-http-lib, passport-jwt, passport-local, passport-oauth2, passport-facebook, passport-google-oauth1, passport-google-oauth2, passport-twitter, passport-auth0), none of them make a distinction between the incoming HTTP method; it's unique to this strategy to throw an exception for an HTTP HEAD request. So it might align with community expectations as well.

Ultimately, the option exists for an app to explicitly define an app.head() function to handle the same endpoints that handle passport authentication. But there'd be no way to truly adhere to HTTP HEAD semantics for an endpoint that's supposed to land on this strategy, though, since the spec really wants an app to identically handle the request in all ways but generating a body — and the app.head() function couldn't call this passport strategy in any way without throwing the same error.

[delfuego]

Just re-upping the above; I'm seeing a few daily HEAD requests to our /login endpoint (that lands in the OIDC Passport strategy) daily. I'd love these to do the right thing and not result in HTTP 500 errors. (The clients that are making the requests are a security scanning service that's used by the government, and at least one site indexer that I don't know any more about.)

I'm happy to just use my fork that adds the single case line which sends HEAD requests down the same switch logic as GET requests, but I'd love to not have to then maintain the fork forevermore.

[panva]

The strategy has nothing to do with a HEAD request. It rightfully errors because it's not a valid request. Having a HEAD switch that will ultimately fail for these types of requests you're talking about is just pushing the problem forward. The right thing to do would be to handle the errors using the means either passport or express has.

[delfuego]

But it is a valid request; there's nothing invalid about HEAD requests, and specifically, the HTTP spec says that HEAD requests should be handled semantically identically to GET requests with the single exception that they should not return a response body. I'm not sure why you say that a HEAD request will ultimately fail; you can see in my fork that there's no failure at all; Passport/Express happily handle the request just fine, and the library's tests all complete without error.

So in the end, there are only three options for handling a HEAD request to a route that's handled by a passport.authenticate call which lands in the OpenID strategy:

1. Exactly as it's being done now, by throwing an error in the strategy, which might or might not then get managed by Passport's or Express's error handling. But again, if a route considers a GET response as valid, then is should/must also consider a HEAD response as valid, so it doesn't make sense at all to throw an error. I presume this is why Express's default functionality is to use a matching app.get handler for any HEAD request that doesn't have an app.head handler explicitly defined, and I also presume this is why I can't find any other Passport strategies which thrown an error for HEAD requests.
2. Add an explicit app.head handler for the route. But this would be wrong, since a HEAD request is supposed to be handled semantically identically to a GET request while droping the response body, meaning that the explicit app.head handler would have to include the call to passport.authenticate... and we'll hit the exact same error in the OIDC strategy that we're hitting in the app.get call.
3. Add proper handling of the HEAD request method in the Passport strategy. This only involves adding the additional case to the switch, doesn't cause any downstream errors, and all the library's tests continue to pass.

In the end, this isn't the sort of thing that's critical; it just leads to unnecessary errors, and if those bother us enough we'll just add an explicit HEAD handler that makes clear that a HEAD call to a Passport-OIDC-strategy-backed route isn't semantically doing the right thing and a client shouldn't expect it to. But it'd be nice if the library supported it, so that that isn't needed.