Add "no name" handling and storage.NewFileOpts

This came out of testing against Transmission in https://github.com/anacrolix/torrent/discussions/556#discussioncomment-1263670.

metainfo/info.go

@@ -23,14 +23,23 @@ type Info struct {
 	Files  []FileInfo `bencode:"files,omitempty"` // BEP3, mutually exclusive with Length
 }
 
-// This is a helper that sets Files and Pieces from a root path and its
-// children.
+// The Info.Name field is "advisory". For multi-file torrents it's usually a suggested directory
+// name. There are situations where we don't want a directory (like using the contents of a torrent
+// as the immediate contents of a directory), or the name is invalid. Transmission will inject the
+// name of the torrent file if it doesn't like the name, resulting in a different infohash
+// (https://github.com/transmission/transmission/issues/1775). To work around these situations, we
+// will use a sentinel name for compatibility with Transmission and to signal to our own client that
+// we intended to have no directory name. By exposing it in the API we can check for references to
+// this behaviour within this implementation.
+const NoName = "-"
+
+// This is a helper that sets Files and Pieces from a root path and its children.
 func (info *Info) BuildFromFilePath(root string) (err error) {
 	info.Name = func() string {
 		b := filepath.Base(root)
 		switch b {
 		case ".", "..", string(filepath.Separator):
-			return ""
+			return NoName
 		default:
 			return b
 		}

storage/file-deprecated.go

@@ -0,0 +1,34 @@
+package storage
+
+import (
+	"github.com/anacrolix/torrent/metainfo"
+)
+
+func NewFileWithCompletion(baseDir string, completion PieceCompletion) ClientImplCloser {
+	return NewFileWithCustomPathMakerAndCompletion(baseDir, nil, completion)
+}
+
+// File storage with data partitioned by infohash.
+func NewFileByInfoHash(baseDir string) ClientImplCloser {
+	return NewFileWithCustomPathMaker(baseDir, infoHashPathMaker)
+}
+
+// Deprecated: Allows passing a function to determine the path for storing torrent data. The
+// function is responsible for sanitizing the info if it uses some part of it (for example
+// sanitizing info.Name).
+func NewFileWithCustomPathMaker(baseDir string, pathMaker func(baseDir string, info *metainfo.Info, infoHash metainfo.Hash) string) ClientImplCloser {
+	return NewFileWithCustomPathMakerAndCompletion(baseDir, pathMaker, pieceCompletionForDir(baseDir))
+}
+
+// Deprecated: Allows passing custom PieceCompletion
+func NewFileWithCustomPathMakerAndCompletion(
+	baseDir string,
+	pathMaker TorrentDirFilePathMaker,
+	completion PieceCompletion,
+) ClientImplCloser {
+	return NewFileOpts(NewFileClientOpts{
+		ClientBaseDir:   baseDir,
+		TorrentDirMaker: pathMaker,
+		PieceCompletion: completion,
+	})
+}

storage/file-paths.go

@@ -0,0 +1,38 @@
+package storage
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+
+	"github.com/anacrolix/torrent/metainfo"
+)
+
+// Determines the filepath to be used for each file in a torrent.
+type FilePathMaker func(opts FilePathMakerOpts) string
+
+// Determines the directory for a given torrent within a storage client.
+type TorrentDirFilePathMaker func(baseDir string, info *metainfo.Info, infoHash metainfo.Hash) string
+
+// Info passed to a FilePathMaker.
+type FilePathMakerOpts struct {
+	Info *metainfo.Info
+	File *metainfo.FileInfo
+}
+
+// defaultPathMaker just returns the storage client's base directory.
+func defaultPathMaker(baseDir string, info *metainfo.Info, infoHash metainfo.Hash) string {
+	return baseDir
+}
+
+func infoHashPathMaker(baseDir string, info *metainfo.Info, infoHash metainfo.Hash) string {
+	return filepath.Join(baseDir, infoHash.HexString())
+}
+
+func isSubFilepath(base, sub string) bool {
+	rel, err := filepath.Rel(base, sub)
+	if err != nil {
+		return false
+	}
+	return rel != ".." && !strings.HasPrefix(rel, ".."+string(os.PathSeparator))
+}

storage/file.go

@@ -13,73 +13,63 @@ import (
 	"github.com/anacrolix/torrent/metainfo"
 )
 
-// File-based storage for torrents, that isn't yet bound to a particular
-// torrent.
+// File-based storage for torrents, that isn't yet bound to a particular torrent.
 type fileClientImpl struct {
-	baseDir   string
-	pathMaker func(baseDir string, info *metainfo.Info, infoHash metainfo.Hash) string
-	pc        PieceCompletion
+	opts NewFileClientOpts
 }
 
-// The Default path maker just returns the current path
-func defaultPathMaker(baseDir string, info *metainfo.Info, infoHash metainfo.Hash) string {
-	return baseDir
-}
-
-func infoHashPathMaker(baseDir string, info *metainfo.Info, infoHash metainfo.Hash) string {
-	return filepath.Join(baseDir, infoHash.HexString())
-}
-
-// All Torrent data stored in this baseDir
+// All Torrent data stored in this baseDir. The info names of each torrent are used as directories.
 func NewFile(baseDir string) ClientImplCloser {
 	return NewFileWithCompletion(baseDir, pieceCompletionForDir(baseDir))
 }
 
-func NewFileWithCompletion(baseDir string, completion PieceCompletion) *fileClientImpl {
-	return NewFileWithCustomPathMakerAndCompletion(baseDir, nil, completion)
+type NewFileClientOpts struct {
+	// The base directory for all downloads.
+	ClientBaseDir   string
+	FilePathMaker   FilePathMaker
+	TorrentDirMaker TorrentDirFilePathMaker
+	PieceCompletion PieceCompletion
 }
 
-// File storage with data partitioned by infohash.
-func NewFileByInfoHash(baseDir string) ClientImplCloser {
-	return NewFileWithCustomPathMaker(baseDir, infoHashPathMaker)
-}
-
-// Allows passing a function to determine the path for storing torrent data. The function is
-// responsible for sanitizing the info if it uses some part of it (for example sanitizing
-// info.Name).
-func NewFileWithCustomPathMaker(baseDir string, pathMaker func(baseDir string, info *metainfo.Info, infoHash metainfo.Hash) string) ClientImplCloser {
-	return NewFileWithCustomPathMakerAndCompletion(baseDir, pathMaker, pieceCompletionForDir(baseDir))
-}
-
-// Allows passing custom PieceCompletion
-func NewFileWithCustomPathMakerAndCompletion(baseDir string, pathMaker func(baseDir string, info *metainfo.Info, infoHash metainfo.Hash) string, completion PieceCompletion) *fileClientImpl {
-	if pathMaker == nil {
-		pathMaker = defaultPathMaker
+// NewFileOpts creates a new ClientImplCloser that stores files using the OS native filesystem.
+func NewFileOpts(opts NewFileClientOpts) ClientImplCloser {
+	if opts.TorrentDirMaker == nil {
+		opts.TorrentDirMaker = defaultPathMaker
 	}
-	return &fileClientImpl{
-		baseDir:   baseDir,
-		pathMaker: pathMaker,
-		pc:        completion,
+	if opts.FilePathMaker == nil {
+		opts.FilePathMaker = func(opts FilePathMakerOpts) string {
+			var parts []string
+			if opts.Info.Name != metainfo.NoName {
+				parts = append(parts, opts.Info.Name)
+			}
+			return filepath.Join(append(parts, opts.File.Path...)...)
+		}
 	}
+	if opts.PieceCompletion == nil {
+		opts.PieceCompletion = pieceCompletionForDir(opts.ClientBaseDir)
+	}
+	return fileClientImpl{opts}
 }
 
-func (me *fileClientImpl) Close() error {
-	return me.pc.Close()
+func (me fileClientImpl) Close() error {
+	return me.opts.PieceCompletion.Close()
 }
 
-func (fs *fileClientImpl) OpenTorrent(info *metainfo.Info, infoHash metainfo.Hash) (_ TorrentImpl, err error) {
-	dir := fs.pathMaker(fs.baseDir, info, infoHash)
+func (fs fileClientImpl) OpenTorrent(info *metainfo.Info, infoHash metainfo.Hash) (_ TorrentImpl, err error) {
+	dir := fs.opts.TorrentDirMaker(fs.opts.ClientBaseDir, info, infoHash)
 	upvertedFiles := info.UpvertedFiles()
 	files := make([]file, 0, len(upvertedFiles))
 	for i, fileInfo := range upvertedFiles {
-		var s string
-		s, err = ToSafeFilePath(append([]string{info.Name}, fileInfo.Path...)...)
-		if err != nil {
-			err = fmt.Errorf("file %v has unsafe path %q: %w", i, fileInfo.Path, err)
+		filePath := filepath.Join(dir, fs.opts.FilePathMaker(FilePathMakerOpts{
+			Info: info,
+			File: &fileInfo,
+		}))
+		if !isSubFilepath(dir, filePath) {
+			err = fmt.Errorf("file %v: path %q is not sub path of %q", i, filePath, dir)
 			return
 		}
 		f := file{
-			path:   filepath.Join(dir, s),
+			path:   filePath,
 			length: fileInfo.Length,
 		}
 		if f.length == 0 {
@@ -95,7 +85,7 @@ func (fs *fileClientImpl) OpenTorrent(info *metainfo.Info, infoHash metainfo.Has
 		files,
 		segments.NewIndex(common.LengthIterFromUpvertedFiles(upvertedFiles)),
 		infoHash,
-		fs.pc,
+		fs.opts.PieceCompletion,
 	}
 	return TorrentImpl{
 		Piece: t.Piece,

storage/mmap.go

@@ -22,6 +22,7 @@ type mmapClientImpl struct {
 	pc      PieceCompletion
 }
 
+// TODO: Support all the same native filepath configuration that NewFileOpts provides.
 func NewMMap(baseDir string) ClientImplCloser {
 	return NewMMapWithCompletion(baseDir, pieceCompletionForDir(baseDir))
 }

storage/safe-path_test.go

@@ -1,28 +1,38 @@
 package storage
 
 import (
+	"fmt"
 	"log"
 	"path/filepath"
 	"testing"
+
+	"github.com/anacrolix/torrent/metainfo"
+	qt "github.com/frankban/quicktest"
 )
 
 func init() {
 	log.SetFlags(log.Flags() | log.Lshortfile)
 }
 
-func TestSafePath(t *testing.T) {
-	for _, _case := range []struct {
-		input     []string
-		expected  string
-		expectErr bool
-	}{
-		{input: []string{"a", filepath.FromSlash(`b/../../..`)}, expectErr: true},
-		{input: []string{"a", filepath.FromSlash(`b/../.././..`)}, expectErr: true},
-		{input: []string{
-			filepath.FromSlash(`NewSuperHeroMovie-2019-English-720p.avi /../../../../../Roaming/Microsoft/Windows/Start Menu/Programs/Startup/test3.exe`)},
-			expectErr: true,
-		},
-	} {
+// I think these are mainly tests for bad metainfos that try to escape the client base directory.
+var safeFilePathTests = []struct {
+	input     []string
+	expectErr bool
+}{
+	// We might want a test for invalid chars inside components, or file maker opt funcs returning
+	// absolute paths (and thus presumably clobbering earlier "makers").
+	{input: []string{"a", filepath.FromSlash(`b/..`)}, expectErr: false},
+	{input: []string{"a", filepath.FromSlash(`b/../../..`)}, expectErr: true},
+	{input: []string{"a", filepath.FromSlash(`b/../.././..`)}, expectErr: true},
+	{input: []string{
+		filepath.FromSlash(`NewSuperHeroMovie-2019-English-720p.avi /../../../../../Roaming/Microsoft/Windows/Start Menu/Programs/Startup/test3.exe`)},
+		expectErr: true,
+	},
+}
+
+// Tests the ToSafeFilePath func.
+func TestToSafeFilePath(t *testing.T) {
+	for _, _case := range safeFilePathTests {
 		actual, err := ToSafeFilePath(_case.input...)
 		if _case.expectErr {
 			if err != nil {
@@ -32,3 +42,27 @@ func TestSafePath(t *testing.T) {
 		}
 	}
 }
+
+// Check that safe file path handling still exists for the newer file-opt-maker variants.
+func TestFileOptsSafeFilePathHandling(t *testing.T) {
+	c := qt.New(t)
+	for i, _case := range safeFilePathTests {
+		c.Run(fmt.Sprintf("Case%v", i), func(c *qt.C) {
+			info := metainfo.Info{
+				Files: []metainfo.FileInfo{
+					{Path: _case.input},
+				},
+			}
+			client := NewFileOpts(NewFileClientOpts{
+				ClientBaseDir: "somedir",
+			})
+			defer func() { c.Check(client.Close(), qt.IsNil) }()
+			torImpl, err := client.OpenTorrent(&info, metainfo.Hash{})
+			if _case.expectErr {
+				c.Check(err, qt.Not(qt.IsNil))
+			} else {
+				c.Check(torImpl.Close(), qt.IsNil)
+			}
+		})
+	}
+}