rs/cors

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

jub0bs opened this issue · 5 comments

jub0bs commented

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

// Normalize options
// Note: for origins and methods matching, the spec requires a case-sensitive matching.
// As it may error prone, we chose to ignore the spec here.

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 rs/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, rs/cors should not normalise method names. More about this in one of my recent blog posts.

rs commented

Please submit a PR

jub0bs commented

@rs I would if I didn't have my own CORS middleware library for Go.

rs commented

Great mindset

  • 😕1
jub0bs commented

@rs No need to take it personally. I've already produced one CORS middleware library, and my time is limited. Besides, my opening GitHub issues doesn't imply that I'm going to fix them.

jub0bs commented

@rs I'm not sure how big of a deal this is, but directly assigning the Options.AllowedMethods slice directly to the cors.Methods field may be problematic. For example, through a reference to the cors.Options value passed to cors.New, users can now change elements of the cors.Methods slice after middleware instantiation:

opts := cors.Options{
    AllowedOrigins: []string{"http://example.com"},
    AllowedMethods: []string{"GET"},
}

c := cors.New(opts)

opts.AllowedMethods[0] = "PUT" // middleware c now allows method PUT instead of GET

Such mutability stands in the way of concurrency safety, and middleware are typically invoked in a concurrent setting. You may want to err on the side of caution and opt for some defensive copying:

c.allowedMethods = slices.Clone(options.AllowedMethods)

Just a thought.

  • 👍1