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 CORS headers to endpoint responses #168

Merged
merged 1 commit into from
Oct 10, 2015
Merged

Add CORS headers to endpoint responses #168

merged 1 commit into from
Oct 10, 2015

Conversation

connor4312
Copy link
Member

This adds CORS headers to all endpoint responses, resolving #167

@connor4312
Copy link
Member Author

Wercker failed, but not because of changes in this PR... builds and runs successfully

http get localhost:8000/skin/connor4312
HTTP/1.1 200 OK
Access-Control-Allow-Headers: Accept, Content-Type, Content-Length, Accept-Encoding
Access-Control-Allow-Methods: GET
Access-Control-Allow-Origin: *
Content-Type: image/png
Date: Fri, 25 Sep 2015 12:54:59 GMT
Transfer-Encoding: chunked
X-Requested: skin
X-Result: ok



+-----------------------------------------+
| NOTE: binary data not shown in terminal |
+-----------------------------------------+

@LukeHandle
Copy link
Member

I'm now away for a few days, but feel free to merge ahead of 2.8 as this has no config changes. I'm almost ready with new ansible stack to deploy all.

@clone1018
Copy link
Member

I'm rebuilding now, I tried to move us to Werckers docker based builds but it failed.

@clone1018
Copy link
Member

Is there not a way to modify the router at an early stage to always include the cors headers?

@LukeHandle
Copy link
Member

I could also add it to Varnish as well

@connor4312
Copy link
Member Author

Is there not a way to modify the router at an early stage to always include the cors headers?

As far as I can see from the docs, no. The best (?) way to do this is to have a hierarchal routing system and be able to apply middleware and other options to subtrees. Gorilla's mux doesn't seem to do this. Others might. If not it'd be an interesting exercise to build one.

@LukeHandle
Copy link
Member

I've run this by @clone1018 on IRC, going to add this tomorrow (ish?) Friday? Saturday? in Release-2.9:

Will probably actually combine the two...

func startServer(listen string) {
    r := Router{Mux: mux.NewRouter()}
    r.Bind()
    http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) {
        w.Header().Set("Access-Control-Allow-Origin", "*")
        w.Header().Set("Access-Control-Allow-Methods", "GET")
        w.Header().Set("Access-Control-Allow-Headers", "Accept, Content-Type, Content-Length, Accept-Encoding")
        r.Mux.ServeHTTP(w, req)
    })
    log.Printf("Starting on %s", listen)
    err := http.ListenAndServe(listen, nil)
    if err != nil {
        log.Fatal("ListenAndServe: \"%s\"", err.Error())
        os.Exit(1)
    }
}

Slightly neater we think, though still open for improvement:

01:37:53 Yeah it's better
01:38:05 I meant I think there would be a better way
01:38:08 Not that I knew of one

Does this trigger any better ideas @connor4312

@LukeHandle
Copy link
Member

I have taken from this and created the release-2.9.1 PR #170

Any thoughts?

@connor4312
Copy link
Member Author

It seems like a mildly hackish way to go about it, but it works. Really the "solution" would be a fancier routing system, which we don't necessarily need (at this point)

@LukeHandle LukeHandle merged commit ed097f3 into master Oct 10, 2015
@LukeHandle LukeHandle deleted the cors-headers branch October 10, 2015 21:16
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.

3 participants