B-21937: Allow Doc Uploads/Downloads with Commas

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 {

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 {

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,14 +520,17 @@ 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)
 		suite.Equal(uploader.FileTypePDF, 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.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)
 			}

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

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