AllowOriginFunc considered harmful (may lead to Web cache poisoning)

[jub0bs]

AllowOriginFunc, a field on cors.Options lets users specify a custom function of the following signature:

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

As indicated by relevant test cases, motivations for this feature include the desire to vary responses on the basis of the presence and/or value of some request headers (e.g. Authorization):

AllowOriginFunc: func(r *http.Request, o string) bool {
  return regexp.MustCompile("^http://foo").MatchString(o) &&
    r.Header.Get("Authorization") == "secret"
}

For this discussion, let's ignore

• the expensive regexp re-compilation at each invocation of the resulting CORS middleware,
• the excessive permissiveness of the regexp in question, and
• the fact that such a CORS middleware would be dysfunctional, since preflight requests never carry an Authorization header.

Unfortunately, as I've written elsewhere, AllowOriginFunc opens the door to Web cache poisoning:

If you vary responses on the basis of the presence and/or value of some request header, HTTP indeed mandates that you list the name of that header in your responses' Vary header; doing this signals to caching intermediaries that the header in question should be part of the cache key. But because the predicate lacks a http.ResponseWriter parameter, it gives you no way of populating the Vary header as required! You have to remember to manually do so outside of the predicate, somehow. If you forget, as I believe most developers do, to adequately populate the Vary header, caching intermediaries are then liable to serve inappropriate (and possibly malicious) cached responses to your clients.

For example, in the test case mentioned above, the response to an authenticated request (one containing request header Authorization: secret) may well get cached and subsequently get served by Web caches as a response to unauthenticated requests... 😬

To rule out Web cache poisoning, AllowOriginFunc should provide users a way to populate the Vary header accordingly. Note that this would require changing the type of that field, which would be a breaking change.

[jub0bs]

Related: rs/cors#157