hacklab/T-HUB
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

fix: branding patch drops asset keys + rejects uppercase hex

Browse source 
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.
This commit is contained in:
glass beadian 2026-10-14 18:57:00 +00:00
parent 4b249e3811
commit 94318de519
2 changed files with 20 additions and 44 deletions
Download patch file Download diff file Expand all files Collapse all files

5
apps/api/src/settings/branding.service.ts
View file

@ -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<string, unknown>)[key] = trimmed;
} else if (!STORAGE_KEY.test(value)) {
throw new BadRequestException(`${key} must be a storage key from POST /workspace/branding/logo (sha256/<hash>.<ext>)`);
} else {
(out as Record<string, unknown>)[key] = value;
}
count += 1;
}

59
apps/api/src/settings/branding.test.ts
View file

@ -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);
});