Add an error message warning developers of oversized headers.

[shankwiler]

Currently if the headers exceed Node's set limit, the client will see a blank page with the text "Unexpected error." and nothing will be logged on the server. This PR adds a server-side log informing the developer of the cause and suggests using a node CLI option to increase the maximum allowable header size.

One scenario which leads to this error is if a project with many CSP rules upgrades from an older version of Meteor, prior to Node's reduction of the maximum allowable size from 80 KB to 8 KB (discussed here nodejs/node#24692).

This issue as it affects Meteor has been discussed here https://forums.meteor.com/t/meteor-1-8-1-unexpected-error-too-many-csp-rules/48447.

[sebakerckhof]

LGTM, but maybe we should set HTTP_MAX_HEADER_SIZE to 8kb (during the test I mean), so the test is consistent in case the value should change in the future ( which seems to be discussed e.g. nodejs/node#27645 )

[shankwiler]

@sebakerckhof That's a good point. I pushed a change to address it.

[sebakerckhof]

Thanks. Just to make sure: you intended for this to only work in development mode (and not in a deployment?)

[shankwiler]

Hmm actually I had intended the logs to show up both in development and production, since the issue could occur in both places. I'm fine putting it behind a flag, though, since developers would likely discover the issue while in development mode.

[sebakerckhof]

As it is, it will currently only run in development mode, since the place you put it in (run-proxy) is only for development purposes. Basically this is a proxy server in front of the actual Meteor application server which handles a couple of things like error pages, requests during rebuilds etc.
This is not present in a production build.

I think (= haven't tested, just by looking at the code), since it passes by this proxy server first in dev mode, you would need to have the changes here anyway. And have the same changes somewhere in the webapp package for production mode.

[shankwiler]

Gotcha -- thanks for the thorough explanation. I will do some investigating to see how to address this for production builds.

[shankwiler]

@sebakerckhof Hey, I looked into whether an extra check is needed elsewhere, and found that when run in a production build the max header size is not actually an issue and the client to server requests are successful. I believe this is because node's http.CreateServer is only protecting against incoming requests with oversized headers, and the oversized headers occur during the outbound response from the server to the client.

My understanding is that this is an issue for dev because of the proxy which will be sent the oversized headers from the server, and throw an error because of them. This wouldn't be an issue in production.

So I think this change should be good as is, but feel free to let me know if my understanding is wrong or there is something I had not thought of.

[shankwiler]

@sebakerckhof I'm just checking in. Does the above make sense? I think it should be good to go.

[sebakerckhof]

Ok, thanks for the clarification. LGTM

[CLAassistant]

All committers have signed the CLA.

[filipenevola]

Thanks @shankwiler