From a37c04ea67cce16a4db51e09ded132f9ff92e03c Mon Sep 17 00:00:00 2001 From: xarmian Date: Sun, 1 Nov 2026 01:51:33 -0400 Subject: [PATCH 01/16] fix(cli,mcp): tell an agent that addresses another agent's DM which target is its own, before any request (TASK-4186 PR C) (#2361) * fix(cli,mcp): tell an agent that addresses another agent's DM which target is its own, before any request (TASK-4186 PR C) models.NormalizeAgentName is now the single normaliser (the server delegates). cli.CheckAgentChatTarget refuses an @x target that is not the calling agent's own, before any list or POST, in Client.ResolveChatTarget (CLI and stdio MCP) and the HTTP MCP resolveChatTarget, for every chat verb. A shared name table (internal/agentchatcases) is run through both the guard and the real ensure handler. Claude-Session: https://claude.ai/code/session_01Pa22hHHb1atXYf1JmBYE1v * fix(cli,mcp): guard only chat send, keep show/read/delete's not_found, tighten the guard tests (TASK-4186 PR C r1) Claude-Session: https://claude.ai/code/session_01Pa22hHHb1atXYf1JmBYE1v * docs(cli): say the chat agent guard runs for send only (TASK-4186 PR C r2) Claude-Session: https://claude.ai/code/session_01Pa22hHHb1atXYf1JmBYE1v --- CLAUDE.md | 1 + cmd/pad/cmd_chat.go | 2 +- cmd/pad/cmd_chat_test.go | 5 + internal/agentchatcases/cases.go | 61 +++++++++ internal/cli/chat.go | 24 ++++ internal/cli/chat_agent_target_test.go | 103 +++++++++++++++ internal/cli/client.go | 7 ++ internal/mcp/dispatch_http_chat.go | 7 ++ .../mcp/task4186_chat_agent_guard_test.go | 117 ++++++++++++++++++ internal/models/agent_name.go | 30 +++++ internal/server/agent_name.go | 26 +--- .../server/task4186_chat_agent_guard_test.go | 42 +++++++ 12 files changed, 402 insertions(+), 23 deletions(-) create mode 100644 internal/agentchatcases/cases.go create mode 100644 internal/cli/chat_agent_target_test.go create mode 100644 internal/mcp/task4186_chat_agent_guard_test.go create mode 100644 internal/models/agent_name.go create mode 100644 internal/server/task4186_chat_agent_guard_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 0f858f6ab..26be13cf1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -159,6 +159,7 @@ PAD_EMAIL_FROM_NAME=Pad # Sender display name (default: Pad) ## CLI Global chat targets are `C`, ``, or `@` for your DM with an agent. +An agent (one that sends X-Pad-Agent) has one chat, with its owner: for it `@` is its OWN name, and `pad chat send @x` to another agent is refused before any request (show, read and delete keep their not_found). All chat commands support `--format json`. ```bash diff --git a/cmd/pad/cmd_chat.go b/cmd/pad/cmd_chat.go index 0e888d12a..cc0035296 100644 --- a/cmd/pad/cmd_chat.go +++ b/cmd/pad/cmd_chat.go @@ -110,7 +110,7 @@ func chatShowCmd() *cobra.Command { func chatSendCmd() *cobra.Command { var replyTo int64 var opKey, contextWorkspace string - cmd := &cobra.Command{Use: "send ", Example: " pad chat send @rook \"Ready for review\"", Short: "Send a chat message (- reads stdin)", Args: cobra.ExactArgs(2), + cmd := &cobra.Command{Use: "send ", Example: " pad chat send @rook \"Ready for review\"", Short: "Send a chat message (- reads stdin)", Long: "Send a chat message. Targets are C, or @.\n\nAs an agent, @ is your own DM with your owner (your own name); sending to another @x is another agent's DM and is refused. Use C from `pad chat list`.", Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { if _, _, err := cli.ParseChatTarget(args[0]); err != nil { return err diff --git a/cmd/pad/cmd_chat_test.go b/cmd/pad/cmd_chat_test.go index ac6680a44..74c5eee31 100644 --- a/cmd/pad/cmd_chat_test.go +++ b/cmd/pad/cmd_chat_test.go @@ -66,6 +66,11 @@ func TestChatShowReadMarker(t *testing.T) { } func TestChatSendAndTargetResolution(t *testing.T) { + // A human caller: an ambient PAD_AGENT or CLAUDECODE would make @rook + // another agent's DM (TASK-4186 PR C). The send cases would then be refused + // by the agent guard before any request, and show would find nothing. + t.Setenv("PAD_AGENT", "") + t.Setenv("CLAUDECODE", "") oldFormat, oldWorkspace, oldURL := formatFlag, workspaceFlag, urlFlag t.Cleanup(func() { formatFlag, workspaceFlag, urlFlag = oldFormat, oldWorkspace, oldURL }) diff --git a/internal/agentchatcases/cases.go b/internal/agentchatcases/cases.go new file mode 100644 index 000000000..3f32ab11b --- /dev/null +++ b/internal/agentchatcases/cases.go @@ -0,0 +1,61 @@ +// Package agentchatcases is the one name table behind the TASK-4186 PR C +// acceptance bar: the client guard (cli.CheckAgentChatTarget) and the server's +// ensure handler must refuse exactly the same rows. The cli and mcp tests and +// the server test all import it, so a row added here is judged by both sides. +// It holds test data only and is imported by tests alone. +package agentchatcases + +import ( + "strings" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// Case is one caller (the X-Pad-Agent header) addressing one target. +type Case struct { + Name string + Caller string + Target string // the chat target as typed: "@x", "C3" or "3" + // Key is the body agent_key the ensure call would send (the name after + // "@"). Empty for a C / target, which never reaches ensure. + Key string + // Refuse is the shared expectation: the guard AND the server's ensure + // handler must both refuse this row. + Refuse bool +} + +func dm(name, caller, key string, refuse bool) Case { + return Case{Name: name, Caller: caller, Target: "@" + key, Key: key, Refuse: refuse} +} + +// Cases is the table. The cap rows are built from models.MaxAgentNameRunes. +func Cases() []Case { + max := models.MaxAgentNameRunes + a := strings.Repeat("a", max) + over := a + "bcdef" + mbFull := strings.Repeat("é", max) + mbEdge := strings.Repeat("a", max-1) + "é" + mbOver := mbEdge + "é" + return []Case{ + dm("exact match", "director", "director", false), + dm("other agent", "director", "dave", true), + dm("case difference refuses", "Director", "director", true), + dm("case difference refuses, other way", "director", "Director", true), + dm("caller trailing space", "director ", "director", false), + dm("caller leading space", " director", "director", false), + dm("target trailing space is raw", "director", "director ", true), + dm("target leading space is raw", "director", " director", true), + dm("caller control char", "dir\tec\x00tor", "director", false), + dm("target control char is raw", "director", "dir\tector", true), + dm("caller over cap, truncated target", over, a, false), + dm("caller over cap, full target", over, over, true), + dm("multi-byte caller exactly at cap", mbFull, mbFull, false), + dm("multi-byte caller one rune over cap, truncated target", mbOver, mbEdge, false), + dm("multi-byte caller one rune over cap, full target", mbOver, mbOver, true), + dm("multi-byte rune on the boundary", mbEdge, mbEdge, false), + dm("empty caller is a human", "", "dave", false), + dm("whitespace-only caller is a human", " \t ", "dave", false), + {Name: "C target", Caller: "director", Target: "C3"}, + {Name: " target", Caller: "director", Target: "3"}, + } +} diff --git a/internal/cli/chat.go b/internal/cli/chat.go index b2198f0ec..4e0f4337c 100644 --- a/internal/cli/chat.go +++ b/internal/cli/chat.go @@ -4,6 +4,8 @@ import ( "fmt" "strconv" "strings" + + "github.com/PerpetualSoftware/pad/internal/models" ) // ParseChatTarget accepts an owner-scoped conversation number (C12 or 12) @@ -22,6 +24,28 @@ func ParseChatTarget(target string) (number int, agent string, err error) { return 0, "", fmt.Errorf("invalid chat target %q: use C, , or @", target) } +// CheckAgentChatTarget refuses, before any request, an @ send target +// that names a DM other than the calling agent's own. An agent has one chat, +// with its owner, and the server enforces that on send +// (handleEnsureChatConversation: agent_key must match the normalised +// X-Pad-Agent); this only replaces that refusal with a message naming the +// right target. Callers run it for send only: show, read and delete keep their +// truthful "not found". It compares exactly as the server does: the caller is +// normalised, the typed name is not, and case is not folded. A human (an empty +// normalised caller) and C / targets pass. It echoes only the caller's +// own name and the target typed, never another agent's. +func CheckAgentChatTarget(callerAgent, target string) error { + caller := models.NormalizeAgentName(callerAgent) + if caller == "" { + return nil + } + _, agent, err := ParseChatTarget(target) + if err != nil || agent == "" || agent == caller { + return nil + } + return fmt.Errorf("as agent %q you have one chat, with your owner: use @%s or C (pad chat list); @%s is another agent's DM", caller, caller, agent) +} + // ChatReadSeq returns the highest displayed sequence only when the page reaches // the existing read marker. A high-water mark must not skip unseen messages. // Conversation sequences are contiguous, including deleted-message tombstones. diff --git a/internal/cli/chat_agent_target_test.go b/internal/cli/chat_agent_target_test.go new file mode 100644 index 000000000..7914d359a --- /dev/null +++ b/internal/cli/chat_agent_target_test.go @@ -0,0 +1,103 @@ +package cli + +import ( + "errors" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + "github.com/PerpetualSoftware/pad/internal/agentchatcases" +) + +func TestCheckAgentChatTargetSharedTable(t *testing.T) { + for _, tc := range agentchatcases.Cases() { + t.Run(tc.Name, func(t *testing.T) { + err := CheckAgentChatTarget(tc.Caller, tc.Target) + if (err != nil) != tc.Refuse { + t.Fatalf("CheckAgentChatTarget(%q, %q) = %v, want refuse=%v", tc.Caller, tc.Target, err, tc.Refuse) + } + }) + } +} + +// chatStub records every request and answers conversations with other agents' +// DMs, so a leak of their keys into an error is visible. +type chatStub struct { + mu sync.Mutex + reqs []string +} + +func (s *chatStub) handler(w http.ResponseWriter, r *http.Request) { + s.mu.Lock() + s.reqs = append(s.reqs, r.Method+" "+r.URL.Path) + s.mu.Unlock() + w.Header().Set("Content-Type", "application/json") + if r.Method == http.MethodPost { + _, _ = w.Write([]byte(`{"conversation":{"number":7}}`)) + return + } + _, _ = w.Write([]byte(`{"conversations":[{"number":1,"agent_key":"secret-sibling"},{"number":2,"agent_key":"hidden-bot"}]}`)) +} + +func TestResolveChatTargetGuardSendsNothingOnRefusal(t *testing.T) { + stub := &chatStub{} + ts := httptest.NewServer(http.HandlerFunc(stub.handler)) + defer ts.Close() + c := NewClientFromURLWithAgentName(ts.URL, "director") + _, err := c.ResolveChatTarget("@dave", true) + if err == nil { + t.Fatal("send: want refusal") + } + if len(stub.reqs) != 0 { + t.Fatalf("refusal still sent %v", stub.reqs) + } + for _, other := range []string{"secret-sibling", "hidden-bot"} { + if strings.Contains(err.Error(), other) { + t.Fatalf("error names another agent %q: %v", other, err) + } + } + if !strings.Contains(err.Error(), `@director`) || !strings.Contains(err.Error(), "C") { + t.Fatalf("error does not point at the caller's own target: %v", err) + } +} + +// show, read and delete resolve with ensure=false: an agent's @other keeps the +// truthful not_found from the lookup, and the guard (whose error is not an +// APIError) is not invoked. +func TestResolveChatTargetNoEnsureKeepsNotFound(t *testing.T) { + stub := &chatStub{} + ts := httptest.NewServer(http.HandlerFunc(stub.handler)) + defer ts.Close() + c := NewClientFromURLWithAgentName(ts.URL, "director") + _, err := c.ResolveChatTarget("@dave", false) + var apiErr *APIError + if !errors.As(err, &apiErr) || apiErr.Code != "not_found" { + t.Fatalf("want not_found APIError, got %v", err) + } + if strings.Contains(err.Error(), "one chat") { + t.Fatalf("guard ran on a no-ensure resolve: %v", err) + } + if len(stub.reqs) != 1 || stub.reqs[0] != "GET /api/v1/me/conversations" { + t.Fatalf("want exactly the lookup, got %v", stub.reqs) + } +} + +func TestResolveChatTargetGuardPasses(t *testing.T) { + stub := &chatStub{} + ts := httptest.NewServer(http.HandlerFunc(stub.handler)) + defer ts.Close() + c := NewClientFromURLWithAgentName(ts.URL, "director ") + n, err := c.ResolveChatTarget("@director", true) + if err != nil || n != 7 { + t.Fatalf("own DM: n=%d err=%v", n, err) + } + if len(stub.reqs) != 2 { + t.Fatalf("requests = %v, want the list then the ensure", stub.reqs) + } + human := NewClientFromURLWithAgentName(ts.URL, "") + if n, err := human.ResolveChatTarget("@rook", true); err != nil || n != 7 { + t.Fatalf("human: n=%d err=%v", n, err) + } +} diff --git a/internal/cli/client.go b/internal/cli/client.go index 68a5787dc..9226de6cb 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -3601,6 +3601,13 @@ func (c *Client) ResolveChatTarget(target string, ensure bool) (int, error) { if err != nil || agent == "" { return n, err } + // Send only (ensure): show, read and delete keep the truthful not_found + // for an agent's @other, since they never create or message a DM. + if ensure { + if err := CheckAgentChatTarget(c.agentName, target); err != nil { + return 0, err + } + } raw, err := c.ListChatConversations() if err != nil { return 0, err diff --git a/internal/mcp/dispatch_http_chat.go b/internal/mcp/dispatch_http_chat.go index 13aa765be..7637f8919 100644 --- a/internal/mcp/dispatch_http_chat.go +++ b/internal/mcp/dispatch_http_chat.go @@ -68,6 +68,13 @@ func (d *HTTPHandlerDispatcher) resolveChatTarget(ctx context.Context, key strin if agent == "" { return n, nil } + // TASK-4186 PR C: send only (ensure); show, read and delete keep not_found. + // The name is the one the dispatcher sends as X-Pad-Agent. + if ensure { + if err := cli.CheckAgentChatTarget(d.Clients.resolveRemoteCaller(ctx).Name, target); err != nil { + return 0, validationFailedResult(key, err.Error(), "Use your own @ or C from pad_chat list.") + } + } res := d.chatRequest(ctx, key, user, http.MethodGet, "/api/v1/me/conversations", nil) if res.IsError { return 0, res diff --git a/internal/mcp/task4186_chat_agent_guard_test.go b/internal/mcp/task4186_chat_agent_guard_test.go new file mode 100644 index 000000000..022299576 --- /dev/null +++ b/internal/mcp/task4186_chat_agent_guard_test.go @@ -0,0 +1,117 @@ +package mcp + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" + padserver "github.com/PerpetualSoftware/pad/internal/server" + "github.com/PerpetualSoftware/pad/internal/store/storetest" +) + +type chatRecorder struct { + next http.Handler + mu sync.Mutex + reqs []string +} + +func (c *chatRecorder) ServeHTTP(w http.ResponseWriter, r *http.Request) { + c.mu.Lock() + c.reqs = append(c.reqs, r.Method+" "+r.URL.Path) + c.mu.Unlock() + c.next.ServeHTTP(w, r) +} + +func (c *chatRecorder) count() int { + c.mu.Lock() + defer c.mu.Unlock() + return len(c.reqs) +} + +func (c *chatRecorder) since(n int) []string { + c.mu.Lock() + defer c.mu.Unlock() + return append([]string(nil), c.reqs[n:]...) +} + +// TASK-4186 PR C: the remote resolveChatTarget refuses send to another agent's @name +// before any list or POST (show, read and delete keep not_found), and does not name any other agent. +func TestChatRemoteAgentTargetGuard(t *testing.T) { + s := storetest.NewSQLite(t) + srv := padserver.New(s) + t.Cleanup(srv.Stop) + owner, err := s.CreateUser(models.UserCreate{Email: "chat4186@example.com", Name: "Owner", Password: "correct-horse-battery-staple"}) + if err != nil { + t.Fatal(err) + } + if _, _, err := s.EnsureAgentDM(owner.ID, "secret-sibling"); err != nil { + t.Fatal(err) + } + rec := &chatRecorder{next: srv} + clients := NewClientRegistry() + d := &HTTPHandlerDispatcher{Handler: rec, Clients: clients, UserResolver: func(context.Context) *models.User { return owner }} + ms := NewServer(Options{Version: "test", Clients: clients}) + if _, err := RegisterCatalog(ms.MCP(), CatalogOptions{Doc: chatTestDoc(), Workspace: NewWorkspaceState(""), Dispatcher: d}); err != nil { + t.Fatal(err) + } + transport := NewRemoteTransport(ms.MCP(), bug2772Sessions{}) + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + transport.ServeHTTP(w, r.WithContext(padserver.WithCurrentUser(r.Context(), owner))) + })) + t.Cleanup(ts.Close) + world := &bug2772World{t: t, s: s, srv: srv, owner: owner, url: ts.URL + "/mcp"} + session := world.initialize("director") + + // Send is guarded: a validation_failed refusal from the guard's own text, + // and no request to /me/conversations. + sendArgs := `{"action":"send","target":"@dave","body":"hi"}` + before := rec.count() + raw := world.call("pad_chat", sendArgs, session) + if !strings.Contains(raw, `"isError":true`) { + t.Fatalf("send: want an error result, got %s", raw) + } + if !strings.Contains(raw, "validation") { + t.Fatalf("send: want a validation code, got %s", raw) + } + if !strings.Contains(raw, "one chat") { + t.Fatalf("send: refusal is not the guard's text: %s", raw) + } + if strings.Contains(raw, "secret-sibling") { + t.Fatalf("send: refusal names another agent: %s", raw) + } + if !strings.Contains(raw, "@director") { + t.Fatalf("send: refusal does not name the caller's own DM: %s", raw) + } + if n := rec.count(); n != before { + t.Fatalf("send: refusal still sent %v", rec.since(before)) + } + + // show, read and delete keep the truthful not_found: the guard is not + // invoked, so the lookup runs and finds no such DM. + for _, args := range []string{ + `{"action":"show","target":"@dave"}`, + `{"action":"read","target":"@dave","to":1}`, + `{"action":"delete","target":"@dave","seq":1}`, + } { + before := rec.count() + raw := world.call("pad_chat", args, session) + if !strings.Contains(raw, `"isError":true`) || !strings.Contains(raw, `"code":"not_found"`) { + t.Fatalf("%s: want not_found, got %s", args, raw) + } + if strings.Contains(raw, "one chat") || strings.Contains(raw, "secret-sibling") { + t.Fatalf("%s: guard text or another agent in result: %s", args, raw) + } + if got := rec.since(before); len(got) != 1 || got[0] != "GET /api/v1/me/conversations" { + t.Fatalf("%s: want exactly the lookup, got %v", args, got) + } + } + + raw = world.call("pad_chat", `{"action":"send","target":"@director","body":"hello"}`, session) + if strings.Contains(raw, `"isError":true`) { + t.Fatalf("own DM send refused: %s", raw) + } +} diff --git a/internal/models/agent_name.go b/internal/models/agent_name.go new file mode 100644 index 000000000..6d44437d0 --- /dev/null +++ b/internal/models/agent_name.go @@ -0,0 +1,30 @@ +package models + +import ( + "strings" + "unicode" +) + +// NormalizeAgentName is the one stored form of a self-declared agent name, +// shared by write attribution, presence (TASK-4040), chat key validation +// (TASK-4039 r3) and the client-side chat target guard (TASK-4186 PR C): +// trimmed, control characters removed, at most MaxAgentNameRunes runes. It +// does not fold case. +func NormalizeAgentName(raw string) string { + if raw == "" { + return "" + } + var b strings.Builder + n := 0 + for _, c := range strings.TrimSpace(raw) { + if unicode.IsControl(c) { + continue + } + if n == MaxAgentNameRunes { + break + } + b.WriteRune(c) + n++ + } + return strings.TrimSpace(b.String()) +} diff --git a/internal/server/agent_name.go b/internal/server/agent_name.go index 757464940..dd350ec0a 100644 --- a/internal/server/agent_name.go +++ b/internal/server/agent_name.go @@ -2,8 +2,6 @@ package server import ( "net/http" - "strings" - "unicode" "github.com/PerpetualSoftware/pad/internal/models" ) @@ -14,25 +12,9 @@ func agentNameFromRequest(r *http.Request) string { return normalizeAgentName(r.Header.Get("X-Pad-Agent")) } -// normalizeAgentName is the one stored form of a self-declared agent name, -// shared by write attribution, presence (TASK-4040) and chat key validation -// (TASK-4039 r3): trimmed, control characters removed, at most -// models.MaxAgentNameRunes runes. +// normalizeAgentName delegates to models.NormalizeAgentName, the single source +// of the stored form of a self-declared agent name (TASK-4186 PR C moved it so +// the CLI and MCP chat guard compares exactly as the server does). func normalizeAgentName(raw string) string { - if raw == "" { - return "" - } - var b strings.Builder - n := 0 - for _, c := range strings.TrimSpace(raw) { - if unicode.IsControl(c) { - continue - } - if n == models.MaxAgentNameRunes { - break - } - b.WriteRune(c) - n++ - } - return strings.TrimSpace(b.String()) + return models.NormalizeAgentName(raw) } diff --git a/internal/server/task4186_chat_agent_guard_test.go b/internal/server/task4186_chat_agent_guard_test.go new file mode 100644 index 000000000..3c8a37fe0 --- /dev/null +++ b/internal/server/task4186_chat_agent_guard_test.go @@ -0,0 +1,42 @@ +package server + +import ( + "testing" + + "github.com/PerpetualSoftware/pad/internal/agentchatcases" + "github.com/PerpetualSoftware/pad/internal/models" +) + +// TASK-4186 PR C: the client guard (cli.CheckAgentChatTarget) is only a +// pre-check, so it must refuse exactly what the real ensure handler refuses. +// The cli test runs the same table through the guard; this half drives the +// real handler. A row the guard refuses and the server accepts is a false +// refusal; the reverse is a guard that stopped guarding. +func TestChat_AgentChatTargetGuardParity(t *testing.T) { + f := newChatFixture(t) + for _, tc := range agentchatcases.Cases() { + if tc.Key == "" { + continue // C / never reaches the ensure handler + } + t.Run(tc.Name, func(t *testing.T) { + rr := f.request("POST", chatBase, tc.Caller, map[string]any{"agent_key": tc.Key}) + refused := rr.Code == 400 + if !refused && rr.Code != 200 && rr.Code != 201 { + t.Fatalf("unexpected status %d: %s", rr.Code, rr.Body.String()) + } + if refused != tc.Refuse { + t.Fatalf("server ensure(caller %q, key %q) status %d, want refuse=%v", tc.Caller, tc.Key, rr.Code, tc.Refuse) + } + }) + } +} + +func TestNormalizeAgentNameDelegatesToModels(t *testing.T) { + for _, tc := range agentchatcases.Cases() { + for _, in := range []string{tc.Caller, tc.Key} { + if got, want := normalizeAgentName(in), models.NormalizeAgentName(in); got != want { + t.Fatalf("normalizeAgentName(%q) = %q, models = %q", in, got, want) + } + } + } +} From 896cfde8e2b40da77483b5c786acd7e696611c18 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sun, 1 Nov 2026 02:45:52 -0500 Subject: [PATCH 02/16] refactor(web): drop announce()'s dead clear, correct the copy-test comments, pin the sync-throw re-announce (TASK-4210) (#2362) * refactor(web): drop announce()'s dead clear, correct the copy-test comments, pin the sync-throw re-announce (TASK-4210) Claude-Session: https://claude.ai/code/session_01Pa22hHHb1atXYf1JmBYE1v * test(web): note announce()'s tick in the double-failure comment (TASK-4210 r1-L1) Claude-Session: https://claude.ai/code/session_01Pa22hHHb1atXYf1JmBYE1v --- .../lib/components/chat/ChatEmptyState.svelte | 10 +++++-- .../chat/ChatEmptyState.svelte.test.ts | 29 +++++++++++++++++-- 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/web/src/lib/components/chat/ChatEmptyState.svelte b/web/src/lib/components/chat/ChatEmptyState.svelte index 851d28c7b..7385c12a5 100644 --- a/web/src/lib/components/chat/ChatEmptyState.svelte +++ b/web/src/lib/components/chat/ChatEmptyState.svelte @@ -26,6 +26,8 @@ ]; let copied = $state>({}); + // Shared across commands: with a second copyable command this must become + // per-command, or one command's outcome would overwrite the other's (TASK-4210 L3). let status = $state(''); let destroyed = false; @@ -41,10 +43,12 @@ // outcome fades: it stays until the next click on that button. const attempt: Record = {}; - // A live region announces a change; 'Copied' -> 'Copied' is none. Clear, let - // the DOM update, then set, so every outcome is announced. + // A live region announces a change; 'Copied' -> 'Copied' is none. The click + // already cleared status, and only the live attempt writes it, so no clear is + // needed here. The tick still matters: when navigator.clipboard is undefined, + // writeText throws synchronously, so without it the click's '' and the failure + // text would batch into one flush and the live region would not change. async function announce(cmd: string, n: number, text: string) { - status = ''; await tick(); if (destroyed || attempt[cmd] !== n) return; status = text; diff --git a/web/src/lib/components/chat/ChatEmptyState.svelte.test.ts b/web/src/lib/components/chat/ChatEmptyState.svelte.test.ts index 8c244a379..3650acb8e 100644 --- a/web/src/lib/components/chat/ChatEmptyState.svelte.test.ts +++ b/web/src/lib/components/chat/ChatEmptyState.svelte.test.ts @@ -161,7 +161,8 @@ describe('ChatEmptyState', () => { await flush(); mo.disconnect(); expect(live.textContent).toBe('Copied'); - // Without the clear, the second copy changes nothing and seen has one entry. + // Without the click's reset of status, the second copy changes nothing and seen + // has one entry. expect(seen.filter((t) => t === 'Copied').length).toBe(2); expect(seen).toContain(''); }); @@ -298,8 +299,10 @@ describe('ChatEmptyState', () => { const btn = getByRole('button', COPY); btn.click(); await flush(); - // writeText is already rejected: without announce()'s own clear-then-tick, the - // click's reset and the failure text land in one flush and the DOM never changes. + // The second click's reset of status ('') and the failure text must reach the DOM + // as separate changes; the click's reset provides the '' (announce() no longer + // clears), and announce()'s tick keeps it from batching with the failure text. + // The sync-throw path, where the tick matters, is pinned below. btn.click(); await flush(); mo.disconnect(); @@ -308,6 +311,26 @@ describe('ChatEmptyState', () => { expect(seen).toContain(''); }); + it('a synchronous writeText throw, clicked twice, re-announces the failure each time', async () => { + // navigator.clipboard undefined: the throw is synchronous, so only announce()'s + // tick separates the click's '' from the failure text. + Object.defineProperty(navigator, 'clipboard', { value: undefined, configurable: true }); + const { getByRole, container } = render(ChatEmptyState); + const live = container.querySelector('[aria-live="polite"]')!; + const seen: string[] = []; + const mo = new MutationObserver(() => seen.push(live.textContent ?? '')); + mo.observe(live, { childList: true, characterData: true, subtree: true }); + const btn = getByRole('button', COPY); + btn.click(); + await flush(); + btn.click(); + await flush(); + mo.disconnect(); + expect(live.textContent).toBe(FAIL); + expect(seen.filter((t) => t === FAIL).length).toBe(2); + expect(seen.filter((t) => t === '').length).toBe(1); + }); + it('writes no state and calls no tick() when writeText settles after unmount', async () => { let resolve!: () => void; writeText.mockImplementation(() => new Promise((r) => (resolve = r))); From 1ad05fe9e365b73b61462aacc510f545b015fb4e Mon Sep 17 00:00:00 2001 From: xarmian Date: Sun, 1 Nov 2026 14:58:34 -0500 Subject: [PATCH 03/16] fix(web): render file names bidi-isolated everywhere they're shown (TASK-4195) (#2363) * fix(web): render file names bidi-isolated everywhere they're shown (TASK-4195) A file name is data a stranger controls; a right-to-left override inside it reorders the text around it. Visible-text sites now wrap the name in (strip tile, storage row, lightbox title/fallback/meta, editor chip NodeView, markdown file chip, upload placeholder). Attribute and toast/prompt sites wrap only the name in FSI..PDI via new isolateText / displayFilenameIsolated helpers. displayFilename, download=, data-filename, node attrs, renderHTML and the Lightbox seeds stay untouched, so nothing stored or parsed back carries an isolate. 'bdi' joins MARKDOWN_ALLOWED_TAGS (the one sanitizer change); its attributes are still stripped. Claude-Session: https://claude.ai/code/session_01Pa22hHHb1atXYf1JmBYE1v * test(e2e): match the isolated delete-button name in the viewer-owners spec (TASK-4195) The strip's delete button now names the file between FSI and PDI, so the substring match on "Delete " no longer finds it. Claude-Session: https://claude.ai/code/session_01Pa22hHHb1atXYf1JmBYE1v * fix(web,render): keep img alt unisolated, and give the Go file chip the same with a byte-parity test (TASK-4195 r1) Claude-Session: https://claude.ai/code/session_01Pa22hHHb1atXYf1JmBYE1v * fix(render): byte-identical escaping for attachment chips and images across Go and TS (TASK-4195 r2) html.EscapeString emits " for a double quote where the TS escapeHtml emits ", so the Go and TS renderers disagreed on any file name or alt containing one. Add a local escapeHTML helper with exactly TS's five replacements and use it at every escape site in the attachment renderers. Extend the shared parity fixture to chips, images and missing placeholders (quote, apostrophe, ampersand, angle brackets, non-ASCII, bidi controls, alt fallback), asserted byte-for-byte by Go and vitest; vitest also asserts every case array is non-empty. Claude-Session: https://claude.ai/code/session_01Pa22hG9hiVzA8nRfEWhc5y * docs(render): scope the attachment parity test's URL half to UUID ids and post-BUG-3769 slugs (TASK-4195 r3) Go url.PathEscape and TS encodeURIComponent differ on !'()* and $&+=:@. The fixture pins parity for UUID attachment ids and slugs minted since BUG-3769 only; a preserved legacy slug may encode differently but decodes to the same route. Comment-only. Claude-Session: https://claude.ai/code/session_01Pa22hG9hiVzA8nRfEWhc5y * fix(web): route both upload-error toasts through a tested uploadFailedMessage helper (TASK-4195 site 17) Editor and CommentEditor built "Couldn't upload : " inline, and no test mounts the full Editor's upload error path, so removing the filename isolation survived (r1-L2, mutant M14). Both now call uploadFailedMessage in editor/attachment-upload.ts. A unit row pins the isolated output and a ?raw source pin asserts both sites call the helper. Claude-Session: https://claude.ai/code/session_01Pa22hG9hiVzA8nRfEWhc5y * docs(web): say where uploadFailedMessage's message comes from (TASK-4195 r5-L1) It is the upload call's Error.message, which may be the server's text; that text names at most an extension from the fixed map, never the file name. Comment-only. Claude-Session: https://claude.ai/code/session_01Pa22hG9hiVzA8nRfEWhc5y * docs(web): drop the reachability claim from uploadFailedMessage's comment (TASK-4195 r6-L1) A malformed multipart body makes the server echo the bad header line, so "never the file name" was false for non-browser clients. The comment now says only what holds: message is shown as received, not isolated. Comment-only. Claude-Session: https://claude.ai/code/session_01Pa22hG9hiVzA8nRfEWhc5y * test(web): match the site-17 test comment to the helper's (TASK-4195 r7-L1) "the server's message is ours" implied trusted text; it is shown as received. Comment-only. Claude-Session: https://claude.ai/code/session_01Pa22hG9hiVzA8nRfEWhc5y --- internal/server/render/attachments.go | 44 +++- internal/server/render/attachments_test.go | 83 ++++++- web/e2e/attachment-surface-chrome.spec.ts | 2 +- web/e2e/attachment-viewer-owners.spec.ts | 2 +- web/e2e/attachment-viewer-parity.spec.ts | 6 +- web/e2e/item-attachment-strip.spec.ts | 2 +- web/e2e/lib/attachment-viewer.ts | 2 +- .../attachments/bidiIsolation.svelte.test.ts | 206 ++++++++++++++++++ .../lib/attachments/bidiIsolation.testutil.ts | 41 ++++ web/src/lib/attachments/display.ts | 16 ++ web/src/lib/components/CommentEditor.svelte | 4 +- .../AttachmentDeleteConfirm.svelte | 6 +- web/src/lib/components/common/Lightbox.svelte | 21 +- .../components/common/Lightbox.svelte.test.ts | 81 +++++-- web/src/lib/components/editor/Editor.svelte | 4 +- .../lib/components/editor/attachment-chip.ts | 8 +- .../components/editor/attachment-upload.ts | 21 +- .../editor/attachmentChipPanel.svelte.test.ts | 37 +++- .../items/ItemAttachmentStrip.svelte | 17 +- .../items/ItemAttachmentStrip.svelte.test.ts | 56 ++++- .../lib/components/settings/StorageTab.svelte | 21 +- .../settings/StorageTab.svelte.test.ts | 46 ++++ web/src/lib/markdown/attachments.ts | 4 +- web/src/lib/markdown/fileChipParity.test.ts | 49 +++++ .../markdown/testdata/file-chip-parity.json | 31 +++ web/src/lib/utils/markdown.ts | 4 +- 26 files changed, 731 insertions(+), 83 deletions(-) create mode 100644 web/src/lib/attachments/bidiIsolation.svelte.test.ts create mode 100644 web/src/lib/attachments/bidiIsolation.testutil.ts create mode 100644 web/src/lib/markdown/fileChipParity.test.ts create mode 100644 web/src/lib/markdown/testdata/file-chip-parity.json diff --git a/internal/server/render/attachments.go b/internal/server/render/attachments.go index cc8f79e89..47c3ea0a4 100644 --- a/internal/server/render/attachments.go +++ b/internal/server/render/attachments.go @@ -12,7 +12,6 @@ package render import ( "fmt" - "html" "net/url" "regexp" "strings" @@ -158,6 +157,27 @@ func ShouldEmbedAsImage(meta *AttachmentMeta, wantVariant string) bool { return imgPaintableMimes[mime] } +// htmlEscaper escapes exactly the five characters the TS escapeHtml in +// web/src/lib/markdown/attachments.ts does, with the same replacements. It is +// a single-pass Replacer, so an emitted entity is never re-escaped (the same +// result as the TS chain that handles `&` first). +var htmlEscaper = strings.NewReplacer( + "&", "&", + "<", "<", + ">", ">", + `"`, """, + "'", "'", +) + +// escapeHTML exists to keep byte parity with escapeHtml in +// web/src/lib/markdown/attachments.ts (TASK-4195 r2-M1). html.EscapeString +// emits `"` for a double quote where the TS side emits `"`, so the +// two renderers disagreed on any name or alt containing one. The shared +// fixture web/src/lib/markdown/testdata/file-chip-parity.json pins the output. +func escapeHTML(s string) string { + return htmlEscaper.Replace(s) +} + // FormatAttachmentSize returns a human-readable byte count ("832 B", // "1.2 MB", "5 GB"). Negative sizes return "" — that's a corrupt-row // signal, not something to print to the user. @@ -206,9 +226,9 @@ func RenderAttachmentImage(meta *AttachmentMeta, alt, workspaceSlug string) stri sizeAttrs = fmt.Sprintf(` width="%d" height="%d"`, *meta.Width, *meta.Height) } return fmt.Sprintf(`%s`, - html.EscapeString(src), - html.EscapeString(meta.ID), - html.EscapeString(altText), + escapeHTML(src), + escapeHTML(meta.ID), + escapeHTML(altText), sizeAttrs, ) } @@ -232,14 +252,14 @@ func RenderAttachmentChip(meta *AttachmentMeta, displayText, workspaceSlug strin size := FormatAttachmentSize(meta.SizeBytes) sizeSpan := "" if size != "" { - sizeSpan = ` · ` + html.EscapeString(size) + `` + sizeSpan = ` · ` + escapeHTML(size) + `` } return fmt.Sprintf( - `%s%s`, - html.EscapeString(href), - html.EscapeString(meta.ID), - html.EscapeString(meta.Filename), - html.EscapeString(label), + `%s%s`, + escapeHTML(href), + escapeHTML(meta.ID), + escapeHTML(meta.Filename), + escapeHTML(label), sizeSpan, ) } @@ -256,8 +276,8 @@ func RenderAttachmentMissing(uuid, alt string) string { } return fmt.Sprintf( `📎 %s`, - html.EscapeString(uuid), - html.EscapeString(safeAlt), + escapeHTML(uuid), + escapeHTML(safeAlt), ) } diff --git a/internal/server/render/attachments_test.go b/internal/server/render/attachments_test.go index 01fa11379..f3d48362f 100644 --- a/internal/server/render/attachments_test.go +++ b/internal/server/render/attachments_test.go @@ -1,6 +1,8 @@ package render import ( + "encoding/json" + "os" "strings" "testing" ) @@ -186,7 +188,7 @@ func TestRenderAttachmentChip(t *testing.T) { func TestRenderAttachmentChip_DisplayTextOverridesFilename(t *testing.T) { meta := &AttachmentMeta{ID: "x", MimeType: "application/zip", Filename: "release.zip", SizeBytes: 100} got := RenderAttachmentChip(meta, "Latest release", "ws") - if !strings.Contains(got, ">Latest release") { + if !strings.Contains(got, ">Latest release") { t.Errorf("expected display text override, got: %s", got) } // download attribute always uses canonical filename, not the display text. @@ -423,14 +425,14 @@ func TestResolveAttachmentReferences_EscapedBrackets(t *testing.T) { in: `[A \] B](pad-attachment:doc-1)`, wantSubs: []string{ `data-attachment-id="doc-1"`, - `>A ] B`, + `>A ] B`, }, }, { name: "escaped backslash and bracket combo", in: `[a \\ b \[ c \] d](pad-attachment:doc-1)`, wantSubs: []string{ - `>a \ b [ c ] d`, + `>a \ b [ c ] d`, }, }, } @@ -511,3 +513,78 @@ func TestExtractAttachmentRefs_None(t *testing.T) { t.Errorf("expected nil for content with no refs, got %v", got) } } + +// attachmentParityFixture is shared with web/src/lib/markdown/fileChipParity.test.ts: +// the Go and TS renderers must emit byte-identical chips, images and missing +// placeholders (TASK-4195). +// +// Scope of the URL half (TASK-4195 r3-M1): Go builds download URLs with +// url.PathEscape and TS with encodeURIComponent, which differ on !'()* and +// $&+=:@. The fixture pins parity only for UUID attachment ids (store newID) +// and workspace slugs minted since BUG-3769 ([a-z0-9-], where both encoders are +// the identity). A preserved legacy slug may hold those characters and encode +// differently on each side, but both forms decode to the same route. +type attachmentParityMeta struct { + ID string `json:"id"` + MimeType string `json:"mime_type"` + Filename string `json:"filename"` + SizeBytes int64 `json:"size_bytes"` + Width *int `json:"width"` + Height *int `json:"height"` +} + +func (m attachmentParityMeta) toMeta() *AttachmentMeta { + return &AttachmentMeta{ID: m.ID, MimeType: m.MimeType, Filename: m.Filename, SizeBytes: m.SizeBytes, Width: m.Width, Height: m.Height} +} + +type attachmentParityFixture struct { + Chips []struct { + Name string `json:"name"` + Workspace string `json:"workspace"` + Meta attachmentParityMeta `json:"meta"` + DisplayText string `json:"display_text"` + HTML string `json:"html"` + } `json:"chips"` + Images []struct { + Name string `json:"name"` + Workspace string `json:"workspace"` + Meta attachmentParityMeta `json:"meta"` + Alt string `json:"alt"` + HTML string `json:"html"` + } `json:"images"` + Missing []struct { + Name string `json:"name"` + UUID string `json:"uuid"` + Alt string `json:"alt"` + HTML string `json:"html"` + } `json:"missing"` +} + +func TestRenderAttachment_ParityWithTypeScript(t *testing.T) { + raw, err := os.ReadFile("../../../web/src/lib/markdown/testdata/file-chip-parity.json") + if err != nil { + t.Fatal(err) + } + var fx attachmentParityFixture + if err := json.Unmarshal(raw, &fx); err != nil { + t.Fatal(err) + } + if len(fx.Chips) == 0 || len(fx.Images) == 0 || len(fx.Missing) == 0 { + t.Fatalf("fixture has an empty case array: chips=%d images=%d missing=%d", len(fx.Chips), len(fx.Images), len(fx.Missing)) + } + for _, c := range fx.Chips { + if got := RenderAttachmentChip(c.Meta.toMeta(), c.DisplayText, c.Workspace); got != c.HTML { + t.Errorf("chip %s:\n got %s\n want %s", c.Name, got, c.HTML) + } + } + for _, c := range fx.Images { + if got := RenderAttachmentImage(c.Meta.toMeta(), c.Alt, c.Workspace); got != c.HTML { + t.Errorf("image %s:\n got %s\n want %s", c.Name, got, c.HTML) + } + } + for _, c := range fx.Missing { + if got := RenderAttachmentMissing(c.UUID, c.Alt); got != c.HTML { + t.Errorf("missing %s:\n got %s\n want %s", c.Name, got, c.HTML) + } + } +} diff --git a/web/e2e/attachment-surface-chrome.spec.ts b/web/e2e/attachment-surface-chrome.spec.ts index eaf065182..8d9fa3e41 100644 --- a/web/e2e/attachment-surface-chrome.spec.ts +++ b/web/e2e/attachment-surface-chrome.spec.ts @@ -256,7 +256,7 @@ test.describe('attachment viewer — 3c-i surface chrome (TASK-2484)', () => { // DR-13: the FULL value is in title, and the box CLIPS WITH AN ELLIPSIS — the // resolved style, not just overflow (a clip without `text-overflow: ellipsis` // would still overflow but drop the "…", which jsdom cannot show). - await expect(name).toHaveAttribute('title', long); + await expect(name).toHaveAttribute('title', `\u2068${long}\u2069`); const clip = await name.evaluate((el) => { const cs = getComputedStyle(el); return { diff --git a/web/e2e/attachment-viewer-owners.spec.ts b/web/e2e/attachment-viewer-owners.spec.ts index 68be71f83..16c3b2538 100644 --- a/web/e2e/attachment-viewer-owners.spec.ts +++ b/web/e2e/attachment-viewer-owners.spec.ts @@ -222,7 +222,7 @@ async function openViewer(page: Page): Promise { async function openStripDeleteSheet(page: Page, filename: string): Promise { await page .locator('.attachment-strip .att-delete') - .and(page.getByRole('button', { name: `Delete ${filename}` })) + .and(page.getByRole('button', { name: `Delete \u2068${filename}\u2069` })) .evaluate((el) => (el as HTMLElement).click()); await expect(page.locator('.bs-sheet')).toBeVisible(); } diff --git a/web/e2e/attachment-viewer-parity.spec.ts b/web/e2e/attachment-viewer-parity.spec.ts index 3fbe4a87b..2d67f235a 100644 --- a/web/e2e/attachment-viewer-parity.spec.ts +++ b/web/e2e/attachment-viewer-parity.spec.ts @@ -397,7 +397,7 @@ test.describe('attachment viewer — four-surface parity (TASK-2436)', () => { // tile's delete control carries the same filename, so an unanchored // name matches two controls) and on the comma that follows it in the // label, so a truncated or mangled name cannot satisfy the match. - const tile = page.getByRole('button', { name: new RegExp(`^View ${escapeRe(name)},`) }); + const tile = page.getByRole('button', { name: new RegExp(`^View \\u2068?${escapeRe(name)}\\u2069?,`) }); await expect(tile).toHaveCount(1); await tile.click(); // The dialog names itself for the file (3c-ii T2b): the name now LEADS with @@ -410,7 +410,7 @@ test.describe('attachment viewer — four-surface parity (TASK-2436)', () => { // is caught here — the tolerant addressing matcher above would accept it. await expect(page.locator(VIEWER)).toHaveAttribute( 'aria-label', - new RegExp(`^${escapeRe(name)}, .+`) + new RegExp(`^\\u2068?${escapeRe(name)}\\u2069?, .+`) ); await page.keyboard.press('Escape'); await expect(page.locator(VIEWER)).toHaveCount(0); @@ -421,7 +421,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(bidiStored)},`) }).click(); + await page.getByRole('button', { name: new RegExp(`^View \\u2068?${escapeRe(bidiStored)}\\u2069?,`) }).click(); await expect(page.locator(VIEWER)).toHaveCount(1); const rtl = await page.evaluate(() => { const el = document.querySelector('.attachment-viewer')!; diff --git a/web/e2e/item-attachment-strip.spec.ts b/web/e2e/item-attachment-strip.spec.ts index 180f53025..2beb5eb6a 100644 --- a/web/e2e/item-attachment-strip.spec.ts +++ b/web/e2e/item-attachment-strip.spec.ts @@ -364,7 +364,7 @@ test.describe('item attachment strip', () => { // A file tile is a real button naming its action, not a download link — // the whole point of the change (DR-1, DR-12). await expect(tile).toHaveJSProperty('tagName', 'BUTTON'); - await expect(tile).toHaveAttribute('aria-label', /^Options for notes\.zip/); + await expect(tile).toHaveAttribute('aria-label', /^Options for \u2068notes\.zip/); await tile.click(); // The converged surface opens — a role="dialog" naming the file, NOT a menu. diff --git a/web/e2e/lib/attachment-viewer.ts b/web/e2e/lib/attachment-viewer.ts index 7c01cadde..4d1918c0c 100644 --- a/web/e2e/lib/attachment-viewer.ts +++ b/web/e2e/lib/attachment-viewer.ts @@ -207,7 +207,7 @@ export function escapeRe(s: string): string { * lives in ONE place. */ export const viewerDialog = (page: Page, name: string) => - page.getByRole('dialog', { name: new RegExp(`^${escapeRe(name)}(,|$)`) }); + page.getByRole('dialog', { name: new RegExp(`^\\u2068?${escapeRe(name)}\\u2069?(,|$)`) }); /** * The frontmost viewer's TABBABLE controls, in DOM (trap) order, as their diff --git a/web/src/lib/attachments/bidiIsolation.svelte.test.ts b/web/src/lib/attachments/bidiIsolation.svelte.test.ts new file mode 100644 index 000000000..20d062256 --- /dev/null +++ b/web/src/lib/attachments/bidiIsolation.svelte.test.ts @@ -0,0 +1,206 @@ +// TASK-4195: a file name is data a stranger controls; wherever it is shown it +// must be isolated so a right-to-left override inside it cannot reorder the +// text around it. Per-component sites have their own tests (strip, storage, +// lightbox, chip); this file covers the helpers and the string/markup sites +// that need no component mount. +import { describe, it, expect } from 'vitest'; +import { Editor } from '@tiptap/core'; +import StarterKit from '@tiptap/starter-kit'; +import { Markdown } from 'tiptap-markdown'; +import { AttachmentImage } from '$lib/components/editor/attachment-image'; +import { + displayFilename, + displayFilenameIsolated, + isolateText +} from './display'; +import { + EVIL_NAME, + FSI, + PDI, + expectFsiPdi, + expectIsolated, + expectNoIsolates +} from './bidiIsolation.testutil'; +import { + attachmentDeletePrompt, + workspaceAttachmentDeletePrompt +} from '$lib/components/attachments/AttachmentDeleteConfirm.svelte'; +import { + buildPlaceholder, + storedNotInsertedMessage, + uploadFailedMessage +} from '$lib/components/editor/attachment-upload'; +import { renderAttachmentChip, renderAttachmentImage } from '$lib/markdown/attachments'; +import { + renderMarkedWithAttachments, + renderMarkdown, + sanitizeMarkdownHtml +} from '$lib/utils/markdown'; +import { restrictShareMarkup } from '$lib/components/share/shareRender'; + +const ID = '11111111-2222-3333-4444-555555555555'; +const meta = (filename: string) => ({ + id: ID, + mime_type: 'application/pdf', + filename, + size_bytes: 10, + width: null, + height: null +}); + +describe('isolateText / displayFilenameIsolated', () => { + it('wraps in FSI .. PDI', () => { + expect(isolateText('x')).toBe(FSI + 'x' + PDI); + expect(FSI).toBe(String.fromCharCode(0x2068)); + expect(PDI).toBe(String.fromCharCode(0x2069)); + }); + it('isolates the normalized name, including the blank fallback', () => { + expectFsiPdi(displayFilenameIsolated(EVIL_NAME), EVIL_NAME); + expect(displayFilenameIsolated(' ')).toBe(FSI + 'Untitled file' + PDI); + }); + it('GUARD: displayFilename itself stays isolate-free (it feeds data seeds)', () => { + expectNoIsolates(displayFilename(EVIL_NAME)); + expect(displayFilename(EVIL_NAME)).toBe(EVIL_NAME); + }); +}); + +describe('delete prompts (site 15)', () => { + it('isolate the name in both prompts', () => { + expectFsiPdi(attachmentDeletePrompt(EVIL_NAME, true), EVIL_NAME); + expectFsiPdi(attachmentDeletePrompt(EVIL_NAME, false), EVIL_NAME); + expectFsiPdi(workspaceAttachmentDeletePrompt(EVIL_NAME), EVIL_NAME); + }); +}); + +describe('attachment-upload (sites 7, 16)', () => { + it('the Uploading placeholder keeps the name inside a bdi', () => { + const el = buildPlaceholder(EVIL_NAME); + expectIsolated(el, EVIL_NAME); + expect(el.textContent).toContain('Uploading '); + }); + it('the stored-not-inserted notice isolates the name only', () => { + const msg = storedNotInsertedMessage(EVIL_NAME); + expectFsiPdi(msg, EVIL_NAME); + expect(msg.startsWith(FSI + EVIL_NAME + PDI + ' was uploaded')).toBe(true); + }); + // Site 17 (TASK-4195 r1-L2): the upload-error toast in Editor and + // CommentEditor. The name is isolated; the message is shown as received. + it('the upload-failed toast isolates the name only', () => { + const msg = uploadFailedMessage(EVIL_NAME, 'too large'); + expectFsiPdi(msg, EVIL_NAME); + expect(msg).toBe("Couldn't upload " + FSI + EVIL_NAME + PDI + ': too large'); + }); +}); + +// Site 17 binding (CONVE-19): no test mounts the full Editor's upload error +// path, so pin that both toast sites build their text with the tested helper +// rather than an inline template that could drop the isolation. +const toastSites = import.meta.glob( + ['../components/editor/Editor.svelte', '../components/CommentEditor.svelte'], + { query: '?raw', import: 'default', eager: true } +) as Record; + +describe('upload-error toast sites (site 17)', () => { + it('finds both sources', () => { + expect(Object.keys(toastSites)).toHaveLength(2); + }); + it.each(Object.entries(toastSites))('%s toasts through uploadFailedMessage', (_name, src) => { + expect(src).toContain('toastStore.show(uploadFailedMessage(filename, message), '); + expect(src).not.toContain("Couldn't upload"); + }); +}); + +describe('markdown file chip (site 6, 12)', () => { + function mount(html: string): HTMLElement { + const d = document.createElement('div'); + d.innerHTML = html; + return d; + } + it('keeps the bdi through the sanitizer, with download= untouched', () => { + const raw = renderAttachmentChip(meta(EVIL_NAME), EVIL_NAME, 'ws'); + const clean = sanitizeMarkdownHtml(raw); + expectIsolated(mount(clean), EVIL_NAME); + const a = mount(clean).querySelector('a.file-chip')!; + expect(a.getAttribute('download')).toBe(EVIL_NAME); + expectNoIsolates(a.getAttribute('download')); + expectNoIsolates(clean); + }); + it('keeps the bdi through the share restrict pass', () => { + const raw = renderAttachmentChip(meta(EVIL_NAME), EVIL_NAME, 'ws'); + const shared = restrictShareMarkup(sanitizeMarkdownHtml(raw)); + expectIsolated(mount(shared), EVIL_NAME); + }); + it('renders through the full body pipeline', () => { + const html = renderMarkedWithAttachments(`[${EVIL_NAME}](pad-attachment:${ID})`, { + resolver: () => meta(EVIL_NAME), + workspaceSlug: 'ws', + urlBuilder: (u: string) => `/a/${u}` + }); + expectIsolated(mount(sanitizeMarkdownHtml(html)), EVIL_NAME); + }); + it('never isolates an image alt: the filename fallback and authored alt are both data', () => { + const img = { ...meta(EVIL_NAME), mime_type: 'image/png', width: 1, height: 1 }; + const fallback = mount(renderAttachmentImage(img, '', 'ws')).querySelector('img')!; + expect(fallback.getAttribute('alt')).toBe(EVIL_NAME); + expectNoIsolates(fallback.getAttribute('alt')); + const authored = mount(renderAttachmentImage(img, 'my alt', 'ws')).querySelector('img')!; + expect(authored.getAttribute('alt')).toBe('my alt'); + }); + it('an empty authored alt AND an empty filename renders alt="" with no isolates', () => { + const img = { ...meta(''), mime_type: 'image/png', width: 1, height: 1 }; + const el = mount(renderAttachmentImage(img, '', 'ws')).querySelector('img')!; + expect(el.getAttribute('alt')).toBe(''); + expectNoIsolates(el.outerHTML); + }); +}); + +describe('image alt round trip through the editor (site 12)', () => { + function roundTrip(html: string): { alt: string | null; markdown: string } { + const editor = new Editor({ + element: null, + extensions: [ + StarterKit, + Markdown.configure({ html: true }), + AttachmentImage.configure({ workspaceSlug: 'ws', getDownloadUrl: (u: string) => `/a/${u}` }) + ], + content: `

${html}

` + }); + let alt: string | null = null; + editor.state.doc.descendants((n) => { + if (n.type.name === 'attachmentImage') alt = n.attrs.alt as string | null; + }); + const markdown = (editor.storage as { markdown: { getMarkdown(): string } }).markdown.getMarkdown(); + editor.destroy(); + return { alt, markdown }; + } + const img = { ...meta('photo.png'), mime_type: 'image/png', width: 1, height: 1 }; + it.each([ + ['the filename fallback', '', 'photo.png'], + ['authored alt', 'my alt', 'my alt'] + ])('%s comes back byte-identical, with no isolates', (_n, alt, want) => { + const out = roundTrip(renderAttachmentImage(img, alt, 'ws')); + expect(out.alt).toBe(want); + expectNoIsolates(out.alt); + expectNoIsolates(out.markdown); + expect(out.markdown).toBe(`![${want}](pad-attachment:${ID})`); + }); +}); + +describe('bdi in the markdown sanitizer allowlist (Q2)', () => { + it('survives, with every attribute it does not allowlist stripped, and is inert', () => { + const dirty = 'hi'; + for (const out of [sanitizeMarkdownHtml(dirty), renderMarkdown(dirty)]) { + const bdi = new DOMParser().parseFromString(out, 'text/html').querySelector('bdi')!; + expect(bdi, out).not.toBeNull(); + expect(bdi.textContent).toBe('hi'); + for (const attr of ['onclick', 'style', 'dir', 'onmouseover']) { + expect(bdi.hasAttribute(attr), attr).toBe(false); + } + } + }); + it('does not let a script ride along inside a user-authored bdi', () => { + const out = sanitizeMarkdownHtml(''); + expect(out).not.toContain('` (visible-text sites). */ +export function expectIsolated(root: Node, name: string): void { + const node = textNodeContaining(root, name); + expect(node, `no text node containing the name`).not.toBeNull(); + expect(node!.parentElement!.closest('bdi'), 'name text is not inside a ').not.toBeNull(); +} + +/** The string carries FSI, then the name, then PDI (attribute and toast sites). */ +export function expectFsiPdi(str: string | null | undefined, name: string): void { + expect(str ?? '').toContain(FSI + name + PDI); +} + +/** The string carries no isolate controls (data sites: download=, node attrs). */ +export function expectNoIsolates(str: string | null | undefined): void { + expect(str ?? '').not.toContain(FSI); + expect(str ?? '').not.toContain(PDI); +} diff --git a/web/src/lib/attachments/display.ts b/web/src/lib/attachments/display.ts index f4d22634e..ed0e1ca85 100644 --- a/web/src/lib/attachments/display.ts +++ b/web/src/lib/attachments/display.ts @@ -44,6 +44,22 @@ export function displayFilename(filename: string | null | undefined): string { return trimmed || 'Untitled file'; } +/** + * Wrap text in a first-strong isolate (U+2068 .. U+2069) so a file name with + * bidi controls (a right-to-left override, say) cannot reorder the text around + * it. For attribute and plain-text strings (title, aria-label, toasts), where + * `` markup cannot go. Display only: never use it for `download=`, node + * attrs, data attributes or any value that is stored or parsed back. + */ +export function isolateText(s: string): string { + return '\u2068' + s + '\u2069'; +} + +/** `displayFilename`, isolated for a sentence, attribute or toast. */ +export function displayFilenameIsolated(filename: string | null | undefined): string { + return isolateText(displayFilename(filename)); +} + export function formatBytes(bytes: number): string { if (bytes < 0) return `${bytes} B`; const KB = 1024; diff --git a/web/src/lib/components/CommentEditor.svelte b/web/src/lib/components/CommentEditor.svelte index a160e1dc0..91c95046a 100644 --- a/web/src/lib/components/CommentEditor.svelte +++ b/web/src/lib/components/CommentEditor.svelte @@ -30,7 +30,7 @@ import { unescapeDocLinks } from '$lib/utils/markdown'; import { AttachmentImage } from './editor/attachment-image'; import { AttachmentChip } from './editor/attachment-chip'; - import { AttachmentUpload } from './editor/attachment-upload'; + import { AttachmentUpload, uploadFailedMessage } from './editor/attachment-upload'; import type { AttachmentHostAddress } from '$lib/attachments/hostAddress'; interface Props { @@ -290,7 +290,7 @@ if (message === PLAN_LIMIT_PROMPTED) return; console.error(`[comment attachment] ${filename}: ${message}`); // A toast, not a blocking window.alert (TASK-2221, audit C39). - toastStore.show(`Couldn't upload ${filename}: ${message}`, 'error'); + toastStore.show(uploadFailedMessage(filename, message), 'error'); }, // An upload that finished after this composer unmounted: when a // pane opens over the timeline, when the user navigates away. The diff --git a/web/src/lib/components/attachments/AttachmentDeleteConfirm.svelte b/web/src/lib/components/attachments/AttachmentDeleteConfirm.svelte index 76a95af83..7e099256d 100644 --- a/web/src/lib/components/attachments/AttachmentDeleteConfirm.svelte +++ b/web/src/lib/components/attachments/AttachmentDeleteConfirm.svelte @@ -38,7 +38,7 @@ a change to one is read alongside the other. --> diff --git a/web/src/lib/components/common/Lightbox.svelte b/web/src/lib/components/common/Lightbox.svelte index f910ec765..48d25feb0 100644 --- a/web/src/lib/components/common/Lightbox.svelte +++ b/web/src/lib/components/common/Lightbox.svelte @@ -110,7 +110,8 @@ describeAttachmentType, formatBytes, iconForAttachment, - isMarkdownAttachment + isMarkdownAttachment, + isolateText } from '$lib/attachments/display'; import { createSurfaceMetadata } from '$lib/attachments/surfaceMetadata.svelte'; import { backdropDismiss } from '$lib/utils/backdropDismiss'; @@ -445,12 +446,14 @@ // string. Shared by the metadata header, the download attribute and the delete // prompt, so all three name the file identically. let displayName = $derived(img?.filename ?? blankToNull(img?.alt) ?? 'Attachment'); + // FSI..PDI-wrapped copy for attribute / title text, where markup (``) cannot go. + let displayNameIsolated = $derived(isolateText(displayName)); // The dialog's accessible NAME (3c-ii T2b): the file's display name plus the // type · size the header shows, so a screen reader announces WHAT is open — the // metadata the retired panel exposed — not a bare alt. Never empty (`displayName` // always resolves), so an unnamed `role="dialog"` — announced as nothing — can't // happen. - let dialogLabel = $derived([displayName, headerDetail].filter(Boolean).join(', ')); + let dialogLabel = $derived([displayNameIsolated, headerDetail].filter(Boolean).join(', ')); // Inline, transient error from a button action (a failed clipboard write). Sits // in the toolbar beside the controls, never a blocking dialog. let toolbarError = $state(null); @@ -2783,7 +2786,7 @@
{#if textLoader.phase === 'loading'} @@ -2820,7 +2823,7 @@
{:else if textLoader.phase === 'ready'} {#if shownRenderer === 'table'} - + {:else}
{#if markdownAttachment} @@ -2854,7 +2857,7 @@ --> {#if shownRenderer === 'pdf' && !soleMissing}
-
+
{#if bytesLoader.phase === 'loading'}

Loading preview…

{:else if bytesLoader.phase === 'too-large'} @@ -2886,7 +2889,7 @@ bind:this={pdfPreview} bytes={bytesLoader.bytes} compact={isSheet} - label={`${displayName} PDF`} + label={`${displayNameIsolated} PDF`} /> {/key} {/if} @@ -2917,7 +2920,7 @@ -

{displayName}

+

{displayName}

{#if headerDetail}

{headerDetail}

{/if} @@ -2939,7 +2942,7 @@ -

{displayName}

+

{displayName}

This file is no longer available. It may have been deleted.

{/if} @@ -2963,7 +2966,7 @@ --> {#if img}
-
{displayName}
+
{displayName}
{#if headerDetail}
{headerDetail}
{/if} diff --git a/web/src/lib/components/common/Lightbox.svelte.test.ts b/web/src/lib/components/common/Lightbox.svelte.test.ts index 900d52078..0d675a8b1 100644 --- a/web/src/lib/components/common/Lightbox.svelte.test.ts +++ b/web/src/lib/components/common/Lightbox.svelte.test.ts @@ -1,6 +1,14 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { flushSync, mount, unmount } from 'svelte'; import Lightbox from './Lightbox.svelte'; +import { renderAttachmentImage } from '$lib/markdown/attachments'; +import { + EVIL_NAME, + FSI, + PDI, + expectFsiPdi, + expectIsolated +} from '$lib/attachments/bidiIsolation.testutil'; import type { LightboxImage } from '$lib/attachments/events'; // TASK-2477 — the deletion bus is REAL in this suite (the viewer subscribes to // it); firing `notifyAttachmentDeleted` drives the survivor logic directly. @@ -312,7 +320,7 @@ describe('Lightbox — dialog semantics', () => { // is absent here (the seed carries none and the probe is async), so the label // is "name, type". const type = describeAttachmentType('image/png', null); - expect(root().getAttribute('aria-label')).toBe(`a diagram, ${type}`); + expect(root().getAttribute('aria-label')).toBe(`${FSI}a diagram${PDI}, ${type}`); }); it('falls back to a generic name when the image has no alt', () => { @@ -321,7 +329,7 @@ describe('Lightbox — dialog semantics', () => { // ("Attachment") plus the type. mountViewer({ images: [image(IMG_A, '')] }); const type = describeAttachmentType('image/png', null); - expect(root().getAttribute('aria-label')).toBe(`Attachment, ${type}`); + expect(root().getAttribute('aria-label')).toBe(`${FSI}Attachment${PDI}, ${type}`); }); it('gives every control a real accessible name, not a glyph', () => { @@ -352,10 +360,10 @@ describe('Lightbox — dialog semantics', () => { ], }); const type = describeAttachmentType('image/png', null); - expect(root().getAttribute('aria-label')).toBe(`first, ${type}`); + expect(root().getAttribute('aria-label')).toBe(`${FSI}first${PDI}, ${type}`); root().querySelector('.lightbox-nav.next')!.click(); flushSync(); - expect(root().getAttribute('aria-label')).toBe(`second, ${type}`); + expect(root().getAttribute('aria-label')).toBe(`${FSI}second${PDI}, ${type}`); }); }); @@ -4548,6 +4556,21 @@ describe('Lightbox — metadata header (TASK-2475)', () => { expect(metaName()).toBe('Attachment'); }); + it('an image rendered with empty alt and empty filename seeds the generic label, not isolates', async () => { + // The timeline seeds this image from the rendered 's alt attribute (viewerImageFor). + const d = document.createElement('div'); + d.innerHTML = renderAttachmentImage( + { id: 'a1', mime_type: 'image/png', filename: '', size_bytes: 1, width: 1, height: 1 }, + '', + 'ws' + ); + const seededAlt = d.querySelector('img')!.getAttribute('alt') ?? ''; + expect(seededAlt).toBe(''); + mountViewer({ images: [metaImage({ filename: null, alt: seededAlt })] }); + await settleAsync(); + expect(metaName()).toBe('Attachment'); + }); + it('treats a whitespace-only alt as blank too, reaching the generic label', async () => { // The chain blank-normalizes alt, not just filename — an all-spaces alt must // not win over 'Attachment' as an empty-but-present string would. @@ -4629,7 +4652,7 @@ describe('Lightbox — metadata header (TASK-2475)', () => { // (overflow/text-overflow/nowrap on `.lightbox-meta-name`) is not applied by // jsdom's `getComputedStyle` (it ignores scoped `