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

content-type to allowedHeadersArr #5888

Closed
wants to merge 1 commit into from
Closed

Conversation

popmanhe
Copy link

@popmanhe popmanhe commented Jan 3, 2019

When using browser(chrow, firefox) to request a resource from IPFS, browser uses a preflight request with content-type header. So without content-type in the allowed list, the request will fail.

When using browser to request resource from IPFS, browser uses a preflight request with content-type header. So without content-type in the allowed list, the request will fail.
@popmanhe popmanhe requested a review from Kubuxu as a code owner January 3, 2019 05:11
@Stebalien
Copy link
Member

This shouldn't be necessary according to the second paragraph here: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Headers

@Stebalien Stebalien added the need/author-input Needs input from the original author label Jan 3, 2019
@popmanhe
Copy link
Author

popmanhe commented Jan 3, 2019

This shouldn't be necessary according to the second paragraph here: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Headers

I am developing a web site. and from that web site, there is a request that links to a IPFS gateway. So it is a CORS request. But without content-type added to the allowed header in IPFS config, the request fails.

The following message is from Chrome. It's the same in firefox.

Access to fetch at 'http://localhost:8080/ipfs/QmYwdHKn3bGePULMqhqmTiM2JjQPEc8sYwEuDYDaa2DP6r' from origin 'http://localhost:3000' has been blocked by CORS policy: Request header field content-type is not allowed by Access-Control-Allow-Headers in preflight response.

@Stebalien Stebalien removed the need/author-input Needs input from the original author label Jan 3, 2019
@@ -190,6 +190,7 @@ func (i *gatewayHandler) getOrHeadHandler(ctx context.Context, w http.ResponseWr
"Content-Range",
"X-Chunked-Output",
"X-Stream-Output",
"content-type"
Copy link
Member

@Stebalien Stebalien Jan 3, 2019

Choose a reason for hiding this comment

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

Suggested change
"content-type"
"Content-Type",

(to be consistent)

Copy link
Member

Choose a reason for hiding this comment

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

Also needs to end with a comma.

@Stebalien
Copy link
Member

Ah, I see. That only applies to form data and text. I assume you're trying to write to a writable gateway, right?

@lidel or @hsanjuan you both know more about this than I do, does this look correct? That is, should users be able to POST with arbitrary content types to a writable gateway. As far as I can tell, the answer is yes.

@Stebalien Stebalien added the need/review Needs a review label Jan 3, 2019
@popmanhe
Copy link
Author

popmanhe commented Jan 3, 2019

No. It's just a GET request. Just it's a CORS request.
I am using brower's native fetch method to get the content from IPFS gateway
const response = await fetch(url, {
headers,
mode: 'cors',
method: "GET"
});

@Stebalien
Copy link
Member

Wait, are you passing a Content-Type header?

@hsanjuan
Copy link
Contributor

hsanjuan commented Jan 4, 2019

I don't see anything wrong with this PR. The gateway sets correct Content-Type headers (well, http.ServeContent does). However, in Chrome these are probably not visible in Ajax requests because they are not part of the Access-Control-Expose-Headers (I think Firefox did not care about these things last time I looked). As a side effect this will also put them in Allow-Headers but that should be ok too.

So LGTM (after the small consistency change).

@hsanjuan
Copy link
Contributor

hsanjuan commented Jan 4, 2019

Also, might kill this one too #5138

Stebalien added a commit that referenced this pull request 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]>
@Stebalien Stebalien mentioned this pull request Jan 4, 2019
2 tasks
@ghost ghost assigned Stebalien Jan 4, 2019
@ghost ghost added the status/in-progress In progress label Jan 4, 2019
@Stebalien
Copy link
Member

I'm going fix this all at once in #5893.

@Stebalien Stebalien closed this Jan 4, 2019
@ghost ghost removed the status/in-progress In progress label Jan 4, 2019
@Stebalien
Copy link
Member

@popmanhe thanks for finding this.

Stebalien added a commit that referenced this pull request 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants