From 18a428c7d465f58c963af738ab119a6bfacad454 Mon Sep 17 00:00:00 2001 From: pedrofrxncx Date: Mon, 13 Jul 2026 10:22:31 -0300 Subject: [PATCH] fix(commerce-onboarding): don't skip site re-claim on a malformed siteUrl param MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The site-match check added in #4439 (re-claim the connection when the requested site differs from the one it's currently claimed for) bypassed itself whenever normalizeCommerceSiteUrl failed — which happens both for an empty requestedSite (intentional: no site requested, trust the existing claim) and for a non-empty but malformed one (unintentional). A garbled ?siteUrl query param therefore skipped re-claiming and skipped the COMMERCE_DISCOVERY_RUN trigger, opening whatever site the connection happened to already be claimed for instead of surfacing the invalid-URL error the rest of the component already handles. Extracted the check into isConnectionClaimedForSite so "no site requested" and "invalid site requested" are distinguished explicitly, and added a regression test for the malformed-but-non-empty case. --- .../src/commerce-discovery/site-url.test.ts | 38 ++++++++++++++++++- apps/mesh/src/commerce-discovery/site-url.ts | 23 +++++++++++ .../src/web/routes/commerce-onboarding.tsx | 17 ++++----- 3 files changed, 68 insertions(+), 10 deletions(-) diff --git a/apps/mesh/src/commerce-discovery/site-url.test.ts b/apps/mesh/src/commerce-discovery/site-url.test.ts index a8e630fdfc..bfcb35dd34 100644 --- a/apps/mesh/src/commerce-discovery/site-url.test.ts +++ b/apps/mesh/src/commerce-discovery/site-url.test.ts @@ -1,5 +1,8 @@ import { describe, expect, test } from "bun:test"; -import { normalizeCommerceSiteUrl } from "./site-url.ts"; +import { + isConnectionClaimedForSite, + normalizeCommerceSiteUrl, +} from "./site-url.ts"; describe("normalizeCommerceSiteUrl", () => { test("adds https to bare hostnames", () => { @@ -100,3 +103,36 @@ describe("normalizeCommerceSiteUrl", () => { } }); }); + +describe("isConnectionClaimedForSite", () => { + test("trusts an existing claim when no site is requested (returning session)", () => { + expect(isConnectionClaimedForSite("", "https://a.com")).toBe(true); + expect(isConnectionClaimedForSite(" ", undefined)).toBe(true); + }); + + test("matches when the requested site equals the claimed site", () => { + expect( + isConnectionClaimedForSite("https://a.com/path", "https://a.com"), + ).toBe(true); + }); + + test("does not match when the requested site differs from the claimed site", () => { + expect(isConnectionClaimedForSite("https://b.com", "https://a.com")).toBe( + false, + ); + }); + + test("does not match when nothing is claimed yet", () => { + expect(isConnectionClaimedForSite("https://a.com", undefined)).toBe(false); + }); + + test("a malformed non-empty request is NOT treated like an empty one", () => { + // Regression: both "" and "not-a-url" fail normalizeCommerceSiteUrl, but + // only the empty case should bypass the site-match check — otherwise a + // garbled ?siteUrl param would silently reuse whatever site the + // connection happens to already be claimed for. + expect(isConnectionClaimedForSite("not-a-url", "https://a.com")).toBe( + false, + ); + }); +}); diff --git a/apps/mesh/src/commerce-discovery/site-url.ts b/apps/mesh/src/commerce-discovery/site-url.ts index 7a67e19baa..33157ff057 100644 --- a/apps/mesh/src/commerce-discovery/site-url.ts +++ b/apps/mesh/src/commerce-discovery/site-url.ts @@ -72,3 +72,26 @@ export function normalizeCommerceSiteUrl(input: string): CommerceSiteUrlResult { value: url.origin, }; } + +/** + * True when an already-provisioned Commerce Discovery connection's claimed + * site matches the one currently requested. + * + * An empty `requestedSite` means no site was specified (a returning session + * with no `?siteUrl`) — trust the existing claim. A non-empty but malformed + * `requestedSite` must NOT be treated the same as "no site requested": both + * fail `normalizeCommerceSiteUrl`, but only the empty case should bypass the + * match check — otherwise a garbled site param silently skips re-claiming + * and surfaces whatever site the connection happens to already be claimed + * for. + */ +export function isConnectionClaimedForSite( + requestedSite: string, + claimedSiteUrl: string | undefined, +): boolean { + if (!requestedSite.trim()) return true; + const requested = normalizeCommerceSiteUrl(requestedSite); + if (!requested.ok || !claimedSiteUrl) return false; + const claimed = normalizeCommerceSiteUrl(claimedSiteUrl); + return claimed.ok && claimed.value === requested.value; +} diff --git a/apps/mesh/src/web/routes/commerce-onboarding.tsx b/apps/mesh/src/web/routes/commerce-onboarding.tsx index 37d68d2716..d4479bca02 100644 --- a/apps/mesh/src/web/routes/commerce-onboarding.tsx +++ b/apps/mesh/src/web/routes/commerce-onboarding.tsx @@ -1,4 +1,7 @@ -import { normalizeCommerceSiteUrl } from "@/commerce-discovery/site-url"; +import { + isConnectionClaimedForSite, + normalizeCommerceSiteUrl, +} from "@/commerce-discovery/site-url"; import { AuthEntry } from "@/web/components/auth-entry"; import { ErrorBoundary } from "@/web/components/error-boundary"; import { AuthSplitLayout } from "@/web/components/auth-split-layout"; @@ -778,14 +781,10 @@ function CommerceSetupContent({ // requested site; when the site differs, fall through to setup (idempotent // re-claim) so the token + metadata.siteUrl follow the site being onboarded. const requestedSite = initialSiteUrl || siteUrlInput || ""; - const requestedNormalized = normalizeCommerceSiteUrl(requestedSite); - const claimedNormalized = connectionSiteUrl - ? normalizeCommerceSiteUrl(connectionSiteUrl) - : null; - const claimedForRequestedSite = - !requestedNormalized.ok || - (!!claimedNormalized?.ok && - claimedNormalized.value === requestedNormalized.value); + const claimedForRequestedSite = isConnectionClaimedForSite( + requestedSite, + connectionSiteUrl, + ); const setupReady = connectionExists && claimedForRequestedSite; const currentSiteUrl = initialSiteUrl || siteUrlInput || connectionSiteUrl || "";