fix(brands): reject anchoring a brand to another org's site

docs/openapi/brands-v2-api.yaml

@@ -92,11 +92,18 @@ v2-brands-for-org:
         $ref: './responses.yaml#/404-organization-not-found-with-id'
       '409':
         description: |
-          Conflict. A by-name upsert matched an existing **active** brand and the
-          supplied `status` would demote it to `pending`. Silent demotion is
-          rejected (`code: brand_status_demotion_not_allowed`). Use
-          `PATCH /v2/orgs/{spaceCatId}/brands/{brandId}/status` for intentional
-          status transitions.
+          Conflict. One of (disambiguated by the `code` field in the response body,
+          where present):
+          - A by-name upsert matched an existing **active** brand and the supplied
+            `status` would demote it to `pending`. Silent demotion is rejected
+            (`code: brand_status_demotion_not_allowed`). Use
+            `PATCH /v2/orgs/{spaceCatId}/brands/{brandId}/status` for intentional
+            status transitions.
+          - The supplied `baseSiteId` is a fresh anchor (the brand has no
+            persisted primary site yet) but that site belongs to a different
+            organization (`code: brand_site_org_mismatch`).
+          - The brand's primary URL is already the primary URL (base site) of
+            another brand in this organization.
         headers:
           X-Error:
             $ref: './headers.yaml#/xError'
@@ -215,6 +222,10 @@ v2-brand-for-org:
             persisted `updatedAt` — another write landed after this client last
             loaded the brand (`code: brand_stale_write`). Reload the brand and
             reapply the edit rather than retrying with the same body.
+          - The supplied `baseSiteId` is a fresh anchor (the brand has no
+            persisted primary site, or is `pending` and re-pointing) but that
+            site belongs to a different organization
+            (`code: brand_site_org_mismatch`).
           - The brand's primary URL is already the primary URL (base site) of
             another brand in this organization.
         headers:

src/controllers/brands.js

@@ -14,6 +14,7 @@
 
 import { randomUUID } from 'crypto';
 
+import { cleanupHeaderValue } from '@adobe/helix-shared-utils';
 import BrandClient, { BrandGovernanceClient } from '@adobe/spacecat-shared-brand-client';
 import DrsClient from '@adobe/spacecat-shared-drs-client';
 import {
@@ -23,7 +24,6 @@ import {
   noContent,
   createResponse,
   forbidden,
-  internalServerError,
 } from '@adobe/spacecat-shared-http-utils';
 import {
   composeBaseURL,
@@ -363,13 +363,29 @@ function BrandsController(ctx, log, env) {
       // client distinguish error cases without regex-matching the message text
       // (LLMO-6591; see the `uq_brand_name_per_org` TODO this same pattern
       // predates in elmo-ui's getBrandSaveErrorDescriptor).
+      // cleanupHeaderValue strips chars HTTP headers can't carry (CR/LF and
+      // non-ASCII that would otherwise throw ERR_INVALID_CHAR — caught via the
+      // it-postgres IT suite when this guard's own message used an em dash,
+      // serenity-docs#346). The JSON body keeps the raw message; only the
+      // header copy needs sanitizing.
       return createResponse(
         { message: appErr.message, ...(appErr.code ? { code: appErr.code } : {}) },
         appErr.status,
-        { [HEADER_ERROR]: appErr.message },
+        { [HEADER_ERROR]: cleanupHeaderValue(appErr.message || 'Error') },
       );
     }
-    return internalServerError(appErr.message);
+    // Same split as the typed-status branch above: internalServerError() would
+    // set the header AND the body from one value, so a non-ASCII character in
+    // a bare Error's message would strip from the body too — operators
+    // debugging a 500 lose it there for no reason (the raw error is still in
+    // the logs regardless). Build the response directly instead so only the
+    // header copy is sanitized.
+    const rawMessage = appErr.message || 'Internal server error';
+    return createResponse(
+      { message: rawMessage },
+      500,
+      { [HEADER_ERROR]: cleanupHeaderValue(rawMessage) },
+    );
   }
 
   function validateBrandGuidanceFields(brandData = {}) {

src/support/brands-storage.js

@@ -336,6 +336,57 @@ async function replaceChildRows(table, brandId, rows, onConflict, postgrestClien
   }
 }
 
+/**
+ * Verifies a candidate primary site (`baseSiteId`) belongs to the same org as the
+ * brand being anchored to it, before that site_id is ever persisted.
+ *
+ * serenity-docs#346: `brand.organization_id != site.organization_id` is exactly the
+ * org-ID mismatch pattern the investigation traced (Tata Capital, BMW, Toyota, ...) —
+ * a brand silently anchored to a *different* org's site. Both upsertBrand (fresh
+ * create / first anchor) and updateBrand (first set, or pending re-point) must call
+ * this before writing site_id; the immutable-once-set branches in each are
+ * unaffected, since they already refuse to change an existing active site_id.
+ *
+ * @param {object} postgrestClient - PostgREST client
+ * @param {string} siteId - Candidate `brands.site_id`
+ * @param {string} organizationId - SpaceCat organization UUID the brand belongs to
+ * @param {string} brandLabel - Whatever identifies the brand in the caller's context,
+ *   for the error message only — upsertBrand passes the brand name (not yet
+ *   persisted, so no id exists yet); updateBrand passes the fetched brand's
+ *   name when its existing-row read found one, else falls back to brandId.
+ * @throws {Error} status 409, code 'brand_site_org_mismatch', if the site does not
+ *   belong to organizationId (including if it doesn't exist at all)
+ */
+async function assertSiteBelongsToOrg(postgrestClient, siteId, organizationId, brandLabel) {
+  const { data: anchorSite, error: anchorSiteError } = await postgrestClient
+    .from('sites')
+    .select('id')
+    .eq('id', siteId)
+    .eq('organization_id', organizationId)
+    .maybeSingle();
+  if (anchorSiteError) {
+    throw new Error(
+      `Failed to verify primary site org for brand "${brandLabel}": ${anchorSiteError.message}`,
+    );
+  }
+  if (!anchorSite) {
+    // Plain ASCII (no em dash) to match this file's other thrown, client-facing
+    // messages: an em dash here previously crashed createErrorResponse's
+    // X-Error header (@adobe/fetch rejects non-Latin1 header content with a
+    // raw TypeError, surfacing as a 500 instead of this 409 — caught by the
+    // it-postgres IT suite, not the mocked unit tests). createErrorResponse
+    // now sanitizes the header regardless (serenity-docs#346), but this stays
+    // ASCII too rather than leaning on that alone.
+    const err = new Error(
+      `Cannot anchor brand "${brandLabel}" to site ${siteId}: that site `
+      + `does not exist, or does not belong to organization ${organizationId}.`,
+    );
+    err.status = 409;
+    err.code = 'brand_site_org_mismatch';
+    throw err;
+  }
+}
+
 /**
  * Fully replaces brand_sites for a brand. Groups submitted URLs by normalized base URL
  * (via composeBaseURL) so that multiple paths under the same site share one brand_sites row.
@@ -1200,6 +1251,18 @@ export async function upsertBrand({
   // (which left Semrush brands' site_id NULL) is removed; a genuine collision with
   // another brand's primary site still surfaces as the brands_base_site_unique 409
   // handled below.
+  // serenity-docs#346: a brand's primary site must belong to the same org as the
+  // brand itself — anchoring to another org's site is exactly the org-ID mismatch
+  // pattern the investigation traced (Tata Capital, BMW, Toyota, ...). Verify on
+  // both paths that assign a *new* anchor (fresh create, or first anchor for a
+  // previously Semrush-only brand); the immutable-once-set branch below is
+  // unaffected since it already refuses to change an existing site_id.
+  const wantsNewAnchor = hasText(brand.baseSiteId)
+    && (existing === null || !hasText(existing.site_id));
+  if (wantsNewAnchor) {
+    await assertSiteBelongsToOrg(postgrestClient, brand.baseSiteId, organizationId, brand.name);
+  }
+
   if (existing === null) {
     row.site_id = hasText(brand.baseSiteId) ? brand.baseSiteId : null;
   } else if (hasText(brand.baseSiteId) && !hasText(existing.site_id)) {
@@ -1312,7 +1375,7 @@ export async function updateBrand({
   if (needsExistingFetch) {
     const { data: current, error: currentError } = await postgrestClient
       .from('brands')
-      .select('site_id, status, updated_at')
+      .select('name, site_id, status, updated_at')
       .eq('id', brandId)
       .maybeSingle();
     // Fail closed: a swallowed read error leaves `current` null, so the guard
@@ -1379,6 +1442,10 @@ export async function updateBrand({
       patch.site_id = null;
     }
   } else if (hasText(updates.baseSiteId) && (!existing?.site_id || isPending)) {
+    // serenity-docs#346: same org-ID mismatch guard as upsertBrand — verify the
+    // new/re-pointed site actually belongs to this brand's org before persisting.
+    const brandLabel = existing?.name || brandId;
+    await assertSiteBelongsToOrg(postgrestClient, updates.baseSiteId, organizationId, brandLabel);
     patch.site_id = updates.baseSiteId;
   }
 

test/controllers/brands.test.js

@@ -7265,6 +7265,64 @@ describe('Brands Controller', () => {
       }
     });
 
+    it('strips a header-unsafe character from the X-Error header without altering the JSON body (serenity-docs#346)', async () => {
+      // A brand name is customer-controlled, and any 409/500 whose message
+      // echoes it (this guard, brand_status_demotion_not_allowed, ...) would
+      // previously crash createErrorResponse's X-Error header with a raw
+      // TypeError [ERR_INVALID_CHAR] instead of returning the intended
+      // status, if that message contained a character outside the header-safe
+      // range (printable ASCII + Latin-1, 0x20-0x7E/0x80-0xFF — plain accented
+      // characters like "é" are actually fine; an em dash is not). This is
+      // exactly what the it-postgres IT suite caught for an em dash in this
+      // guard's own message.
+      const err = new Error(
+        'Cannot anchor brand "Global — Direct" to site abc: that site does not belong to organization xyz.',
+      );
+      err.status = 409;
+      err.code = 'brand_site_org_mismatch';
+      const updateBrandStub = sinon.stub().rejects(err);
+
+      const controller = await buildUpdateController({ updateBrand: updateBrandStub });
+      const response = await controller.updateBrandForOrg({
+        ...context,
+        params: { spaceCatId: ORGANIZATION_ID, brandId: BRAND_UUID },
+        data: { baseSiteId: 'some-other-orgs-site' },
+        dataAccess: mockDataAccess,
+        attributes: { authInfo: { getType: () => 'ims', profile: { email: 'user@test.com' } } },
+      });
+
+      expect(response.status).to.equal(409);
+      const body = await response.json();
+      expect(body.code).to.equal('brand_site_org_mismatch');
+      expect(body.message).to.equal(err.message); // body keeps the full, unsanitized message
+      expect(response.headers.get('x-error')).to.equal(
+        'Cannot anchor brand "Global  Direct" to site abc: that site does not belong to organization xyz.',
+      );
+    });
+
+    it('sanitizes only the X-Error header on the untyped-error (500) fallback too, keeping the body raw', async () => {
+      // Same split as the 409 case above, on the OTHER branch of
+      // createErrorResponse: a bare Error with no .status (e.g. the
+      // anchorSiteError DB-failure path) must not have its body message
+      // sanitized just because the header copy needs to be.
+      const err = new Error('DB read failed — connection reset');
+      const updateBrandStub = sinon.stub().rejects(err);
+
+      const controller = await buildUpdateController({ updateBrand: updateBrandStub });
+      const response = await controller.updateBrandForOrg({
+        ...context,
+        params: { spaceCatId: ORGANIZATION_ID, brandId: BRAND_UUID },
+        data: { baseSiteId: 'some-other-orgs-site' },
+        dataAccess: mockDataAccess,
+        attributes: { authInfo: { getType: () => 'ims', profile: { email: 'user@test.com' } } },
+      });
+
+      expect(response.status).to.equal(500);
+      const body = await response.json();
+      expect(body.message).to.equal(err.message); // body keeps the full, unsanitized message
+      expect(response.headers.get('x-error')).to.equal('DB read failed  connection reset');
+    });
+
     it('succeeds when expectedUpdatedAt matches the persisted row (LLMO-6591)', async () => {
       const response = await brandsController.updateBrandForOrg({
         ...context,

test/it/shared/tests/brands.js

@@ -16,6 +16,7 @@ import {
   BRAND_1_ID,
   SITE_2_ID,
   SITE_2_BASE_URL,
+  SITE_3_ID, // belongs to ORG_2, not ORG_1 (seed-data/sites.js) — used for cross-org tests
   MARKET_SITE_1_ID,
   MARKET_SITE_1_BASE_URL,
 } from '../seed-ids.js';
@@ -237,6 +238,50 @@ export default function brandsTests(getHttpClient, resetData) {
     });
   });
 
+  describe('Brands v2 rejects a cross-org baseSiteId (serenity-docs#346)', () => {
+    before(() => resetData());
+
+    it('rejects a fresh create anchored to another org\'s site', async () => {
+      const http = getHttpClient();
+
+      // SITE_3_ID belongs to ORG_2 (seed-data/sites.js) — anchoring an ORG_1
+      // brand to it is exactly the org-ID mismatch signature the investigation
+      // traced (a brand silently pointing at a different org's site).
+      //
+      // status: 'pending' defers ALL Semrush provisioning (see createBrandForOrg),
+      // so this stays a plain flat-mode create regardless of ORG_1's serenity
+      // rollout flag — the guard fires on the baseSiteId anchor check itself,
+      // independent of that unrelated machinery.
+      const res = await http.admin.post(`/v2/orgs/${ORG_1_ID}/brands`, {
+        name: 'Cross-Org Anchor Attempt', region: ['US'], status: 'pending', baseSiteId: SITE_3_ID,
+      });
+
+      expect(res.status).to.equal(409);
+      expect(res.body.code).to.equal('brand_site_org_mismatch');
+    });
+
+    it('rejects setting an existing pending brand\'s baseSiteId to another org\'s site', async () => {
+      const http = getHttpClient();
+
+      const create = await http.admin.post(`/v2/orgs/${ORG_1_ID}/brands`, {
+        name: 'Pending Brand For Cross-Org Attempt', region: ['US'], status: 'pending',
+      });
+      expect(create.status).to.equal(201);
+      const { id: brandId } = create.body;
+
+      const res = await http.admin.patch(`/v2/orgs/${ORG_1_ID}/brands/${brandId}`, {
+        baseSiteId: SITE_3_ID,
+      });
+
+      expect(res.status).to.equal(409);
+      expect(res.body.code).to.equal('brand_site_org_mismatch');
+
+      // The brand must be untouched — no partial write.
+      const getRes = await http.admin.get(`/v2/orgs/${ORG_1_ID}/brands/${brandId}`);
+      expect(getRes.body.baseSiteId == null).to.equal(true);
+    });
+  });
+
   describe('Brands v2 delete frees the name for reuse (LLMO-6978)', () => {
     before(() => resetData());