cors(feature): adds cors support via textile config #387
Conversation
|
|
||
| control, ok := headers["Access-Control-Allow-Origin"] | ||
| if ok && len(control) > 0 { | ||
| cconfig.AllowOrigins = control |
There was a problem hiding this comment.
There was a problem hiding this comment.
should just be able to set AllowOrigins to[]string{"*"} but released version doesn't support that it seems.
core/api.go
Outdated
| if ok && len(control) > 0 { | ||
| cconfig.AllowMethods = control | ||
| } else { | ||
| cconfig.AllowMethods = []string{"GET", "POST", "DELETE", "OPTIONS"} |
There was a problem hiding this comment.
Should be good set of defaults?
core/api.go
Outdated
| cconfig.AllowHeaders = control | ||
| } else { | ||
| // default already has "Origin", "Content-Length", "Content-Type" | ||
| textileHeaders := []string{"Method", "X-Textile-Args", "X-Textile-Opts", "X-Requested-With", "Access-Control-Allow-Headers"} |
There was a problem hiding this comment.
Seems like a reasonable set? I think this is all we ever need to deal with?
| } else { | ||
| // By default use API origin host | ||
| defaultHost := config.Addresses.API | ||
| match, _ := regexp.MatchString("^https?://", defaultHost) |
There was a problem hiding this comment.
This feels messy to me, but the cors package requires http{s} prefixes on origins... ipfs for example, hard-codes:
localhosts: https://github.com/ipfs/go-ipfs/blob/af73c5097ce0ff70a18db25f801b34fa66a98202/core/corehttp/commands.go#L40
There was a problem hiding this comment.
leaving this in, seems to work, and is simpler than requiring prefixing etc...
|
Nice! Just took a quick look... I was imagining adding something to the textile config as defaults? Just so that people can see that as a tweakable setting when looking at the config... IPFS has these defaults for the gateway: |
|
Oh yeh for sure can add defaults to config... was thinking we'd want to keep these out, but that actually makes things simpler, so sure 👍. Also, I haven't touched the Gateway stuff yet, but can also add that to this branch. |
|
Ok nice. I'd say skip the gateway. I was just using that as an example snippet from the config... IPFS does't put those as defaults for the API... dunno why. |
|
probs because you don't want anything other than localhost to access your API 99% of the time? |
|
|
||
| control, ok := headers["Access-Control-Allow-Origin"] | ||
| if ok && len(control) > 0 { | ||
| cconfig.AllowOrigins = control |
There was a problem hiding this comment.
should just be able to set AllowOrigins to[]string{"*"} but released version doesn't support that it seems.
| } else { | ||
| // By default use API origin host | ||
| defaultHost := config.Addresses.API | ||
| match, _ := regexp.MatchString("^https?://", defaultHost) |
There was a problem hiding this comment.
leaving this in, seems to work, and is simpler than requiring prefixing etc...
| "Access-Control-Allow-Headers": []string{ | ||
| "Content-Type", "Method", "X-Textile-Args", "X-Textile-Opts", "X-Requested-With", | ||
| }, | ||
| "Access-Control-Allow-Origin": []string{}, |
There was a problem hiding this comment.
default is empty, which in turn means "http://127.0.0.1:40600" using current defaults...
Take a look at this one @sanderpick. Should be pretty straight forward. Should be reasonable defaults. Basically just adds relevant CORS headers as Gin middle-wear. By default, leaves Allowed-Origin as length-one slice with API address, and Methods and Headers response Headers contain what you'd expect. Easy to change defaults, tried to keep these out of the default config setup though.