fix(connector-rest): a baseUrl that carries a query string sends each action to its own path

.changeset/28736-rest-base-url-query.md

@@ -0,0 +1,12 @@
+---
+'@objectstack/connector-rest': patch
+---
+
+fix(connector-rest): a `baseUrl` that carries a query string sends each action to its own path
+
+Clause-②: no
+Metadata-protocol: none
+
+A REST connector whose `baseUrl` carries a fixed query parameter (`https://api.example.com/v1?region=eu`, `?api-version=2024-01`) used to put every action's path inside that query: the base and the path were joined as text, so `/items` became `…/v1?region=eu/items` and the request went to `/v1`. The request URL is now built from the parsed base: the path joins the base's path with one slash between, the base's query parameters stay the query, and the action's own parameters (written inline in `path`, or in `query`, an api-key `paramName` included) come after them. **On a clash the action's value wins**: it is the more specific intent, and refusing would break a working action the day its base gains the parameter.
+
+A base without a query sends exactly the URL it sent before, and a path still only ever lands under the base's origin (a path like `//other.example` or an absolute URL stays a path segment). A base fragment keeps the place the text join gave it.

packages/connectors/connector-rest/src/rest-connector.test.ts

@@ -89,6 +89,81 @@ describe('createRestConnector — request action', () => {
     });
 });
 
+// ─── request URL: a base URL that carries its own query ──────────────
+
+describe('createRestConnector — base URL with a query string', () => {
+    /** The URL one `request` dispatch sent, for a base URL and an action input. */
+    async function sentUrl(baseUrl: string, input: Record<string, unknown>, auth?: Parameters<typeof createRestConnector>[0]['auth']) {
+        const { impl, calls } = stubFetch();
+        const { handlers } = createRestConnector({ baseUrl, auth, fetchImpl: impl });
+        await handlers.request(input, {});
+        expect(calls).toHaveLength(1);
+        return calls[0].url;
+    }
+
+    it.each([
+        ['a path', 'https://api.example.com/v1?region=eu', '/items', 'https://api.example.com/v1/items?region=eu'],
+        ['a path without its leading slash', 'https://api.example.com/v1?region=eu', 'items', 'https://api.example.com/v1/items?region=eu'],
+        ['a base with a trailing slash', 'https://api.example.com/v1/?region=eu', '/items', 'https://api.example.com/v1/items?region=eu'],
+        ['a base with no path', 'https://api.example.com?api-version=2024-01', '/items', 'https://api.example.com/items?api-version=2024-01'],
+        ['several base parameters', 'https://api.example.com/v1?region=eu&api-version=2024-01', '/items/7', 'https://api.example.com/v1/items/7?region=eu&api-version=2024-01'],
+        ['an empty path', 'https://api.example.com/v1?region=eu', '', 'https://api.example.com/v1?region=eu'],
+    ])('%s: the path joins the base path and the base query stays the query', async (_label, baseUrl, path, expected) => {
+        expect(await sentUrl(baseUrl, { path })).toBe(expected);
+    });
+
+    it('merges the action query after the base query', async () => {
+        expect(await sentUrl('https://api.example.com/v1?region=eu', { path: '/items', query: { page: 2 } }))
+            .toBe('https://api.example.com/v1/items?region=eu&page=2');
+    });
+
+    it('a clash with the base parameter: the action query wins', async () => {
+        // `query` replaces the value where the parameter already stands.
+        expect(await sentUrl('https://api.example.com/v1?region=eu&api-version=1', { path: '/items', query: { region: 'us' } }))
+            .toBe('https://api.example.com/v1/items?region=us&api-version=1');
+    });
+
+    it('a clash with the base parameter: a query written inline in the path wins too', async () => {
+        expect(await sentUrl('https://api.example.com/v1?region=eu&api-version=1', { path: '/items?region=us&page=2' }))
+            .toBe('https://api.example.com/v1/items?api-version=1&region=us&page=2');
+    });
+
+    it('an api-key query parameter rides beside the base query', async () => {
+        expect(await sentUrl('https://api.example.com/v1?region=eu', { path: '/me' }, { type: 'api-key', key: 'k-1', paramName: 'api_key' }))
+            .toBe('https://api.example.com/v1/me?region=eu&api_key=k-1');
+    });
+
+    it('a base fragment keeps the place the text join gave it', async () => {
+        expect(await sentUrl('https://api.example.com/v1?region=eu#frag', { path: '/items' }))
+            .toBe('https://api.example.com/v1?region=eu#frag/items');
+        expect(await sentUrl('https://api.example.com/v1#frag', { path: '/items' }))
+            .toBe('https://api.example.com/v1#frag/items');
+    });
+
+    it.each([
+        ['no query', 'https://api.example.com/v1'],
+        ['a query', 'https://api.example.com/v1?region=eu'],
+    ])('a path never leaves the base origin (base with %s)', async (_label, baseUrl) => {
+        for (const path of ['//evil.example/x', 'https://evil.example/x', '\\\\evil.example/x', '/@evil.example/x', '/../../x']) {
+            const sent = new URL(await sentUrl(baseUrl, { path }));
+            expect(sent.origin, path).toBe('https://api.example.com');
+            expect(sent.username, path).toBe('');
+            expect(sent.search, path).toBe(new URL(baseUrl).search);
+        }
+    });
+
+    it('a base without a query sends the URL the text join sent', async () => {
+        const cases: Array<[string, string, string]> = [
+            ['https://api.example.com/v1', '/items?page=2', 'https://api.example.com/v1/items?page=2'],
+            ['https://api.example.com/v1/', 'items', 'https://api.example.com/v1/items'],
+            ['https://api.example.com/v1', '//evil.example/x', 'https://api.example.com/v1//evil.example/x'],
+            ['https://api.example.com/v1', 'https://evil.example/x', 'https://api.example.com/v1/https://evil.example/x'],
+            ['https://api.example.com', '', 'https://api.example.com/'],
+        ];
+        for (const [baseUrl, path, expected] of cases) expect(await sentUrl(baseUrl, { path }), `${baseUrl} + ${path}`).toBe(expected);
+    });
+});
+
 // ─── auth injection ──────────────────────────────────────────────────
 
 describe('createRestConnector — static auth', () => {

packages/connectors/connector-rest/src/rest-connector.ts

@@ -100,11 +100,37 @@ export interface RestConnectorBundle {
     >;
 }
 
-/** Build the request URL from base + path + query, encoding query params. */
+/**
+ * Build the request URL from base + path + query, encoding query params.
+ *
+ * The base is parsed first and its own query is lifted off before the path is
+ * joined: a text join of `https://api.example.com/v1?region=eu` and `/items`
+ * reads `…/v1?region=eu/items`, which sends every path-bearing action to `/v1`
+ * with the path inside the `region` value. The path is then joined onto the
+ * base's path with one slash between, exactly as before, so a base without a
+ * query yields the same URL byte for byte, and a path can only ever land under
+ * the base's origin: it is appended after the base's path, never resolved
+ * against it, so `//other.example` or an absolute URL stays a path segment.
+ *
+ * The base's query parameters come back first, then the action's own: those
+ * written inline in `path`, then `query` (auth's query parameter included).
+ * **On a clash the action's value wins** — it is the more specific intent, and
+ * refusing would break a working action the day its base gains the parameter.
+ * A base fragment is left where the text join put it, as before.
+ */
 function buildUrl(baseUrl: string, path: string, query?: RestRequestInput['query']): string {
-    const base = baseUrl.replace(/\/+$/, '');
+    const base = new URL(baseUrl);
+    const baseParams = [...base.searchParams];
+    base.search = '';
     const suffix = path ? (path.startsWith('/') ? path : `/${path}`) : '';
-    const url = new URL(base + suffix);
+    const url = new URL(base.toString().replace(/\/+$/, '') + suffix);
+    if (baseParams.length > 0) {
+        const own = [...url.searchParams];
+        const ownKeys = new Set(own.map(([key]) => key));
+        const merged = new URLSearchParams(baseParams.filter(([key]) => !ownKeys.has(key)));
+        for (const [key, value] of own) merged.append(key, value);
+        url.search = merged.toString();
+    }
     if (query) {
         for (const [key, value] of Object.entries(query)) {
             if (value !== undefined && value !== null) url.searchParams.set(key, String(value));

packages/qa/dogfood/test/connector-meta-door-entry-and-url-hardening.dogfood.test.ts

@@ -267,9 +267,9 @@ describe('connector metadata doors: undeclared entry keys refused; URL credentia
       expect(atRest.providerConfig.baseUrl).toBe(carriedUrl);
 
       await waitReady(CARRIED);
-      // No request path: the rest provider appends a path to the base URL as
-      // text, so the base URL's own query stays the query only without one.
+      // The base URL's own query stays the query with and without a request path.
       expect(await dispatch(CARRIED, '')).toBe(`/v1?api_key=${PLACEHOLDERS.carried}&region=eu`);
+      expect(await dispatch(CARRIED, '/probe')).toBe(`/v1/probe?api_key=${PLACEHOLDERS.carried}&region=eu`);
     });
   });
 });