From f89a44319c3ed1870f7890042dc3fd9268ecec59 Mon Sep 17 00:00:00 2001 From: Justin Poehnelt Date: Tue, 18 Nov 2025 12:42:36 -0700 Subject: [PATCH] fix(extensions): resolve GitHub API 415 error for source tarballs - Conditionally set Accept header in downloadFromGitHubRelease. - Use application/vnd.github+json for source tarballs to correctly handle redirects. - Retain application/octet-stream for binary release assets. - Update downloadFile to support custom headers. - Add tests for GitHub release download headers, redirect handling, and custom headers --- .../cli/src/config/extensions/github.test.ts | 239 ++++++++++++++++++ packages/cli/src/config/extensions/github.ts | 46 +++- 2 files changed, 276 insertions(+), 9 deletions(-) diff --git a/packages/cli/src/config/extensions/github.test.ts b/packages/cli/src/config/extensions/github.test.ts index a2fb65996da..4709b8150ca 100644 --- a/packages/cli/src/config/extensions/github.test.ts +++ b/packages/cli/src/config/extensions/github.test.ts @@ -285,6 +285,131 @@ describe('github.ts', () => { expect(result.failureReason).toBe('failed to fetch release data'); } }); + + it('should use correct headers for release assets', async () => { + vi.mocked(fetchJson).mockResolvedValue({ + tag_name: 'v1.0.0', + assets: [{ name: 'asset.tar.gz', url: 'http://asset.url' }], + }); + vi.mocked(os.platform).mockReturnValue('linux'); + vi.mocked(os.arch).mockReturnValue('x64'); + + // Mock https.get and fs.createWriteStream for downloadFile + const mockReq = new EventEmitter(); + const mockRes = + new EventEmitter() as unknown as import('node:http').IncomingMessage; + Object.assign(mockRes, { statusCode: 200, pipe: vi.fn() }); + + vi.mocked(https.get).mockImplementation((url, options, cb) => { + if (typeof options === 'function') { + cb = options; + } + if (cb) cb(mockRes); + return mockReq as unknown as import('node:http').ClientRequest; + }); + + const mockStream = new EventEmitter() as unknown as fs.WriteStream; + Object.assign(mockStream, { close: vi.fn((cb) => cb && cb()) }); + vi.mocked(fs.createWriteStream).mockReturnValue(mockStream); + + // Mock fs.promises.readdir to return empty array (no cleanup needed) + vi.mocked(fs.promises.readdir).mockResolvedValue([]); + // Mock fs.promises.unlink + vi.mocked(fs.promises.unlink).mockResolvedValue(undefined); + + const promise = downloadFromGitHubRelease( + { + type: 'github-release', + source: 'owner/repo', + ref: 'v1.0.0', + } as unknown as ExtensionInstallMetadata, + '/dest', + { owner: 'owner', repo: 'repo' }, + ); + + // Wait for downloadFile to be called and stream to be created + await vi.waitUntil( + () => vi.mocked(fs.createWriteStream).mock.calls.length > 0, + ); + + // Trigger stream events to complete download + mockRes.emit('end'); + mockStream.emit('finish'); + + await promise; + + expect(https.get).toHaveBeenCalledWith( + 'http://asset.url', + expect.objectContaining({ + headers: expect.objectContaining({ + Accept: 'application/octet-stream', + }), + }), + expect.anything(), + ); + }); + + it('should use correct headers for source tarballs', async () => { + vi.mocked(fetchJson).mockResolvedValue({ + tag_name: 'v1.0.0', + assets: [], + tarball_url: 'http://tarball.url', + }); + + // Mock https.get and fs.createWriteStream for downloadFile + const mockReq = new EventEmitter(); + const mockRes = + new EventEmitter() as unknown as import('node:http').IncomingMessage; + Object.assign(mockRes, { statusCode: 200, pipe: vi.fn() }); + + vi.mocked(https.get).mockImplementation((url, options, cb) => { + if (typeof options === 'function') { + cb = options; + } + if (cb) cb(mockRes); + return mockReq as unknown as import('node:http').ClientRequest; + }); + + const mockStream = new EventEmitter() as unknown as fs.WriteStream; + Object.assign(mockStream, { close: vi.fn((cb) => cb && cb()) }); + vi.mocked(fs.createWriteStream).mockReturnValue(mockStream); + + // Mock fs.promises.readdir to return empty array + vi.mocked(fs.promises.readdir).mockResolvedValue([]); + // Mock fs.promises.unlink + vi.mocked(fs.promises.unlink).mockResolvedValue(undefined); + + const promise = downloadFromGitHubRelease( + { + type: 'github-release', + source: 'owner/repo', + ref: 'v1.0.0', + } as unknown as ExtensionInstallMetadata, + '/dest', + { owner: 'owner', repo: 'repo' }, + ); + + // Wait for downloadFile to be called and stream to be created + await vi.waitUntil( + () => vi.mocked(fs.createWriteStream).mock.calls.length > 0, + ); + + // Trigger stream events to complete download + mockRes.emit('end'); + mockStream.emit('finish'); + + await promise; + + expect(https.get).toHaveBeenCalledWith( + 'http://tarball.url', + expect.objectContaining({ + headers: expect.objectContaining({ + Accept: 'application/vnd.github+json', + }), + }), + expect.anything(), + ); + }); }); describe('findReleaseAsset', () => { @@ -349,6 +474,120 @@ describe('github.ts', () => { 'Request failed with status code 404', ); }); + + it('should follow redirects', async () => { + const mockReq = new EventEmitter(); + const mockResRedirect = + new EventEmitter() as unknown as import('node:http').IncomingMessage; + Object.assign(mockResRedirect, { + statusCode: 302, + headers: { location: 'new-url' }, + }); + + const mockResSuccess = + new EventEmitter() as unknown as import('node:http').IncomingMessage; + Object.assign(mockResSuccess, { statusCode: 200, pipe: vi.fn() }); + + vi.mocked(https.get) + .mockImplementationOnce((url, options, cb) => { + if (typeof options === 'function') cb = options; + if (cb) cb(mockResRedirect); + return mockReq as unknown as import('node:http').ClientRequest; + }) + .mockImplementationOnce((url, options, cb) => { + if (typeof options === 'function') cb = options; + if (cb) cb(mockResSuccess); + return mockReq as unknown as import('node:http').ClientRequest; + }); + + const mockStream = new EventEmitter() as unknown as fs.WriteStream; + Object.assign(mockStream, { close: vi.fn((cb) => cb && cb()) }); + vi.mocked(fs.createWriteStream).mockReturnValue(mockStream); + + const promise = downloadFile('url', '/dest'); + mockResSuccess.emit('end'); + mockStream.emit('finish'); + + await expect(promise).resolves.toBeUndefined(); + expect(https.get).toHaveBeenCalledTimes(2); + expect(https.get).toHaveBeenLastCalledWith( + 'new-url', + expect.anything(), + expect.anything(), + ); + }); + + it('should fail after too many redirects', async () => { + const mockReq = new EventEmitter(); + const mockResRedirect = + new EventEmitter() as unknown as import('node:http').IncomingMessage; + Object.assign(mockResRedirect, { + statusCode: 302, + headers: { location: 'new-url' }, + }); + + vi.mocked(https.get).mockImplementation((url, options, cb) => { + if (typeof options === 'function') cb = options; + if (cb) cb(mockResRedirect); + return mockReq as unknown as import('node:http').ClientRequest; + }); + + await expect(downloadFile('url', '/dest')).rejects.toThrow( + 'Too many redirects', + ); + }, 10000); // Increase timeout for this test if needed, though with mocks it should be fast + + it('should fail if redirect location is missing', async () => { + const mockReq = new EventEmitter(); + const mockResRedirect = + new EventEmitter() as unknown as import('node:http').IncomingMessage; + Object.assign(mockResRedirect, { + statusCode: 302, + headers: {}, // No location + }); + + vi.mocked(https.get).mockImplementation((url, options, cb) => { + if (typeof options === 'function') cb = options; + if (cb) cb(mockResRedirect); + return mockReq as unknown as import('node:http').ClientRequest; + }); + + await expect(downloadFile('url', '/dest')).rejects.toThrow( + 'Redirect response missing Location header', + ); + }); + + it('should pass custom headers', async () => { + const mockReq = new EventEmitter(); + const mockRes = + new EventEmitter() as unknown as import('node:http').IncomingMessage; + Object.assign(mockRes, { statusCode: 200, pipe: vi.fn() }); + + vi.mocked(https.get).mockImplementation((url, options, cb) => { + if (typeof options === 'function') cb = options; + if (cb) cb(mockRes); + return mockReq as unknown as import('node:http').ClientRequest; + }); + + const mockStream = new EventEmitter() as unknown as fs.WriteStream; + Object.assign(mockStream, { close: vi.fn((cb) => cb && cb()) }); + vi.mocked(fs.createWriteStream).mockReturnValue(mockStream); + + const promise = downloadFile('url', '/dest', { + headers: { 'X-Custom': 'value' }, + }); + mockRes.emit('end'); + mockStream.emit('finish'); + + await expect(promise).resolves.toBeUndefined(); + expect(https.get).toHaveBeenCalledWith( + 'url', + expect.objectContaining({ + headers: expect.objectContaining({ 'X-Custom': 'value' }), + }), + expect.anything(), + ); + }); }); describe('extractFile', () => { diff --git a/packages/cli/src/config/extensions/github.ts b/packages/cli/src/config/extensions/github.ts index f2a6bb677f0..37a974bf8a8 100644 --- a/packages/cli/src/config/extensions/github.ts +++ b/packages/cli/src/config/extensions/github.ts @@ -355,7 +355,17 @@ export async function downloadFromGitHubRelease( } try { - await downloadFile(archiveUrl, downloadedAssetPath); + // GitHub API requires different Accept headers for different types of downloads: + // 1. Binary Assets (e.g. release artifacts): Require 'application/octet-stream' to return the raw content. + // 2. Source Tarballs (e.g. /tarball/{ref}): Require 'application/vnd.github+json' (or similar) to return + // a 302 Redirect to the actual download location (codeload.github.com). + // Sending 'application/octet-stream' for tarballs results in a 415 Unsupported Media Type error. + const headers = { + ...(asset + ? { Accept: 'application/octet-stream' } + : { Accept: 'application/vnd.github+json' }), + }; + await downloadFile(archiveUrl, downloadedAssetPath, { headers }); } catch (error) { return { failureReason: 'failed to download asset', @@ -472,24 +482,42 @@ export function findReleaseAsset(assets: Asset[]): Asset | undefined { return undefined; } -export async function downloadFile(url: string, dest: string): Promise { - const headers: { - 'User-agent': string; - Accept: string; - Authorization?: string; - } = { +export interface DownloadOptions { + headers?: Record; +} + +export async function downloadFile( + url: string, + dest: string, + options?: DownloadOptions, + redirectCount: number = 0, +): Promise { + const headers: Record = { 'User-agent': 'gemini-cli', Accept: 'application/octet-stream', + ...options?.headers, }; const token = getGitHubToken(); if (token) { - headers.Authorization = `token ${token}`; + headers['Authorization'] = `token ${token}`; } + return new Promise((resolve, reject) => { https .get(url, { headers }, (res) => { if (res.statusCode === 302 || res.statusCode === 301) { - downloadFile(res.headers.location!, dest).then(resolve).catch(reject); + if (redirectCount >= 10) { + return reject(new Error('Too many redirects')); + } + + if (!res.headers.location) { + return reject( + new Error('Redirect response missing Location header'), + ); + } + downloadFile(res.headers.location, dest, options, redirectCount + 1) + .then(resolve) + .catch(reject); return; } if (res.statusCode !== 200) {