diff --git a/changelog.d/fixes/12958-gitlab-duo-403-entitlement-fallback.md b/changelog.d/fixes/12958-gitlab-duo-403-entitlement-fallback.md new file mode 100644 index 0000000000..5c512c556b --- /dev/null +++ b/changelog.d/fixes/12958-gitlab-duo-403-entitlement-fallback.md @@ -0,0 +1 @@ +- **fix(providers):** GitLab Duo Retest and chat requests now fall back to the public Code Suggestions endpoint for ANY `direct_access` 403 (not only the "direct connections are disabled" tenant-config message), and surface the real upstream error body instead of a generic "Access denied" when both endpoints reject the token (#12958) — thanks @Rahulsharma0810 diff --git a/open-sse/executors/gitlab.ts b/open-sse/executors/gitlab.ts index 1d985a16da..a7840323f2 100644 --- a/open-sse/executors/gitlab.ts +++ b/open-sse/executors/gitlab.ts @@ -599,12 +599,17 @@ export class GitlabExecutor extends BaseExecutor { }; } - if (response.status === 403 && !isGitLabDirectAccessDisabled(response.status, bodyText)) { - return { - target: null, - credentials, - errorResponse: toOpenAIError(403, "GitLab Duo direct access scope is unavailable"), - }; + // #12958: any direct_access 403 (not only GitLab's exact "direct connections + // are disabled" tenant-config message) is recoverable via the public + // completions fallback — mirrors the 401 branch above and the connection-test + // path's shouldFallbackToPublicCodeSuggestions() contract. + if (response.status === 403 && input.log) { + input.log.warn( + "GITLAB-DUO", + isGitLabDirectAccessDisabled(response.status, bodyText) + ? "direct_access exchange rejected (403, direct connections disabled); falling back to public completions endpoint" + : `direct_access exchange rejected (403); falling back to public completions endpoint. Body: ${bodyText.slice(0, 500)}` + ); } return { diff --git a/src/app/api/providers/[id]/test/route.ts b/src/app/api/providers/[id]/test/route.ts index e83ce61e09..7d72f28098 100644 --- a/src/app/api/providers/[id]/test/route.ts +++ b/src/app/api/providers/[id]/test/route.ts @@ -239,6 +239,16 @@ function isTokenExpired(connection: any) { return expiresAt <= Date.now() + buffer; } +// #12958: GitLab's own `direct_access` 403 JSON body (e.g. `{"error":"insufficient_scope"}`) +// is safe operator-facing diagnostic text — it is not a stack trace and does not echo the +// token — but is capped and stripped of control characters defensively before it reaches +// the stored/surfaced error message, per docs/security/ERROR_SANITIZATION.md. +function sanitizeUpstreamBodyText(bodyText: string): string { + const collapsed = bodyText.replace(/[\r\n\t-]+/g, " ").trim(); + const MAX_LENGTH = 300; + return collapsed.length > MAX_LENGTH ? `${collapsed.slice(0, MAX_LENGTH)}…` : collapsed; +} + /** * #10365 / #10499: the real chat path (open-sse/executors/gitlab.ts) treats a rejected * `direct_access` exchange (401) or an explicitly disabled direct-connections tenant @@ -644,9 +654,14 @@ export async function testOAuthConnection( }; } + // #12958: `res.text()` can only be read once — capture it here in the outer + // function scope so the generic bodyText selection below (which used to call + // `res.text()` a second time and silently get "" back, discarding the real + // GitLab error) can reuse the same string instead of re-reading a drained body. + let gitlabDuoDirectAccessBodyText: string | null = null; if (connection.provider === "gitlab-duo") { - const gitlabText = await res.text(); - if (shouldFallbackToPublicCodeSuggestions(res.status, gitlabText)) { + gitlabDuoDirectAccessBodyText = await res.text(); + if (shouldFallbackToPublicCodeSuggestions(res.status, gitlabDuoDirectAccessBodyText)) { const fallbackOk = await probeGitLabDuoPublicFallback(connection, accessToken, timeoutMs); if (fallbackOk) { return { @@ -788,14 +803,26 @@ export async function testOAuthConnection( // revoked token. (The body is unread here for non-gitlab providers; the guard keeps // it safe if it was already consumed.) antigravity/agy read any failure body so a // geo-blocked egress location is labeled with an actionable message instead of a - // generic "API returned 400". + // generic "API returned 400". gitlab-duo already consumed the body above (`res.text()` + // is single-read) — reuse it instead of re-reading a drained stream (#12958). const bodyText = - res.status === 401 || - res.status === 403 || - connection.provider === "antigravity" || - connection.provider === "agy" - ? await res.text().catch(() => "") - : ""; + connection.provider === "gitlab-duo" + ? (gitlabDuoDirectAccessBodyText ?? "") + : res.status === 401 || + res.status === 403 || + connection.provider === "antigravity" || + connection.provider === "agy" + ? await res.text().catch(() => "") + : ""; + // #12958: surface the real upstream body for a gitlab-duo 403 that also fails the + // public-fallback probe, instead of a generic "Access denied" — the operator needs + // to tell an entitlement/scope failure apart from an instance-config or revoked-token + // one. Trimmed/truncated per docs/security/ERROR_SANITIZATION.md (no stack traces are + // involved; this is GitLab's own JSON error body, capped defensively). + const gitlabDuoAccessDeniedMessage = + connection.provider === "gitlab-duo" && res.status === 403 + ? `Access denied: ${sanitizeUpstreamBodyText(bodyText)}` + : "Access denied"; const error = isGeoBlockedError(bodyText) ? "Egress location blocked by Google (User location is not supported). The Cloud Code API is not offered from this server's proxy exit region — route antigravity/agy through a proxy in a supported region (e.g. US/EU) or use a different provider. This is NOT an account problem." : isAccountDeactivatedMessage(bodyText) @@ -803,7 +830,7 @@ export async function testOAuthConnection( : res.status === 401 ? "Token invalid or revoked" : res.status === 403 - ? "Access denied" + ? gitlabDuoAccessDeniedMessage : `API returned ${res.status}`; return { diff --git a/src/lib/oauth/gitlab.ts b/src/lib/oauth/gitlab.ts index e9bf961a10..8fa9c2d2c5 100644 --- a/src/lib/oauth/gitlab.ts +++ b/src/lib/oauth/gitlab.ts @@ -103,16 +103,19 @@ export function isGitLabDirectAccessDisabled(status: number, bodyText: string): } /** - * #10365 / #10499: same predicate the chat-path executor (open-sse/executors/gitlab.ts) - * uses to decide whether a failed `direct_access` exchange should fall back to the - * public Code Suggestions completions endpoint instead of surfacing a hard error. - * A rejected exchange (401 — invalid/expired direct_access grant) or an explicitly - * disabled direct-connections tenant (403 with the GitLab-specific message) both mean - * "direct mode unavailable, but the public monolith endpoint may still work" — never a - * definitive "the token itself is bad" signal on their own. + * #10365 / #10499 / #12958: same predicate the chat-path executor + * (open-sse/executors/gitlab.ts) uses to decide whether a failed `direct_access` + * exchange should fall back to the public Code Suggestions completions endpoint + * instead of surfacing a hard error. A rejected exchange (401 — invalid/expired + * direct_access grant) or ANY 403 (an explicitly disabled direct-connections tenant, + * or an entitlement/scope-resolution failure GitLab does not document a distinct + * status for — #12958) both mean "direct mode unavailable, but the public monolith + * endpoint may still work" — never a definitive "the token itself is bad" signal on + * their own. `isGitLabDirectAccessDisabled()` stays available for log/diagnostic + * labeling; it no longer gates this decision. */ -export function shouldFallbackToPublicCodeSuggestions(status: number, bodyText: string): boolean { - return status === 401 || isGitLabDirectAccessDisabled(status, bodyText); +export function shouldFallbackToPublicCodeSuggestions(status: number, _bodyText: string): boolean { + return status === 401 || status === 403; } /** Headers for a public Code Suggestions completions probe (chat path and connection test). */ diff --git a/tests/unit/executor-gitlab.test.ts b/tests/unit/executor-gitlab.test.ts index 8bc41efb1f..5f98bbb1b5 100644 --- a/tests/unit/executor-gitlab.test.ts +++ b/tests/unit/executor-gitlab.test.ts @@ -320,3 +320,55 @@ test("GitlabExecutor falls back to the public Code Suggestions endpoint when dir globalThis.fetch = originalFetch; } }); + +// #12958: an entitlement/scope-resolution 403 (NOT the "direct connections are +// disabled" tenant-config message) must ALSO fall back to the public Code Suggestions +// completions endpoint — previously only that exact message recovered; any other 403 +// hard-failed the request even when the same token was accepted by the public endpoint. +test("GitlabExecutor falls back to the public Code Suggestions endpoint on an entitlement-flavored 403 (#12958)", async () => { + const executor = (await getExecutor("gitlab-duo")) as GitlabExecutor; + const originalFetch = globalThis.fetch; + const calls: string[] = []; + + globalThis.fetch = async (url) => { + calls.push(String(url)); + + if (String(url) === "https://gitlab.example.com/api/v4/code_suggestions/direct_access") { + return jsonResponse({ error: "insufficient_scope", scope: "ai_features" }, 403); + } + + return jsonResponse({ + model: { name: "code-gecko" }, + choices: [{ text: "fallback path works" }], + }); + }; + + try { + const result = await executor.execute({ + model: "gitlab-duo-code-suggestions", + body: { + messages: [{ role: "user", content: "Say hello" }], + }, + stream: false, + credentials: { + accessToken: "oauth-access", + providerSpecificData: { + baseUrl: "https://gitlab.example.com", + }, + }, + signal: AbortSignal.timeout(10_000), + log: null, + }); + + assert.deepEqual(calls, [ + "https://gitlab.example.com/api/v4/code_suggestions/direct_access", + "https://gitlab.example.com/api/v4/code_suggestions/completions", + ]); + + const body = (await result.response.json()) as GitLabResponseBody; + assert.equal(body.model, "code-gecko"); + assert.match(body.choices[0].message.content, /fallback path/i); + } finally { + globalThis.fetch = originalFetch; + } +}); diff --git a/tests/unit/issue-12958-gitlab-duo-403-entitlement-fallback.test.ts b/tests/unit/issue-12958-gitlab-duo-403-entitlement-fallback.test.ts new file mode 100644 index 0000000000..db32b842bc --- /dev/null +++ b/tests/unit/issue-12958-gitlab-duo-403-entitlement-fallback.test.ts @@ -0,0 +1,113 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { testOAuthConnection } from "../../src/app/api/providers/[id]/test/route"; + +// #12958: the reporter has a valid Duo seat and a configured default namespace, but +// gitlab.com returns an entitlement/scope-resolution 403 from `direct_access` for their +// API-only client. That 403 is NOT the "direct connections are disabled" tenant-config +// message the #10365/#10499 fallback guard recognizes, so the connection test never tries +// the public Code Suggestions fallback (which the reporter proved works with the same +// token) and instead reports the connection unhealthy with a generic "Access denied" that +// discards the real upstream body. These tests lock in the corrected contract: ANY +// direct_access 403 is recoverable via the fallback probe (same as 401 already is), and +// when both endpoints genuinely reject the token, the real upstream body is surfaced. + +const DIRECT_ACCESS_URL = "https://gitlab.example.com/api/v4/code_suggestions/direct_access"; +const PUBLIC_COMPLETIONS_URL = "https://gitlab.example.com/api/v4/code_suggestions/completions"; + +function futureExpiresAt(): string { + return new Date(Date.now() + 60 * 60 * 1000).toISOString(); +} + +function baseConnection(overrides: Record = {}) { + return { + provider: "gitlab-duo", + authType: "oauth", + accessToken: "oauth-access", + refreshToken: "oauth-refresh", + expiresAt: futureExpiresAt(), + providerSpecificData: { baseUrl: "https://gitlab.example.com" }, + ...overrides, + }; +} + +function mockFetch(handler: (url: string, init?: RequestInit) => Response) { + const calls: Array<{ url: string; init?: RequestInit }> = []; + const fn = (async (url: RequestInfo | URL, init?: RequestInit) => { + const u = typeof url === "string" ? url : url instanceof URL ? url.toString() : String(url); + calls.push({ url: u, init }); + return handler(u, init); + }) as typeof fetch; + return { fn, calls }; +} + +test("gitlab-duo Retest does NOT fall back on an entitlement-flavored 403 (#12958)", async (t) => { + const original = globalThis.fetch; + const { fn, calls } = mockFetch((url) => { + if (url === DIRECT_ACCESS_URL) { + return new Response(JSON.stringify({ message: "Access denied" }), { + status: 403, + headers: { "content-type": "application/json" }, + }); + } + if (url === PUBLIC_COMPLETIONS_URL) { + return new Response(JSON.stringify({ model: { name: "code-gecko" }, choices: [] }), { + status: 200, + headers: { "content-type": "application/json" }, + }); + } + throw new Error(`Unexpected fetch to ${url}`); + }); + globalThis.fetch = fn; + t.after(() => { + globalThis.fetch = original; + }); + + const result = await testOAuthConnection(baseConnection(), 5000); + + assert.equal( + result.valid, + true, + "an entitlement-flavored direct_access 403 must also be verified against the public " + + "completions fallback before declaring the connection unhealthy — same contract as " + + "401 and the 'direct connections are disabled' 403" + ); + assert.deepEqual( + calls.map((c) => c.url), + [DIRECT_ACCESS_URL, PUBLIC_COMPLETIONS_URL], + "the fallback probe must be attempted for ANY direct_access 403, not only the exact " + + "'direct connections are disabled' tenant-config message" + ); +}); + +test("gitlab-duo Retest surfaces the real upstream 403 body when BOTH endpoints reject (#12958)", async (t) => { + const original = globalThis.fetch; + const { fn } = mockFetch((url) => { + if (url === DIRECT_ACCESS_URL) { + return new Response(JSON.stringify({ error: "insufficient_scope", scope: "ai_features" }), { + status: 403, + headers: { "content-type": "application/json" }, + }); + } + if (url === PUBLIC_COMPLETIONS_URL) { + return new Response(JSON.stringify({ message: "Access denied" }), { + status: 403, + headers: { "content-type": "application/json" }, + }); + } + throw new Error(`Unexpected fetch to ${url}`); + }); + globalThis.fetch = fn; + t.after(() => { + globalThis.fetch = original; + }); + + const result = await testOAuthConnection(baseConnection(), 5000); + + assert.equal(result.valid, false); + assert.ok( + result.error && result.error.includes("insufficient_scope"), + `expected the real upstream direct_access body to be surfaced, got: ${JSON.stringify(result.error)}` + ); +});