HTTP cache/CDN do not seem to be fully supported

Create issue
Issue #16 resolved
Former user created an issue

It looks like the CORSRequestHandler does not handle HTTP caches/CDNs well.

An example I found is this handling inside the handleActualRequest method:

response.addHeader("Access-Control-Allow-Origin", requestOrigin.toString());

From W3 spec

"Resources that wish to enable themselves to be shared with multiple Origins but do not respond uniformly with "*" must in practice generate the Access-Control-Allow-Origin header dynamically in response to every request they wish to allow. As a consequence, authors of such resources should send a Vary: Origin HTTP header or provide other appropriate control directives to prevent caching of such responses, which may be inaccurate if re-used across-origins."

My preference is to set the Access-Control-Allow-Origin header to the value of the configuration rather than the incoming Origin header, therefore reducing cache size in the instances where multiple origins may be used.

Comments (17)

  1. Vladimir Dzhuvinov
    • changed status to open

    Hi Chris,

    Thank you for posting this suggestion.

    I changed the code to return the general * instead of the Origin value, see commit e5b0176. The CORS spec allows a * only if credentials are not supported, otherwise the response value must match the received Origin value. If you're satisfied with this I'll merge the change and release a new version of the filter later today. If not we'll have to look at adding additional headers to control caching (when credentials are allowed). Let me know.



  2. Chris Chris

    Hi, yeah I'm kind of thinking we need to add to the Vary response header in the case we are responding with a specific Origin value - that's pretty much what the CORS spec suggests too. Your change looks good but maybe it won't work so well with an architecture that has user credentials and intermediate caches.

    The CORS spec seems pretty vague about user credentials so I'm not sure whether we need them enabled or not since our services have user credentials via the HTTP Authorization header but setting user credentials to false still seems to work fine.

  3. Vladimir Dzhuvinov

    I'll get in touch with the guys on the w3c WG regarding the Authorization header and whether it qualifies as a credential.

  4. Chris Chris

    OK, great thanks.

    I found which says "The .withCredentials property will include any cookies from the remote domain in the request, and it will also set any cookies from the remote domain. Note that these cookies still honor same-origin policies, so your JavaScript code can’t access the cookies from document.cookie or the response headers. They can only be controlled by the remote domain."

    So I know it's not definitive but judging from what others are saying it revolves around cookies rather than anything else. For now we've set user creds to disabled and just added the Authorization header to Access-Control-Allow-Headers.

    If you put out another release with the Origin change then we can use that and worry about the Vary header for user credentials later?

  5. Chris Chris

    Fantastic! That makes sense yes and roughly what I thought it would mean.

    I'm just trying the new filter and all is working as expected. I thought we could use your new feature to replay all the allowed headers from the request, however this does not work so well in this situation: * Multiple clients calling the same end point and including different sets under "Access-Control-Request-Headers" * Cacheable preflight requests

    Both of these add together to mean that end up caching the allowed headers based on the first client request and then other clients may not be accepted. It looks like we have to configure a specific list of headers which encapsulates all clients, which is fine just takes a little longer. I'm not sure if it's worth warning the library user in this configuration case.

    Shall we keep a separate issue open to set the Vary header to include Origin when credentials are turned on?

  6. Vladimir Dzhuvinov
    • changed status to open

    Hi Chris,

    I reopened the ticket to tackle the Vary header issue.

    What is your opinion - should we always include that with credentials or shall we add config setting to activate the Vary header?

  7. Chris Chris

    I think if credentials are enabled then it should be included, at least by default to agree well with the HTTP spec. If anything perhaps provide a setting to disable this behaviour? I say this because by default I think the filter should be as simple to install as possible and I think the default should be behaviour to correctly ensure any HTTP caches react well. Maybe it's not even a good idea to provide an ability to disable, I can't think of any use cases where the Vary header should not be set. Can you?

  8. Vladimir Dzhuvinov

    I decided to go along with the CORS spec suggestion and added Vary when credentials are allowed or the allowed origin list is bounded (commit 620677d). If someone files an issue with that later I'll revisit it.

  9. Vladimir Dzhuvinov

    The Vary support is now in version 1.9.2, which should reach Maven Central shorty.

    Ok, enough work for today :)

  10. Chris Chris

    Just wanted to add something to this. If your system has some clients using CORS and some not using it but with both sets of clients using the same URLs against the same CDN, then I think you will also need the latest 1.9.2 update in order to make sure the CDN knows how it can cache the content based on the Vary response header.

  11. Henry Johnson

    If the "Vary: Origin" header is only added when credentials are allowed or the allowed origin list is bounded, then it will not be added when credentials are not allowed and the origin list is unbounded.

    This is problematic in the situation described by Chris above, where there is a CDN and some clients use CORS and some do not.

    It is possible that non-CORS responses will be cached and served to CORS clients because the original non-CORS response doesn't include the "Vary: Origin" header.

  12. Log in to comment