Consider providing an option to disable the "x-powered-by" header

[scottohara]

Affected package/version: [email protected]

Issue description

When using apollo-server (without explicitly calling server.applyMiddleware()), the out-of-box configuration defaults internally to apollo-server-express.

Responses from this default server include the following response header:

X-Powered-By: Express

As per Express Production Best Practices: Security, consider disabling this header to avoid unnecessary disclosure to potential attackers that the server is running Express.

Alternatively, consider providing an option in the ApolloServer constructor to allow developers to opt into, e.g.

const server = new ApolloServer({
  ...opts,
  disable: 'x-powered-by'
});

The presence of this header is commonly flagged by security scanning tools such as Netsparker.

Steps to reproduce

Assume NODE_ENV=production (playground disabled).

Server setup

const { ApolloServer } = require('apollo-server');
const server = new ApolloServer({ ...opts });
(async () => {
	const { url } = await server.listen()
	console.log(`🚀  Server ready at ${url}`);
})()

Request

GET / HTTP/1.1
Host: localhost:4000
Accept: */*
Accept-Encoding: gzip, deflate
Cache-Control: no-cache
Connection: Keep-Alive

Response

HTTP/1.1 500 Internal Server Error
X-Powered-By: Express
Content-Type: application/json
Access-Control-Allow-Origin: *

Note the presence of the x-powered-by header in the response.

Additional notes

It is possible to disable the header when using apollo-server-express directly, e.g.

const express = require('express');
const { ApolloServer } = require('apollo-server-express');
const server = new ApolloServer({ ...opts });

const app = express();
app.disable('x-powered-by');  // <- ** HEADER DISABLED HERE **
server.applyMiddleware({ app });

app.listen({ port: 4000 }, () =>
  console.log(`🚀 Server ready at http://localhost:4000${server.graphqlPath}`)
);

However developers following the Getting Started guide, which uses code similar to the reproduction above, should be set up for success by defaulting to a server configuration that adheres to Express best practices for security.

[abernix]

PR welcome!

[andrewmcgivery]

@abernix I feel like there may be a few options here.

Option 1, does it make sense to introduce generic properties for adding and removing static headers? For example, I may want to add headers similar to ones that the Helmet package adds and I'm not sure if that is possible now if not using the express middleware.

Option 2 would be a apollo-helmet plugin... take the functionality of helmet and make it into an apollo plugin.

const helmet = require('helmet');

const ApolloHelmet = (options = {}) => {
  const helmetInstance = helmet(options);

  return {
    requestDidStart() {
      return {
        willSendResponse(requestContext) {
          const next = () => {};
          const req = {
            headers: requestContext.response.http.headers
          };
          const res = {
            setHeader: (name, value) => {
              requestContext.response.http.headers.set(name, value);
            },
            removeHeader: name => {
              requestContext.response.http.headers.delete(name);
            }
          };

          helmetInstance(req, res, next);
        }
      };
    }
  };
};

module.exports = ApolloHelmet;

This works for the most part except that the X-Powered-By isn't present when willSendResponse is called which makes me think that Express hasn't attached the header yet. Is this expected or possibly a bug?

Option 3 would be adding helmet as a dependency for apollo-server-express and attaching it to express by default, since it applies security best practices. This option is the most "forceful" option but not necessarily a bad idea as these headers are generally considered security best practices.

[abernix]

@andrewmcgivery Your proposal seems like a bigger conversation. Perhaps we can just disable this default Express header for now without any configuration? I don't think it adds much value in its current reveal of the server being "Express".

@scottohara, @pauloedurezende others: Thoughts of just disable the header for now, without a top-level configuration setting which I'd rather not introduce for this?

[abernix]

I think in the future, the transport abstraction (#3184, #3576, #2360) will be the place where any such default headers would be set. It is not currently an easily accomplished feat with the current integration packages.

[scottohara]

@abernix

I’d be happy with just disabling the x-powered-by header by default for now.

[abernix]

@scottohara Unless someone objects, I think that would be fine and welcome. However, it's worth noting that @pauloedurezende opened a PR recently which referenced this issue and I left a comment on it: #3821 (comment). Perhaps the adjustments can just be made to that PR, otherwise, a new PR would be welcome.

And if anyone watching objects to disabling this default header in the apollo-server integration, please speak up! (Also worth noting: anyone can disable this in apollo-server-express by calling app.disable('x-powered-by'); on their own Express app!)

[scottohara]

Yep, thanks @abernix; I noticed that PR was already up after my earlier comment (so I edited accordingly).

I’ll wait to see if the other PR progresses or stalls. Thanks.

[andrewmcgivery]

@abernix I'm good with the simple disable of the header change.

However, I'm still wondering if it is worth shipping apollo-server-express with the helmet middleware out of the box to encourage security best practices. Is there any downsides to that approach that you can think of?

[abernix]

@andrewmcgivery I agree with the idea generally, but introducing those middleware, which might be carefully placed in existing app's middleware chain, would be a breaking change for some deployments. Additionally, these might be more restrictive than are desired by some!

As noted above: in a not-too-distant version of Apollo Server, this functionality will all live in an HTTP-specific transport in future versions. We plan on providing a production best-practice / pre-flight guide to supplement that pattern, and including helmet in there seems reasonable, to me!

[andrewmcgivery]

@abernix Thanks!

[abernix]

#3821