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

Allow configuring which headers are exposed #202

Closed
wants to merge 1 commit into from

Conversation

mitar
Copy link
Contributor

@mitar mitar commented Apr 30, 2016

This is a backwards compatible change but I really think that it should be allowed which headers are available to the program, for better or worse. In my case I need some x- prefixed headers exposed by our reverse proxy so there is no way somebody could fake them (it sanitizes and sets them).

Fixes #198.

@kentonv
Copy link

kentonv commented May 5, 2016

If the x- prefixed headers are themselves derived from cookies -- as is the case in Sandstorm -- then they are no more trustworthy than the cookie header, despite being added server-side.

@kentonv
Copy link

kentonv commented May 5, 2016

@mitar My understanding is that the "iframe" transports bypass CORS and will in fact deliver messages with cookies. These transports are intended to work on browsers that predate CORS.

I had exactly the same misunderstanding early on, too, and thought that whitelisting the Sandstorm headers would be safe. I'm glad sock.js never gave me the opportunity to shoot myself in the foot, and I think it's best to leave the whitelist non-configurable to minimize future foot-shooting by others.

@mitar
Copy link
Contributor Author

mitar commented May 5, 2016

I'm glad sock.js never gave me the opportunity to shoot myself in the foot

No, sock.js is choosing a particular design option here where it could be different. It seems that sock.js endpoints are by design allowing cross-origin requests: iframe allows messages from every origin, other requests even allow credentials (why, if cookies are stripped afterwards?). I can understand that allowing connecting for some apps to sock.js from other origins might be desirable, but in my opinion that should not be a default to begin with. And it is easy to make it not work like that:

  • you remove all res.setHeader('Access-Control-Allow-Credentials', 'true')
  • you require that all message from postMessage share the same origin as iframe (which you provide on your host), a simple e.origin === location.origin
  • alternatively, you could make origin server-side option which would be injected into bootstrap_iframe call, and only messages matching that origin would be processed further by the iframe

With this, only the app could talk back to sock.js. You lose sock.js "public API" nature so other sites cannot connect to it anymore, but for Sandstorm this does not work anyway, because it requires one to open URL through Sandstorm (or use public API URL). So, for Sandstorm in particular, all those holes could be closed, and then x- headers could be whitelisted and respected.

So sock.js could be allowing you to shoot yourself in the foot because it is choosing a different default (cross-origin allowed by default) than what people are used to normally on the web. If this would not be a surprising default, then also exposing headers would not have a surprising consequences.

@brycekahle
Copy link
Contributor

@mitar Unfortunately, I cannot accept this PR. It is too flexible, and would allow the user to bypass very important security restrictions. I do think a non-cross-origin mode is something we could add, but it needs some small changes to the /info endpoint to communicate that to the client, so it doesn't try to use any of the iframe transports.

@obonyojimmy
Copy link

Probably , then we should allow standardized headers like traceparent , tracestate #meteor/meteor-feature-requests#397

@mitar
Copy link
Contributor Author

mitar commented Apr 26, 2020

Yes, those might be possible to add. After this PR was denied, the following did get in: #212

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow configuring which connection headers are exposed
4 participants