AllowOriginRequestFunc considered harmful (may lead to Web cache poisoning)

[jub0bs]

PR #60 introduced AllowOriginRequestFunc, a field on cors.Options that lets users specify a custom function of the following signature:

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

As indicated by test cases that were added at the same time, 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):

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

(Let's ignore the expensive regexp re-compilation at each invocation of the resulting CORS middleware and the excessive permissiveness of the regexp in question, here.)

Unfortunately, as I've written elsewhere, AllowOriginRequestFunc 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, AllowOriginRequestFunc should provide rs/cors 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.

[rs]

There is a new AllowOriginVaryRequestFunc that will update the Vary and an update to the doc saying to use it if headers are used for the condition. I can't remove the other option as it would be a breaking change.

[jub0bs]

@rs I can't think of a way to fix AllowOriginRequestFunc without triggering a major-version bump either. The best thing you can do is deprecate it.