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] 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 5a735cf5e..810fa9022 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 3ceb12a59..16ba0c51b 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 63d5cbcf4..20417d1cc 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) }