From 76906d5acfa8558abe29df26edcf4e6b6dfd26a5 Mon Sep 17 00:00:00 2001 From: ryan-mchugh Date: Mon, 7 Oct 2024 19:36:08 +0000 Subject: [PATCH 1/5] B-20766 - string instead of string pointer on filename. --- pkg/handlers/ghcapi/documents.go | 2 +- .../internal/payloads/model_to_payload.go | 6 +++--- pkg/handlers/ghcapi/uploads.go | 2 +- .../internal/payloads/model_to_payload.go | 2 +- .../primeapi/payloads/model_to_payload.go | 2 +- pkg/storage/filesystem.go | 3 ++- pkg/storage/filesystem_test.go | 4 ++-- pkg/storage/memory.go | 3 ++- pkg/storage/memory_test.go | 4 ++-- pkg/storage/mocks/FileStorer.go | 18 +++++++++--------- pkg/storage/s3.go | 4 ++-- pkg/storage/storage.go | 2 +- pkg/storage/test/s3.go | 4 ++-- pkg/uploader/uploader.go | 2 +- 14 files changed, 30 insertions(+), 28 deletions(-) diff --git a/pkg/handlers/ghcapi/documents.go b/pkg/handlers/ghcapi/documents.go index 4b385a7c1a5..b150eb2a5d3 100644 --- a/pkg/handlers/ghcapi/documents.go +++ b/pkg/handlers/ghcapi/documents.go @@ -22,7 +22,7 @@ func payloadForDocumentModel(storer storage.FileStorer, document models.Document if userUpload.Upload.ID == uuid.Nil { return nil, errors.New("No uploads for user") } - url, err := storer.PresignedURL(userUpload.Upload.StorageKey, userUpload.Upload.ContentType) + url, err := storer.PresignedURL(userUpload.Upload.StorageKey, userUpload.Upload.ContentType, userUpload.Upload.Filename) if err != nil { return nil, err } diff --git a/pkg/handlers/ghcapi/internal/payloads/model_to_payload.go b/pkg/handlers/ghcapi/internal/payloads/model_to_payload.go index 933d9431941..f832dd00fb9 100644 --- a/pkg/handlers/ghcapi/internal/payloads/model_to_payload.go +++ b/pkg/handlers/ghcapi/internal/payloads/model_to_payload.go @@ -1697,7 +1697,7 @@ func ServiceRequestDoc(serviceRequest models.ServiceRequestDocument, storer stor if serviceRequest.ServiceRequestDocumentUploads != nil && len(serviceRequest.ServiceRequestDocumentUploads) > 0 { for i, serviceRequestUpload := range serviceRequest.ServiceRequestDocumentUploads { - url, err := storer.PresignedURL(serviceRequestUpload.Upload.StorageKey, serviceRequestUpload.Upload.ContentType) + url, err := storer.PresignedURL(serviceRequestUpload.Upload.StorageKey, serviceRequestUpload.Upload.ContentType, serviceRequestUpload.Upload.Filename) if err != nil { return nil, err } @@ -1934,7 +1934,7 @@ func ProofOfServiceDoc(proofOfService models.ProofOfServiceDoc, storer storage.F uploads := make([]*ghcmessages.Upload, len(proofOfService.PrimeUploads)) if proofOfService.PrimeUploads != nil && len(proofOfService.PrimeUploads) > 0 { for i, primeUpload := range proofOfService.PrimeUploads { - url, err := storer.PresignedURL(primeUpload.Upload.StorageKey, primeUpload.Upload.ContentType) + url, err := storer.PresignedURL(primeUpload.Upload.StorageKey, primeUpload.Upload.ContentType, primeUpload.Upload.Filename) if err != nil { return nil, err } @@ -1990,7 +1990,7 @@ func PayloadForDocumentModel(storer storage.FileStorer, document models.Document if userUpload.Upload.ID == uuid.Nil { return nil, errors.New("no uploads for user") } - url, err := storer.PresignedURL(userUpload.Upload.StorageKey, userUpload.Upload.ContentType) + url, err := storer.PresignedURL(userUpload.Upload.StorageKey, userUpload.Upload.ContentType, userUpload.Upload.Filename) if err != nil { return nil, err } diff --git a/pkg/handlers/ghcapi/uploads.go b/pkg/handlers/ghcapi/uploads.go index 3b9b72724c9..a74e5d48498 100644 --- a/pkg/handlers/ghcapi/uploads.go +++ b/pkg/handlers/ghcapi/uploads.go @@ -109,7 +109,7 @@ func (h UpdateUploadHandler) Handle(params uploadop.UpdateUploadParams) middlewa return nil, apperror.NewBadDataError("unable to update upload") } - url, err := h.FileStorer().PresignedURL(newUpload.StorageKey, newUpload.ContentType) + url, err := h.FileStorer().PresignedURL(newUpload.StorageKey, newUpload.ContentType, newUpload.Filename) if err != nil { return nil, err } diff --git a/pkg/handlers/internalapi/internal/payloads/model_to_payload.go b/pkg/handlers/internalapi/internal/payloads/model_to_payload.go index 96908a15230..d70829704bd 100644 --- a/pkg/handlers/internalapi/internal/payloads/model_to_payload.go +++ b/pkg/handlers/internalapi/internal/payloads/model_to_payload.go @@ -345,7 +345,7 @@ func PayloadForDocumentModel(storer storage.FileStorer, document models.Document if userUpload.Upload.ID == uuid.Nil { return nil, errors.New("no uploads for user") } - url, err := storer.PresignedURL(userUpload.Upload.StorageKey, userUpload.Upload.ContentType) + url, err := storer.PresignedURL(userUpload.Upload.StorageKey, userUpload.Upload.ContentType, userUpload.Upload.Filename) if err != nil { return nil, err } diff --git a/pkg/handlers/primeapi/payloads/model_to_payload.go b/pkg/handlers/primeapi/payloads/model_to_payload.go index 85ca0157ad5..b869e673ad8 100644 --- a/pkg/handlers/primeapi/payloads/model_to_payload.go +++ b/pkg/handlers/primeapi/payloads/model_to_payload.go @@ -793,7 +793,7 @@ func Upload(appCtx appcontext.AppContext, storer storage.FileStorer, upload *mod UpdatedAt: strfmt.DateTime(upload.UpdatedAt), } - url, err := storer.PresignedURL(upload.StorageKey, upload.ContentType) + url, err := storer.PresignedURL(upload.StorageKey, upload.ContentType, upload.Filename) if err == nil { payload.URL = *handlers.FmtURI(url) } else { diff --git a/pkg/storage/filesystem.go b/pkg/storage/filesystem.go index f33c9fdf380..259fd4ee8ab 100644 --- a/pkg/storage/filesystem.go +++ b/pkg/storage/filesystem.go @@ -95,9 +95,10 @@ func (fs *Filesystem) Delete(key string) error { } // PresignedURL returns a URL that provides access to a file for 15 mintes. -func (fs *Filesystem) PresignedURL(key, contentType string) (string, error) { +func (fs *Filesystem) PresignedURL(key, contentType string, filename string) (string, error) { values := url.Values{} values.Add("contentType", contentType) + values.Add("filename", filename) url := fs.webRoot + "/" + key + "?" + values.Encode() return url, nil } diff --git a/pkg/storage/filesystem_test.go b/pkg/storage/filesystem_test.go index d501201809d..27ecc5e951c 100644 --- a/pkg/storage/filesystem_test.go +++ b/pkg/storage/filesystem_test.go @@ -11,12 +11,12 @@ func TestFilesystemPresignedURL(t *testing.T) { } fs := NewFilesystem(fsParams) - url, err := fs.PresignedURL("key/to/file/12345", "image/jpeg") + url, err := fs.PresignedURL("key/to/file/12345", "image/jpeg", "testimage.jpeg") if err != nil { t.Fatalf("could not get presigned url: %s", err) } - expected := "https://example.text/files/key/to/file/12345?contentType=image%2Fjpeg" + expected := "https://example.text/files/key/to/file/12345?contentType=image%2Fjpeg&filename=testimage.jpeg" if url != expected { t.Errorf("wrong presigned url: expected %s, got %s", expected, url) } diff --git a/pkg/storage/memory.go b/pkg/storage/memory.go index 6d2de9a3893..2f06ed6b96e 100644 --- a/pkg/storage/memory.go +++ b/pkg/storage/memory.go @@ -95,9 +95,10 @@ func (fs *Memory) Delete(key string) error { } // PresignedURL returns a URL that provides access to a file for 15 mintes. -func (fs *Memory) PresignedURL(key, contentType string) (string, error) { +func (fs *Memory) PresignedURL(key, contentType string, filename string) (string, error) { values := url.Values{} values.Add("contentType", contentType) + values.Add("filename", filename) url := fs.webRoot + "/" + key + "?" + values.Encode() return url, nil } diff --git a/pkg/storage/memory_test.go b/pkg/storage/memory_test.go index ef346358e63..59384c5acee 100644 --- a/pkg/storage/memory_test.go +++ b/pkg/storage/memory_test.go @@ -11,12 +11,12 @@ func TestMemoryPresignedURL(t *testing.T) { } fs := NewMemory(fsParams) - url, err := fs.PresignedURL("key/to/file/12345", "image/jpeg") + url, err := fs.PresignedURL("key/to/file/12345", "image/jpeg", "testimage.jpeg") if err != nil { t.Fatalf("could not get presigned url: %s", err) } - expected := "https://example.text/files/key/to/file/12345?contentType=image%2Fjpeg" + expected := "https://example.text/files/key/to/file/12345?contentType=image%2Fjpeg&filename=testimage.jpeg" if url != expected { t.Errorf("wrong presigned url: expected %s, got %s", expected, url) } diff --git a/pkg/storage/mocks/FileStorer.go b/pkg/storage/mocks/FileStorer.go index 851c718b1d8..3f120a8004d 100644 --- a/pkg/storage/mocks/FileStorer.go +++ b/pkg/storage/mocks/FileStorer.go @@ -73,23 +73,23 @@ func (_m *FileStorer) FileSystem() *afero.Afero { return r0 } -// PresignedURL provides a mock function with given fields: _a0, _a1 -func (_m *FileStorer) PresignedURL(_a0 string, _a1 string) (string, error) { - ret := _m.Called(_a0, _a1) +// PresignedURL provides a mock function with given fields: _a0, _a1, _a2 +func (_m *FileStorer) PresignedURL(_a0 string, _a1 string, _a2 string) (string, error) { + ret := _m.Called(_a0, _a1, _a2) var r0 string var r1 error - if rf, ok := ret.Get(0).(func(string, string) (string, error)); ok { - return rf(_a0, _a1) + if rf, ok := ret.Get(0).(func(string, string, string) (string, error)); ok { + return rf(_a0, _a1, _a2) } - if rf, ok := ret.Get(0).(func(string, string) string); ok { - r0 = rf(_a0, _a1) + if rf, ok := ret.Get(0).(func(string, string, string) string); ok { + r0 = rf(_a0, _a1, _a2) } else { r0 = ret.Get(0).(string) } - if rf, ok := ret.Get(1).(func(string, string) error); ok { - r1 = rf(_a0, _a1) + if rf, ok := ret.Get(1).(func(string, string, string) error); ok { + r1 = rf(_a0, _a1, _a2) } else { r1 = ret.Error(1) } diff --git a/pkg/storage/s3.go b/pkg/storage/s3.go index 1a492549aef..7ff15650f6d 100644 --- a/pkg/storage/s3.go +++ b/pkg/storage/s3.go @@ -114,7 +114,7 @@ func (s *S3) TempFileSystem() *afero.Afero { } // PresignedURL returns a URL that provides access to a file for 15 minutes. -func (s *S3) PresignedURL(key string, contentType string) (string, error) { +func (s *S3) PresignedURL(key string, contentType string, filename string) (string, error) { namespacedKey := path.Join(s.keyNamespace, key) presignClient := s3.NewPresignClient(s.client) @@ -123,7 +123,7 @@ func (s *S3) PresignedURL(key string, contentType string) (string, error) { Bucket: &s.bucket, Key: &namespacedKey, ResponseContentType: &contentType, - ResponseContentDisposition: models.StringPointer("attachment"), + ResponseContentDisposition: models.StringPointer("attachment; filename=" + filename), }, func(opts *s3.PresignOptions) { opts.Expires = 15 * time.Minute diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 6f01fb70a83..719afc124b6 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -37,7 +37,7 @@ type FileStorer interface { Store(string, io.ReadSeeker, string, *string) (*StoreResult, error) Fetch(string) (io.ReadCloser, error) Delete(string) error - PresignedURL(string, string) (string, error) + PresignedURL(string, string, string) (string, error) FileSystem() *afero.Afero TempFileSystem() *afero.Afero Tags(string) (map[string]string, error) diff --git a/pkg/storage/test/s3.go b/pkg/storage/test/s3.go index 3cd02c5b683..028799c0b85 100644 --- a/pkg/storage/test/s3.go +++ b/pkg/storage/test/s3.go @@ -54,8 +54,8 @@ func (fake *FakeS3Storage) Fetch(key string) (io.ReadCloser, error) { } // PresignedURL returns a URL that can be used to retrieve a file. -func (fake *FakeS3Storage) PresignedURL(key string, contentType string) (string, error) { - url := fmt.Sprintf("https://example.com/dir/%s?contentType=%s&signed=test", key, contentType) +func (fake *FakeS3Storage) PresignedURL(key string, contentType string, filename string) (string, error) { + url := fmt.Sprintf("https://example.com/dir/%s?contentType=%s&filename=%s&signed=test", key, contentType, filename) return url, nil } diff --git a/pkg/uploader/uploader.go b/pkg/uploader/uploader.go index d7d64dee3bc..6b0c43d3238 100644 --- a/pkg/uploader/uploader.go +++ b/pkg/uploader/uploader.go @@ -258,7 +258,7 @@ func (u *Uploader) CreateUpload(appCtx appcontext.AppContext, file File, allowed // PresignedURL returns a URL that can be used to access an Upload's file. func (u *Uploader) PresignedURL(appCtx appcontext.AppContext, upload *models.Upload) (string, error) { - url, err := u.Storer.PresignedURL(upload.StorageKey, upload.ContentType) + url, err := u.Storer.PresignedURL(upload.StorageKey, upload.ContentType, upload.Filename) if err != nil { appCtx.Logger().Error("failed to get presigned url", zap.Error(err)) return "", err From 5621a169a5eb64da71280ba192c729eb4ade28e3 Mon Sep 17 00:00:00 2001 From: ryan-mchugh Date: Mon, 7 Oct 2024 20:52:38 +0000 Subject: [PATCH 2/5] B-20766 - filename removed from download attribute. --- src/components/UploadsTable/UploadsTable.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/UploadsTable/UploadsTable.jsx b/src/components/UploadsTable/UploadsTable.jsx index e1b9b10e4de..711d6e51a2f 100644 --- a/src/components/UploadsTable/UploadsTable.jsx +++ b/src/components/UploadsTable/UploadsTable.jsx @@ -39,7 +39,7 @@ const UploadsTable = ({ className, uploads, onDelete, showDeleteButton, showDown

{showDownloadLink ? ( - + {upload.filename} ) : ( From c15aaaeb1bc98992d20120e739b33f665c627a48 Mon Sep 17 00:00:00 2001 From: ryan-mchugh Date: Tue, 8 Oct 2024 19:59:40 +0000 Subject: [PATCH 3/5] B-20766 - more appropriate testing on fakeS3 document url. --- pkg/handlers/ghcapi/documents_test.go | 7 ++++++- pkg/handlers/internalapi/documents_test.go | 7 ++++++- pkg/storage/test/s3.go | 7 ++++++- 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/pkg/handlers/ghcapi/documents_test.go b/pkg/handlers/ghcapi/documents_test.go index d814726d26d..5f495c760a8 100644 --- a/pkg/handlers/ghcapi/documents_test.go +++ b/pkg/handlers/ghcapi/documents_test.go @@ -3,6 +3,7 @@ package ghcapi import ( "fmt" "net/http" + "net/url" "github.com/go-openapi/strfmt" "github.com/gofrs/uuid" @@ -64,7 +65,11 @@ func (suite *HandlerSuite) TestGetDocumentHandler() { } uploadPayload := documentPayload.Uploads[0] - expectedURL := fmt.Sprintf("https://example.com/dir/%s?contentType=%s&signed=test", userUpload.Upload.StorageKey, uploader.FileTypePDF) + values := url.Values{} + values.Add("response-content-type", uploader.FileTypePDF) + values.Add("response-content-disposition", "attachment; filename="+userUpload.Upload.Filename) + values.Add("signed", "test") + expectedURL := fmt.Sprintf("https://example.com/dir/%s?", userUpload.Upload.StorageKey) + values.Encode() if (uploadPayload.URL).String() != expectedURL { t.Errorf("wrong URL for upload, expected %s, got %s", expectedURL, uploadPayload.URL) } diff --git a/pkg/handlers/internalapi/documents_test.go b/pkg/handlers/internalapi/documents_test.go index 92771bf15e3..4ef3886038e 100644 --- a/pkg/handlers/internalapi/documents_test.go +++ b/pkg/handlers/internalapi/documents_test.go @@ -3,6 +3,7 @@ package internalapi import ( "fmt" "net/http" + "net/url" "github.com/go-openapi/strfmt" "github.com/gofrs/uuid" @@ -100,7 +101,11 @@ func (suite *HandlerSuite) TestShowDocumentHandler() { } uploadPayload := documentPayload.Uploads[0] - expectedURL := fmt.Sprintf("https://example.com/dir/%s?contentType=%s&signed=test", userUpload.Upload.StorageKey, uploader.FileTypePDF) + values := url.Values{} + values.Add("response-content-type", uploader.FileTypePDF) + values.Add("response-content-disposition", "attachment; filename="+userUpload.Upload.Filename) + values.Add("signed", "test") + expectedURL := fmt.Sprintf("https://example.com/dir/%s?", userUpload.Upload.StorageKey) + values.Encode() if (uploadPayload.URL).String() != expectedURL { t.Errorf("wrong URL for upload, expected %s, got %s", expectedURL, uploadPayload.URL) } diff --git a/pkg/storage/test/s3.go b/pkg/storage/test/s3.go index 028799c0b85..a9e404541b1 100644 --- a/pkg/storage/test/s3.go +++ b/pkg/storage/test/s3.go @@ -3,6 +3,7 @@ package test import ( "fmt" "io" + "net/url" "github.com/pkg/errors" "github.com/spf13/afero" @@ -55,7 +56,11 @@ func (fake *FakeS3Storage) Fetch(key string) (io.ReadCloser, error) { // PresignedURL returns a URL that can be used to retrieve a file. func (fake *FakeS3Storage) PresignedURL(key string, contentType string, filename string) (string, error) { - url := fmt.Sprintf("https://example.com/dir/%s?contentType=%s&filename=%s&signed=test", key, contentType, filename) + values := url.Values{} + values.Add("response-content-type", contentType) + values.Add("response-content-disposition", "attachment; filename="+filename) + values.Add("signed", "test") + url := fmt.Sprintf("https://example.com/dir/%s?", key) + values.Encode() return url, nil } From a82672ed2dc5eab22be154ed96135205bd963f6a Mon Sep 17 00:00:00 2001 From: ryan-mchugh Date: Wed, 9 Oct 2024 15:07:04 +0000 Subject: [PATCH 4/5] B-20766 - fix encoding for url check on tests. --- pkg/handlers/internalapi/uploads_test.go | 31 ++++++++++++------------ 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/pkg/handlers/internalapi/uploads_test.go b/pkg/handlers/internalapi/uploads_test.go index 263b4f6f0a0..5780ce695f2 100644 --- a/pkg/handlers/internalapi/uploads_test.go +++ b/pkg/handlers/internalapi/uploads_test.go @@ -12,6 +12,7 @@ package internalapi import ( "fmt" "net/http" + "net/url" "github.com/go-openapi/runtime/middleware" "github.com/go-openapi/strfmt" @@ -464,9 +465,9 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() { suite.NotEmpty(createdResponse.Payload.ID) suite.Equal(FixtureXLS, createdResponse.Payload.Filename) suite.Equal(uploader.FileTypeExcel, createdResponse.Payload.ContentType) - suite.Contains(createdResponse.Payload.URL, document.ServiceMember.UserID.String()) - suite.Contains(createdResponse.Payload.URL, upload.ID.String()) - suite.Contains(createdResponse.Payload.URL, uploader.FileTypeExcel) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape(document.ServiceMember.UserID.String())) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape(upload.ID.String())) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape(uploader.FileTypeExcel)) }) suite.Run("uploads .xlsx file", func() { @@ -488,9 +489,9 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() { suite.NotEmpty(createdResponse.Payload.ID) suite.Equal(FixtureXLSX, createdResponse.Payload.Filename) suite.Equal(uploader.FileTypeExcelXLSX, createdResponse.Payload.ContentType) - suite.Contains(createdResponse.Payload.URL, document.ServiceMember.UserID.String()) - suite.Contains(createdResponse.Payload.URL, upload.ID.String()) - suite.Contains(createdResponse.Payload.URL, uploader.FileTypeExcelXLSX) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape(document.ServiceMember.UserID.String())) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape(upload.ID.String())) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape(uploader.FileTypeExcelXLSX)) }) suite.Run("uploads weight estimator .xlsx file (full weight)", func() { @@ -512,9 +513,9 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() { suite.NotEmpty(createdResponse.Payload.ID) suite.Contains(createdResponse.Payload.Filename, WeightEstimatorPrefix) suite.Equal(uploader.FileTypePDF, createdResponse.Payload.ContentType) - suite.Contains(createdResponse.Payload.URL, document.ServiceMember.UserID.String()) - suite.Contains(createdResponse.Payload.URL, upload.ID.String()) - suite.Contains(createdResponse.Payload.URL, uploader.FileTypePDF) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape(document.ServiceMember.UserID.String())) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape(upload.ID.String())) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape(uploader.FileTypePDF)) }) suite.Run("uploads file for a progear document", func() { @@ -536,9 +537,9 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() { suite.NotEmpty(createdResponse.Payload.ID) suite.Equal(FixturePNG, createdResponse.Payload.Filename) suite.Equal(uploader.FileTypePNG, createdResponse.Payload.ContentType) - suite.Contains(createdResponse.Payload.URL, document.ServiceMember.UserID.String()) - suite.Contains(createdResponse.Payload.URL, upload.ID.String()) - suite.Contains(createdResponse.Payload.URL, uploader.FileTypePNG) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape(document.ServiceMember.UserID.String())) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape(upload.ID.String())) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape(uploader.FileTypePNG)) }) suite.Run("uploads file for an expense document", func() { @@ -560,9 +561,9 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() { suite.NotEmpty(createdResponse.Payload.ID) suite.Equal(FixtureJPG, createdResponse.Payload.Filename) suite.Equal(uploader.FileTypeJPEG, createdResponse.Payload.ContentType) - suite.Contains(createdResponse.Payload.URL, document.ServiceMember.UserID.String()) - suite.Contains(createdResponse.Payload.URL, upload.ID.String()) - suite.Contains(createdResponse.Payload.URL, uploader.FileTypeJPEG) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape(document.ServiceMember.UserID.String())) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape(upload.ID.String())) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape(uploader.FileTypeJPEG)) }) } From 1da63118a5a34f8d9fa272bedf93d6e6d9b92c3b Mon Sep 17 00:00:00 2001 From: ryan-mchugh Date: Thu, 10 Oct 2024 17:55:36 +0000 Subject: [PATCH 5/5] B-20766 - fix for edge case where filename contains unsupported chars for s3 header. --- pkg/handlers/internalapi/uploads_test.go | 40 ++++++++++++++++++ pkg/storage/s3.go | 17 ++++++-- pkg/storage/test/s3.go | 15 ++++++- ... 2024-10-10 at 10.46.48\342\200\257AM.png" | Bin 0 -> 45105 bytes 4 files changed, 68 insertions(+), 4 deletions(-) create mode 100644 "pkg/testdatagen/testdata/Screenshot 2024-10-10 at 10.46.48\342\200\257AM.png" diff --git a/pkg/handlers/internalapi/uploads_test.go b/pkg/handlers/internalapi/uploads_test.go index 5780ce695f2..077f037f357 100644 --- a/pkg/handlers/internalapi/uploads_test.go +++ b/pkg/handlers/internalapi/uploads_test.go @@ -17,6 +17,7 @@ import ( "github.com/go-openapi/runtime/middleware" "github.com/go-openapi/strfmt" "github.com/gofrs/uuid" + "golang.org/x/text/encoding/charmap" "github.com/transcom/mymove/pkg/factory" ppmop "github.com/transcom/mymove/pkg/gen/internalapi/internaloperations/ppm" @@ -39,6 +40,7 @@ const FixtureXLSX = "Weight Estimator.xlsx" const WeightEstimatorFullXLSX = "Weight Estimator Full.xlsx" const WeightEstimatorPrefix = "Weight Estimator Full" const FixtureEmpty = "empty.pdf" +const FixtureScreenshot = "Screenshot 2024-10-10 at 10.46.48 AM.png" func createPrereqs(suite *HandlerSuite, fixtureFile string) (models.Document, uploadop.CreateUploadParams) { document := factory.BuildDocument(suite.DB(), nil, nil) @@ -468,6 +470,7 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() { suite.Contains(createdResponse.Payload.URL, url.QueryEscape(document.ServiceMember.UserID.String())) suite.Contains(createdResponse.Payload.URL, url.QueryEscape(upload.ID.String())) suite.Contains(createdResponse.Payload.URL, url.QueryEscape(uploader.FileTypeExcel)) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape("attachment; filename="+upload.Filename)) }) suite.Run("uploads .xlsx file", func() { @@ -492,6 +495,7 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() { suite.Contains(createdResponse.Payload.URL, url.QueryEscape(document.ServiceMember.UserID.String())) suite.Contains(createdResponse.Payload.URL, url.QueryEscape(upload.ID.String())) suite.Contains(createdResponse.Payload.URL, url.QueryEscape(uploader.FileTypeExcelXLSX)) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape("attachment; filename="+upload.Filename)) }) suite.Run("uploads weight estimator .xlsx file (full weight)", func() { @@ -516,6 +520,7 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() { suite.Contains(createdResponse.Payload.URL, url.QueryEscape(document.ServiceMember.UserID.String())) suite.Contains(createdResponse.Payload.URL, url.QueryEscape(upload.ID.String())) suite.Contains(createdResponse.Payload.URL, url.QueryEscape(uploader.FileTypePDF)) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape("attachment; filename="+upload.Filename)) }) suite.Run("uploads file for a progear document", func() { @@ -540,6 +545,7 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() { suite.Contains(createdResponse.Payload.URL, url.QueryEscape(document.ServiceMember.UserID.String())) suite.Contains(createdResponse.Payload.URL, url.QueryEscape(upload.ID.String())) suite.Contains(createdResponse.Payload.URL, url.QueryEscape(uploader.FileTypePNG)) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape("attachment; filename="+upload.Filename)) }) suite.Run("uploads file for an expense document", func() { @@ -564,6 +570,40 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() { suite.Contains(createdResponse.Payload.URL, url.QueryEscape(document.ServiceMember.UserID.String())) suite.Contains(createdResponse.Payload.URL, url.QueryEscape(upload.ID.String())) suite.Contains(createdResponse.Payload.URL, url.QueryEscape(uploader.FileTypeJPEG)) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape("attachment; filename="+upload.Filename)) + }) + + suite.Run("uploads file with filename characters not supported by ISO8859_1", func() { + fakeS3 := storageTest.NewFakeS3Storage(true) + document, params := createPPMExpensePrereqs(suite, FixtureScreenshot) + + response := makePPMRequest(suite, params, document.ServiceMember, fakeS3) + + suite.IsType(&ppmop.CreatePPMUploadCreated{}, response) + + createdResponse, _ := response.(*ppmop.CreatePPMUploadCreated) + + upload := models.Upload{} + err := suite.DB().Find(&upload, createdResponse.Payload.ID) + + filenameBuffer := make([]byte, 0) + for _, r := range upload.Filename { + if encodedRune, ok := charmap.ISO8859_1.EncodeRune(r); ok { + filenameBuffer = append(filenameBuffer, encodedRune) + } + } + + suite.NoError(err) + suite.Equal("qEnueX0FLpoz4bTnliprog==", upload.Checksum) + + suite.NotEmpty(createdResponse.Payload.ID) + suite.Equal(FixtureScreenshot, createdResponse.Payload.Filename) + suite.Equal(uploader.FileTypePNG, createdResponse.Payload.ContentType) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape(document.ServiceMember.UserID.String())) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape(upload.ID.String())) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape(uploader.FileTypePNG)) + suite.NotContains(createdResponse.Payload.URL, url.QueryEscape(upload.Filename)) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape("attachment; filename="+string(filenameBuffer))) }) } diff --git a/pkg/storage/s3.go b/pkg/storage/s3.go index 7ff15650f6d..02a425e00a3 100644 --- a/pkg/storage/s3.go +++ b/pkg/storage/s3.go @@ -11,8 +11,7 @@ import ( "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/pkg/errors" "github.com/spf13/afero" - - "github.com/transcom/mymove/pkg/models" + "golang.org/x/text/encoding/charmap" ) // S3 implements the file storage API using S3. @@ -118,12 +117,24 @@ func (s *S3) PresignedURL(key string, contentType string, filename string) (stri namespacedKey := path.Join(s.keyNamespace, key) presignClient := s3.NewPresignClient(s.client) + filenameBuffer := make([]byte, 0) + for _, r := range filename { + if encodedRune, ok := charmap.ISO8859_1.EncodeRune(r); ok { + filenameBuffer = append(filenameBuffer, encodedRune) + } + } + + contentDisposition := "attachment" + if len(filenameBuffer) > 0 { + contentDisposition += "; filename=" + string(filenameBuffer) + } + req, err := presignClient.PresignGetObject(context.Background(), &s3.GetObjectInput{ Bucket: &s.bucket, Key: &namespacedKey, ResponseContentType: &contentType, - ResponseContentDisposition: models.StringPointer("attachment; filename=" + filename), + ResponseContentDisposition: &contentDisposition, }, func(opts *s3.PresignOptions) { opts.Expires = 15 * time.Minute diff --git a/pkg/storage/test/s3.go b/pkg/storage/test/s3.go index a9e404541b1..da076681dfe 100644 --- a/pkg/storage/test/s3.go +++ b/pkg/storage/test/s3.go @@ -7,6 +7,7 @@ import ( "github.com/pkg/errors" "github.com/spf13/afero" + "golang.org/x/text/encoding/charmap" "github.com/transcom/mymove/pkg/storage" ) @@ -56,9 +57,21 @@ func (fake *FakeS3Storage) Fetch(key string) (io.ReadCloser, error) { // PresignedURL returns a URL that can be used to retrieve a file. func (fake *FakeS3Storage) PresignedURL(key string, contentType string, filename string) (string, error) { + filenameBuffer := make([]byte, 0) + for _, r := range filename { + if encodedRune, ok := charmap.ISO8859_1.EncodeRune(r); ok { + filenameBuffer = append(filenameBuffer, encodedRune) + } + } + + contentDisposition := "attachment" + if len(filenameBuffer) > 0 { + contentDisposition += "; filename=" + string(filenameBuffer) + } + values := url.Values{} values.Add("response-content-type", contentType) - values.Add("response-content-disposition", "attachment; filename="+filename) + values.Add("response-content-disposition", contentDisposition) values.Add("signed", "test") url := fmt.Sprintf("https://example.com/dir/%s?", key) + values.Encode() return url, nil diff --git "a/pkg/testdatagen/testdata/Screenshot 2024-10-10 at 10.46.48\342\200\257AM.png" "b/pkg/testdatagen/testdata/Screenshot 2024-10-10 at 10.46.48\342\200\257AM.png" new file mode 100644 index 0000000000000000000000000000000000000000..858b92d7a4ac85a9e5388123a878b08d01ff0c21 GIT binary patch literal 45105 zcmeI4c~nyQ-^X!l87r$PE6ru5XU594akL!Q#x}FuYP3+nC1c7I(+C4ashOI}rp+ZY zrN+!1Qd1K|%*rKArd-ez&`eDra6=RXp6kr?Ykt4zJbygD^PK0L-`8_E+ymT;_ulX4 z{eHba-+MXl<*{?|XL_H(V6eq*ySDkjVDqA3FiqKldC)5bH!K`sFg24SuC89;t~*`P z$I#LHqJl%iwjM%()zF|IpIv6_8-tez1vNf5-SAm7*5{XBl6-;^ z>S~*e8*85jJ*~ZYBOst}_0qn_Fik7Hm2tHC+6BK^@=v$aB{yh4^zpfPoj7S^-0ESm zeq(S@Pz$UyrP=*67lEO%|9@@JZ$ZF9R;d{*ID}R!Y5PL(d63Q0DTDhruNb3@Wg1tW zr)eIxCJJ7=pS!Q+ax9N-Z2b1dHBxc`Y@B}fPEO9xU!Nx!FWBv4y4&SJ3}I`S_hHP?O|S_tJ9O#tX?ptW=h))-*5Eb0CePzg&lA^ zymv2bBec8#rm3a}(}I@Npof#1{y&y?sI7--{PDUv40i4aO!KeLctFo{f2q)8Zk<1$ z)o-i6|HQoL+ZykeHDz-jtsolgf}R#e?K&6(gRNRV_fSKTUP33-1?IMG>;70Z0sB-^ zCZ=lss8RCM(<_#)=!I)NFyC0ZazI^g>DK+nyq6Vp{Gxw)!|tnJ?Fv!P-=?;5(*^$( z-Y$EW9ap>BdB8h4$0b|q>UOv6hp_U*v$|CZ4xZUNa^h8MlJ#TJ*vu_W{Ln4yVF%{0 zjFT{g=T1aUb}`UTFTvE*H5V*3c1iqb;kF&|QbtCj%O~ERTOl*?%Lbx~RsY)9kNaJ& zw*6>T|K%q?vOapP#wWK=+@ZC??$W$avrqkpiO|c| zY1}uLciqiDvSujeD3w?_> zHmr@=ldo(gIWrvI9r5hx6D<;Jh0btXGy550Om5~$c-3$geqJED?x%%e<8PY6q?03I zLoMlHWzTK>2QWs`-_JcmD#p`FpWF5%$F$nz9;+Us%HHY_M$Zf?WFpHl6w`u&>nQAu zB}rY)BWvXE&}rf1s(Z6MKgLG)RI?E$^t0p*VU_HYrVR$zfb>nF^>D_p%3WKKlvcY# zsc^lgTs1lN(qr5%K`}vUUScn>5=11-4j83PFr683e1Go7iJ|AFs7y@fJZ|XVvy`xx zciq^0!bAr5M8Yh$&vNnDA3L}c$~iSy57m6SDG&?XaR?u*c+O$TwDtz8rR*5yVe*=eFFmDMV-QG>U&PJ9at2?BXg*i^|?;_bPFxKdtNt?|9kF zP=q?_<8oA8d_^6d$|oS2Lb6Sjmt_NW#UAyfyZgr<&ZU15@`hA~!-~ArgDG z^|Y4uGk>nMGy28$RmBnksXwlUwOm!lH7HL#RFWpTC*W# zxQf;$6OzN|$TE3#hp&IiPQz1oW$YJDp&@2m{I+JrD#c4fykOJY5--mcMGe0qpQ$QBW>RZ?Cd3l|x$a|_4I4?JJ?VuwBkMOf@mh?20emS}uW z&og_@*ssx#v{ogYjcYJI=q)$Z8A`c`7Cz)K-FGI8 z*PhSzJGR)tI-8Kv!%&DR!aXDfsE_MUEI=jhxwYkZ}d@ zzqnH96xwWFxpw-RaQIHwVoBM2`E88D0qs=l)-Bm_ynQ6z1u9&9Np(kQPL^F+RDa$0 zQ%gcgH9t_KH+^ku0;1yBK?x>8+_cZbE-VxI+?PV%$CX?#C9fB6$wR$aA3g6Yv9YQ$ z3tbQGylVj3F`9_G_w?#c!|uS0yGoj%dZLvbiWkwbl2KP3$NfBv7q3`p6V1~SIFAx_ zP7Lmnyiv+M%{M!ZU%gy}p(iUSBW$B)37Is;Oi+}q#S2OX>$EKe_Jw(p?w&|m#0(?= zaD;@)5*zAGas#pK^7b$Duy`h()5ea6YPI?-Tg*ZtIpe97lkq7B;=Ro;AjU5EozQv; z)g=pm@tgg$JA{*5eVqvp$FjoK9~eiAbvJ0B=k$gdH)Kr<@4XdOw{Y-^>ynY(_lXq! z=@`f6T-^lE*t*ZDp@;e=*Vt$XVIefn~?I$^Mz)987(5&}yxoVwC<=~&FljH&v zjmtjXVBk(4a;K*@y{qIRgbxRPCHLjCJkqU#zn9FPAsUTOkd)PHk3U(KgFxpf6wQ!2 z3aKXImP;m1YZo(gBFBx6WY(Q}aS^fKw;0}$862&Y zOVT!1_b+B+&`0yj$o}RfS<_EUQgK5*nU#bHwyys%?+PwKR)d`3NNQN`;9MH6ojKFP zRrnIdE+Uf)$<%_iQMGDpzv~?@4{ebx9xRicUHvE%ci7wUaUSO=adTI(p^3A6#IyOr zO4DbX8rJTsnaC6^nkuz1EJTX@&^AgF=b5H(hs|9&q6DSPUCvZJuX6cZZT;njbJ9KP zuFLXJS9(tkXXfZGD{q`>K{>U_Ux^438ae6!3m0vJ9CGs@A#W*@*^O7)5qlxFI3eo8GWd| zKH5;SV3E}g7ss+UvlC3@0M1riH*x*zy*z~Cobzn(eI(zokd>5Qi3`kxADWC5H5;%&fO3J2Z(14k(;rA?>N(l+Cj@57uh$_H4_<)w2@KD0RH$O`;rR}`8C;g=; zT}hY8MG1`dhWRpz?iolmxp2mS%6eFkl#@vD)prjRIffABQ$3XS*E!0F*H6wv(rA9L z^seagn4BbHlk8#+W2ZdKx8C!}gfDTP9gXfI&QDFNnap9^1oJS#J8=5N60)_Ke?<{G z30J?zvF*o^mpOy_-02AK0X6i-@4K-prq5klR_%2l%~Y60ishYB zNDI9vqBfMxz=^n`?ag_b#LRMKCFMXF~>?3Vz_((gYRrZQiQ_;!iX+Xp0 zICntqUc)(4p68ErJ3tA4v*Y7g!bBKxY=zgGq}&RoUa1RR@oX1beA5!*312&L`@Uuk z9S1Kb_zKY(;hj^LSDopN3lTy%=uTLE-Di;-7~j_K_ieu`wi{6vd11 zv<4hfTv4w*t2(4?n%lcC$d!gwE=p^YvoeeF1vfUM5$V<_kC}d*gahR2@n=Gum3YrZ zq*BUa8_D?H8^qxcY`YJ|Dow1Q0qlNV+HqLSrSds=Q6Pj(6moS}yoBBSVyBVgk1wnO zESdOB-tKc8* zj$u0P{I=mNAs?ZgC-ir}oy2R^^CljxS3~uH7_Mykil@U33=EZd^-AJ|5@QW9jEC&a_n2>jJV~F zz3^eS(sf-8k~yAVX*An6pDhlZSK}>*S3cj&t(H*`Ca4Ef`qOp;;utxvS|?%pF}opN z7(?PeDhp+ z1)cYHNv~8m*?R8}2&HKA=_jWI-8l&7p-1zL%;s3Tc@CxU1u|ZR6mv*i0qIyOG?LbJ zqXS87$<~3m+$_uw+WDA3<~;(t*TiY;d}IMlNG%jVNX|~T({Y}S;@&m&BMZnS5AKv< zq=ZKXA0^c}X-m4A_$4YSi2iQC#H;yyamA~Gc58p0*o$W*mT(i4lZ(iB!bF%k%gK!42x8gWKs1aXC5hY|n zynyv2&1EkrJPkIkyDT7(ziNgY(yyW`Sx{6cR3N)$i&VrLqJnK{R* z22eu;1~NS%e0!lE#bPrzE+)-2KG2-XTN5BIe6_@3v{u33eS+}z z24yEGUr0)h%f2<*7IaIC*R3nAlZXqSp6m)f_syyCH$Dt___vYSV;pf|8}HyIaX9@X z8^aO`Zg7q@r&(HxsW#%G-w+Es@sW3xWO#Xjy>_0$pAlWG>(8fNswePqB3&cO&j;m4 zspmg8Np+m+%C8KrH}0__J#TY5~*&s0C09@B8vV4}cy3Jpg(D^x#A9 z1ZX1AM4*X46M-fIP5k?H6TlaMF92Tvz5sjy_yX{Sf1d6CPxeNJF=^BL-vR*I0JH&U z1JH(ly{z&-Ss#HW0!;*(2s9CB;>UNB0$%_Jte?MfRO+r0Y(Ch^uJ~#<>M@a z70_>cE?p|%!hhB8Nfdv4|H0DbQEz^#U!L;!lKcA?Pebp=y(>2V)V&8_eQ^B1_yP3; z^y34z0sZ`7-oQVAe*%60`~>*%@2>D?n`iD7z|Vl60Y3wN2K)^8`F-pA?@j^u8SpdU zXTZ;ZpZ~6kfS>24%0Yer`3dAlkRL&Q1o;uxE}_yFm~Ivb^l+@O2FcP#Q}>076&X2SRAnU z`wkVzB492P%;kc)Y%rG%dNH6E19~x_7Xx}R|K?r{=)e2{SfKv|`cI($1o}^){{;F^ zp#KE=Pyfo9Ab*1V3Gye%pZ}^0@0Akd2aq2?e)xZsABg3s`P*SIn5L$zGIgNdHpp?= zhuJi!YKd18*O`2qnx`6fT@q{464wofqazloqCv1eIDTOKfcgRY0rXSVomG(20RI5~ r3HSl<6X3^x!tQ_c*Z*h!u7y?0e`vs4&SOFt4|Ch@v5owF(CPmM