fix!: add escaped abspath header (#434)

CHANGELOG.md

@@ -33,6 +33,12 @@ The following emojis are used to highlight certain changes:
 
 - HTTP Gateway API: Not having a block will result in a 5xx error rather than 404
 - HTTP Gateway API: CAR requests will return 200s and a CAR file proving a requested path does not exist rather than returning an error
+- 🛠 `MultiFileReader` has been updated with a new header with the encoded file name instead of the plain filename, due to a regression found in  [`net/textproto`](https://github.com/golang/go/issues/60674). This only affects files with binary characters in their name. By keeping the old header, we maximize backwards compatibility.
+  |            | New Client | Old Client  |
+  |------------|------------|-------------|
+  | New Server | ✅         | 🟡*         |
+  | Old Server | ✅         | ✅          |
+   *Old clients can only send Unicode file paths to the server.
 
 ### Security
 

files/multifilereader.go

@@ -29,19 +29,26 @@ type MultiFileReader struct {
 	// if true, the content disposition will be "form-data"
 	// if false, the content disposition will be "attachment"
 	form bool
+
+	// if true, 'abspath' header will be sent with raw (potentially binary) file
+	// name. This must only be used for legacy purposes to talk with old servers.
+	// if false, 'abspath-encoded' header will be sent with %-encoded filename
+	rawAbsPath bool
 }
 
 // NewMultiFileReader constructs a MultiFileReader. `file` can be any `commands.Directory`.
 // If `form` is set to true, the Content-Disposition will be "form-data".
-// Otherwise, it will be "attachment".
-func NewMultiFileReader(file Directory, form bool) *MultiFileReader {
+// Otherwise, it will be "attachment". If `rawAbsPath` is set to true, the
+// "abspath" header will be sent. Otherwise, the "abspath-encoded" header will be sent.
+func NewMultiFileReader(file Directory, form, rawAbsPath bool) *MultiFileReader {
 	it := file.Entries()
 
 	mfr := &MultiFileReader{
-		files: []DirIterator{it},
-		path:  []string{""},
-		form:  form,
-		mutex: &sync.Mutex{},
+		files:      []DirIterator{it},
+		path:       []string{""},
+		form:       form,
+		rawAbsPath: rawAbsPath,
+		mutex:      &sync.Mutex{},
 	}
 	mfr.mpWriter = multipart.NewWriter(&mfr.buf)
 
@@ -114,7 +121,12 @@ func (mfr *MultiFileReader) Read(buf []byte) (written int, err error) {
 
 			header.Set("Content-Type", contentType)
 			if rf, ok := entry.Node().(FileInfo); ok {
-				header.Set("abspath", rf.AbsPath())
+				if mfr.rawAbsPath {
+					// Legacy compatibility with old servers.
+					header.Set("abspath", rf.AbsPath())
+				} else {
+					header.Set("abspath-encoded", url.QueryEscape(rf.AbsPath()))
+				}
 			}
 
 			_, err := mfr.mpWriter.CreatePart(header)

files/multifilereader_binary_go119_test.go

@@ -0,0 +1,10 @@
+//go:build !go1.20
+
+package files
+
+import "testing"
+
+func TestAbspathHeaderWithBinaryFilenameSucceeds(t *testing.T) {
+	// Simulates old client talking to old server (< Go 1.20).
+	runMultiFileReaderToMultiFileTest(t, true, true, false)
+}

files/multifilereader_binary_go120_test.go

@@ -0,0 +1,13 @@
+//go:build go1.20
+
+package files
+
+import (
+	"testing"
+)
+
+func TestAbspathHeaderWithBinaryFilenameFails(t *testing.T) {
+	// Simulates old client talking to new server (>= Go 1.20). Old client will
+	// send the binary filename in the regular headers and the new server will error.
+	runMultiFileReaderToMultiFileTest(t, true, true, true)
+}

files/multifilereader_test.go

@@ -1,29 +1,48 @@
 package files
 
 import (
+	"bytes"
 	"io"
 	"mime/multipart"
 	"testing"
+
+	"github.com/stretchr/testify/require"
 )
 
 var text = "Some text! :)"
 
-func getTestMultiFileReader(t *testing.T) *MultiFileReader {
+func newBytesFileWithPath(abspath string, b []byte) File {
+	return &ReaderFile{abspath, bytesReaderCloser{bytes.NewReader(b)}, nil, int64(len(b))}
+}
+
+func makeMultiFileReader(t *testing.T, binaryFileName, rawAbsPath bool) (string, *MultiFileReader) {
+	var (
+		filename string
+		file     File
+	)
+
+	if binaryFileName {
+		filename = "bad\x7fname.txt"
+		file = newBytesFileWithPath("/my/path/boop/bad\x7fname.txt", []byte("bloop"))
+	} else {
+		filename = "résumé🥳.txt"
+		file = newBytesFileWithPath("/my/path/boop/résumé🥳.txt", []byte("bloop"))
+	}
+
 	sf := NewMapDirectory(map[string]Node{
-		"file.txt": NewBytesFile([]byte(text)),
+		"file.txt": newBytesFileWithPath("/my/path/file.txt", []byte(text)),
 		"boop": NewMapDirectory(map[string]Node{
-			"a.txt": NewBytesFile([]byte("bleep")),
-			"b.txt": NewBytesFile([]byte("bloop")),
+			"a.txt":  newBytesFileWithPath("/my/path/boop/a.txt", []byte("bleep")),
+			filename: file,
 		}),
-		"beep.txt": NewBytesFile([]byte("beep")),
+		"beep.txt": newBytesFileWithPath("/my/path/beep.txt", []byte("beep")),
 	})
 
-	// testing output by reading it with the go stdlib "mime/multipart" Reader
-	return NewMultiFileReader(sf, true)
+	return filename, NewMultiFileReader(sf, true, rawAbsPath)
 }
 
-func TestMultiFileReaderToMultiFile(t *testing.T) {
-	mfr := getTestMultiFileReader(t)
+func runMultiFileReaderToMultiFileTest(t *testing.T, binaryFileName, rawAbsPath, expectFailure bool) {
+	filename, mfr := makeMultiFileReader(t, binaryFileName, rawAbsPath)
 	mpReader := multipart.NewReader(mfr, mfr.Boundary())
 	mf, err := NewFileFromPartReader(mpReader, multipartFormdataType)
 	if err != nil {
@@ -32,40 +51,68 @@ func TestMultiFileReaderToMultiFile(t *testing.T) {
 
 	it := mf.Entries()
 
-	if !it.Next() || it.Name() != "beep.txt" {
-		t.Fatal("iterator didn't work as expected")
-	}
-
-	if !it.Next() || it.Name() != "boop" || DirFromEntry(it) == nil {
-		t.Fatal("iterator didn't work as expected")
-	}
+	require.True(t, it.Next())
+	require.Equal(t, "beep.txt", it.Name())
+	require.True(t, it.Next())
+	require.Equal(t, "boop", it.Name())
+	require.NotNil(t, DirFromEntry(it))
 
 	subIt := DirFromEntry(it).Entries()
+	require.True(t, subIt.Next(), subIt.Err())
+	require.Equal(t, "a.txt", subIt.Name())
+	require.Nil(t, DirFromEntry(subIt))
 
-	if !subIt.Next() || subIt.Name() != "a.txt" || DirFromEntry(subIt) != nil {
-		t.Fatal("iterator didn't work as expected")
-	}
+	if expectFailure {
+		require.False(t, subIt.Next())
+		require.Error(t, subIt.Err())
+	} else {
+		require.True(t, subIt.Next(), subIt.Err())
+		require.Equal(t, filename, subIt.Name())
+		require.Nil(t, DirFromEntry(subIt))
 
-	if !subIt.Next() || subIt.Name() != "b.txt" || DirFromEntry(subIt) != nil {
-		t.Fatal("iterator didn't work as expected")
-	}
+		require.False(t, subIt.Next())
+		require.Nil(t, it.Err())
 
-	if subIt.Next() || it.Err() != nil {
-		t.Fatal("iterator didn't work as expected")
-	}
+		// try to break internal state
+		require.False(t, subIt.Next())
+		require.Nil(t, it.Err())
 
-	// try to break internal state
-	if subIt.Next() || it.Err() != nil {
-		t.Fatal("iterator didn't work as expected")
-	}
+		require.True(t, it.Next())
+		require.Equal(t, "file.txt", it.Name())
+		require.Nil(t, DirFromEntry(it))
+		require.Nil(t, it.Err())
 
-	if !it.Next() || it.Name() != "file.txt" || DirFromEntry(it) != nil || it.Err() != nil {
-		t.Fatal("iterator didn't work as expected")
+		require.False(t, it.Next())
+		require.Nil(t, it.Err())
 	}
+}
 
-	if it.Next() || it.Err() != nil {
-		t.Fatal("iterator didn't work as expected")
-	}
+func TestMultiFileReaderToMultiFile(t *testing.T) {
+	t.Run("Header 'abspath' with unicode filename succeeds", func(t *testing.T) {
+		runMultiFileReaderToMultiFileTest(t, false, true, false)
+	})
+
+	t.Run("Header 'abspath-encoded' with unicode filename succeeds", func(t *testing.T) {
+		runMultiFileReaderToMultiFileTest(t, false, false, false)
+	})
+
+	t.Run("Header 'abspath-encoded' with binary filename succeeds", func(t *testing.T) {
+		runMultiFileReaderToMultiFileTest(t, true, false, false)
+	})
+}
+
+func getTestMultiFileReader(t *testing.T) *MultiFileReader {
+	sf := NewMapDirectory(map[string]Node{
+		"file.txt": NewBytesFile([]byte(text)),
+		"boop": NewMapDirectory(map[string]Node{
+			"a.txt": NewBytesFile([]byte("bleep")),
+			"b.txt": NewBytesFile([]byte("bloop")),
+		}),
+		"beep.txt": NewBytesFile([]byte("beep")),
+	})
+
+	// testing output by reading it with the go stdlib "mime/multipart" Reader
+	return NewMultiFileReader(sf, true, false)
 }
 
 func TestMultiFileReaderToMultiFileSkip(t *testing.T) {
@@ -164,7 +211,7 @@ func TestCommonPrefix(t *testing.T) {
 			"aaa": NewBytesFile([]byte("bleep")),
 		}),
 	})
-	mfr := NewMultiFileReader(sf, true)
+	mfr := NewMultiFileReader(sf, true, false)
 	reader, err := NewFileFromPartReader(multipart.NewReader(mfr, mfr.Boundary()), multipartFormdataType)
 	if err != nil {
 		t.Fatal(err)

files/multipartfile.go

@@ -100,9 +100,19 @@ func (w *multipartWalker) nextFile() (Node, error) {
 
 		return NewLinkFile(string(out), nil), nil
 	default:
+		var absPath string
+		if absPathEncoded := part.Header.Get("abspath-encoded"); absPathEncoded != "" {
+			absPath, err = url.QueryUnescape(absPathEncoded)
+			if err != nil {
+				return nil, err
+			}
+		} else {
+			absPath = part.Header.Get("abspath")
+		}
+
 		return &ReaderFile{
 			reader:  part,
-			abspath: part.Header.Get("abspath"),
+			abspath: absPath,
 		}, nil
 	}
 }