From 501d07b0f20fcda465a833b813b87ebdd89e9325 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 7 May 2026 14:36:02 -0700 Subject: [PATCH] fix(canvas): consolidate platform-auth headers via shared helper (#178) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the post-Task-#176 self-review gap: the bearer-token + tenant- slug header construction was duplicated across 7 raw-fetch callsites in the canvas (lib/api.ts request(), uploads.ts × 2, and 5 Attachment* components). Each callsite read NEXT_PUBLIC_ADMIN_TOKEN, attached Authorization: Bearer manually, computed getTenantSlug locally (three of them inline-redefined it from /lib/tenant!), and attached X-Molecule-Org-Slug. A new poller / raw-fetch added without going through this exact recipe silently 401s against workspace-server when ADMIN_TOKEN is set on the server side — the bug shape called out in the original task. Adds platformAuthHeaders() to lib/api.ts as the single source of truth and routes all 7 raw-fetch callsites through it. Removes 4 duplicate local getTenantSlug() copies (Image, Video, Audio, PDF, TextPreview) that were inline-redefining what /lib/tenant.ts already exports. Also preserves the AttachmentTextPreview off-platform branch — when isPlatformAttachment() is false, headers is {} (no bearer leakage to third-party URLs). Tests: - 6 unit tests in platform-auth-headers.test.ts covering: empty, bearer-only, slug-only, both, empty-string-as-unset, fresh-object- per-call. Mutation-tested: removing the bearer attach inside the helper fails 2 of 6 tests immediately. - All 1389 existing canvas vitest tests pass unchanged. - npx tsc --noEmit clean. - npm run build succeeds (canvas Next.js build). Per feedback_assert_exact_not_substring: tests use exact toEqual() equality, not substring/contains, so an extra-header bug also fails the assertion. Per feedback_oss_design_philosophy: this is the "plugin/abstract/modular/SSOT" move applied to the auth-header construction surface — one helper, six call sites, no duplication. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../components/tabs/chat/AttachmentAudio.tsx | 16 +-- .../components/tabs/chat/AttachmentImage.tsx | 33 ++----- .../components/tabs/chat/AttachmentPDF.tsx | 16 +-- .../tabs/chat/AttachmentTextPreview.tsx | 23 ++--- .../components/tabs/chat/AttachmentVideo.tsx | 18 +--- canvas/src/components/tabs/chat/uploads.ts | 30 +++--- .../__tests__/platform-auth-headers.test.ts | 97 +++++++++++++++++++ canvas/src/lib/api.ts | 58 +++++++++-- 8 files changed, 187 insertions(+), 104 deletions(-) create mode 100644 canvas/src/lib/__tests__/platform-auth-headers.test.ts diff --git a/canvas/src/components/tabs/chat/AttachmentAudio.tsx b/canvas/src/components/tabs/chat/AttachmentAudio.tsx index b696125d..d81ee80a 100644 --- a/canvas/src/components/tabs/chat/AttachmentAudio.tsx +++ b/canvas/src/components/tabs/chat/AttachmentAudio.tsx @@ -9,6 +9,7 @@ // AttachmentLightbox). import { useState, useEffect, useRef } from "react"; +import { platformAuthHeaders } from "@/lib/api"; import type { ChatAttachment } from "./types"; import { isPlatformAttachment, resolveAttachmentHref } from "./uploads"; import { AttachmentChip } from "./AttachmentViews"; @@ -43,13 +44,8 @@ export function AttachmentAudio({ workspaceId, attachment, onDownload, tone }: P void (async () => { try { const href = resolveAttachmentHref(workspaceId, attachment.uri); - const headers: Record = {}; - const adminToken = process.env.NEXT_PUBLIC_ADMIN_TOKEN; - if (adminToken) headers["Authorization"] = `Bearer ${adminToken}`; - const slug = getTenantSlug(); - if (slug) headers["X-Molecule-Org-Slug"] = slug; const res = await fetch(href, { - headers, + headers: platformAuthHeaders(), credentials: "include", signal: AbortSignal.timeout(60_000), }); @@ -116,9 +112,5 @@ export function AttachmentAudio({ workspaceId, attachment, onDownload, tone }: P ); } -function getTenantSlug(): string | null { - if (typeof window === "undefined") return null; - const host = window.location.hostname; - const m = host.match(/^([^.]+)\.moleculesai\.app$/); - return m ? m[1] : null; -} +// Local getTenantSlug() removed — auth-header construction now goes +// through platformAuthHeaders() from @/lib/api (#178). diff --git a/canvas/src/components/tabs/chat/AttachmentImage.tsx b/canvas/src/components/tabs/chat/AttachmentImage.tsx index a219cb80..ca4df242 100644 --- a/canvas/src/components/tabs/chat/AttachmentImage.tsx +++ b/canvas/src/components/tabs/chat/AttachmentImage.tsx @@ -35,6 +35,7 @@ // downscale via canvas, but defer that to v2. import { useState, useEffect, useRef } from "react"; +import { platformAuthHeaders } from "@/lib/api"; import type { ChatAttachment } from "./types"; import { isPlatformAttachment, resolveAttachmentHref } from "./uploads"; import { AttachmentLightbox } from "./AttachmentLightbox"; @@ -75,22 +76,14 @@ export function AttachmentImage({ workspaceId, attachment, onDownload, tone }: P } // Platform-auth path: identical to downloadChatFile but we keep - // the blob (don't trigger a Save-As). Use the same headers it does - // by going through it indirectly — no, downloadChatFile triggers a - // Save-As. Need a separate fetch. + // the blob (don't trigger a Save-As). Auth headers come from the + // shared `platformAuthHeaders()` helper — one source of truth for + // every authenticated raw fetch in the canvas (#178). void (async () => { try { const href = resolveAttachmentHref(workspaceId, attachment.uri); - const headers: Record = {}; - // Read the same env var downloadChatFile reads — single source - // of truth would be cleaner; refactor opportunity for PR-2 if - // we add the same path to AttachmentVideo. - const adminToken = process.env.NEXT_PUBLIC_ADMIN_TOKEN; - if (adminToken) headers["Authorization"] = `Bearer ${adminToken}`; - const slug = getTenantSlug(); - if (slug) headers["X-Molecule-Org-Slug"] = slug; const res = await fetch(href, { - headers, + headers: platformAuthHeaders(), credentials: "include", signal: AbortSignal.timeout(30_000), }); @@ -184,15 +177,7 @@ export function AttachmentImage({ workspaceId, attachment, onDownload, tone }: P ); } -// Internal helper — duplicated from uploads.ts (it's not exported -// there). Kept local so this component doesn't reach into private -// surface; if AttachmentVideo / AttachmentPDF in PR-2/PR-3 also need -// it, lift to an exported helper at that point (the third-caller -// rule). -function getTenantSlug(): string | null { - if (typeof window === "undefined") return null; - const host = window.location.hostname; - // Tenant subdomain shape: .moleculesai.app - const m = host.match(/^([^.]+)\.moleculesai\.app$/); - return m ? m[1] : null; -} +// Local getTenantSlug() removed — auth-header construction now goes +// through platformAuthHeaders() from @/lib/api which uses the canonical +// getTenantSlug() from @/lib/tenant. This eliminates the duplicate +// hostname-regex + the duplicate bearer-token-attach pattern (#178). diff --git a/canvas/src/components/tabs/chat/AttachmentPDF.tsx b/canvas/src/components/tabs/chat/AttachmentPDF.tsx index efe5fc90..784aa444 100644 --- a/canvas/src/components/tabs/chat/AttachmentPDF.tsx +++ b/canvas/src/components/tabs/chat/AttachmentPDF.tsx @@ -33,6 +33,7 @@ // timeout, swap to chip. Implemented as a 3-second watchdog. import { useState, useEffect, useRef } from "react"; +import { platformAuthHeaders } from "@/lib/api"; import type { ChatAttachment } from "./types"; import { isPlatformAttachment, resolveAttachmentHref } from "./uploads"; import { AttachmentLightbox } from "./AttachmentLightbox"; @@ -69,13 +70,8 @@ export function AttachmentPDF({ workspaceId, attachment, onDownload, tone }: Pro void (async () => { try { const href = resolveAttachmentHref(workspaceId, attachment.uri); - const headers: Record = {}; - const adminToken = process.env.NEXT_PUBLIC_ADMIN_TOKEN; - if (adminToken) headers["Authorization"] = `Bearer ${adminToken}`; - const slug = getTenantSlug(); - if (slug) headers["X-Molecule-Org-Slug"] = slug; const res = await fetch(href, { - headers, + headers: platformAuthHeaders(), credentials: "include", signal: AbortSignal.timeout(60_000), }); @@ -189,9 +185,5 @@ function PdfGlyph() { ); } -function getTenantSlug(): string | null { - if (typeof window === "undefined") return null; - const host = window.location.hostname; - const m = host.match(/^([^.]+)\.moleculesai\.app$/); - return m ? m[1] : null; -} +// Local getTenantSlug() removed — auth-header construction now goes +// through platformAuthHeaders() from @/lib/api (#178). diff --git a/canvas/src/components/tabs/chat/AttachmentTextPreview.tsx b/canvas/src/components/tabs/chat/AttachmentTextPreview.tsx index 41a92a45..c10c3098 100644 --- a/canvas/src/components/tabs/chat/AttachmentTextPreview.tsx +++ b/canvas/src/components/tabs/chat/AttachmentTextPreview.tsx @@ -26,6 +26,7 @@ // to download the full file. import { useState, useEffect } from "react"; +import { platformAuthHeaders } from "@/lib/api"; import type { ChatAttachment } from "./types"; import { isPlatformAttachment, resolveAttachmentHref } from "./uploads"; import { AttachmentChip } from "./AttachmentViews"; @@ -57,13 +58,13 @@ export function AttachmentTextPreview({ workspaceId, attachment, onDownload, ton void (async () => { try { const href = resolveAttachmentHref(workspaceId, attachment.uri); - const headers: Record = {}; - if (isPlatformAttachment(attachment.uri)) { - const adminToken = process.env.NEXT_PUBLIC_ADMIN_TOKEN; - if (adminToken) headers["Authorization"] = `Bearer ${adminToken}`; - const slug = getTenantSlug(); - if (slug) headers["X-Molecule-Org-Slug"] = slug; - } + // Only attach platform auth headers for in-platform URIs — + // off-platform URLs (HTTP/HTTPS attachments) MUST NOT receive + // our bearer token (it would leak the admin token to a third + // party). The branch is preserved with the new shared helper. + const headers: Record = isPlatformAttachment(attachment.uri) + ? platformAuthHeaders() + : {}; const res = await fetch(href, { headers, credentials: "include", @@ -182,9 +183,5 @@ export function AttachmentTextPreview({ workspaceId, attachment, onDownload, ton ); } -function getTenantSlug(): string | null { - if (typeof window === "undefined") return null; - const host = window.location.hostname; - const m = host.match(/^([^.]+)\.moleculesai\.app$/); - return m ? m[1] : null; -} +// Local getTenantSlug() removed — auth-header construction now goes +// through platformAuthHeaders() from @/lib/api (#178). diff --git a/canvas/src/components/tabs/chat/AttachmentVideo.tsx b/canvas/src/components/tabs/chat/AttachmentVideo.tsx index 0691006d..83d25a02 100644 --- a/canvas/src/components/tabs/chat/AttachmentVideo.tsx +++ b/canvas/src/components/tabs/chat/AttachmentVideo.tsx @@ -25,6 +25,7 @@ // fetch via service worker. v2 if measured-needed. import { useState, useEffect, useRef } from "react"; +import { platformAuthHeaders } from "@/lib/api"; import type { ChatAttachment } from "./types"; import { isPlatformAttachment, resolveAttachmentHref } from "./uploads"; import { AttachmentChip } from "./AttachmentViews"; @@ -61,13 +62,8 @@ export function AttachmentVideo({ workspaceId, attachment, onDownload, tone }: P void (async () => { try { const href = resolveAttachmentHref(workspaceId, attachment.uri); - const headers: Record = {}; - const adminToken = process.env.NEXT_PUBLIC_ADMIN_TOKEN; - if (adminToken) headers["Authorization"] = `Bearer ${adminToken}`; - const slug = getTenantSlug(); - if (slug) headers["X-Molecule-Org-Slug"] = slug; const res = await fetch(href, { - headers, + headers: platformAuthHeaders(), credentials: "include", // Videos are larger than images on average; give the request // more headroom. The server's per-request body cap (50MB) is @@ -147,11 +143,5 @@ export function AttachmentVideo({ workspaceId, attachment, onDownload, tone }: P ); } -// Internal helper — same shape as AttachmentImage's. Lifted to a -// shared util in PR-2.5 if a third caller needs it (PDF, audio). -function getTenantSlug(): string | null { - if (typeof window === "undefined") return null; - const host = window.location.hostname; - const m = host.match(/^([^.]+)\.moleculesai\.app$/); - return m ? m[1] : null; -} +// Local getTenantSlug() removed — auth-header construction now goes +// through platformAuthHeaders() from @/lib/api (#178). diff --git a/canvas/src/components/tabs/chat/uploads.ts b/canvas/src/components/tabs/chat/uploads.ts index 76fbdba9..d9cdde31 100644 --- a/canvas/src/components/tabs/chat/uploads.ts +++ b/canvas/src/components/tabs/chat/uploads.ts @@ -1,12 +1,16 @@ -import { PLATFORM_URL } from "@/lib/api"; -import { getTenantSlug } from "@/lib/tenant"; +import { PLATFORM_URL, platformAuthHeaders } from "@/lib/api"; import type { ChatAttachment } from "./types"; /** Chat attachments are intentionally uploaded via a direct fetch() * instead of the `api.post` helper — `api.post` JSON-stringifies the - * body, which would 500 on a Blob. Mirrors the header plumbing - * (tenant slug, admin token, credentials) so SaaS + self-hosted - * callers work the same way. */ + * body, which would 500 on a Blob. Auth headers (tenant slug, admin + * token, credentials) come from `platformAuthHeaders()` — the same + * helper `request()` uses, so a missing bearer surfaces as a single + * fix site instead of N copies. We deliberately do NOT set + * Content-Type so the browser writes the multipart boundary into the + * header; setting it manually would yield a multipart body the server + * can't parse. See lib/api.ts platformAuthHeaders() for the full + * rationale on why this pair must stay matched. */ export async function uploadChatFiles( workspaceId: string, files: File[], @@ -16,18 +20,12 @@ export async function uploadChatFiles( const form = new FormData(); for (const f of files) form.append("files", f, f.name); - const headers: Record = {}; - const slug = getTenantSlug(); - if (slug) headers["X-Molecule-Org-Slug"] = slug; - const adminToken = process.env.NEXT_PUBLIC_ADMIN_TOKEN; - if (adminToken) headers["Authorization"] = `Bearer ${adminToken}`; - // Uploads legitimately take a while on cold cache (tar write + // docker cp into the container). 60s is comfortable for the 25MB/ // 50MB caps the server enforces. const res = await fetch(`${PLATFORM_URL}/workspaces/${workspaceId}/chat/uploads`, { method: "POST", - headers, + headers: platformAuthHeaders(), body: form, credentials: "include", signal: AbortSignal.timeout(60_000), @@ -143,14 +141,8 @@ export async function downloadChatFile( return; } - const headers: Record = {}; - const slug = getTenantSlug(); - if (slug) headers["X-Molecule-Org-Slug"] = slug; - const adminToken = process.env.NEXT_PUBLIC_ADMIN_TOKEN; - if (adminToken) headers["Authorization"] = `Bearer ${adminToken}`; - const res = await fetch(href, { - headers, + headers: platformAuthHeaders(), credentials: "include", signal: AbortSignal.timeout(60_000), }); diff --git a/canvas/src/lib/__tests__/platform-auth-headers.test.ts b/canvas/src/lib/__tests__/platform-auth-headers.test.ts new file mode 100644 index 00000000..b750f226 --- /dev/null +++ b/canvas/src/lib/__tests__/platform-auth-headers.test.ts @@ -0,0 +1,97 @@ +// @vitest-environment jsdom +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; + +// Tests for platformAuthHeaders — the shared helper extracted in #178 +// to consolidate the bearer-token-attach + tenant-slug-attach pattern +// that was previously duplicated across 7 raw-fetch callsites in the +// canvas (uploads + 5 Attachment* components + the api.ts request() +// function). +// +// What we pin here: +// - Returns a fresh object each call (so callers can mutate without +// leaking into each other). +// - Empty result on a non-tenant host with no admin token (the +// localhost / self-hosted shape). +// - Bearer attached when NEXT_PUBLIC_ADMIN_TOKEN is set. +// - X-Molecule-Org-Slug attached when window.location.hostname is a +// tenant subdomain (.moleculesai.app). +// - Both attached when both apply (the production SaaS shape). +// +// Why jsdom: getTenantSlug() reads window.location.hostname. Node-only +// environment yields no window and getTenantSlug returns null +// unconditionally — wouldn't exercise the slug branch. + +import { platformAuthHeaders } from "../api"; + +describe("platformAuthHeaders", () => { + let originalAdminToken: string | undefined; + + beforeEach(() => { + originalAdminToken = process.env.NEXT_PUBLIC_ADMIN_TOKEN; + delete process.env.NEXT_PUBLIC_ADMIN_TOKEN; + }); + + afterEach(() => { + if (originalAdminToken === undefined) delete process.env.NEXT_PUBLIC_ADMIN_TOKEN; + else process.env.NEXT_PUBLIC_ADMIN_TOKEN = originalAdminToken; + // jsdom resets hostname between tests via the @vitest-environment + // pragma's per-test isolation. No explicit reset needed. + }); + + it("returns an empty object on a non-tenant host with no admin token", () => { + // jsdom default hostname is "localhost" — not a tenant slug, so + // getTenantSlug() returns null and no X-Molecule-Org-Slug is added. + const headers = platformAuthHeaders(); + expect(headers).toEqual({}); + }); + + it("attaches Authorization when NEXT_PUBLIC_ADMIN_TOKEN is set", () => { + process.env.NEXT_PUBLIC_ADMIN_TOKEN = "local-dev-admin"; + const headers = platformAuthHeaders(); + expect(headers).toEqual({ Authorization: "Bearer local-dev-admin" }); + }); + + it("does NOT attach Authorization when NEXT_PUBLIC_ADMIN_TOKEN is empty string", () => { + // Empty-string env is the JS-side shape of `KEY=` in .env. + // Treating it as unset matches the matched-pair guard in + // next.config.ts (admin-token-pair.test.ts) — symmetric semantics. + process.env.NEXT_PUBLIC_ADMIN_TOKEN = ""; + const headers = platformAuthHeaders(); + expect(headers).toEqual({}); + }); + + it("attaches X-Molecule-Org-Slug on a tenant subdomain", () => { + Object.defineProperty(window, "location", { + value: { hostname: "reno-stars.moleculesai.app" }, + writable: true, + }); + const headers = platformAuthHeaders(); + expect(headers).toEqual({ "X-Molecule-Org-Slug": "reno-stars" }); + }); + + it("attaches both when both apply (production SaaS shape)", () => { + Object.defineProperty(window, "location", { + value: { hostname: "reno-stars.moleculesai.app" }, + writable: true, + }); + process.env.NEXT_PUBLIC_ADMIN_TOKEN = "tenant-bearer"; + const headers = platformAuthHeaders(); + // Pin exact-equality on the full shape — substring/contains + // assertions would also pass for an extra-header bug. + expect(headers).toEqual({ + "X-Molecule-Org-Slug": "reno-stars", + Authorization: "Bearer tenant-bearer", + }); + }); + + it("returns a fresh object each call (callers can mutate safely)", () => { + process.env.NEXT_PUBLIC_ADMIN_TOKEN = "tok"; + const a = platformAuthHeaders(); + const b = platformAuthHeaders(); + expect(a).not.toBe(b); // distinct refs + expect(a).toEqual(b); // same content + a["Content-Type"] = "application/json"; + // Mutation on `a` does not leak into `b`. + expect(b["Content-Type"]).toBeUndefined(); + }); +}); diff --git a/canvas/src/lib/api.ts b/canvas/src/lib/api.ts index dae1152b..3ae5f413 100644 --- a/canvas/src/lib/api.ts +++ b/canvas/src/lib/api.ts @@ -21,6 +21,45 @@ export interface RequestOptions { timeoutMs?: number; } +/** + * Build the platform auth header set used by every authenticated fetch + * from the canvas. Returns a fresh object so callers can mutate (e.g. + * append `Content-Type` for JSON requests, omit it for FormData). + * + * SaaS cross-origin shape: + * - `X-Molecule-Org-Slug` — derived from `window.location.hostname` + * by `getTenantSlug()`. Control plane uses it for fly-replay + * routing. Empty on localhost / non-tenant hosts — safe to omit. + * - `Authorization: Bearer ` — `NEXT_PUBLIC_ADMIN_TOKEN` baked + * into the canvas build (see canvas/Dockerfile L8/L11). Required by + * the workspace-server when `ADMIN_TOKEN` is set on the server side + * (Tier-2b AdminAuth gate, wsauth_middleware.go ~L245). Empty when + * no admin token was provisioned — the Tier-1 session-cookie path + * handles that case via `credentials:"include"`. + * + * Why a shared helper: the two-line "read env, attach bearer; read + * slug, attach header" pattern was duplicated across `request()` and + * 7 raw-fetch callsites (chat uploads/download + 5 Attachment* + * components) before this consolidation. A new poller or raw fetch + * that forgets one of the two headers silently 401s against + * workspace-server when ADMIN_TOKEN is set — the exact bug shape + * called out in #178 / closes the post-#176 self-review gap. + * + * Callers that want JSON Content-Type should spread this and add it + * themselves; FormData callers should NOT add Content-Type (the + * browser sets the multipart boundary). Centralizing the auth pair + * but leaving Content-Type up to the caller is the minimum viable + * shared shape. + */ +export function platformAuthHeaders(): Record { + const headers: Record = {}; + const slug = getTenantSlug(); + if (slug) headers["X-Molecule-Org-Slug"] = slug; + const adminToken = process.env.NEXT_PUBLIC_ADMIN_TOKEN; + if (adminToken) headers["Authorization"] = `Bearer ${adminToken}`; + return headers; +} + async function request( method: string, path: string, @@ -28,17 +67,16 @@ async function request( retryCount = 0, options?: RequestOptions, ): Promise { - // SaaS cross-origin shape: - // - X-Molecule-Org-Slug: derived from window.location.hostname by - // getTenantSlug(). Control plane uses it for fly-replay routing. - // Empty on localhost / non-tenant hosts — safe to omit. - // - credentials:"include": sends the session cookie cross-origin. - // Cookie's Domain=.moleculesai.app attribute + cp's CORS allow this. - const headers: Record = { "Content-Type": "application/json" }; + // JSON-bodied request — Content-Type is JSON. Auth pair comes from + // the shared helper; see its doc comment for the SaaS-shape rationale. + const headers: Record = { + "Content-Type": "application/json", + ...platformAuthHeaders(), + }; + // Re-read slug locally for the 401 handler below — `headers` already + // has it, but the 401 branch needs the bare value to gate the + // session-probe + redirect logic on tenant context. const slug = getTenantSlug(); - if (slug) headers["X-Molecule-Org-Slug"] = slug; - const adminToken = process.env.NEXT_PUBLIC_ADMIN_TOKEN; - if (adminToken) headers["Authorization"] = `Bearer ${adminToken}`; const res = await fetch(`${PLATFORM_URL}${path}`, { method, -- 2.45.2