diff --git a/e2e/trust/trust_center_logo_test.go b/e2e/trust/trust_center_logo_test.go index e9e8685c0..48b701817 100644 --- a/e2e/trust/trust_center_logo_test.go +++ b/e2e/trust/trust_center_logo_test.go @@ -15,6 +15,8 @@ package trust_test import ( + "io" + "net/http" "strings" "testing" @@ -155,10 +157,64 @@ func TestTrustCenter_LogoFileDownloadURL(t *testing.T) { require.NoError(t, err) require.NotNil(t, trustResult.CurrentTrustCenter.Logo) assert.Equal(t, uploadResult.UpdateTrustCenterBrand.TrustCenter.Logo.ID, trustResult.CurrentTrustCenter.Logo.ID) + + downloadURL := trustResult.CurrentTrustCenter.Logo.DownloadURL assert.True( t, - strings.Contains(trustResult.CurrentTrustCenter.Logo.DownloadURL, "/api/files/v1/public/"), + strings.Contains(downloadURL, "/api/files/v1/public/"), "downloadUrl must route through the public files API, got %q", - trustResult.CurrentTrustCenter.Logo.DownloadURL, + downloadURL, ) + + // The public endpoint streams the file bytes directly (no presigned + // redirect) with cache headers, so the stable URL is CDN/browser cacheable. + resp, err := http.Get(downloadURL) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + + require.Equal(t, http.StatusOK, resp.StatusCode) + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + assert.Equal(t, pngContent, body, "public endpoint must stream the original file bytes") + + cacheControl := resp.Header.Get("Cache-Control") + assert.Contains(t, cacheControl, "public") + assert.Contains(t, cacheControl, "max-age=31536000") + assert.Contains(t, cacheControl, "immutable") + + etag := resp.Header.Get("ETag") + require.NotEmpty(t, etag, "public file response must carry an ETag for revalidation") + + // Untrusted, unauthenticated content served from the app origin must be + // hardened against MIME sniffing and script execution (e.g. SVG uploads). + assert.Equal(t, "nosniff", resp.Header.Get("X-Content-Type-Options")) + assert.Contains(t, resp.Header.Get("Content-Security-Policy"), "sandbox") + + // A matching If-None-Match must revalidate to 304 without transferring + // the body again. + revalidateReq, err := http.NewRequest(http.MethodGet, downloadURL, nil) + require.NoError(t, err) + revalidateReq.Header.Set("If-None-Match", etag) + + revalidateResp, err := http.DefaultClient.Do(revalidateReq) + require.NoError(t, err) + defer func() { _ = revalidateResp.Body.Close() }() + + assert.Equal(t, http.StatusNotModified, revalidateResp.StatusCode) + + // A matching If-Modified-Since (using the returned Last-Modified) must also + // revalidate to 304. + lastModified := resp.Header.Get("Last-Modified") + require.NotEmpty(t, lastModified, "public file response must carry Last-Modified") + + sinceReq, err := http.NewRequest(http.MethodGet, downloadURL, nil) + require.NoError(t, err) + sinceReq.Header.Set("If-Modified-Since", lastModified) + + sinceResp, err := http.DefaultClient.Do(sinceReq) + require.NoError(t, err) + defer func() { _ = sinceResp.Body.Close() }() + + assert.Equal(t, http.StatusNotModified, sinceResp.StatusCode) } diff --git a/pkg/filemanager/s3.go b/pkg/filemanager/s3.go index 1a166177c..aad80495e 100644 --- a/pkg/filemanager/s3.go +++ b/pkg/filemanager/s3.go @@ -17,16 +17,39 @@ package filemanager import ( "context" "encoding/base64" + "errors" "fmt" "io" + "net/http" "net/url" "strings" "time" "github.com/aws/aws-sdk-go-v2/service/s3" + smithyhttp "github.com/aws/smithy-go/transport/http" "go.probo.inc/probo/pkg/coredata" ) +// FileObject carries a streamed S3 object body plus the metadata needed to set +// HTTP response headers. NotModified is set when the caller's conditional +// request matched the current object, in which case Body is nil. +type FileObject struct { + Body io.ReadCloser + ContentType string + ContentLength int64 + ETag string + LastModified time.Time + NotModified bool +} + +// FileConditions carries HTTP conditional-request values forwarded to S3 so it +// can answer with 304 Not Modified without transferring the body. Zero values +// are omitted. +type FileConditions struct { + IfNoneMatch string + IfModifiedSince time.Time +} + func (s *Service) GetFileBase64( ctx context.Context, file *coredata.File, @@ -77,6 +100,53 @@ func (s *Service) GetFileBytes( return data, nil } +// OpenFile streams an object from S3 without buffering it in memory. Conditional +// request values in conds are forwarded to S3; a 304 Not Modified response is +// surfaced as a FileObject with NotModified set (and a nil Body). The caller +// owns closing Body. +func (s *Service) OpenFile( + ctx context.Context, + file *coredata.File, + conds FileConditions, +) (*FileObject, error) { + input := &s3.GetObjectInput{ + Bucket: new(file.BucketName), + Key: new(file.FileKey), + } + if conds.IfNoneMatch != "" { + input.IfNoneMatch = &conds.IfNoneMatch + } + if !conds.IfModifiedSince.IsZero() { + input.IfModifiedSince = &conds.IfModifiedSince + } + + result, err := s.s3Client.GetObject(ctx, input) + if err != nil { + if respErr, ok := errors.AsType[*smithyhttp.ResponseError](err); ok { + if respErr.HTTPStatusCode() == http.StatusNotModified { + return &FileObject{NotModified: true}, nil + } + } + + return nil, fmt.Errorf("cannot get file from S3: %w", err) + } + + obj := &FileObject{ + Body: result.Body, + ContentType: file.MimeType, + ContentLength: file.FileSize, + LastModified: file.UpdatedAt, + } + if result.ETag != nil { + obj.ETag = *result.ETag + } + if result.LastModified != nil { + obj.LastModified = *result.LastModified + } + + return obj, nil +} + func (s *Service) PutFile( ctx context.Context, file *coredata.File, diff --git a/pkg/filemanager/s3_test.go b/pkg/filemanager/s3_test.go new file mode 100644 index 000000000..71005f089 --- /dev/null +++ b/pkg/filemanager/s3_test.go @@ -0,0 +1,155 @@ +// Copyright (c) 2026 Probo Inc . +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR +// OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +package filemanager_test + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/credentials" + awss3 "github.com/aws/aws-sdk-go-v2/service/s3" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.probo.inc/probo/pkg/coredata" + "go.probo.inc/probo/pkg/filemanager" +) + +func newTestS3Service(t *testing.T, handler http.HandlerFunc) *filemanager.Service { + t.Helper() + + srv := httptest.NewServer(handler) + t.Cleanup(srv.Close) + + s3Client := awss3.NewFromConfig( + aws.Config{ + Region: "us-east-1", + Credentials: credentials.NewStaticCredentialsProvider("access-key", "secret-key", ""), + }, + func(o *awss3.Options) { + o.BaseEndpoint = aws.String(srv.URL) + o.UsePathStyle = true + }, + ) + + return filemanager.NewService(nil, nil, s3Client) +} + +func TestOpenFile_StreamsBody(t *testing.T) { + t.Parallel() + + const ( + etag = `"abc123"` + content = "hello world" + ) + + svc := newTestS3Service( + t, + func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("ETag", etag) + w.Header().Set("Content-Type", "application/octet-stream") + _, _ = io.WriteString(w, content) + }, + ) + + file := &coredata.File{ + BucketName: "uploads", + FileKey: "tenant/file", + MimeType: "text/plain", + FileSize: int64(len(content)), + } + + obj, err := svc.OpenFile(context.Background(), file, filemanager.FileConditions{}) + require.NoError(t, err) + require.NotNil(t, obj) + require.False(t, obj.NotModified) + + defer func() { _ = obj.Body.Close() }() + + assert.Equal(t, etag, obj.ETag) + assert.Equal(t, "text/plain", obj.ContentType) + assert.Equal(t, int64(len(content)), obj.ContentLength) + + body, err := io.ReadAll(obj.Body) + require.NoError(t, err) + assert.Equal(t, content, string(body)) +} + +func TestOpenFile_NotModifiedByETag(t *testing.T) { + t.Parallel() + + const etag = `"abc123"` + + svc := newTestS3Service( + t, + func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("If-None-Match") == etag { + w.WriteHeader(http.StatusNotModified) + return + } + + w.Header().Set("ETag", etag) + _, _ = io.WriteString(w, "content") + }, + ) + + file := &coredata.File{ + BucketName: "uploads", + FileKey: "tenant/file", + MimeType: "text/plain", + } + + obj, err := svc.OpenFile(context.Background(), file, filemanager.FileConditions{IfNoneMatch: etag}) + require.NoError(t, err) + require.NotNil(t, obj) + assert.True(t, obj.NotModified) + assert.Nil(t, obj.Body) +} + +func TestOpenFile_NotModifiedByModifiedSince(t *testing.T) { + t.Parallel() + + svc := newTestS3Service( + t, + func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("If-Modified-Since") != "" { + w.WriteHeader(http.StatusNotModified) + return + } + + _, _ = io.WriteString(w, "content") + }, + ) + + file := &coredata.File{ + BucketName: "uploads", + FileKey: "tenant/file", + MimeType: "text/plain", + } + + obj, err := svc.OpenFile( + context.Background(), + file, + filemanager.FileConditions{IfModifiedSince: time.Now()}, + ) + require.NoError(t, err) + require.NotNil(t, obj) + assert.True(t, obj.NotModified) + assert.Nil(t, obj.Body) +} diff --git a/pkg/server/api/files/v1/handler.go b/pkg/server/api/files/v1/handler.go index 8ae99c284..cb86d31a0 100644 --- a/pkg/server/api/files/v1/handler.go +++ b/pkg/server/api/files/v1/handler.go @@ -17,7 +17,9 @@ package files_v1 import ( "errors" "fmt" + "io" "net/http" + "strconv" "time" "github.com/go-chi/chi/v5" @@ -121,11 +123,18 @@ func (h *Handler) handleGetPublicFile(w http.ResponseWriter, r *http.Request) { return } - presignedURL, err := h.fileSvc.GeneratePresignedURL(r.Context(), file, presignedURLExpiry) + conds := filemanager.FileConditions{IfNoneMatch: r.Header.Get("If-None-Match")} + if ifModifiedSince := r.Header.Get("If-Modified-Since"); ifModifiedSince != "" { + if t, parseErr := http.ParseTime(ifModifiedSince); parseErr == nil { + conds.IfModifiedSince = t + } + } + + obj, err := h.fileSvc.OpenFile(r.Context(), file, conds) if err != nil { h.logger.ErrorCtx( r.Context(), - "cannot get public file URL", + "cannot open public file", log.Error(err), log.String("file_id", fileIDStr), ) @@ -134,7 +143,38 @@ func (h *Handler) handleGetPublicFile(w http.ResponseWriter, r *http.Request) { return } - http.Redirect(w, r, presignedURL, http.StatusTemporaryRedirect) + w.Header().Set("Cache-Control", "public, max-age=31536000, immutable") + + if obj.ETag != "" { + w.Header().Set("ETag", obj.ETag) + } + + if obj.NotModified { + w.WriteHeader(http.StatusNotModified) + return + } + + defer func() { _ = obj.Body.Close() }() + + w.Header().Set("Content-Type", file.MimeType) + w.Header().Set("Content-Length", strconv.FormatInt(file.FileSize, 10)) + w.Header().Set("X-Content-Type-Options", "nosniff") + w.Header().Set("Content-Security-Policy", "default-src 'none'; style-src 'unsafe-inline'; sandbox") + + if !obj.LastModified.IsZero() { + w.Header().Set("Last-Modified", obj.LastModified.UTC().Format(http.TimeFormat)) + } + + if _, err := io.Copy(w, obj.Body); err != nil { + h.logger.ErrorCtx( + r.Context(), + "cannot stream public file", + log.Error(err), + log.String("file_id", fileIDStr), + ) + + return + } } func (h *Handler) handleGetFile(w http.ResponseWriter, r *http.Request) {