Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement media file redirect #3497

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 98 additions & 2 deletions mediaapi/routing/download.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ package routing
import (
"context"
"encoding/json"
"time"
"fmt"
"io"
"io/fs"
Expand Down Expand Up @@ -938,7 +939,6 @@ func parseMultipartResponse(r *downloadRequest, resp *http.Response, maxFileSize
if err = json.NewDecoder(p).Decode(&meta); err != nil {
return 0, nil, err
}
defer p.Close() // nolint: errcheck

// Get the actual media content
p, err = mr.NextPart()
Expand All @@ -948,7 +948,8 @@ func parseMultipartResponse(r *downloadRequest, resp *http.Response, maxFileSize

redirect := p.Header.Get("Location")
if redirect != "" {
return 0, nil, fmt.Errorf("Location header is not yet supported")
// Handle redirect
return handleMultipartRedirect(r, redirect, maxFileSizeBytes)
}

contentLength, reader, err := r.GetContentLengthAndReader(p.Header.Get("Content-Length"), p, maxFileSizeBytes)
Expand All @@ -957,6 +958,101 @@ func parseMultipartResponse(r *downloadRequest, resp *http.Response, maxFileSize
return contentLength, reader, err
}

// handleMultipartRedirect processes a redirect URL from a multipart response
func handleMultipartRedirect(r *downloadRequest, redirectURL string, maxFileSizeBytes config.FileSizeBytes) (int64, io.Reader, error) {
const maxRedirects = 10
redirectCount := 0
currentURL := redirectURL
var lastResponse *http.Response

// Ensure we clean up any response body if we exit early
defer func() {
if lastResponse != nil && lastResponse.Body != nil {
lastResponse.Body.Close()
}
}()

for redirectCount < maxRedirects {
// Validate the redirect URL
parsedURL, err := url.Parse(currentURL)
if err != nil {
return 0, nil, fmt.Errorf("invalid redirect URL: %w", err)
}

// Create a new request for the redirect
req, err := http.NewRequest("GET", currentURL, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should definitely not use the default http client here. We just merged an update (e9cc37a) to fix a SSRF vuln.

So yea, prefer using the federation client here.

Copy link
Author

@dnnspaul dnnspaul Jan 16, 2025

Choose a reason for hiding this comment

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

Can totally understand that. I never worked with the federation client before, but do I understand correctly that it's mostly used for getting in contact with other matrix servers?

Because I had a look into the federation client interface and didn't find a function for a simple GET request. The best candidates I found were https://github.com/matrix-org/gomatrixserverlib/blob/main/fclient/federationclient.go#L67 and https://github.com/matrix-org/gomatrixserverlib/blob/main/fclient/federationclient.go#L22 but both implementations had a URL manipulator logic that would make it impossible with current state to access a non-matrix server with it, like I mentioned in the initial message for example an s3 URL like in beeper.com's case.

EDIT: I found the DoHTTPRequest() function in the *fclient.Client, but it always makes GET requests to the 8448 port and I can't find a possibility to overwrite it. I'm passing it an URL without any port suggestion.

time="2025-01-16T21:58:46.889093655Z" level=error msg="r.fetchRemoteFileAndStoreMetadata: failed to fetch remote file" MediaID=xxx_xxx Origin=local.beeper.com error="failed to send http request to redirect: Get \"https://s3.eu-central-1.wasabisys.com/hungryserv-media-eu-central-1.beeper.com/xxx/media/xxx?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=xxx%2F20250116%2Feu-central-1%2Fs3%2Faws4_request&X-Amz-Date=20250116T215836Z&X-Amz-Expires=3600&X-Amz-SignedHeaders=host&response-content-disposition=attachment&X-Amz-Signature=xxx\": dial tcp 130.117.252.101:8448: i/o timeout" req.id=c6fSn19V3pwC req.method=OPTIONS req.path=/_matrix/media/v3/download/local.beeper.com/xxx_xxx

if err != nil {
return 0, nil, fmt.Errorf("failed to create redirect request: %w", err)
}

// Close the previous response body before making a new request
if lastResponse != nil {
lastResponse.Body.Close()
lastResponse = nil
}

// Use a regular client for redirects, as they might point to external storage
client := &http.Client{
CheckRedirect: func(req *http.Request, via []*http.Request) error {
return http.ErrUseLastResponse // Prevent auto-redirect
},
Timeout: 30 * time.Second,
}

resp, err := client.Do(req)
if err != nil {
return 0, nil, fmt.Errorf("failed to follow redirect: %w", err)
}
lastResponse = resp

// Check if we get another redirect
if resp.StatusCode == http.StatusFound || resp.StatusCode == http.StatusMovedPermanently {
nextURL := resp.Header.Get("Location")
if nextURL == "" {
return 0, nil, fmt.Errorf("redirect response without Location header")
}

// Handle relative URLs
nextParsedURL, err := url.Parse(nextURL)
if err != nil {
return 0, nil, fmt.Errorf("invalid redirect URL: %w", err)
}

if !nextParsedURL.IsAbs() {
nextParsedURL = parsedURL.ResolveReference(nextParsedURL)
nextURL = nextParsedURL.String()
}

currentURL = nextURL
redirectCount++
continue
}

// If we got a successful response, process it
if resp.StatusCode == http.StatusOK {
// Check if the response is multipart
contentType := resp.Header.Get("Content-Type")
if strings.HasPrefix(contentType, "multipart/") {
// For multipart responses, we need to keep the response body open
// The caller will be responsible for closing it
lastResponse = nil // Don't close in defer
return parseMultipartResponse(r, resp, maxFileSizeBytes)
}

// For regular responses, create a new reader that will close the response body
body := resp.Body
lastResponse = nil // Don't close in defer
reader := io.NopCloser(body)

return r.GetContentLengthAndReader(resp.Header.Get("Content-Length"), reader, maxFileSizeBytes)
}

return 0, nil, fmt.Errorf("unexpected status code following redirect: %d", resp.StatusCode)
}

return 0, nil, fmt.Errorf("too many redirects (max %d)", maxRedirects)
}

// contentDispositionFor returns the Content-Disposition for a given
// content type.
func contentDispositionFor(contentType types.ContentType) string {
Expand Down