From d305e48547c78a6b6d4e7c4e16f89eff90c1c28f Mon Sep 17 00:00:00 2001 From: Drx <116107889+afifhaziq@users.noreply.github.com> Date: Mon, 10 Aug 2026 16:10:12 +0800 Subject: [PATCH] fix(gitlab): guard against non-array API responses in MR/issue listing (#12911) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(gitlab): guard against non-array API responses in MR/issue listing fetchIssuesAsWorkItems and listMergeRequests parsed glab's JSON output and called .map straight on it. When the GitLab API returns a JSON object instead of an array (error body, unexpected shape) on a successful exit, this crashed with a bare TypeError that got misclassified as "Failed to load issues: JSON.parse(...).map is not a function" instead of a useful message. Co-Authored-By: Claude Sonnet 5 * fix(gitlab): cover listIssues and keep payloads out of error classification The guard missed listIssues in issues.ts — the RPC-backed issue list that produces the reported "Failed to load issues: JSON.parse(...).map is not a function". Hoist the guard into glab-api-response.ts so both files share it. The thrown message is fed to classifyGlabError, which substring-matches it. A response payload is content, not a diagnostic: an MR titled "fix network timeout" classified as network_error and the canned copy replaced the payload the user needed. Report a GitLab error envelope by its own message, and mark an opaque body so classification is skipped. * test(gitlab): make the list-guard tests fail on the regressions they name Two assertions were vacuous under mutation. The envelope test used a "403 Forbidden" message whose keyword matches earlier in the classifier chain than its sibling payload, so leaking the payload into classification still passed; it now uses a 404 envelope beside a "403 forbidden" sibling. No call-site test carried a classifier keyword, so deleting the marker-error branch entirely failed only one unit test; the MR API path now uses a keyword-bearing body. Also give the non-list branch the same "Failed to load issues" prefix as every other list error, cover the `{ error }` envelope field, and pin the thrown type. * test(gitlab): pin the reported-payload bound Removing the 300-char slice survived the whole suite, and the banner's break-words now depends on it. Name the limit and assert both branches truncate, plus the envelope falling through a blank message to `error`. * test(gitlab): pin message-over-error envelope precedence Swapping the lookup order passed the whole suite. Anchor the bound regex too so it cannot match an incidental ": " near the end of a message. --------- Co-authored-by: Claude Sonnet 5 Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> --- src/main/gitlab/client-mr.test.ts | 51 +++++++++++++++ src/main/gitlab/client-work-items.test.ts | 12 ++++ src/main/gitlab/client.ts | 17 +++-- src/main/gitlab/gl-utils.test.ts | 66 ++++++++++++++++++++ src/main/gitlab/gl-utils.ts | 3 +- src/main/gitlab/glab-api-response.ts | 42 +++++++++++++ src/main/gitlab/glab-error-classification.ts | 14 ++++- src/main/gitlab/issues.test.ts | 26 ++++++++ src/main/gitlab/issues.ts | 7 +-- 9 files changed, 223 insertions(+), 15 deletions(-) diff --git a/src/main/gitlab/client-mr.test.ts b/src/main/gitlab/client-mr.test.ts index 089de41d877..e82b82d3360 100644 --- a/src/main/gitlab/client-mr.test.ts +++ b/src/main/gitlab/client-mr.test.ts @@ -911,6 +911,57 @@ describe('gitlab client — MR operations', () => { expect(result.error?.type).toBe('permission_denied') expect(result.items).toEqual([]) }) + + // Why: the title carries a classifier keyword, so this also pins that a wrapped payload stays + // out of the substring matcher — classifying it would swap the body for "check your connection". + it('reports the body instead of ".map is not a function" when the API returns a non-array', async () => { + glabApiWithHeadersMock.mockResolvedValueOnce({ + body: JSON.stringify({ data: [{ iid: 7, title: 'fix network timeout' }] }), + headers: {} + }) + const result = await listMergeRequests('/repo', 'opened') + expect(result.items).toEqual([]) + expect(result.error?.type).toBe('unknown') + expect(result.error?.message).toContain('fix network timeout') + expect(result.error?.message).not.toContain('is not a function') + }) + + it('reports the body instead of ".map is not a function" when the cwd fallback returns a non-array', async () => { + resolveIssueSourceMock.mockResolvedValueOnce({ source: null, fellBack: false }) + glabExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify({ data: [], total: 0 }) + }) + const result = await listMergeRequests('/repo', 'opened') + expect(result.items).toEqual([]) + expect(result.error?.message).toContain('{"data":[],"total":0}') + expect(result.error?.message).not.toContain('is not a function') + }) + + // Why: the whole point of surfacing the body — a GitLab error envelope now + // classifies like any other glab failure instead of collapsing to 'unknown'. + it('classifies a GitLab error envelope returned on exit 0', async () => { + glabApiWithHeadersMock.mockResolvedValueOnce({ + body: JSON.stringify({ message: '403 Forbidden' }), + headers: {} + }) + const result = await listMergeRequests('/repo', 'opened') + expect(result.items).toEqual([]) + expect(result.error?.type).toBe('permission_denied') + }) + + // Why: the sibling title matches an earlier classifier branch than the envelope does, so this + // fails if the payload leaks into classification instead of only the envelope's own message. + it('classifies an error envelope by its message, not its sibling payload', async () => { + glabApiWithHeadersMock.mockResolvedValueOnce({ + body: JSON.stringify({ + message: '404 Project Not Found', + data: [{ iid: 7, title: '403 forbidden in CI' }] + }), + headers: {} + }) + const result = await listMergeRequests('/repo', 'opened') + expect(result.error?.type).toBe('not_found') + }) }) describe('updateMR', () => { diff --git a/src/main/gitlab/client-work-items.test.ts b/src/main/gitlab/client-work-items.test.ts index 5de8522d4e7..80e412862d8 100644 --- a/src/main/gitlab/client-work-items.test.ts +++ b/src/main/gitlab/client-work-items.test.ts @@ -191,4 +191,16 @@ describe('gitlab client — combined listWorkItems', () => { expect(result.items[0].title).toBe('live issue') expect(result.error).toBeDefined() }) + + it('reports the body instead of ".map is not a function" when the issues fetch returns a non-array', async () => { + glabApiWithHeadersMock.mockResolvedValueOnce({ body: '[]', headers: {} }) + glabExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify({ data: [], total: 0 }) + }) + + const result = await listWorkItems('/repo', 'opened', 1, 20) + expect(result.items).toEqual([]) + expect(result.error?.message).toContain('{"data":[],"total":0}') + expect(result.error?.message).not.toContain('is not a function') + }) }) diff --git a/src/main/gitlab/client.ts b/src/main/gitlab/client.ts index 2e25ebb6dec..138b2cac93d 100644 --- a/src/main/gitlab/client.ts +++ b/src/main/gitlab/client.ts @@ -26,7 +26,7 @@ import { acquire, classifyGlabError, classifyJobLogError, - classifyListIssuesError, + classifyListFetchError, isMissingJobLogError, getGlabKnownHosts, getProjectRef, @@ -36,6 +36,7 @@ import { glabApiWithHeaders, glabExecFileAsync, parseGlabAuthStatusHosts, + parseGlabJsonList, release, resolveIssueSource, type LocalGitExecOptions, @@ -468,7 +469,7 @@ export async function listMergeRequests( ], glabRepoExecOptions(repoPath, connectionId, localGitOptions) ) - const data = JSON.parse(stdout) as Parameters[0][] + const data = parseGlabJsonList[0]>(stdout) return { items: data.map((d) => mapMRToWorkItem(d, 'unknown')), page, @@ -478,14 +479,13 @@ export async function listMergeRequests( totalPages: data.length < perPage ? page : page + 1 } } catch (err) { - const stderr = err instanceof Error ? err.message : String(err) return { items: [], page, perPage, totalCount: 0, totalPages: 0, - error: classifyListIssuesError(stderr) + error: classifyListFetchError(err) } } finally { release() @@ -505,7 +505,7 @@ export async function listMergeRequests( [...glabHostnameArgs(projectRef, connectionId), path], glabRepoExecOptions(repoPath, connectionId, localGitOptions) ) - const data = JSON.parse(body) as Parameters[0][] + const data = parseGlabJsonList[0]>(body) return { items: data.map((d) => mapMRToWorkItem(d, repoId, projectRef)), page, @@ -517,14 +517,13 @@ export async function listMergeRequests( Math.max(1, Math.ceil(parseHeaderInt(headers['x-total'], 0) / perPage)) } } catch (err) { - const stderr = err instanceof Error ? err.message : String(err) return { items: [], page, perPage, totalCount: 0, totalPages: 0, - error: classifyListIssuesError(stderr) + error: classifyListFetchError(err) } } finally { release() @@ -688,7 +687,7 @@ export async function fetchIssuesAsWorkItems( ], glabRepoExecOptions(repoPath, connectionId, localGitOptions) ) - const data = JSON.parse(stdout) as Parameters[0][] + const data = parseGlabJsonList[0]>(stdout) return { items: data.map((d) => mapIssueToWorkItem(d, projectRef.path, projectRef)), error: undefined @@ -696,7 +695,7 @@ export async function fetchIssuesAsWorkItems( } catch (err) { return { items: [], - error: classifyListIssuesError(err instanceof Error ? err.message : String(err)) + error: classifyListFetchError(err) } } finally { release() diff --git a/src/main/gitlab/gl-utils.test.ts b/src/main/gitlab/gl-utils.test.ts index c2396c9aa7d..4ffd0d11ed5 100644 --- a/src/main/gitlab/gl-utils.test.ts +++ b/src/main/gitlab/gl-utils.test.ts @@ -17,8 +17,10 @@ import { _resetProjectRefCache, classifyGlabError, classifyJobLogError, + classifyListFetchError, classifyListIssuesError, getIssueProjectRef, + parseGlabJsonList, isMissingJobLogError, getGlabKnownHosts, getProjectRef, @@ -27,6 +29,7 @@ import { parseGlabAuthStatusHosts, resolveIssueSource } from './gl-utils' +import { GlabNonListResponseError } from './glab-api-response' import { rememberGlabKnownHost, rememberGlabKnownHosts } from './gitlab-known-host-probe' import { registerSshGitProvider, unregisterSshGitProvider } from '../providers/ssh-git-dispatch' import { REMOTE_URL_PROBE_TIMEOUT_MS } from '../git/remote-url-probe' @@ -501,6 +504,69 @@ gitlab.example.com:8080: }) }) +describe('parseGlabJsonList', () => { + it('returns the parsed list unchanged', () => { + expect(parseGlabJsonList<{ iid: number }>('[{"iid":1}]')).toEqual([{ iid: 1 }]) + }) + + it.each([ + ['null', 'null'], + ['a number', '0'], + ['a string', '"nope"'], + ['an object', '{"data":[]}'] + ])('reports the raw payload for %s as an unclassifiable body', (_label, payload) => { + expect(() => parseGlabJsonList(payload)).toThrow(GlabNonListResponseError) + expect(() => parseGlabJsonList(payload)).toThrow(payload) + }) + + // Why: glab allows a 10MB body, and the renderer's error banner has no length guard of its own. + it.each([ + ['an opaque body', `{"data":"${'x'.repeat(50_000)}"}`], + ['an error envelope', `{"message":"${'x'.repeat(50_000)}"}`] + ])('bounds the reported payload for %s', (_label, payload) => { + expect(() => parseGlabJsonList(payload)).toThrow( + /^GitLab returned (?:a non-list response|an error): .{300}$/ + ) + }) + + it.each([ + ['message', '{"message":"403 Forbidden"}', '403 Forbidden'], + ['error', '{"error":"insufficient_scope"}', 'insufficient_scope'], + ['error when message is blank', '{"message":" ","error":"real_error"}', 'real_error'], + // Why: GitLab sends both on some endpoints; `message` is the human-facing one. + [ + 'message when both are set', + '{"message":"404 Project Not Found","error":"insufficient_scope"}', + '404 Project Not Found' + ] + ])('reports a GitLab error envelope by its %s field', (_label, payload, reported) => { + // Why: an envelope is GitLab's own diagnostic, so it stays classifiable — unlike a raw body. + expect(() => parseGlabJsonList(payload)).toThrow(`GitLab returned an error: ${reported}`) + expect(() => parseGlabJsonList(payload)).not.toThrow(GlabNonListResponseError) + }) +}) + +describe('classifyListFetchError', () => { + it('keeps opaque payload text away from the classifier', () => { + // Why: the title would otherwise substring-match as a network failure and replace the payload. + const payload = '{"data":[{"title":"fix network timeout"}]}' + let thrown: unknown + try { + parseGlabJsonList(payload) + } catch (err) { + thrown = err + } + expect(thrown).toBeInstanceOf(GlabNonListResponseError) + const classified = classifyListFetchError(thrown) + expect(classified.type).toBe('unknown') + expect(classified.message).toContain('fix network timeout') + }) + + it('still classifies ordinary glab failures by their stderr', () => { + expect(classifyListFetchError(new Error('HTTP 403 Forbidden')).type).toBe('permission_denied') + }) +}) + describe('parseGlabApiResponse', () => { it('splits headers and body at the first blank line (LF)', () => { const stdout = 'HTTP/2.0 200 OK\nX-Total: 42\nX-Total-Pages: 3\n\n[{"iid":1}]' diff --git a/src/main/gitlab/gl-utils.ts b/src/main/gitlab/gl-utils.ts index 6531793dac6..0d361356692 100644 --- a/src/main/gitlab/gl-utils.ts +++ b/src/main/gitlab/gl-utils.ts @@ -5,6 +5,7 @@ export { glabExecFileAsync, gitExecFileAsync } export { classifyGlabError, classifyJobLogError, + classifyListFetchError, classifyListIssuesError, isMissingJobLogError } from './glab-error-classification' @@ -28,7 +29,7 @@ export type { ProjectRef, ResolvedIssueSource } from './gitlab-project-ref-resolution' -export { parseGlabApiResponse, type GlabApiResponse } from './glab-api-response' +export { parseGlabApiResponse, parseGlabJsonList, type GlabApiResponse } from './glab-api-response' const MAX_CONCURRENT = 4 let running = 0 diff --git a/src/main/gitlab/glab-api-response.ts b/src/main/gitlab/glab-api-response.ts index a8ea6380769..fc7c946aad1 100644 --- a/src/main/gitlab/glab-api-response.ts +++ b/src/main/gitlab/glab-api-response.ts @@ -25,6 +25,48 @@ export function parseGlabApiResponse(stdout: string): GlabApiResponse { return { body, headers } } +/** A non-list body carrying no GitLab error text — opaque data, so there is nothing to classify. */ +export class GlabNonListResponseError extends Error {} + +// Why: glab allows a 10MB body; this is what keeps a proxy's whole response out of the error banner. +const REPORTED_PAYLOAD_LIMIT = 300 + +/** + * Parse a glab list response, failing readably when GitLab answers with a JSON object. + * + * Why: glab exits 0 on error envelopes and proxy wrappers, so `JSON.parse(...).map` threw an + * opaque `.map is not a function` that the caller's classifier could only report as "unknown". + */ +export function parseGlabJsonList(payload: string): T[] { + const parsed: unknown = JSON.parse(payload) + if (Array.isArray(parsed)) { + return parsed as T[] + } + const reported = gitlabErrorText(parsed) + if (reported) { + throw new Error(`GitLab returned an error: ${reported}`) + } + // Why: slice the raw payload rather than re-serializing `parsed` — same text, without + // stringifying a multi-megabyte body just to keep the preview. + throw new GlabNonListResponseError( + `GitLab returned a non-list response: ${payload.trim().slice(0, REPORTED_PAYLOAD_LIMIT)}` + ) +} + +/** GitLab reports API failures as `{ message }` or `{ error }`; anything else is opaque data. */ +function gitlabErrorText(parsed: unknown): string | null { + if (typeof parsed !== 'object' || parsed === null) { + return null + } + const { message, error } = parsed as { message?: unknown; error?: unknown } + for (const value of [message, error]) { + if (typeof value === 'string' && value.trim()) { + return value.trim().slice(0, REPORTED_PAYLOAD_LIMIT) + } + } + return null +} + function findHeaderBodySeparator(stdout: string): { index: number; bodyStart: number } | null { let lineStart = 0 for (let index = 0; index < stdout.length; index++) { diff --git a/src/main/gitlab/glab-error-classification.ts b/src/main/gitlab/glab-error-classification.ts index d769bdd108c..75af66ca4ea 100644 --- a/src/main/gitlab/glab-error-classification.ts +++ b/src/main/gitlab/glab-error-classification.ts @@ -1,4 +1,5 @@ import type { ClassifiedError } from '../../shared/types' +import { GlabNonListResponseError } from './glab-api-response' // Why: glab CLI surfaces API errors as unstructured stderr. Map known // patterns to typed errors so callers can show user-friendly messages. @@ -33,6 +34,8 @@ export function classifyGlabError(stderr: string): ClassifiedError { return { type: 'unknown', message: `Failed to update issue: ${stderr.trim()}` } } +const LIST_READ_FAILURE = 'Failed to load issues' + // Why: classifyGlabError's copy is phrased for edit/update operations; list // issues is a read op, so rewrite messages for read-context banners. export function classifyListIssuesError(stderr: string): ClassifiedError { @@ -46,11 +49,20 @@ export function classifyListIssuesError(stderr: string): ClassifiedError { validation_error: `Invalid request — ${trimmed}`, rate_limited: 'GitLab rate limit hit. Try again in a few minutes.', network_error: 'Network error — check your connection.', - unknown: `Failed to load issues: ${trimmed}` + unknown: `${LIST_READ_FAILURE}: ${trimmed}` } return { type: c.type, message: readMessages[c.type] } } +// Why: an opaque response body is content, not a diagnostic — substring-matching it would render +// an MR titled "fix network timeout" as "check your connection" and discard the body. +export function classifyListFetchError(err: unknown): ClassifiedError { + if (err instanceof GlabNonListResponseError) { + return { type: 'unknown', message: `${LIST_READ_FAILURE}: ${err.message}` } + } + return classifyListIssuesError(err instanceof Error ? err.message : String(err)) +} + // Why: a job trace is a read on a pipeline job, so classifyGlabError's issue-edit // copy ("permission to edit this issue") would land verbatim on a Checks row. export function classifyJobLogError(stderr: string): ClassifiedError { diff --git a/src/main/gitlab/issues.test.ts b/src/main/gitlab/issues.test.ts index cde9d4f9e01..221041356ec 100644 --- a/src/main/gitlab/issues.test.ts +++ b/src/main/gitlab/issues.test.ts @@ -186,6 +186,32 @@ describe('gitlab issue operations', () => { expect(result.error?.type).toBe('permission_denied') }) + it('reports the body instead of ".map is not a function" when the API returns a non-array', async () => { + getIssueProjectRefMock.mockResolvedValueOnce({ host: 'gitlab.com', path: 'stablyai/orca' }) + glabExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify({ data: [], total: 0 }) + }) + + const result = await listIssues('/repo-root', 5) + + expect(result.items).toEqual([]) + expect(result.error?.type).toBe('unknown') + expect(result.error?.message).toContain('{"data":[],"total":0}') + expect(result.error?.message).not.toContain('is not a function') + }) + + it('reports a GitLab error envelope by its own message', async () => { + getIssueProjectRefMock.mockResolvedValueOnce({ host: 'gitlab.com', path: 'stablyai/orca' }) + glabExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify({ message: '403 Forbidden' }) + }) + + const result = await listIssues('/repo-root', 5) + + expect(result.items).toEqual([]) + expect(result.error?.type).toBe('permission_denied') + }) + it('returns an isolated not_found error (never a cwd-inferred glab call) when the project is unresolved', async () => { // Why: a cwd-inferred `glab issue list` would hit `git: exit status 128` // on an SSH connection and, in an "All projects" aggregate, sink the diff --git a/src/main/gitlab/issues.ts b/src/main/gitlab/issues.ts index 970f9ff1f03..8635805337d 100644 --- a/src/main/gitlab/issues.ts +++ b/src/main/gitlab/issues.ts @@ -13,7 +13,7 @@ import type { } from '../../shared/types' import { mapGitLabIssueInfo } from './mappers' // prettier-ignore -import { glabExecFileAsync, acquire, release, getIssueProjectRef, resolveIssueSource, classifyGlabError, classifyListIssuesError, getGlabKnownHosts, glabRepoExecOptions, glabHostnameArgs, type LocalGitExecOptions, type ProjectRef } from './gl-utils' +import { glabExecFileAsync, acquire, release, getIssueProjectRef, resolveIssueSource, classifyGlabError, classifyListFetchError, getGlabKnownHosts, glabRepoExecOptions, glabHostnameArgs, parseGlabJsonList, type LocalGitExecOptions, type ProjectRef } from './gl-utils' // Why: parallel to GitHub's IssueListResult — distinguishes a successful- // empty listing from a failed fetch. @@ -128,7 +128,7 @@ export async function listIssues( ], glabRepoExecOptions(repoPath, connectionId, localGitOptions) ) - const data = JSON.parse(stdout) as Record[] + const data = parseGlabJsonList>(stdout) // Why: GitLab's project issues endpoint returns true issues only // (MRs are a separate endpoint), so no equivalent of GitHub's // pull_request filter is needed here. @@ -136,10 +136,9 @@ export async function listIssues( items: data.map((d) => mapGitLabIssueInfo(d as Parameters[0])) } } catch (err) { - const stderr = err instanceof Error ? err.message : String(err) return { items: [], - error: classifyListIssuesError(stderr) + error: classifyListFetchError(err) } } finally { release()