Skip to content

Commit cd0ec76

Browse files
fix(connector-rest): a baseUrl that carries a query string sends each action to its own path (#28767)
<!-- claude-projects-attribution: {"github_login":"os-project-manager"} --> _Requested by **Leehom** · [project thread](https://claude.ai/code/project/chan_01TenzsRopZj4jMbvjX8uz1X?thread=cmsg_01TenzsRopZj4jMbvjX8uz1XHVWgh9oMbncfSuTB7i56uG)_ Fixes #28736 Clause-②: no — no accepted value or public member changes; a path-bearing action on a base URL with a query now reaches its own path. Metadata-protocol: none ## 维护者速读 - 改了什么:REST 连接器在 `baseUrl` 自带查询参数时(例如 `https://api.example.com/v1?region=eu`),动作的路径不再被拼进查询串里,而是拼在基址路径后面:`/items` 发往 `/v1/items?region=eu`。基址的查询参数保留在前,动作自己的参数(写在 path 里的,或 `query` 里的,含 api-key 的查询参数)跟在后面;同名时以动作的值为准。 - 为什么改:原来基址和路径按文本拼接,`?region=eu` 加 `/items` 得到 `?region=eu/items`,请求打到 `/v1`,作者只看到对方 404 或错数据,毫无线索。带固定查询参数的基址是常见的服务商写法(`?api-version=…`)。 - 风险与回滚:不带查询参数的基址发出的 URL 与原来逐字节一致(480 组输入对照全同);路径仍只能落在基址的源(origin)之下,`//other.example` 或完整 URL 仍是路径的一段,有测试钉住。回滚即还原本 PR。 - 先看:`packages/connectors/connector-rest/src/rest-connector.ts` 的 `buildUrl`(查询参数先摘下、再拼路径、再合并,同名动作优先);`src/rest-connector.test.ts` 新增的 "base URL with a query string" 一节。 - 两分钟验证:`pnpm --filter @objectstack/connector-rest exec vitest run src/rest-connector.test.ts`,预期 24/24 通过(其中新增 14 例,10 例在 main 上为红)。 - 需要你做的:无,席位落地。 ## What changes - **A base URL with a query string.** `buildUrl` now parses the base, lifts its query off, joins the action path onto the base's path exactly as before (trailing slashes of the base dropped, one slash between), and puts the base's query parameters back first, then the action's own: those written inline in `path`, then `query` (an api-key `paramName` rides `query`). So `https://api.example.com/v1?region=eu` plus `/items` sends `https://api.example.com/v1/items?region=eu` (on `main`: `https://api.example.com/v1?region=eu/items`, which goes to `/v1`). - **On a clash the action's value wins.** A parameter the action sets (in `query` or inline in `path`) replaces the base's value. Picked over refusing because refusing would break a working action the day its base gains the parameter; stated in the code comment and the changeset. - **Unchanged.** A base without a query sends byte-for-byte the URL it sent before. A path can only land under the base's origin: it is appended after the base's path, never resolved against it, so `//evil.example/x`, `https://evil.example/x`, a backslash form or `/@evil.example` stay path segments (pinned, for a base with and without a query). A base fragment keeps the place the text join gave it (measured below). - **Dogfood.** The carry-forward case of `connector-meta-door-entry-and-url-hardening.dogfood.test.ts` dispatched with an empty path only to avoid this bug; it now also dispatches `/probe` through the materialized instance and pins `/v1/probe?api_key=…&region=eu` at the upstream. ## Tests - **Unit, `rest-connector.test.ts`, new describe "base URL with a query string" (14 cases):** a path, a path without a leading slash, a base with a trailing slash, a base with no path, several base parameters, an empty path; action `query` merged after the base query; a clash with `query` and a clash with an inline path query (action wins); an api-key query parameter beside the base query; the base fragment's place; a path never leaving the base origin (base with and without a query, five hostile path shapes); a base without a query sending the text join's URL (five shapes including `//evil.example/x` and an inline path query). - **Dogfood:** the carry-forward case gains the path-bearing dispatch. <details><summary>Red on base, green on head, ablations, controls</summary> - Red (unit): `rest-connector.ts` restored to base `7a48bf73`: `10 failed | 14 passed (24)`, the ten query-base cases; the empty-path, fragment, no-query origin and no-query byte-identity cases and every existing case pass on base. Restored with `git checkout HEAD`, `git diff HEAD` empty. - Red (dogfood): `rest-connector.ts` at base, `@objectstack/connector-rest` rebuilt (new code absent from `dist/`): `1 failed | 8 passed (9)`, Expected `/v1/probe?api_key=TEST_PLACEHOLDER_CARRIED_QUERY&region=eu`, Received `/v1?api_key=TEST_PLACEHOLDER_CARRIED_QUERY&region=eu/probe`. Restored and rebuilt (new code back in `dist/`), tree clean. - Green on head: unit `24 passed (24)`; package suite `5 passed` files, `57 passed` tests; dogfood file `9 passed (9)`. - Ablation 1 (`scripts/ablation-replace.mjs`: the base query is not lifted, `base.search = base.search;`): `10 failed | 14 passed (24)`, the same ten query-base cases only. Restored: blob `9180629b` == HEAD, `git diff HEAD` empty. - Ablation 2 (the inline-path clash rule off: base parameters kept even when the path sets them): `1 failed | 23 passed (24)`, the inline clash case only. Restored: blob == HEAD, `git diff HEAD` empty. - Control sweep (one-off script against the built `dist/`, not a kept test): 10 query-free bases (incl. uppercase host, `//v1//`, dot segments, userinfo, explicit default port, a fragment, an encoded space) × 16 paths (incl. `//evil.example/x`, an absolute URL, a backslash form, `/../x`, `?only=1`, `/items?`, `//`) × 3 `query` values = 480 cases: new URL == the base text join's URL in all 480. Positive control: adding one query base to the same sweep makes 45 of its 48 cases differ (the 3 identical are the empty path). </details> <details><summary>Base readings (text join on main)</summary> - `https://api.example.com/v1?region=eu` + `/items` → `https://api.example.com/v1?region=eu/items` (request goes to `/v1`). - `…/v1?region=eu` + `/items` with `query: { region: 'us' }` → `https://api.example.com/v1?region=us` (the path is lost entirely). - `…/v1#frag` + `/items` → `https://api.example.com/v1#frag/items`; `…/v1?region=eu#frag` + `/items` → `…/v1?region=eu#frag/items`. Both unchanged on head: the path still lands inside a base fragment, so the wire request goes to `/v1`. - `…/v1` + `//evil.example/x` → `https://api.example.com/v1//evil.example/x`; `…/v1` + `https://evil.example/x` → `https://api.example.com/v1/https://evil.example/x`. No path left the base host on main either; unchanged on head. </details> <details><summary>Package suite and type check</summary> - `pnpm --filter @objectstack/connector-rest build && pnpm --filter @objectstack/connector-rest typecheck`: exit 0 (`check-dts-emitted` 2/2, `check:test-typecheck` OK). - `pnpm --filter @objectstack/connector-rest test`: `5 passed` files, `57 passed` tests. </details> ## Acceptance notes - **No origin/scheme/host checks exist in the request path today.** PR #28688's URL hardening is read-side redaction of credential parts and the save-door refusal of undeclared entry keys (`packages/spec`); neither sits in `buildUrl` or the `rest` provider, and none is touched here. What stops a path from changing the origin is the append-only join, unchanged, and now pinned for both base shapes. - **A base fragment still swallows the path** (kept as measured, per the order). Is a `#` in a `baseUrl` ever meaningful for an HTTP API? Not handled here; with the seat. - A path starting `//` keeps its double slash after the base path, as on main; "one slash between" holds for every path that does not itself start with two slashes. ## Verification Every local reading above is on head `a9a80de3`. Derived gate families (`node scripts/pm/dispatch-gates.mjs --commands`, reconciled with `--ran`): 61 derived, 60 run with exit 0, 1 not measured (`check:dual-build-cjs-loads`, exit 3: its prerequisite, every package's `dist/`, is not met in this checkout). `check-plugin-teardown-shape --self-test` first answered exit 3 (its pinned fixture commit was outside the shallow clone); after `git fetch --depth=1` of that commit it answered exit 0 (48 cases). `check-nul-bytes`: OK. The repo-wide lint, `test:repo` and the full farm are CI's. Full dogfood not run: no `!` in the title and no `(narrowing)` arm. Overlap: no open PR touches `packages/connectors/connector-rest/` or the dogfood pin (14 open code PRs' heads diffed against their merge bases). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01FEkQ5oht6tMAsnKGYsgbiT Co-authored-by: Claude <noreply@anthropic.com>
1 parent 52de5a5 commit cd0ec76

4 files changed

Lines changed: 118 additions & 5 deletions

File tree

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'@objectstack/connector-rest': patch
3+
---
4+
5+
fix(connector-rest): a `baseUrl` that carries a query string sends each action to its own path
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
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.
11+
12+
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‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,81 @@ describe('createRestConnector — request action', () => {
8989
});
9090
});
9191

92+
// ─── request URL: a base URL that carries its own query ──────────────
93+
94+
describe('createRestConnector — base URL with a query string', () => {
95+
/** The URL one `request` dispatch sent, for a base URL and an action input. */
96+
async function sentUrl(baseUrl: string, input: Record<string, unknown>, auth?: Parameters<typeof createRestConnector>[0]['auth']) {
97+
const { impl, calls } = stubFetch();
98+
const { handlers } = createRestConnector({ baseUrl, auth, fetchImpl: impl });
99+
await handlers.request(input, {});
100+
expect(calls).toHaveLength(1);
101+
return calls[0].url;
102+
}
103+
104+
it.each([
105+
['a path', 'https://api.example.com/v1?region=eu', '/items', 'https://api.example.com/v1/items?region=eu'],
106+
['a path without its leading slash', 'https://api.example.com/v1?region=eu', 'items', 'https://api.example.com/v1/items?region=eu'],
107+
['a base with a trailing slash', 'https://api.example.com/v1/?region=eu', '/items', 'https://api.example.com/v1/items?region=eu'],
108+
['a base with no path', 'https://api.example.com?api-version=2024-01', '/items', 'https://api.example.com/items?api-version=2024-01'],
109+
['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'],
110+
['an empty path', 'https://api.example.com/v1?region=eu', '', 'https://api.example.com/v1?region=eu'],
111+
])('%s: the path joins the base path and the base query stays the query', async (_label, baseUrl, path, expected) => {
112+
expect(await sentUrl(baseUrl, { path })).toBe(expected);
113+
});
114+
115+
it('merges the action query after the base query', async () => {
116+
expect(await sentUrl('https://api.example.com/v1?region=eu', { path: '/items', query: { page: 2 } }))
117+
.toBe('https://api.example.com/v1/items?region=eu&page=2');
118+
});
119+
120+
it('a clash with the base parameter: the action query wins', async () => {
121+
// `query` replaces the value where the parameter already stands.
122+
expect(await sentUrl('https://api.example.com/v1?region=eu&api-version=1', { path: '/items', query: { region: 'us' } }))
123+
.toBe('https://api.example.com/v1/items?region=us&api-version=1');
124+
});
125+
126+
it('a clash with the base parameter: a query written inline in the path wins too', async () => {
127+
expect(await sentUrl('https://api.example.com/v1?region=eu&api-version=1', { path: '/items?region=us&page=2' }))
128+
.toBe('https://api.example.com/v1/items?api-version=1&region=us&page=2');
129+
});
130+
131+
it('an api-key query parameter rides beside the base query', async () => {
132+
expect(await sentUrl('https://api.example.com/v1?region=eu', { path: '/me' }, { type: 'api-key', key: 'k-1', paramName: 'api_key' }))
133+
.toBe('https://api.example.com/v1/me?region=eu&api_key=k-1');
134+
});
135+
136+
it('a base fragment keeps the place the text join gave it', async () => {
137+
expect(await sentUrl('https://api.example.com/v1?region=eu#frag', { path: '/items' }))
138+
.toBe('https://api.example.com/v1?region=eu#frag/items');
139+
expect(await sentUrl('https://api.example.com/v1#frag', { path: '/items' }))
140+
.toBe('https://api.example.com/v1#frag/items');
141+
});
142+
143+
it.each([
144+
['no query', 'https://api.example.com/v1'],
145+
['a query', 'https://api.example.com/v1?region=eu'],
146+
])('a path never leaves the base origin (base with %s)', async (_label, baseUrl) => {
147+
for (const path of ['//evil.example/x', 'https://evil.example/x', '\\\\evil.example/x', '/@evil.example/x', '/../../x']) {
148+
const sent = new URL(await sentUrl(baseUrl, { path }));
149+
expect(sent.origin, path).toBe('https://api.example.com');
150+
expect(sent.username, path).toBe('');
151+
expect(sent.search, path).toBe(new URL(baseUrl).search);
152+
}
153+
});
154+
155+
it('a base without a query sends the URL the text join sent', async () => {
156+
const cases: Array<[string, string, string]> = [
157+
['https://api.example.com/v1', '/items?page=2', 'https://api.example.com/v1/items?page=2'],
158+
['https://api.example.com/v1/', 'items', 'https://api.example.com/v1/items'],
159+
['https://api.example.com/v1', '//evil.example/x', 'https://api.example.com/v1//evil.example/x'],
160+
['https://api.example.com/v1', 'https://evil.example/x', 'https://api.example.com/v1/https://evil.example/x'],
161+
['https://api.example.com', '', 'https://api.example.com/'],
162+
];
163+
for (const [baseUrl, path, expected] of cases) expect(await sentUrl(baseUrl, { path }), `${baseUrl} + ${path}`).toBe(expected);
164+
});
165+
});
166+
92167
// ─── auth injection ──────────────────────────────────────────────────
93168

94169
describe('createRestConnector — static auth', () => {

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

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -100,11 +100,37 @@ export interface RestConnectorBundle {
100100
>;
101101
}
102102

103-
/** Build the request URL from base + path + query, encoding query params. */
103+
/**
104+
* Build the request URL from base + path + query, encoding query params.
105+
*
106+
* The base is parsed first and its own query is lifted off before the path is
107+
* joined: a text join of `https://api.example.com/v1?region=eu` and `/items`
108+
* reads `…/v1?region=eu/items`, which sends every path-bearing action to `/v1`
109+
* with the path inside the `region` value. The path is then joined onto the
110+
* base's path with one slash between, exactly as before, so a base without a
111+
* query yields the same URL byte for byte, and a path can only ever land under
112+
* the base's origin: it is appended after the base's path, never resolved
113+
* against it, so `//other.example` or an absolute URL stays a path segment.
114+
*
115+
* The base's query parameters come back first, then the action's own: those
116+
* written inline in `path`, then `query` (auth's query parameter included).
117+
* **On a clash the action's value wins** — it is the more specific intent, and
118+
* refusing would break a working action the day its base gains the parameter.
119+
* A base fragment is left where the text join put it, as before.
120+
*/
104121
function buildUrl(baseUrl: string, path: string, query?: RestRequestInput['query']): string {
105-
const base = baseUrl.replace(/\/+$/, '');
122+
const base = new URL(baseUrl);
123+
const baseParams = [...base.searchParams];
124+
base.search = '';
106125
const suffix = path ? (path.startsWith('/') ? path : `/${path}`) : '';
107-
const url = new URL(base + suffix);
126+
const url = new URL(base.toString().replace(/\/+$/, '') + suffix);
127+
if (baseParams.length > 0) {
128+
const own = [...url.searchParams];
129+
const ownKeys = new Set(own.map(([key]) => key));
130+
const merged = new URLSearchParams(baseParams.filter(([key]) => !ownKeys.has(key)));
131+
for (const [key, value] of own) merged.append(key, value);
132+
url.search = merged.toString();
133+
}
108134
if (query) {
109135
for (const [key, value] of Object.entries(query)) {
110136
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‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -267,9 +267,9 @@ describe('connector metadata doors: undeclared entry keys refused; URL credentia
267267
expect(atRest.providerConfig.baseUrl).toBe(carriedUrl);
268268

269269
await waitReady(CARRIED);
270-
// No request path: the rest provider appends a path to the base URL as
271-
// text, so the base URL's own query stays the query only without one.
270+
// The base URL's own query stays the query with and without a request path.
272271
expect(await dispatch(CARRIED, '')).toBe(`/v1?api_key=${PLACEHOLDERS.carried}&region=eu`);
272+
expect(await dispatch(CARRIED, '/probe')).toBe(`/v1/probe?api_key=${PLACEHOLDERS.carried}&region=eu`);
273273
});
274274
});
275275
});

0 commit comments

Comments
 (0)