From 40eb7cbeef8d8e70e7751defe11521b1d27202ac Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Mon, 17 Jun 2024 10:25:41 +0200 Subject: [PATCH 01/16] Use httputil.Routers for the media API --- mediaapi/mediaapi.go | 6 +++--- mediaapi/routing/routing.go | 4 ++-- setup/monolith.go | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/mediaapi/mediaapi.go b/mediaapi/mediaapi.go index 3425fbce65..4eb45b3d1b 100644 --- a/mediaapi/mediaapi.go +++ b/mediaapi/mediaapi.go @@ -15,7 +15,7 @@ package mediaapi import ( - "github.com/gorilla/mux" + "github.com/matrix-org/dendrite/internal/httputil" "github.com/matrix-org/dendrite/internal/sqlutil" "github.com/matrix-org/dendrite/mediaapi/routing" "github.com/matrix-org/dendrite/mediaapi/storage" @@ -27,7 +27,7 @@ import ( // AddPublicRoutes sets up and registers HTTP handlers for the MediaAPI component. func AddPublicRoutes( - mediaRouter *mux.Router, + routers httputil.Routers, cm *sqlutil.Connections, cfg *config.Dendrite, userAPI userapi.MediaUserAPI, @@ -39,6 +39,6 @@ func AddPublicRoutes( } routing.Setup( - mediaRouter, cfg, mediaDB, userAPI, client, + routers, cfg, mediaDB, userAPI, client, ) } diff --git a/mediaapi/routing/routing.go b/mediaapi/routing/routing.go index 5963eeaae5..4edb5ab2c6 100644 --- a/mediaapi/routing/routing.go +++ b/mediaapi/routing/routing.go @@ -45,7 +45,7 @@ type configResponse struct { // applied: // nolint: gocyclo func Setup( - publicAPIMux *mux.Router, + routers httputil.Routers, cfg *config.Dendrite, db storage.Database, userAPI userapi.MediaUserAPI, @@ -53,7 +53,7 @@ func Setup( ) { rateLimits := httputil.NewRateLimits(&cfg.ClientAPI.RateLimiting) - v3mux := publicAPIMux.PathPrefix("/{apiversion:(?:r0|v1|v3)}/").Subrouter() + v3mux := routers.Media.PathPrefix("/{apiversion:(?:r0|v1|v3)}/").Subrouter() activeThumbnailGeneration := &types.ActiveThumbnailGeneration{ PathToResult: map[string]*types.ThumbnailGenerationResult{}, diff --git a/setup/monolith.go b/setup/monolith.go index 4856d6e835..64a447acba 100644 --- a/setup/monolith.go +++ b/setup/monolith.go @@ -78,7 +78,7 @@ func (m *Monolith) AddAllPublicRoutes( federationapi.AddPublicRoutes( processCtx, routers, cfg, natsInstance, m.UserAPI, m.FedClient, m.KeyRing, m.RoomserverAPI, m.FederationAPI, enableMetrics, ) - mediaapi.AddPublicRoutes(routers.Media, cm, cfg, m.UserAPI, m.Client) + mediaapi.AddPublicRoutes(routers, cm, cfg, m.UserAPI, m.Client) syncapi.AddPublicRoutes(processCtx, routers, cfg, cm, natsInstance, m.UserAPI, m.RoomserverAPI, caches, enableMetrics) if m.RelayAPI != nil { From ccd337e1d4595e563913b1499589e839b70b8a2a Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Mon, 17 Jun 2024 11:33:03 +0200 Subject: [PATCH 02/16] Add authentication to MakeHTMLAPI and use it for media requests --- clientapi/routing/routing.go | 2 +- internal/httputil/httpapi.go | 31 ++++++++++++++++++++++++++++++- mediaapi/routing/routing.go | 10 ++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-) diff --git a/clientapi/routing/routing.go b/clientapi/routing/routing.go index 60dad54331..ced75303e0 100644 --- a/clientapi/routing/routing.go +++ b/clientapi/routing/routing.go @@ -732,7 +732,7 @@ func Setup( ).Methods(http.MethodGet, http.MethodPost, http.MethodOptions) v3mux.Handle("/auth/{authType}/fallback/web", - httputil.MakeHTMLAPI("auth_fallback", enableMetrics, func(w http.ResponseWriter, req *http.Request) { + httputil.MakeHTMLAPI("auth_fallback", userAPI, enableMetrics, func(w http.ResponseWriter, req *http.Request) { vars := mux.Vars(req) AuthFallback(w, req, vars["authType"], cfg) }), diff --git a/internal/httputil/httpapi.go b/internal/httputil/httpapi.go index c78aadf892..26b2f4d734 100644 --- a/internal/httputil/httpapi.go +++ b/internal/httputil/httpapi.go @@ -15,6 +15,7 @@ package httputil import ( + "encoding/json" "fmt" "io" "net/http" @@ -44,6 +45,7 @@ type BasicAuth struct { type AuthAPIOpts struct { GuestAccessAllowed bool + WithAuth bool } // AuthAPIOption is an option to MakeAuthAPI to add additional checks (e.g. guest access) to verify @@ -57,6 +59,13 @@ func WithAllowGuests() AuthAPIOption { } } +// WithAuth is an option to MakeHTMLAPI to add authentication. +func WithAuth() AuthAPIOption { + return func(opts *AuthAPIOpts) { + opts.WithAuth = true + } +} + // MakeAuthAPI turns a util.JSONRequestHandler function into an http.Handler which authenticates the request. func MakeAuthAPI( metricsName string, userAPI userapi.QueryAcccessTokenAPI, @@ -199,11 +208,31 @@ func MakeExternalAPI(metricsName string, f func(*http.Request) util.JSONResponse // MakeHTMLAPI adds Span metrics to the HTML Handler function // This is used to serve HTML alongside JSON error messages -func MakeHTMLAPI(metricsName string, enableMetrics bool, f func(http.ResponseWriter, *http.Request)) http.Handler { +func MakeHTMLAPI(metricsName string, userAPI userapi.QueryAcccessTokenAPI, enableMetrics bool, f func(http.ResponseWriter, *http.Request), checks ...AuthAPIOption) http.Handler { withSpan := func(w http.ResponseWriter, req *http.Request) { trace, ctx := internal.StartTask(req.Context(), metricsName) defer trace.EndTask() req = req.WithContext(ctx) + + // apply additional checks, if any + opts := AuthAPIOpts{} + for _, opt := range checks { + opt(&opts) + } + + if opts.WithAuth { + logger := util.GetLogger(req.Context()) + _, jsonErr := auth.VerifyUserFromRequest(req, userAPI) + if jsonErr != nil { + logger.Debugf("VerifyUserFromRequest %s -> HTTP %d", req.RemoteAddr, jsonErr.Code) + w.WriteHeader(jsonErr.Code) + if err := json.NewEncoder(w).Encode(jsonErr); err != nil { + logger.WithError(err).Error("failed to encode JSON response") + } + return + } + } + f(w, req) } diff --git a/mediaapi/routing/routing.go b/mediaapi/routing/routing.go index 4edb5ab2c6..f524d9b17a 100644 --- a/mediaapi/routing/routing.go +++ b/mediaapi/routing/routing.go @@ -54,6 +54,7 @@ func Setup( rateLimits := httputil.NewRateLimits(&cfg.ClientAPI.RateLimiting) v3mux := routers.Media.PathPrefix("/{apiversion:(?:r0|v1|v3)}/").Subrouter() + v1mux := routers.Client.PathPrefix("/v1/media/").Subrouter() activeThumbnailGeneration := &types.ActiveThumbnailGeneration{ PathToResult: map[string]*types.ThumbnailGenerationResult{}, @@ -97,6 +98,15 @@ func Setup( v3mux.Handle("/thumbnail/{serverName}/{mediaId}", makeDownloadAPI("thumbnail", &cfg.MediaAPI, rateLimits, db, client, activeRemoteRequests, activeThumbnailGeneration), ).Methods(http.MethodGet, http.MethodOptions) + + // v1 client endpoints requiring auth + v1mux.Handle("/config", configHandler).Methods(http.MethodGet, http.MethodOptions) + v1mux.Handle("/download/{serverName}/{mediaId}", httputil.MakeHTMLAPI("download", userAPI, cfg.Global.Metrics.Enabled, downloadHandler, httputil.WithAuth())).Methods(http.MethodGet, http.MethodOptions) + v1mux.Handle("/download/{serverName}/{mediaId}/{downloadName}", httputil.MakeHTMLAPI("download", userAPI, cfg.Global.Metrics.Enabled, downloadHandler, httputil.WithAuth())).Methods(http.MethodGet, http.MethodOptions) + + v1mux.Handle("/thumbnail/{serverName}/{mediaId}", + httputil.MakeHTMLAPI("thumbnail", userAPI, cfg.Global.Metrics.Enabled, makeDownloadAPI("thumbnail", &cfg.MediaAPI, rateLimits, db, client, activeRemoteRequests, activeThumbnailGeneration), httputil.WithAuth()), + ).Methods(http.MethodGet, http.MethodOptions) } func makeDownloadAPI( From 29ee5401ee6e727193c554d8c338e0cb6a26baf6 Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Tue, 18 Jun 2024 12:36:35 +0200 Subject: [PATCH 03/16] Add support for authenticated federation media requests --- federationapi/routing/routing.go | 48 +++++++++++++++++ mediaapi/mediaapi.go | 4 +- mediaapi/routing/download.go | 55 ++++++++++++++++++- mediaapi/routing/routing.go | 91 +++++++++++++++++++++++++++----- setup/monolith.go | 2 +- 5 files changed, 183 insertions(+), 17 deletions(-) diff --git a/federationapi/routing/routing.go b/federationapi/routing/routing.go index 6328d165e0..5a735cf5e6 100644 --- a/federationapi/routing/routing.go +++ b/federationapi/routing/routing.go @@ -16,6 +16,7 @@ package routing import ( "context" + "encoding/json" "fmt" "net/http" "sync" @@ -678,6 +679,53 @@ func MakeFedAPI( return httputil.MakeExternalAPI(metricsName, h) } +// MakeFedAPI makes an http.Handler that checks matrix federation authentication. +func MakeFedAPIHTML( + serverName spec.ServerName, + isLocalServerName func(spec.ServerName) bool, + keyRing gomatrixserverlib.JSONVerifier, + f func(http.ResponseWriter, *http.Request), +) http.Handler { + h := func(w http.ResponseWriter, req *http.Request) { + fedReq, errResp := fclient.VerifyHTTPRequest( + req, time.Now(), serverName, isLocalServerName, keyRing, + ) + + enc := json.NewEncoder(w) + logger := util.GetLogger(req.Context()) + if fedReq == nil { + + logger.Debugf("VerifyUserFromRequest %s -> HTTP %d", req.RemoteAddr, errResp.Code) + w.WriteHeader(errResp.Code) + if err := enc.Encode(errResp); err != nil { + logger.WithError(err).Error("failed to encode JSON response") + } + return + } + // add the user to Sentry, if enabled + hub := sentry.GetHubFromContext(req.Context()) + if hub != nil { + // clone the hub, so we don't send garbage events with e.g. mismatching rooms/event_ids + hub = hub.Clone() + hub.Scope().SetTag("origin", string(fedReq.Origin())) + hub.Scope().SetTag("uri", fedReq.RequestURI()) + } + defer func() { + if r := recover(); r != nil { + if hub != nil { + hub.CaptureException(fmt.Errorf("%s panicked", req.URL.Path)) + } + // re-panic to return the 500 + panic(r) + } + }() + + f(w, req) + } + + return http.HandlerFunc(h) +} + type FederationWakeups struct { FsAPI *fedInternal.FederationInternalAPI origins sync.Map diff --git a/mediaapi/mediaapi.go b/mediaapi/mediaapi.go index 4eb45b3d1b..adeb89ab24 100644 --- a/mediaapi/mediaapi.go +++ b/mediaapi/mediaapi.go @@ -21,6 +21,7 @@ import ( "github.com/matrix-org/dendrite/mediaapi/storage" "github.com/matrix-org/dendrite/setup/config" userapi "github.com/matrix-org/dendrite/userapi/api" + "github.com/matrix-org/gomatrixserverlib" "github.com/matrix-org/gomatrixserverlib/fclient" "github.com/sirupsen/logrus" ) @@ -32,6 +33,7 @@ func AddPublicRoutes( cfg *config.Dendrite, userAPI userapi.MediaUserAPI, client *fclient.Client, + keyRing gomatrixserverlib.JSONVerifier, ) { mediaDB, err := storage.NewMediaAPIDatasource(cm, &cfg.MediaAPI.Database) if err != nil { @@ -39,6 +41,6 @@ func AddPublicRoutes( } routing.Setup( - routers, cfg, mediaDB, userAPI, client, + routers, cfg, mediaDB, userAPI, client, keyRing, ) } diff --git a/mediaapi/routing/download.go b/mediaapi/routing/download.go index fa1c417aa6..75140cffa3 100644 --- a/mediaapi/routing/download.go +++ b/mediaapi/routing/download.go @@ -21,7 +21,9 @@ import ( "io" "io/fs" "mime" + "mime/multipart" "net/http" + "net/textproto" "net/url" "os" "path/filepath" @@ -31,6 +33,7 @@ import ( "sync" "unicode" + "github.com/google/uuid" "github.com/matrix-org/dendrite/mediaapi/fileutils" "github.com/matrix-org/dendrite/mediaapi/storage" "github.com/matrix-org/dendrite/mediaapi/thumbnailer" @@ -61,6 +64,7 @@ type downloadRequest struct { ThumbnailSize types.ThumbnailSize Logger *log.Entry DownloadFilename string + forFederation bool // whether we need to return a multipart/mixed response } // Taken from: https://github.com/matrix-org/synapse/blob/c3627d0f99ed5a23479305dc2bd0e71ca25ce2b1/synapse/media/_base.py#L53C1-L84 @@ -115,7 +119,12 @@ func Download( activeThumbnailGeneration *types.ActiveThumbnailGeneration, isThumbnailRequest bool, customFilename string, + forFederation bool, ) { + // This happens if we call Download for a federation request + if forFederation && origin == "" { + origin = cfg.Matrix.ServerName + } dReq := &downloadRequest{ MediaMetadata: &types.MediaMetadata{ MediaID: mediaID, @@ -127,6 +136,7 @@ func Download( "MediaID": mediaID, }), DownloadFilename: customFilename, + forFederation: forFederation, } if dReq.IsThumbnailRequest { @@ -369,8 +379,49 @@ func (r *downloadRequest) respondFromLocalFile( " object-src 'self';" w.Header().Set("Content-Security-Policy", contentSecurityPolicy) - if _, err := io.Copy(w, responseFile); err != nil { - return nil, fmt.Errorf("io.Copy: %w", err) + if !r.forFederation { + if _, err := io.Copy(w, responseFile); err != nil { + return nil, fmt.Errorf("io.Copy: %w", err) + } + } else { + // Update the header to be multipart/mixed; boundary=$randomBoundary + boundary := uuid.NewString() + w.Header().Set("Content-Type", "multipart/mixed; boundary="+boundary) + + w.Header().Del("Content-Length") // let Go handle the content length + w.Header().Del("Content-Security-Policy") // S-S request, so does not really matter? + mw := multipart.NewWriter(w) + defer func() { + if err = mw.Close(); err != nil { + r.Logger.WithError(err).Error("Failed to close multipart writer") + } + }() + + if err = mw.SetBoundary(boundary); err != nil { + return nil, fmt.Errorf("failed to set multipart boundary: %w", err) + } + + // JSON object part + jsonWriter, err := mw.CreatePart(textproto.MIMEHeader{ + "Content-Type": {"application/json"}, + }) + if err != nil { + return nil, fmt.Errorf("failed to create json writer: %w", err) + } + if _, err = jsonWriter.Write([]byte("{}")); err != nil { + return nil, fmt.Errorf("failed to write to json writer: %w", err) + } + + // media part + mediaWriter, err := mw.CreatePart(textproto.MIMEHeader{ + "Content-Type": {string(responseMetadata.ContentType)}, + }) + if err != nil { + return nil, fmt.Errorf("failed to create media writer: %w", err) + } + if _, err = io.Copy(mediaWriter, responseFile); err != nil { + return nil, fmt.Errorf("failed to write to media writer: %w", err) + } } return responseMetadata, nil } diff --git a/mediaapi/routing/routing.go b/mediaapi/routing/routing.go index f524d9b17a..b5998d686b 100644 --- a/mediaapi/routing/routing.go +++ b/mediaapi/routing/routing.go @@ -20,11 +20,13 @@ import ( "strings" "github.com/gorilla/mux" + "github.com/matrix-org/dendrite/federationapi/routing" "github.com/matrix-org/dendrite/internal/httputil" "github.com/matrix-org/dendrite/mediaapi/storage" "github.com/matrix-org/dendrite/mediaapi/types" "github.com/matrix-org/dendrite/setup/config" userapi "github.com/matrix-org/dendrite/userapi/api" + "github.com/matrix-org/gomatrixserverlib" "github.com/matrix-org/gomatrixserverlib/fclient" "github.com/matrix-org/gomatrixserverlib/spec" "github.com/matrix-org/util" @@ -50,11 +52,13 @@ func Setup( db storage.Database, userAPI userapi.MediaUserAPI, client *fclient.Client, + keyRing gomatrixserverlib.JSONVerifier, ) { rateLimits := httputil.NewRateLimits(&cfg.ClientAPI.RateLimiting) v3mux := routers.Media.PathPrefix("/{apiversion:(?:r0|v1|v3)}/").Subrouter() v1mux := routers.Client.PathPrefix("/v1/media/").Subrouter() + v1fedMux := routers.Federation.PathPrefix("/v1/media/").Subrouter() activeThumbnailGeneration := &types.ActiveThumbnailGeneration{ PathToResult: map[string]*types.ThumbnailGenerationResult{}, @@ -91,24 +95,75 @@ func Setup( MXCToResult: map[string]*types.RemoteRequestResult{}, } - downloadHandler := makeDownloadAPI("download", &cfg.MediaAPI, rateLimits, db, client, activeRemoteRequests, activeThumbnailGeneration) + downloadHandler := makeDownloadAPI("download_unauthed", &cfg.MediaAPI, rateLimits, db, client, activeRemoteRequests, activeThumbnailGeneration, false) v3mux.Handle("/download/{serverName}/{mediaId}", downloadHandler).Methods(http.MethodGet, http.MethodOptions) v3mux.Handle("/download/{serverName}/{mediaId}/{downloadName}", downloadHandler).Methods(http.MethodGet, http.MethodOptions) v3mux.Handle("/thumbnail/{serverName}/{mediaId}", - makeDownloadAPI("thumbnail", &cfg.MediaAPI, rateLimits, db, client, activeRemoteRequests, activeThumbnailGeneration), + makeDownloadAPI("thumbnail_unauthed", &cfg.MediaAPI, rateLimits, db, client, activeRemoteRequests, activeThumbnailGeneration, false), ).Methods(http.MethodGet, http.MethodOptions) // v1 client endpoints requiring auth + downloadHandlerAuthed := httputil.MakeHTMLAPI("download_authed_client", userAPI, cfg.Global.Metrics.Enabled, downloadHandler, httputil.WithAuth()) v1mux.Handle("/config", configHandler).Methods(http.MethodGet, http.MethodOptions) - v1mux.Handle("/download/{serverName}/{mediaId}", httputil.MakeHTMLAPI("download", userAPI, cfg.Global.Metrics.Enabled, downloadHandler, httputil.WithAuth())).Methods(http.MethodGet, http.MethodOptions) - v1mux.Handle("/download/{serverName}/{mediaId}/{downloadName}", httputil.MakeHTMLAPI("download", userAPI, cfg.Global.Metrics.Enabled, downloadHandler, httputil.WithAuth())).Methods(http.MethodGet, http.MethodOptions) + v1mux.Handle("/download/{serverName}/{mediaId}", downloadHandlerAuthed).Methods(http.MethodGet, http.MethodOptions) + v1mux.Handle("/download/{serverName}/{mediaId}/{downloadName}", downloadHandlerAuthed).Methods(http.MethodGet, http.MethodOptions) v1mux.Handle("/thumbnail/{serverName}/{mediaId}", - httputil.MakeHTMLAPI("thumbnail", userAPI, cfg.Global.Metrics.Enabled, makeDownloadAPI("thumbnail", &cfg.MediaAPI, rateLimits, db, client, activeRemoteRequests, activeThumbnailGeneration), httputil.WithAuth()), + httputil.MakeHTMLAPI("thumbnail", userAPI, cfg.Global.Metrics.Enabled, makeDownloadAPI("thumbnail_authed_client", &cfg.MediaAPI, rateLimits, db, client, activeRemoteRequests, activeThumbnailGeneration, false), httputil.WithAuth()), ).Methods(http.MethodGet, http.MethodOptions) + + // same, but for federation + v1fedMux.Handle("/download/{mediaId}", routing.MakeFedAPIHTML(cfg.Global.ServerName, cfg.Global.IsLocalServerName, keyRing, + makeDownloadAPI("download_authed_federation", &cfg.MediaAPI, rateLimits, db, client, activeRemoteRequests, activeThumbnailGeneration, true), + )).Methods(http.MethodGet, http.MethodOptions) + v1fedMux.Handle("/thumbnail/{mediaId}", routing.MakeFedAPIHTML(cfg.Global.ServerName, cfg.Global.IsLocalServerName, keyRing, + makeDownloadAPI("thumbnail_authed_federation", &cfg.MediaAPI, rateLimits, db, client, activeRemoteRequests, activeThumbnailGeneration, true), + )).Methods(http.MethodGet, http.MethodOptions) } +var thumbnailCounter = promauto.NewCounterVec( + prometheus.CounterOpts{ + Namespace: "dendrite", + Subsystem: "mediaapi", + Name: "thumbnail", + Help: "Total number of media_api requests for thumbnails", + }, + []string{"code", "type"}, +) + +var thumbnailSize = promauto.NewHistogramVec( + prometheus.HistogramOpts{ + Namespace: "dendrite", + Subsystem: "mediaapi", + Name: "thumbnail_size_bytes", + Help: "Total number of media_api requests for thumbnails", + Buckets: []float64{50, 100, 200, 500, 900, 1500, 3000, 6000}, + }, + []string{"code", "type"}, +) + +var downloadCounter = promauto.NewCounterVec( + prometheus.CounterOpts{ + Namespace: "dendrite", + Subsystem: "mediaapi", + Name: "download", + Help: "Total number of media_api requests for full downloads", + }, + []string{"code", "type"}, +) + +var downloadSize = promauto.NewHistogramVec( + prometheus.HistogramOpts{ + Namespace: "dendrite", + Subsystem: "mediaapi", + Name: "download_size_bytes", + Help: "Total number of media_api requests for full downloads", + Buckets: []float64{200, 500, 900, 1500, 3000, 6000, 10_000, 50_000, 100_000}, + }, + []string{"code", "type"}, +) + func makeDownloadAPI( name string, cfg *config.MediaAPI, @@ -117,16 +172,22 @@ func makeDownloadAPI( client *fclient.Client, activeRemoteRequests *types.ActiveRemoteRequests, activeThumbnailGeneration *types.ActiveThumbnailGeneration, + forFederation bool, ) http.HandlerFunc { var counterVec *prometheus.CounterVec + var sizeVec *prometheus.HistogramVec + var requestType string if cfg.Matrix.Metrics.Enabled { - counterVec = promauto.NewCounterVec( - prometheus.CounterOpts{ - Name: name, - Help: "Total number of media_api requests for either thumbnails or full downloads", - }, - []string{"code"}, - ) + split := strings.Split(name, "_") + name = split[0] + requestType = strings.Join(split[1:], "_") + + counterVec = thumbnailCounter + sizeVec = thumbnailSize + if name != "thumbnail" { + counterVec = downloadCounter + sizeVec = downloadSize + } } httpHandler := func(w http.ResponseWriter, req *http.Request) { req = util.RequestWithLogging(req) @@ -176,14 +237,18 @@ func makeDownloadAPI( client, activeRemoteRequests, activeThumbnailGeneration, - name == "thumbnail", + strings.HasPrefix(name, "thumbnail"), vars["downloadName"], + forFederation, ) } var handlerFunc http.HandlerFunc if counterVec != nil { + counterVec = counterVec.MustCurryWith(prometheus.Labels{"type": requestType}) + sizeVec2 := sizeVec.MustCurryWith(prometheus.Labels{"type": requestType}) handlerFunc = promhttp.InstrumentHandlerCounter(counterVec, http.HandlerFunc(httpHandler)) + handlerFunc = promhttp.InstrumentHandlerResponseSize(sizeVec2, handlerFunc).ServeHTTP } else { handlerFunc = http.HandlerFunc(httpHandler) } diff --git a/setup/monolith.go b/setup/monolith.go index 64a447acba..11cc59e276 100644 --- a/setup/monolith.go +++ b/setup/monolith.go @@ -78,7 +78,7 @@ func (m *Monolith) AddAllPublicRoutes( federationapi.AddPublicRoutes( processCtx, routers, cfg, natsInstance, m.UserAPI, m.FedClient, m.KeyRing, m.RoomserverAPI, m.FederationAPI, enableMetrics, ) - mediaapi.AddPublicRoutes(routers, cm, cfg, m.UserAPI, m.Client) + mediaapi.AddPublicRoutes(routers, cm, cfg, m.UserAPI, m.Client, m.KeyRing) syncapi.AddPublicRoutes(processCtx, routers, cfg, cm, natsInstance, m.UserAPI, m.RoomserverAPI, caches, enableMetrics) if m.RelayAPI != nil { From 89d64c6dca99474111883502ffad83c041fd15aa Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Tue, 18 Jun 2024 15:15:56 +0200 Subject: [PATCH 04/16] Add support for getting authenticated federation media requests --- mediaapi/mediaapi.go | 3 +- mediaapi/routing/download.go | 70 +++++++++++++++++++++++++++++------- mediaapi/routing/routing.go | 13 ++++--- setup/monolith.go | 2 +- 4 files changed, 69 insertions(+), 19 deletions(-) diff --git a/mediaapi/mediaapi.go b/mediaapi/mediaapi.go index adeb89ab24..8b843e9072 100644 --- a/mediaapi/mediaapi.go +++ b/mediaapi/mediaapi.go @@ -33,6 +33,7 @@ func AddPublicRoutes( cfg *config.Dendrite, userAPI userapi.MediaUserAPI, client *fclient.Client, + fedClient fclient.FederationClient, keyRing gomatrixserverlib.JSONVerifier, ) { mediaDB, err := storage.NewMediaAPIDatasource(cm, &cfg.MediaAPI.Database) @@ -41,6 +42,6 @@ func AddPublicRoutes( } routing.Setup( - routers, cfg, mediaDB, userAPI, client, keyRing, + routers, cfg, mediaDB, userAPI, client, fedClient, keyRing, ) } diff --git a/mediaapi/routing/download.go b/mediaapi/routing/download.go index 75140cffa3..93040bcba8 100644 --- a/mediaapi/routing/download.go +++ b/mediaapi/routing/download.go @@ -65,6 +65,8 @@ type downloadRequest struct { Logger *log.Entry DownloadFilename string forFederation bool // whether we need to return a multipart/mixed response + fedClient fclient.FederationClient + origin spec.ServerName } // Taken from: https://github.com/matrix-org/synapse/blob/c3627d0f99ed5a23479305dc2bd0e71ca25ce2b1/synapse/media/_base.py#L53C1-L84 @@ -115,6 +117,7 @@ func Download( cfg *config.MediaAPI, db storage.Database, client *fclient.Client, + fedClient fclient.FederationClient, activeRemoteRequests *types.ActiveRemoteRequests, activeThumbnailGeneration *types.ActiveThumbnailGeneration, isThumbnailRequest bool, @@ -137,6 +140,8 @@ func Download( }), DownloadFilename: customFilename, forFederation: forFederation, + origin: cfg.Matrix.ServerName, + fedClient: fedClient, } if dReq.IsThumbnailRequest { @@ -773,8 +778,7 @@ func (r *downloadRequest) fetchRemoteFileAndStoreMetadata( return nil } -func (r *downloadRequest) GetContentLengthAndReader(contentLengthHeader string, body *io.ReadCloser, maxFileSizeBytes config.FileSizeBytes) (int64, io.Reader, error) { - reader := *body +func (r *downloadRequest) GetContentLengthAndReader(contentLengthHeader string, reader io.ReadCloser, maxFileSizeBytes config.FileSizeBytes) (int64, io.Reader, error) { var contentLength int64 if contentLengthHeader != "" { @@ -793,7 +797,7 @@ func (r *downloadRequest) GetContentLengthAndReader(contentLengthHeader string, // We successfully parsed the Content-Length, so we'll return a limited // reader that restricts us to reading only up to this size. - reader = io.NopCloser(io.LimitReader(*body, parsedLength)) + reader = io.NopCloser(io.LimitReader(reader, parsedLength)) contentLength = parsedLength } else { // Content-Length header is missing. If we have a maximum file size @@ -802,7 +806,7 @@ func (r *downloadRequest) GetContentLengthAndReader(contentLengthHeader string, // ultimately it will get rewritten later when the temp file is written // to disk. if maxFileSizeBytes > 0 { - reader = io.NopCloser(io.LimitReader(*body, int64(maxFileSizeBytes))) + reader = io.NopCloser(io.LimitReader(reader, int64(maxFileSizeBytes))) } contentLength = 0 } @@ -818,19 +822,61 @@ func (r *downloadRequest) fetchRemoteFile( ) (types.Path, bool, error) { r.Logger.Debug("Fetching remote file") - // create request for remote file - resp, err := client.CreateMediaDownloadRequest(ctx, r.MediaMetadata.Origin, string(r.MediaMetadata.MediaID)) + // Attempt to download via authenticated media endpoint + isMultiPart := true + resp, err := r.fedClient.DownloadMedia(ctx, r.origin, r.MediaMetadata.Origin, string(r.MediaMetadata.MediaID)) if err != nil || (resp != nil && resp.StatusCode != http.StatusOK) { - if resp != nil && resp.StatusCode == http.StatusNotFound { - return "", false, fmt.Errorf("File with media ID %q does not exist on %s", r.MediaMetadata.MediaID, r.MediaMetadata.Origin) + isMultiPart = false + // try again on the unauthed endpoint + // create request for remote file + resp, err = client.CreateMediaDownloadRequest(ctx, r.MediaMetadata.Origin, string(r.MediaMetadata.MediaID)) + if err != nil || (resp != nil && resp.StatusCode != http.StatusOK) { + if resp != nil && resp.StatusCode == http.StatusNotFound { + return "", false, fmt.Errorf("File with media ID %q does not exist on %s", r.MediaMetadata.MediaID, r.MediaMetadata.Origin) + } + return "", false, fmt.Errorf("file with media ID %q could not be downloaded from %s", r.MediaMetadata.MediaID, r.MediaMetadata.Origin) } - return "", false, fmt.Errorf("file with media ID %q could not be downloaded from %s", r.MediaMetadata.MediaID, r.MediaMetadata.Origin) } defer resp.Body.Close() // nolint: errcheck - // The reader returned here will be limited either by the Content-Length - // and/or the configured maximum media size. - contentLength, reader, parseErr := r.GetContentLengthAndReader(resp.Header.Get("Content-Length"), &resp.Body, maxFileSizeBytes) + var contentLength int64 + var reader io.Reader + var parseErr error + if isMultiPart { + r.Logger.Info("Downloaded file using authenticated endpoint") + _, params, err := mime.ParseMediaType(resp.Header.Get("Content-Type")) + if err != nil { + panic(err) + } + if params["boundary"] == "" { + return "", false, fmt.Errorf("no boundary header found on %s", r.MediaMetadata.Origin) + } + mr := multipart.NewReader(resp.Body, params["boundary"]) + + first := true + for { + p, err := mr.NextPart() + if err == io.EOF { + break + } + if err != nil { + return "", false, err + } + + if !first { + readCloser := io.NopCloser(p) + contentLength, reader, parseErr = r.GetContentLengthAndReader(p.Header.Get("Content-Length"), readCloser, maxFileSizeBytes) + break + } + + first = false + } + } else { + // The reader returned here will be limited either by the Content-Length + // and/or the configured maximum media size. + contentLength, reader, parseErr = r.GetContentLengthAndReader(resp.Header.Get("Content-Length"), resp.Body, maxFileSizeBytes) + } + if parseErr != nil { return "", false, parseErr } diff --git a/mediaapi/routing/routing.go b/mediaapi/routing/routing.go index b5998d686b..82d397e00d 100644 --- a/mediaapi/routing/routing.go +++ b/mediaapi/routing/routing.go @@ -52,6 +52,7 @@ func Setup( db storage.Database, userAPI userapi.MediaUserAPI, client *fclient.Client, + federationClient fclient.FederationClient, keyRing gomatrixserverlib.JSONVerifier, ) { rateLimits := httputil.NewRateLimits(&cfg.ClientAPI.RateLimiting) @@ -95,12 +96,12 @@ func Setup( MXCToResult: map[string]*types.RemoteRequestResult{}, } - downloadHandler := makeDownloadAPI("download_unauthed", &cfg.MediaAPI, rateLimits, db, client, activeRemoteRequests, activeThumbnailGeneration, false) + downloadHandler := makeDownloadAPI("download_unauthed", &cfg.MediaAPI, rateLimits, db, client, federationClient, activeRemoteRequests, activeThumbnailGeneration, false) v3mux.Handle("/download/{serverName}/{mediaId}", downloadHandler).Methods(http.MethodGet, http.MethodOptions) v3mux.Handle("/download/{serverName}/{mediaId}/{downloadName}", downloadHandler).Methods(http.MethodGet, http.MethodOptions) v3mux.Handle("/thumbnail/{serverName}/{mediaId}", - makeDownloadAPI("thumbnail_unauthed", &cfg.MediaAPI, rateLimits, db, client, activeRemoteRequests, activeThumbnailGeneration, false), + makeDownloadAPI("thumbnail_unauthed", &cfg.MediaAPI, rateLimits, db, client, federationClient, activeRemoteRequests, activeThumbnailGeneration, false), ).Methods(http.MethodGet, http.MethodOptions) // v1 client endpoints requiring auth @@ -110,15 +111,15 @@ func Setup( v1mux.Handle("/download/{serverName}/{mediaId}/{downloadName}", downloadHandlerAuthed).Methods(http.MethodGet, http.MethodOptions) v1mux.Handle("/thumbnail/{serverName}/{mediaId}", - httputil.MakeHTMLAPI("thumbnail", userAPI, cfg.Global.Metrics.Enabled, makeDownloadAPI("thumbnail_authed_client", &cfg.MediaAPI, rateLimits, db, client, activeRemoteRequests, activeThumbnailGeneration, false), httputil.WithAuth()), + httputil.MakeHTMLAPI("thumbnail", userAPI, cfg.Global.Metrics.Enabled, makeDownloadAPI("thumbnail_authed_client", &cfg.MediaAPI, rateLimits, db, client, federationClient, activeRemoteRequests, activeThumbnailGeneration, false), httputil.WithAuth()), ).Methods(http.MethodGet, http.MethodOptions) // same, but for federation v1fedMux.Handle("/download/{mediaId}", routing.MakeFedAPIHTML(cfg.Global.ServerName, cfg.Global.IsLocalServerName, keyRing, - makeDownloadAPI("download_authed_federation", &cfg.MediaAPI, rateLimits, db, client, activeRemoteRequests, activeThumbnailGeneration, true), + makeDownloadAPI("download_authed_federation", &cfg.MediaAPI, rateLimits, db, client, federationClient, activeRemoteRequests, activeThumbnailGeneration, true), )).Methods(http.MethodGet, http.MethodOptions) v1fedMux.Handle("/thumbnail/{mediaId}", routing.MakeFedAPIHTML(cfg.Global.ServerName, cfg.Global.IsLocalServerName, keyRing, - makeDownloadAPI("thumbnail_authed_federation", &cfg.MediaAPI, rateLimits, db, client, activeRemoteRequests, activeThumbnailGeneration, true), + makeDownloadAPI("thumbnail_authed_federation", &cfg.MediaAPI, rateLimits, db, client, federationClient, activeRemoteRequests, activeThumbnailGeneration, true), )).Methods(http.MethodGet, http.MethodOptions) } @@ -170,6 +171,7 @@ func makeDownloadAPI( rateLimits *httputil.RateLimits, db storage.Database, client *fclient.Client, + fedClient fclient.FederationClient, activeRemoteRequests *types.ActiveRemoteRequests, activeThumbnailGeneration *types.ActiveThumbnailGeneration, forFederation bool, @@ -235,6 +237,7 @@ func makeDownloadAPI( cfg, db, client, + fedClient, activeRemoteRequests, activeThumbnailGeneration, strings.HasPrefix(name, "thumbnail"), diff --git a/setup/monolith.go b/setup/monolith.go index 11cc59e276..72750354b4 100644 --- a/setup/monolith.go +++ b/setup/monolith.go @@ -78,7 +78,7 @@ func (m *Monolith) AddAllPublicRoutes( federationapi.AddPublicRoutes( processCtx, routers, cfg, natsInstance, m.UserAPI, m.FedClient, m.KeyRing, m.RoomserverAPI, m.FederationAPI, enableMetrics, ) - mediaapi.AddPublicRoutes(routers, cm, cfg, m.UserAPI, m.Client, m.KeyRing) + mediaapi.AddPublicRoutes(routers, cm, cfg, m.UserAPI, m.Client, m.FedClient, m.KeyRing) syncapi.AddPublicRoutes(processCtx, routers, cfg, cm, natsInstance, m.UserAPI, m.RoomserverAPI, caches, enableMetrics) if m.RelayAPI != nil { From aeee46bb7159c610835036769aea43d391e04eb1 Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Wed, 19 Jun 2024 12:02:47 +0200 Subject: [PATCH 05/16] Announce support for MSC3916, fix wrong JSON message on error --- clientapi/routing/routing.go | 1 + internal/httputil/httpapi.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/clientapi/routing/routing.go b/clientapi/routing/routing.go index ced75303e0..4e7a05b825 100644 --- a/clientapi/routing/routing.go +++ b/clientapi/routing/routing.go @@ -94,6 +94,7 @@ func Setup( unstableFeatures := map[string]bool{ "org.matrix.e2e_cross_signing": true, "org.matrix.msc2285.stable": true, + "org.matrix.msc3916": true, } for _, msc := range cfg.MSCs.MSCs { unstableFeatures["org.matrix."+msc] = true diff --git a/internal/httputil/httpapi.go b/internal/httputil/httpapi.go index 26b2f4d734..921c12a0e9 100644 --- a/internal/httputil/httpapi.go +++ b/internal/httputil/httpapi.go @@ -226,7 +226,7 @@ func MakeHTMLAPI(metricsName string, userAPI userapi.QueryAcccessTokenAPI, enabl if jsonErr != nil { logger.Debugf("VerifyUserFromRequest %s -> HTTP %d", req.RemoteAddr, jsonErr.Code) w.WriteHeader(jsonErr.Code) - if err := json.NewEncoder(w).Encode(jsonErr); err != nil { + if err := json.NewEncoder(w).Encode(jsonErr.JSON); err != nil { logger.WithError(err).Error("failed to encode JSON response") } return From 51f6dcbdc585cb5addb4b4c285bea60ff347e979 Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Wed, 19 Jun 2024 12:05:30 +0200 Subject: [PATCH 06/16] Make the linter happy --- mediaapi/routing/download.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/mediaapi/routing/download.go b/mediaapi/routing/download.go index 93040bcba8..f2303b3f2b 100644 --- a/mediaapi/routing/download.go +++ b/mediaapi/routing/download.go @@ -370,7 +370,7 @@ func (r *downloadRequest) respondFromLocalFile( }).Trace("Responding with file") responseFile = file responseMetadata = r.MediaMetadata - if err := r.addDownloadFilenameToHeaders(w, responseMetadata); err != nil { + if err = r.addDownloadFilenameToHeaders(w, responseMetadata); err != nil { return nil, err } } @@ -385,7 +385,7 @@ func (r *downloadRequest) respondFromLocalFile( w.Header().Set("Content-Security-Policy", contentSecurityPolicy) if !r.forFederation { - if _, err := io.Copy(w, responseFile); err != nil { + if _, err = io.Copy(w, responseFile); err != nil { return nil, fmt.Errorf("io.Copy: %w", err) } } else { @@ -814,6 +814,7 @@ func (r *downloadRequest) GetContentLengthAndReader(contentLengthHeader string, return contentLength, reader, nil } +// nolint: gocyclo func (r *downloadRequest) fetchRemoteFile( ctx context.Context, client *fclient.Client, @@ -844,7 +845,8 @@ func (r *downloadRequest) fetchRemoteFile( var parseErr error if isMultiPart { r.Logger.Info("Downloaded file using authenticated endpoint") - _, params, err := mime.ParseMediaType(resp.Header.Get("Content-Type")) + var params map[string]string + _, params, err = mime.ParseMediaType(resp.Header.Get("Content-Type")) if err != nil { panic(err) } @@ -855,7 +857,8 @@ func (r *downloadRequest) fetchRemoteFile( first := true for { - p, err := mr.NextPart() + var p *multipart.Part + p, err = mr.NextPart() if err == io.EOF { break } From 53e545ddc5a9e90faa3aed04b43850cfe52a90c8 Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Tue, 2 Jul 2024 08:51:16 +0200 Subject: [PATCH 07/16] Update GMSL --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 5c8155281d..d071e4d50f 100644 --- a/go.mod +++ b/go.mod @@ -22,7 +22,7 @@ require ( github.com/matrix-org/dugong v0.0.0-20210921133753-66e6b1c67e2e github.com/matrix-org/go-sqlite3-js v0.0.0-20220419092513-28aa791a1c91 github.com/matrix-org/gomatrix v0.0.0-20220926102614-ceba4d9f7530 - github.com/matrix-org/gomatrixserverlib v0.0.0-20240328203753-c2391f7113a5 + github.com/matrix-org/gomatrixserverlib v0.0.0-20240618131621-ba4c4992f66d github.com/matrix-org/pinecone v0.11.1-0.20230810010612-ea4c33717fd7 github.com/matrix-org/util v0.0.0-20221111132719-399730281e66 github.com/mattn/go-sqlite3 v1.14.22 diff --git a/go.sum b/go.sum index 2ffa2c8732..d82b10152e 100644 --- a/go.sum +++ b/go.sum @@ -211,8 +211,8 @@ github.com/matrix-org/go-sqlite3-js v0.0.0-20220419092513-28aa791a1c91 h1:s7fexw github.com/matrix-org/go-sqlite3-js v0.0.0-20220419092513-28aa791a1c91/go.mod h1:e+cg2q7C7yE5QnAXgzo512tgFh1RbQLC0+jozuegKgo= github.com/matrix-org/gomatrix v0.0.0-20220926102614-ceba4d9f7530 h1:kHKxCOLcHH8r4Fzarl4+Y3K5hjothkVW5z7T1dUM11U= github.com/matrix-org/gomatrix v0.0.0-20220926102614-ceba4d9f7530/go.mod h1:/gBX06Kw0exX1HrwmoBibFA98yBk/jxKpGVeyQbff+s= -github.com/matrix-org/gomatrixserverlib v0.0.0-20240328203753-c2391f7113a5 h1:GuxmpyjZQoqb6UFQgKq8Td3wIITlXln/sItqp1jbTTA= -github.com/matrix-org/gomatrixserverlib v0.0.0-20240328203753-c2391f7113a5/go.mod h1:HZGsVJ3bUE+DkZtufkH9H0mlsvbhEGK5CpX0Zlavylg= +github.com/matrix-org/gomatrixserverlib v0.0.0-20240618131621-ba4c4992f66d h1:drDloOznSn5b9vhrsK5LNziiPECZDvgqwYim6s4hmqU= +github.com/matrix-org/gomatrixserverlib v0.0.0-20240618131621-ba4c4992f66d/go.mod h1:HZGsVJ3bUE+DkZtufkH9H0mlsvbhEGK5CpX0Zlavylg= github.com/matrix-org/pinecone v0.11.1-0.20230810010612-ea4c33717fd7 h1:6t8kJr8i1/1I5nNttw6nn1ryQJgzVlBmSGgPiiaTdw4= github.com/matrix-org/pinecone v0.11.1-0.20230810010612-ea4c33717fd7/go.mod h1:ReWMS/LoVnOiRAdq9sNUC2NZnd1mZkMNB52QhpTRWjg= github.com/matrix-org/util v0.0.0-20221111132719-399730281e66 h1:6z4KxomXSIGWqhHcfzExgkH3Z3UkIXry4ibJS4Aqz2Y= From e8b69fba0306dc0af25654d9327655463666bde4 Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Tue, 2 Jul 2024 08:52:28 +0200 Subject: [PATCH 08/16] Set correct Content-Type --- mediaapi/routing/download.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/mediaapi/routing/download.go b/mediaapi/routing/download.go index f2303b3f2b..15544d3c82 100644 --- a/mediaapi/routing/download.go +++ b/mediaapi/routing/download.go @@ -840,6 +840,10 @@ func (r *downloadRequest) fetchRemoteFile( } defer resp.Body.Close() // nolint: errcheck + // If this wasn't a multipart response, set the Content-Type now. Will be overwritten + // by the multipart Content-Type below. + r.MediaMetadata.ContentType = types.ContentType(resp.Header.Get("Content-Type")) + var contentLength int64 var reader io.Reader var parseErr error @@ -869,6 +873,8 @@ func (r *downloadRequest) fetchRemoteFile( if !first { readCloser := io.NopCloser(p) contentLength, reader, parseErr = r.GetContentLengthAndReader(p.Header.Get("Content-Length"), readCloser, maxFileSizeBytes) + // For multipart requests, we need to get the Content-Type of the second part, which is the actual media + r.MediaMetadata.ContentType = types.ContentType(p.Header.Get("Content-Type")) break } @@ -890,7 +896,6 @@ func (r *downloadRequest) fetchRemoteFile( } r.MediaMetadata.FileSizeBytes = types.FileSizeBytes(contentLength) - r.MediaMetadata.ContentType = types.ContentType(resp.Header.Get("Content-Type")) dispositionHeader := resp.Header.Get("Content-Disposition") if _, params, e := mime.ParseMediaType(dispositionHeader); e == nil { From 48a3ce0d85ace7f1214f8bd25c249dba1bb353f1 Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Fri, 2 Aug 2024 20:07:23 +0200 Subject: [PATCH 09/16] Bump GMSL --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 4879baede5..033d52c6d9 100644 --- a/go.mod +++ b/go.mod @@ -22,7 +22,7 @@ require ( github.com/matrix-org/dugong v0.0.0-20210921133753-66e6b1c67e2e github.com/matrix-org/go-sqlite3-js v0.0.0-20220419092513-28aa791a1c91 github.com/matrix-org/gomatrix v0.0.0-20220926102614-ceba4d9f7530 - github.com/matrix-org/gomatrixserverlib v0.0.0-20240618131621-ba4c4992f66d + github.com/matrix-org/gomatrixserverlib v0.0.0-20240801173829-d531860ad2cb github.com/matrix-org/pinecone v0.11.1-0.20230810010612-ea4c33717fd7 github.com/matrix-org/util v0.0.0-20221111132719-399730281e66 github.com/mattn/go-sqlite3 v1.14.22 diff --git a/go.sum b/go.sum index b5fefbb34b..0795537d7f 100644 --- a/go.sum +++ b/go.sum @@ -233,8 +233,8 @@ github.com/matrix-org/go-sqlite3-js v0.0.0-20220419092513-28aa791a1c91 h1:s7fexw github.com/matrix-org/go-sqlite3-js v0.0.0-20220419092513-28aa791a1c91/go.mod h1:e+cg2q7C7yE5QnAXgzo512tgFh1RbQLC0+jozuegKgo= github.com/matrix-org/gomatrix v0.0.0-20220926102614-ceba4d9f7530 h1:kHKxCOLcHH8r4Fzarl4+Y3K5hjothkVW5z7T1dUM11U= github.com/matrix-org/gomatrix v0.0.0-20220926102614-ceba4d9f7530/go.mod h1:/gBX06Kw0exX1HrwmoBibFA98yBk/jxKpGVeyQbff+s= -github.com/matrix-org/gomatrixserverlib v0.0.0-20240618131621-ba4c4992f66d h1:drDloOznSn5b9vhrsK5LNziiPECZDvgqwYim6s4hmqU= -github.com/matrix-org/gomatrixserverlib v0.0.0-20240618131621-ba4c4992f66d/go.mod h1:HZGsVJ3bUE+DkZtufkH9H0mlsvbhEGK5CpX0Zlavylg= +github.com/matrix-org/gomatrixserverlib v0.0.0-20240801173829-d531860ad2cb h1:vb9RyAU+5r5jGTIjlteq8XK71X6Q+fqnmh8gSUUuLrI= +github.com/matrix-org/gomatrixserverlib v0.0.0-20240801173829-d531860ad2cb/go.mod h1:HZGsVJ3bUE+DkZtufkH9H0mlsvbhEGK5CpX0Zlavylg= github.com/matrix-org/pinecone v0.11.1-0.20230810010612-ea4c33717fd7 h1:6t8kJr8i1/1I5nNttw6nn1ryQJgzVlBmSGgPiiaTdw4= github.com/matrix-org/pinecone v0.11.1-0.20230810010612-ea4c33717fd7/go.mod h1:ReWMS/LoVnOiRAdq9sNUC2NZnd1mZkMNB52QhpTRWjg= github.com/matrix-org/util v0.0.0-20221111132719-399730281e66 h1:6z4KxomXSIGWqhHcfzExgkH3Z3UkIXry4ibJS4Aqz2Y= From c024c15a6ee73592737fb8db866cd4cda8ec74f1 Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Fri, 2 Aug 2024 20:17:00 +0200 Subject: [PATCH 10/16] Simplify parsing multipart responses --- mediaapi/routing/download.go | 39 ++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/mediaapi/routing/download.go b/mediaapi/routing/download.go index 15544d3c82..3129948dde 100644 --- a/mediaapi/routing/download.go +++ b/mediaapi/routing/download.go @@ -848,38 +848,37 @@ func (r *downloadRequest) fetchRemoteFile( var reader io.Reader var parseErr error if isMultiPart { - r.Logger.Info("Downloaded file using authenticated endpoint") + r.Logger.Debug("Downloaded file using authenticated endpoint") var params map[string]string _, params, err = mime.ParseMediaType(resp.Header.Get("Content-Type")) if err != nil { panic(err) } if params["boundary"] == "" { - return "", false, fmt.Errorf("no boundary header found on %s", r.MediaMetadata.Origin) + return "", false, fmt.Errorf("no boundary header found on media %s from %s", r.MediaMetadata.MediaID, r.MediaMetadata.Origin) } mr := multipart.NewReader(resp.Body, params["boundary"]) - first := true - for { - var p *multipart.Part - p, err = mr.NextPart() - if err == io.EOF { - break - } - if err != nil { - return "", false, err - } + // Get the first, JSON, part + p, multipartErr := mr.NextPart() + if multipartErr != nil { + return "", false, multipartErr + } - if !first { - readCloser := io.NopCloser(p) - contentLength, reader, parseErr = r.GetContentLengthAndReader(p.Header.Get("Content-Length"), readCloser, maxFileSizeBytes) - // For multipart requests, we need to get the Content-Type of the second part, which is the actual media - r.MediaMetadata.ContentType = types.ContentType(p.Header.Get("Content-Type")) - break - } + if p.Header.Get("Content-Type") != "application/json" { + return "", false, fmt.Errorf("first part of the response must be application/json") + } + // TODO: Once something is defined, parse the JSON content - first = false + // Get the actual media content + p, multipartErr = mr.NextPart() + if multipartErr != nil { + return "", false, multipartErr } + + contentLength, reader, parseErr = r.GetContentLengthAndReader(p.Header.Get("Content-Length"), p, maxFileSizeBytes) + // For multipart requests, we need to get the Content-Type of the second part, which is the actual media + r.MediaMetadata.ContentType = types.ContentType(p.Header.Get("Content-Type")) } else { // The reader returned here will be limited either by the Content-Length // and/or the configured maximum media size. From 7451e203c730987395da32ab163bcad8a9c876d6 Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Sun, 4 Aug 2024 09:58:44 +0200 Subject: [PATCH 11/16] Add support for the Location header (by @tulir), more review changes --- mediaapi/routing/download.go | 45 +++++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/mediaapi/routing/download.go b/mediaapi/routing/download.go index 3129948dde..3ceb12a591 100644 --- a/mediaapi/routing/download.go +++ b/mediaapi/routing/download.go @@ -382,9 +382,9 @@ func (r *downloadRequest) respondFromLocalFile( " plugin-types application/pdf;" + " style-src 'unsafe-inline';" + " object-src 'self';" - w.Header().Set("Content-Security-Policy", contentSecurityPolicy) if !r.forFederation { + w.Header().Set("Content-Security-Policy", contentSecurityPolicy) if _, err = io.Copy(w, responseFile); err != nil { return nil, fmt.Errorf("io.Copy: %w", err) } @@ -393,8 +393,7 @@ func (r *downloadRequest) respondFromLocalFile( boundary := uuid.NewString() w.Header().Set("Content-Type", "multipart/mixed; boundary="+boundary) - w.Header().Del("Content-Length") // let Go handle the content length - w.Header().Del("Content-Security-Policy") // S-S request, so does not really matter? + w.Header().Del("Content-Length") // let Go handle the content length mw := multipart.NewWriter(w) defer func() { if err = mw.Close(); err != nil { @@ -814,6 +813,10 @@ func (r *downloadRequest) GetContentLengthAndReader(contentLengthHeader string, return contentLength, reader, nil } +// mediaMeta contains information about a multipart media response. +// TODO: extend once something is defined. +type mediaMeta struct{} + // nolint: gocyclo func (r *downloadRequest) fetchRemoteFile( ctx context.Context, @@ -852,7 +855,7 @@ func (r *downloadRequest) fetchRemoteFile( var params map[string]string _, params, err = mime.ParseMediaType(resp.Header.Get("Content-Type")) if err != nil { - panic(err) + return "", false, err } if params["boundary"] == "" { return "", false, fmt.Errorf("no boundary header found on media %s from %s", r.MediaMetadata.MediaID, r.MediaMetadata.Origin) @@ -868,7 +871,12 @@ func (r *downloadRequest) fetchRemoteFile( if p.Header.Get("Content-Type") != "application/json" { return "", false, fmt.Errorf("first part of the response must be application/json") } - // TODO: Once something is defined, parse the JSON content + // Try to parse media meta information + meta := mediaMeta{} + if err = json.NewDecoder(p).Decode(&meta); err != nil { + return "", false, err + } + defer p.Close() // nolint: errcheck // Get the actual media content p, multipartErr = mr.NextPart() @@ -876,9 +884,30 @@ func (r *downloadRequest) fetchRemoteFile( return "", false, multipartErr } - contentLength, reader, parseErr = r.GetContentLengthAndReader(p.Header.Get("Content-Length"), p, maxFileSizeBytes) - // For multipart requests, we need to get the Content-Type of the second part, which is the actual media - r.MediaMetadata.ContentType = types.ContentType(p.Header.Get("Content-Type")) + redirect := p.Header.Get("Location") + if redirect != "" { + if !strings.HasPrefix(redirect, "https://") { + return "", false, fmt.Errorf("redirect URL must be HTTPS") + } + req, reqErr := http.NewRequest(http.MethodGet, redirect, nil) + if reqErr != nil { + return "", false, fmt.Errorf("failed to create request to %s: %w", redirect, err) + } + redirectResp, reqErr := client.DoHTTPRequest(ctx, req) + if reqErr != nil { + return "", false, fmt.Errorf("error following redirect: %w", err) + } + defer redirectResp.Body.Close() // nolint: errcheck + if redirectResp.StatusCode != http.StatusOK { + return "", false, fmt.Errorf("unexpected status code %d after following redirect", resp.StatusCode) + } + contentLength, reader, parseErr = r.GetContentLengthAndReader(redirectResp.Header.Get("Content-Length"), redirectResp.Body, maxFileSizeBytes) + r.MediaMetadata.ContentType = types.ContentType(redirectResp.Header.Get("Content-Type")) + } else { + contentLength, reader, parseErr = r.GetContentLengthAndReader(p.Header.Get("Content-Length"), p, maxFileSizeBytes) + // For multipart requests, we need to get the Content-Type of the second part, which is the actual media + r.MediaMetadata.ContentType = types.ContentType(p.Header.Get("Content-Type")) + } } else { // The reader returned here will be limited either by the Content-Length // and/or the configured maximum media size. From c2aa33e983c7426457b7e5f441b06accad3cb7fc Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Sun, 4 Aug 2024 19:04:26 +0200 Subject: [PATCH 12/16] Use `.stable`, fix prometheus metric --- clientapi/routing/routing.go | 2 +- mediaapi/routing/routing.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/clientapi/routing/routing.go b/clientapi/routing/routing.go index 4e7a05b825..cce150343c 100644 --- a/clientapi/routing/routing.go +++ b/clientapi/routing/routing.go @@ -94,7 +94,7 @@ func Setup( unstableFeatures := map[string]bool{ "org.matrix.e2e_cross_signing": true, "org.matrix.msc2285.stable": true, - "org.matrix.msc3916": true, + "org.matrix.msc3916.stable": true, } for _, msc := range cfg.MSCs.MSCs { unstableFeatures["org.matrix."+msc] = true diff --git a/mediaapi/routing/routing.go b/mediaapi/routing/routing.go index 82d397e00d..63d5cbcf4a 100644 --- a/mediaapi/routing/routing.go +++ b/mediaapi/routing/routing.go @@ -105,7 +105,7 @@ func Setup( ).Methods(http.MethodGet, http.MethodOptions) // v1 client endpoints requiring auth - downloadHandlerAuthed := httputil.MakeHTMLAPI("download_authed_client", userAPI, cfg.Global.Metrics.Enabled, downloadHandler, httputil.WithAuth()) + downloadHandlerAuthed := httputil.MakeHTMLAPI("download", userAPI, cfg.Global.Metrics.Enabled, makeDownloadAPI("download_authed_client", &cfg.MediaAPI, rateLimits, db, client, federationClient, activeRemoteRequests, activeThumbnailGeneration, false), httputil.WithAuth()) v1mux.Handle("/config", configHandler).Methods(http.MethodGet, http.MethodOptions) v1mux.Handle("/download/{serverName}/{mediaId}", downloadHandlerAuthed).Methods(http.MethodGet, http.MethodOptions) v1mux.Handle("/download/{serverName}/{mediaId}/{downloadName}", downloadHandlerAuthed).Methods(http.MethodGet, http.MethodOptions) From a3c76e9f2f682670e93273768877a5a536d187f5 Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Thu, 8 Aug 2024 19:30:17 +0200 Subject: [PATCH 13/16] Review comments --- federationapi/routing/routing.go | 4 ++-- mediaapi/routing/download.go | 30 +++++++++++++++--------------- mediaapi/routing/routing.go | 4 ++-- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/federationapi/routing/routing.go b/federationapi/routing/routing.go index 5a735cf5e6..810fa9022f 100644 --- a/federationapi/routing/routing.go +++ b/federationapi/routing/routing.go @@ -679,8 +679,8 @@ func MakeFedAPI( return httputil.MakeExternalAPI(metricsName, h) } -// MakeFedAPI makes an http.Handler that checks matrix federation authentication. -func MakeFedAPIHTML( +// MakeFedHTMLAPI makes an http.Handler that checks matrix federation authentication. +func MakeFedHTMLAPI( serverName spec.ServerName, isLocalServerName func(spec.ServerName) bool, keyRing gomatrixserverlib.JSONVerifier, diff --git a/mediaapi/routing/download.go b/mediaapi/routing/download.go index 3ceb12a591..16ba0c51b9 100644 --- a/mediaapi/routing/download.go +++ b/mediaapi/routing/download.go @@ -64,7 +64,7 @@ type downloadRequest struct { ThumbnailSize types.ThumbnailSize Logger *log.Entry DownloadFilename string - forFederation bool // whether we need to return a multipart/mixed response + multipartResponse bool // whether we need to return a multipart/mixed response (for requests coming in over federation) fedClient fclient.FederationClient origin spec.ServerName } @@ -122,10 +122,10 @@ func Download( activeThumbnailGeneration *types.ActiveThumbnailGeneration, isThumbnailRequest bool, customFilename string, - forFederation bool, + federationRequest bool, ) { // This happens if we call Download for a federation request - if forFederation && origin == "" { + if federationRequest && origin == "" { origin = cfg.Matrix.ServerName } dReq := &downloadRequest{ @@ -138,10 +138,10 @@ func Download( "Origin": origin, "MediaID": mediaID, }), - DownloadFilename: customFilename, - forFederation: forFederation, - origin: cfg.Matrix.ServerName, - fedClient: fedClient, + DownloadFilename: customFilename, + multipartResponse: federationRequest, + origin: cfg.Matrix.ServerName, + fedClient: fedClient, } if dReq.IsThumbnailRequest { @@ -383,7 +383,7 @@ func (r *downloadRequest) respondFromLocalFile( " style-src 'unsafe-inline';" + " object-src 'self';" - if !r.forFederation { + if !r.multipartResponse { w.Header().Set("Content-Security-Policy", contentSecurityPolicy) if _, err = io.Copy(w, responseFile); err != nil { return nil, fmt.Errorf("io.Copy: %w", err) @@ -827,10 +827,10 @@ func (r *downloadRequest) fetchRemoteFile( r.Logger.Debug("Fetching remote file") // Attempt to download via authenticated media endpoint - isMultiPart := true + isAuthed := true resp, err := r.fedClient.DownloadMedia(ctx, r.origin, r.MediaMetadata.Origin, string(r.MediaMetadata.MediaID)) if err != nil || (resp != nil && resp.StatusCode != http.StatusOK) { - isMultiPart = false + isAuthed = false // try again on the unauthed endpoint // create request for remote file resp, err = client.CreateMediaDownloadRequest(ctx, r.MediaMetadata.Origin, string(r.MediaMetadata.MediaID)) @@ -838,7 +838,7 @@ func (r *downloadRequest) fetchRemoteFile( if resp != nil && resp.StatusCode == http.StatusNotFound { return "", false, fmt.Errorf("File with media ID %q does not exist on %s", r.MediaMetadata.MediaID, r.MediaMetadata.Origin) } - return "", false, fmt.Errorf("file with media ID %q could not be downloaded from %s", r.MediaMetadata.MediaID, r.MediaMetadata.Origin) + return "", false, fmt.Errorf("file with media ID %q could not be downloaded from %s: %w", r.MediaMetadata.MediaID, r.MediaMetadata.Origin, err) } } defer resp.Body.Close() // nolint: errcheck @@ -850,7 +850,7 @@ func (r *downloadRequest) fetchRemoteFile( var contentLength int64 var reader io.Reader var parseErr error - if isMultiPart { + if isAuthed { r.Logger.Debug("Downloaded file using authenticated endpoint") var params map[string]string _, params, err = mime.ParseMediaType(resp.Header.Get("Content-Type")) @@ -891,15 +891,15 @@ func (r *downloadRequest) fetchRemoteFile( } req, reqErr := http.NewRequest(http.MethodGet, redirect, nil) if reqErr != nil { - return "", false, fmt.Errorf("failed to create request to %s: %w", redirect, err) + return "", false, fmt.Errorf("failed to create request to %s: %w", redirect, reqErr) } redirectResp, reqErr := client.DoHTTPRequest(ctx, req) if reqErr != nil { - return "", false, fmt.Errorf("error following redirect: %w", err) + return "", false, fmt.Errorf("error following redirect: %w", reqErr) } defer redirectResp.Body.Close() // nolint: errcheck if redirectResp.StatusCode != http.StatusOK { - return "", false, fmt.Errorf("unexpected status code %d after following redirect", resp.StatusCode) + return "", false, fmt.Errorf("unexpected status code %d after following redirect", redirectResp.StatusCode) } contentLength, reader, parseErr = r.GetContentLengthAndReader(redirectResp.Header.Get("Content-Length"), redirectResp.Body, maxFileSizeBytes) r.MediaMetadata.ContentType = types.ContentType(redirectResp.Header.Get("Content-Type")) diff --git a/mediaapi/routing/routing.go b/mediaapi/routing/routing.go index 63d5cbcf4a..20417d1cc1 100644 --- a/mediaapi/routing/routing.go +++ b/mediaapi/routing/routing.go @@ -115,10 +115,10 @@ func Setup( ).Methods(http.MethodGet, http.MethodOptions) // same, but for federation - v1fedMux.Handle("/download/{mediaId}", routing.MakeFedAPIHTML(cfg.Global.ServerName, cfg.Global.IsLocalServerName, keyRing, + v1fedMux.Handle("/download/{mediaId}", routing.MakeFedHTMLAPI(cfg.Global.ServerName, cfg.Global.IsLocalServerName, keyRing, makeDownloadAPI("download_authed_federation", &cfg.MediaAPI, rateLimits, db, client, federationClient, activeRemoteRequests, activeThumbnailGeneration, true), )).Methods(http.MethodGet, http.MethodOptions) - v1fedMux.Handle("/thumbnail/{mediaId}", routing.MakeFedAPIHTML(cfg.Global.ServerName, cfg.Global.IsLocalServerName, keyRing, + v1fedMux.Handle("/thumbnail/{mediaId}", routing.MakeFedHTMLAPI(cfg.Global.ServerName, cfg.Global.IsLocalServerName, keyRing, makeDownloadAPI("thumbnail_authed_federation", &cfg.MediaAPI, rateLimits, db, client, federationClient, activeRemoteRequests, activeThumbnailGeneration, true), )).Methods(http.MethodGet, http.MethodOptions) } From 82e89743a12d98543d03c86ccdb840e39340354b Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Thu, 8 Aug 2024 20:04:44 +0200 Subject: [PATCH 14/16] Remove Location forwarding --- mediaapi/routing/download.go | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/mediaapi/routing/download.go b/mediaapi/routing/download.go index 16ba0c51b9..ada9098f80 100644 --- a/mediaapi/routing/download.go +++ b/mediaapi/routing/download.go @@ -886,23 +886,7 @@ func (r *downloadRequest) fetchRemoteFile( redirect := p.Header.Get("Location") if redirect != "" { - if !strings.HasPrefix(redirect, "https://") { - return "", false, fmt.Errorf("redirect URL must be HTTPS") - } - req, reqErr := http.NewRequest(http.MethodGet, redirect, nil) - if reqErr != nil { - return "", false, fmt.Errorf("failed to create request to %s: %w", redirect, reqErr) - } - redirectResp, reqErr := client.DoHTTPRequest(ctx, req) - if reqErr != nil { - return "", false, fmt.Errorf("error following redirect: %w", reqErr) - } - defer redirectResp.Body.Close() // nolint: errcheck - if redirectResp.StatusCode != http.StatusOK { - return "", false, fmt.Errorf("unexpected status code %d after following redirect", redirectResp.StatusCode) - } - contentLength, reader, parseErr = r.GetContentLengthAndReader(redirectResp.Header.Get("Content-Length"), redirectResp.Body, maxFileSizeBytes) - r.MediaMetadata.ContentType = types.ContentType(redirectResp.Header.Get("Content-Type")) + return "", false, fmt.Errorf("Location header is not yet supported") } else { contentLength, reader, parseErr = r.GetContentLengthAndReader(p.Header.Get("Content-Length"), p, maxFileSizeBytes) // For multipart requests, we need to get the Content-Type of the second part, which is the actual media From 919208be7ac16cda89649201e1bef3dc415723ce Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Fri, 16 Aug 2024 11:50:03 +0200 Subject: [PATCH 15/16] Add tests and comments --- mediaapi/routing/download.go | 159 ++++++++++++++++-------------- mediaapi/routing/download_test.go | 30 ++++++ mediaapi/routing/routing.go | 11 ++- 3 files changed, 122 insertions(+), 78 deletions(-) diff --git a/mediaapi/routing/download.go b/mediaapi/routing/download.go index ada9098f80..c812b9d65e 100644 --- a/mediaapi/routing/download.go +++ b/mediaapi/routing/download.go @@ -389,45 +389,52 @@ func (r *downloadRequest) respondFromLocalFile( return nil, fmt.Errorf("io.Copy: %w", err) } } else { - // Update the header to be multipart/mixed; boundary=$randomBoundary - boundary := uuid.NewString() - w.Header().Set("Content-Type", "multipart/mixed; boundary="+boundary) + var written int64 + written, err = multipartResponse(w, r, string(responseMetadata.ContentType), responseFile) + if err != nil { + return nil, err + } + responseMetadata.FileSizeBytes = types.FileSizeBytes(written) + } + return responseMetadata, nil +} - w.Header().Del("Content-Length") // let Go handle the content length - mw := multipart.NewWriter(w) - defer func() { - if err = mw.Close(); err != nil { - r.Logger.WithError(err).Error("Failed to close multipart writer") - } - }() +func multipartResponse(w http.ResponseWriter, r *downloadRequest, contentType string, responseFile io.Reader) (int64, error) { + // Update the header to be multipart/mixed; boundary=$randomBoundary + boundary := uuid.NewString() + w.Header().Set("Content-Type", "multipart/mixed; boundary="+boundary) - if err = mw.SetBoundary(boundary); err != nil { - return nil, fmt.Errorf("failed to set multipart boundary: %w", err) + w.Header().Del("Content-Length") // let Go handle the content length + mw := multipart.NewWriter(w) + defer func() { + if err := mw.Close(); err != nil { + r.Logger.WithError(err).Error("Failed to close multipart writer") } + }() - // JSON object part - jsonWriter, err := mw.CreatePart(textproto.MIMEHeader{ - "Content-Type": {"application/json"}, - }) - if err != nil { - return nil, fmt.Errorf("failed to create json writer: %w", err) - } - if _, err = jsonWriter.Write([]byte("{}")); err != nil { - return nil, fmt.Errorf("failed to write to json writer: %w", err) - } + if err := mw.SetBoundary(boundary); err != nil { + return 0, fmt.Errorf("failed to set multipart boundary: %w", err) + } - // media part - mediaWriter, err := mw.CreatePart(textproto.MIMEHeader{ - "Content-Type": {string(responseMetadata.ContentType)}, - }) - if err != nil { - return nil, fmt.Errorf("failed to create media writer: %w", err) - } - if _, err = io.Copy(mediaWriter, responseFile); err != nil { - return nil, fmt.Errorf("failed to write to media writer: %w", err) - } + // JSON object part + jsonWriter, err := mw.CreatePart(textproto.MIMEHeader{ + "Content-Type": {"application/json"}, + }) + if err != nil { + return 0, fmt.Errorf("failed to create json writer: %w", err) } - return responseMetadata, nil + if _, err = jsonWriter.Write([]byte("{}")); err != nil { + return 0, fmt.Errorf("failed to write to json writer: %w", err) + } + + // media part + mediaWriter, err := mw.CreatePart(textproto.MIMEHeader{ + "Content-Type": {contentType}, + }) + if err != nil { + return 0, fmt.Errorf("failed to create media writer: %w", err) + } + return io.Copy(mediaWriter, responseFile) } func (r *downloadRequest) addDownloadFilenameToHeaders( @@ -851,47 +858,7 @@ func (r *downloadRequest) fetchRemoteFile( var reader io.Reader var parseErr error if isAuthed { - r.Logger.Debug("Downloaded file using authenticated endpoint") - var params map[string]string - _, params, err = mime.ParseMediaType(resp.Header.Get("Content-Type")) - if err != nil { - return "", false, err - } - if params["boundary"] == "" { - return "", false, fmt.Errorf("no boundary header found on media %s from %s", r.MediaMetadata.MediaID, r.MediaMetadata.Origin) - } - mr := multipart.NewReader(resp.Body, params["boundary"]) - - // Get the first, JSON, part - p, multipartErr := mr.NextPart() - if multipartErr != nil { - return "", false, multipartErr - } - - if p.Header.Get("Content-Type") != "application/json" { - return "", false, fmt.Errorf("first part of the response must be application/json") - } - // Try to parse media meta information - meta := mediaMeta{} - if err = json.NewDecoder(p).Decode(&meta); err != nil { - return "", false, err - } - defer p.Close() // nolint: errcheck - - // Get the actual media content - p, multipartErr = mr.NextPart() - if multipartErr != nil { - return "", false, multipartErr - } - - redirect := p.Header.Get("Location") - if redirect != "" { - return "", false, fmt.Errorf("Location header is not yet supported") - } else { - contentLength, reader, parseErr = r.GetContentLengthAndReader(p.Header.Get("Content-Length"), p, maxFileSizeBytes) - // For multipart requests, we need to get the Content-Type of the second part, which is the actual media - r.MediaMetadata.ContentType = types.ContentType(p.Header.Get("Content-Type")) - } + parseErr, contentLength, reader = parseMultipartResponse(r, resp, maxFileSizeBytes) } else { // The reader returned here will be limited either by the Content-Length // and/or the configured maximum media size. @@ -961,6 +928,50 @@ func (r *downloadRequest) fetchRemoteFile( return types.Path(finalPath), duplicate, nil } +func parseMultipartResponse(r *downloadRequest, resp *http.Response, maxFileSizeBytes config.FileSizeBytes) (error, int64, io.Reader) { + _, params, err := mime.ParseMediaType(resp.Header.Get("Content-Type")) + if err != nil { + return err, 0, nil + } + if params["boundary"] == "" { + return fmt.Errorf("no boundary header found on media %s from %s", r.MediaMetadata.MediaID, r.MediaMetadata.Origin), 0, nil + } + mr := multipart.NewReader(resp.Body, params["boundary"]) + + // Get the first, JSON, part + p, err := mr.NextPart() + if err != nil { + return err, 0, nil + } + defer p.Close() // nolint: errcheck + + if p.Header.Get("Content-Type") != "application/json" { + return fmt.Errorf("first part of the response must be application/json"), 0, nil + } + // Try to parse media meta information + meta := mediaMeta{} + if err = json.NewDecoder(p).Decode(&meta); err != nil { + return err, 0, nil + } + defer p.Close() // nolint: errcheck + + // Get the actual media content + p, err = mr.NextPart() + if err != nil { + return err, 0, nil + } + + redirect := p.Header.Get("Location") + if redirect != "" { + return fmt.Errorf("Location header is not yet supported"), 0, nil + } + + contentLength, reader, err := r.GetContentLengthAndReader(p.Header.Get("Content-Length"), p, maxFileSizeBytes) + // For multipart requests, we need to get the Content-Type of the second part, which is the actual media + r.MediaMetadata.ContentType = types.ContentType(p.Header.Get("Content-Type")) + return err, contentLength, reader +} + // contentDispositionFor returns the Content-Disposition for a given // content type. func contentDispositionFor(contentType types.ContentType) string { diff --git a/mediaapi/routing/download_test.go b/mediaapi/routing/download_test.go index 21f6bfc2c7..11368919ae 100644 --- a/mediaapi/routing/download_test.go +++ b/mediaapi/routing/download_test.go @@ -1,8 +1,13 @@ package routing import ( + "bytes" + "io" + "net/http" + "net/http/httptest" "testing" + "github.com/matrix-org/dendrite/mediaapi/types" "github.com/stretchr/testify/assert" ) @@ -11,3 +16,28 @@ func Test_dispositionFor(t *testing.T) { assert.Equal(t, "attachment", contentDispositionFor("image/svg"), "image/svg") assert.Equal(t, "inline", contentDispositionFor("image/jpeg"), "image/jpg") } + +func Test_Multipart(t *testing.T) { + r := &downloadRequest{ + MediaMetadata: &types.MediaMetadata{}, + } + data := bytes.Buffer{} + responseBody := "This media is plain text. Maybe somebody used it as a paste bin." + data.WriteString(responseBody) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, err := multipartResponse(w, r, "text/plain", &data) + assert.NoError(t, err) + })) + defer srv.Close() + + resp, err := srv.Client().Get(srv.URL) + assert.NoError(t, err) + defer resp.Body.Close() + // contentLength is always 0, since there's no Content-Length header on the multipart part. + err, _, reader := parseMultipartResponse(r, resp, 1000) + assert.NoError(t, err) + gotResponse, err := io.ReadAll(reader) + assert.NoError(t, err) + assert.Equal(t, responseBody, string(gotResponse)) +} diff --git a/mediaapi/routing/routing.go b/mediaapi/routing/routing.go index 20417d1cc1..d567d9abdc 100644 --- a/mediaapi/routing/routing.go +++ b/mediaapi/routing/routing.go @@ -138,7 +138,7 @@ var thumbnailSize = promauto.NewHistogramVec( Namespace: "dendrite", Subsystem: "mediaapi", Name: "thumbnail_size_bytes", - Help: "Total number of media_api requests for thumbnails", + Help: "Total size of media_api requests for thumbnails", Buckets: []float64{50, 100, 200, 500, 900, 1500, 3000, 6000}, }, []string{"code", "type"}, @@ -149,7 +149,7 @@ var downloadCounter = promauto.NewCounterVec( Namespace: "dendrite", Subsystem: "mediaapi", Name: "download", - Help: "Total number of media_api requests for full downloads", + Help: "Total size of media_api requests for full downloads", }, []string{"code", "type"}, ) @@ -159,8 +159,8 @@ var downloadSize = promauto.NewHistogramVec( Namespace: "dendrite", Subsystem: "mediaapi", Name: "download_size_bytes", - Help: "Total number of media_api requests for full downloads", - Buckets: []float64{200, 500, 900, 1500, 3000, 6000, 10_000, 50_000, 100_000}, + Help: "Total size of media_api requests for full downloads", + Buckets: []float64{1500, 3000, 6000, 10_000, 50_000, 100_000}, }, []string{"code", "type"}, ) @@ -181,7 +181,10 @@ func makeDownloadAPI( var requestType string if cfg.Matrix.Metrics.Enabled { split := strings.Split(name, "_") + // The first part of the split is either "download" or "thumbnail" name = split[0] + // The remainder of the split is something like "authed_download" or "unauthed_thumbnail", etc. + // This is used to curry the metrics with the given types. requestType = strings.Join(split[1:], "_") counterVec = thumbnailCounter From 6f17931740500c707fb97334d7b356431c8d1ee4 Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Fri, 16 Aug 2024 12:14:23 +0200 Subject: [PATCH 16/16] Fix linter issues, rename functions --- clientapi/routing/routing.go | 2 +- federationapi/routing/routing.go | 4 ++-- internal/httputil/httpapi.go | 7 +++---- internal/sqlutil/sqlutil_test.go | 2 +- mediaapi/routing/routing.go | 8 ++++---- roomserver/acls/acls_test.go | 6 +++--- userapi/internal/key_api.go | 2 +- 7 files changed, 15 insertions(+), 16 deletions(-) diff --git a/clientapi/routing/routing.go b/clientapi/routing/routing.go index cce150343c..e82c8861ca 100644 --- a/clientapi/routing/routing.go +++ b/clientapi/routing/routing.go @@ -733,7 +733,7 @@ func Setup( ).Methods(http.MethodGet, http.MethodPost, http.MethodOptions) v3mux.Handle("/auth/{authType}/fallback/web", - httputil.MakeHTMLAPI("auth_fallback", userAPI, enableMetrics, func(w http.ResponseWriter, req *http.Request) { + httputil.MakeHTTPAPI("auth_fallback", userAPI, enableMetrics, func(w http.ResponseWriter, req *http.Request) { vars := mux.Vars(req) AuthFallback(w, req, vars["authType"], cfg) }), diff --git a/federationapi/routing/routing.go b/federationapi/routing/routing.go index 810fa9022f..91718efdb3 100644 --- a/federationapi/routing/routing.go +++ b/federationapi/routing/routing.go @@ -679,8 +679,8 @@ func MakeFedAPI( return httputil.MakeExternalAPI(metricsName, h) } -// MakeFedHTMLAPI makes an http.Handler that checks matrix federation authentication. -func MakeFedHTMLAPI( +// MakeFedHTTPAPI makes an http.Handler that checks matrix federation authentication. +func MakeFedHTTPAPI( serverName spec.ServerName, isLocalServerName func(spec.ServerName) bool, keyRing gomatrixserverlib.JSONVerifier, diff --git a/internal/httputil/httpapi.go b/internal/httputil/httpapi.go index 921c12a0e9..0559fbb727 100644 --- a/internal/httputil/httpapi.go +++ b/internal/httputil/httpapi.go @@ -59,7 +59,7 @@ func WithAllowGuests() AuthAPIOption { } } -// WithAuth is an option to MakeHTMLAPI to add authentication. +// WithAuth is an option to MakeHTTPAPI to add authentication. func WithAuth() AuthAPIOption { return func(opts *AuthAPIOpts) { opts.WithAuth = true @@ -206,9 +206,9 @@ func MakeExternalAPI(metricsName string, f func(*http.Request) util.JSONResponse return http.HandlerFunc(withSpan) } -// MakeHTMLAPI adds Span metrics to the HTML Handler function +// MakeHTTPAPI adds Span metrics to the HTML Handler function // This is used to serve HTML alongside JSON error messages -func MakeHTMLAPI(metricsName string, userAPI userapi.QueryAcccessTokenAPI, enableMetrics bool, f func(http.ResponseWriter, *http.Request), checks ...AuthAPIOption) http.Handler { +func MakeHTTPAPI(metricsName string, userAPI userapi.QueryAcccessTokenAPI, enableMetrics bool, f func(http.ResponseWriter, *http.Request), checks ...AuthAPIOption) http.Handler { withSpan := func(w http.ResponseWriter, req *http.Request) { trace, ctx := internal.StartTask(req.Context(), metricsName) defer trace.EndTask() @@ -224,7 +224,6 @@ func MakeHTMLAPI(metricsName string, userAPI userapi.QueryAcccessTokenAPI, enabl logger := util.GetLogger(req.Context()) _, jsonErr := auth.VerifyUserFromRequest(req, userAPI) if jsonErr != nil { - logger.Debugf("VerifyUserFromRequest %s -> HTTP %d", req.RemoteAddr, jsonErr.Code) w.WriteHeader(jsonErr.Code) if err := json.NewEncoder(w).Encode(jsonErr.JSON); err != nil { logger.WithError(err).Error("failed to encode JSON response") diff --git a/internal/sqlutil/sqlutil_test.go b/internal/sqlutil/sqlutil_test.go index c40757893e..93b84aa201 100644 --- a/internal/sqlutil/sqlutil_test.go +++ b/internal/sqlutil/sqlutil_test.go @@ -218,5 +218,5 @@ func assertNoError(t *testing.T, err error, msg string) { if err == nil { return } - t.Fatalf(msg) + t.Fatal(msg) } diff --git a/mediaapi/routing/routing.go b/mediaapi/routing/routing.go index d567d9abdc..2867df605c 100644 --- a/mediaapi/routing/routing.go +++ b/mediaapi/routing/routing.go @@ -105,20 +105,20 @@ func Setup( ).Methods(http.MethodGet, http.MethodOptions) // v1 client endpoints requiring auth - downloadHandlerAuthed := httputil.MakeHTMLAPI("download", userAPI, cfg.Global.Metrics.Enabled, makeDownloadAPI("download_authed_client", &cfg.MediaAPI, rateLimits, db, client, federationClient, activeRemoteRequests, activeThumbnailGeneration, false), httputil.WithAuth()) + downloadHandlerAuthed := httputil.MakeHTTPAPI("download", userAPI, cfg.Global.Metrics.Enabled, makeDownloadAPI("download_authed_client", &cfg.MediaAPI, rateLimits, db, client, federationClient, activeRemoteRequests, activeThumbnailGeneration, false), httputil.WithAuth()) v1mux.Handle("/config", configHandler).Methods(http.MethodGet, http.MethodOptions) v1mux.Handle("/download/{serverName}/{mediaId}", downloadHandlerAuthed).Methods(http.MethodGet, http.MethodOptions) v1mux.Handle("/download/{serverName}/{mediaId}/{downloadName}", downloadHandlerAuthed).Methods(http.MethodGet, http.MethodOptions) v1mux.Handle("/thumbnail/{serverName}/{mediaId}", - httputil.MakeHTMLAPI("thumbnail", userAPI, cfg.Global.Metrics.Enabled, makeDownloadAPI("thumbnail_authed_client", &cfg.MediaAPI, rateLimits, db, client, federationClient, activeRemoteRequests, activeThumbnailGeneration, false), httputil.WithAuth()), + httputil.MakeHTTPAPI("thumbnail", userAPI, cfg.Global.Metrics.Enabled, makeDownloadAPI("thumbnail_authed_client", &cfg.MediaAPI, rateLimits, db, client, federationClient, activeRemoteRequests, activeThumbnailGeneration, false), httputil.WithAuth()), ).Methods(http.MethodGet, http.MethodOptions) // same, but for federation - v1fedMux.Handle("/download/{mediaId}", routing.MakeFedHTMLAPI(cfg.Global.ServerName, cfg.Global.IsLocalServerName, keyRing, + v1fedMux.Handle("/download/{mediaId}", routing.MakeFedHTTPAPI(cfg.Global.ServerName, cfg.Global.IsLocalServerName, keyRing, makeDownloadAPI("download_authed_federation", &cfg.MediaAPI, rateLimits, db, client, federationClient, activeRemoteRequests, activeThumbnailGeneration, true), )).Methods(http.MethodGet, http.MethodOptions) - v1fedMux.Handle("/thumbnail/{mediaId}", routing.MakeFedHTMLAPI(cfg.Global.ServerName, cfg.Global.IsLocalServerName, keyRing, + v1fedMux.Handle("/thumbnail/{mediaId}", routing.MakeFedHTTPAPI(cfg.Global.ServerName, cfg.Global.IsLocalServerName, keyRing, makeDownloadAPI("thumbnail_authed_federation", &cfg.MediaAPI, rateLimits, db, client, federationClient, activeRemoteRequests, activeThumbnailGeneration, true), )).Methods(http.MethodGet, http.MethodOptions) } diff --git a/roomserver/acls/acls_test.go b/roomserver/acls/acls_test.go index 09920308c5..7fd20f114c 100644 --- a/roomserver/acls/acls_test.go +++ b/roomserver/acls/acls_test.go @@ -29,11 +29,11 @@ func TestOpenACLsWithBlacklist(t *testing.T) { roomID := "!test:test.com" allowRegex, err := compileACLRegex("*") if err != nil { - t.Fatalf(err.Error()) + t.Fatal(err) } denyRegex, err := compileACLRegex("foo.com") if err != nil { - t.Fatalf(err.Error()) + t.Fatal(err) } acls := ServerACLs{ @@ -72,7 +72,7 @@ func TestDefaultACLsWithWhitelist(t *testing.T) { roomID := "!test:test.com" allowRegex, err := compileACLRegex("foo.com") if err != nil { - t.Fatalf(err.Error()) + t.Fatal(err) } acls := ServerACLs{ diff --git a/userapi/internal/key_api.go b/userapi/internal/key_api.go index 422898c70a..81127481ec 100644 --- a/userapi/internal/key_api.go +++ b/userapi/internal/key_api.go @@ -196,7 +196,7 @@ func (a *UserInternalAPI) QueryDeviceMessages(ctx context.Context, req *api.Quer if m.StreamID > maxStreamID { maxStreamID = m.StreamID } - if m.KeyJSON == nil || len(m.KeyJSON) == 0 { + if len(m.KeyJSON) == 0 { continue } result = append(result, m)