diff --git a/CHANGELOG.md b/CHANGELOG.md index 12ec5116a..f20a65582 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,7 +32,11 @@ The following emojis are used to highlight certain changes: ### Fixed - Address a Bitswap findpeers / connect race condition that can prevent peer communication ([#435](https://github.com/ipfs/boxo/issues/435)) -- 🛠 `MultiFileReader` has been updated to escape the absolute paths due to the regression found in [`net/textproto`](https://github.com/golang/go/issues/60674). This is no longer compatible with previous versions when using files that have binary characters in their name. +- 🛠 `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 | ✅ | ✅ | ### Security diff --git a/files/multifilereader.go b/files/multifilereader.go index 4811ac155..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", url.QueryEscape(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_test.go b/files/multifilereader_test.go index e36788a91..a79ed8c54 100644 --- a/files/multifilereader_test.go +++ b/files/multifilereader_test.go @@ -4,11 +4,13 @@ import ( "io" "mime/multipart" "testing" + + "github.com/stretchr/testify/assert" ) var text = "Some text! :)" -func getTestMultiFileReader(t *testing.T) *MultiFileReader { +func getTestMultiFileReader(t *testing.T, rawAbsPath bool) *MultiFileReader { sf := NewMapDirectory(map[string]Node{ "file.txt": NewBytesFile([]byte(text)), "boop": NewMapDirectory(map[string]Node{ @@ -19,57 +21,62 @@ func getTestMultiFileReader(t *testing.T) *MultiFileReader { }) // testing output by reading it with the go stdlib "mime/multipart" Reader - return NewMultiFileReader(sf, true) + return NewMultiFileReader(sf, true, rawAbsPath) } 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) - } + do := func(t *testing.T, rawAbsPath bool) { + mfr := getTestMultiFileReader(t, rawAbsPath) + mpReader := multipart.NewReader(mfr, mfr.Boundary()) + mf, err := NewFileFromPartReader(mpReader, multipartFormdataType) + if err != nil { + t.Fatal(err) + } - it := mf.Entries() + it := mf.Entries() - if !it.Next() || it.Name() != "beep.txt" { - t.Fatal("iterator didn't work as expected") - } + assert.True(t, it.Next()) + assert.Equal(t, "beep.txt", it.Name()) + assert.True(t, it.Next()) + assert.Equal(t, "boop", it.Name()) + assert.NotNil(t, DirFromEntry(it)) - if !it.Next() || it.Name() != "boop" || DirFromEntry(it) == nil { - t.Fatal("iterator didn't work as expected") - } + subIt := DirFromEntry(it).Entries() + assert.True(t, subIt.Next()) + assert.Equal(t, "a.txt", subIt.Name()) + assert.Nil(t, DirFromEntry(subIt)) - subIt := DirFromEntry(it).Entries() + assert.True(t, subIt.Next()) + assert.Equal(t, "b.txt", subIt.Name()) + assert.Nil(t, DirFromEntry(subIt)) - if !subIt.Next() || subIt.Name() != "a.txt" || DirFromEntry(subIt) != nil { - t.Fatal("iterator didn't work as expected") - } + assert.False(t, subIt.Next()) + assert.Nil(t, it.Err()) - if !subIt.Next() || subIt.Name() != "b.txt" || DirFromEntry(subIt) != nil { - t.Fatal("iterator didn't work as expected") - } + // try to break internal state + assert.False(t, subIt.Next()) + assert.Nil(t, it.Err()) - if subIt.Next() || it.Err() != nil { - t.Fatal("iterator didn't work as expected") - } + assert.True(t, it.Next()) + assert.Equal(t, "file.txt", it.Name()) + assert.Nil(t, DirFromEntry(it)) + assert.Nil(t, it.Err()) - // try to break internal state - if subIt.Next() || it.Err() != nil { - t.Fatal("iterator didn't work as expected") + assert.False(t, it.Next()) + assert.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") - } + t.Run("With Header 'abspath'", func(t *testing.T) { + do(t, true) + }) - if it.Next() || it.Err() != nil { - t.Fatal("iterator didn't work as expected") - } + t.Run("With Header 'abspath-encoded'", func(t *testing.T) { + do(t, false) + }) } func TestMultiFileReaderToMultiFileSkip(t *testing.T) { - mfr := getTestMultiFileReader(t) + mfr := getTestMultiFileReader(t, false) mpReader := multipart.NewReader(mfr, mfr.Boundary()) mf, err := NewFileFromPartReader(mpReader, multipartFormdataType) if err != nil { @@ -96,7 +103,7 @@ func TestMultiFileReaderToMultiFileSkip(t *testing.T) { } func TestOutput(t *testing.T) { - mfr := getTestMultiFileReader(t) + mfr := getTestMultiFileReader(t, false) walker := &multipartWalker{reader: multipart.NewReader(mfr, mfr.Boundary())} buf := make([]byte, 20) @@ -164,7 +171,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 242182190..b5aab9620 100644 --- a/files/multipartfile.go +++ b/files/multipartfile.go @@ -100,14 +100,19 @@ func (w *multipartWalker) nextFile() (Node, error) { return NewLinkFile(string(out), nil), nil default: - abspath, err := url.QueryUnescape(part.Header.Get("abspath")) - if err != nil { - return nil, err + 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: abspath, + abspath: absPath, }, nil } }