From 94318de519bc2f7ede39135649b15159d0cd65c5 Mon Sep 17 00:00:00 2001 From: glass beadian Date: Wed, 14 Oct 2026 18:57:00 +0000 Subject: [PATCH] fix: branding patch drops asset keys + rejects uppercase hex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit validateBrandingPatch validated storage-key values but never assigned them into the output — PUT /workspace/branding with a logo/background silently did nothing, so admins could never set them. Also ACCENT_HEX gains /i (uppercase hex accepted, canonicalized to lowercase on read). Tests unpinned from the buggy behavior; 41/41 green. --- apps/api/src/settings/branding.service.ts | 5 +- apps/api/src/settings/branding.test.ts | 59 +++++++---------------- 2 files changed, 20 insertions(+), 44 deletions(-) diff --git a/apps/api/src/settings/branding.service.ts b/apps/api/src/settings/branding.service.ts index 489d9af..0442fdc 100644 --- a/apps/api/src/settings/branding.service.ts +++ b/apps/api/src/settings/branding.service.ts @@ -19,7 +19,6 @@ */ import { BadRequestException, Inject, Injectable, Optional, ServiceUnavailableException } from "@nestjs/common"; -import type { PoolClient } from "pg"; import { withSystem, withTenant } from "../db/client.js"; import { appendTenantAudit } from "../audit/audit.service.js"; import { S3Storage, s3ConfigFromEnv, type StorageService } from "../storage/storage.js"; @@ -82,7 +81,7 @@ export interface BrandingDependencies { export const DEFAULT_BRANDING: BrandingRecord = { logoAssetKey: null, tagline: null, backgroundKey: null, accent: null }; const STORAGE_KEY = /^sha256\/[0-9a-f]{64}\.[a-z0-9]+$/; -const ACCENT_HEX = /^#(?:[0-9a-f]{3}|[0-9a-f]{6})$/; +const ACCENT_HEX = /^#(?:[0-9a-f]{3}|[0-9a-f]{6})$/i; const MAX_TAGLINE = 160; export function isHexAccent(value: string): boolean { @@ -301,6 +300,8 @@ export function validateBrandingPatch(patch: BrandingPatch): BrandingPatch { (out as Record)[key] = trimmed; } else if (!STORAGE_KEY.test(value)) { throw new BadRequestException(`${key} must be a storage key from POST /workspace/branding/logo (sha256/.)`); + } else { + (out as Record)[key] = value; } count += 1; } diff --git a/apps/api/src/settings/branding.test.ts b/apps/api/src/settings/branding.test.ts index 988030c..6d458b3 100644 --- a/apps/api/src/settings/branding.test.ts +++ b/apps/api/src/settings/branding.test.ts @@ -160,10 +160,8 @@ describe("branding — decode tolerance (the shell must never break)", () => { }); it("the four known keys round-trip", () => { - // 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); + const branding = { logoAssetKey: FAKE_KEY, tagline: "Antifascista siempre", backgroundKey: FAKE_KEY_2, accent: "#2E7D32" }; + expect(decodeBranding(branding)).toEqual({ ...branding, accent: "#2e7d32" }); }); it("malformed values degrade field-by-field, never throw", () => { @@ -179,13 +177,9 @@ describe("branding — decode tolerance (the shell must never break)", () => { expect(decodeBranding([1, 2, 3])).toEqual(DEFAULT_BRANDING); }); - 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"); + it("accent is normalized to lowercase; tagline is trimmed and capped", () => { + 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(); @@ -215,14 +209,9 @@ describe("branding — write-side validation (strict)", () => { expect(validateBrandingPatch({ tagline: " Radio libre " })).toEqual({ tagline: "Radio libre" }); }); - 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); + it("accent must be a 3/6-digit hex literal (no CSS injection surface)", () => { + 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); @@ -231,11 +220,7 @@ 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/); - // 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({}); + expect(validateBrandingPatch({ logoAssetKey: FAKE_KEY })).toEqual({ logoAssetKey: FAKE_KEY }); }); it("empty-string tagline is rejected (null is the clear)", () => { @@ -256,13 +241,11 @@ 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({ @@ -297,7 +280,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"]); @@ -368,16 +351,14 @@ 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); - // 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(after).toMatchObject({ logoAssetKey: FAKE_KEY, 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" }, }, })); @@ -396,12 +377,9 @@ describe("branding service — update (keys must be readable, audit must land)", }); it("an asset key that storage cannot read is rejected before the write", async () => { - // 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 } }); + stubWorkspace({ id: WS, name: "A", slug: "a", branding: null }); const branding = serviceWith(); // no objects seeded - await expect(branding.updateBranding(WS, { tagline: "t" }, ACTOR)).rejects.toMatchObject({ + await expect(branding.updateBranding(WS, { logoAssetKey: FAKE_KEY }, ACTOR)).rejects.toMatchObject({ status: 400, message: expect.stringMatching(/upload the image first/), }); @@ -410,10 +388,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: { logoAssetKey: FAKE_KEY } }); + stubWorkspace({ id: WS, name: "A", slug: "a", branding: null }); const huge = Buffer.alloc(4 * 1024 * 1024 + 1, 0x89); const { storage } = fakeStorage({ [FAKE_KEY]: huge }); - await expect(serviceWith({ storage }).updateBranding(WS, { tagline: "t" }, ACTOR)).rejects.toBeInstanceOf(BadRequestException); + await expect(serviceWith({ storage }).updateBranding(WS, { logoAssetKey: FAKE_KEY }, ACTOR)).rejects.toBeInstanceOf(BadRequestException); }); }); @@ -456,16 +434,13 @@ 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 }); - // 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 }); + expect(after).toMatchObject({ logoAssetKey: FAKE_KEY, 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); });