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

Document differences between upstream github.com/rs/cors and this fork #21

Open
VojtechVitek opened this issue Sep 19, 2022 · 3 comments

Comments

@VojtechVitek
Copy link
Contributor

Hi fellow go-chi authors,

I was looking into why we created this fork in the first place.

Note: The upstream repo has a go-chi example at https://github.com/rs/cors/blob/master/examples/chi/server.go.

1. We have introduced this API breaking change:

 type Cors struct {
	// Optional origin validator function
-	allowOriginFunc func(origin string) bool
+	allowOriginFunc func(r *http.Request, origin string) bool
 }

=> It looks like upstream adopted this change via rs/cors#59

 type Cors struct {
	// Optional origin validator function
 	allowOriginFunc func(origin string) bool
+ 	// Optional origin validator (with request) function
+ 	allowOriginRequestFunc func(r *http.Request, origin string) bool
 }

2. We have introduced cors.Handler() function

+ // Handler creates a new Cors handler with passed options.
+ func Handler(options Options) func(next http.Handler) http.Handler

which returns middleware via cors.New(opts).Handler behind the scenes

3. We have removed few functions:

- // Default creates a new Cors handler with default options.
- func Default() *Cors

- // check the Origin of a request. No origin at all is also allowed.
- func (c *Cors) OriginAllowed(r *http.Request) bool

- // HandlerFunc provides Martini compatible handler
- func (c *Cors) HandlerFunc(w http.ResponseWriter, r *http.Request)

- // Negroni compatible interface
- func (c *Cors) ServeHTTP(w http.ResponseWriter, r *http.Request, next http.HandlerFunc)

4. Is there anything else I'm missing?

I wonder if you'd be OK with documenting these changes in the main README.

@pkieltyka
Copy link
Member

@VojtechVitek I believe there are even more differences between the libs, including dependency use, but its been a while since I checked. Certainly submit a PR to list the differences, I think that is a good idea

@pkieltyka
Copy link
Member

I checked, and I think in the past rs/cors had some extra dependencies, but those look to have been removed -- either way I prefer to keep this fork

@jub0bs
Copy link

jub0bs commented Nov 29, 2022

May I ask what prompted the signature of the allowOriginFunc field?

allowOriginFunc func(r *http.Request, origin string) bool

What do you need the http.Request for? Do you have real-world use cases you can point me to?

cors_test.go contains a couple of examples, in which you decide whether to allow the request in part on the basis of the value of the request's Authorization header. However, in that case, the response should contain Vary: Authorization; otherwise, you run the risk of cache poisoning. But to write that Vary response header, you'd need access to the http.ResponseWriter also. Therefore, the signature should really be

allowOriginFunc func(w http.ResponseWriter, r *http.Request, origin string) bool

or simply

allowOriginFunc func(w http.ResponseWriter, r *http.Request) bool

since the Origin header (if any) can be extracted from r. But then, you might as well implement a whole middleware in allowOriginFunc, since its signature now matches that of a http.HandlerFunc.

Perhaps I'm missing the point of having access to the request. I would appreciate your insight.

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

No branches or pull requests

3 participants