From 719231c53b4329a1808572066360e20e5ba7d87b Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Tue, 15 Aug 2023 15:05:42 +0200 Subject: [PATCH 1/3] fix!: query escape abspath header --- CHANGELOG.md | 6 ++ files/multifilereader.go | 26 +++++-- files/multifilereader_test.go | 140 ++++++++++++++++++++++------------ files/multipartfile.go | 12 ++- files/readerfile.go | 4 + 5 files changed, 132 insertions(+), 56 deletions(-) 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_test.go b/files/multifilereader_test.go index e36788a91..1d6ac7e24 100644 --- a/files/multifilereader_test.go +++ b/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) 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 } } diff --git a/files/readerfile.go b/files/readerfile.go index bf3fa1c9e..524987e63 100644 --- a/files/readerfile.go +++ b/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 From bfffd833c6350b8599b1abcecdf958db99e1dbaf Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Mon, 21 Aug 2023 11:04:50 +0200 Subject: [PATCH 2/3] test: separate test cases per Go version --- files/multifilereader_binary_go119_test.go | 10 ++ files/multifilereader_binary_go120_test.go | 13 ++ files/multifilereader_test.go | 134 ++++++++++----------- 3 files changed, 89 insertions(+), 68 deletions(-) create mode 100644 files/multifilereader_binary_go119_test.go create mode 100644 files/multifilereader_binary_go120_test.go 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 1d6ac7e24..84b4ebe21 100644 --- a/files/multifilereader_test.go +++ b/files/multifilereader_test.go @@ -10,91 +10,89 @@ import ( 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() +func makeMultiFileReader(t *testing.T, binaryFileName, rawAbsPath bool) (string, *MultiFileReader) { + var ( + filename string + file File + ) - 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)) + 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")) + } - subIt := DirFromEntry(it).Entries() + 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")), + }) + + return filename, NewMultiFileReader(sf, true, rawAbsPath) +} + +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 { + 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, "a.txt", subIt.Name()) + require.Equal(t, filename, 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()) - 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()) - // 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.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()) - } + require.False(t, it.Next()) + require.Nil(t, it.Err()) } +} +func TestMultiFileReaderToMultiFile(t *testing.T) { t.Run("Header 'abspath' with unicode filename succeeds", func(t *testing.T) { - do(t, false, true, false) + runMultiFileReaderToMultiFileTest(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) + runMultiFileReaderToMultiFileTest(t, false, false, false) }) t.Run("Header 'abspath-encoded' with binary filename succeeds", func(t *testing.T) { - do(t, true, false, false) + runMultiFileReaderToMultiFileTest(t, true, false, false) }) } From dfc4949c84fdd574b6251d4672896327c9358c77 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Mon, 21 Aug 2023 14:44:21 +0200 Subject: [PATCH 3/3] fix: do not export newBytesFileWithPath --- files/multifilereader_test.go | 15 ++++++++++----- files/readerfile.go | 4 ---- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/files/multifilereader_test.go b/files/multifilereader_test.go index 84b4ebe21..b39217037 100644 --- a/files/multifilereader_test.go +++ b/files/multifilereader_test.go @@ -1,6 +1,7 @@ package files import ( + "bytes" "io" "mime/multipart" "testing" @@ -10,6 +11,10 @@ import ( var text = "Some text! :)" +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 @@ -18,19 +23,19 @@ func makeMultiFileReader(t *testing.T, binaryFileName, rawAbsPath bool) (string, if binaryFileName { filename = "bad\x7fname.txt" - file = NewBytesFileWithPath("/my/path/boop/bad\x7fname.txt", []byte("bloop")) + 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")) + file = newBytesFileWithPath("/my/path/boop/résumé🥳.txt", []byte("bloop")) } sf := NewMapDirectory(map[string]Node{ - "file.txt": NewBytesFileWithPath("/my/path/file.txt", []byte(text)), + "file.txt": newBytesFileWithPath("/my/path/file.txt", []byte(text)), "boop": NewMapDirectory(map[string]Node{ - "a.txt": NewBytesFileWithPath("/my/path/boop/a.txt", []byte("bleep")), + "a.txt": newBytesFileWithPath("/my/path/boop/a.txt", []byte("bleep")), filename: file, }), - "beep.txt": NewBytesFileWithPath("/my/path/beep.txt", []byte("beep")), + "beep.txt": newBytesFileWithPath("/my/path/beep.txt", []byte("beep")), }) return filename, NewMultiFileReader(sf, true, rawAbsPath) diff --git a/files/readerfile.go b/files/readerfile.go index 524987e63..bf3fa1c9e 100644 --- a/files/readerfile.go +++ b/files/readerfile.go @@ -21,10 +21,6 @@ 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