From fa7a3d0c85d681c2dda7e2636a08e9dc034cb5ac Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Sun, 11 Oct 2026 18:06:58 +0200 Subject: [PATCH 1/2] feat: Look Up Pull Requests on a Configured GitHub Enterprise Host --- librechat.example.yaml | 2 + packages/api/src/pulls/github.spec.ts | 38 +++++++++++- packages/api/src/pulls/github.ts | 34 +++++++---- packages/api/src/pulls/handler.spec.ts | 74 ++++++++++++++++++++++- packages/api/src/pulls/handler.ts | 53 ++++++++++------ packages/api/src/pulls/links.spec.ts | 6 +- packages/api/src/pulls/links.ts | 8 ++- packages/api/src/pulls/lookup.spec.ts | 10 +++ packages/api/src/pulls/lookup.ts | 4 +- packages/api/src/pulls/settings.spec.ts | 35 ++++++++++- packages/api/src/pulls/settings.ts | 13 ++++ packages/api/src/pulls/types.ts | 2 + packages/data-provider/src/config.spec.ts | 21 +++++++ packages/data-provider/src/config.ts | 13 ++++ 14 files changed, 274 insertions(+), 39 deletions(-) diff --git a/librechat.example.yaml b/librechat.example.yaml index 3932f7ae192..80d64276c58 100644 --- a/librechat.example.yaml +++ b/librechat.example.yaml @@ -940,6 +940,8 @@ endpoints: # # whose branch has moved on since its last command does. # pullRequests: # enabled: true # set false to turn the feature off + # # GitHub Enterprise Server or a *.ghe.com tenant; github.com when omitted. + # # host: "github.example.com" # # Environment variable reference only, never the token itself. Optional, see above. # token: "${GITHUB_PULL_REQUEST_TOKEN}" # # Repositories the token may be used for, as owner/name or owner/*. A worker reports its diff --git a/packages/api/src/pulls/github.spec.ts b/packages/api/src/pulls/github.spec.ts index 480c62946ec..a8bd6063497 100644 --- a/packages/api/src/pulls/github.spec.ts +++ b/packages/api/src/pulls/github.spec.ts @@ -1,4 +1,4 @@ -import { createGitHubPullRequestSource, summarizeChecks } from './github'; +import { createGitHubPullRequestSource, summarizeChecks, gitHubApiBase } from './github'; import { PullRequestSourceError } from './types'; const sha = 'a'.repeat(40); @@ -187,6 +187,13 @@ describe('createGitHubPullRequestSource', () => { await expect(find(source)).rejects.toMatchObject({ code: 'UPSTREAM_ERROR' }); }); + it('rejects a github.com page when the configured host is an Enterprise one', async () => { + const { source } = sourceFor(routes()); + await expect( + source.find({ repo: 'o/r', branch: 'feat/x', token: 't', host: 'ghe.example.com' }), + ).rejects.toMatchObject({ code: 'UPSTREAM_ERROR' }); + }); + it('bounds the title', async () => { const { source } = sourceFor(routes(pull({ title: 'x'.repeat(1000) }))); const result = await find(source); @@ -194,6 +201,35 @@ describe('createGitHubPullRequestSource', () => { }); }); +describe('GitHub host', () => { + it.each([ + ['github.com', 'https://api.github.com'], + ['acme.ghe.com', 'https://api.acme.ghe.com'], + ['ghe.example.com', 'https://ghe.example.com/api/v3'], + ['ghe.local:8443', 'https://ghe.local:8443/api/v3'], + ])('serves the API of %s from %s', (host, base) => { + expect(gitHubApiBase(host)).toBe(base); + }); + + it('sends every request of an Enterprise lookup to that host and accepts its pages', async () => { + const host = 'ghe.example.com'; + const fetchFn = jest.fn(async (input: string) => { + const url = String(input); + if (url.includes('/pulls?state=open')) return json(listed()); + if (url.includes('/pulls/7')) return json(pull({ html_url: `https://${host}/o/r/pull/7` })); + if (url.includes('/check-runs')) return json({ check_runs: [] }); + throw new Error(`unexpected ${url}`); + }); + const source = createGitHubPullRequestSource({ fetchFn }); + const result = await source.find({ repo: 'o/r', branch: 'feat/x', token: 't', host }); + expect(result).toMatchObject({ number: 7, url: `https://${host}/o/r/pull/7` }); + expect(fetchFn.mock.calls.length).toBeGreaterThan(0); + for (const [url] of fetchFn.mock.calls) { + expect(String(url).startsWith(`https://${host}/api/v3/`)).toBe(true); + } + }); +}); + describe('check runs across pages', () => { const run = (conclusion: string | null, status = 'completed') => ({ status, conclusion }); const passing = (count: number) => Array.from({ length: count }, () => run('success')); diff --git a/packages/api/src/pulls/github.ts b/packages/api/src/pulls/github.ts index e4f637686a1..354ee7e9efa 100644 --- a/packages/api/src/pulls/github.ts +++ b/packages/api/src/pulls/github.ts @@ -8,7 +8,7 @@ import type { PullRequestSource } from './types'; import { isAllowedRepository } from './repository'; import { PullRequestSourceError } from './types'; -const GITHUB_API_BASE = 'https://api.github.com'; +export const DEFAULT_GITHUB_HOST = 'github.com'; /** Defaults for the operator bounds in `endpoints.agents.pullRequests`. */ const DEFAULT_REQUEST_TIMEOUT_MS = 10_000; const DEFAULT_LOOKUP_TIMEOUT_MS = 30_000; @@ -64,23 +64,33 @@ const isRecord = (value: unknown): value is Record => const isCount = (value: unknown): value is number => typeof value === 'number' && Number.isSafeInteger(value) && value >= 0; -/** The URL ends up in an href, so only a plain https github.com page is accepted. */ -function isGitHubPageUrl(value: unknown): value is string { +/** + * The REST API of a GitHub host: `api.github.com` for github.com, `api.` for a data residency + * tenant on `ghe.com`, and `/api/v3` on the host itself for GitHub Enterprise Server. + */ +export function gitHubApiBase(host: string): string { + if (host === DEFAULT_GITHUB_HOST) return 'https://api.github.com'; + if (host.endsWith('.ghe.com')) return `https://api.${host}`; + return `https://${host}/api/v3`; +} + +/** The URL ends up in an href, so only a plain https page on the configured host is accepted. */ +function isGitHubPageUrl(value: unknown, host: string): value is string { if (typeof value !== 'string') return false; try { const url = new URL(value); - return url.protocol === 'https:' && url.hostname === 'github.com'; + return url.protocol === 'https:' && url.host === host; } catch { return false; } } -function parsePull(value: unknown): GitHubPull { +function parsePull(value: unknown, host: string): GitHubPull { if ( !isRecord(value) || !isCount(value.number) || typeof value.title !== 'string' || - !isGitHubPageUrl(value.html_url) || + !isGitHubPageUrl(value.html_url, host) || (value.state !== 'open' && value.state !== 'closed') || typeof value.merged !== 'boolean' || typeof value.draft !== 'boolean' || @@ -242,15 +252,15 @@ const isPathSegment = (value: string): boolean => value !== '.' && value !== '.. export function createGitHubPullRequestSource({ fetchFn, - apiBase = GITHUB_API_BASE, }: { /** The deployment's HTTP client, so proxying and instrumentation are decided by the caller. */ fetchFn: PullRequestFetch; - apiBase?: string; }): PullRequestSource { /** One lookup's credential and deadlines, shared by every request it makes. */ type Lookup = { token: string; + /** The host's REST API root; every request of the lookup goes there and nowhere else. */ + apiBase: string; requestTimeoutMs: number; /** Aborts when the whole lookup runs out of time. */ deadline: AbortSignal; @@ -270,7 +280,7 @@ export function createGitHubPullRequestSource({ ): Promise { let response: Response; try { - response = await fetchFn(`${apiBase}${pathname}`, { + response = await fetchFn(`${lookup.apiBase}${pathname}`, { headers: { Accept: 'application/vnd.github+json', Authorization: `Bearer ${lookup.token}`, @@ -338,6 +348,7 @@ export function createGitHubPullRequestSource({ branch, head: recordedHead, pullRequest, + host = DEFAULT_GITHUB_HOST, token, limits, allowedRepositories, @@ -350,6 +361,7 @@ export function createGitHubPullRequestSource({ if (recordedHead === null) return null; const lookup: Lookup = { token, + apiBase: gitHubApiBase(host), requestTimeoutMs: limits?.requestTimeoutMs ?? DEFAULT_REQUEST_TIMEOUT_MS, deadline: AbortSignal.timeout(limits?.lookupTimeoutMs ?? DEFAULT_LOOKUP_TIMEOUT_MS), maxCheckRunPages: limits?.maxCheckRunPages ?? DEFAULT_MAX_CHECK_RUN_PAGES, @@ -450,7 +462,7 @@ export function createGitHubPullRequestSource({ if (pullRequest != null && Number.isSafeInteger(pullRequest) && pullRequest > 0) { const linked = await getJson(`${base}/pulls/${pullRequest}`, lookup); if (linked != null) { - const pull = parsePull(linked); + const pull = parsePull(linked, host); const sameRepository = pull.headRepo?.toLowerCase() === repo.toLowerCase(); if ( pull.headRef === branch && @@ -501,7 +513,7 @@ export function createGitHubPullRequestSource({ */ const detail = await getJson(`${base}/pulls/${chosen.number}`, lookup); if (detail == null) return null; - const pull = parsePull(detail); + const pull = parsePull(detail, host); /** A push between the list and the detail can move the head off the recorded commit. */ if (pull.headSha !== chosen.sha && !(await contains(pull.headSha))) return null; return report(pull); diff --git a/packages/api/src/pulls/handler.spec.ts b/packages/api/src/pulls/handler.spec.ts index 0ed446e2741..54fe9a42776 100644 --- a/packages/api/src/pulls/handler.spec.ts +++ b/packages/api/src/pulls/handler.spec.ts @@ -90,6 +90,7 @@ describe('createConversationPullRequestHandler', () => { branch: 'feat/x', token: 'ghp_secret', head: null, + host: 'github.com', ttlMs: 30_000, cacheMaxEntries: 500, cacheMaxCredentials: 256, @@ -107,6 +108,51 @@ describe('createConversationPullRequestHandler', () => { expect(res.json).toHaveBeenCalledWith({ pullRequest: pr }); }); + it('looks up on the configured GitHub host, normalized', async () => { + const { run, lookup } = setup({ settings: { ...enabled, host: ' GHE.Example.com ' } }); + await run(); + expect(lookup).toHaveBeenCalledWith(expect.objectContaining({ host: 'ghe.example.com' })); + }); + + /** The operator's YAML holds `base`; a role, group or user override in the database holds `override`. */ + async function hostFor(base: Record, override: Record) { + const lookup = jest.fn().mockResolvedValue({ ok: true, value: pr }); + const handler = createConversationPullRequestHandler({ + getConvoLaneGit: jest.fn().mockResolvedValue({ branch: 'feat/x', head: null, repo: 'o/r' }), + getAppConfig: jest + .fn() + .mockImplementation(async (options: { baseOnly?: boolean }) => + configWith(options.baseOnly ? base : override), + ), + lookup, + env: { GH_TOKEN: 'ghp_secret' }, + }); + const res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; + await handler( + { user: { id: 'u1' }, params: { conversationId: 'c1' } } as unknown as ServerRequest, + res as unknown as Response, + ); + return lookup.mock.calls[0]?.[0]?.host; + } + + it('takes the host from the operator config, never from a database override', async () => { + const override = { ...enabled, host: 'attacker.example' }; + expect(await hostFor({ ...enabled, host: 'ghe.example.com' }, override)).toBe( + 'ghe.example.com', + ); + expect(await hostFor(enabled, override)).toBe('github.com'); + }); + + it('never sends the token anywhere when the host is not a host name', async () => { + const { run, res, lookup } = setup({ + settings: { ...enabled, host: 'https://ghe.example.com' }, + }); + await run(); + expect(lookup).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(200); + expect(res.json).toHaveBeenCalledWith({ pullRequest: null }); + }); + it('passes the pull request the conversation linked to', async () => { const { run, lookup } = setup({ laneGit: { branch: 'feat/x', head: null, repo: 'o/r', pullRequest: 7 }, @@ -280,12 +326,13 @@ describe('createConversationPullRequestHandler', () => { it('starts the config and the owner-scoped lane reads together, not one after the other', async () => { const order: string[] = []; - let releaseConfig: (config: unknown) => void = () => undefined; + const pending: Array<(config: unknown) => void> = []; + const releaseConfig = (config: unknown) => pending.forEach((resolve) => resolve(config)); const getAppConfig = jest.fn( () => new Promise((resolve) => { order.push('config:start'); - releaseConfig = resolve; + pending.push(resolve); }), ); const getConvoLaneGit = jest.fn(async () => { @@ -304,7 +351,8 @@ describe('createConversationPullRequestHandler', () => { res as unknown as Response, ); await new Promise((resolve) => setImmediate(resolve)); - expect(order).toEqual(['config:start', 'lane:start']); + /** The resolved config and the operator's base config, then the lane, all before any settles. */ + expect(order).toEqual(['config:start', 'config:start', 'lane:start']); releaseConfig(configWith(enabled)); await done; expect(res.json).toHaveBeenCalledWith({ pullRequest: pr }); @@ -417,6 +465,26 @@ describe('createConversationPullRequestsHandler', () => { return { run, res, lookup, getConvosLaneGit }; } + it('takes the host from the operator config for the batch too', async () => { + const lookup = jest.fn().mockResolvedValue({ ok: true, value: pr }); + const handler = createConversationPullRequestsHandler({ + getConvosLaneGit: jest.fn().mockResolvedValue([lane('a')]), + getAppConfig: jest + .fn() + .mockImplementation(async (options: { baseOnly?: boolean }) => + configWith(options.baseOnly ? enabled : { ...enabled, host: 'attacker.example' }), + ), + lookup, + env: { GH_TOKEN: 'ghp_secret' }, + }); + const res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; + await handler( + { user: { id: 'u1' }, body: { conversationIds: ['a'] } } as unknown as ServerRequest, + res as unknown as Response, + ); + expect(lookup).toHaveBeenCalledWith(expect.objectContaining({ host: 'github.com' })); + }); + it('keeps conversations on one lane apart when they linked different pull requests', async () => { const linked = (conversationId: string, pullRequest: number) => ({ conversationId, diff --git a/packages/api/src/pulls/handler.ts b/packages/api/src/pulls/handler.ts index 8a62ae4f56c..a1e3e43d363 100644 --- a/packages/api/src/pulls/handler.ts +++ b/packages/api/src/pulls/handler.ts @@ -14,7 +14,11 @@ import type { GetAppConfigOptions } from '~/app/service'; import type { PullRequestSettings } from './settings'; import type { PullRequestLookup } from './types'; import type { ServerRequest } from '~/types'; -import { isPullRequestFeatureActive, resolvePullRequestToken } from './settings'; +import { + resolvePullRequestHost, + resolvePullRequestToken, + isPullRequestFeatureActive, +} from './settings'; import { ALL_REPOSITORIES, isAllowedRepository } from './repository'; import { getAppConfigOptionsFromUser } from '~/app/service'; import { getSafeErrorMetadata } from '~/utils'; @@ -177,6 +181,32 @@ function eligibleLane( }; } +type ServerUser = ServerRequest['user']; + +const agentsPullRequests = (config: AppConfig): PullRequestSettings | undefined => + (config.endpoints?.[EModelEndpoint.agents] as TAgentsEndpoint | undefined)?.pullRequests; + +/** + * The user's settings, with `host` taken from the operator's YAML alone. Role, group and user + * overrides live in the database and may be edited by delegated administrators; letting one set + * the host would let it send the deployment's token to a server of its choosing. + */ +async function readSettings( + getAppConfig: (options: GetAppConfigOptions) => Promise, + user: ServerUser, +): Promise { + const options = { ...getAppConfigOptionsFromUser(user), skipRuntimeAugmentation: true }; + const [resolved, base] = await Promise.all([ + getAppConfig({ ...options, failClosed: true }), + getAppConfig({ ...options, baseOnly: true }), + ]); + const settings = agentsPullRequests(resolved); + if (settings == null) return undefined; + const { host: _override, ...rest } = settings; + const host = agentsPullRequests(base)?.host; + return host != null ? { ...rest, host } : rest; +} + /** Everything one lookup needs from the settings, so the single and batch routes cannot differ. */ function lookupInput(settings: PullRequestSettings, token: string, lane: EligibleLane) { return { @@ -184,6 +214,7 @@ function lookupInput(settings: PullRequestSettings, token: string, lane: Eligibl branch: lane.branch, head: lane.head, ...(lane.pullRequest != null ? { pullRequest: lane.pullRequest } : {}), + host: resolvePullRequestHost(settings.host) ?? undefined, token, ttlMs: (settings.cacheTtlSeconds ?? 30) * 1000, cacheMaxEntries: settings.cacheMaxEntries ?? 500, @@ -253,16 +284,10 @@ export function createConversationPullRequestHandler(deps: { * used when the resolved config turns the feature on, which keeps a disabled deployment's * answer the same as before. */ - const [appConfig, laneGit] = await Promise.all([ - deps.getAppConfig({ - ...getAppConfigOptionsFromUser(req.user), - skipRuntimeAugmentation: true, - failClosed: true, - }), + const [settings, laneGit] = await Promise.all([ + readSettings(deps.getAppConfig, req.user), deps.getConvoLaneGit(userId, conversationId), ]); - const settings = (appConfig.endpoints?.[EModelEndpoint.agents] as TAgentsEndpoint | undefined) - ?.pullRequests; if (!settings || !isPullRequestFeatureActive(settings, deps.env)) { res.status(200).json(NONE); return; @@ -334,11 +359,7 @@ export function createConversationPullRequestsHandler(deps: { * overlapped. */ const startedAt = Date.now(); - const configRead = deps.getAppConfig({ - ...getAppConfigOptionsFromUser(req.user), - skipRuntimeAugmentation: true, - failClosed: true, - }); + const configRead = readSettings(deps.getAppConfig, req.user); const laneRead = deps.getConvosLaneGit(userId, ids); /** A lane read nobody waits for (feature off, config timed out) must not fail unhandled. */ laneRead.catch(() => undefined); @@ -351,9 +372,7 @@ export function createConversationPullRequestsHandler(deps: { unavailable(); return; } - const appConfig = loaded; - const settings = (appConfig.endpoints?.[EModelEndpoint.agents] as TAgentsEndpoint | undefined) - ?.pullRequests; + const settings = loaded; const deadline = startedAt + (settings?.batchTimeoutSeconds ?? DEFAULT_BATCH_TIMEOUT_SECONDS) * 1000; const none = (): TConversationPullRequestsResponse => ({ diff --git a/packages/api/src/pulls/links.spec.ts b/packages/api/src/pulls/links.spec.ts index e2a5eb96b24..550137bf58a 100644 --- a/packages/api/src/pulls/links.spec.ts +++ b/packages/api/src/pulls/links.spec.ts @@ -16,6 +16,11 @@ describe('findPullRequestLink', () => { expect(findPullRequestLink(output, 'o/r')).toBe(8); }); + it('finds a link on a GitHub Enterprise host, which the lookup then checks', () => { + expect(findPullRequestLink('https://ghe.example.com/o/r/pull/9', 'o/r')).toBe(9); + expect(findPullRequestLink('https://ghe.local:8443/o/r/pull/9', 'o/r')).toBe(9); + }); + it('accepts a link to a page of the pull request', () => { expect(findPullRequestLink('https://github.com/o/r/pull/12/files', 'o/r')).toBe(12); }); @@ -29,7 +34,6 @@ describe('findPullRequestLink', () => { ['a repository that only starts the same', 'https://github.com/o/r-fork/pull/4'], ['an issue', 'https://github.com/o/r/issues/4'], ['the page git push suggests', 'https://github.com/o/r/pull/new/feat/x'], - ['another host', 'https://example.com/o/r/pull/4'], ['plain http', 'http://github.com/o/r/pull/4'], ['a number glued to more text', 'https://github.com/o/r/pull/4abc'], ['pull request zero', 'https://github.com/o/r/pull/0'], diff --git a/packages/api/src/pulls/links.ts b/packages/api/src/pulls/links.ts index 99f5c4ee55a..911a42fd2b9 100644 --- a/packages/api/src/pulls/links.ts +++ b/packages/api/src/pulls/links.ts @@ -1,9 +1,11 @@ /** - * `https://github.com/{owner}/{name}/pull/{number}`, ending where the number does. A `/pull/new/` - * page (what `git push` suggests) has no number and never matches. + * `https://{host}/{owner}/{name}/pull/{number}`, ending where the number does. A `/pull/new/` page + * (what `git push` suggests) has no number and never matches. Any host matches, so a GitHub + * Enterprise link is found too: the number is only a candidate, which the lookup reads from the + * configured host and accepts only when its head is the lane's branch. */ const PULL_REQUEST_URL = - /https:\/\/github\.com\/([A-Za-z0-9_.-]+)\/([A-Za-z0-9_.-]+)\/pull\/(\d+)(?![\w-])/g; + /https:\/\/[A-Za-z0-9.-]+(?::\d{1,5})?\/([A-Za-z0-9_.-]+)\/([A-Za-z0-9_.-]+)\/pull\/(\d+)(?![\w-])/g; /** * The pull request of `repo` that command output links to, the last one when it links several, diff --git a/packages/api/src/pulls/lookup.spec.ts b/packages/api/src/pulls/lookup.spec.ts index eb36f1f991a..8fc89025380 100644 --- a/packages/api/src/pulls/lookup.spec.ts +++ b/packages/api/src/pulls/lookup.spec.ts @@ -66,6 +66,16 @@ describe('createPullRequestLookup', () => { expect(find).toHaveBeenCalledTimes(3); }); + it('keys by host, so one GitHub never answers for another', async () => { + const find = jest.fn().mockResolvedValue(value); + const lookup = createPullRequestLookup({ source: { find } }); + await lookup(input); + await lookup({ ...input, host: 'ghe.example.com' }); + await lookup({ ...input, host: 'ghe.example.com' }); + expect(find).toHaveBeenCalledTimes(2); + expect(find).toHaveBeenLastCalledWith(expect.objectContaining({ host: 'ghe.example.com' })); + }); + it('keys by the linked pull request and hands it to the source', async () => { const find = jest.fn().mockResolvedValue(value); const lookup = createPullRequestLookup({ source: { find } }); diff --git a/packages/api/src/pulls/lookup.ts b/packages/api/src/pulls/lookup.ts index 3aa16e5efff..bde8af52755 100644 --- a/packages/api/src/pulls/lookup.ts +++ b/packages/api/src/pulls/lookup.ts @@ -111,6 +111,7 @@ export function createPullRequestLookup({ branch, head, pullRequest, + host, token, ttlMs, limits, @@ -134,7 +135,7 @@ export function createPullRequestLookup({ .sort() .join('|'), ].join(','); - const key = `${scope}\0${repo}#${branch}\0${head ?? ''}\0${pullRequest ?? ''}\0${policy}`; + const key = `${scope}\0${host ?? ''}\0${repo}#${branch}\0${head ?? ''}\0${pullRequest ?? ''}\0${policy}`; const cached = touch(scope)?.get(key); if (cached != null && cached.expiresAt > now()) return cached.result; const pending = inflight.get(key); @@ -155,6 +156,7 @@ export function createPullRequestLookup({ branch, head, pullRequest, + ...(host != null ? { host } : {}), token, limits, allowedRepositories, diff --git a/packages/api/src/pulls/settings.spec.ts b/packages/api/src/pulls/settings.spec.ts index fb2aa65ecd8..35dceb63d79 100644 --- a/packages/api/src/pulls/settings.spec.ts +++ b/packages/api/src/pulls/settings.spec.ts @@ -1,9 +1,28 @@ import { - isPullRequestFeatureActive, - resolvePullRequestToken, resolveTokenReference, + resolvePullRequestHost, + resolvePullRequestToken, + isPullRequestFeatureActive, } from './settings'; +describe('resolvePullRequestHost', () => { + it('is github.com when no host is configured', () => { + expect(resolvePullRequestHost(undefined)).toBe('github.com'); + }); + + it('normalizes a configured host, which the raw YAML does not', () => { + expect(resolvePullRequestHost(' GitHub.Example.com ')).toBe('github.example.com'); + expect(resolvePullRequestHost('ghe.local:8443')).toBe('ghe.local:8443'); + }); + + it.each(['https://github.example.com', 'github.example.com/api', 'u@github.example.com', ''])( + 'refuses %p rather than falling back to github.com', + (host) => { + expect(resolvePullRequestHost(host)).toBeNull(); + }, + ); +}); + describe('resolvePullRequestToken', () => { it('prefers the configured reference over any fallback', () => { expect( @@ -79,6 +98,18 @@ describe('isPullRequestFeatureActive', () => { ); }); + it('stays dormant with a host that is not a host name, so the token goes nowhere', () => { + expect( + isPullRequestFeatureActive( + { host: 'https://ghe.example.com', allowAllRepositories: true }, + env, + ), + ).toBe(false); + expect( + isPullRequestFeatureActive({ host: 'ghe.example.com', allowAllRepositories: true }, env), + ).toBe(true); + }); + it('is on when explicitly enabled and fully scoped', () => { expect(isPullRequestFeatureActive({ enabled: true, allowAllRepositories: true }, env)).toBe( true, diff --git a/packages/api/src/pulls/settings.ts b/packages/api/src/pulls/settings.ts index f5f0152be68..ab7f3bc8f35 100644 --- a/packages/api/src/pulls/settings.ts +++ b/packages/api/src/pulls/settings.ts @@ -20,6 +20,18 @@ export function resolveTokenReference(reference: string | undefined, env: Env): return env[name]?.trim() || null; } +const HOST = /^(?=[a-z0-9])[a-z0-9.-]*[a-z0-9](?::[0-9]{1,5})?$/; + +/** + * The configured GitHub host, lowercased, or `github.com` when none is set. `null` for a value that + * is not a plain host name: the token is then sent nowhere rather than to a host nobody named. + */ +export function resolvePullRequestHost(host: string | undefined): string | null { + if (host == null) return 'github.com'; + const normalized = host.trim().toLowerCase(); + return normalized.length <= 253 && HOST.test(normalized) ? normalized : null; +} + /** The configured token reference, else the first fallback variable that is set. */ export function resolvePullRequestToken(reference: string | undefined, env: Env): string | null { if (reference != null) return resolveTokenReference(reference, env); @@ -41,6 +53,7 @@ export function isPullRequestFeatureActive( env: Env, ): boolean { if (settings?.enabled === false) return false; + if (resolvePullRequestHost(settings?.host) == null) return false; if (settings?.token == null && resolvePullRequestToken(undefined, env) == null) return false; return ( settings?.allowAllRepositories === true || (settings?.allowedRepositories?.length ?? 0) > 0 diff --git a/packages/api/src/pulls/types.ts b/packages/api/src/pulls/types.ts index b864c282104..8d143e036f6 100644 --- a/packages/api/src/pulls/types.ts +++ b/packages/api/src/pulls/types.ts @@ -49,6 +49,8 @@ export type PullRequestFindInput = { * branch is searched as if no link had been seen. */ pullRequest?: number | null; + /** The GitHub host the token belongs to, `github.com` when omitted. Already normalized. */ + host?: string; token: string; limits?: PullRequestLookupLimits; /** The repositories the token may be used for. A pull request from a fork has its checks read diff --git a/packages/data-provider/src/config.spec.ts b/packages/data-provider/src/config.spec.ts index 161e92b83cb..4e15b18d6f3 100644 --- a/packages/data-provider/src/config.spec.ts +++ b/packages/data-provider/src/config.spec.ts @@ -1189,6 +1189,27 @@ describe('agent pull request config', () => { }, ); + it.each([ + 'github.com', + 'github.example.com', + 'acme.ghe.com', + 'GHE.Example.com', + 'ghe.local:8443', + ])('accepts %s as the GitHub host', (host) => { + expect(parse({ ...enabled, host }).success).toBe(true); + }); + + it.each([ + 'https://github.example.com', + 'github.example.com/api/v3', + 'user@github.example.com', + '-github.com', + 'github.com.', + '', + ])('rejects %p as the GitHub host', (host) => { + expect(parse({ ...enabled, host }).success).toBe(false); + }); + it.each(['ghp_abcdef', '${bad name}', '$GITHUB_TOKEN', '${}'])( 'rejects %s as a token because only a reference is allowed', (token) => { diff --git a/packages/data-provider/src/config.ts b/packages/data-provider/src/config.ts index 0ccac0e2567..b13045fdace 100644 --- a/packages/data-provider/src/config.ts +++ b/packages/data-provider/src/config.ts @@ -513,6 +513,16 @@ const pullRequestRepositorySchema = z message: 'must not contain dot segments', }); +/** A GitHub host name, optionally with a port: `github.com`, an Enterprise Server host such as + * `github.example.com`, or a data residency tenant such as `acme.ghe.com`. Never a URL. */ +const pullRequestHostSchema = z + .string() + .trim() + .max(253) + .regex(/^(?=[a-z0-9])[a-z0-9.-]*[a-z0-9](?::[0-9]{1,5})?$/i, { + message: 'must be a host name like github.example.com, without a scheme or path', + }); + const pullRequestTokenReferenceSchema = z .string() .trim() @@ -1879,6 +1889,9 @@ export const agentsEndpointSchema = baseEndpointSchema pullRequests: z .object({ enabled: z.boolean().optional(), + /** The GitHub the token belongs to: `github.com` (the default), a GitHub Enterprise Server + * host, or a GitHub Enterprise Cloud data residency host ending in `.ghe.com`. */ + host: pullRequestHostSchema.optional(), /** Environment variable reference holding a read-only GitHub token, e.g. * `${GITHUB_PULL_REQUEST_TOKEN}`. Never the token itself. Optional: without it the first * of `GITHUB_PULL_REQUEST_TOKEN`, `GITHUB_TOKEN` and `GH_TOKEN` that is set is used. */ From efc238db77953d8a55f5ebdce1dc0fe98dc0f937 Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Sun, 11 Oct 2026 19:29:44 +0200 Subject: [PATCH 2/2] fix: Normalize and Range-Check the Pull Request Host Port --- packages/api/src/pulls/settings.spec.ts | 23 +++++++++++++++++------ packages/api/src/pulls/settings.ts | 10 ++++++++-- packages/data-provider/src/config.spec.ts | 2 ++ packages/data-provider/src/config.ts | 9 ++++++++- 4 files changed, 35 insertions(+), 9 deletions(-) diff --git a/packages/api/src/pulls/settings.spec.ts b/packages/api/src/pulls/settings.spec.ts index 35dceb63d79..b17e5563a23 100644 --- a/packages/api/src/pulls/settings.spec.ts +++ b/packages/api/src/pulls/settings.spec.ts @@ -13,14 +13,25 @@ describe('resolvePullRequestHost', () => { it('normalizes a configured host, which the raw YAML does not', () => { expect(resolvePullRequestHost(' GitHub.Example.com ')).toBe('github.example.com'); expect(resolvePullRequestHost('ghe.local:8443')).toBe('ghe.local:8443'); + expect(resolvePullRequestHost('ghe.local:08443')).toBe('ghe.local:8443'); }); - it.each(['https://github.example.com', 'github.example.com/api', 'u@github.example.com', ''])( - 'refuses %p rather than falling back to github.com', - (host) => { - expect(resolvePullRequestHost(host)).toBeNull(); - }, - ); + it('drops the https default port, which a page URL never carries', () => { + expect(resolvePullRequestHost('github.com:443')).toBe('github.com'); + expect(resolvePullRequestHost('GHE.Example.com:443')).toBe('ghe.example.com'); + }); + + it.each([ + 'https://github.example.com', + 'github.example.com/api', + 'u@github.example.com', + 'ghe.local:0', + 'ghe.local:65536', + 'ghe.local:99999', + '', + ])('refuses %p rather than falling back to github.com', (host) => { + expect(resolvePullRequestHost(host)).toBeNull(); + }); }); describe('resolvePullRequestToken', () => { diff --git a/packages/api/src/pulls/settings.ts b/packages/api/src/pulls/settings.ts index ab7f3bc8f35..dd0993353c5 100644 --- a/packages/api/src/pulls/settings.ts +++ b/packages/api/src/pulls/settings.ts @@ -28,8 +28,14 @@ const HOST = /^(?=[a-z0-9])[a-z0-9.-]*[a-z0-9](?::[0-9]{1,5})?$/; */ export function resolvePullRequestHost(host: string | undefined): string | null { if (host == null) return 'github.com'; - const normalized = host.trim().toLowerCase(); - return normalized.length <= 253 && HOST.test(normalized) ? normalized : null; + const value = host.trim().toLowerCase(); + if (value.length > 253 || !HOST.test(value)) return null; + const [name, port] = value.split(':'); + if (port == null) return value; + const number = Number(port); + if (number < 1 || number > 65535) return null; + /** A URL drops the https default port, so a page on `host:443` compares as `host`. */ + return number === 443 ? name : `${name}:${number}`; } /** The configured token reference, else the first fallback variable that is set. */ diff --git a/packages/data-provider/src/config.spec.ts b/packages/data-provider/src/config.spec.ts index 4e15b18d6f3..73321f6d64a 100644 --- a/packages/data-provider/src/config.spec.ts +++ b/packages/data-provider/src/config.spec.ts @@ -1205,6 +1205,8 @@ describe('agent pull request config', () => { 'user@github.example.com', '-github.com', 'github.com.', + 'ghe.local:0', + 'ghe.local:65536', '', ])('rejects %p as the GitHub host', (host) => { expect(parse({ ...enabled, host }).success).toBe(false); diff --git a/packages/data-provider/src/config.ts b/packages/data-provider/src/config.ts index b13045fdace..c09c8895958 100644 --- a/packages/data-provider/src/config.ts +++ b/packages/data-provider/src/config.ts @@ -521,7 +521,14 @@ const pullRequestHostSchema = z .max(253) .regex(/^(?=[a-z0-9])[a-z0-9.-]*[a-z0-9](?::[0-9]{1,5})?$/i, { message: 'must be a host name like github.example.com, without a scheme or path', - }); + }) + .refine( + (value) => { + const port = value.split(':')[1]; + return port == null || (Number(port) >= 1 && Number(port) <= 65535); + }, + { message: 'must use a port from 1 to 65535' }, + ); const pullRequestTokenReferenceSchema = z .string()