Skip to content

Commit 16a626d

Browse files
ndisidorendisidore
authored
Fix GitHub issue search repository scope (#49)
* Fix GitHub issue search repository scope * Simplify GitHub gatekeeper test setup
1 parent c6d8999 commit 16a626d

9 files changed

Lines changed: 186 additions & 22 deletions

File tree

‎packages/gatekeeper-github/__tests__/github-api.test.ts‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
import { afterEach, describe, expect, it, vi } from "vitest";
2+
import {
3+
GitHubApi,
4+
type GitHubIssueResponse,
5+
} from "../src/github-api";
6+
import {
7+
assertIssueSearchResultsInRepo,
8+
buildIssueSearchQuery,
9+
} from "../src/github-search";
10+
11+
function issueAt(htmlUrl: string): Pick<GitHubIssueResponse, "html_url"> {
12+
return { html_url: htmlUrl };
13+
}
14+
15+
afterEach(() => {
16+
vi.unstubAllGlobals();
17+
});
18+
19+
describe("assertIssueSearchResultsInRepo", () => {
20+
it("accepts exact repository path segments case-insensitively", () => {
21+
expect(() => assertIssueSearchResultsInRepo("Cloudflare", "Workerd", [
22+
issueAt("https://github.com/cloudflare/workerd/issues/1"),
23+
])).not.toThrow();
24+
});
25+
26+
it("rejects results from another repository", () => {
27+
expect(() => assertIssueSearchResultsInRepo("cloudflare", "workerd", [
28+
issueAt("https://github.com/cloudflare/quiche/issues/1"),
29+
])).toThrow("outside the connected repository");
30+
});
31+
32+
it("does not accept repository names that only share a prefix", () => {
33+
expect(() => assertIssueSearchResultsInRepo("cloudflare", "workerd", [
34+
issueAt("https://github.com/cloudflare/workerd-private/issues/1"),
35+
])).toThrow("outside the connected repository");
36+
});
37+
38+
it("rejects pull requests returned by an injected search expression", () => {
39+
expect(() => assertIssueSearchResultsInRepo("cloudflare", "workerd", [
40+
issueAt("https://github.com/cloudflare/workerd/pull/1"),
41+
])).toThrow("non-issue result");
42+
});
43+
44+
it("rejects malformed and non-GitHub result URLs", () => {
45+
expect(() => assertIssueSearchResultsInRepo("cloudflare", "workerd", [
46+
issueAt("not a URL"),
47+
])).toThrow("outside the connected repository");
48+
expect(() => assertIssueSearchResultsInRepo("cloudflare", "workerd", [
49+
issueAt("https://example.com/cloudflare/workerd/issues/1"),
50+
])).toThrow("outside the connected repository");
51+
});
52+
});
53+
54+
describe("buildIssueSearchQuery", () => {
55+
it("builds a benign literal phrase search with structured filters", () => {
56+
expect(buildIssueSearchQuery("cloudflare", "workerd", {
57+
text: "durable objects",
58+
state: "open",
59+
labels: ["bug"],
60+
author: "jasnell",
61+
})).toBe(
62+
'"durable objects" repo:cloudflare/workerd is:issue state:open label:"bug" author:"jasnell"',
63+
);
64+
});
65+
66+
it("quotes every caller-controlled query fragment", () => {
67+
expect(buildIssueSearchQuery("cloudflare", "workerd", {
68+
text: "repo:cloudflare/quiche OR scheduler",
69+
author: "jasnell OR repo:cloudflare/quiche",
70+
assignee: "octocat OR repo:cloudflare/quiche",
71+
})).toBe(
72+
'"repo:cloudflare/quiche OR scheduler" repo:cloudflare/workerd is:issue '
73+
+ 'author:"jasnell OR repo:cloudflare/quiche" assignee:"octocat OR repo:cloudflare/quiche"',
74+
);
75+
});
76+
77+
it("escapes quotes inside plain search text", () => {
78+
expect(buildIssueSearchQuery("cloudflare", "workerd", {
79+
text: 'bug" OR repo:cloudflare/quiche OR "',
80+
})).toBe('"bug\\" OR repo:cloudflare/quiche OR \\"" repo:cloudflare/workerd is:issue');
81+
});
82+
});
83+
84+
describe("GitHubApi.searchIssuesConditional", () => {
85+
it("enables GitHub advanced search parsing", async () => {
86+
let requestUrl: URL | undefined;
87+
vi.stubGlobal("fetch", vi.fn(async (input: string | URL | Request) => {
88+
requestUrl = new URL(String(input));
89+
return new Response(JSON.stringify({ items: [] }), {
90+
headers: { "content-type": "application/json" },
91+
});
92+
}));
93+
94+
const api = new GitHubApi(async () => "test-token");
95+
await api.searchIssuesConditional(
96+
"repo:cloudflare/quiche OR repo:cloudflare/workerd is:issue",
97+
1,
98+
100,
99+
);
100+
101+
expect(requestUrl?.searchParams.get("advanced_search")).toBe("true");
102+
});
103+
});

‎packages/gatekeeper-github/package.json‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
"deploy": "pnpm run build:configurator && wrangler deploy",
1010
"build": "pnpm run build:configurator && tsc",
1111
"types:check": "pnpm run build:configurator && tsc --noEmit",
12+
"test": "vitest run",
1213
"clean": "rm -rf dist src/generated"
1314
},
1415
"dependencies": {
@@ -20,6 +21,7 @@
2021
},
2122
"devDependencies": {
2223
"typescript": "^5.9.3",
24+
"vitest": "^4.1.10",
2325
"wrangler": "^4.119.0"
2426
}
2527
}

‎packages/gatekeeper-github/src/github-api.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
import "cloudflare:workers";
2-
31
export type GitHubOAuthGrant = {
42
accessToken: string;
53
scopes: string[];
@@ -596,6 +594,7 @@ export class GitHubApi {
596594
"/search/issues",
597595
{
598596
q: query,
597+
advanced_search: true,
599598
page,
600599
per_page: perPage,
601600
sort,

‎packages/gatekeeper-github/src/github-search.ts‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
import type { GitHubIssueResponse } from "./github-api";
2+
import type { GitHubIssueSearch } from "./types";
3+
4+
export function buildIssueSearchQuery(owner: string, repo: string, query: GitHubIssueSearch): string {
5+
const parts = [query.text ? JSON.stringify(query.text) : "", `repo:${owner}/${repo}`, "is:issue"];
6+
if (query.state && query.state !== "all") parts.push(`state:${query.state}`);
7+
for (const label of query.labels ?? []) {
8+
parts.push(`label:${JSON.stringify(label)}`);
9+
}
10+
if (query.author) parts.push(`author:${JSON.stringify(query.author)}`);
11+
if (query.assignee) parts.push(`assignee:${JSON.stringify(query.assignee)}`);
12+
return parts.filter(Boolean).join(" ");
13+
}
14+
15+
export function assertIssueSearchResultsInRepo(
16+
owner: string,
17+
repo: string,
18+
results: readonly Pick<GitHubIssueResponse, "html_url">[],
19+
): void {
20+
const expectedOwner = owner.toLowerCase();
21+
const expectedRepo = repo.toLowerCase();
22+
23+
for (const result of results) {
24+
let url: URL | undefined;
25+
try {
26+
url = new URL(result.html_url);
27+
} catch {
28+
// Handled by the scope check below.
29+
}
30+
31+
const [resultOwner, resultRepo, resultKind] = url?.pathname.split("/").filter(Boolean) ?? [];
32+
if (url?.protocol !== "https:" || url.hostname.toLowerCase() !== "github.com"
33+
|| resultOwner?.toLowerCase() !== expectedOwner || resultRepo?.toLowerCase() !== expectedRepo) {
34+
throw new Error("GitHub returned an issue outside the connected repository.");
35+
}
36+
if (resultKind !== "issues") {
37+
throw new Error("GitHub returned a non-issue result for an issue search.");
38+
}
39+
}
40+
}

‎packages/gatekeeper-github/src/github.ts‎

Lines changed: 26 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import {
3030
type GitHubPullRequestResponse,
3131
type GitHubPullRequestReviewCommentResponse,
3232
} from "./github-api";
33+
import { assertIssueSearchResultsInRepo, buildIssueSearchQuery } from "./github-search";
3334
import GITHUB_LOGO_SVG from "./github-logo.svg";
3435
import type {
3536
GitHubActor,
@@ -111,6 +112,11 @@ type Cached<T> = {
111112
generation: number;
112113
};
113114

115+
type CachedIssueSearchResult = {
116+
html_url: string;
117+
summary: GitHubIssueSummary;
118+
};
119+
114120
type GitHubDiscussionCommentEntry = Extract<GitHubDiscussionEntry, { kind: "comment" }>;
115121

116122
type StoredCommentCacheState = {
@@ -697,17 +703,6 @@ function pullComparator(
697703
};
698704
}
699705

700-
function buildIssueSearchQuery(owner: string, repo: string, query: GitHubIssueSearch): string {
701-
const parts = [query.text, `repo:${owner}/${repo}`, "is:issue"];
702-
if (query.state && query.state !== "all") parts.push(`state:${query.state}`);
703-
for (const label of query.labels ?? []) {
704-
parts.push(`label:${JSON.stringify(label)}`);
705-
}
706-
if (query.author) parts.push(`author:${query.author}`);
707-
if (query.assignee) parts.push(`assignee:${query.assignee}`);
708-
return parts.filter(Boolean).join(" ");
709-
}
710-
711706
function parseDiffSide(side?: "LEFT" | "RIGHT" | null): "old" | "new" {
712707
return side === "LEFT" ? "old" : "new";
713708
}
@@ -2563,28 +2558,42 @@ export class GitHubGatekeeperImpl extends DurableObject<Env, GitHubGatekeeperImp
25632558
const owner = this.ctx.props.owner;
25642559
const repo = this.ctx.props.repo;
25652560
const searchQuery = buildIssueSearchQuery(owner, repo, query);
2561+
const assertSearchScope = (results: readonly Pick<GitHubIssueResponse, "html_url">[]) => {
2562+
try {
2563+
assertIssueSearchResultsInRepo(owner, repo, results);
2564+
} catch (error) {
2565+
logger.warn("GitHub issue search scope validation failed", {
2566+
event: "issue.search.scope.validation.failed", error,
2567+
});
2568+
throw error;
2569+
}
2570+
};
25662571
return new StreamingCursor<GitHubIssueSummary>({
25672572
fetchPage: async (page, perPage) => {
2568-
const cacheKey = this.#cacheKey("search-issues", stableKey(query), `p${page}`);
2569-
return await this.#loadCachedWithEtag<GitHubIssueSummary[]>(cacheKey, LIST_CACHE_TTL_MS, async etag => {
2573+
const cacheKey = this.#cacheKey("search-issues-scoped-v1", stableKey(query), `p${page}`);
2574+
const results = await this.#loadCachedWithEtag<CachedIssueSearchResult[]>(cacheKey, LIST_CACHE_TTL_MS, async etag => {
25702575
const raw = await this.#withApi(api =>
25712576
api.searchIssuesConditional(searchQuery, page, perPage, remoteSort, remoteDirection, { ifNoneMatch: etag })
25722577
);
25732578
if (raw.status === 304) {
25742579
return raw;
25752580
}
25762581

2582+
assertSearchScope(raw.data.items);
25772583
return {
25782584
status: 200,
25792585
headers: raw.headers,
2580-
data: raw.data.items
2581-
.filter(item => !item.pull_request)
2582-
.map(item => normalizeIssueSummary(owner, repo, item)),
2586+
data: raw.data.items.map(item => ({
2587+
html_url: item.html_url,
2588+
summary: normalizeIssueSummary(owner, repo, item),
2589+
})),
25832590
};
25842591
});
2592+
assertSearchScope(results);
2593+
return results.map(item => item.summary);
25852594
},
25862595
overlay: item => this.#overlayIssueLike(item, "issue", item.id),
2587-
filter: () => true, // Remote search results are already filtered by GitHub's search API.
2596+
filter: () => true, // Search scope was validated before results entered the cursor.
25882597
comparator: compare,
25892598
injectedItems: provisionals,
25902599
pageSize,

‎packages/gatekeeper-github/src/types.d.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,8 @@ export interface GitHubRepo {
5353
/**
5454
* Searches issues in this repository.
5555
*
56-
* The search is limited to this repository. `query.text` is a plain-text search
57-
* string; the remaining fields are structured filters.
56+
* The search is limited to this repository. `query.text` is matched as a literal phrase;
57+
* search qualifiers in it are not interpreted. The remaining fields are structured filters.
5858
*/
5959
searchIssues(query: GitHubIssueSearch): Promise<Cursor<GitHubIssueSummary>>;
6060

‎packages/gatekeeper-github/storage-schema.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ Implemented TTL cache families:
125125
- `cache:issue:<realId>` -> `GitHubIssueDetails`
126126
- `cache:pull:<realId>` -> `GitHubPullRequestDetails`
127127
- `cache:list-issues:<encodedQuery>` -> `GitHubIssueSummary[]`
128-
- `cache:search-issues:<encodedQuery>` -> `GitHubIssueSummary[]`
128+
- `cache:search-issues-scoped-v1:<encodedQuery>` -> validated source URLs and `GitHubIssueSummary` values
129129
- `cache:list-pulls:<encodedQuery>` -> `GitHubPullRequestSummary[]`
130130
- `cache:search-pulls:<encodedQuery>` -> `GitHubPullRequestSummary[]`
131131
- `cache:discussion-reviews:<realId>:p<page>` -> `GitHubDiscussionEntry[]` review-summary pages for pull discussions

‎packages/gatekeeper-github/vitest.config.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
import { defineConfig } from "vitest/config";
2+
3+
export default defineConfig({
4+
test: {
5+
include: ["__tests__/*.test.ts"],
6+
environment: "node",
7+
},
8+
});

‎pnpm-lock.yaml‎

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)