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

Allow CORS for gateway handler. #1444

Closed
wants to merge 1 commit into from
Closed

Conversation

d11e9
Copy link

@d11e9 d11e9 commented Jul 3, 2015

The gateway endpoint is read-only anyway. This enables gateways to service other sites on the regular internet.

@GitCop
Copy link

GitCop commented Jul 3, 2015

There were the following issues with your Pull Request

  • Commit: d62adb5
    • Invalid signoff. Commit message must end with License: MIT
      Signed-off-by: .* <.*>

Guidelines are available to help. Your feedback on GitCop is welcome on this issue


This message was auto-generated by https://gitcop.com

@jbenet jbenet added the backlog label Jul 3, 2015
License: MIT
Signed-off-by: Doug A <[email protected]>
@Luzifer
Copy link
Member

Luzifer commented Jul 3, 2015

❤️

@carver
Copy link

carver commented Jul 3, 2015

Probably worth noting that (it appears) you can make the gateway writable with the ipfs daemon --writable startup.

Maybe you could disable the allow * if writable is enabled?

I think it's this config: https://github.com/d11e9/go-ipfs/blob/gateway-cors/core/corehttp/gateway_handler.go#L80

@jbenet
Copy link
Member

jbenet commented Jul 3, 2015

Probably worth noting that (it appears) you can make the gateway writable with the ipfs daemon --writable startup.

That got disabled temporarily-- we need to bring it back, and error out in the meantime

@@ -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: *?

@notslang
Copy link
Contributor

(ref #934)

@jbenet
Copy link
Member

jbenet commented Jul 28, 2015

Closed in favor of #1529 -- thanks though @d11e9 👍

@jbenet jbenet closed this Jul 28, 2015
@jbenet jbenet removed the backlog label Jul 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants