8000 Cache cannot distinguish between OPTIONS and GET · Issue #20202 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content
8000

Cache cannot distinguish between OPTIONS and GET #20202

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.

Sign up for GitHub

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

Jump to bottom
Closed
nico-incubiq opened this issue Oct 11, 2016 · 10 comments
Closed

Cache cannot distinguish between OPTIONS and GET #20202

nico-incubiq opened this issue Oct 11, 2016 · 10 comments
Labels
BC Break Bug HttpKernel

Comments

@nico-incubiq
Copy link
Contributor

Hello,

I just ran into an issue with Symfony 3.1.5 concerning the HttpCache mechanism, which sends back the content of a GET method while executing an OPTIONS call.
I have a JS application which calls an API. This app uses CORS preflight OPTIONS calls to determine if he can call the API. Cache is configured on some public APIs.

On a basic example here is what happens:

  • OPTIONS call from the JS app returns the right Response with headers such as "access-control-allow-methods: GET" and "cache-control: no-cache" (if I do it multiple times using Postman it always generates a new Response with the expected content, so that I am sure it is not cached)
  • GET call issued just after returns a Response with headers such as "cache-control: max-age=86400, public, s-maxage=86400" (this one is being cached by HttpCache, which stores it in a file on the server)
  • But when another computer accesses the page, the OPTIONS call returns a cached Response, with "cache-control: max-age=86400, public, s-maxage=86400"

I found that because the Store::generateCacheKey() method only uses the Uri to compute the cache the key, then HttpCache thinks it has found a cached version of the OPTIONS call and sends back the headers and body of the GET call instead.

I think the generateCacheKey should at least use the method when computing the key, and maybe headers like Accept-*.
@stof
Copy link
Member
stof commented Oct 11, 2016

Accept-* should not be part of the cache key by default (but then, we should still be able to vary on it)

@nico-incubiq
Copy link
Contributor Author

Just a follow up on this one, I confirm the issue is not present on Symfony 3.1.2, so that I think this also comes from the same commit which caused #19582 (OPTIONS method is now considered as safe).

@dmaicher
Copy link
Contributor

I think its not as easy as just putting the request method into the cache key generation 😉 As far as I can see the HttpCache will try to return a cached reponse for a previous GET request when performing a HEAD request. So we might need some extra logic for OPTIONS requests then.

Added a test case to confirm the issue here: 78c4a0c

Headers should not be part of the cache key like @stof said. That's what the Vary headers are meant to be used for 😉

@xabbuh
Copy link
Member
xabbuh commented Oct 11, 2016

But OPTIONS requests are not cacheable. So the only thing we have to solve is to bypass the cache entirely in this case (thus no cache key building involved), right?

@dmaicher
Copy link
Contributor

@xabbuh ah good point 😊 👍 Will simply adjust my branch and create a PR then 😉

@dmaicher dmaicher mentioned this issue Oct 11, 2016
Merged
@dunglas
Copy link
Member
dunglas commented Oct 13, 2016 •
edited
Loading

Instead of hardcoding this behavior in the HttpCache class, we can add a new isMethodCacheable to the Request class. Such helper methods are very useful (for instance when building APIs).

@dmaicher
Copy link
Contributor

@dunglas agreed 😊 Makes sense. Updated my PR.

fabpot added a commit that referenced this issue Oct 14, 2016
@fabpot
bug #20205 [HttpCache] fix: do not cache OPTIONS request (dmaicher)
55c3f89 
This PR was merged into the 2.7 branch.

Discussion
----------

[HttpCache] fix: do not cache OPTIONS request

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #20202
| License       | MIT
| Doc PR        | -

The HttpCache should not cache any OPTIONS request as they are by spec not cacheable (mentioned here #20202 (comment) by @xabbuh).

Commits
-------

c43de7f [HttpCache] fix: do not cache OPTIONS request
@fabpot fabpot closed this as completed Oct 14, 2016
@teohhanhui
Copy link
Contributor
teohhanhui commented Oct 20, 2016 •
edited
Loading

@xabbuh:

But OPTIONS requests are not cacheable.

That should not apply so cleanly to a reverse caching proxy. CORS preflight requests hitting the app can take much longer than necessary, when they can actually be cached and served from the reverse proxy.

@xabbuh
Copy link
Member
xabbuh commented Oct 20, 2016

@teohhanhui I don't see where you can take that from the RFC. IMO it's very clear about the possibility to cache OPTIONS requests.

@teohhanhui
Copy link
Contributor
teohhanhui commented Oct 20, 2016 •
edited
Loading

@xabbuh The RFC sets out the rule for HTTP caching. The AppCache, like Varnish, is not (just) a HTTP cache. A reverse caching proxy is properly part of the web server because out-of-band information can be used to allow caching where it's not usually possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
BC Break Bug HttpKernel
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@fabpot @dunglas @stof @teohhanhui @dmaicher @xabbuh @nico-incubiq @carsonbot
0