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 CORS for gateway handler. #1444

Closed
wants to merge 1 commit into from
Closed

Allow CORS for gateway handler. #1444

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/corehttp/gateway_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ func (i *gatewayHandler) getOrHeadHandler(w http.ResponseWriter, r *http.Request
}

w.Header().Set("X-IPFS-Path", urlPath)
w.Header().Set("Access-Control-Allow-Origin", "*")
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't set it always like this. this should be an option.

Also, like the API (see commands/http/handler.go) we may have to set the incoming domain, as * doesn't work for certain things.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any use-case for treating browsers differently than non-browser clients?

Copy link
Member

Choose a reason for hiding this comment

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

Is there any use-case for treating browsers differently than non-browser clients?

Where are we treating them differently?

Copy link
Contributor

Choose a reason for hiding this comment

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

Non-browser clients don't read CORS headers, so setting it to anything other than Access-Control-Allow-Origin: * would be treating them differently. So unless there's a use-case for treating them differently, we should be able to always set it like the line we're referencing does.

Copy link
Member

Choose a reason for hiding this comment

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

@slang800

this applies to browsers and non-browsers alike.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, of course - adding configuration options makes the program more complex both to use and maintain. And in this case, being able to define arbitrary headers moves into the territory of softcoding.

I think that adding arbitrary headers should be handled by another tool (like nginx), because changing response headers has nothing to do with IPFS. We should just provide reasonable defaults and let more advanced things be handled by tools that are made to deal with them.

On Jul 7, 2015 9:04 PM, "Juan Batiz-Benet" [email protected] wrote:

In core/corehttp/gateway_handler.go
#1444 (comment):

@@ -133,6 +133,7 @@ func (i *gatewayHandler) getOrHeadHandler(w http.ResponseWriter, r *http.Request
}

w.Header().Set("X-IPFS-Path", urlPath)
  • w.Header().Set("Access-Control-Allow-Origin", "*")

@slang800 https://github.com/slang800 do you lose anything if we allow
you to specify gateway headers thus:

ipfs config Gateway.Headers.Access-Control-Allow-Origin "*"

or

ipfs config Gateway.Headers.Access-Control-Allow-Origin ""

or

ipfs config Gateway.Headers.Access-Control-Allow-Origin "mysite.com"

?

—
Reply to this email directly or view it on GitHub
https://github.com/ipfs/go-ipfs/pull/1444/files#r34108168.

Copy link
Member

Choose a reason for hiding this comment

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

strongly disagree.

the IPFS gateway is an HTTP server like any other. People will run into annoying configuration problems with it.

ideally for us, IPFS itself would remain completely unaware of any garbage like having to worry about CORS headers, and instead provide the user the tooling necessary to solve their own problem, whatever it might be. different deployments will call for different things.

adding configuration options makes the program more complex both to use and maintain.

so does adding some options and not others. it creates management and choice complexity on our end. instead, forwarding configurations obviates our decision making, and is a unix-tool thing to do, too.

softcoding.

i disagree with much in this article. a counter example is auth tokens and private keys.

it's also not very applicable, as this is not "specific, soft coded business logic complicating one group's deployments", but rather facilities in a general tool dealing with the concerns of many different types of users with many different types of concerns. In the quest of not solving everybody's problems, people tend to solve a few people's problems and leave it at that. Instead, we can sidestep the issue and let the users solve their own problems.

I think that adding arbitrary headers should be handled by another tool (like nginx),

Sure, and this applies to CORS headers too.

But it's a common enough thing that it was worth considering

Copy link
Contributor

Choose a reason for hiding this comment

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

People will run into annoying configuration problems with it.

But will they run into problems caused by Access-Control-Allow-Origin: *? I don't think that they will, because I don't see a use-case for treating browsers differently than non-browser clients.

and instead provide the user the tooling necessary to solve their own problem, whatever it might be.

But that is what tools like nginx, apache, tiny-proxy, and a myriad of others are built for. It's pointless to duplicate this kind of functionality, especially when these tools are so much more powerful. They let you add headers based on conditions & locations, control access by IP &/or interface, and even allow/disallow endpoints. It's just like what you're proposing with ipfs config Gateway.Headers..., except it's decoupled from ipfs, it's code that doesn't have to be maintained as part of ipfs, and it's a tool built for that specific purpose, so it's naturally better at the job.

a counter example is auth tokens and private keys

Tokens and keys are usually different for every user/instance/deployment - they need to be changed in the normal operation of the application. Headers that the HTTP server sends are not.

facilities in a general tool dealing with the concerns of many different types of users with many different types of concerns

It shouldn't be a tool for many different types of concerns - it should only deal with ipfs 😛. Anything else should be done by another tool. To quote Doug McIlroy: "Make each program do one thing well. To do a new job, build afresh rather than complicate old programs by adding new features".

Sure, and this applies to CORS headers too. But it's a common enough thing that it was worth considering

Yes, it does apply to CORS headers, which is why I think we should just provide a reasonable default and leave it at that.

Copy link
Member

Choose a reason for hiding this comment

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

I think we have very different understandings of how unix tools work. you think that by forwarding configs we're making ipfs "do more", when instead, i think, we're making it "do less". less even that what you propose ("always send Access-Control-Allow-Origin: *", making ipfs know something about CORS, instead of nothing). this is what tool composition is about. i explained my reasoning earlier as clearly as I have time for for this issue. sorry if i didn't get through. my mind's pretty set on this path for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you suggesting that, by default, ipfs wouldn't send Access-Control-Allow-Origin: *?


// Suborigin header, sandboxes apps from each other in the browser (even
// though they are served from the same gateway domain). NOTE: This is not
Expand Down