From 321e932142ddfa035345fcde6715e7c8742475c9 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 10 Aug 2026 23:57:11 -0700 Subject: [PATCH] fix(relay): degrade to legacy branch diff instead of rejecting Harden the pinned-OID route against mixed-version and unpinned inputs: - Treat an explicit null headOid as unpinned. GitBranchCompareSummary types headOid as `string | null`, so a client built before the SSH provider normalized it can put null on the wire; rejecting there would turn a diff an old relay served into a user-visible error. - Require a full-hex baseRef to select the pinned route. Previously a symbolic base selected the route and then threw from inside the operation; it now falls through to the untouched legacy path. - Normalize a null headOid to absent in SshGitProvider so it is omitted from relay params and shares the absent-head dedupe key. Keeps the module-boundary OID asserts as defense-in-depth and restores direct coverage for them. Co-authored-by: Orca --- src/main/providers/ssh-git-provider.test.ts | 46 +++++++ src/main/providers/ssh-git-provider.ts | 6 +- src/relay/git-handler-branch-diff-ops.ts | 11 +- src/relay/git-handler-branch-diff.test.ts | 129 ++++++++++++++++---- src/relay/git-handler.ts | 2 + 5 files changed, 170 insertions(+), 24 deletions(-) diff --git a/src/main/providers/ssh-git-provider.test.ts b/src/main/providers/ssh-git-provider.test.ts index 955e840632a..20ebd9293a1 100644 --- a/src/main/providers/ssh-git-provider.test.ts +++ b/src/main/providers/ssh-git-provider.test.ts @@ -1113,6 +1113,52 @@ describe('SshGitProvider', () => { }) }) + // Why: an unpinned compare snapshot reaches getBranchDiff as null despite the type. + it('omits a null head OID and shares the absent-head dedupe key', async () => { + const diffs = [{ kind: 'text', originalContent: 'old', modifiedContent: 'new' }] + const pendingDiff = deferredValue(diffs) + const headOid = 'a'.repeat(40) + mux.request.mockReturnValue(pendingDiff.promise) + + const reads = [ + provider.getBranchDiff('/home/user/repo', 'main', { + includePatch: true, + filePath: 'src/file.ts', + headOid: null as unknown as undefined + }), + provider.getBranchDiff('/home/user/repo', 'main', { + includePatch: true, + filePath: 'src/file.ts' + }), + provider.getBranchDiff('/home/user/repo', 'main', { + includePatch: true, + filePath: 'src/file.ts', + headOid + }) + ] + + await waitForRequestCount(mux.request, 2) + expect(mux.request).toHaveBeenCalledTimes(2) + expect(mux.request).toHaveBeenNthCalledWith(1, 'git.branchDiff', { + worktreePath: '/home/user/repo', + baseRef: 'main', + includePatch: true, + filePath: 'src/file.ts', + __streamResponse: true + }) + expect(mux.request).toHaveBeenNthCalledWith(2, 'git.branchDiff', { + worktreePath: '/home/user/repo', + baseRef: 'main', + includePatch: true, + filePath: 'src/file.ts', + headOid, + __streamResponse: true + }) + + pendingDiff.resolve() + await expect(Promise.all(reads)).resolves.toEqual(Array.from({ length: 3 }, () => diffs)) + }) + it('coalesces matching branch diff heads while keeping distinct and absent heads separate', async () => { const diffs = [{ kind: 'text', originalContent: 'old', modifiedContent: 'new' }] const pendingDiff = deferredValue(diffs) diff --git a/src/main/providers/ssh-git-provider.ts b/src/main/providers/ssh-git-provider.ts index 5b91eada817..51e2b91e1c2 100644 --- a/src/main/providers/ssh-git-provider.ts +++ b/src/main/providers/ssh-git-provider.ts @@ -662,7 +662,11 @@ export class SshGitProvider implements IGitProvider { options?: { includePatch?: boolean; filePath?: string; oldPath?: string; headOid?: string } ): Promise { const keyOptions = options ?? {} - const { headOid, ...relayOptions } = keyOptions + const { headOid: rawHeadOid, ...relayOptions } = keyOptions + // Why: compare snapshots type headOid as `string | null`, so collapse an + // unpinned null to absent instead of putting a field on the wire that a + // pinned-OID relay must reject. + const headOid = rawHeadOid == null ? undefined : rawHeadOid return this.gitDiffReadDedupe.run( stableInFlightKey([ 'branchDiff', diff --git a/src/relay/git-handler-branch-diff-ops.ts b/src/relay/git-handler-branch-diff-ops.ts index 98553266d85..1da0de82be8 100644 --- a/src/relay/git-handler-branch-diff-ops.ts +++ b/src/relay/git-handler-branch-diff-ops.ts @@ -9,13 +9,20 @@ function assertFullGitObjectId(value: unknown, label: string): asserts value is } } +export function isFullGitObjectId(value: unknown): value is string { + return typeof value === 'string' && FULL_GIT_OBJECT_ID_PATTERN.test(value) +} + export function parseOptionalBranchDiffHeadOid( params: Record ): string | undefined { - if (!Object.hasOwn(params, 'headOid')) { + const { headOid } = params + // Why: GitBranchCompareSummary.headOid is `string | null`, so a mixed-version + // client can put an explicit null on the wire. Treat it as unpinned rather + // than rejecting a request the legacy path would have served. + if (headOid == null) { return undefined } - const { headOid } = params assertFullGitObjectId(headOid, 'headOid') return headOid } diff --git a/src/relay/git-handler-branch-diff.test.ts b/src/relay/git-handler-branch-diff.test.ts index b52d459a3ca..ce60c8cd439 100644 --- a/src/relay/git-handler-branch-diff.test.ts +++ b/src/relay/git-handler-branch-diff.test.ts @@ -6,6 +6,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { RelayContext } from './context' import { branchDiffEntryAtPinnedOids, + isFullGitObjectId, parseOptionalBranchDiffHeadOid } from './git-handler-branch-diff-ops' import { GitHandler } from './git-handler' @@ -21,6 +22,7 @@ import { const BASE_OID = 'a'.repeat(40) const HEAD_OID = 'b'.repeat(40) const OTHER_HEAD_OID = 'c'.repeat(40) +const MERGE_BASE_OID = 'd'.repeat(40) const FILE_PATH = 'src/file.ts' type GitBufferTarget = { @@ -99,11 +101,51 @@ describe('pinned relay branch diff operation', () => { expect(pinned).toEqual(legacy) }) - it('treats an omitted head OID differently from a supplied invalid value', () => { - expect(parseOptionalBranchDiffHeadOid({})).toBeUndefined() - expect(() => parseOptionalBranchDiffHeadOid({ headOid: null })).toThrow( - 'headOid must be a full git object id' - ) + // Why: GitBranchCompareSummary.headOid is `string | null`, so an unpinned + // snapshot arrives as an explicit null and must stay servable. + it.each([ + { name: 'absent', params: {} }, + { name: 'null', params: { headOid: null } }, + { name: 'undefined', params: { headOid: undefined } } + ])('treats a $name head OID as unpinned', ({ params }) => { + expect(parseOptionalBranchDiffHeadOid(params)).toBeUndefined() + }) + + it.each([ + { name: 'SHA-1', headOid: HEAD_OID }, + { name: 'SHA-256', headOid: 'b'.repeat(64) } + ])('returns a supplied $name head OID', ({ headOid }) => { + expect(parseOptionalBranchDiffHeadOid({ headOid })).toBe(headOid) + }) + + it.each([{ headOid: 'abc123' }, { headOid: '' }, { headOid: 'main' }, { headOid: 123 }])( + 'rejects the supplied malformed head OID $headOid', + ({ headOid }) => { + expect(() => parseOptionalBranchDiffHeadOid({ headOid })).toThrow( + 'headOid must be a full git object id' + ) + } + ) + + it('recognizes only full object ids', () => { + expect(isFullGitObjectId(BASE_OID)).toBe(true) + expect(isFullGitObjectId('A'.repeat(64))).toBe(true) + for (const value of ['origin/main', 'abc123', '', 'g'.repeat(40), null, undefined, 123, {}]) { + expect(isFullGitObjectId(value)).toBe(false) + } + }) + + // Why: the route guards this today, but the export is reachable on its own — + // a symbolic ref here would resolve `git show origin/main:` at live HEAD. + it('rejects a symbolic ref at the module boundary', async () => { + const gitBuffer = vi.fn(async () => Buffer.from('')) + await expect( + branchDiffEntryAtPinnedOids(gitBuffer, '/repo', 'origin/main', HEAD_OID, FILE_PATH) + ).rejects.toThrow('baseRef must be a full git object id') + await expect( + branchDiffEntryAtPinnedOids(gitBuffer, '/repo', BASE_OID, 'HEAD', FILE_PATH) + ).rejects.toThrow('headOid must be a full git object id') + expect(gitBuffer).not.toHaveBeenCalled() }) }) @@ -132,6 +174,21 @@ describe('GitHandler pinned branch diff route', () => { }) } + function mockLegacyGit(nameStatus = '') { + return vi.spyOn(handler as unknown as GitTarget, 'git').mockImplementation(async (args) => { + if (args[0] === 'rev-parse' && args.includes('HEAD')) { + return { stdout: `${HEAD_OID}\n`, stderr: '' } + } + if (args[0] === 'rev-parse') { + return { stdout: `${BASE_OID}\n`, stderr: '' } + } + if (args[0] === 'merge-base') { + return { stdout: `${MERGE_BASE_OID}\n`, stderr: '' } + } + return { stdout: nameStatus, stderr: '' } + }) + } + it('starts exactly two pinned blob reads concurrently without metadata commands', async () => { const left = deferredBuffer() const right = deferredBuffer() @@ -206,7 +263,7 @@ describe('GitHandler pinned branch diff route', () => { it('rejects every supplied malformed head OID before running Git', async () => { const gitBufferSpy = vi.spyOn(handler as unknown as GitBufferTarget, 'gitBuffer') - const invalidHeadOids = [null, 7, {}, '', 'a'.repeat(39), 'g'.repeat(40)] + const invalidHeadOids = [7, {}, '', 'a'.repeat(39), 'g'.repeat(40)] for (const headOid of invalidHeadOids) { await expect(request({ headOid })).rejects.toThrow('headOid must be a full git object id') @@ -230,27 +287,57 @@ describe('GitHandler pinned branch diff route', () => { ]) }) - it('requires a full base OID only on the pinned route', async () => { + // Why: a symbolic base ref used to select the pinned route and then throw. + it('selects the pinned route only when the base ref is a full object id', async () => { + const gitSpy = mockLegacyGit(`M\t${FILE_PATH}\n`) + const gitBufferSpy = vi + .spyOn(handler as unknown as GitBufferTarget, 'gitBuffer') + .mockResolvedValue(Buffer.from('content\n')) + + await expect(request({ baseRef: 'origin/main' })).resolves.toHaveLength(1) + + expect(gitSpy.mock.calls.map(([args]) => args.join(' '))).toEqual([ + 'rev-parse --verify HEAD', + 'rev-parse --verify origin/main', + `merge-base ${BASE_OID} ${HEAD_OID}`, + `-c core.quotePath=false diff --name-status -M -C ${MERGE_BASE_OID} ${HEAD_OID}` + ]) + expect(gitBufferSpy.mock.calls.map(([args]) => args[2])).toEqual([ + `${MERGE_BASE_OID}:${FILE_PATH}`, + `${HEAD_OID}:${FILE_PATH}` + ]) + + gitSpy.mockClear() + gitBufferSpy.mockClear() + await request() + + expect(gitSpy).not.toHaveBeenCalled() + expect(gitBufferSpy.mock.calls.map(([args]) => args[2])).toEqual([ + `${BASE_OID}:${FILE_PATH}`, + `${HEAD_OID}:${FILE_PATH}` + ]) + }) + + it('serves an explicitly null head OID through the legacy path', async () => { + const gitSpy = mockLegacyGit(`M\t${FILE_PATH}\n`) const gitBufferSpy = vi.spyOn(handler as unknown as GitBufferTarget, 'gitBuffer') - await expect(request({ baseRef: 'main' })).rejects.toThrow( - 'baseRef must be a full git object id' - ) + await expect(request({ headOid: null, includePatch: false })).resolves.toEqual([ + { + kind: 'text', + originalContent: '', + modifiedContent: '', + originalIsBinary: false, + modifiedIsBinary: false + } + ]) + + expect(gitSpy).toHaveBeenCalledTimes(4) expect(gitBufferSpy).not.toHaveBeenCalled() }) it('uses the legacy path when patch or file-path conditions are not met', async () => { - const gitSpy = vi - .spyOn(handler as unknown as GitTarget, 'git') - .mockImplementation(async (args) => { - if (args[0] === 'rev-parse' && args.includes('HEAD')) { - return { stdout: `${HEAD_OID}\n`, stderr: '' } - } - if (args[0] === 'rev-parse' || args[0] === 'merge-base') { - return { stdout: `${BASE_OID}\n`, stderr: '' } - } - return { stdout: '', stderr: '' } - }) + const gitSpy = mockLegacyGit() const gitBufferSpy = vi.spyOn(handler as unknown as GitBufferTarget, 'gitBuffer') await Promise.all([ diff --git a/src/relay/git-handler.ts b/src/relay/git-handler.ts index c3105ea588f..3e8a20ea992 100644 --- a/src/relay/git-handler.ts +++ b/src/relay/git-handler.ts @@ -20,6 +20,7 @@ import { } from './git-handler-ops' import { branchDiffEntryAtPinnedOids, + isFullGitObjectId, parseOptionalBranchDiffHeadOid } from './git-handler-branch-diff-ops' import { @@ -1171,6 +1172,7 @@ export class GitHandler { () => { if ( headOid && + isFullGitObjectId(baseRef) && options.includePatch === true && typeof options.filePath === 'string' && options.filePath.length > 0