diff --git a/.changeset/28736-rest-base-url-query.md b/.changeset/28736-rest-base-url-query.md new file mode 100644 index 00000000000..de23e62bf18 --- /dev/null +++ b/.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. diff --git a/packages/connectors/connector-rest/src/rest-connector.test.ts b/packages/connectors/connector-rest/src/rest-connector.test.ts index ccb1fcf3b50..a82f35b2ca5 100644 --- a/packages/connectors/connector-rest/src/rest-connector.test.ts +++ b/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, auth?: Parameters[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®ion=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', () => { diff --git a/packages/connectors/connector-rest/src/rest-connector.ts b/packages/connectors/connector-rest/src/rest-connector.ts index a0721192b9a..9180629bef4 100644 --- a/packages/connectors/connector-rest/src/rest-connector.ts +++ b/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)); diff --git a/packages/qa/dogfood/test/connector-meta-door-entry-and-url-hardening.dogfood.test.ts b/packages/qa/dogfood/test/connector-meta-door-entry-and-url-hardening.dogfood.test.ts index 870594c4c02..11ac81af6d9 100644 --- a/packages/qa/dogfood/test/connector-meta-door-entry-and-url-hardening.dogfood.test.ts +++ b/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}®ion=eu`); + expect(await dispatch(CARRIED, '/probe')).toBe(`/v1/probe?api_key=${PLACEHOLDERS.carried}®ion=eu`); }); }); });