go-chi / cors

CORS net/http middleware for Go
MIT License
332 stars 32 forks source link

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

Open VojtechVitek opened 2 years ago

VojtechVitek commented 2 years ago

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 https://github.com/rs/cors/issues/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 commented 2 years ago

@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 commented 2 years ago

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 commented 1 year ago

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.

VojtechVitek commented 4 months ago

Another difference -- this fork was not vulnerable to upstream security issue https://pkg.go.dev/vuln/GO-2024-2883.