[CORS] Old library makes exploitable CORS configuration (need to update dependency)

[hsanjuan]

Version information:

master

Type:

Bug

Description:

When Access-Control-Allow-Origin is set to * (think this is the default), the CORS library we use automatically sets it to the value of the Origin header of the request. However we also set or recommend to set Access-Control-Allow-Credentials: true (not sure why).

The result is that this is going around a browser security feature where Access-Control-Allow-Credentials: true is not allowed when Allow-Origin: *. The library we use for that patched this behaviour and we should upgrade accordingly.

More info: https://github.com/rs/cors#allow--with-credentials-security-protection (and linked issues and linked discussions from those issues).

Also, since the go-ipfs APIs have nothing with credentials, cookies etc, I'm not sure why Access-Control-Allow-Credentials: true is there (and the documentation invites the user to configure it at least - web UI too). There's probably a reason so I'd love to know (cc. @olizilla, @lidel)

Also not clear why Access-Control-Allow-Headers: X-Stream-Output, X-Chunked-Output, X-Content-Length is hardcoded, since probably (?) these headers are not relevant as part of requests made against the API (only responses, that's why Expose-Headers is set as well), but I might be wrong.

[lidel]

• 👍 for updating the rs/cors dependency

• I've never had Access-Control-Allow-Credential: true set up on my nodes, but now that I looked at docs and past issues.. indeed, suggestion to set it up is nearly everywhere.
  My guess is that it survived and multiplied due to compulsive copy&paste.
  We should remove it from places like ipfs-webui/README :)
  @olizilla is that ok, or am I missing something?

• AFAIK Access-Control-Allow-Headers are only meaningful during preflight (OPTIONS) requests:

  The Access-Control-Allow-Headers response header is used in response to a preflight [HTTP OPTIONS] request which includes the Access-Control-Request-Headers to indicate which HTTP headers can be used during the actual request. – #

  • Why is -Allow-Headers sent in replies to HTTP GET if Access-Control-Expose-Header is already present there? Probably hardcoded somewhere, or because ipfs config --json API.HTTPHeaders is a blanket setting applied to all request types and nobody bothered to clean this up.

[hsanjuan]

Ok, so my intuition is correct and fixing all this means:

• Access-Control-Allow-Credential: true should be removed from docs etc. (it's also on the --help cmd, apart from WebUI instructions page (when it cannot connect properly to the API) and README.
• There's no point in having Access-Control-Allow-Headers with the current allowed header set (or any other at this point) (go-ipfs hardcodes it), so it should be removed.
• Need to update rs/cors

[Stebalien]

When Access-Control-Allow-Origin is set to * (think this is the default)

This is the default for the gateway but not for the API. However, the gateway doesn't use the CORs library.

the CORS library we use automatically sets it to the value of the Origin header of the request.

go-ipfs should never set the Origin header. Do you mean the Access-Control-Allow-Origin header? If so, yes. If Access-Control-Allow-Origin is set to * and credentials are enabled, the CORs library will set Access-Control-Allow-Origin to the specific origin (because * is invalid in that case).

More info: https://github.com/rs/cors#allow--with-credentials-security-protection (and linked issues and linked discussions from those issues).

We don't really use credentials (cookies or otherwise) so this isn't really an issue for us. However, we should still upgrade (and probably shouldn't be allowing credentials when we don't use them).

Again note: this only affects the gateway.

Also not clear why Access-Control-Allow-Headers: X-Stream-Output, X-Chunked-Output, X-Content-Length is hardcoded, since probably (?) these headers are not relevant as part of requests made against the API (only responses, that's why Expose-Headers is set as well), but I might be wrong.

Those should have gone in the Exposed headers. This has now been fixed.

[Stebalien]

Although... we should be applying the gateway config to the gateway's API.

[hsanjuan]

We don't really use credentials (cookies or otherwise) so this isn't really an issue for us.

Might affect people who have put some middleware that uses credentials in front of the Gateway (?).

Again note: this only affects the gateway.

By default but potentially affects any user who has modified the API config to set Allow-Origin to "*" too. Anyway it's not super critical.

Although... we should be applying the gateway config to the gateway's API.

Yeah: #5909