Skip to content

Commit

Permalink
fix!: add escaped abspath header (#434)
Browse files Browse the repository at this point in the history
  • Loading branch information
hacdias authored Aug 21, 2023
1 parent 7f075b1 commit aa7add0
Show file tree
Hide file tree
Showing 6 changed files with 141 additions and 43 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
26 changes: 19 additions & 7 deletions files/multifilereader.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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)
Expand Down
10 changes: 10 additions & 0 deletions files/multifilereader_binary_go119_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
13 changes: 13 additions & 0 deletions files/multifilereader_binary_go120_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
117 changes: 82 additions & 35 deletions files/multifilereader_test.go
Original file line number Diff line number Diff line change
@@ -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 {
Expand All @@ -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) {
Expand Down Expand Up @@ -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)
Expand Down
12 changes: 11 additions & 1 deletion files/multipartfile.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down

0 comments on commit aa7add0

Please sign in to comment.