Add Vary headers in responses
#536
Conversation
server/handlers.go
Outdated
| @@ -243,6 +243,8 @@ func canContainsXSS(contentType string) bool { | |||
|
|
|||
| /* The preview handler will show a preview of the content for browsers (accept type text/html), and referer is not transfer.sh */ | |||
| func (s *Server) previewHandler(w http.ResponseWriter, r *http.Request) { | |||
| w.Header().Set("Vary", "Accept, X-Decrypt-Password") | |||
There was a problem hiding this comment.
previewHandler should not be sensible to X-Decrypt-Password
There was a problem hiding this comment.
indeed not event to Accept, or do I miss anything?
There was a problem hiding this comment.
I think the Vary header should always be the same for the same URL.
There was a problem hiding this comment.
I think the Vary header should always be the same for the same URL.
I had this doubt just after leaving the comment
the for previewHandler, getHandler and headHandler we must set Range, X-Decrypt-Password
for viewHandler only Accept
There was a problem hiding this comment.
viewHandler is only for /, yes? I don't know if it should vary based on X-Decrypt-Password, only for the file route.
Is that okay?
There was a problem hiding this comment.
Should be fixed in the latest commit
There was a problem hiding this comment.
sorry, file route should have w.Header().Set("Vary", "Range, Referer, X-Decrypt-Password")
There was a problem hiding this comment.
Sorry, I thought getHandler was the file route, do you mean zipHandler, tarHandler, tarGzHandler, etc should also have the vary header?
There was a problem hiding this comment.
these three ones:
https://github.com/dutchcoders/transfer.sh/pull/536/files#diff-961509ed3ff668f3172148150903514fb3394d6ff090d6db78d986875b13689fR246
https://github.com/dutchcoders/transfer.sh/pull/536/files#diff-961509ed3ff668f3172148150903514fb3394d6ff090d6db78d986875b13689fR1161
https://github.com/dutchcoders/transfer.sh/pull/536/files#diff-961509ed3ff668f3172148150903514fb3394d6ff090d6db78d986875b13689fR1255
currently they have w.Header().Set("Vary", "Accept, Referer, X-Decrypt-Password")
it should be w.Header().Set("Vary", "Range, Referer, X-Decrypt-Password")
response changes based on Accept only for / (viewHandler)
reponse for /{token}/{filename} changes Range, Referer, X-Decrypt-Password
Range and X-Decrypt-Password are used on getHandler
according to the Referer value we route to previewHandler or getHandler
finally headHandler should have the same Vary value since this should be the same based on URL and not URL+HTTP Method
There was a problem hiding this comment.
Got it, so sorry for the confusion!
Made the changes you requested
I'm not 100% sure about the cloudflare behaviour on the If not, and they only "react" to |
I've only just discovered this, but apparently Cloudflare only allows Vary for images on the non-free plans? :( But normally, Cloudflare and any server should not cache if the headers in |
yes, I'm fine adding the |
|
@kotx could you please merge the main branch on yours? |
Done, hope this works |
This prevents caching servers like Cloudflare from caching responses when
AcceptorX-Decrypt-Passwordrequest headers vary. Before this, servers like Cloudflare would cache the html preview page regardless ofAcceptheader (meaning curls and non-browser requests would also return a cached preview page). Adding aVaryheader should prevent that.More info here: https://blog.cloudflare.com/vary-for-images-serve-the-correct-images-to-the-correct-browsers/