From 12b6cdcd8ce52269be5a1ca8acaae006896808b5 Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Thu, 13 Jul 2023 21:27:25 +0200 Subject: [PATCH] [bugfix] Set Vary header correctly on cache-control (#1988) * [bugfix] Set Vary header correctly on cache-control * Prefer activitypub types on AP endpoints * use immutable on file server, vary by range * vary auth on Accept --- internal/api/activitypub.go | 19 +++++-- internal/api/activitypub/emoji/emojiget.go | 2 +- .../api/activitypub/publickey/publickeyget.go | 2 +- internal/api/activitypub/users/featured.go | 2 +- internal/api/activitypub/users/followers.go | 2 +- internal/api/activitypub/users/following.go | 2 +- internal/api/activitypub/users/outboxget.go | 2 +- internal/api/activitypub/users/repliesget.go | 2 +- internal/api/activitypub/users/statusget.go | 2 +- internal/api/activitypub/users/userget.go | 2 +- internal/api/auth.go | 11 ++-- internal/api/client.go | 5 +- internal/api/fileserver.go | 17 +++++-- internal/api/fileserver/servefile.go | 2 +- internal/api/nodeinfo.go | 7 ++- internal/api/util/negotiate.go | 32 +++++++++--- internal/api/wellknown.go | 7 ++- internal/middleware/cachecontrol.go | 51 +++++++++++++++++-- internal/web/web.go | 4 +- 19 files changed, 132 insertions(+), 41 deletions(-) diff --git a/internal/api/activitypub.go b/internal/api/activitypub.go index ec5effe55..02ae0767c 100644 --- a/internal/api/activitypub.go +++ b/internal/api/activitypub.go @@ -41,11 +41,13 @@ func (a *ActivityPub) Route(r router.Router, m ...gin.HandlerFunc) { usersGroup := r.AttachGroup("users") // attach shared, non-global middlewares to both of these groups - cacheControlMiddleware := middleware.CacheControl("no-store") + ccMiddleware := middleware.CacheControl(middleware.CacheControlConfig{ + Directives: []string{"no-store"}, + }) emojiGroup.Use(m...) usersGroup.Use(m...) - emojiGroup.Use(a.signatureCheckMiddleware, cacheControlMiddleware) - usersGroup.Use(a.signatureCheckMiddleware, cacheControlMiddleware) + emojiGroup.Use(a.signatureCheckMiddleware, ccMiddleware) + usersGroup.Use(a.signatureCheckMiddleware, ccMiddleware) a.emoji.Route(emojiGroup.Handle) a.users.Route(usersGroup.Handle) @@ -53,8 +55,17 @@ func (a *ActivityPub) Route(r router.Router, m ...gin.HandlerFunc) { // Public key endpoint requires different middleware + cache policies from other AP endpoints. func (a *ActivityPub) RoutePublicKey(r router.Router, m ...gin.HandlerFunc) { + // Create grouping for the 'users/[username]/main-key' prefix. publicKeyGroup := r.AttachGroup(publickey.PublicKeyPath) - publicKeyGroup.Use(a.signatureCheckMiddleware, middleware.CacheControl("public,max-age=604800")) + + // Attach middleware allowing public cacheing of main-key. + ccMiddleware := middleware.CacheControl(middleware.CacheControlConfig{ + Directives: []string{"public", "max-age=604800"}, + Vary: []string{"Accept", "Accept-Encoding"}, + }) + publicKeyGroup.Use(m...) + publicKeyGroup.Use(a.signatureCheckMiddleware, ccMiddleware) + a.publicKey.Route(publicKeyGroup.Handle) } diff --git a/internal/api/activitypub/emoji/emojiget.go b/internal/api/activitypub/emoji/emojiget.go index 8602b6052..c291a500a 100644 --- a/internal/api/activitypub/emoji/emojiget.go +++ b/internal/api/activitypub/emoji/emojiget.go @@ -36,7 +36,7 @@ func (m *Module) EmojiGetHandler(c *gin.Context) { return } - format, err := apiutil.NegotiateAccept(c, apiutil.ActivityPubAcceptHeaders...) + format, err := apiutil.NegotiateAccept(c, apiutil.ActivityPubHeaders...) if err != nil { apiutil.ErrorHandler(c, gtserror.NewErrorNotAcceptable(err, err.Error()), m.processor.InstanceGetV1) return diff --git a/internal/api/activitypub/publickey/publickeyget.go b/internal/api/activitypub/publickey/publickeyget.go index 5ccb86328..a7de4efad 100644 --- a/internal/api/activitypub/publickey/publickeyget.go +++ b/internal/api/activitypub/publickey/publickeyget.go @@ -42,7 +42,7 @@ func (m *Module) PublicKeyGETHandler(c *gin.Context) { return } - format, err := apiutil.NegotiateAccept(c, apiutil.HTMLOrActivityPubHeaders...) + format, err := apiutil.NegotiateAccept(c, apiutil.ActivityPubOrHTMLHeaders...) if err != nil { apiutil.ErrorHandler(c, gtserror.NewErrorNotAcceptable(err, err.Error()), m.processor.InstanceGetV1) return diff --git a/internal/api/activitypub/users/featured.go b/internal/api/activitypub/users/featured.go index de6ff14ae..7a2b73a6f 100644 --- a/internal/api/activitypub/users/featured.go +++ b/internal/api/activitypub/users/featured.go @@ -67,7 +67,7 @@ func (m *Module) FeaturedCollectionGETHandler(c *gin.Context) { return } - format, err := apiutil.NegotiateAccept(c, apiutil.HTMLOrActivityPubHeaders...) + format, err := apiutil.NegotiateAccept(c, apiutil.ActivityPubOrHTMLHeaders...) if err != nil { apiutil.ErrorHandler(c, gtserror.NewErrorNotAcceptable(err, err.Error()), m.processor.InstanceGetV1) return diff --git a/internal/api/activitypub/users/followers.go b/internal/api/activitypub/users/followers.go index 0825651a8..07bf5a421 100644 --- a/internal/api/activitypub/users/followers.go +++ b/internal/api/activitypub/users/followers.go @@ -38,7 +38,7 @@ func (m *Module) FollowersGETHandler(c *gin.Context) { return } - format, err := apiutil.NegotiateAccept(c, apiutil.HTMLOrActivityPubHeaders...) + format, err := apiutil.NegotiateAccept(c, apiutil.ActivityPubOrHTMLHeaders...) if err != nil { apiutil.ErrorHandler(c, gtserror.NewErrorNotAcceptable(err, err.Error()), m.processor.InstanceGetV1) return diff --git a/internal/api/activitypub/users/following.go b/internal/api/activitypub/users/following.go index bc6e96ca1..126ef99b2 100644 --- a/internal/api/activitypub/users/following.go +++ b/internal/api/activitypub/users/following.go @@ -38,7 +38,7 @@ func (m *Module) FollowingGETHandler(c *gin.Context) { return } - format, err := apiutil.NegotiateAccept(c, apiutil.HTMLOrActivityPubHeaders...) + format, err := apiutil.NegotiateAccept(c, apiutil.ActivityPubOrHTMLHeaders...) if err != nil { apiutil.ErrorHandler(c, gtserror.NewErrorNotAcceptable(err, err.Error()), m.processor.InstanceGetV1) return diff --git a/internal/api/activitypub/users/outboxget.go b/internal/api/activitypub/users/outboxget.go index 9e3ec2d15..e4617ba90 100644 --- a/internal/api/activitypub/users/outboxget.go +++ b/internal/api/activitypub/users/outboxget.go @@ -93,7 +93,7 @@ func (m *Module) OutboxGETHandler(c *gin.Context) { return } - format, err := apiutil.NegotiateAccept(c, apiutil.HTMLOrActivityPubHeaders...) + format, err := apiutil.NegotiateAccept(c, apiutil.ActivityPubOrHTMLHeaders...) if err != nil { apiutil.ErrorHandler(c, gtserror.NewErrorNotAcceptable(err, err.Error()), m.processor.InstanceGetV1) return diff --git a/internal/api/activitypub/users/repliesget.go b/internal/api/activitypub/users/repliesget.go index 70764a73d..fd9dc090b 100644 --- a/internal/api/activitypub/users/repliesget.go +++ b/internal/api/activitypub/users/repliesget.go @@ -108,7 +108,7 @@ func (m *Module) StatusRepliesGETHandler(c *gin.Context) { return } - format, err := apiutil.NegotiateAccept(c, apiutil.HTMLOrActivityPubHeaders...) + format, err := apiutil.NegotiateAccept(c, apiutil.ActivityPubOrHTMLHeaders...) if err != nil { apiutil.ErrorHandler(c, gtserror.NewErrorNotAcceptable(err, err.Error()), m.processor.InstanceGetV1) return diff --git a/internal/api/activitypub/users/statusget.go b/internal/api/activitypub/users/statusget.go index 4a107c5a1..f7b5ccfd1 100644 --- a/internal/api/activitypub/users/statusget.go +++ b/internal/api/activitypub/users/statusget.go @@ -46,7 +46,7 @@ func (m *Module) StatusGETHandler(c *gin.Context) { return } - format, err := apiutil.NegotiateAccept(c, apiutil.HTMLOrActivityPubHeaders...) + format, err := apiutil.NegotiateAccept(c, apiutil.ActivityPubOrHTMLHeaders...) if err != nil { apiutil.ErrorHandler(c, gtserror.NewErrorNotAcceptable(err, err.Error()), m.processor.InstanceGetV1) return diff --git a/internal/api/activitypub/users/userget.go b/internal/api/activitypub/users/userget.go index 536da9e81..61be69836 100644 --- a/internal/api/activitypub/users/userget.go +++ b/internal/api/activitypub/users/userget.go @@ -46,7 +46,7 @@ func (m *Module) UsersGETHandler(c *gin.Context) { return } - format, err := apiutil.NegotiateAccept(c, apiutil.HTMLOrActivityPubHeaders...) + format, err := apiutil.NegotiateAccept(c, apiutil.ActivityPubOrHTMLHeaders...) if err != nil { apiutil.ErrorHandler(c, gtserror.NewErrorNotAcceptable(err, err.Error()), m.processor.InstanceGetV1) return diff --git a/internal/api/auth.go b/internal/api/auth.go index e2411229d..961caa981 100644 --- a/internal/api/auth.go +++ b/internal/api/auth.go @@ -43,13 +43,16 @@ func (a *Auth) Route(r router.Router, m ...gin.HandlerFunc) { // instantiate + attach shared, non-global middlewares to both of these groups var ( - cacheControlMiddleware = middleware.CacheControl("private", "max-age=120") - sessionMiddleware = middleware.Session(a.sessionName, a.routerSession.Auth, a.routerSession.Crypt) + ccMiddleware = middleware.CacheControl(middleware.CacheControlConfig{ + Directives: []string{"private", "max-age=120"}, + Vary: []string{"Accept", "Accept-Encoding"}, + }) + sessionMiddleware = middleware.Session(a.sessionName, a.routerSession.Auth, a.routerSession.Crypt) ) authGroup.Use(m...) oauthGroup.Use(m...) - authGroup.Use(cacheControlMiddleware, sessionMiddleware) - oauthGroup.Use(cacheControlMiddleware, sessionMiddleware) + authGroup.Use(ccMiddleware, sessionMiddleware) + oauthGroup.Use(ccMiddleware, sessionMiddleware) a.auth.RouteAuth(authGroup.Handle) a.auth.RouteOauth(oauthGroup.Handle) diff --git a/internal/api/client.go b/internal/api/client.go index 59fa0cf96..d9f131e3b 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -83,7 +83,10 @@ func (c *Client) Route(r router.Router, m ...gin.HandlerFunc) { apiGroup.Use(m...) apiGroup.Use( middleware.TokenCheck(c.db, c.processor.OAuthValidateBearerToken), - middleware.CacheControl("no-store"), // never cache api responses + middleware.CacheControl(middleware.CacheControlConfig{ + // Never cache client api responses. + Directives: []string{"no-store"}, + }), ) // for each client api module, pass it the Handle function diff --git a/internal/api/fileserver.go b/internal/api/fileserver.go index 9a2fdd633..8f1e60b82 100644 --- a/internal/api/fileserver.go +++ b/internal/api/fileserver.go @@ -36,8 +36,8 @@ func (f *Fileserver) Route(r router.Router, m ...gin.HandlerFunc) { // Attach middlewares appropriate for this group. fileserverGroup.Use(m...) // If we're using local storage or proxying s3, we can set a - // long max-age on all file requests to reflect that we - // never host different files at the same URL (since + // long max-age + immutable on all file requests to reflect + // that we never host different files at the same URL (since // ULIDs are generated per piece of media), so we can // easily prevent clients having to fetch files repeatedly. // @@ -45,9 +45,18 @@ func (f *Fileserver) Route(r router.Router, m ...gin.HandlerFunc) { // must be set dynamically within the request handler, // based on how long the signed URL has left to live before // it expires. This ensures that clients won't cache expired - // links. This is done within fileserver/servefile.go. + // links. This is done within fileserver/servefile.go, so we + // should not set the middleware here in that case. + // + // See: + // + // - https://developer.mozilla.org/en-US/docs/Web/HTTP/Caching#avoiding_revalidation + // - https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#immutable if config.GetStorageBackend() == "local" || config.GetStorageS3Proxy() { - fileserverGroup.Use(middleware.CacheControl("private", "max-age=604800")) // 7d + fileserverGroup.Use(middleware.CacheControl(middleware.CacheControlConfig{ + Directives: []string{"private", "max-age=604800", "immutable"}, + Vary: []string{"Range"}, // Cache partial ranges separately. + })) } f.fileserver.Route(fileserverGroup.Handle) diff --git a/internal/api/fileserver/servefile.go b/internal/api/fileserver/servefile.go index 25a414319..12d24ca76 100644 --- a/internal/api/fileserver/servefile.go +++ b/internal/api/fileserver/servefile.go @@ -95,7 +95,7 @@ func (m *Module) ServeFile(c *gin.Context) { // Derive the max-age value from how long the link has left until // it expires. maxAge := int(time.Until(content.URL.Expiry).Seconds()) - c.Header("Cache-Control", "private,max-age="+strconv.Itoa(maxAge)) + c.Header("Cache-Control", "private, max-age="+strconv.Itoa(maxAge)+", immutable") c.Redirect(http.StatusFound, content.URL.String()) return } diff --git a/internal/api/nodeinfo.go b/internal/api/nodeinfo.go index c700a3e68..22d314033 100644 --- a/internal/api/nodeinfo.go +++ b/internal/api/nodeinfo.go @@ -36,8 +36,11 @@ func (w *NodeInfo) Route(r router.Router, m ...gin.HandlerFunc) { // attach middlewares appropriate for this group nodeInfoGroup.Use(m...) nodeInfoGroup.Use( - // allow cache for 2 minutes - middleware.CacheControl("public", "max-age=120"), + // Allow public cache for 2 minutes. + middleware.CacheControl(middleware.CacheControlConfig{ + Directives: []string{"public", "max-age=120"}, + Vary: []string{"Accept-Encoding"}, + }), ) w.nodeInfo.Route(nodeInfoGroup.Handle) diff --git a/internal/api/util/negotiate.go b/internal/api/util/negotiate.go index 6d68a0df3..f4268511b 100644 --- a/internal/api/util/negotiate.go +++ b/internal/api/util/negotiate.go @@ -25,12 +25,6 @@ "github.com/gin-gonic/gin" ) -// ActivityPubAcceptHeaders represents the Accept headers mentioned here: -var ActivityPubAcceptHeaders = []MIME{ - AppActivityJSON, - AppActivityLDJSON, -} - // JSONAcceptHeaders is a slice of offers that just contains application/json types. var JSONAcceptHeaders = []MIME{ AppJSON, @@ -59,12 +53,34 @@ } // HTMLOrActivityPubHeaders matches text/html first, then activitypub types. -// This is useful for user URLs that a user might go to in their browser. +// This is useful for user URLs that a user might go to in their browser, +// but which should also be able to serve ActivityPub as a fallback. +// // https://www.w3.org/TR/activitypub/#retrieving-objects var HTMLOrActivityPubHeaders = []MIME{ TextHTML, - AppActivityJSON, AppActivityLDJSON, + AppActivityJSON, +} + +// ActivityPubOrHTMLHeaders matches activitypub types first, then text/html. +// This is useful for URLs that should serve ActivityPub by default, but +// which a user might also go to in their browser sometimes. +// +// https://www.w3.org/TR/activitypub/#retrieving-objects +var ActivityPubOrHTMLHeaders = []MIME{ + AppActivityLDJSON, + AppActivityJSON, + TextHTML, +} + +// ActivityPubHeaders matches only activitypub Accept headers. +// This is useful for URLs should only serve ActivityPub. +// +// https://www.w3.org/TR/activitypub/#retrieving-objects +var ActivityPubHeaders = []MIME{ + AppActivityLDJSON, + AppActivityJSON, } var HostMetaHeaders = []MIME{ diff --git a/internal/api/wellknown.go b/internal/api/wellknown.go index f277ea274..63ca48ef7 100644 --- a/internal/api/wellknown.go +++ b/internal/api/wellknown.go @@ -40,8 +40,11 @@ func (w *WellKnown) Route(r router.Router, m ...gin.HandlerFunc) { // attach middlewares appropriate for this group wellKnownGroup.Use(m...) wellKnownGroup.Use( - // allow .well-known responses to be cached for 2 minutes - middleware.CacheControl("public", "max-age=120"), + // Allow public cache for 2 minutes. + middleware.CacheControl(middleware.CacheControlConfig{ + Directives: []string{"public", "max-age=120"}, + Vary: []string{"Accept-Encoding"}, + }), ) w.nodeInfo.Route(wellKnownGroup.Handle) diff --git a/internal/middleware/cachecontrol.go b/internal/middleware/cachecontrol.go index 1b471a87c..03b9ca2cb 100644 --- a/internal/middleware/cachecontrol.go +++ b/internal/middleware/cachecontrol.go @@ -23,12 +23,53 @@ "github.com/gin-gonic/gin" ) -// CacheControl returns a new gin middleware which allows callers to control cache settings on response headers. -// -// For directives, see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control -func CacheControl(directives ...string) gin.HandlerFunc { - ccHeader := strings.Join(directives, ", ") +type CacheControlConfig struct { + // Slice of Cache-Control directives, which will be + // joined comma-separated and served as the value of + // the Cache-Control header. + // + // If no directives are set, the Cache-Control header + // will not be sent in the response at all. + // + // For possible Cache-Control directive values, see: + // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control + Directives []string + + // Slice of Vary header values, which will be joined + // comma-separated and served as the value of the Vary + // header in the response. + // + // If no Vary header values are supplied, then the + // Vary header will be omitted in the response. + // + // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Vary + Vary []string +} + +// CacheControl returns a new gin middleware which allows +// routes to control cache settings on response headers. +func CacheControl(config CacheControlConfig) gin.HandlerFunc { + if len(config.Directives) == 0 { + // No Cache-Control directives provided, + // return empty/stub function. + return func(c *gin.Context) {} + } + + // Cache control is usually done on hot paths so + // parse vars outside of the returned function. + var ( + ccHeader = strings.Join(config.Directives, ", ") + varyHeader = strings.Join(config.Vary, ", ") + ) + + if varyHeader == "" { + return func(c *gin.Context) { + c.Header("Cache-Control", ccHeader) + } + } + return func(c *gin.Context) { c.Header("Cache-Control", ccHeader) + c.Header("Vary", varyHeader) } } diff --git a/internal/web/web.go b/internal/web/web.go index b28aaa3bc..c53433730 100644 --- a/internal/web/web.go +++ b/internal/web/web.go @@ -91,7 +91,9 @@ func (m *Module) Route(r router.Router, mi ...gin.HandlerFunc) { // can still be served profileGroup := r.AttachGroup(profileGroupPath) profileGroup.Use(mi...) - profileGroup.Use(middleware.SignatureCheck(m.isURIBlocked), middleware.CacheControl("no-store")) + profileGroup.Use(middleware.SignatureCheck(m.isURIBlocked), middleware.CacheControl(middleware.CacheControlConfig{ + Directives: []string{"no-store"}, + })) profileGroup.Handle(http.MethodGet, "", m.profileGETHandler) // use empty path here since it's the base of the group profileGroup.Handle(http.MethodGet, statusPath, m.threadGETHandler)