From d01ffd5108d1ec085bcda4e8058789fb5d04dcf2 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 7 Nov 2026 15:25:26 +0800 Subject: [PATCH] fix(channels): exclude the channel's own API host and paths from URL shortening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 review findings on #16493, all closed in this commit: - R1-1 (Critical, fix-induced): the host gate re-admitted the channel's own API host — with baseUrl set, a pasted REST URL shortened to `repos/octo` (api.github.com) or `api/v3` (GHES /api/v3), phantom repositories both validators accepted. Only the WEB host now qualifies: an api.-prefixed baseUrl host contributes only its stripped form, and paths under the baseUrl's own API prefix are excluded. Genuine GHES web URLs still shorten. Pinned with baseUrl actually passed through validation, for both REST shapes; re-admitting the raw API host reddens the test. - R2-1: the baseUrl argument now has a call-site witness on both paths — a GHES draft validates clean and saves ['octo/repo'] through buildChannelUpsertRequest, mirrored in the parity table against the daemon authority. Dropping either argument reddens its half. - Deferred notes (all three closed, none deferred onward): trailing sentence punctuation is stripped before matching (a pasted `…/octo/repo.` no longer becomes the phantom `octo/repo.`); `enterprises` joins the reserved set (snapshot updated); and a dialog case witnesses the parked-list round trip — hidden while Off, restored with its text on re-enable. Mutation witnesses: re-admitting the raw API host, dropping the validator baseUrl argument, and dropping the save-path baseUrl argument each fail exactly their new test; all restored byte-identical. --- docs/design/github-repo-filter.md | 23 ++--- docs/design/github-repo-filter.zh-CN.md | 19 ++-- .../channels/ChannelEditorDialog.test.tsx | 31 +++++++ .../channels/channel-editor-parity.test.ts | 30 +++++++ .../channels/channel-editor-state.test.ts | 77 ++++++++++++++++ .../channels/channel-editor-state.ts | 90 +++++++++++-------- 6 files changed, 217 insertions(+), 53 deletions(-) diff --git a/docs/design/github-repo-filter.md b/docs/design/github-repo-filter.md index 1d9a947e5c7..edbf20129fd 100644 --- a/docs/design/github-repo-filter.md +++ b/docs/design/github-repo-filter.md @@ -108,16 +108,19 @@ Flat fields (not one object field) because the Web Shell editor cannot edit - `channel-editor-state.ts`: - `normalizeRepositoryFilterInput(value, baseUrl)` — splits on commas, - trims, lowercases, shortens GitHub web URLs - (`https://github.com/owner/repo`, with optional `.git`, trailing slash, - query, fragment, or `/pull/123`-style suffix) to `owner/repo`, - deduplicates, joins with `, `. Two gates decide whether a URL is - shortened at all. Structurally, only URLs whose host is the channel's - own GitHub host qualify: `github.com` plus the host derived from - `baseUrl` for GitHub Enterprise (an `api.` prefix is stripped); any - other host — `api.github.com`, `gist.github.com`, `gitlab.com`, an - arbitrary site — passes through unchanged for validation to reject, - which closes the "first two segments of any host" class. On a qualified + trims, strips trailing sentence punctuation a paste can carry, lowercases, + shortens GitHub web URLs (`https://github.com/owner/repo`, with optional + `.git`, trailing slash, query, fragment, or `/pull/123`-style suffix) to + `owner/repo`, deduplicates, joins with `, `. Two gates decide whether a + URL is shortened at all. Structurally, only URLs on the channel's own + GitHub WEB host qualify: `github.com` plus the web host derived from + `baseUrl` for GitHub Enterprise — an `api.`-prefixed baseUrl host + contributes only its stripped form, never the raw API host, and paths + under the baseUrl's own API prefix (e.g. `/api/v3/`) are excluded, so a + pasted REST URL cannot shorten to `repos/octo` or `api/v3`. Any other + host — `gist.github.com`, `gitlab.com`, an arbitrary site — passes + through unchanged for validation to reject, which closes the "first two + segments of any host" class. On a qualified host, a URL whose first path segment is a reserved GitHub area (`orgs`, `apps`, `settings`, `trending`, ...) still passes through unchanged so validation rejects it instead of silently filtering a nonexistent diff --git a/docs/design/github-repo-filter.zh-CN.md b/docs/design/github-repo-filter.zh-CN.md index dbc504faeaa..4af72a459e8 100644 --- a/docs/design/github-repo-filter.zh-CN.md +++ b/docs/design/github-repo-filter.zh-CN.md @@ -96,14 +96,17 @@ Allowed repositories」下面。没有黑名单，而且访问策略的框架让 - `channel-editor-state.ts`： - `normalizeRepositoryFilterInput(value, baseUrl)` —— 按逗号拆分、trim、 - 小写，把 GitHub web URL（`https://github.com/owner/repo`，可带 `.git`、 - 尾斜杠、query、fragment 或 `/pull/123` 式后缀）缩短为 `owner/repo`， - 去重后用 `, ` 连接。是否缩短由两道门决定。结构上，只有主机属于频道 - 自己的 GitHub 主机才可缩短：`github.com` 加上由 `baseUrl` 派生的 - GitHub Enterprise 主机（剥掉 `api.` 前缀）；其他任何主机—— - `api.github.com`、`gist.github.com`、`gitlab.com`、任意站点——原样 - 透传交给校验拒绝，从结构上关闭「任意主机的头两段路径」这一类隐患。 - 在合格主机上，首段路径是 GitHub 保留区域（`orgs`、`apps`、`settings`、 + 剥掉粘贴可能带上的句尾标点、小写，把 GitHub web URL + （`https://github.com/owner/repo`，可带 `.git`、尾斜杠、query、fragment + 或 `/pull/123` 式后缀）缩短为 `owner/repo`，去重后用 `, ` 连接。是否 + 缩短由两道门决定。结构上，只有频道自己的 GitHub WEB 主机上的 URL 才 + 可缩短：`github.com` 加上由 `baseUrl` 派生的 GitHub Enterprise web + 主机——带 `api.` 前缀的 baseUrl 主机只贡献去前缀后的形式，绝不加入 + 原始 API 主机；baseUrl 自身 API 前缀（如 `/api/v3/`）下的路径也被 + 排除，因此粘贴 REST URL 不会被缩短成 `repos/octo` 或 `api/v3`。其他 + 任何主机——`gist.github.com`、`gitlab.com`、任意站点——原样透传交给 + 校验拒绝，从结构上关闭「任意主机的头两段路径」这一类隐患。在合格 + 主机上，首段路径是 GitHub 保留区域（`orgs`、`apps`、`settings`、 `trending` 等）的 URL 仍原样透传，让校验拒绝它，而不是静默地去过滤 一个不存在的仓库。这份区域清单在构造上就是尽力而为——对 GitHub 顶层 路径做黑名单不可能穷尽，未来新增的区域在被补进清单之前会重新引入该 diff --git a/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx b/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx index 78d4ac080f3..4081c35dbc3 100644 --- a/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx +++ b/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx @@ -3998,6 +3998,37 @@ describe('GitHub repository event filter', () => { expect(request.config['repoFilter']).toEqual(['octo/repo']); }); + it('hides the list while Off and restores it on re-enable', async () => { + await renderDialog({ + descriptor: GITHUB_REPO_FILTER, + instance: { + ...FILTER_INSTANCE, + config: { + ...FILTER_INSTANCE.config, + repoFilterMode: 'blocklist', + repoFilter: ['octo/repo'], + }, + }, + }); + + expect(inputByLabel('Filtered Repositories')).toHaveProperty( + 'value', + 'octo/repo', + ); + + await selectOption('Repository Filter', 'Off — all repositories'); + expect(inputByLabel('Filtered Repositories')).toBeNull(); + + await selectOption( + 'Repository Filter', + 'Blocklist — all except listed repositories', + ); + expect(inputByLabel('Filtered Repositories')).toHaveProperty( + 'value', + 'octo/repo', + ); + }); + it('rejects entries that are not owner/repo after normalization', async () => { const onSave = vi.fn().mockResolvedValue(undefined); await renderDialog({ diff --git a/packages/web-shell/client/components/channels/channel-editor-parity.test.ts b/packages/web-shell/client/components/channels/channel-editor-parity.test.ts index f3e4a1b031a..a21d24a5bb4 100644 --- a/packages/web-shell/client/components/channels/channel-editor-parity.test.ts +++ b/packages/web-shell/client/components/channels/channel-editor-parity.test.ts @@ -207,4 +207,34 @@ describe('repository filter rule parity (editor mirror vs daemon authority)', () expect(config['repoFilter']).toEqual(['octo/repo']); expect(management.validateConfig?.(config)).toBeUndefined(); }); + + it('binds a GHES paste through the editor and the daemon authority', () => { + const instance: DaemonChannelInstanceSnapshot = { + name: 'parity', + config: { + type: 'github', + useLocalGh: true, + baseUrl: 'https://ghe.example.com/api/v3', + }, + secrets: {}, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + const draft = createChannelEditorDraft(descriptor, instance); + draft.name = 'parity'; + draft.values['repoFilterMode'] = 'blocklist'; + draft.values['repoFilter'] = 'https://ghe.example.com/octo/repo'; + + expect(validateChannelEditorDraft(descriptor, draft, [], instance)).toEqual( + {}, + ); + const { config } = buildChannelUpsertRequest( + descriptor, + draft, + 'rev-1', + instance, + ); + expect(config['repoFilter']).toEqual(['octo/repo']); + expect(management.validateConfig?.(config)).toBeUndefined(); + }); }); diff --git a/packages/web-shell/client/components/channels/channel-editor-state.test.ts b/packages/web-shell/client/components/channels/channel-editor-state.test.ts index 10a3f760bfe..b539ecdd506 100644 --- a/packages/web-shell/client/components/channels/channel-editor-state.test.ts +++ b/packages/web-shell/client/components/channels/channel-editor-state.test.ts @@ -2962,6 +2962,12 @@ describe('GitHub repository event filter', () => { ...GITHUB, fields: [ ...GITHUB.fields, + { + key: 'baseUrl', + label: 'Base URL', + kind: 'string', + envResolvable: true, + }, { key: 'repoFilterMode', label: 'Repository Filter', @@ -3048,6 +3054,76 @@ describe('GitHub repository event filter', () => { ).toBe('octo/repo'); }); + it('rejects REST-shaped URLs on the channel own API host', () => { + // The host gate must not re-admit the channel's own API host: with + // baseUrl set, a REST URL (the shape `gh api` prints) must pass through + // for validation to reject, not shorten to `repos/octo` or `api/v3`. + const cases = [ + ['https://api.github.com/repos/octo/repo', 'https://api.github.com'], + [ + 'https://ghe.example.com/api/v3/repos/octo/repo', + 'https://ghe.example.com/api/v3', + ], + ]; + for (const [url, baseUrl] of cases) { + expect(normalizeRepositoryFilterInput(url!, baseUrl)).toBe( + url!.toLowerCase(), + ); + const draft = filterDraft('allowlist', url!); + draft.values.baseUrl = baseUrl!; + expect(validateChannelEditorDraft(REPO_FILTER, draft, [])).toEqual({ + repoFilter: 'invalidRepoId', + }); + } + }); + + it('strips trailing sentence punctuation from a pasted URL', () => { + expect( + normalizeRepositoryFilterInput( + 'https://github.com/octo/repo., https://github.com/octo/repo)', + ), + ).toBe('octo/repo'); + expect( + validateChannelEditorDraft( + REPO_FILTER, + filterDraft('blocklist', 'https://github.com/octo/repo.'), + [], + ), + ).toEqual({}); + }); + + it('binds the GHES host through both the validator and the save path', () => { + // Dropping either baseUrl argument must fail here: the validator would + // reject the paste while the save path still shortens it (or vice + // versa) — the editor/daemon divergence the shared normalizer prevents. + const instance: DaemonChannelInstanceSnapshot = { + name: 'my-bot', + config: { + type: 'github', + useLocalGh: true, + baseUrl: 'https://ghe.example.com/api/v3', + }, + secrets: {}, + startsWithServe: false, + runtime: { state: 'stopped' }, + }; + const draft = filterDraft( + 'blocklist', + 'https://ghe.example.com/octo/repo', + instance, + ); + expect( + validateChannelEditorDraft(REPO_FILTER, draft, [], instance), + ).toEqual({}); + const request = buildChannelUpsertRequest( + REPO_FILTER, + draft, + 'rev-1', + instance, + ); + expect(request.config['repoFilter']).toEqual(['octo/repo']); + }); + it('does not shorten non-repository GitHub URLs', () => { // Regression pins for the measured verify-round shapes: each of these // once shortened to a plausible-but-nonexistent owner/repo that both @@ -3094,6 +3170,7 @@ describe('GitHub repository event filter', () => { 'customer-stories', 'discussions', 'enterprise', + 'enterprises', 'events', 'explore', 'features', diff --git a/packages/web-shell/client/components/channels/channel-editor-state.ts b/packages/web-shell/client/components/channels/channel-editor-state.ts index 89c6191f919..2172f2c9834 100644 --- a/packages/web-shell/client/components/channels/channel-editor-state.ts +++ b/packages/web-shell/client/components/channels/channel-editor-state.ts @@ -124,6 +124,7 @@ export const GITHUB_RESERVED_URL_PATHS = new Set([ 'customer-stories', 'discussions', 'enterprise', + 'enterprises', 'events', 'explore', 'features', @@ -161,60 +162,79 @@ export const GITHUB_RESERVED_URL_PATHS = new Set([ /** * The hosts whose two-segment paths may be repository URLs: github.com, plus - * the channel's own GitHub Enterprise host derived from baseUrl (the web host - * equals the API host there, and an `api.` prefix is stripped the same way - * githubEventWebUrl does). Any other host — api.github.com, gist.github.com, - * gitlab.com, an arbitrary site — passes through unchanged for validation to - * reject, which closes the "first two segments of any host" class structurally - * instead of through the area denylist. + * the channel's own GitHub Enterprise WEB host derived from baseUrl. Only the + * web host qualifies: an `api.`-prefixed baseUrl host denotes the deployment's + * API, so the stripped form is added and the raw API host is NOT (pasting a + * REST URL like https://api.github.com/repos/octo/repo must not shorten to + * `repos/octo`). The baseUrl's own API path prefix (e.g. /api/v3) is returned + * alongside so REST-shaped paths on a shared web+API host are excluded too. + * Any other host — gist.github.com, gitlab.com, an arbitrary site — passes + * through unchanged for validation to reject. */ -function repositoryUrlHosts(baseUrl: string): Set { +function repositoryUrlContext(baseUrl: string): { + hosts: Set; + apiPath: string; +} { const hosts = new Set(['github.com', 'www.github.com']); + let apiPath = ''; const trimmed = baseUrl.trim(); if (trimmed) { try { - const host = new URL(trimmed).hostname.toLowerCase(); - hosts.add(host); - if (host.startsWith('api.')) hosts.add(host.slice(4)); + const url = new URL(trimmed); + const host = url.hostname.toLowerCase(); + hosts.add(host.startsWith('api.') ? host.slice(4) : host); + const path = url.pathname.replace(/\/+$/, '').toLowerCase(); + if (path) apiPath = path; } catch { // A malformed baseUrl is its own field's validation error; only // github.com remains shortenable here. } } - return hosts; + return { hosts, apiPath }; } /** - * Canonicalize the repository filter input: lowercase, deduplicate, and - * shorten pasted GitHub web URLs (repository, PR, or issue pages, with an - * optional `.git`, trailing path, query, or fragment) to `owner/repo` — only - * when the URL's host is the channel's own GitHub host. Entries that are not - * repository URLs on that host pass through unchanged so validation can name - * them. Shared by the validator and the save path so the two cannot disagree. + * Canonicalize the repository filter input: lowercase, deduplicate, strip + * trailing sentence punctuation a paste can carry (`.`, `)`, …), and shorten + * GitHub web URLs (repository, PR, or issue pages, with an optional `.git`, + * trailing path, query, or fragment) to `owner/repo` — only when the URL's + * host is the channel's own GitHub web host and its path is not the baseUrl's + * API prefix. Entries that are not repository URLs on that host pass through + * unchanged so validation can name them. Shared by the validator and the save + * path so the two cannot disagree. */ export function normalizeRepositoryFilterInput( value: string, baseUrl = '', ): string { - const hosts = repositoryUrlHosts(baseUrl); - const entries = toList(value).map((entry) => { - const match = GITHUB_REPOSITORY_URL_RE.exec(entry); - let host = ''; - if (match) { - try { - host = new URL(entry).hostname.toLowerCase(); - } catch { - host = ''; + const { hosts, apiPath } = repositoryUrlContext(baseUrl); + const entries = toList(value) + .map((raw) => raw.replace(/[.,;:!?)\]]+$/, '')) + .filter(Boolean) + .map((entry) => { + const match = GITHUB_REPOSITORY_URL_RE.exec(entry); + let url: URL | undefined; + if (match) { + try { + url = new URL(entry); + } catch { + url = undefined; + } } - } - const shortened = - match && - hosts.has(host) && - !GITHUB_RESERVED_URL_PATHS.has(match[1].toLowerCase()) - ? `${match[1]}/${match[2]}` - : entry; - return shortened.toLowerCase(); - }); + const onApiPath = + url !== undefined && + apiPath !== '' && + url.pathname.toLowerCase().startsWith(`${apiPath}/`); + const shortened = + match && + url !== undefined && + hosts.has(url.hostname.toLowerCase()) && + !onApiPath && + !GITHUB_RESERVED_URL_PATHS.has(match[1].toLowerCase()) + ? `${match[1]}/${match[2]}` + : entry; + return shortened.toLowerCase(); + }); return [...new Set(entries)].join(', '); }