fix(attachments): drop Bidi_Control characters from attachment names and rewrite legacy rows (BUG-3153) (#1452)

* fix(attachments): drop Bidi_Control characters from attachment names and rewrite legacy rows (BUG-3153)

A name such as "x<U+202E>gvs.txt" is a .txt that every bidi-aware renderer
shows as "xtxt.svg". About forty web read sites, the CLI list and the server
render the stored name, so the fix is at the source rather than per renderer.

DroppedFilenameRune now also drops the Unicode Bidi_Control property: U+202A to
U+202E, U+2066 to U+2069, and the marks U+200E, U+200F and U+061C. It is chosen
by property, so ZWJ (Join_Control) and emoji sequences are untouched. The rule
is shared, so both ingest doors store the name clean and the download header
drops the characters for any row. One of these characters inside an extension
also hid that extension from the blocklist, as a control byte did: an SVG
uploaded as "x.s<U+202E>vg" was accepted, and is now refused as ".svg".

Legacy rows are rewritten once, at startup, by
BackfillBidiAttachmentFilenames, and only rows containing a Bidi_Control
character are touched. The new name is the SERVED name (ServedFilename), not a
plain strip, so a legacy "x.s<U+202E>vg" becomes "x.bin" rather than storing the
blocked "x.svg". The pass is idempotent with no marker, since a rewritten row no
longer matches. Each UPDATE is conditional on the old name. Every rewrite is
logged with its old and new names quoted to ASCII, so the rewrite is reversible,
and the count is logged on every boot, 0 included.

Claude-Session: https://claude.ai/code/session_01Vf8uZmYdsrQqJtbAmjUFSX

* fix(store): the bidi filename backfill states its wider rewrite and is tested per rune on both dialects (BUG-3153, codex round 1)

- The rewrite is the served name, so a selected row also loses whatever else
  NormalizeFilename removes from a legacy name. That is now said in the
  comment and pinned by a case, and rows without a Bidi_Control are still
  never touched.
- "Reversible by the log" now says: for as long as the log is retained, with
  no durable record in the database.
- A row that gains such a name later (derived from a parent a failed pass
  left) is picked up by the next boot, since selection is by content.
- New test: one row per member of unicode.Bidi_Control, each selected by the
  SQL and rewritten. It enumerates the property itself rather than the list
  under test, so a rune missing from the LIKE list fails it.

Claude-Session: https://claude.ai/code/session_01Vf8uZmYdsrQqJtbAmjUFSX

* fix(store): a rows-affected error stops the bidi backfill instead of passing as a concurrent rename (BUG-3153, codex round 2)

The UPDATE may have landed, and without a count the row could not be logged as
rewritten, which the reversibility rests on. The comment also names the
readers it means, instead of an unmeasured count.

Claude-Session: https://claude.ai/code/session_01Vf8uZmYdsrQqJtbAmjUFSX

* docs(attachments): NormalizeFilename covers the caller-supplied doors, and derived rows inherit (BUG-3153, codex round 3)

"Every door that writes attachments.filename calls it" was not true of
thumbnails, transforms or copies, which derive their name from another row.

Claude-Session: https://claude.ai/code/session_01Vf8uZmYdsrQqJtbAmjUFSX

* test(e2e): the hostile-name parity spec expects the stored name, with Bidi_Control dropped (BUG-3153)

The spec uploads "start <U+202E>reversed<U+202C> مرحبا.png" and asserted that
the tile and dialog carry it exactly as uploaded. Ingest now drops the
override. The spec still uploads the hostile name, and now asserts that the
STORED name (RTL letters kept, controls gone) is addressable and names its
dialog. Its fixture now uses \u escapes instead of literal controls. CI caught
this on #1452; the local sweep had not searched web/e2e.

Against this tree's binary the spec passes, and against a binary without the
fix it fails, so the spec discriminates.

Claude-Session: https://claude.ai/code/session_01Vf8uZmYdsrQqJtbAmjUFSX

cmd/pad/cmd_server.go

@@ -209,6 +209,19 @@ func serveCmd() *cobra.Command {
 				)
 			}
 
+			// Rewrite attachment names stored with Bidi_Control characters
+			// (BUG-3153), which display an extension the file does not have.
+			// Each rewrite is logged with its old name; the count is logged
+			// on every boot, 0 included, so an instance's exposure is visible.
+			// Non-fatal: an unrewritten row still downloads under its served
+			// name, which drops the characters.
+			if bb, err := s.BackfillBidiAttachmentFilenames(); err != nil {
+				slog.Warn("attachment bidi filename backfill failed; non-fatal", "error", err,
+					"rows_rewritten_before_error", bb.RowsRewritten)
+			} else {
+				slog.Info("Attachment bidi filename backfill complete", "rows_rewritten", bb.RowsRewritten)
+			}
+
 			// Backfill: populate status_transitions from the historical
 			// activity log (PLAN-1628 / TASK-1637). Idempotent — gated on an
 			// empty table, so it replays history exactly once on the first

internal/attachments/filename.go

@@ -31,16 +31,27 @@ import (
 //     and the header cannot carry them.
 //   - '"' and '\': they break the header's quoted-string, so the header has to
 //     drop them, and anything it drops the blocklist must not have seen.
+//   - The Unicode Bidi_Control property (BUG-3153): the embeddings, overrides
+//     and isolates (U+202A-U+202E, U+2066-U+2069) and the implicit marks
+//     (U+200E, U+200F, U+061C). They make a bidi-aware renderer DISPLAY an
+//     extension the name does not have, so "x<U+202E>gvs.txt" (a .txt) shows as
+//     "xtxt.svg". Chosen by property rather than by range so the set is the
+//     standard's; ZWJ is Join_Control, not Bidi_Control, and emoji sequences
+//     keep it. One inside an extension also hides it from the blocklist, as a
+//     control byte did: "x.s<U+202E>vg" normalises to "x.svg" and is refused.
 func DroppedFilenameRune(r rune) bool {
-	return unicode.IsControl(r) || r == '"' || r == '\\'
+	return unicode.IsControl(r) || r == '"' || r == '\\' || unicode.Is(unicode.Bidi_Control, r)
 }
 
 // fallbackFilename is the name for an upload whose own name leaves nothing.
 const fallbackFilename = "upload.bin"
 
 // NormalizeFilename is the stored form of a caller-supplied attachment name.
-// Every door that writes attachments.filename calls it, before ValidateUpload
-// sees the name. It normalises rather than refuses: the bytes of the upload are
+// Every door that writes a CALLER-SUPPLIED name calls it (upload, bundle
+// import), before ValidateUpload sees the name. Derived rows (thumbnails,
+// transforms, cross-workspace copies) take their name from a row that already
+// went through it; a legacy row that did not is rewritten at startup when it
+// carries a Bidi_Control character (store.BackfillBidiAttachmentFilenames). It normalises rather than refuses: the bytes of the upload are
 // fine and only the label is at issue, and an attack name still ends in a
 // refusal, because its normalised form carries the extension it was hiding
 // (checkpoint 1 on BUG-2818 has the full reasoning).

internal/attachments/filename_test.go

@@ -25,6 +25,20 @@ func TestNormalizeFilename(t *testing.T) {
 		{"x.s\u0085vg", "x.svg"}, // C1: a control character too
 		{"x.s\x7fvg", "x.svg"},
 		{`x.s"vg`, "x.svg"},
+		// BUG-3153: every Bidi_Control is dropped. The spoof itself — a .txt
+		// that a bidi-aware renderer shows as "xtxt.svg" — keeps its letters
+		// and its real extension.
+		{"x\u202Egvs.txt", "xgvs.txt"},
+		{"x\u202Agvs\u202C.txt", "xgvs.txt"},
+		{"x\u2066a\u2067b\u2068c\u2069.txt", "xabc.txt"},
+		{"a\u200Eb\u200Fc\u061Cd.txt", "abcd.txt"},
+		// One inside the extension hid it from the blocklist, as a control
+		// byte did.
+		{"x.s\u202Evg", "x.svg"},
+		// ZWJ is not Bidi_Control: an emoji sequence survives intact.
+		{"\U0001F469\u200D\U0001F4BB notes.txt", "\U0001F469\u200D\U0001F4BB notes.txt"},
+		// Nor are other format characters touched (U+00AD soft hyphen, Cf).
+		{"co\u00ADop.txt", "co\u00ADop.txt"},
 		// Trailing dots and spaces, which a download save strips (Q1).
 		{"x.svg.", "x.svg"},
 		{"x.svg ", "x.svg"},

internal/server/handlers_attachments.go

@@ -894,10 +894,11 @@ func isKnownVariant(v string) bool {
 }
 
 // sanitizeHeaderFilename strips characters that can't safely appear in
-// a Content-Disposition filename header: quotes, backslashes and control
-// characters, by attachments.DroppedFilenameRune, which is also the rule
-// ingest normalises with (BUG-2818). The caller passes the SERVED name
-// (attachments.ServedFilename), so for any row stored since that fix this
+// a Content-Disposition filename header: quotes, backslashes, control
+// characters and Bidi_Control characters (BUG-3153), by
+// attachments.DroppedFilenameRune, which is also the rule ingest normalises
+// with (BUG-2818). The caller passes the SERVED name
+// (attachments.ServedFilename), which already applies that rule, so this
 // drops nothing; it still runs because a header value must be parseable
 // whatever reaches it.
 func sanitizeHeaderFilename(name string) string {

internal/server/handlers_attachments_bug3153_test.go

@@ -0,0 +1,72 @@
+package server
+
+import (
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+)
+
+// BUG-3153 at the doors. The character rule itself is pinned in
+// internal/attachments (TestNormalizeFilename); these show the upload door
+// and the download header apply it. The import door calls the same
+// NormalizeFilename, and BUG-2818's bundle test shows it does.
+
+// U+202E RIGHT-TO-LEFT OVERRIDE, percent-encoded as UTF-8 for the multipart
+// filename parameter.
+const rloEncoded = "%E2%80%AE"
+
+// The spoof is stored without its override: "x<RLO>gvs.txt" renders as
+// "xtxt.svg" in a bidi-aware UI; stored, it is the .txt it always was.
+func TestUpload_BUG3153_BidiOverrideIsStrippedNotRefused(t *testing.T) {
+	srv, slug := testServerWithAttachments(t)
+	rr := uploadEncodedName(t, srv, slug, "x"+rloEncoded+"gvs.txt", []byte("plain text\n"))
+	if rr.Code != http.StatusCreated {
+		t.Fatalf("status=%d body=%s", rr.Code, rr.Body.String())
+	}
+	var got struct {
+		Filename string `json:"filename"`
+	}
+	if err := json.Unmarshal(rr.Body.Bytes(), &got); err != nil {
+		t.Fatalf("decode: %v", err)
+	}
+	if got.Filename != "xgvs.txt" {
+		t.Errorf("stored filename = %q, want %q", got.Filename, "xgvs.txt")
+	}
+}
+
+// An override INSIDE the extension hid it from the blocklist, which judged
+// ".s<RLO>vg". The normalised name carries ".svg" and is refused.
+func TestUpload_BUG3153_OverrideHidingABlockedExtensionIsRefused(t *testing.T) {
+	srv, slug := testServerWithAttachments(t)
+	rr := uploadEncodedName(t, srv, slug, "x.s"+rloEncoded+"vg", svgScript)
+	if rr.Code != http.StatusUnsupportedMediaType || !strings.Contains(rr.Body.String(), "extension_blocked") {
+		t.Errorf("status=%d body=%s; want 415 extension_blocked", rr.Code, rr.Body.String())
+	}
+}
+
+// A LEGACY row is served without the characters, and one hiding a blocked
+// extension is served as .bin, before the startup backfill has rewritten it.
+func TestDownload_BUG3153_LegacyBidiRowIsServedClean(t *testing.T) {
+	srv, slug := testServerWithAttachments(t)
+	for _, c := range []struct{ legacy, want string }{
+		{"x\u202Egvs.txt", "xgvs.txt"},
+		{"x.s\u202Evg", "x.bin"},
+	} {
+		id, _, urlPath := uploadHelper(t, srv, slug, "legacy.xml", svgScript)
+		if _, err := srv.store.DB().Exec(`UPDATE attachments SET filename = ? WHERE id = ?`, c.legacy, id); err != nil {
+			t.Fatalf("plant legacy name %q: %v", c.legacy, err)
+		}
+		req := httptest.NewRequest("GET", urlPath, nil)
+		req.RemoteAddr = "127.0.0.1:1234"
+		rr := httptest.NewRecorder()
+		srv.ServeHTTP(rr, req)
+		if rr.Code != http.StatusOK {
+			t.Fatalf("%q: status=%d body=%s", c.legacy, rr.Code, rr.Body.String())
+		}
+		if served := dispositionFilename(t, rr.Header().Get("Content-Disposition")); served != c.want {
+			t.Errorf("legacy %q served as %q, want %q", c.legacy, served, c.want)
+		}
+	}
+}

internal/store/attachment_bidi_backfill.go

@@ -0,0 +1,155 @@
+package store
+
+import (
+	"fmt"
+	"log/slog"
+	"strconv"
+	"strings"
+	"unicode"
+
+	"github.com/PerpetualSoftware/pad/internal/attachments"
+)
+
+// BUG-3153: attachment names stored before ingest dropped Bidi_Control
+// characters.
+//
+// A name such as "x<U+202E>gvs.txt" is a .txt that a bidi-aware renderer —
+// the web UI's attachment strip, storage tab, lightbox and chips, the CLI list,
+// server-rendered chips — displays as "xtxt.svg". Ingest now drops those characters (attachments.DroppedFilenameRune),
+// and the download header drops them for any row by construction. What is left
+// is the stored value those display readers show, so the rows are rewritten at
+// rest, once, rather than every reader being patched.
+//
+// The new name is the SERVED name, not a plain strip. A Bidi_Control inside an
+// extension hid it from the blocklist ("x.s<U+202E>vg" is no known extension),
+// so stripping alone could store "x.svg" — the blocked extension BUG-2818's
+// ingest exists to keep out of the table. attachments.ServedFilename strips and
+// then maps a blocked extension to ".bin". For a selected row the rewrite is
+// therefore WIDER than the Bidi_Control characters: whatever else
+// NormalizeFilename changes about a legacy name (other control characters,
+// trailing dots and spaces) changes too, which makes the stored name equal the
+// name the download header has served since BUG-2818. Rows without a
+// Bidi_Control character are never selected, so no other row changes.
+// That choice lives HERE rather than
+// at the startup call site, so no wiring can substitute a plain strip without
+// a test in this package seeing it.
+
+// bidiControlRunes is the Unicode Bidi_Control property, enumerated for SQL
+// LIKE patterns. TestBidiControlRunesIsTheProperty pins it against
+// unicode.Bidi_Control, so the SQL selection and the Go check cannot drift.
+var bidiControlRunes = []rune{
+	0x061C,
+	0x200E, 0x200F,
+	0x202A, 0x202B, 0x202C, 0x202D, 0x202E,
+	0x2066, 0x2067, 0x2068, 0x2069,
+}
+
+// BackfillBidiFilenamesResult reports what the backfill did, for the startup
+// log line.
+type BackfillBidiFilenamesResult struct {
+	RowsRewritten int
+}
+
+const bidiBackfillBatch = 200
+
+// BackfillBidiAttachmentFilenames rewrites every attachment row whose filename
+// carries a Bidi_Control character to its served name. Called from server startup
+// after migrations; soft-deleted rows are included, since a restore would bring
+// them back.
+//
+// No completion marker, like BackfillYjsContentBearing: the progress marker is
+// the row itself. The new name must be free of Bidi_Control characters,
+// so a rewritten row no longer matches the selection and a second run finds
+// nothing. A rename that leaves one is refused as an error rather than written,
+// because it would be selected again on every boot.
+//
+// Reversible for as long as the server log is retained, and no longer: every
+// rewrite logs the attachment id, its workspace, and the old and new names
+// quoted to ASCII (so the log line itself cannot spoof a reader, and the old
+// name can be restored byte for byte). There is no durable record in the
+// database. Each UPDATE is conditional on the old name, so a concurrent rename
+// is never overwritten.
+//
+// It runs on EVERY boot and selects by content, so a row that gains such a
+// name later — a variant or copy derived from a parent that a failed pass left
+// unrewritten — is picked up by the next start.
+func (s *Store) BackfillBidiAttachmentFilenames() (*BackfillBidiFilenamesResult, error) {
+	return s.backfillBidiAttachmentFilenames(attachments.ServedFilename)
+}
+
+// backfillBidiAttachmentFilenames takes the rename as a parameter so tests can
+// drive the refusal and a concurrent rename; production passes ServedFilename.
+func (s *Store) backfillBidiAttachmentFilenames(rename func(string) string) (*BackfillBidiFilenamesResult, error) {
+	res := &BackfillBidiFilenamesResult{}
+
+	likes := make([]string, len(bidiControlRunes))
+	args := make([]any, 0, len(bidiControlRunes)+2)
+	for i, r := range bidiControlRunes {
+		likes[i] = "filename LIKE ?"
+		args = append(args, "%"+string(r)+"%")
+	}
+	query := s.q(`SELECT id, workspace_id, filename FROM attachments
+		WHERE id > ? AND (` + strings.Join(likes, " OR ") + `)
+		ORDER BY id LIMIT ?`)
+
+	lastID := ""
+	for {
+		type row struct{ id, workspaceID, filename string }
+		rows, err := s.db.Query(query, append(append([]any{lastID}, args...), bidiBackfillBatch)...)
+		if err != nil {
+			return res, fmt.Errorf("backfill bidi filenames: select: %w", err)
+		}
+		var batch []row
+		for rows.Next() {
+			var r row
+			if err := rows.Scan(&r.id, &r.workspaceID, &r.filename); err != nil {
+				_ = rows.Close()
+				return res, fmt.Errorf("backfill bidi filenames: scan: %w", err)
+			}
+			batch = append(batch, r)
+		}
+		if err := rows.Close(); err != nil {
+			return res, err
+		}
+		if err := rows.Err(); err != nil {
+			return res, err
+		}
+		if len(batch) == 0 {
+			return res, nil
+		}
+
+		for _, r := range batch {
+			lastID = r.id
+			if !strings.ContainsFunc(r.filename, isBidiControl) {
+				continue // a LIKE match the property does not confirm
+			}
+			newName := rename(r.filename)
+			if strings.ContainsFunc(newName, isBidiControl) {
+				return res, fmt.Errorf("backfill bidi filenames: rename of attachment %s left a Bidi_Control character", r.id)
+			}
+			result, err := s.db.Exec(s.q(`UPDATE attachments SET filename = ? WHERE id = ? AND filename = ?`),
+				newName, r.id, r.filename)
+			if err != nil {
+				return res, fmt.Errorf("backfill bidi filenames: update %s: %w", r.id, err)
+			}
+			n, err := result.RowsAffected()
+			if err != nil {
+				// The UPDATE may have landed; without a count this row cannot
+				// be logged as rewritten, so stop rather than go unrecorded.
+				return res, fmt.Errorf("backfill bidi filenames: rows affected %s: %w", r.id, err)
+			}
+			if n == 0 {
+				continue // renamed concurrently; that write stands
+			}
+			res.RowsRewritten++
+			slog.Info("attachment filename rewritten: Bidi_Control characters removed (BUG-3153)",
+				"attachment_id", r.id,
+				"workspace_id", r.workspaceID,
+				"old_filename", strconv.QuoteToASCII(r.filename),
+				"new_filename", strconv.QuoteToASCII(newName),
+			)
+		}
+	}
+}
+
+func isBidiControl(r rune) bool { return unicode.Is(unicode.Bidi_Control, r) }

internal/store/attachment_bidi_backfill_test.go

@@ -0,0 +1,182 @@
+package store
+
+import (
+	"strings"
+	"testing"
+	"unicode"
+
+	"github.com/PerpetualSoftware/pad/internal/models"
+)
+
+// The SQL selection is built from bidiControlRunes and the Go check from
+// unicode.Bidi_Control; this keeps them the same set.
+func TestBidiControlRunesIsTheProperty(t *testing.T) {
+	var want []rune
+	for r := rune(0); r <= unicode.MaxRune; r++ {
+		if unicode.Is(unicode.Bidi_Control, r) {
+			want = append(want, r)
+		}
+	}
+	if string(want) != string(bidiControlRunes) {
+		t.Fatalf("bidiControlRunes = %U, unicode.Bidi_Control = %U", bidiControlRunes, want)
+	}
+}
+
+func seedAttachmentNamed(t *testing.T, s *Store, ws *models.Workspace, filename string) *models.Attachment {
+	t.Helper()
+	a := &models.Attachment{
+		WorkspaceID: ws.ID,
+		UploadedBy:  "legacy-uploader",
+		StorageKey:  "fs:" + newID(),
+		ContentHash: newID(),
+		MimeType:    "text/plain",
+		SizeBytes:   1,
+		Filename:    filename,
+	}
+	if err := s.CreateAttachment(a); err != nil {
+		t.Fatalf("CreateAttachment(%q): %v", filename, err)
+	}
+	return a
+}
+
+func storedFilename(t *testing.T, s *Store, id string) string {
+	t.Helper()
+	a, err := s.GetAttachment(id)
+	if err != nil || a == nil {
+		t.Fatalf("GetAttachment(%s): %v", id, err)
+	}
+	return a.Filename
+}
+
+// BUG-3153: rows stored before ingest dropped Bidi_Control characters are
+// rewritten to their SERVED name, once, and nothing else is touched.
+func TestBackfillBidiAttachmentFilenames(t *testing.T) {
+	s := testStore(t)
+	ws := createTestWorkspace(t, s, "Bidi Legacy")
+
+	// The spoof: a .txt that renders as "xtxt.svg". Its letters and real
+	// extension survive.
+	spoof := seedAttachmentNamed(t, s, ws, "x\u202Egvs.txt")
+	// A control inside the extension hid it from the blocklist. A plain strip
+	// would store "x.svg"; the served name maps the uncovered blocked
+	// extension to .bin.
+	hidden := seedAttachmentNamed(t, s, ws, "x.s\u202Evg")
+	// The rewrite is the SERVED name, so other legacy characters on a
+	// selected row go too (here a trailing dot and space).
+	wider := seedAttachmentNamed(t, s, ws, "w\u202Egvs.txt. ")
+	// Marks and isolates count too.
+	marks := seedAttachmentNamed(t, s, ws, "a\u200Fb\u2066c\u2069.txt")
+	// Soft-deleted rows are included: a restore would bring them back.
+	deleted := seedAttachmentNamed(t, s, ws, "d\u202Egvs.txt")
+	if err := s.SoftDeleteAttachment(deleted.ID); err != nil {
+		t.Fatalf("SoftDeleteAttachment: %v", err)
+	}
+	// Untouched: an ordinary name, an emoji ZWJ sequence (Join_Control, not
+	// Bidi_Control), and an RTL-script name with no control character.
+	plain := seedAttachmentNamed(t, s, ws, "notes.txt")
+	emoji := seedAttachmentNamed(t, s, ws, "\U0001F469\u200D\U0001F4BB notes.txt")
+	hebrew := seedAttachmentNamed(t, s, ws, "\u05E9\u05DC\u05D5\u05DD.txt")
+
+	res, err := s.BackfillBidiAttachmentFilenames()
+	if err != nil {
+		t.Fatalf("backfill: %v", err)
+	}
+	if res.RowsRewritten != 5 {
+		t.Errorf("RowsRewritten = %d, want 5", res.RowsRewritten)
+	}
+	for _, c := range []struct {
+		a    *models.Attachment
+		want string
+	}{
+		{spoof, "xgvs.txt"},
+		{hidden, "x.bin"},
+		{wider, "wgvs.txt"},
+		{marks, "abc.txt"},
+		{deleted, "dgvs.txt"},
+		{plain, "notes.txt"},
+		{emoji, "\U0001F469\u200D\U0001F4BB notes.txt"},
+		{hebrew, "\u05E9\u05DC\u05D5\u05DD.txt"},
+	} {
+		if got := storedFilename(t, s, c.a.ID); got != c.want {
+			t.Errorf("attachment %q: stored %q, want %q", c.a.Filename, got, c.want)
+		}
+	}
+
+	// Idempotent: nothing left to select.
+	again, err := s.BackfillBidiAttachmentFilenames()
+	if err != nil || again.RowsRewritten != 0 {
+		t.Errorf("second run: rewritten=%d err=%v, want 0 and nil", again.RowsRewritten, err)
+	}
+}
+
+// A rename that leaves a Bidi_Control character would be selected again on
+// every boot, so it is refused and not written.
+func TestBackfillBidiAttachmentFilenamesRefusesARenameThatLeavesOne(t *testing.T) {
+	s := testStore(t)
+	ws := createTestWorkspace(t, s, "Bidi Refuse")
+	a := seedAttachmentNamed(t, s, ws, "x\u202Egvs.txt")
+
+	res, err := s.backfillBidiAttachmentFilenames(func(old string) string { return old + ".bin" })
+	if err == nil || !strings.Contains(err.Error(), a.ID) {
+		t.Fatalf("expected an error naming %s, got %v", a.ID, err)
+	}
+	if res.RowsRewritten != 0 {
+		t.Errorf("RowsRewritten = %d, want 0", res.RowsRewritten)
+	}
+	if got := storedFilename(t, s, a.ID); got != "x\u202Egvs.txt" {
+		t.Errorf("refused rename was written: stored %q", got)
+	}
+}
+
+// Each UPDATE is conditional on the name it read, so a rename that lands
+// between the select and the update stands and is not counted. The rename
+// callback runs in that gap, which makes the interleaving deterministic.
+func TestBackfillBidiAttachmentFilenamesKeepsAConcurrentRename(t *testing.T) {
+	s := testStore(t)
+	ws := createTestWorkspace(t, s, "Bidi Concurrent")
+	a := seedAttachmentNamed(t, s, ws, "x\u202Egvs.txt")
+
+	res, err := s.backfillBidiAttachmentFilenames(func(old string) string {
+		if _, err := s.db.Exec(s.q(`UPDATE attachments SET filename = ? WHERE id = ?`), "renamed-by-user.txt", a.ID); err != nil {
+			t.Fatalf("concurrent rename: %v", err)
+		}
+		return "xgvs.txt"
+	})
+	if err != nil {
+		t.Fatalf("backfill: %v", err)
+	}
+	if res.RowsRewritten != 0 {
+		t.Errorf("RowsRewritten = %d, want 0: the row was renamed under it", res.RowsRewritten)
+	}
+	if got := storedFilename(t, s, a.ID); got != "renamed-by-user.txt" {
+		t.Errorf("the concurrent rename was overwritten: stored %q", got)
+	}
+}
+
+// Every member of the property is selected by the SQL, on both dialects: one
+// row per rune, each rewritten. The runes come from unicode.Bidi_Control, NOT
+// from bidiControlRunes, so a member missing from the SQL list fails here.
+func TestBackfillBidiAttachmentFilenamesSelectsEveryBidiControl(t *testing.T) {
+	s := testStore(t)
+	ws := createTestWorkspace(t, s, "Bidi Every Rune")
+	seeded := map[string]rune{}
+	for r := rune(0); r <= unicode.MaxRune; r++ {
+		if !unicode.Is(unicode.Bidi_Control, r) {
+			continue
+		}
+		a := seedAttachmentNamed(t, s, ws, "n"+string(r)+"ame.txt")
+		seeded[a.ID] = r
+	}
+	res, err := s.BackfillBidiAttachmentFilenames()
+	if err != nil {
+		t.Fatalf("backfill: %v", err)
+	}
+	if res.RowsRewritten != len(seeded) {
+		t.Errorf("RowsRewritten = %d, want %d", res.RowsRewritten, len(seeded))
+	}
+	for id, r := range seeded {
+		if got := storedFilename(t, s, id); got != "name.txt" {
+			t.Errorf("%U: stored %q, want %q", r, got, "name.txt")
+		}
+	}
+}

web/e2e/attachment-viewer-parity.spec.ts

@@ -372,7 +372,12 @@ test.describe('attachment viewer — four-surface parity (TASK-2436)', () => {
 		// a dialog that names itself — not an empty name, not a truncated one,
 		// and not a layout that pushes the page sideways.
 		const longName = `${'long-'.repeat(30)}name.png`;
-		const bidiName = 'start ‮reversed‬ مرحبا.png';
+		const bidiName = 'start \u202Ereversed\u202C مرحبا.png';
+		// Ingest drops Bidi_Control characters (BUG-3153): the override is how a
+		// name displays an extension it does not have. The name the UI shows is
+		// the STORED one — the RTL letters kept, the controls gone — and it must
+		// still be addressable and name its dialog.
+		const bidiStored = 'start reversed مرحبا.png';
 
 		await browserLogin(page);
 		const doc = await seedDoc(fixture, request, 'Parity hostile names');
@@ -381,7 +386,7 @@ test.describe('attachment viewer — four-surface parity (TASK-2436)', () => {
 		await page.goto(itemUrl(fixture, doc.slug));
 		await expect(page.locator(TILE)).toHaveCount(2);
 
-		for (const name of [longName, bidiName]) {
+		for (const name of [longName, bidiStored]) {
 			// Addressable BY NAME — the accessible name is the whole filename,
 			// exactly as stored.
 			// ANCHORED at BOTH ends of the filename: on the action verb (the
@@ -412,7 +417,7 @@ test.describe('attachment viewer — four-surface parity (TASK-2436)', () => {
 		// horizontal overflow — the failure a physical `left`/`right` pair would
 		// produce if the layout depended on it.
 		await page.evaluate(() => document.documentElement.setAttribute('dir', 'rtl'));
-		await page.getByRole('button', { name: new RegExp(`^View ${escapeRe(bidiName)},`) }).click();
+		await page.getByRole('button', { name: new RegExp(`^View ${escapeRe(bidiStored)},`) }).click();
 		await expect(page.locator(VIEWER)).toHaveCount(1);
 		const rtl = await page.evaluate(() => {
 			const el = document.querySelector<HTMLElement>('.attachment-viewer')!;