Fix/disallow filename characters (#376)

dev_utils/README.md

@@ -51,10 +51,9 @@ mc admin trace -v proxydev
 Run the go proxy from the root directory
 
 ```bash
-export GO111MODULE=on
 export SERVER_CONFFILE=dev_utils/config.yaml
-go build main.go
-./main
+go build .
+./S3-Upload-Proxy
 ```
 
 

helper/helper.go

@@ -8,11 +8,14 @@ import (
 	"crypto/x509"
 	"crypto/x509/pkix"
 	"encoding/pem"
+	"fmt"
 	"log"
 	"math/big"
 	"net"
 	"os"
 	"path/filepath"
+	"regexp"
+	"strings"
 	"time"
 
 	"github.com/golang-jwt/jwt/v4"
@@ -363,3 +366,27 @@ func TLScertToFile(filename string, derBytes []byte) error {
 
 	return err
 }
+
+// FormatUploadFilePath ensures that path separators are "/", and returns error if the
+// filepath contains a disallowed character matched with regex
+func FormatUploadFilePath(filePath string) (string, error) {
+
+	// Check for mixed "\" and "/" in filepath. Stop and throw an error if true so that
+	// we do not end up with unintended folder structure when applying ReplaceAll below
+	if strings.Contains(filePath, "\\") && strings.Contains(filePath, "/") {
+		return filePath, fmt.Errorf("filepath contains mixed '\\' and '/' characters")
+	}
+
+	// make any windows path separators linux compatible
+	outPath := strings.ReplaceAll(filePath, "\\", "/")
+
+	// [\x00-\x1F\x7F] is the control character set
+	re := regexp.MustCompile(`[\\:\*\?"<>\|\x00-\x1F\x7F]`)
+
+	dissallowedChars := re.FindAllString(outPath, -1)
+	if dissallowedChars != nil {
+		return outPath, fmt.Errorf("filepath contains disallowed characters: %+v", strings.Join(dissallowedChars, ", "))
+	}
+
+	return outPath, nil
+}

helper/helper_test.go

@@ -70,3 +70,23 @@ func TestCreateECToken(t *testing.T) {
 
 	defer os.RemoveAll("dummy-folder")
 }
+
+func TestFormatUploadFilePath(t *testing.T) {
+
+	unixPath := "a/b/c.c4gh"
+	testPath := "a\\b\\c.c4gh"
+	uploadPath, err := FormatUploadFilePath(testPath)
+	assert.NoError(t, err)
+	assert.Equal(t, unixPath, uploadPath)
+
+	// mixed "\" and "/"
+	weirdPath := `dq\sw:*?"<>|\t\s/df.c4gh`
+	_, err = FormatUploadFilePath(weirdPath)
+	assert.EqualError(t, err, "filepath contains mixed '\\' and '/' characters")
+
+	// no mixed "\" and "/" but not allowed
+	weirdPath = `dq\sw:*?"<>|\t\sdf.c4gh`
+	_, err = FormatUploadFilePath(weirdPath)
+	assert.EqualError(t, err, "filepath contains disallowed characters: :, *, ?, \", <, >, |")
+
+}

proxy.go

@@ -14,6 +14,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/NBISweden/S3-Upload-Proxy/helper"
 	common "github.com/neicnordic/sda-common/database"
 
 	"github.com/aws/aws-sdk-go/aws"
@@ -77,19 +78,18 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 }
 
 func (p *Proxy) internalServerError(w http.ResponseWriter, r *http.Request) {
-	log.Debug("internal server error")
 	log.Debugf("Internal server error for request (%v)", r)
-	w.WriteHeader(500)
+	w.WriteHeader(http.StatusInternalServerError)
 }
 
 func (p *Proxy) notAllowedResponse(w http.ResponseWriter, _ *http.Request) {
 	log.Debug("not allowed response")
-	w.WriteHeader(403)
+	w.WriteHeader(http.StatusForbidden)
 }
 
 func (p *Proxy) notAuthorized(w http.ResponseWriter, _ *http.Request) {
 	log.Debug("not authorized")
-	w.WriteHeader(401) // Actually correct!
+	w.WriteHeader(http.StatusUnauthorized)
 }
 
 func (p *Proxy) allowedResponse(w http.ResponseWriter, r *http.Request) {
@@ -105,7 +105,16 @@ func (p *Proxy) allowedResponse(w http.ResponseWriter, r *http.Request) {
 	p.prependBucketToHostPath(r)
 
 	username := fmt.Sprintf("%v", claims["sub"])
-	filepath := strings.Replace(r.URL.Path, "/"+p.s3.bucket+"/", "", 1)
+	rawFilepath := strings.Replace(r.URL.Path, "/"+p.s3.bucket+"/", "", 1)
+
+	filepath, err := helper.FormatUploadFilePath(rawFilepath)
+	if err != nil {
+		log.Debugf(err.Error())
+		w.WriteHeader(http.StatusNotAcceptable)
+
+		return
+	}
+
 	// register file in database if it's the start of an upload
 	if p.detectRequestType(r) == Put && p.fileIds[r.URL.Path] == "" {
 		log.Debugf("registering file %v in the database", r.URL.Path)
@@ -409,7 +418,6 @@ func (p *Proxy) requestInfo(fullPath string) (string, int64, error) {
 
 		return "", 0, err
 	}
-	fmt.Println(strings.ReplaceAll(*result.Contents[0].ETag, "\"", ""))
 
 	return fmt.Sprintf("%x", sha256.Sum256([]byte(strings.ReplaceAll(*result.Contents[0].ETag, "\"", "")))), *result.Contents[0].Size, nil
 

proxy_test.go

@@ -310,6 +310,16 @@ func (suite *ProxyTests) TestServeHTTP_allowed() {
 	proxy.ServeHTTP(w, r)
 	assert.Equal(suite.T(), 200, w.Result().StatusCode)
 	assert.Equal(suite.T(), true, suite.fakeServer.PingedAndRestore())
+
+	// Filenames with platform incompatible characters are disallowed
+	// not checked in TestServeHTTP_allowed() because we look for a non 403 response
+	r.Method = "PUT"
+	r.URL, _ = url.Parse("/username/fi|le")
+	w = httptest.NewRecorder()
+	proxy.ServeHTTP(w, r)
+	assert.Equal(suite.T(), 406, w.Result().StatusCode)
+	assert.Equal(suite.T(), false, suite.fakeServer.PingedAndRestore())
+
 }
 
 func (suite *ProxyTests) TestMessageFormatting() {