Skip to content
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.

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

Add Cache-Control header to html and api responses, add no-transform #20432

Merged
merged 16 commits into from
Jul 23, 2022

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Jul 21, 2022

no-transform allegedly disables CloudFlare auto-minify and we did not set caching headers on html requests, which seems good to have regardless.

Transformation is still allowed for asset requests.

Fixes: #19309

`no-transform` allegedly disables CloudFlare auto-minify and we did not
set caching headers on html or api requests, which seems good to have
regardless.
@silverwind silverwind added the type/enhancement An improvement of existing functionality label Jul 21, 2022
@wxiaoguang
Copy link
Contributor

This PR will close #19309

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 21, 2022
@wxiaoguang wxiaoguang linked an issue Jul 21, 2022 that may be closed by this pull request
@silverwind silverwind changed the title Add Cache-Control header to html/api responses, add no-transform Add Cache-Control header to html responses, add no-transform Jul 21, 2022
@silverwind silverwind changed the title Add Cache-Control header to html responses, add no-transform Add Cache-Control header to html and api responses, add no-transform Jul 21, 2022
modules/context/context.go Outdated Show resolved Hide resolved
modules/httpcache/httpcache.go Outdated Show resolved Hide resolved
modules/httpcache/httpcache.go Outdated Show resolved Hide resolved
@silverwind
Copy link
Member Author

AddCacheControlToHeader now accepts a slice of string, so this should be very flexible now.

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Golang's nil can be used as empty slice directly, so no need to allocate memory for an empty slice when calling fn(..., []string{}).

Although I think argument additionalDirectives ...string could be more simple:

  • fn(..., time) instead of fn(..., time, nil)
  • fn(..., time, "no-transform") instead of fn(..., time, []string{"no-transform"})

Current approach is also fine to me.

routers/web/user/avatar.go Outdated Show resolved Hide resolved
modules/httpcache/httpcache.go Outdated Show resolved Hide resolved
modules/httpcache/httpcache.go Outdated Show resolved Hide resolved
modules/httpcache/httpcache.go Outdated Show resolved Hide resolved
modules/httpcache/httpcache.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 21, 2022
@silverwind
Copy link
Member Author

Golang's nil can be used as empty slice directly

Thanks, didn't know that. I'm still a golang noob 😉

routers/web/base.go Outdated Show resolved Hide resolved
modules/context/context.go Outdated Show resolved Hide resolved
routers/install/routes.go Outdated Show resolved Hide resolved
modules/context/api.go Outdated Show resolved Hide resolved
@zeripath
Copy link
Contributor

I think @wxiaoguang is right and we should use the varargs notation for this so I've pushed up a change to do that.

I guess one thing I'm wondering is if it's really correct that we don't want any caching of our generated html. I guess that's right but it seems bad.

@silverwind
Copy link
Member Author

There's not much point in allowing HTML cache that I see. Content is too dynamic to allow caching. Most pages change on every page load.

$ curl -s 'https://try.gitea.io/silverwind/symlink-test' | md5sum
c5adddf3c81b370ab1312edad42ad4dd  -
$ curl -s 'https://try.gitea.io/silverwind/symlink-test' | md5sum
2a2b1ac6f9bce1196d752b3891ce6ce5  -
$ curl -s 'https://try.gitea.io/silverwind/symlink-test' | md5sum
d0bff48164495658ab1c89c8eb60eb47  -

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 22, 2022
silverwind and others added 2 commits July 22, 2022 21:54
Signed-off-by: Andrew Thornton <art27@cantab.net>
@wxiaoguang
Copy link
Contributor

IMO one more thing that why Gitea need 'no-store' or 'no-cache': there are a lot of JS code. If the page is cached, then when users make the browser navigate back to a history page, if the page is cached, then the JS won't be executed, the UI may be stuck in a strange state. So Gitea should tell browsers to reload every page during navigation.

@silverwind
Copy link
Member Author

We are in good company regarding anti-cache headers on HTML:

gitlab: max-age=0, private, must-revalidate
github: no-store

@wxiaoguang wxiaoguang merged commit 14178c5 into go-gitea:main Jul 23, 2022
@silverwind silverwind deleted the no-tranform branch July 23, 2022 06:53
silverwind added a commit to silverwind/gitea that referenced this pull request Jul 23, 2022
…o-gitea#20432)

`no-transform` allegedly disables CloudFlare auto-minify and we did not
set caching headers on html or api requests, which seems good to have
regardless.

Transformation is still allowed for asset requests.

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: Andrew Thornton <art27@cantab.net>
zeripath added a commit that referenced this pull request Jul 23, 2022
…20432) (#20459)

`no-transform` allegedly disables CloudFlare auto-minify and we did not
set caching headers on html or api requests, which seems good to have
regardless.

Transformation is still allowed for asset requests.

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: Andrew Thornton <art27@cantab.net>
@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor

https://try.gitea.io/silverwind/symlink-test

sorry, late to the party: I checked and I think the only thing changing here (for consecutive requests) is the embedded csrf token, in fact most of the page stays the same (until something changes of course, a month from now the dates will show +1 in "X months ago") for quite some time.

do we know how these changes affect deployments with Gitea behind proxies?

won't adding no-transform tell proxies to stop compressing Gitea pages?

should no-cache really be added as a new hard-coded global default because some people didn't visit their CloudFlare settings page?
wouldn't it be better to have this tweakable from the config?

@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor

We are in good company regarding anti-cache headers on HTML:

gitlab: max-age=0, private, must-revalidate github: no-store

since Gitea is not a hosted service, I'd really prefer if this stayed user-tunable, e.g. in my set-up, Cache-Control headers are set on the proxy sitting in front of Gitea. granted, there are instances when it absolutely makes sense for the application to set these headers.

@silverwind
Copy link
Member Author

silverwind commented Jul 23, 2022

won't adding no-transform tell proxies to stop compressing Gitea pages?

Yes, it's a hint to not transform responses. Cloudfare does destructive HTML minification per default and that problem came up a few times in the past. I don't think no-transform should generally prevent compression via content-encoding as that it's non-destructive.

There's no real spec for what the header allows/forbids, but I think it's mostly the destructive stuff. If you control the proxy, you can of course always do whatever you like, this header is just a hint to automated systems.

@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor

won't adding no-transform tell proxies to stop compressing Gitea pages?

Yes, it's a hint to not transform responses. Cloudfare does destructive HTML minification per default and that problem came up a few times in the past. I don't think no-transform should generally prevent compression via content-encoding as that it's non-destructive.

There's no real spec for what the header allows/forbids, but I think it's mostly the destructive stuff. If you control the proxy, you can of course always do whatever you like, this header is just a hint to automated systems.

thanks for the reply.

that probably means with nginx I'll have to start clearing headers before setting them...will test, report back in case of trouble.

zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 25, 2022
* giteaofficial/main:
  Fix Ruby package parsing by removed unused email field (go-gitea#20470)
  [skip ci] Updated translations via Crowdin
  Add repository condition for issue count (go-gitea#20454)
  Prepend commit message to template content (go-gitea#20429)
  Improve pprof doc (go-gitea#20463)
  Improve code diff highlight, fix incorrect rendered diff result (go-gitea#19958)
  Add Cache-Control header to html and api responses, add no-transform (go-gitea#20432)
  [skip ci] Updated translations via Crowdin
  Allow non-semver packages in the Conan package registry (go-gitea#20412)
  Use body text color in repository files table links (go-gitea#20386)
  Correct code block in installation docs for Snap (go-gitea#20440)
  Downgrade golangci-lint to 1.47.0 (go-gitea#20445)
  Add eslint-plugin-sonarjs (go-gitea#20431)
  Fix: Actor is required to get user repositories (go-gitea#20443)
  Add "X-Gitea-Object-Type" header for GET `/raw/` & `/media/` API (go-gitea#20438)
  Simplify visibility checks (go-gitea#20406)
@6543 6543 added this to the 1.18.0 milestone Jul 28, 2022
@6543 6543 added the backport/done All backports for this PR have been created label Jul 28, 2022
vsysoev pushed a commit to IntegraSDL/gitea that referenced this pull request Aug 10, 2022
…o-gitea#20432)

`no-transform` allegedly disables CloudFlare auto-minify and we did not
set caching headers on html or api requests, which seems good to have
regardless.

Transformation is still allowed for asset requests.

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: Andrew Thornton <art27@cantab.net>
brechtvl added a commit to brechtvl/gitea that referenced this pull request Jan 26, 2023
The no-store cache control added in go-gitea#20432 is causing form input to be cleared
unnecessarily on page reload. Instead use max-age=0,private,must-revalidate
which avoid this.

This was particularly a problem when typing a long comment for an issue and
then for example changing the label. The page would be reload and lose the
unsubmitted comment.

Fixes go-gitea#22603
jolheiser pushed a commit that referenced this pull request Feb 1, 2023
…2604)

The `no-store` cache control added in #20432 is causing form input to be
cleared unnecessarily on page reload. Instead use
`max-age=0,private,must-revalidate` which avoids this.

This was particularly a problem when typing a long comment for an issue
and then for example changing the label. The page would be reloaded and
lose the unsubmitted comment.

Fixes #22603
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Cache-Control: no-transform to disable Cloudflare auto minify
6 participants