Fix paste ownership, input bounds, and IPC validation

Supersedes #5745, #5746, and #5747.
This commit is contained in:
Jinwoo Hong
2026-06-19 17:14:55 -07:00
committed by GitHub
parent a5920b2183
commit 972078f2c4
592 changed files with 33301 additions and 3735 deletions
+37
View File
@@ -75,6 +75,7 @@ import {
_resetMergeQueueCacheForTests,
_resetOwnerRepoCache
} from './client'
import { GITHUB_WORK_ITEMS_QUERY_MAX_BYTES } from '../../shared/github-work-items-query-bounds'
describe('listWorkItems', () => {
beforeEach(() => {
@@ -277,6 +278,28 @@ describe('listWorkItems', () => {
)
})
it('rejects oversized queries before resolving repo sources or executing gh', async () => {
const secret = 'main-github-work-items-secret'
const oversizedQuery = secret + 'x'.repeat(GITHUB_WORK_ITEMS_QUERY_MAX_BYTES)
await expect(listWorkItems('/repo-root', 10, oversizedQuery)).resolves.toEqual({
items: [],
sources: {
issues: null,
prs: null,
originCandidate: null,
upstreamCandidate: null
}
})
expect(resolveIssueSourceMock).not.toHaveBeenCalled()
expect(getIssueOwnerRepoMock).not.toHaveBeenCalled()
expect(getOwnerRepoMock).not.toHaveBeenCalled()
expect(ghExecFileAsyncMock).not.toHaveBeenCalled()
expect(acquireMock).not.toHaveBeenCalled()
expect(releaseMock).not.toHaveBeenCalled()
})
it('hydrates PR list rows with repository merge metadata', async () => {
getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
@@ -517,6 +540,20 @@ describe('listWorkItems', () => {
expect(apiPath).not.toContain('-is:merged')
})
it('returns zero for oversized count queries before resolving repo sources', async () => {
const secret = 'main-github-work-items-secret'
const oversizedQuery = secret + 'x'.repeat(GITHUB_WORK_ITEMS_QUERY_MAX_BYTES)
await expect(countWorkItems('/repo-root', oversizedQuery)).resolves.toBe(0)
expect(resolveIssueSourceMock).not.toHaveBeenCalled()
expect(getIssueOwnerRepoMock).not.toHaveBeenCalled()
expect(getOwnerRepoMock).not.toHaveBeenCalled()
expect(ghExecFileAsyncMock).not.toHaveBeenCalled()
expect(acquireMock).not.toHaveBeenCalled()
expect(releaseMock).not.toHaveBeenCalled()
})
it('passes review-requested as a --search qualifier (gh CLI has no dedicated flag)', async () => {
getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
+17 -2
View File
@@ -28,6 +28,7 @@ import {
normalizeHostedReviewHeadRef
} from '../../shared/hosted-review-refs'
import { normalizeGitHubPRMergeMethodSettings } from '../../shared/github-pr-merge-methods'
import { isGitHubWorkItemsQueryTooLarge } from '../../shared/github-work-items-query-bounds'
import { parseTaskQuery, type ParsedTaskQuery } from '../../shared/task-query'
import {
GITHUB_WORK_ITEMS_SSH_REMOTE_REQUIRED_MESSAGE,
@@ -1259,13 +1260,24 @@ export async function listWorkItems(
noCache?: boolean,
localGitOptions: LocalGitExecOptions = {}
): Promise<ListWorkItemsResult<MainWorkItem>> {
const trimmedQuery = query?.trim() ?? ''
if (isGitHubWorkItemsQueryTooLarge(trimmedQuery)) {
return {
items: [],
sources: {
issues: null,
prs: null,
originCandidate: null,
upstreamCandidate: null
}
}
}
const [issueResolved, prResolved] = await Promise.all([
resolveIssueSource(repoPath, preference, connectionId, localGitOptions),
resolvePrWorkItemSource(repoPath, preference, connectionId, localGitOptions)
])
const issueOwnerRepo = issueResolved.source
const prOwnerRepo = prResolved.source
const trimmedQuery = query?.trim() ?? ''
await acquire()
try {
// Why: errors propagate to IPC so the renderer's cross-repo aggregator can
@@ -1417,6 +1429,10 @@ export async function countWorkItems(
connectionId?: string | null,
localGitOptions: LocalGitExecOptions = {}
): Promise<number> {
const trimmedQuery = query?.trim() ?? ''
if (isGitHubWorkItemsQueryTooLarge(trimmedQuery)) {
return 0
}
const [issueResolved, prResolved] = await Promise.all([
resolveIssueSource(repoPath, preference, connectionId, localGitOptions),
resolvePrWorkItemSource(repoPath, preference, connectionId, localGitOptions)
@@ -1428,7 +1444,6 @@ export async function countWorkItems(
return 0
}
const trimmedQuery = query?.trim() ?? ''
const parsedQuery = trimmedQuery ? parseTaskQuery(trimmedQuery) : null
const effectiveQuery = parsedQuery ?? defaultOpenWorkItemQuery()
+39 -1
View File
@@ -8,6 +8,10 @@
// (d) parseProjectPaste shorthand owner-only alphabet matches the renderer,
// (e) project owner/capability caches stay bounded in long sessions.
import { beforeEach, describe, expect, it } from 'vitest'
import {
GITHUB_PROJECT_REF_INPUT_MAX_BYTES,
GITHUB_PROJECT_REF_INPUT_TOO_LARGE_ERROR
} from '../../shared/github-project-ref-input'
import {
PROJECT_VIEW_OWNER_CACHE_MAX_ENTRIES,
_getProjectViewCacheSizesForTests,
@@ -21,7 +25,8 @@ import {
classifyProjectError,
isValidOwnerSlug,
isValidRepoSlug,
parseProjectPaste
parseProjectPaste,
resolveProjectRef
} from './project-view'
describe('classifyProjectError', () => {
@@ -151,6 +156,39 @@ describe('parseProjectPaste', () => {
expect(parseProjectPaste('')).toBeNull()
expect(parseProjectPaste(' ')).toBeNull()
})
it('rejects oversized valid-looking URLs without parsing the secret-bearing tail', () => {
const secret = 'project-url-secret'
const input = [
'https://github.com/orgs/acme/projects/42?',
secret,
'x'.repeat(GITHUB_PROJECT_REF_INPUT_MAX_BYTES)
].join('')
expect(parseProjectPaste(input)).toBeNull()
})
})
describe('resolveProjectRef', () => {
it('rejects oversized project refs with a metadata-only validation error', async () => {
const secret = 'project-url-secret'
const input = [
'https://github.com/orgs/acme/projects/42?',
secret,
'x'.repeat(GITHUB_PROJECT_REF_INPUT_MAX_BYTES)
].join('')
await expect(resolveProjectRef({ input })).resolves.toEqual({
ok: false,
error: {
type: 'validation_error',
message: GITHUB_PROJECT_REF_INPUT_TOO_LARGE_ERROR
}
})
await expect(resolveProjectRef({ input })).resolves.not.toMatchObject({
error: { message: expect.stringContaining(secret) }
})
})
})
describe('project view owner caches', () => {
+16 -2
View File
@@ -47,6 +47,10 @@ import type {
ResolveProjectRefArgs,
ResolveProjectRefResult
} from '../../shared/github-project-types'
import {
GITHUB_PROJECT_REF_INPUT_TOO_LARGE_ERROR,
isGitHubProjectRefInputTooLarge
} from '../../shared/github-project-ref-input'
// Re-export the public API so existing call sites (`./project-view`) keep
// working unchanged. The split is internal-only.
@@ -1565,6 +1569,9 @@ export function parseProjectPaste(input: string): ParsedPaste | null {
if (!trimmed) {
return null
}
if (isGitHubProjectRefInputTooLarge(trimmed)) {
return null
}
// URL forms
const urlRe =
/^https?:\/\/github\.com\/(orgs|users)\/([^/]+)\/projects\/(\d+)(?:\/views\/(\d+))?/i
@@ -1686,13 +1693,20 @@ async function resolveOwnerType(
export async function resolveProjectRef(
args: ResolveProjectRefArgs
): Promise<ResolveProjectRefResult> {
if (typeof args.input !== 'string' || !args.input.trim()) {
const input = typeof args.input === 'string' ? args.input.trim() : ''
if (!input) {
return {
ok: false,
error: { type: 'validation_error', message: 'Input required.' }
}
}
const parsed = parseProjectPaste(args.input)
if (isGitHubProjectRefInputTooLarge(input)) {
return {
ok: false,
error: { type: 'validation_error', message: GITHUB_PROJECT_REF_INPUT_TOO_LARGE_ERROR }
}
}
const parsed = parseProjectPaste(input)
if (!parsed) {
return {
ok: false,