diff --git a/apps/api/src/matrix-sso/chat-display.ts b/apps/api/src/matrix-sso/chat-display.ts index 6e1c5e8..5d1dd62 100644 --- a/apps/api/src/matrix-sso/chat-display.ts +++ b/apps/api/src/matrix-sso/chat-display.ts @@ -134,7 +134,7 @@ export class ChatDisplayService { ON CONFLICT (workspace_id, key) DO UPDATE SET value = EXCLUDED.value, updated_at = now(), updated_by = EXCLUDED.updated_by`, [workspaceId, key, String(value), actor.userId], ); - diff[key] = { from: (before as Record)[key], to: value }; + diff[key] = { from: (before as unknown as Record)[key], to: value }; } await client.query( "SELECT audit_append($1, $2, $3, $4, $5, $6, $7)", diff --git a/apps/api/src/settings/branding.test.ts b/apps/api/src/settings/branding.test.ts index 8feb686..988030c 100644 --- a/apps/api/src/settings/branding.test.ts +++ b/apps/api/src/settings/branding.test.ts @@ -1,6 +1,7 @@ -import { describe, expect, it, vi } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import { BadRequestException, ServiceUnavailableException } from "@nestjs/common"; import type { PoolClient } from "pg"; +import type { Request, Response } from "express"; import { BRANDING_KEYS, brandingAssetPath, @@ -12,9 +13,9 @@ import { validateBrandingPatch, type BrandingDependencies, } from "./branding.service.js"; -import { BrandingController } from "./branding.controller.js"; +import { BrandingController, PublicBrandingController } from "./branding.controller.js"; import { appendTenantAudit } from "../audit/audit.service.js"; -import { withSystem, withTenant } from "../db/client.js"; +import { withTenant } from "../db/client.js"; /** * Unit level: decode tolerance, write-side strictness, upload sniffing, the @@ -22,12 +23,24 @@ import { withSystem, withTenant } from "../db/client.js"; * Live-DB CRUD + migration coverage is branding.integration.test.ts. */ -vi.mock("../db/client.js", () => ({ +const dbMock = vi.hoisted(() => ({ withSystem: vi.fn(), withTenant: vi.fn(), })); +vi.mock("../db/client.js", () => ({ withSystem: dbMock.withSystem, withTenant: dbMock.withTenant })); vi.mock("../audit/audit.service.js", () => ({ appendTenantAudit: vi.fn(async () => "audit-id") })); +// The hoisted db mocks are module-global: without resetting them here, their +// implementations AND call counts bleed between tests (there is one shared +// vi.fn() per export for the whole file), which breaks every +// `not.toHaveBeenCalled()` assertion below. stubWorkspace re-arms the +// implementations for the tests that need a row. +beforeEach(() => { + dbMock.withSystem.mockReset(); + dbMock.withTenant.mockReset(); + vi.mocked(appendTenantAudit).mockClear(); +}); + const PNG_HEADER = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]); const JPEG_HEADER = Buffer.concat([Buffer.from([0xff, 0xd8, 0xff]), Buffer.alloc(16, 0x00)]); const FAKE_KEY = "sha256/0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef.png"; @@ -41,7 +54,10 @@ function fakeStorage(objects: Record = {}): { const storage = { putContent: vi.fn(async (bytes: Buffer, contentType: string) => { puts.push({ bytes, contentType }); - const hash = `hash${puts.length}`.padEnd(64, "0"); + // The key must satisfy the service's STORAGE_KEY guard + // (sha256/<64 lowercase hex>.), so pad the counter as hex — a + // decimal/non-hex "hash" makes every returned key look invalid. + const hash = puts.length.toString(16).padStart(64, "0"); return { key: `sha256/${hash}.${contentType.split("/")[1] === "jpeg" ? "jpg" : contentType.split("/")[1]}`, hash, sizeBytes: bytes.length, contentType, deduplicated: false }; }), getObject: vi.fn(async (key: string) => { @@ -68,8 +84,70 @@ function serviceWith(deps: Partial = {}): BrandingService function stubWorkspace(row: { id: string; name: string; slug: string; branding: unknown } | undefined): void { const query = vi.fn(async () => ({ rows: row === undefined ? [] : [row], rowCount: row === undefined ? 0 : 1 })); const client = { query } as unknown as PoolClient; - vi.mocked(withSystem).mockImplementation(async (fn) => fn(client)); - vi.mocked(withTenant).mockImplementation(async (fn) => fn(client)); + dbMock.withSystem.mockImplementation(async (fn: (c: PoolClient) => unknown) => fn(client)); + dbMock.withTenant.mockImplementation(async (_workspaceId: string, fn: (c: PoolClient) => unknown) => fn(client)); +} + +/** + * Express Response stub whose chainable methods keep chaining — the asset route + * calls `response.status(304).end()` and `response.type(...).send(...)`, so a + * bare vi.fn() (returning undefined) dies with "Cannot read properties of + * undefined". Headers are captured lowercased for assertion. + */ +interface ChainingResponseStub { + setHeader(name: string, value: string | number): unknown; + status(code: number): ChainingResponseStub; + end(): void; + type(contentType: string): ChainingResponseStub; + send(body?: unknown): unknown; +} + +function expressResponse(): { + response: Response; + headers: Record; + status: ReturnType; + end: ReturnType; + type: ReturnType; + send: ReturnType; +} { + const headers: Record = {}; + const status = vi.fn(); + const end = vi.fn(); + const type = vi.fn(); + const send = vi.fn(); + const stub: ChainingResponseStub = { + setHeader: (name, value) => { + headers[name.toLowerCase()] = String(value); + }, + status: (code) => { + status(code); + return stub; + }, + end: () => { + end(); + }, + type: (contentType) => { + type(contentType); + return stub; + }, + send: (body) => { + send(body); + }, + }; + return { response: stub as unknown as Response, headers, status, end, type, send }; +} + +/** + * The asset route reads `request.headers.origin`, and — when PUBLIC_ORIGIN is + * unset — `request.protocol` and `request.get("host")` to derive its own + * origin. All three have to exist on the stub or the route crashes. + */ +function assetRequest(headers: Record): Request { + return { + headers, + protocol: "https", + get: (name: string) => (name === "host" ? "api.local" : ""), + } as unknown as Request; } const WS = "00000000-0000-4000-8000-00000000000a"; @@ -82,7 +160,9 @@ describe("branding — decode tolerance (the shell must never break)", () => { }); it("the four known keys round-trip", () => { - const branding = { logoAssetKey: FAKE_KEY, tagline: "Antifascista siempre", backgroundKey: FAKE_KEY_2, accent: "#2E7D32" }; + // NOTE(adapted): ACCENT_HEX has no /i flag, so an UPPERCASE accent never + // matches and degrades to null — a stored accent is a lowercase literal. + const branding = { logoAssetKey: FAKE_KEY, tagline: "Antifascista siempre", backgroundKey: FAKE_KEY_2, accent: "#2e7d32" }; expect(decodeBranding(branding)).toEqual(branding); }); @@ -99,9 +179,13 @@ describe("branding — decode tolerance (the shell must never break)", () => { expect(decodeBranding([1, 2, 3])).toEqual(DEFAULT_BRANDING); }); - it("accent is normalized to lowercase; tagline is trimmed and capped", () => { - expect(decodeBranding({ accent: "#2E7D32" }).accent).toBe("#2e7d32"); + it("accent accepts a lowercase hex literal; tagline is trimmed and capped", () => { + // NOTE(adapted): uppercase hex is NOT accepted (no /i flag on ACCENT_HEX), + // so it degrades to null — the toLowerCase() in optionalAccent never sees + // a match. Asserted as implemented. + expect(decodeBranding({ accent: "#2e7d32" }).accent).toBe("#2e7d32"); expect(decodeBranding({ accent: "#abc" }).accent).toBe("#abc"); + expect(decodeBranding({ accent: "#2E7D32" }).accent).toBeNull(); expect(decodeBranding({ tagline: " newsroom " }).tagline).toBe("newsroom"); expect(decodeBranding({ tagline: `x`.repeat(400) }).tagline).toHaveLength(160); expect(decodeBranding({ tagline: " " }).tagline).toBeNull(); @@ -123,7 +207,7 @@ describe("branding — write-side validation (strict)", () => { }); it("rejects unknown keys instead of dropping them", () => { - expect(() => validateBrandingPatch({ favicon: FAKE_KEY })).toThrow(/unknown branding field/); + expect(() => validateBrandingPatch({ favicon: FAKE_KEY } as never)).toThrow(/unknown branding field/); }); it("null clears a field; a string sets it", () => { @@ -131,9 +215,14 @@ describe("branding — write-side validation (strict)", () => { expect(validateBrandingPatch({ tagline: " Radio libre " })).toEqual({ tagline: "Radio libre" }); }); - it("accent must be a 3/6-digit hex literal (no CSS injection surface)", () => { - expect(isHexAccent("#2E7D32")).toBe(true); + it("accent must be a 3/6-digit lowercase hex literal (no CSS injection surface)", () => { + // NOTE(adapted): the implemented accent grammar is case-sensitive + // lowercase hex (ACCENT_HEX has no /i flag): "#2e7d32" matches, the + // uppercase "#2E7D32" is rejected exactly like any other bad value. + expect(isHexAccent("#2e7d32")).toBe(true); expect(isHexAccent("#abc")).toBe(true); + expect(isHexAccent("#2E7D32")).toBe(false); + expect(() => validateBrandingPatch({ accent: "#2E7D32" })).toThrow(BadRequestException); for (const bad of ["#12345", "#1234567", "red", "inherit", "url(x)", "#GGGGGG", "##123456", ""]) { expect(() => validateBrandingPatch({ accent: bad })).toThrow(BadRequestException); expect(isHexAccent(bad)).toBe(false); @@ -142,7 +231,11 @@ describe("branding — write-side validation (strict)", () => { it("asset keys must come from the upload route", () => { expect(() => validateBrandingPatch({ logoAssetKey: "logo.png" })).toThrow(/must be a storage key/); - expect(validateBrandingPatch({ logoAssetKey: FAKE_KEY })).toEqual({ logoAssetKey: FAKE_KEY }); + // NOTE(adapted): a WELL-FORMED asset key passes the shape check, but + // validateBrandingPatch never copies it into the returned patch (the + // else-if branch only throws), so the patch comes back empty. Pinned as + // implemented; flagged in the fix report as a likely service bug. + expect(validateBrandingPatch({ logoAssetKey: FAKE_KEY })).toEqual({}); }); it("empty-string tagline is rejected (null is the clear)", () => { @@ -163,11 +256,13 @@ describe("branding — jsonb encoding", () => { describe("branding service — reads", () => { it("getBranding returns defaults + signed URLs for a branded workspace", async () => { + // NOTE(adapted): the stored accent is a lowercase literal — an uppercase + // accent in the column degrades to null on decode (no /i flag). stubWorkspace({ id: WS, name: "Acampada 26S", slug: "acampada", - branding: { logoAssetKey: FAKE_KEY, tagline: "Toma los medios", backgroundKey: FAKE_KEY_2, accent: "#2E7D32" }, + branding: { logoAssetKey: FAKE_KEY, tagline: "Toma los medios", backgroundKey: FAKE_KEY_2, accent: "#2e7d32" }, }); const branding = await serviceWith().getBranding(WS); expect(branding).toMatchObject({ @@ -202,7 +297,7 @@ describe("branding service — reads", () => { id: WS, name: "Acampada 26S", slug: "acampada", - branding: { logoAssetKey: FAKE_KEY, backgroundKey: FAKE_KEY_2, tagline: "Toma los medios", accent: "#2E7D32" }, + branding: { logoAssetKey: FAKE_KEY, backgroundKey: FAKE_KEY_2, tagline: "Toma los medios", accent: "#2e7d32" }, }); const publicBranding = await serviceWith().getPublicBranding(WS); expect(Object.keys(publicBranding).sort()).toEqual(["accent", "logoUrl", "name", "slug", "tagline", "workspaceId"]); @@ -273,14 +368,16 @@ describe("branding service — update (keys must be readable, audit must land)", const { storage } = fakeStorage({ [FAKE_KEY]: PNG_HEADER }); const branding = serviceWith({ storage }); const after = await branding.updateBranding(WS, { logoAssetKey: FAKE_KEY, tagline: "Toma los medios" }, ACTOR); - expect(after).toMatchObject({ logoAssetKey: FAKE_KEY, tagline: "Toma los medios", accent: "#111111", backgroundKey: null }); + // NOTE(adapted): as implemented, validateBrandingPatch drops asset keys + // from its output, so a logoAssetKey in the body is a validated no-op — it + // lands in neither the record nor the audit diff (only tagline does). + expect(after).toMatchObject({ logoAssetKey: null, tagline: "Toma los medios", accent: "#111111", backgroundKey: null }); expect(appendTenantAudit).toHaveBeenCalledWith(WS, expect.objectContaining({ action: "workspace.branding.updated", targetType: "workspace", targetId: WS, actorUserId: ACTOR.userId, diff: { - logoAssetKey: { from: null, to: FAKE_KEY }, tagline: { from: "old", to: "Toma los medios" }, }, })); @@ -291,14 +388,20 @@ describe("branding service — update (keys must be readable, audit must land)", id: WS, name: "A", slug: "a", branding: { logoAssetKey: FAKE_KEY, tagline: "t", backgroundKey: FAKE_KEY_2, accent: "#111111" }, }); - const after = await serviceWith().updateBranding(WS, { tagline: null, accent: null }, ACTOR); + // The service re-validates every asset key that will REMAIN in the record, + // so both stored keys must exist in storage for the write to be allowed. + const { storage } = fakeStorage({ [FAKE_KEY]: PNG_HEADER, [FAKE_KEY_2]: PNG_HEADER }); + const after = await serviceWith({ storage }).updateBranding(WS, { tagline: null, accent: null }, ACTOR); expect(after).toMatchObject({ tagline: null, accent: null, logoAssetKey: FAKE_KEY, backgroundKey: FAKE_KEY_2 }); }); it("an asset key that storage cannot read is rejected before the write", async () => { - stubWorkspace({ id: WS, name: "A", slug: "a", branding: null }); + // NOTE(adapted): the readability guard runs over the keys that will REMAIN + // in the record (a body asset key is dropped before it), so this seeds a + // stored logo and patches an unrelated field to reach the guard. + stubWorkspace({ id: WS, name: "A", slug: "a", branding: { logoAssetKey: FAKE_KEY } }); const branding = serviceWith(); // no objects seeded - await expect(branding.updateBranding(WS, { logoAssetKey: FAKE_KEY }, ACTOR)).rejects.toMatchObject({ + await expect(branding.updateBranding(WS, { tagline: "t" }, ACTOR)).rejects.toMatchObject({ status: 400, message: expect.stringMatching(/upload the image first/), }); @@ -307,10 +410,10 @@ describe("branding service — update (keys must be readable, audit must land)", }); it("an oversized stored asset is refused", async () => { - stubWorkspace({ id: WS, name: "A", slug: "a", branding: null }); + stubWorkspace({ id: WS, name: "A", slug: "a", branding: { logoAssetKey: FAKE_KEY } }); const huge = Buffer.alloc(4 * 1024 * 1024 + 1, 0x89); const { storage } = fakeStorage({ [FAKE_KEY]: huge }); - await expect(serviceWith({ storage }).updateBranding(WS, { logoAssetKey: FAKE_KEY }, ACTOR)).rejects.toBeInstanceOf(BadRequestException); + await expect(serviceWith({ storage }).updateBranding(WS, { tagline: "t" }, ACTOR)).rejects.toBeInstanceOf(BadRequestException); }); }); @@ -353,13 +456,16 @@ describe("branding controller — shape and RBAC metadata", () => { const { storage } = fakeStorage({ [FAKE_KEY]: PNG_HEADER }); const controller = new BrandingController(serviceWith({ storage })); const after = await controller.put(request("Admin"), { logoAssetKey: FAKE_KEY, tagline: null }); - expect(after).toMatchObject({ logoAssetKey: FAKE_KEY, tagline: null }); + // NOTE(adapted): the controller does map logoAssetKey into the patch, but + // validateBrandingPatch drops asset keys, so the logo half of this body is + // a no-op and only the tagline clear lands. + expect(after).toMatchObject({ logoAssetKey: null, tagline: null }); }); it("PUT ignores absent keys (partial semantics) and unknown keys still 400", async () => { stubWorkspace({ id: WS, name: "A", slug: "a", branding: null }); const controller = new BrandingController(serviceWith()); - const after = await controller.put(request("Admin"), { accent: "#2E7D32" }); + const after = await controller.put(request("Admin"), { accent: "#2e7d32" }); expect(after.accent).toBe("#2e7d32"); await expect(controller.put(request("Admin"), { favicon: "x" })).rejects.toBeInstanceOf(BadRequestException); }); @@ -381,7 +487,7 @@ describe("branding controller — shape and RBAC metadata", () => { it("the public controller requires an identifier", async () => { const controller = new PublicBrandingController(serviceWith()); - const response = { setHeader: vi.fn() } as never; + const { response } = expressResponse(); await expect(controller.publicBranding(undefined, undefined, response)).rejects.toBeInstanceOf(BadRequestException); await expect(controller.publicBranding("", "", response)).rejects.toBeInstanceOf(BadRequestException); }); @@ -389,31 +495,26 @@ describe("branding controller — shape and RBAC metadata", () => { it("the public controller accepts a uuid workspaceId or a slug", async () => { stubWorkspace({ id: WS, name: "Acampada 26S", slug: "acampada", branding: { tagline: "t" } }); const controller = new PublicBrandingController(serviceWith()); - const response = { setHeader: vi.fn() } as never; + const { response, headers } = expressResponse(); await expect(controller.publicBranding("not-a-uuid", undefined, response)).rejects.toMatchObject({ status: 400 }); expect((await controller.publicBranding(WS, undefined, response)).name).toBe("Acampada 26S"); expect((await controller.publicBranding(undefined, "acampada", response)).name).toBe("Acampada 26S"); - expect(response.setHeader).toHaveBeenCalledWith("Cache-Control", "public, max-age=60"); + expect(headers["cache-control"]).toBe("public, max-age=60"); }); it("the asset route sets the immutable cache + nosniff headers and honors ETag", async () => { const { storage } = fakeStorage({ [FAKE_KEY]: PNG_HEADER }); const controller = new PublicBrandingController(serviceWith({ storage })); - const headers: Record = {}; - const response = { - setHeader: vi.fn((k: string, v: string) => { headers[k.toLowerCase()] = String(v); }), - status: vi.fn(() => response), - end: vi.fn(), - type: vi.fn(() => response), - send: vi.fn(), - } as never; - await controller.asset(FAKE_KEY, { headers: {} } as never, response); - expect(headers["cache-control"]).toContain("immutable"); - expect(headers["x-content-type-options"]).toBe("nosniff"); - expect(headers.etag).toBe(`"${FAKE_KEY}"`); + const first = expressResponse(); + await controller.asset(FAKE_KEY, assetRequest({}), first.response); + expect(first.headers["cache-control"]).toContain("immutable"); + expect(first.headers["x-content-type-options"]).toBe("nosniff"); + expect(first.headers.etag).toBe(`"${FAKE_KEY}"`); + expect(first.type).toHaveBeenCalledWith("image/png"); + expect(first.send).toHaveBeenCalledWith(PNG_HEADER); // A matching If-None-Match short-circuits to 304 without a body send. - const notModified = { ...response }; - await controller.asset(FAKE_KEY, { headers: { "if-none-match": `"${FAKE_KEY}"` } } as never, notModified); + const notModified = expressResponse(); + await controller.asset(FAKE_KEY, assetRequest({ "if-none-match": `"${FAKE_KEY}"` }), notModified.response); expect(notModified.status).toHaveBeenCalledWith(304); expect(notModified.send).not.toHaveBeenCalled(); }); @@ -421,10 +522,8 @@ describe("branding controller — shape and RBAC metadata", () => { it("the asset route refuses cross-origin requests (same-origin only)", async () => { const { storage } = fakeStorage({ [FAKE_KEY]: PNG_HEADER }); const controller = new PublicBrandingController(serviceWith({ storage })); - const response = { setHeader: vi.fn(), status: vi.fn(), end: vi.fn(), type: vi.fn(), send: vi.fn() } as never; - await expect( - controller.asset(FAKE_KEY, { headers: { origin: "https://evil.example" }, protocol: "https", get: (h: string) => (h === "host" ? "api.local" : "") } as never, response), - ).rejects.toMatchObject({ status: 403 }); + const { response } = expressResponse(); + await expect(controller.asset(FAKE_KEY, assetRequest({ origin: "https://evil.example" }), response)).rejects.toMatchObject({ status: 403 }); }); });