Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apollo-server: Disable "x-powered-by" header #3821

Conversation

pauloedurezende
Copy link
Contributor

@andrewmcgivery
Copy link
Contributor

andrewmcgivery commented Feb 24, 2020

Just curious if this should be more discussed per my comment in #3812 instead of just rushing in a root level property.

More specifically, would it be useful to be able to both add and remove headers through something like this instead of just disabling headers.

Maybe something like this?

headers: {
   add: [{'X-Content-Type-Options', 'nosniff'}]
   remove: ['x-powered-by']
}

@abernix
Copy link
Member

abernix commented Feb 24, 2020

I don't think a top-level attribute to the ApolloServer class is the right way to achieve this (nor do I think disable as a term provides near enough clarity as to what that option would be meant to achieve).

While it's not currently the case, the ApolloServerCore class is meant to be as HTTP-agnostic as possible. Future versions of Apollo Server will make this even more strict to ensure we don't conflate transport concerns with GraphQL server functionality.

I do think that the ability to add headers is a great option for the transport, but that seems it should be accomplished in a transport-specific way on the HTTP transport which #3576 introduced and that is part of the larger #3184 work in #2360. While it might be tempting to open a PR to that PR, I wouldn't suggest that just yet until we stabilize some of the 3.0 work, and most notably that transport.

How about changing this PR to resolve exactly what #3812 reported? Specifically, that means disabling the default Express header when using apollo-server (not any of other integrations) by adding app.disable('x-powered-by'); below this line:

const app = express();

Thoguhts?

@pauloedurezende
Copy link
Contributor Author

Guys, thanks to review this PR 😄

It ended up being my fault to pay more attention to the proposed alternative than to actually solve the specific problem, even more, focused only on one server, in this case, Express. I will make the necessary changes to be able to deliver the correction of the proposed problem... 😉

@pauloedurezende pauloedurezende force-pushed the feature/disable-selected-headers branch from 980610a to 9ad6680 Compare February 24, 2020 13:42
@abernix abernix changed the title [FEATURE] - Disable Defined Headers apollo-server: Disable "x-powered-by" header Feb 25, 2020
@abernix abernix changed the base branch from master to release-2.12.0 March 10, 2020 11:06
@abernix abernix force-pushed the feature/disable-selected-headers branch from bd9b435 to 4a94639 Compare March 10, 2020 11:08
@abernix abernix added this to the Release 2.12.0 milestone Mar 10, 2020
@abernix abernix merged commit 6597e39 into apollographql:release-2.12.0 Mar 10, 2020
@pauloedurezende pauloedurezende deleted the feature/disable-selected-headers branch March 10, 2020 17:30
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants