failing local test since v1.11.0
alicebob opened this issue · 3 comments
Hi,
This test fails with the new release. It might very well be a broken test, but I don't see any change in the diff which would explain this.
We setup a request which is not allowed (wrong origin), and then we change the origin, and we should be fine.
In v1.10.1 I get:
harmen@ld:~/src/cors$ go test
2024/04/29 09:33:03 Handler: Preflight request
2024/04/29 09:33:03 Preflight aborted: origin 'https://foobar.com' not allowed
2024/04/29 09:33:03 Handler: Preflight request
2024/04/29 09:33:03 Preflight response headers: map[Access-Control-Allow-Credentials:[true] Access-Control-Allow-Headers:[Authorization] Access-Control-Allow-Methods:[GET] Access-Control-Allow-Origin:[https://ui.engagespark.com] Access-Control-Max-Age:[600] Vary:[Origin, Access-Control-Request-Method, Access-Control-Request-Headers]]
PASS
ok github.com/rs/cors 0.007s
In v1.11.0 I get:
harmen@ld:~/src/cors$ go test
2024/04/29 09:33:38 Handler: Preflight request
2024/04/29 09:33:38 Preflight aborted: origin 'https://foobar.com' not allowed
2024/04/29 09:33:38 Handler: Preflight request
2024/04/29 09:33:38 Preflight aborted: headers 'Authorization' not allowed
--- FAIL: TestThing (0.00s)
my_test.go:50: have []string(nil), want #[GET]
FAIL
exit status 1
FAIL github.com/rs/cors 0.007s
Any ideas? Thanks!
package cors
import (
"log"
"net/http"
"net/http/httptest"
"reflect"
"testing"
)
func TestThing(t *testing.T) {
c := New(Options{
AllowedHeaders: []string{
"Content-Type", // we allow application/json
"Authorization",
},
AllowedMethods: []string{"GET", "POST", "DELETE", "PUT"},
AllowedOrigins: []string{"http://localhost:8050", "https://ui.engagespark.com"},
AllowCredentials: true,
MaxAge: 10 * 60,
Logger: log.Default(),
})
r := http.NewServeMux()
m := c.Handler(r)
// Origin not accepted
req := httptest.NewRequest("OPTIONS", "/v1/user/", nil)
req.Header.Set("Content-Type", "application/json")
req.Header.Set("Origin", "https://foobar.com")
req.Header.Set("Access-Control-Request-Method", "GET")
req.Header.Set("Access-Control-Request-Headers", "Authorization")
res := httptest.NewRecorder()
m.ServeHTTP(res, req)
if have, want := res.Result().StatusCode, 204; have != want {
t.Fatalf("have %d, want %d", have, want)
}
if have := res.Result().Header["Access-Control-Allow-Methods"]; have != nil {
t.Fatalf("have %#v, want nil", have)
}
// Origin accepted
req.Header.Set("Origin", "https://ui.engagespark.com")
res = httptest.NewRecorder()
m.ServeHTTP(res, req)
if have, want := res.Result().StatusCode, 204; have != want {
t.Fatalf("have %d, want %d", have, want)
}
if have, want := res.Result().Header["Access-Control-Allow-Methods"], []string{"GET"}; !reflect.DeepEqual(have, want) {
t.Fatalf("have %#v, want #%v", have, want)
}
}
The middleware is working as expected.
For better performance, the middleware's behaviour was modified in v1.11.0 to adhere more closely than previous versions to the Fetch standard (the de facto standard for CORS). According to the latter, when browsers include an Access-Control-Request-Headers header in a preflight request, the value of that header consists of comma-separated lowercase tokens. Your test fails with rs/cors v1.11.0 because the preflight request you're simulating isn't valid (Authorization should be lowercase: authorization) and the middleware, now being aware of that, rejects the request.
The remedy to your failing test is simple:
- req.Header.Set("Access-Control-Request-Headers", "Authorization")
+ req.Header.Set("Access-Control-Request-Headers", "authorization")Why you have tests that check the behaviour of rs/cors is unclear to me, though...
Incidentally, you should avoid reflect.DeepEqual in tests if you can; its semantics are ill-defined. In this case, under the assumption that you rely on Go 1.21 or above, you can use slices.Equal instead.
That fixes it, thanks!
Why you have tests that check the behaviour of rs/cors is unclear to me, though...
Cors is not easy, so I want to confirm I hooked up everything correctly. Also if we ever change to a brand new cors library cough https://lobste.rs/s/13pci5/jub0bs_cors_new_cors_middleware_library cough I can easily verify it behaves similarly.
you should avoid reflect.DeepEqual
I usually use a assert library, just wrote something to get a standalone test here.