From 23c88c8b9b357207c3c3c0b0b16decee7c1a3d65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= Date: Tue, 8 Sep 2026 21:22:43 +0200 Subject: [PATCH] [client] Report a partial file drop delivery instead of losing track of it Delivery moved payloads one at a time and returned on the first failure, and the caller discarded the paths it returned along with the error. Files that had already been moved stayed in the destination while the history said the transfer failed and recorded no path for them, so the user was told nothing arrived and had nothing to go looking with. Every item is attempted now, the error names the ones that did not make it, and the paths that did land are kept on the entry. A transfer settled elsewhere still keeps the outcome it has, which is what finishTransfer gave. Delivered names also drop the characters that change how the rest of the name renders - a U+202E override makes "gpj.exe" display as though it ended in ".jpg" - and are held to one filesystem entry with the extension preserved. An over-long name used to be refused by the OS, which is the likeliest way to reach the partial delivery above. --- client/internal/filedrop/delivery.go | 64 ++++++++++++++++++- client/internal/filedrop/filedrop_test.go | 77 +++++++++++++++++++++++ client/internal/filedrop/manager.go | 11 +++- 3 files changed, 149 insertions(+), 3 deletions(-) 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 }