From c74d0ecc94415cd0a108198b574dbe860438e5ab Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Tue, 5 May 2026 17:51:28 -0700 Subject: [PATCH] test(canvas/chat): cover platform-pending: branch + isPlatformAttachment (#2973) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #2973 — the followup test gap I flagged on PR #2968's review. Pre-merge #2968 added the platform-pending: URI scheme branch to resolveAttachmentHref + introduced the isPlatformAttachment SSOT helper, but the existing uploads.test.ts only covered the older workspace: / file:/// / absolute-path branches. The new branch shipped on prod-impact (live console error on reno-stars) with manual post- deploy verification; the regression gate was filed as a followup (#2973) so a future canvas refactor can't silently re-break the poll-mode chat-attachment download path. Adds 15 new test cases across two existing describe blocks: resolveAttachmentHref — platform-pending: scheme (poll-mode uploads): - well-formed platform-pending:/ resolves to the /pending-uploads//content endpoint - uses the URI's wsid, NOT the chat workspace_id (cross-workspace forwarding case — pinning the explicit decision from #2968's commit message so a regression that flipped this would mis-route the download to the wrong workspace's pending-uploads store) - defensive fallback to raw URI on missing slash, empty fileID, empty wsid (so a future "helpful" change can't synthesize a broken /pending-uploads// path) - regression test against the EXACT production repro from #2968's body (reno-stars, 2026-05-05 console error) isPlatformAttachment: - positive cases for platform-pending: (well-formed and malformed), workspace:, file:///, absolute paths under allowed roots - NEGATIVE cases for HTTPS/HTTP URLs to other origins (auth-leak class regression — a helper that always returned true would attach workspace tokens to third-party requests), non-allowlisted roots like /etc/passwd or /var/log/x, empty string, and unrecognised schemes (s3://, ftp://) All 21 tests pass. The 6 pre-existing tests are unchanged. The 15 new tests are the regression gate that #2973 asked for. Verification: - pnpm exec vitest run src/components/tabs/chat/__tests__/uploads.test.ts → 21 passed --- .../tabs/chat/__tests__/uploads.test.ts | 127 +++++++++++++++++- 1 file changed, 126 insertions(+), 1 deletion(-) diff --git a/canvas/src/components/tabs/chat/__tests__/uploads.test.ts b/canvas/src/components/tabs/chat/__tests__/uploads.test.ts index a08d5d19..54a298a1 100644 --- a/canvas/src/components/tabs/chat/__tests__/uploads.test.ts +++ b/canvas/src/components/tabs/chat/__tests__/uploads.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from "vitest"; -import { resolveAttachmentHref } from "../uploads"; +import { isPlatformAttachment, resolveAttachmentHref } from "../uploads"; describe("resolveAttachmentHref — URI scheme normalisation", () => { const wsId = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"; @@ -39,3 +39,128 @@ describe("resolveAttachmentHref — URI scheme normalisation", () => { expect(resolveAttachmentHref(wsId, "s3://bucket/key")).toBe("s3://bucket/key"); }); }); + +// #2973 follow-up to #2968: cover the platform-pending: scheme branch +// (poll-mode chat uploads) + the isPlatformAttachment SSOT helper that +// the chip-download and markdown-link paths both consume. +// +// Pre-fix the platform-pending: URI fell through to the raw URI → +// browser saw an unhandled-protocol click → about:blank. The fix +// resolves it to the platform pending-uploads endpoint with auth +// headers attached. +describe("resolveAttachmentHref — platform-pending: scheme (poll-mode uploads)", () => { + // Use a chat workspace ID that DIFFERS from the one in the URI, so + // tests can verify which one the resolver uses. The forward-across- + // workspace case is real production behavior — files dragged into one + // workspace's chat can be referenced from another. + const chatWs = "chat-ws-aaaaaaaa"; + const sourceWs = "source-ws-bbbbbbbb"; + + it("resolves a well-formed platform-pending: URI to /pending-uploads//content", () => { + const url = resolveAttachmentHref( + chatWs, + `platform-pending:${sourceWs}/file-12345`, + ); + expect(url).toContain(`/workspaces/${sourceWs}/pending-uploads/file-12345/content`); + }); + + it("uses the URI's wsid, NOT the chat workspace_id (cross-workspace forwarding)", () => { + // The two ids differ — this is the case PR #2968's commit + // explicitly calls out. A regression that flipped this would + // silently mis-route the download to the WRONG workspace's + // pending-uploads store, returning 404 (or worse, leaking). + const url = resolveAttachmentHref( + chatWs, + `platform-pending:${sourceWs}/file-xyz`, + ); + expect(url).toContain(`/workspaces/${sourceWs}/`); + expect(url).not.toContain(`/workspaces/${chatWs}/`); + }); + + it("falls back to raw URI when platform-pending: is missing the slash", () => { + // Defensive: a URI that drifted from the expected wsid/fileid shape + // returns raw rather than producing a broken /pending-uploads// + // path. Pinned to detect a regression where a future "helpful" + // change synthesizes empty wsid/fileID. + expect(resolveAttachmentHref(chatWs, "platform-pending:no-slash")).toBe( + "platform-pending:no-slash", + ); + }); + + it("falls back to raw URI when platform-pending: has empty fileID", () => { + expect(resolveAttachmentHref(chatWs, "platform-pending:abc/")).toBe( + "platform-pending:abc/", + ); + }); + + it("falls back to raw URI when platform-pending: has empty wsid", () => { + expect(resolveAttachmentHref(chatWs, "platform-pending:/file-xyz")).toBe( + "platform-pending:/file-xyz", + ); + }); + + it("regression: exact production repro from #2968 (reno-stars)", () => { + // From the original PR #2968 body: the chat's markdown-link + // override fell through on this exact shape and the browser + // navigated to about:blank. Pin the post-fix output so a future + // refactor can't reintroduce the original bug. + const url = resolveAttachmentHref( + "chat-ws", + "platform-pending:d76977b1-uuid/bb0dcaf3-uuid", + ); + expect(url).toContain("/workspaces/d76977b1-uuid/pending-uploads/bb0dcaf3-uuid/content"); + expect(url).not.toContain("chat-ws"); + }); +}); + +describe("isPlatformAttachment", () => { + it("returns true for platform-pending: URIs", () => { + expect(isPlatformAttachment("platform-pending:abc/file")).toBe(true); + }); + + it("returns true even for malformed platform-pending: URIs", () => { + // The helper is a SHAPE check — caller routes through + // downloadChatFile and downloadChatFile handles the malformed case + // downstream. Pinning so a future helper that "validates" the + // wsid/fileID shape doesn't silently break the auth-attached + // download flow for in-flight URIs. + expect(isPlatformAttachment("platform-pending:no-slash")).toBe(true); + }); + + it("returns true for workspace: URIs", () => { + expect(isPlatformAttachment("workspace:/configs/foo")).toBe(true); + expect(isPlatformAttachment("workspace:/workspace/x.pdf")).toBe(true); + }); + + it("returns true for file:/// URIs", () => { + expect(isPlatformAttachment("file:///workspace/x")).toBe(true); + }); + + it("returns true for absolute paths under allowed roots", () => { + expect(isPlatformAttachment("/home/user/x")).toBe(true); + expect(isPlatformAttachment("/configs/y")).toBe(true); + }); + + it("returns FALSE for bare HTTPS URLs to other origins", () => { + // Auth-leak class regression: a helper that always returned true + // would attach workspace tokens to third-party requests. Pin + // the negative case explicitly. + expect(isPlatformAttachment("https://example.com/file")).toBe(false); + expect(isPlatformAttachment("http://example.com/file")).toBe(false); + }); + + it("returns FALSE for non-allowlisted root paths", () => { + expect(isPlatformAttachment("/etc/passwd")).toBe(false); + expect(isPlatformAttachment("/var/log/x")).toBe(false); + expect(isPlatformAttachment("/tmp/x")).toBe(false); + }); + + it("returns FALSE for empty string", () => { + expect(isPlatformAttachment("")).toBe(false); + }); + + it("returns FALSE for unrecognised schemes", () => { + expect(isPlatformAttachment("s3://bucket/key")).toBe(false); + expect(isPlatformAttachment("ftp://server/file")).toBe(false); + }); +});