fix!: query escape abspath header

CHANGELOG.md

@@ -34,6 +34,12 @@ The following emojis are used to highlight certain changes:
 - Address a Bitswap findpeers / connect race condition that can prevent peer communication ([#435](https://github.com/ipfs/boxo/issues/435))
 - 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_test.go

@@ -4,10 +4,100 @@ import (
 	"io"
 	"mime/multipart"
 	"testing"
+
+	"github.com/stretchr/testify/require"
 )
 
 var text = "Some text! :)"
 
+func TestMultiFileReaderToMultiFile(t *testing.T) {
+	do := func(t *testing.T, binaryFileName, rawAbsPath, expectFailure bool) {
+		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": NewBytesFileWithPath("/my/path/file.txt", []byte(text)),
+			"boop": NewMapDirectory(map[string]Node{
+				"a.txt":  NewBytesFileWithPath("/my/path/boop/a.txt", []byte("bleep")),
+				filename: file,
+			}),
+			"beep.txt": NewBytesFileWithPath("/my/path/beep.txt", []byte("beep")),
+		})
+
+		mfr := NewMultiFileReader(sf, true, rawAbsPath)
+		mpReader := multipart.NewReader(mfr, mfr.Boundary())
+		mf, err := NewFileFromPartReader(mpReader, multipartFormdataType)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		it := mf.Entries()
+
+		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 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))
+
+			require.False(t, subIt.Next())
+			require.Nil(t, it.Err())
+
+			// try to break internal state
+			require.False(t, subIt.Next())
+			require.Nil(t, it.Err())
+
+			require.True(t, it.Next())
+			require.Equal(t, "file.txt", it.Name())
+			require.Nil(t, DirFromEntry(it))
+			require.Nil(t, it.Err())
+
+			require.False(t, it.Next())
+			require.Nil(t, it.Err())
+		}
+	}
+
+	t.Run("Header 'abspath' with unicode filename succeeds", func(t *testing.T) {
+		do(t, false, true, false)
+	})
+
+	t.Run("Header 'abspath-encoded' with unicode filename succeeds", func(t *testing.T) {
+		do(t, false, false, false)
+	})
+
+	t.Run("Header 'abspath' with binary filename fails", func(t *testing.T) {
+		// Simulates old client talking to new server. Old client will send the
+		// binary filename in the regular headers and the new server will error.
+		do(t, true, true, true)
+	})
+
+	t.Run("Header 'abspath-encoded' with binary filename succeeds", func(t *testing.T) {
+		do(t, true, false, false)
+	})
+}
+
 func getTestMultiFileReader(t *testing.T) *MultiFileReader {
 	sf := NewMapDirectory(map[string]Node{
 		"file.txt": NewBytesFile([]byte(text)),
@@ -19,53 +109,7 @@ func getTestMultiFileReader(t *testing.T) *MultiFileReader {
 	})
 
 	// testing output by reading it with the go stdlib "mime/multipart" Reader
-	return NewMultiFileReader(sf, true)
-}
-
-func TestMultiFileReaderToMultiFile(t *testing.T) {
-	mfr := getTestMultiFileReader(t)
-	mpReader := multipart.NewReader(mfr, mfr.Boundary())
-	mf, err := NewFileFromPartReader(mpReader, multipartFormdataType)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	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")
-	}
-
-	subIt := DirFromEntry(it).Entries()
-
-	if !subIt.Next() || subIt.Name() != "a.txt" || DirFromEntry(subIt) != nil {
-		t.Fatal("iterator didn't work as expected")
-	}
-
-	if !subIt.Next() || subIt.Name() != "b.txt" || DirFromEntry(subIt) != nil {
-		t.Fatal("iterator didn't work as expected")
-	}
-
-	if subIt.Next() || it.Err() != nil {
-		t.Fatal("iterator didn't work as expected")
-	}
-
-	// try to break internal state
-	if subIt.Next() || it.Err() != nil {
-		t.Fatal("iterator didn't work as expected")
-	}
-
-	if !it.Next() || it.Name() != "file.txt" || DirFromEntry(it) != nil || it.Err() != nil {
-		t.Fatal("iterator didn't work as expected")
-	}
-
-	if it.Next() || it.Err() != nil {
-		t.Fatal("iterator didn't work as expected")
-	}
+	return NewMultiFileReader(sf, true, false)
 }
 
 func TestMultiFileReaderToMultiFileSkip(t *testing.T) {
@@ -164,7 +208,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
 	}
 }

files/readerfile.go

@@ -21,6 +21,10 @@ func NewBytesFile(b []byte) File {
 	return &ReaderFile{"", bytesReaderCloser{bytes.NewReader(b)}, nil, int64(len(b))}
 }
 
+func NewBytesFileWithPath(abspath string, b []byte) File {
+	return &ReaderFile{abspath, bytesReaderCloser{bytes.NewReader(b)}, nil, int64(len(b))}
+}
+
 // TODO: Is this the best way to fix this bug?
 // The bug is we want to be an io.ReadSeekCloser, but bytes.NewReader only gives a io.ReadSeeker and io.NopCloser
 // effectively removes the io.Seeker ability from the passed in io.Reader