Skip to content

Proxy server could cache http response when it is not private #20897

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
Merged
rullzer merged 3 commits into from
May 13, 2020
Merged

Proxy server could cache http response when it is not private #20897

rullzer merged 3 commits into from
May 13, 2020

Conversation

@kklem0
Copy link
Contributor

@kklem0 kklem0 commented May 10, 2020

Proxy server could cache http response when it is not private
Need this for nextcloud/mail#3071

@kklem0 kklem0 requested a review from ChristophWurst May 10, 2020 09:10
@kklem0
Copy link
Contributor Author

kklem0 commented May 10, 2020

See https://www.nginx.com/blog/nginx-caching-guide/

How Does NGINX Determine Whether or Not to Cache Something?

By default, NGINX respects the Cache-Control headers from origin servers. It does not cache responses with Cache-Control set to Private, No-Cache, or No-Store or with Set-Cookie in the response header. NGINX only caches GET and HEAD client requests. You can override these defaults as described in the answers below.

@kklem0
Copy link
Contributor Author

kklem0 commented May 10, 2020

Note that there are situations when proxy caching make sense like avatar, so I'm not 100% sure of this PR if we should just make everything only cache by browser or if we wanna have another function for private caching.

@kklem0 kklem0 force-pushed the bugfix/httpcache branch from 354f54f to 401210d Compare May 10, 2020 09:24
@rullzer
Copy link
Member

rullzer commented May 10, 2020

right yes. This was on my list for ages.

So avatars is not a good example. But the endpoints that server bundle js or compiled scss are public of course.

Would you mind adding an optional argument 'public' (set to false by default)?

@rullzer rullzer self-assigned this May 10, 2020
@rullzer rullzer added the 2. developing Work in progress label May 10, 2020
@kklem0 kklem0 force-pushed the bugfix/httpcache branch from 0cffd89 to e9be3a9 Compare May 10, 2020 18:24
@kklem0
Copy link
Contributor Author

kklem0 commented May 10, 2020

Don't think the failed tests are related.

@ChristophWurst
Copy link
Member

ChristophWurst commented May 12, 2020

Don't think the failed tests are related.

https://drone.nextcloud.com/nextcloud/server/28807/10/4

Bildschirmfoto von 2020-05-12 10-31-50

That one is :)

@ChristophWurst
Copy link
Member

ChristophWurst commented May 12, 2020

@rullzer backport?

@kklem0
Copy link
Contributor Author

kklem0 commented May 12, 2020

Don't think the failed tests are related.

https://drone.nextcloud.com/nextcloud/server/28807/10/4

Bildschirmfoto von 2020-05-12 10-31-50

That one is :)

Missed that, and was looking at the wrong test file.
Thanks!

@kklem0
Fix http cache test
979dd1b 
Signed-off-by: Clement Wong <[email protected]>
@kklem0 kklem0 force-pushed the bugfix/httpcache branch from 96ce64d to 979dd1b Compare May 12, 2020 09:50
@kklem0
Copy link
Contributor Author

kklem0 commented May 12, 2020

@rullzer backport?

Would be cool if it can be backported as I need to use it with the mail app and I'm hoping to ditch my custom code in it.

@rullzer
Copy link
Member

rullzer commented May 12, 2020

I'm not 100% sure about the backport. Since it changes the API.

Of course it just adds a parameter which php will happily swallow....

@ChristophWurst
Copy link
Member

ChristophWurst commented May 12, 2020

I'm not 100% sure about the backport. Since it changes the API.

If we don't want to change the API we should backport the default to private caching IMO.

@kklem0
Copy link
Contributor Author

kklem0 commented May 12, 2020

Shall I default to $public = true for now?

@rullzer
Copy link
Member

rullzer commented May 13, 2020

Nah leave it to false I'd say. Like in the worst case you don't cache something in the proxy that could be cached in theory.

@rullzer rullzer merged commit 4fbea31 into master May 13, 2020
@rullzer rullzer deleted the bugfix/httpcache branch May 13, 2020 06:27
@ChristophWurst
Copy link
Member

ChristophWurst commented May 13, 2020

/backport to stable19

@ChristophWurst
Copy link
Member

ChristophWurst commented May 13, 2020

/backport to stable18

@ChristophWurst
Copy link
Member

ChristophWurst commented May 13, 2020

/backport to stable17

@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented May 13, 2020

The backport to stable18 failed. Please do this backport manually.

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request May 13, 2020
Merged
@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented May 13, 2020

The backport to stable17 failed. Please do this backport manually.

@kklem0 kklem0 mentioned this pull request May 13, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

Assignees

@rullzer rullzer

Labels

2. developing Work in progress

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kklem0 @rullzer @ChristophWurst