From 90dce8bfcc84fbecdebc6a36cad4e53bb4158e88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Phorcys=20=F0=9F=90=BE?= <57866459+phorcys420@users.noreply.github.com> Date: Mon, 24 Aug 2026 22:44:09 +0000 Subject: [PATCH] fix: anchor github-url host to GITHUB_SERVER_URL for GHES support The github-url host was hardcoded to github.com, so every issue/PR URL on GitHub Enterprise Server failed validation and the action exited before commenting. Anchor the host to the runner-provided GITHUB_SERVER_URL instead. The anchor stays runner-controlled, not user-controlled, so a workflow that templates user input into github-url still cannot redirect the action to an attacker-chosen host. - parseGithubItemURL/deriveCommentKey take an optional serverURL (default https://github.com); env is read at the action.ts edge. - Build the matcher with RegExp.escape so host metacharacters stay literal, no hand-rolled escaping. - Error message reflects the resolved server URL. Fixes #39 --- dist/index.js | 45 +++++++++++++++------ src/action.test.ts | 56 +++++++++++++++++++++++++- src/action.ts | 31 ++++++++++++--- src/comment.test.ts | 97 +++++++++++++++++++++++++++++++++++++++++++++ src/comment.ts | 51 ++++++++++++++++-------- 5 files changed, 243 insertions(+), 37 deletions(-) diff --git a/dist/index.js b/dist/index.js index 7240d0a..36964a9 100644 --- a/dist/index.js +++ b/dist/index.js @@ -36791,20 +36791,27 @@ function sanitizeLabelToken(input) { } // src/comment.ts -var GITHUB_URL_REGEX = /^https:\/\/github\.com\/([^/]+)\/([^/]+)\/(?:issues|pull)\/(\d+)\/?(?:[?#].*)?$/; -function parseGithubItemURL(input) { +var DEFAULT_GITHUB_SERVER_URL = "https://github.com"; +var githubURLRegexCache = new Map; +function githubURLRegex(serverURL) { + let regex = githubURLRegexCache.get(serverURL); + if (!regex) { + const base = RegExp.escape(normalizeBaseUrl(serverURL)); + regex = new RegExp(`^${base}/([^/]+)/([^/]+)/(?:issues|pull)/(\\d+)/?(?:[?#].*)?$`); + githubURLRegexCache.set(serverURL, regex); + } + return regex; +} +function parseGithubItemURL(input, serverURL = DEFAULT_GITHUB_SERVER_URL) { if (!input) { return; } - const match = input.match(GITHUB_URL_REGEX); + const match = githubURLRegex(serverURL).exec(input); if (!match) { return; } - return { - owner: match[1], - repo: match[2], - number: Number.parseInt(match[3], 10) - }; + const [, owner, repo, number4] = match; + return { owner, repo, number: Number.parseInt(number4, 10) }; } var COMMENT_MARKER_PREFIX = ""; @@ -36815,7 +36822,7 @@ function deriveCommentKey(inputs) { if (inputs.idempotencyKey) { return sanitizeLabelToken(inputs.idempotencyKey); } - const parsed = parseGithubItemURL(inputs.githubURL); + const parsed = parseGithubItemURL(inputs.githubURL, inputs.serverURL); let base; if (!parsed) { base = inputs.githubURL; @@ -37097,9 +37104,10 @@ class CoderAgentChatAction { if (!this.inputs.githubURL) { throw new Error("Missing GitHub URL"); } - const parsed = parseGithubItemURL(this.inputs.githubURL); + const serverURL = this.githubServerURL(); + const parsed = parseGithubItemURL(this.inputs.githubURL, serverURL); if (!parsed) { - throw new Error(`Invalid \`github-url\` input "${this.inputs.githubURL}". ` + "Expected `https://github.com///issues/` or " + "`https://github.com///pull/`. The action " + "rejects non-github.com hosts so a workflow that templates " + "user-controlled content into this input cannot redirect the " + "action to an attacker-chosen repository."); + throw new Error(`Invalid \`github-url\` input "${this.inputs.githubURL}". ` + `Expected \`${serverURL}///issues/\` or ` + `\`${serverURL}///pull/\`. The action rejects ` + "hosts other than the current GitHub server " + "(`GITHUB_SERVER_URL`) so a workflow that templates " + "user-controlled content into this input cannot redirect the " + "action to an attacker-chosen repository."); } return { githubOrg: parsed.owner, @@ -37107,12 +37115,19 @@ class CoderAgentChatAction { githubIssueNumber: parsed.number }; } + githubServerURL() { + return process.env.GITHUB_SERVER_URL || DEFAULT_GITHUB_SERVER_URL; + } generateChatUrl(chatId) { return `${normalizeBaseUrl(this.inputs.coderURL)}/agents/${chatId}`; } async commentOnIssue(args) { const workflow = process.env.GITHUB_WORKFLOW || undefined; - const marker = buildCommentMarker(deriveCommentKey({ ...this.inputs, workflow })); + const marker = buildCommentMarker(deriveCommentKey({ + ...this.inputs, + workflow, + serverURL: this.githubServerURL() + })); const diff = args.chat?.diff_status; const hasPR = diff?.pr_number != null; const body = buildSuccessCommentBody({ @@ -37283,7 +37298,11 @@ class CoderAgentChatAction { return failure; } const workflow = process.env.GITHUB_WORKFLOW || undefined; - const marker = buildCommentMarker(deriveCommentKey({ ...this.inputs, workflow })); + const marker = buildCommentMarker(deriveCommentKey({ + ...this.inputs, + workflow, + serverURL: this.githubServerURL() + })); const body = buildFailureCommentBody(detail, { agentsUrl: buildDeploymentAgentsUrl(this.inputs.coderURL), marker, diff --git a/src/action.test.ts b/src/action.test.ts index c625910..3635bac 100644 --- a/src/action.test.ts +++ b/src/action.test.ts @@ -177,7 +177,8 @@ describe("CoderAgentChatAction", () => { // `https://attacker.example/coder/coder/issues/1` would have // called `octokit.rest.issues.createComment` with owner=coder, // repo=coder, number=1 under the workflow's `github-token`. The - // action now refuses. + // host is now anchored to `GITHUB_SERVER_URL` (default + // `https://github.com`), so a different host is refused. const inputs = createMockInputs({ githubURL: "https://code.acme.com/owner/repo/issues/123", }); @@ -188,7 +189,7 @@ describe("CoderAgentChatAction", () => { ); expect(() => action.parseGithubURL()).toThrowError( - /non-github.com hosts/, + /current GitHub server/, ); }); @@ -206,6 +207,57 @@ describe("CoderAgentChatAction", () => { /Invalid `github-url` input/, ); }); + test("accepts a GHES URL when GITHUB_SERVER_URL points at that host", () => { + const previous = process.env.GITHUB_SERVER_URL; + process.env.GITHUB_SERVER_URL = "https://github.example.com"; + try { + const inputs = createMockInputs({ + githubURL: "https://github.example.com/owner/repo/pull/7", + }); + const action = new CoderAgentChatAction( + coderClient, + octokit as unknown as Octokit, + inputs, + ); + + expect(action.parseGithubURL()).toEqual({ + githubOrg: "owner", + githubRepo: "repo", + githubIssueNumber: 7, + }); + } finally { + if (previous === undefined) { + delete process.env.GITHUB_SERVER_URL; + } else { + process.env.GITHUB_SERVER_URL = previous; + } + } + }); + + test("rejects a dotcom URL when GITHUB_SERVER_URL points at a GHES host", () => { + const previous = process.env.GITHUB_SERVER_URL; + process.env.GITHUB_SERVER_URL = "https://github.example.com"; + try { + const inputs = createMockInputs({ + githubURL: "https://github.com/owner/repo/issues/1", + }); + const action = new CoderAgentChatAction( + coderClient, + octokit as unknown as Octokit, + inputs, + ); + + expect(() => action.parseGithubURL()).toThrowError( + /current GitHub server/, + ); + } finally { + if (previous === undefined) { + delete process.env.GITHUB_SERVER_URL; + } else { + process.env.GITHUB_SERVER_URL = previous; + } + } + }); }); describe("generateChatUrl", () => { diff --git a/src/action.ts b/src/action.ts index 22af193..810577c 100644 --- a/src/action.ts +++ b/src/action.ts @@ -16,6 +16,7 @@ import { buildFailureCommentBody, buildSuccessCommentBody, classifyError, + DEFAULT_GITHUB_SERVER_URL, deriveCommentKey, type FailureDetail, normalizeBaseUrl, @@ -112,13 +113,15 @@ export class CoderAgentChatAction { throw new Error("Missing GitHub URL"); } - const parsed = parseGithubItemURL(this.inputs.githubURL); + const serverURL = this.githubServerURL(); + const parsed = parseGithubItemURL(this.inputs.githubURL, serverURL); if (!parsed) { throw new Error( `Invalid \`github-url\` input "${this.inputs.githubURL}". ` + - "Expected `https://github.com///issues/` or " + - "`https://github.com///pull/`. The action " + - "rejects non-github.com hosts so a workflow that templates " + + `Expected \`${serverURL}///issues/\` or ` + + `\`${serverURL}///pull/\`. The action rejects ` + + "hosts other than the current GitHub server " + + "(`GITHUB_SERVER_URL`) so a workflow that templates " + "user-controlled content into this input cannot redirect the " + "action to an attacker-chosen repository.", ); @@ -130,6 +133,14 @@ export class CoderAgentChatAction { }; } + // The GitHub server the action runs against. The Actions runner sets + // `GITHUB_SERVER_URL` (e.g. `https://github.com` on dotcom, + // `https://github.example.com` on GHES). Falls back to dotcom outside a + // runner. Read at this edge so `comment.ts` helpers stay pure. + private githubServerURL(): string { + return process.env.GITHUB_SERVER_URL || DEFAULT_GITHUB_SERVER_URL; + } + /** * Generate chat URL. */ @@ -158,7 +169,11 @@ export class CoderAgentChatAction { // stays pure and tests stay deterministic. const workflow = process.env.GITHUB_WORKFLOW || undefined; const marker = buildCommentMarker( - deriveCommentKey({ ...this.inputs, workflow }), + deriveCommentKey({ + ...this.inputs, + workflow, + serverURL: this.githubServerURL(), + }), ); const diff = args.chat?.diff_status; const hasPR = diff?.pr_number != null; @@ -514,7 +529,11 @@ export class CoderAgentChatAction { // stays pure and tests stay deterministic. const workflow = process.env.GITHUB_WORKFLOW || undefined; const marker = buildCommentMarker( - deriveCommentKey({ ...this.inputs, workflow }), + deriveCommentKey({ + ...this.inputs, + workflow, + serverURL: this.githubServerURL(), + }), ); const body = buildFailureCommentBody(detail, { agentsUrl: buildDeploymentAgentsUrl(this.inputs.coderURL), diff --git a/src/comment.test.ts b/src/comment.test.ts index 4f80fd3..3c5ea8d 100644 --- a/src/comment.test.ts +++ b/src/comment.test.ts @@ -12,6 +12,7 @@ import { type FailureDetail, findCommentByPredicate, normalizeBaseUrl, + parseGithubItemURL, renderDetailBlock, type SuccessCommentContext, } from "./comment"; @@ -39,6 +40,93 @@ describe("buildCommentMarker", () => { }); }); +describe("parseGithubItemURL", () => { + test("parses a dotcom issue URL under the default server", () => { + expect( + parseGithubItemURL("https://github.com/owner/repo/issues/123"), + ).toEqual({ owner: "owner", repo: "repo", number: 123 }); + }); + + test("parses a dotcom PR URL under the default server", () => { + expect(parseGithubItemURL("https://github.com/owner/repo/pull/42")).toEqual( + { owner: "owner", repo: "repo", number: 42 }, + ); + }); + + test("tolerates a trailing slash, query string, and fragment", () => { + expect( + parseGithubItemURL("https://github.com/owner/repo/pull/42/?tab=files"), + ).toEqual({ owner: "owner", repo: "repo", number: 42 }); + expect( + parseGithubItemURL( + "https://github.com/owner/repo/issues/7#issuecomment-1", + ), + ).toEqual({ owner: "owner", repo: "repo", number: 7 }); + }); + + test("rejects extra path segments", () => { + expect( + parseGithubItemURL("https://github.com/owner/repo/issues/123/files"), + ).toBeUndefined(); + }); + + test("parses a GHES URL when serverURL matches (host anchoring)", () => { + expect( + parseGithubItemURL( + "https://github.example.com/owner/repo/pull/1", + "https://github.example.com", + ), + ).toEqual({ owner: "owner", repo: "repo", number: 1 }); + }); + + test("tolerates a trailing slash on serverURL", () => { + expect( + parseGithubItemURL( + "https://github.example.com/owner/repo/pull/1", + "https://github.example.com/", + ), + ).toEqual({ owner: "owner", repo: "repo", number: 1 }); + }); + + test("rejects a host that does not match serverURL (security case)", () => { + // A URL on a different host than the runner's GITHUB_SERVER_URL must + // not parse, so user-controlled github-url cannot redirect the action + // to an attacker-chosen host. + expect( + parseGithubItemURL( + "https://attacker.example/owner/repo/issues/1", + "https://github.example.com", + ), + ).toBeUndefined(); + }); + + test("rejects a dotcom URL when serverURL is a GHES host", () => { + expect( + parseGithubItemURL( + "https://github.com/owner/repo/issues/1", + "https://github.example.com", + ), + ).toBeUndefined(); + }); + + test("does not treat the server host as a regex pattern", () => { + // The dots in the host are metacharacters; RegExp.escape must keep + // them literal so a host that merely matches the pattern (dot as + // wildcard) is still rejected. + expect( + parseGithubItemURL( + "https://githubXexample.com/owner/repo/issues/1", + "https://github.example.com", + ), + ).toBeUndefined(); + }); + + test("returns undefined for empty input", () => { + expect(parseGithubItemURL(undefined)).toBeUndefined(); + expect(parseGithubItemURL("")).toBeUndefined(); + }); +}); + describe("deriveCommentKey", () => { test("uses idempotencyKey when set", () => { expect( @@ -77,6 +165,15 @@ describe("deriveCommentKey", () => { ).toBe("https://code.acme.com/owner/repo/issues/42"); }); + test("derives /# from a GHES URL when serverURL matches", () => { + expect( + deriveCommentKey({ + githubURL: "https://github.example.com/owner/repo/issues/123", + serverURL: "https://github.example.com", + }), + ).toBe("owner/repo#123"); + }); + test("appends workflow suffix to the derived per-target key", () => { expect( deriveCommentKey({ diff --git a/src/comment.ts b/src/comment.ts index fbb7aff..13a1079 100644 --- a/src/comment.ts +++ b/src/comment.ts @@ -10,15 +10,34 @@ export { normalizeBaseUrl } from "./url"; type Octokit = ReturnType; -// Anchored regex for a GitHub issue or PR URL on `github.com`. Anchored -// at both ends so a non-github host or extra path segments +// Default GitHub server, used when `GITHUB_SERVER_URL` is unset (outside a +// real Actions runner, e.g. tests or local invocation). +export const DEFAULT_GITHUB_SERVER_URL = "https://github.com"; + +// Anchored issue/PR URL matcher for `serverURL`, compiled once per server URL. +// In production the server URL is fixed for the run, so this cache only ever +// holds a single entry; it grows past one entry solely under tests that +// exercise multiple hosts. `RegExp.escape` neutralizes metacharacters in the +// host (e.g. the dots in `github.com`) so it matches literally. Anchored at +// both ends so a non-server host or extra path segments // (e.g. `.../issues/123/files`, `https://attacker.example/owner/repo/issues/1`) // are rejected rather than silently truncated. The `(?:[?#].*)?` group keeps // the anchor tolerant of query strings and fragments that real-world // `github-url` inputs can carry (e.g. a URL copied while viewing a specific // comment). -const GITHUB_URL_REGEX = - /^https:\/\/github\.com\/([^/]+)\/([^/]+)\/(?:issues|pull)\/(\d+)\/?(?:[?#].*)?$/; +const githubURLRegexCache = new Map(); + +function githubURLRegex(serverURL: string): RegExp { + let regex = githubURLRegexCache.get(serverURL); + if (!regex) { + const base = RegExp.escape(normalizeBaseUrl(serverURL)); + regex = new RegExp( + `^${base}/([^/]+)/([^/]+)/(?:issues|pull)/(\\d+)/?(?:[?#].*)?$`, + ); + githubURLRegexCache.set(serverURL, regex); + } + return regex; +} /** * Parsed components of a `github-url` input. Returned by @@ -31,27 +50,26 @@ export interface GithubItemURL { } /** - * Validate `input` as a `https://github.com///(issues|pull)/` - * URL and return its components, or `undefined` if it does not match. The - * host is anchored to `github.com` so a workflow that templates user- - * controlled content into `github-url` cannot coerce the action into - * commenting on an arbitrary attacker-chosen owner/repo. + * Validate `input` as a `///(issues|pull)/` URL and + * return its components, or `undefined` if it does not match. The host is + * anchored to `serverURL` (the runner-provided `GITHUB_SERVER_URL`, defaulting + * to `https://github.com`) so a workflow that templates user-controlled content + * into `github-url` cannot coerce the action into commenting on an arbitrary + * attacker-chosen host. `serverURL` is runner-controlled, never user-controlled. */ export function parseGithubItemURL( input: string | undefined, + serverURL: string = DEFAULT_GITHUB_SERVER_URL, ): GithubItemURL | undefined { if (!input) { return undefined; } - const match = input.match(GITHUB_URL_REGEX); + const match = githubURLRegex(serverURL).exec(input); if (!match) { return undefined; } - return { - owner: match[1], - repo: match[2], - number: Number.parseInt(match[3], 10), - }; + const [, owner, repo, number] = match; + return { owner, repo, number: Number.parseInt(number, 10) }; } // Discriminated union so spend-exceeded fields are only representable on the @@ -97,12 +115,13 @@ export function deriveCommentKey( inputs: Pick & { idempotencyKey?: string; workflow?: string; + serverURL?: string; }, ): string { if (inputs.idempotencyKey) { return sanitizeLabelToken(inputs.idempotencyKey); } - const parsed = parseGithubItemURL(inputs.githubURL); + const parsed = parseGithubItemURL(inputs.githubURL, inputs.serverURL); let base: string; if (!parsed) { // The action validates githubURL upstream; if we get here the input is