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

gateway: fix CORs headers #5893

Merged
merged 4 commits into from
Jan 8, 2019
Merged

gateway: fix CORs headers #5893

merged 4 commits into from
Jan 8, 2019

Conversation

Stebalien
Copy link
Member

TODO:

  • Sharness tests.
  • Remove header defaults from the config.

License: MIT
Signed-off-by: Steven Allen <[email protected]>
@Stebalien Stebalien requested a review from Kubuxu as a code owner January 4, 2019 20:14
@ghost ghost assigned Stebalien Jan 4, 2019
@ghost ghost added the status/in-progress In progress label Jan 4, 2019
@Stebalien
Copy link
Member Author

cc @lidel, @popmanhe.

@Stebalien Stebalien added the need/review Needs a review label Jan 4, 2019
fixes #5138 -- always add user-agent to access-control-allow-headers.
fixes #5888 -- same with content-type.
fixes #5892 -- extend user-provided headers instead of overriding them.

License: MIT
Signed-off-by: Steven Allen <[email protected]>
License: MIT
Signed-off-by: Steven Allen <[email protected]>
c.Headers[h] = v
h = http.CanonicalHeaderKey(h)
switch h {
case cmdsHttp.ACAOrigin, cmdsHttp.ACAMethods, cmdsHttp.ACACredentials:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already skip these in the commands lib so this probably isn't necessary.

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are fixing CORS, it may be worth addressing a nuance related to #5138, #5892 and ipfs/in-web-browsers#132 (comment):

  • Access-Control-Allow-Headers is relevant only during OPTIONS request, can be skipped in other response types
  • Access-Control-Expose-Headers is relevant only during non-OPTIONS request (GET, POST etc), and can be skipped in OPTIONS response

Right now we return them for every request type which is really confusing and suggests brute-force approach to CORS. Would be nice to address this in default config and remove noise of irrelevant headers (test suggestions below)

grep "Access-Control-Allow-Headers:" curl_output
grep "< Access-Control-Allow-Origin: \*" curl_output &&
grep "< Access-Control-Allow-Methods: GET" curl_output &&
grep "< Access-Control-Allow-Headers: Range" curl_output &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Access-Control-Allow-Headers is relevant only during OPTIONS request, should not be returned with GET response:

Suggested change
grep "< Access-Control-Allow-Headers: Range" curl_output &&
grep -vL "< Access-Control-Allow-Headers" curl_output &&

Rationale: ipfs/in-web-browsers#132 (comment)

grep "< Access-Control-Allow-Origin: \*" curl_output &&
grep "< Access-Control-Allow-Methods: GET" curl_output &&
grep "< Access-Control-Allow-Headers: Range" curl_output &&
grep "< Access-Control-Expose-Headers: Content-Range" curl_output
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Access-Control-Expose-Headers is relevant only during non-OPTIONS request (GET, PUT etc), and should not be returned with OPTIONS response:

Suggested change
grep "< Access-Control-Expose-Headers: Content-Range" curl_output
grep -vL "< Access-Control-Expose-Headers" curl_output

Rationale: ipfs/in-web-browsers#132 (comment)

@Stebalien
Copy link
Member Author

While we are fixing CORS, it may be worth addressing a nuance related to #5138, #5892 and ipfs/in-web-browsers#132 (comment):

We should fix this but it may require a bit of refactoring to get it right (beyond the scope of this PR).

@daviddias
Copy link
Member

@Stebalien is it the right time to ask that the IPFS config.json gets organized by system rather than a mix of everything. By this I mean, have a

{
  libp2p: {
    // All things libp2p
  },
  Gateway: {
    // All things gateway
  },
  RemoteAPI: {
    All things RemoteAPI
  }
}

This would be a great improvement to create easy to parse tutorials. Right now we see things such as:

  "Gateway": {
    "APICommands": [],
    "HTTPHeaders": {
      "Access-Control-Allow-Headers": [
        "X-Requested-With",
        "Range"
      ],
      "Access-Control-Allow-Methods": [
        "GET"
      ],
      "Access-Control-Allow-Origin": [
        "*"
      ]
    },
    "PathPrefixes": [],
    "RootRedirect": "",
    "Writable": false
  },

Which raises questions such as "Why does the Gateway need CORS headers, it only receives GET requests?", "What is a Writable Gateway?" and more.

@lidel
Copy link
Member

lidel commented Jan 8, 2019

is it the right time to ask that the IPFS config.json gets organized by system rather than a mix of everything.

@daviddias I feel I miss some context here: what exactly feels to you like mixing concerns?

My understanding is that we already have separate namespaces for API and Gateway ports, and we are able to define different HTTPHeaders for each:

$ ipfs config --json Gateway.HTTPHeaders
$ ipfs config --json API.HTTPHeaders 

APICommands is present in Gateway section because it is an optional override of a "safe" API subset exposed on Gateway port (see below).

Which raises questions such as "Why does the Gateway need CORS headers, it only receives GET requests?"

CORS on Gateway is useful for API subset exposed there. Websites are able to read data via cross-origin XHR to https://ipfs.io/api/v0/dns/ipfs.io?r=true and AFAIK that is why our public Gateway at ipfs.io returns Access-Control-Allow-Origin: * currently.

Copy link
Contributor

@hsanjuan hsanjuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks OK for me (knowing it doesn't fix everything, it does improve some things).

@Stebalien
Copy link
Member Author

@daviddias, @lidel is correct but I do agree that the config could use a bit of a reorg with 20-20 hindsight (same as the commands). However, that would be a massively breaking change. Let's reconsider this when we reorganize the API/commands. When we do that, we can insert a config translation layer such that ipfs-old config sees the old config and ipfs-new config sees the new one.

@Stebalien Stebalien removed the need/review Needs a review label Jan 8, 2019
@Stebalien Stebalien merged commit 42a15ba into master Jan 8, 2019
@ghost ghost removed the status/in-progress In progress label Jan 8, 2019
@daviddias daviddias deleted the fix/gateway-headers branch January 9, 2019 10:49
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
gateway: fix CORs headers

This commit was moved from ipfs/kubo@42a15ba
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants