diff --git a/cmd/pad/cmd_server.go b/cmd/pad/cmd_server.go index 87624fc2c..f060aeaca 100644 --- a/cmd/pad/cmd_server.go +++ b/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 diff --git a/internal/attachments/filename.go b/internal/attachments/filename.go index 65b6d6ae9..b6405bc50 100644 --- a/internal/attachments/filename.go +++ b/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 "xgvs.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.svg" 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). diff --git a/internal/attachments/filename_test.go b/internal/attachments/filename_test.go index f00a45deb..0969d10d6 100644 --- a/internal/attachments/filename_test.go +++ b/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"}, diff --git a/internal/server/handlers_attachments.go b/internal/server/handlers_attachments.go index 9f546ed48..57b0b07e9 100644 --- a/internal/server/handlers_attachments.go +++ b/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 { diff --git a/internal/server/handlers_attachments_bug3153_test.go b/internal/server/handlers_attachments_bug3153_test.go new file mode 100644 index 000000000..95ed9ab55 --- /dev/null +++ b/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: "xgvs.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 +// ".svg". 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) + } + } +} diff --git a/internal/store/attachment_bidi_backfill.go b/internal/store/attachment_bidi_backfill.go new file mode 100644 index 000000000..d8e64b143 --- /dev/null +++ b/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 "xgvs.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.svg" 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) } diff --git a/internal/store/attachment_bidi_backfill_test.go b/internal/store/attachment_bidi_backfill_test.go new file mode 100644 index 000000000..c4901c376 --- /dev/null +++ b/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") + } + } +} diff --git a/web/e2e/attachment-viewer-parity.spec.ts b/web/e2e/attachment-viewer-parity.spec.ts index 4fae3e094..2d1b142c0 100644 --- a/web/e2e/attachment-viewer-parity.spec.ts +++ b/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('.attachment-viewer')!;