From 749c309ac9d5161c751bf5cb5b20983ec102a239 Mon Sep 17 00:00:00 2001 From: Timon Heuser Date: Wed, 8 Jul 2026 11:06:01 +0200 Subject: [PATCH] BUGFIX: receive fails with absolute S3 urls #48 --- pkg/receive/receive-session.go | 6 +- pkg/receive/receive-session_test.go | 123 ++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 2 deletions(-) create mode 100644 pkg/receive/receive-session_test.go diff --git a/pkg/receive/receive-session.go b/pkg/receive/receive-session.go index 600c1c1..8d3ea1d 100644 --- a/pkg/receive/receive-session.go +++ b/pkg/receive/receive-session.go @@ -223,8 +223,10 @@ func (rs *ReceiveSession) FetchAndDecryptFileWithProgressBar(fileName string) (* func (rs *ReceiveSession) FetchFileWithProgressBar(fileName string, fileDefinition dto.PublicFilesIndexEntry, progress *pterm.ProgressbarPrinter) (*bytes.Buffer, error) { var urlToLoad string var err error - if strings.HasPrefix(fileName, "http://") || strings.HasPrefix(fileName, "https://") { - urlToLoad = fileName + if strings.HasPrefix(fileDefinition.PublicUri, "http://") || strings.HasPrefix(fileDefinition.PublicUri, "https://") { + // the public URI is already a full URL (e.g. an S3/CDN target with an absolute baseUri) + // -> use it directly, without prepending the base URL. + urlToLoad = fileDefinition.PublicUri } else if fileDefinition.IsAbsoluteUrl { urlToLoad = strings.ReplaceAll(*rs.baseUrl, "/_Resources", "") + fileDefinition.PublicUri } else { diff --git a/pkg/receive/receive-session_test.go b/pkg/receive/receive-session_test.go new file mode 100644 index 0000000..a890599 --- /dev/null +++ b/pkg/receive/receive-session_test.go @@ -0,0 +1,123 @@ +package receive + +import ( + "io" + "net/http" + "strings" + "testing" + + "github.com/pterm/pterm" + "github.com/sandstorm/synco/v2/pkg/common/dto" +) + +// recordingTransport answers every request with the given body and records +// the URLs it was asked for, so tests can assert how FetchFileWithProgressBar +// builds the download URL without opening a real network listener. +type recordingTransport struct { + body string + urls []string +} + +func (rt *recordingTransport) RoundTrip(req *http.Request) (*http.Response, error) { + rt.urls = append(rt.urls, req.URL.String()) + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(rt.body)), + ContentLength: int64(len(rt.body)), + Request: req, + }, nil +} + +func newTestReceiveSession(baseUrl string, transport *recordingTransport) *ReceiveSession { + return &ReceiveSession{ + baseUrl: &baseUrl, + identifier: "synco-test", + httpClient: &http.Client{Transport: transport}, + } +} + +// silentProgressbar returns a progressbar that FetchFileWithProgressBar can +// write to without printing anything: with Total == 0, pterm's Add is a no-op. +func silentProgressbar() *pterm.ProgressbarPrinter { + return &pterm.ProgressbarPrinter{} +} + +// fetchAndAssertURL runs FetchFileWithProgressBar for the given entry and +// asserts that exactly one request was made, to wantUrl. +func fetchAndAssertURL(t *testing.T, baseUrl string, fileName string, entry dto.PublicFilesIndexEntry, wantUrl string) { + t.Helper() + transport := &recordingTransport{body: "file content"} + rs := newTestReceiveSession(baseUrl, transport) + + buf, err := rs.FetchFileWithProgressBar(fileName, entry, silentProgressbar()) + if err != nil { + t.Fatalf("FetchFileWithProgressBar: %v", err) + } + if got := buf.String(); got != "file content" { + t.Errorf("unexpected content: got %q, want %q", got, "file content") + } + if len(transport.urls) != 1 || transport.urls[0] != wantUrl { + t.Errorf("unexpected requests: got %v, want [%s]", transport.urls, wantUrl) + } +} + +// Regression test: an entry whose PublicUri is already a full URL (e.g. an +// S3/CDN target with an absolute baseUri) must be downloaded from that URL +// directly, without prepending the base URL. The old code checked fileName +// (the local file name / index key) instead of PublicUri for the http(s) +// prefix, so such entries fell into the base-URL branches and produced +// broken URLs. +func TestFetchFileWithProgressBar_AbsolutePublicUri(t *testing.T) { + fetchAndAssertURL(t, + "https://origin.example.com/_Resources", + "persistent/logo.png", + dto.PublicFilesIndexEntry{ + PublicUri: "https://cdn.example.com/bucket/persistent/logo.png", + IsAbsoluteUrl: false, + }, + "https://cdn.example.com/bucket/persistent/logo.png", + ) +} + +// Same regression, for entries flagged IsAbsoluteUrl: a fully-qualified +// PublicUri must win over the IsAbsoluteUrl handling (which would concatenate +// the base URL and the full URL into garbage). +func TestFetchFileWithProgressBar_AbsolutePublicUriWithIsAbsoluteUrlFlag(t *testing.T) { + fetchAndAssertURL(t, + "https://origin.example.com/_Resources", + "persistent/logo.png", + dto.PublicFilesIndexEntry{ + PublicUri: "http://cdn.example.com/bucket/persistent/logo.png", + IsAbsoluteUrl: true, + }, + "http://cdn.example.com/bucket/persistent/logo.png", + ) +} + +// IsAbsoluteUrl entries with a host-relative PublicUri are resolved against +// the base URL with the "/_Resources" suffix stripped. +func TestFetchFileWithProgressBar_IsAbsoluteUrlRelativeUri(t *testing.T) { + fetchAndAssertURL(t, + "https://origin.example.com/_Resources", + "media/site/logo.png", + dto.PublicFilesIndexEntry{ + PublicUri: "/media/site/logo.png", + IsAbsoluteUrl: true, + }, + "https://origin.example.com/media/site/logo.png", + ) +} + +// Relative entries are joined onto the base URL, with the placeholder +// removed. +func TestFetchFileWithProgressBar_RelativeUri(t *testing.T) { + fetchAndAssertURL(t, + "https://origin.example.com/downloads", + "css/main.css", + dto.PublicFilesIndexEntry{ + PublicUri: "/css/main.css", + IsAbsoluteUrl: false, + }, + "https://origin.example.com/downloads/css/main.css", + ) +}