diff --git a/client/internal/filedrop/delivery.go b/client/internal/filedrop/delivery.go index 6415f0d41..2c76bbbaf 100644 --- a/client/internal/filedrop/delivery.go +++ b/client/internal/filedrop/delivery.go @@ -6,10 +6,16 @@ import ( "os" "path/filepath" "strings" + "unicode/utf8" log "github.com/sirupsen/logrus" ) +// maxDeliveredNameBytes bounds a delivered filename. Common filesystems stop at +// 255 bytes per entry, and the room left over takes the " (N)" a name collision +// appends without pushing the result back over the limit. +const maxDeliveredNameBytes = 240 + func deliver(spool *Spool, offer Offer, destDir string) ([]string, error) { files := false for _, f := range offer.Files { @@ -31,7 +37,12 @@ func deliver(spool *Spool, offer Offer, destDir string) ([]string, error) { return nil, fmt.Errorf("create destination dir: %w", err) } + // Every item is attempted, and the ones that landed are reported alongside + // the failure: stopping at the first error left earlier files sitting in the + // destination while the caller was told the whole delivery had failed, with + // no path recorded for them. var delivered []string + var failed []string for i, f := range offer.Files { if f.Kind == KindText { continue @@ -39,7 +50,9 @@ func deliver(spool *Spool, offer Offer, destDir string) ([]string, error) { dest, err := moveToUniqueName(spool.Path(offer.ID, i), destDir, sanitizeFileName(f.Name, i)) if err != nil { - return delivered, fmt.Errorf("deliver %s: %w", f.Name, err) + log.Warnf("failed to deliver %s of offer %s: %v", f.Name, offer.ID, err) + failed = append(failed, f.Name) + continue } if err := chownToDirOwner(dest, destDir); err != nil { log.Debugf("failed to adopt owner for %s: %v", dest, err) @@ -48,15 +61,62 @@ func deliver(spool *Spool, offer Offer, destDir string) ([]string, error) { } spool.removeLocked(offer.ID) + + if len(failed) > 0 { + return delivered, fmt.Errorf("deliver %s", strings.Join(failed, ", ")) + } return delivered, nil } func sanitizeFileName(name string, index int) string { + name = stripNameControls(name) name = filepath.Base(filepath.Clean(strings.ReplaceAll(name, "\\", "/"))) if name == "" || name == "." || name == ".." || name == string(filepath.Separator) { return fmt.Sprintf("file-%d", index) } - return name + return truncateNameBytes(name, maxDeliveredNameBytes) +} + +// stripNameControls drops the characters that change how the rest of the name +// renders rather than what it addresses. A name ending in "gpj.exe" preceded by +// U+202E displays in a file manager as though it ended in ".jpg", and the C0 +// controls have no business in a filename either. +func stripNameControls(name string) string { + return strings.Map(func(r rune) rune { + switch { + case r < 0x20, r == 0x7f: + return -1 + case r >= 0x202a && r <= 0x202e, r >= 0x2066 && r <= 0x2069: + return -1 + case r == 0x200e, r == 0x200f, r == 0x061c: + return -1 + default: + return r + } + }, name) +} + +// truncateNameBytes keeps a name within one filesystem entry, preserving the +// extension so the delivered file still opens with the right application. A +// name over the limit is refused by the OS outright, which used to fail the +// whole delivery. +func truncateNameBytes(name string, limit int) string { + if len(name) <= limit { + return name + } + + ext := filepath.Ext(name) + if len(ext) > limit/2 { + ext = "" + } + stem := name[:len(name)-len(ext)] + + room := limit - len(ext) + for len(stem) > room { + _, size := utf8.DecodeLastRuneInString(stem) + stem = stem[:len(stem)-size] + } + return stem + ext } func moveToUniqueName(src, dir, name string) (string, error) { diff --git a/client/internal/filedrop/filedrop_test.go b/client/internal/filedrop/filedrop_test.go index 87228f13f..182e2751f 100644 --- a/client/internal/filedrop/filedrop_test.go +++ b/client/internal/filedrop/filedrop_test.go @@ -14,6 +14,7 @@ import ( "sync" "testing" "time" + "unicode/utf8" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -782,6 +783,82 @@ func TestSpoolCleanupDropsStalePartials(t *testing.T) { assert.NoError(t, err, "a recent offer dir must survive") } +func TestSanitizeFileNameStripsRenderingControls(t *testing.T) { + // U+202E makes a file manager render the rest of the name reversed, so + // "gpj.exe" reads as though it ended in ".jpg". + got := sanitizeFileName("\u202egpj.exe", 0) + assert.Equal(t, "gpj.exe", got, "a bidi override must not survive into the delivered name") + assert.NotContains(t, got, "\u202e") + + assert.Equal(t, "report.pdf", sanitizeFileName("re\u200eport.pdf", 0), + "a bidi mark must be dropped without eating the rest of the name") + assert.Equal(t, "notes.txt", sanitizeFileName("no\x00tes.txt", 0), + "a NUL must be dropped rather than refused by the filesystem") + + assert.Equal(t, "file-3", sanitizeFileName("\u202e\u2066", 3), + "a name that is nothing but controls falls back to the index") +} + +func TestSanitizeFileNameBoundsTheNameLength(t *testing.T) { + long := strings.Repeat("a", 400) + ".txt" + got := sanitizeFileName(long, 0) + + assert.LessOrEqual(t, len(got), maxDeliveredNameBytes, "the name must fit a filesystem entry") + assert.True(t, strings.HasSuffix(got, ".txt"), "the extension must survive truncation") + + // A multibyte stem must not be cut through a rune. + multi := strings.Repeat("é", 300) + ".txt" + got = sanitizeFileName(multi, 0) + assert.LessOrEqual(t, len(got), maxDeliveredNameBytes) + assert.True(t, utf8.ValidString(got), "truncation must leave valid UTF-8") + + assert.Equal(t, "short.txt", sanitizeFileName("short.txt", 0), "a normal name is untouched") +} + +func TestDeliverReportsWhatLandedWhenOneItemFails(t *testing.T) { + spool, err := NewSpool(t.TempDir()) + require.NoError(t, err) + destDir := t.TempDir() + + store := NewOfferStore(time.Minute) + files := []FileMeta{ + {Name: "first.txt", Size: 4}, + {Name: "second.txt", Size: 4}, + {Name: "third.txt", Size: 4}, + } + offer := store.Add(testPeer, "sender", files, DecisionAccepted) + require.NoError(t, spool.Prepare(offer.ID)) + + // Stage every item but the middle one, which is the state a failed write + // leaves behind. + for _, i := range []int{0, 2} { + _, err = spool.Write(offer.ID, i, "", 0, strings.NewReader("data"), 4) + require.NoError(t, err) + } + + full, ok := store.Get(testPeer, offer.ID) + require.True(t, ok) + + delivered, derr := spool.Deliver(full, destDir) + require.Error(t, derr, "the missing item must be reported") + assert.Contains(t, derr.Error(), "second.txt", "the error must name what failed") + + require.Len(t, delivered, 2, "the items that landed must be reported, not discarded") + for _, p := range delivered { + _, serr := os.Stat(p) + require.NoErrorf(t, serr, "a reported path must exist: %s", p) + } + + names := map[string]bool{} + entries, err := os.ReadDir(destDir) + require.NoError(t, err) + for _, e := range entries { + names[e.Name()] = true + } + assert.True(t, names["first.txt"], "the item before the failure must be delivered") + assert.True(t, names["third.txt"], "the item after the failure must still be attempted") +} + func TestParseOfferPath(t *testing.T) { tests := []struct { path string diff --git a/client/internal/filedrop/manager.go b/client/internal/filedrop/manager.go index 6315aac96..30908b017 100644 --- a/client/internal/filedrop/manager.go +++ b/client/internal/filedrop/manager.go @@ -606,7 +606,16 @@ func (m *Manager) OnCompleted(offer Offer) { // Nothing will ask for these payloads again, and a sink that cannot // address them by path has no sweep of its own to fall back on. server.Spool().Remove(offer.ID) - m.finishTransfer(offer.ID, StateFailed, err.Error()) + // Whatever did land is kept on the entry: those files are in the + // destination whether the rest arrived or not, and a failure with no + // paths would leave the user unable to find them. A transfer already + // settled elsewhere keeps the outcome it has, as finishTransfer would. + if !transfer.terminal() { + transfer.State = StateFailed + transfer.Error = err.Error() + transfer.DeliveredPaths = delivered + m.history.Upsert(transfer) + } m.emit(EventFailed, m.transferOf(offer.ID)) return }