Fix GitHub issue search repository scope

Author: ndisidore

packages/gatekeeper-github/__tests__/cloudflare-workers.ts

@@ -0,0 +1 @@
+// Node test shim for github-api.ts's Workers runtime import.

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

@@ -0,0 +1,103 @@
+import { afterEach, describe, expect, it, vi } from "vitest";
+import {
+  GitHubApi,
+  type GitHubIssueResponse,
+} from "../src/github-api";
+import {
+  assertIssueSearchResultsInRepo,
+  buildIssueSearchQuery,
+} from "../src/github-search";
+
+function issueAt(htmlUrl: string): Pick<GitHubIssueResponse, "html_url"> {
+  return { html_url: htmlUrl };
+}
+
+afterEach(() => {
+  vi.unstubAllGlobals();
+});
+
+describe("assertIssueSearchResultsInRepo", () => {
+  it("accepts exact repository path segments case-insensitively", () => {
+    expect(() => assertIssueSearchResultsInRepo("Cloudflare", "Workerd", [
+      issueAt("https://github.com/cloudflare/workerd/issues/1"),
+    ])).not.toThrow();
+  });
+
+  it("rejects results from another repository", () => {
+    expect(() => assertIssueSearchResultsInRepo("cloudflare", "workerd", [
+      issueAt("https://github.com/cloudflare/quiche/issues/1"),
+    ])).toThrow("outside the connected repository");
+  });
+
+  it("does not accept repository names that only share a prefix", () => {
+    expect(() => assertIssueSearchResultsInRepo("cloudflare", "workerd", [
+      issueAt("https://github.com/cloudflare/workerd-private/issues/1"),
+    ])).toThrow("outside the connected repository");
+  });
+
+  it("rejects pull requests returned by an injected search expression", () => {
+    expect(() => assertIssueSearchResultsInRepo("cloudflare", "workerd", [
+      issueAt("https://github.com/cloudflare/workerd/pull/1"),
+    ])).toThrow("non-issue result");
+  });
+
+  it("rejects malformed and non-GitHub result URLs", () => {
+    expect(() => assertIssueSearchResultsInRepo("cloudflare", "workerd", [
+      issueAt("not a URL"),
+    ])).toThrow("outside the connected repository");
+    expect(() => assertIssueSearchResultsInRepo("cloudflare", "workerd", [
+      issueAt("https://example.com/cloudflare/workerd/issues/1"),
+    ])).toThrow("outside the connected repository");
+  });
+});
+
+describe("buildIssueSearchQuery", () => {
+  it("builds a benign literal phrase search with structured filters", () => {
+    expect(buildIssueSearchQuery("cloudflare", "workerd", {
+      text: "durable objects",
+      state: "open",
+      labels: ["bug"],
+      author: "jasnell",
+    })).toBe(
+      '"durable objects" repo:cloudflare/workerd is:issue state:open label:"bug" author:"jasnell"',
+    );
+  });
+
+  it("quotes every caller-controlled query fragment", () => {
+    expect(buildIssueSearchQuery("cloudflare", "workerd", {
+      text: "repo:cloudflare/quiche OR scheduler",
+      author: "jasnell OR repo:cloudflare/quiche",
+      assignee: "octocat OR repo:cloudflare/quiche",
+    })).toBe(
+      '"repo:cloudflare/quiche OR scheduler" repo:cloudflare/workerd is:issue '
+      + 'author:"jasnell OR repo:cloudflare/quiche" assignee:"octocat OR repo:cloudflare/quiche"',
+    );
+  });
+
+  it("escapes quotes inside plain search text", () => {
+    expect(buildIssueSearchQuery("cloudflare", "workerd", {
+      text: 'bug" OR repo:cloudflare/quiche OR "',
+    })).toBe('"bug\\" OR repo:cloudflare/quiche OR \\"" repo:cloudflare/workerd is:issue');
+  });
+});
+
+describe("GitHubApi.searchIssuesConditional", () => {
+  it("enables GitHub advanced search parsing", async () => {
+    let requestUrl: URL | undefined;
+    vi.stubGlobal("fetch", vi.fn(async (input: string | URL | Request) => {
+      requestUrl = new URL(String(input));
+      return new Response(JSON.stringify({ items: [] }), {
+        headers: { "content-type": "application/json" },
+      });
+    }));
+
+    const api = new GitHubApi(async () => "test-token");
+    await api.searchIssuesConditional(
+      "repo:cloudflare/quiche OR repo:cloudflare/workerd is:issue",
+      1,
+      100,
+    );
+
+    expect(requestUrl?.searchParams.get("advanced_search")).toBe("true");
+  });
+});

packages/gatekeeper-github/package.json

@@ -9,6 +9,7 @@
     "deploy": "pnpm run build:configurator && wrangler deploy",
     "build": "pnpm run build:configurator && tsc",
     "types:check": "pnpm run build:configurator && tsc --noEmit",
+    "test": "vitest run",
     "clean": "rm -rf dist src/generated"
   },
   "dependencies": {
@@ -20,6 +21,7 @@
   },
   "devDependencies": {
     "typescript": "^5.9.3",
+    "vitest": "^4.1.10",
     "wrangler": "^4.115.0"
   }
 }

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

@@ -596,6 +596,7 @@ export class GitHubApi {
       "/search/issues",
       {
         q: query,
+        advanced_search: true,
         page,
         per_page: perPage,
         sort,

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

@@ -0,0 +1,40 @@
+import type { GitHubIssueResponse } from "./github-api";
+import type { GitHubIssueSearch } from "./types";
+
+export function buildIssueSearchQuery(owner: string, repo: string, query: GitHubIssueSearch): string {
+  const parts = [query.text ? JSON.stringify(query.text) : "", `repo:${owner}/${repo}`, "is:issue"];
+  if (query.state && query.state !== "all") parts.push(`state:${query.state}`);
+  for (const label of query.labels ?? []) {
+    parts.push(`label:${JSON.stringify(label)}`);
+  }
+  if (query.author) parts.push(`author:${JSON.stringify(query.author)}`);
+  if (query.assignee) parts.push(`assignee:${JSON.stringify(query.assignee)}`);
+  return parts.filter(Boolean).join(" ");
+}
+
+export function assertIssueSearchResultsInRepo(
+  owner: string,
+  repo: string,
+  results: readonly Pick<GitHubIssueResponse, "html_url">[],
+): void {
+  const expectedOwner = owner.toLowerCase();
+  const expectedRepo = repo.toLowerCase();
+
+  for (const result of results) {
+    let url: URL | undefined;
+    try {
+      url = new URL(result.html_url);
+    } catch {
+      // Handled by the scope check below.
+    }
+
+    const [resultOwner, resultRepo, resultKind] = url?.pathname.split("/").filter(Boolean) ?? [];
+    if (url?.protocol !== "https:" || url.hostname.toLowerCase() !== "github.com"
+        || resultOwner?.toLowerCase() !== expectedOwner || resultRepo?.toLowerCase() !== expectedRepo) {
+      throw new Error("GitHub returned an issue outside the connected repository.");
+    }
+    if (resultKind !== "issues") {
+      throw new Error("GitHub returned a non-issue result for an issue search.");
+    }
+  }
+}

packages/gatekeeper-github/src/github.ts

@@ -29,6 +29,7 @@ import {
   type GitHubPullRequestResponse,
   type GitHubPullRequestReviewCommentResponse,
 } from "./github-api";
+import { assertIssueSearchResultsInRepo, buildIssueSearchQuery } from "./github-search";
 import GITHUB_LOGO_SVG from "./github-logo.svg";
 import type {
   GitHubActor,
@@ -110,6 +111,11 @@ type Cached<T> = {
   generation: number;
 };
 
+type CachedIssueSearchResult = {
+  html_url: string;
+  summary: GitHubIssueSummary;
+};
+
 type GitHubDiscussionCommentEntry = Extract<GitHubDiscussionEntry, { kind: "comment" }>;
 
 type StoredCommentCacheState = {
@@ -696,17 +702,6 @@ function pullComparator(
   };
 }
 
-function buildIssueSearchQuery(owner: string, repo: string, query: GitHubIssueSearch): string {
-  const parts = [query.text, `repo:${owner}/${repo}`, "is:issue"];
-  if (query.state && query.state !== "all") parts.push(`state:${query.state}`);
-  for (const label of query.labels ?? []) {
-    parts.push(`label:${JSON.stringify(label)}`);
-  }
-  if (query.author) parts.push(`author:${query.author}`);
-  if (query.assignee) parts.push(`assignee:${query.assignee}`);
-  return parts.filter(Boolean).join(" ");
-}
-
 function parseDiffSide(side?: "LEFT" | "RIGHT" | null): "old" | "new" {
   return side === "LEFT" ? "old" : "new";
 }
@@ -2562,28 +2557,42 @@ export class GitHubGatekeeperImpl extends DurableObject<Env, GitHubGatekeeperImp
     const owner = this.ctx.props.owner;
     const repo = this.ctx.props.repo;
     const searchQuery = buildIssueSearchQuery(owner, repo, query);
+    const assertSearchScope = (results: readonly Pick<GitHubIssueResponse, "html_url">[]) => {
+      try {
+        assertIssueSearchResultsInRepo(owner, repo, results);
+      } catch (error) {
+        logger.warn("GitHub issue search scope validation failed", {
+          event: "issue.search.scope.validation.failed", error,
+        });
+        throw error;
+      }
+    };
     return new StreamingCursor<GitHubIssueSummary>({
       fetchPage: async (page, perPage) => {
-        const cacheKey = this.#cacheKey("search-issues", stableKey(query), `p${page}`);
-        return await this.#loadCachedWithEtag<GitHubIssueSummary[]>(cacheKey, LIST_CACHE_TTL_MS, async etag => {
+        const cacheKey = this.#cacheKey("search-issues-scoped-v1", stableKey(query), `p${page}`);
+        const results = await this.#loadCachedWithEtag<CachedIssueSearchResult[]>(cacheKey, LIST_CACHE_TTL_MS, async etag => {
           const raw = await this.#withApi(api =>
             api.searchIssuesConditional(searchQuery, page, perPage, remoteSort, remoteDirection, { ifNoneMatch: etag })
           );
           if (raw.status === 304) {
             return raw;
           }
 
+          assertSearchScope(raw.data.items);
           return {
             status: 200,
             headers: raw.headers,
-            data: raw.data.items
-              .filter(item => !item.pull_request)
-              .map(item => normalizeIssueSummary(owner, repo, item)),
+            data: raw.data.items.map(item => ({
+              html_url: item.html_url,
+              summary: normalizeIssueSummary(owner, repo, item),
+            })),
           };
         });
+        assertSearchScope(results);
+        return results.map(item => item.summary);
       },
       overlay: item => this.#overlayIssueLike(item, "issue", item.id),
-      filter: () => true,  // Remote search results are already filtered by GitHub's search API.
+      filter: () => true,  // Search scope was validated before results entered the cursor.
       comparator: compare,
       injectedItems: provisionals,
       pageSize,

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

@@ -53,8 +53,8 @@ export interface GitHubRepo {
   /**
    * Searches issues in this repository.
    *
-   * The search is limited to this repository. `query.text` is a plain-text search
-   * string; the remaining fields are structured filters.
+   * The search is limited to this repository. `query.text` is matched as a literal phrase;
+   * search qualifiers in it are not interpreted. The remaining fields are structured filters.
    */
   searchIssues(query: GitHubIssueSearch): Promise<Cursor<GitHubIssueSummary>>;
 

packages/gatekeeper-github/storage-schema.md

@@ -125,7 +125,7 @@ Implemented TTL cache families:
 - `cache:issue:<realId>` -> `GitHubIssueDetails`
 - `cache:pull:<realId>` -> `GitHubPullRequestDetails`
 - `cache:list-issues:<encodedQuery>` -> `GitHubIssueSummary[]`
-- `cache:search-issues:<encodedQuery>` -> `GitHubIssueSummary[]`
+- `cache:search-issues-scoped-v1:<encodedQuery>` -> validated source URLs and `GitHubIssueSummary` values
 - `cache:list-pulls:<encodedQuery>` -> `GitHubPullRequestSummary[]`
 - `cache:search-pulls:<encodedQuery>` -> `GitHubPullRequestSummary[]`
 - `cache:discussion-reviews:<realId>:p<page>` -> `GitHubDiscussionEntry[]` review-summary pages for pull discussions

packages/gatekeeper-github/vitest.config.ts

@@ -0,0 +1,14 @@
+import { fileURLToPath } from "node:url";
+import { defineConfig } from "vitest/config";
+
+export default defineConfig({
+  resolve: {
+    alias: {
+      "cloudflare:workers": fileURLToPath(new URL("./__tests__/cloudflare-workers.ts", import.meta.url)),
+    },
+  },
+  test: {
+    include: ["__tests__/*.test.ts"],
+    environment: "node",
+  },
+});