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.

Sign up for GitHub

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

Jump to bottom

Allow/Expose-Headers headers overwrite user-provided ones #5892

Closed
hsanjuan opened this issue Jan 4, 2019 · 3 comments · Fixed by #5893
Closed

Allow/Expose-Headers headers overwrite user-provided ones #5892

hsanjuan opened this issue Jan 4, 2019 · 3 comments · Fixed by #5893
Assignees
@Stebalien
Labels
kind/bug A bug in existing code (including security flaws)

Comments

@hsanjuan
Copy link
Contributor

hsanjuan commented Jan 4, 2019

Version information: master

Type: bug?

Description:

Per https://github.com/ipfs/go-ipfs/blob/master/core/corehttp/gateway_handler.go#L197 we set Access-Control-Allow-Headers and Access-Control-Expose-Headers AFTER i.addUserHeaders(w).

Per https://github.com/ipfs/go-ipfs/blob/master/docs/config.md#gateway the users are told that they can customize these headers.

Per https://github.com/ipfs/go-ipfs-config/blob/e6bdf3c437bc6820f8d7e0cb337bf4b900fd1f1b/init.go#L67 the default configuration sets Access-Control-Allow-Headers, but whatever the user puts here they are overwritten (?).

Am I missing something or should i.addUserHeaders(w) happen afterwards setting the defaults?

cc. @lidel
@hsanjuan hsanjuan added the kind/bug A bug in existing code (including security flaws) label Jan 4, 2019
@Stebalien
Copy link
Member

No, this is a bug. I'm trying to fix it now.

Ironically, preflight requests still work (as far as I can tell) because we only override these headers in HEAD/GET, not in OPTIONS (where they're actually useful).

@Stebalien
Copy link
Member

This is a complete mess. We have different code for the gateway and the API and the API doesn't use the CORs library.

I'm not even sure where we should start untangling this mess.

Stebalien added a commit that referenced this issue Jan 4, 2019
@Stebalien
gateway: fix CORs headers
ad6373e 
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 issue Jan 4, 2019
Merged
2 tasks
@ghost ghost assigned Stebalien Jan 4, 2019
@ghost ghost added the status/in-progress In progress label Jan 4, 2019
Stebalien added a commit that referenced this issue Jan 4, 2019
@Stebalien
gateway: fix CORs headers
82629c0 
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]>
@lidel
Copy link
Member

lidel commented Jan 6, 2019

I think we should be smart when it comes to CORS and return headers only when they are actually meaningful for specific request type – wrote some notes in #5893 (review)

@ghost ghost removed the status/in-progress In progress label Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Stebalien Stebalien

Labels
kind/bug A bug in existing code (including security flaws)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

gateway: fix CORs headers ipfs/kubo
3 participants
@lidel @Stebalien @hsanjuan