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(', '); }