diff --git a/CHANGELOG.md b/CHANGELOG.md index 28cafa33c..2c1f16447 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/files/multifilereader.go b/files/multifilereader.go index af708dc7f..1a5d4ac1a 100644 --- a/files/multifilereader.go +++ b/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) diff --git a/files/multifilereader_binary_go119_test.go b/files/multifilereader_binary_go119_test.go new file mode 100644 index 000000000..c71514b61 --- /dev/null +++ b/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) +} diff --git a/files/multifilereader_binary_go120_test.go b/files/multifilereader_binary_go120_test.go new file mode 100644 index 000000000..bc3cf097d --- /dev/null +++ b/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) +} diff --git a/files/multifilereader_test.go b/files/multifilereader_test.go index e36788a91..b39217037 100644 --- a/files/multifilereader_test.go +++ b/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) diff --git a/files/multipartfile.go b/files/multipartfile.go index 27653982c..b5aab9620 100644 --- a/files/multipartfile.go +++ b/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 } }