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

add http://{127.0.0.1,localhost}[:5001] as allowed default CORS referer #1966

Closed
wants to merge 1 commit into from

Conversation

MichaelMure
Copy link
Contributor

Fix #1883 and #1839 for me.

License: MIT
Signed-off-by: Michael Muré mure.michael@gmail.com

License: MIT
Signed-off-by: Michael Muré <mure.michael@gmail.com>
@jbenet jbenet added the backlog label Nov 13, 2015
@rht
Copy link
Contributor

rht commented Nov 14, 2015

And ipfs/ipfs-webui#82

@ghost
Copy link

ghost commented Nov 16, 2015

I like!

Maybe we should do the same for the gateway? And also add :8080 to the API headers, to make the API-via-Gateway work?

@jbenet
Copy link
Member

jbenet commented Nov 16, 2015

@MichaelMure yeah-- there is something very odd going on. this is supposed to happen naturally: https://github.com/ipfs/go-ipfs/blob/27eece3e81f4e946db4294702c9ac39bd86ccce6/core/corehttp/commands.go#L70

@jbenet
Copy link
Member

jbenet commented Nov 16, 2015

im not sure adding this to config is the right way-- i think we should fix the bug in the code.

@ghost
Copy link

ghost commented Nov 16, 2015

Oh @jbenet is right, we should fix the code

@MichaelMure
Copy link
Contributor Author

Soooo, what happen is that the default origins are stored as string with a placeholder for the port (for instance http://127.0.0.1:<port>). The patchCORSVars is here to replace <port> with the correct port. This function use a sample net.Listener to find the right port. There is a race condition somewhere, and the port selected end up being 8080 instead of 5001.

@rht
Copy link
Contributor

rht commented Nov 21, 2015

patchCORSVars is called for each http api and gateway that there shouldn't be a race condition?

I tried fmt.Println-ing the port after this line, I got both 5001 and 8080.

@jbenet
Copy link
Member

jbenet commented Dec 1, 2015

ah interesting. that's likely the problem!

@jbenet
Copy link
Member

jbenet commented Dec 1, 2015

this is a priority fix as this is screwing up uses of the API. anybody have a bugfix?

@whyrusleeping if we find a bugfix in the next couple of days can we fit this into 0.3.10 along with the other api fix?

cc @dignifiedquire and @diasdavid as they're probably interested

@whyrusleeping whyrusleeping self-assigned this Dec 1, 2015
@daviddias
Copy link
Member

SGTM! :)

@dignifiedquire
Copy link
Member

Would love to have a fix for this yes, can't comment on race conditions in go code I'm afraid ;)

@rht
Copy link
Contributor

rht commented Dec 1, 2015

I noticed the defaultLocalhostOrigins slice was mutated in https://github.com/ipfs/go-ipfs/blob/792da9d87587e28d65b98a28fa4cc2d20388ccca/core/corehttp/commands.go#L95 regardless of the rwmutex protection from 4555085.

A possible fix: https://github.com/rht/go-ipfs/tree/fix/corsconfig (rht@e7dee98). I checked the c.AllowedOrigins(), with this fix they show 5001 for 8080 for each api and gateway. Previously it depends on which one mutates the slice first (the race condition described by @MichaelMure).
This means the rwmutex thing is probably unnecessary.

@rht rht mentioned this pull request Dec 1, 2015
@whyrusleeping whyrusleeping mentioned this pull request Dec 1, 2015
14 tasks
@parkan
Copy link

parkan commented Dec 9, 2015

+1 for this, webui is broken for me

@ghost ghost added the kind/bug A bug in existing code (including security flaws) label Dec 22, 2015
@daviddias daviddias removed the backlog label Jan 2, 2016
@jbenet jbenet closed this in #2021 Jan 22, 2016
@MichaelMure MichaelMure deleted the cors_config branch March 22, 2016 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants