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

Allowing methods that are not uppercase should be possible but isn't #27

Open
jub0bs opened this issue Dec 15, 2023 · 1 comment
Open

Comments

@jub0bs
Copy link

jub0bs commented Dec 15, 2023

Although method names are case-sensitive, go-chi/cors takes the unusual approach of normalising method names by uppercasing them:

// Spec says: Since the list of methods can be unbounded, simply returning the method indicated
// by Access-Control-Request-Method (if supported) can be enough

Such unwarranted case normalisation causes problems for clients that send requests whose method is not uppercase—and not some case-insensitive match for one of DELETE, GET, HEAD, OPTIONS, POST, or PUT, names for which the Fetch standard carves out an exception. Here is an example:

c := cors.New(cors.Options{
  AllowedOrigins: []string{"https://example.com"},
  AllowedMethods: []string{"patch"},
})

Assume then that a client running in the context of Web origin https://example.com in a Fetch-compliant browser sends a patch request (note the lower case) to the server. As go-chi/cors internally normalises method names to uppercase, the preflight response would contain

Access-Control-Allow-Methods: PATCH

as opposed to

Access-Control-Allow-Methods: patch

Browsers rule this as a mismatch and fail CORS preflight with an error message of this kind:

Access to fetch at [REDACTED] from origin ‘https://example.com’ has been blocked by CORS policy: Method patch is not allowed by Access-Control-Allow-Methods in preflight response.

For this reason, go-chi/cors should not normalise method names. More about this in one of my recent blog posts.

@jub0bs
Copy link
Author

jub0bs commented Dec 17, 2023

Here is a (failing) test case that illustrates the issue:

func TestHandlePreflightLowercaseAllowedMethod(t *testing.T) {
	const (
		origin = "https://foo.com"
		method = "patch"
	)
	s := New(Options{
		AllowedOrigins: []string{origin},
		AllowedMethods: []string{method},
	})
	res := httptest.NewRecorder()
	req, _ := http.NewRequest(http.MethodOptions, "http://example.com/foo", nil)
	req.Header.Add("Origin", origin)
	req.Header.Add("Access-Control-Request-Method", method)

	s.handlePreflight(res, req)

	assertHeaders(t, res.Result().Header, map[string]string{
		"Vary":                         "Origin, Access-Control-Request-Method, Access-Control-Request-Headers",
		"Access-Control-Allow-Origin":  origin,
		"Access-Control-Allow-Methods": method,
	})
}

Result:

$ go test -run ^TestHandlePreflightLowercaseAllowedMethod$ github.com/go-chi/cors 
--- FAIL: TestHandlePreflightLowercaseAllowedMethod (0.00s)
    cors_test.go:30: Response header "Access-Control-Allow-Methods" = "PATCH", want "patch"
FAIL
FAIL    github.com/go-chi/cors  0.277s
FAIL

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

1 participant