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 1 commit
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
Prev Previous commit
Next Next commit
implement redirect handler
  • Loading branch information
dnnspaul committed Jan 16, 2025
commit 03dd120886b58f8ea13ba7938d5ec1b6a9f8ad39
94 changes: 44 additions & 50 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 @@ -963,6 +963,14 @@ func handleMultipartRedirect(r *downloadRequest, redirectURL string, maxFileSize
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
Expand All @@ -971,62 +979,50 @@ func handleMultipartRedirect(r *downloadRequest, redirectURL string, maxFileSize
return 0, nil, fmt.Errorf("invalid redirect URL: %w", err)
}

// Security check: Only allow HTTPS URLs unless it's a trusted server
if parsedURL.Scheme != "https" && !isAllowedInsecureRedirect(parsedURL.Host, r.origin) {
return 0, nil, fmt.Errorf("insecure redirect URL: HTTPS required")
}

// 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)
}

var resp *http.Response
if r.fedClient != nil {
// Extract media ID from the redirect URL
parsedURL, err := url.Parse(currentURL)
if err != nil {
return 0, nil, fmt.Errorf("invalid redirect URL: %w", err)
}

// Extract the media ID from the path
pathParts := strings.Split(parsedURL.Path, "/")
if len(pathParts) == 0 {
return 0, nil, fmt.Errorf("invalid media URL path")
}
mediaID := pathParts[len(pathParts)-1]

// Use the federation client's DownloadMedia method
resp, err = r.fedClient.DownloadMedia(req.Context(), r.origin, spec.ServerName(parsedURL.Host), mediaID)
if err != nil {
return 0, nil, fmt.Errorf("federation client failed to download media: %w", err)
}
} else {
// For non-federation requests, use a regular client
client := &http.Client{
CheckRedirect: func(req *http.Request, via []*http.Request) error {
return http.ErrUseLastResponse // Prevent auto-redirect
},
}
// Close the previous response body before making a new request
if lastResponse != nil {
lastResponse.Body.Close()
lastResponse = nil
}

resp, err = client.Do(req)
if err != nil {
return 0, nil, fmt.Errorf("failed to perform request: %w", err)
}
// 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,
}
defer resp.Body.Close()

resp, err := client.Do(req)
if err != nil {
return 0, nil, fmt.Errorf("failed to follow redirect: %w", err)
}
defer resp.Body.Close()
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
Expand All @@ -1037,12 +1033,18 @@ func handleMultipartRedirect(r *downloadRequest, redirectURL string, maxFileSize
// Check if the response is multipart
contentType := resp.Header.Get("Content-Type")
if strings.HasPrefix(contentType, "multipart/") {
// Handle nested multipart response
// 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)
}

// Handle regular response
return r.GetContentLengthAndReader(resp.Header.Get("Content-Length"), resp.Body, 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)
Expand All @@ -1051,14 +1053,6 @@ func handleMultipartRedirect(r *downloadRequest, redirectURL string, maxFileSize
return 0, nil, fmt.Errorf("too many redirects (max %d)", maxRedirects)
}

// isAllowedInsecureRedirect checks if insecure redirects are allowed for the given host
func isAllowedInsecureRedirect(host string, origin spec.ServerName) bool {
// Implementation depends on your security requirements
// You might want to check against a whitelist of trusted servers
// or compare against the origin server
return string(origin) == host
}

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