diff --git a/pkg/handlers/ghcapi/documents_test.go b/pkg/handlers/ghcapi/documents_test.go index 5f495c760a8..582f9cf0902 100644 --- a/pkg/handlers/ghcapi/documents_test.go +++ b/pkg/handlers/ghcapi/documents_test.go @@ -4,6 +4,7 @@ import ( "fmt" "net/http" "net/url" + "strconv" "github.com/go-openapi/strfmt" "github.com/gofrs/uuid" @@ -52,6 +53,9 @@ func (suite *HandlerSuite) TestGetDocumentHandler() { } documentPayload := showResponse.Payload + // Double quote the filename to be able to handle filenames with commas in them + quotedFilename := strconv.Quote(userUpload.Upload.Filename) + // Validate outgoing payload suite.NoError(documentPayload.Validate(strfmt.Default)) @@ -67,7 +71,74 @@ func (suite *HandlerSuite) TestGetDocumentHandler() { uploadPayload := documentPayload.Uploads[0] values := url.Values{} values.Add("response-content-type", uploader.FileTypePDF) - values.Add("response-content-disposition", "attachment; filename="+userUpload.Upload.Filename) + values.Add("response-content-disposition", "attachment; filename="+quotedFilename) + 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) + } +} + +func (suite *HandlerSuite) TestGetDocumentHandlerForFilenamesWithCommas() { + t := suite.T() + + userUpload := factory.BuildUserUpload(suite.DB(), []factory.Customization{ + { + Model: models.Upload{ + Filename: "FIRST, LAST - ISM AUTHORIZATION", + }, + }, + }, nil) + + documentID := userUpload.DocumentID + var document models.Document + + err := suite.DB().Eager("ServiceMember.User").Find(&document, documentID) + if err != nil { + suite.Fail("could not load document: %s", err) + } + + params := documentop.NewGetDocumentParams() + params.DocumentID = strfmt.UUID(documentID.String()) + + req := &http.Request{} + req = suite.AuthenticateRequest(req, document.ServiceMember) + params.HTTPRequest = req + + handlerConfig := suite.HandlerConfig() + fakeS3 := storageTest.NewFakeS3Storage(true) + handlerConfig.SetFileStorer(fakeS3) + handler := GetDocumentHandler{handlerConfig} + + // Validate incoming payload: no body to validate + + response := handler.Handle(params) + + showResponse, ok := response.(*documentop.GetDocumentOK) + if !ok { + suite.Fail("Request failed: %#v", response) + } + documentPayload := showResponse.Payload + + // Validate outgoing payload + suite.NoError(documentPayload.Validate(strfmt.Default)) + + responseDocumentUUID := documentPayload.ID.String() + if responseDocumentUUID != documentID.String() { + t.Errorf("wrong document uuid, expected %v, got %v", documentID, responseDocumentUUID) + } + + if len(documentPayload.Uploads) != 1 { + t.Errorf("wrong number of uploads, expected 1, got %d", len(documentPayload.Uploads)) + } + + // Double quote the filename to be able to handle filenames with commas in them + quotedFilename := strconv.Quote(userUpload.Upload.Filename) + + uploadPayload := documentPayload.Uploads[0] + values := url.Values{} + values.Add("response-content-type", uploader.FileTypePDF) + values.Add("response-content-disposition", "attachment; filename="+quotedFilename) values.Add("signed", "test") expectedURL := fmt.Sprintf("https://example.com/dir/%s?", userUpload.Upload.StorageKey) + values.Encode() if (uploadPayload.URL).String() != expectedURL { diff --git a/pkg/handlers/internalapi/documents_test.go b/pkg/handlers/internalapi/documents_test.go index 4ef3886038e..49f41024c97 100644 --- a/pkg/handlers/internalapi/documents_test.go +++ b/pkg/handlers/internalapi/documents_test.go @@ -4,6 +4,7 @@ import ( "fmt" "net/http" "net/url" + "strconv" "github.com/go-openapi/strfmt" "github.com/gofrs/uuid" @@ -91,6 +92,9 @@ func (suite *HandlerSuite) TestShowDocumentHandler() { } documentPayload := showResponse.Payload + // Double quote the filename to be able to handle filenames with commas in them + quotedFilename := strconv.Quote(userUpload.Upload.Filename) + responseDocumentUUID := documentPayload.ID.String() if responseDocumentUUID != documentID.String() { t.Errorf("wrong document uuid, expected %v, got %v", documentID, responseDocumentUUID) @@ -103,7 +107,7 @@ func (suite *HandlerSuite) TestShowDocumentHandler() { uploadPayload := documentPayload.Uploads[0] values := url.Values{} values.Add("response-content-type", uploader.FileTypePDF) - values.Add("response-content-disposition", "attachment; filename="+userUpload.Upload.Filename) + values.Add("response-content-disposition", "attachment; filename="+quotedFilename) values.Add("signed", "test") expectedURL := fmt.Sprintf("https://example.com/dir/%s?", userUpload.Upload.StorageKey) + values.Encode() if (uploadPayload.URL).String() != expectedURL { diff --git a/pkg/handlers/internalapi/uploads_test.go b/pkg/handlers/internalapi/uploads_test.go index 36119617912..36823072f73 100644 --- a/pkg/handlers/internalapi/uploads_test.go +++ b/pkg/handlers/internalapi/uploads_test.go @@ -13,6 +13,7 @@ import ( "fmt" "net/http" "net/url" + "strconv" "github.com/go-openapi/runtime/middleware" "github.com/go-openapi/strfmt" @@ -461,6 +462,9 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() { upload := models.Upload{} err := suite.DB().Find(&upload, createdResponse.Payload.ID) + // Double quote the filename to be able to handle filenames with commas in them + quotedFilename := strconv.Quote(upload.Filename) + suite.NoError(err) suite.Equal("V/Q6K9rVdEPVzgKbh5cn2x4Oci4XDaG4fcG04R41Iz4=", upload.Checksum) @@ -470,7 +474,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.Contains(createdResponse.Payload.URL, url.QueryEscape("attachment; filename="+quotedFilename)) }) suite.Run("uploads .xlsx file", func() { @@ -486,6 +490,9 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() { upload := models.Upload{} err := suite.DB().Find(&upload, createdResponse.Payload.ID) + // Double quote the filename to be able to handle filenames with commas in them + quotedFilename := strconv.Quote(upload.Filename) + suite.NoError(err) suite.Equal("eRZ1Cr3Ms0692k03ftoEdqXpvd/CHcbxmhEGEQBYVdY=", upload.Checksum) @@ -495,7 +502,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.Contains(createdResponse.Payload.URL, url.QueryEscape("attachment; filename="+quotedFilename)) }) suite.Run("uploads weight estimator .xlsx file (full weight)", func() { @@ -513,6 +520,9 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() { suite.NoError(err) + // Double quote the filename to be able to handle filenames with commas in them + quotedFilename := strconv.Quote(upload.Filename) + // uploaded xlsx document should now be converted to a pdf so we check for pdf instead of xlsx suite.NotEmpty(createdResponse.Payload.ID) suite.Contains(createdResponse.Payload.Filename, WeightEstimatorPrefix) @@ -520,7 +530,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.Contains(createdResponse.Payload.URL, url.QueryEscape("attachment; filename="+quotedFilename)) }) suite.Run("uploads file for a progear document", func() { @@ -536,6 +546,9 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() { upload := models.Upload{} err := suite.DB().Find(&upload, createdResponse.Payload.ID) + // Double quote the filename to be able to handle filenames with commas in them + quotedFilename := strconv.Quote(upload.Filename) + suite.NoError(err) suite.Equal("/io1MRhLi2BFk9eF+lH1Ax+hyH+bPhlEK7A9/bqWlPY=", upload.Checksum) @@ -545,7 +558,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.Contains(createdResponse.Payload.URL, url.QueryEscape("attachment; filename="+quotedFilename)) }) suite.Run("uploads file for an expense document", func() { @@ -561,6 +574,9 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() { upload := models.Upload{} err := suite.DB().Find(&upload, createdResponse.Payload.ID) + // Double quote the filename to be able to handle filenames with commas in them + quotedFilename := strconv.Quote(upload.Filename) + suite.NoError(err) suite.Equal("ibKT78j4CJecDXC6CbGISkqWFG5eSjCjlZJHlaFRho4=", upload.Checksum) @@ -570,7 +586,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.FileTypeJPEG)) - suite.Contains(createdResponse.Payload.URL, url.QueryEscape("attachment; filename="+upload.Filename)) + suite.Contains(createdResponse.Payload.URL, url.QueryEscape("attachment; filename="+quotedFilename)) }) suite.Run("uploads file with filename characters not supported by ISO8859_1", func() { @@ -586,8 +602,11 @@ func (suite *HandlerSuite) TestCreatePPMUploadsHandlerSuccess() { upload := models.Upload{} err := suite.DB().Find(&upload, createdResponse.Payload.ID) + // Double quote the filename to be able to handle filenames with commas in them + quotedFilename := strconv.Quote(upload.Filename) + filenameBuffer := make([]byte, 0) - for _, r := range upload.Filename { + for _, r := range quotedFilename { if encodedRune, ok := charmap.ISO8859_1.EncodeRune(r); ok { filenameBuffer = append(filenameBuffer, encodedRune) } diff --git a/pkg/storage/s3.go b/pkg/storage/s3.go index a59443629e5..a5a04250d67 100644 --- a/pkg/storage/s3.go +++ b/pkg/storage/s3.go @@ -4,6 +4,7 @@ import ( "context" "io" "path" + "strconv" "time" "github.com/aws/aws-sdk-go-v2/aws" @@ -116,9 +117,11 @@ func (s *S3) TempFileSystem() *afero.Afero { func (s *S3) PresignedURL(key string, contentType string, filename string) (string, error) { namespacedKey := path.Join(s.keyNamespace, key) presignClient := s3.NewPresignClient(s.client) + // Double quote the filename to be able to handle filenames with commas in them + quotedFilename := strconv.Quote(filename) filenameBuffer := make([]byte, 0) - for _, r := range filename { + for _, r := range quotedFilename { if encodedRune, ok := charmap.ISO8859_1.EncodeRune(r); ok { filenameBuffer = append(filenameBuffer, encodedRune) } diff --git a/pkg/storage/test/s3.go b/pkg/storage/test/s3.go index da076681dfe..97d06e7733d 100644 --- a/pkg/storage/test/s3.go +++ b/pkg/storage/test/s3.go @@ -4,6 +4,7 @@ import ( "fmt" "io" "net/url" + "strconv" "github.com/pkg/errors" "github.com/spf13/afero" @@ -58,7 +59,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) { filenameBuffer := make([]byte, 0) - for _, r := range filename { + + // Double quote the filename to be able to handle filenames with commas in them + quotedFilename := strconv.Quote(filename) + + for _, r := range quotedFilename { if encodedRune, ok := charmap.ISO8859_1.EncodeRune(r); ok { filenameBuffer = append(filenameBuffer, encodedRune) }