From 032387401a2ca66109371bd5dd692db60ee78b3f Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com> Date: Fri, 19 Jun 2026 21:31:42 -0300 Subject: [PATCH] fix(oauth): GitHub Copilot token refresh sends the public client_id (#4320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHub Copilot is a public device-flow OAuth client (client_id, no client_secret), but the github provider config never populated clientId. The standalone refresh path omitted client_id (buildFormParams drops undefined) and the executor path sent the literal "client_id=undefined&client_secret=undefined" — both rejected by GitHub, so a Copilot connection got stuck once its short-lived token expired and the long-lived refresh path was needed. Populate the provider clientId from the embedded public cred (resolvePublicCred, never a literal) and only send client_secret when one exists. The prior github refresh test patched a fake clientId/clientSecret onto PROVIDERS.github, masking the broken real config — it now exercises the real config. Co-authored-by: Manuel B. <1494154+baslr@users.noreply.github.com> --- CHANGELOG.md | 1 + .../config/providers/registry/github/index.ts | 14 +++++- open-sse/executors/github.ts | 18 ++++--- tests/unit/executor-github.test.ts | 40 +++++++++++++++ tests/unit/token-refresh-service.test.ts | 50 +++++++++---------- 5 files changed, 91 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6bcf860b6b..d8d1fcb56a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ _In development — bullets added per PR; finalized at release._ - **fix(api): cache-HIT `X-OmniRoute-Response-Cost` now reports the incremental cost (≈0), not the original** — on a semantic-cache HIT the gateway serves the stored response **without** an upstream call, but `X-OmniRoute-Response-Cost` was reporting the original call's full cost (recomputed from the cached `usage`). A consumer summing `response-cost` for billing was therefore charging for responses that cost ≈$0 to serve (and stale entries could inflate it). Cache hits now bill `X-OmniRoute-Response-Cost: 0.0000000000` (the real incremental cost), and the avoided cost is surfaced in a new **`X-OmniRoute-Cost-Saved`** header for cache analytics — mirroring the existing `tokens_saved` concept. The MISS path is unchanged. (PRD-2026-06-19-cache-hit-cost-reporting) - **fix(models): imported vision-capable models keep their vision capability** — after importing a provider key, vision-capable models (e.g. OpenRouter models whose `architecture` declares image input, and other synced providers) were listed as text-only in `/v1/models` and the dashboard — even though image requests actually worked. Synced model records never captured the vision flag, and the catalog's OpenRouter live-enrichment (which derives vision from `architecture.input_modalities`) is skipped once a provider has synced models. Discovery now captures `supportsVision` at sync time (from `architecture.input_modalities`, the string `architecture.modality`, or a top-level `input_modalities`), mirroring the existing `supportsThinking` capture, and the catalog surfaces `capabilities.vision` for synced models. ([#4264](https://github.com/diegosouzapw/OmniRoute/issues/4264) — thanks @FerLuisxd) - **fix(providers): Cloudflare Workers AI model discovery shows model names, not UUIDs** — importing a Cloudflare Workers AI key listed models with internal UUID identifiers (e.g. `429b9e8b-d99e-…`) instead of their usable slugs (`@cf/meta/llama-3.1-8b-instruct`). Cloudflare's `/ai/models/search` returns `{ id: "", name: "@cf/…" }`, and discovery was passing the raw objects through — so the UUID `id` became the callable model id. The `cloudflare-ai` discovery now maps each result's `name` → id, surfacing the real `@cf/…` model ids. ([#4259](https://github.com/diegosouzapw/OmniRoute/issues/4259) — thanks @FerLuisxd) +- **fix(oauth): GitHub Copilot token refresh now sends the public client_id** — the `github` provider config never carried a `clientId`, so GitHub OAuth `refresh_token` exchanges either omitted `client_id` or sent the literal string `undefined` (and a bogus `client_secret=undefined`), which GitHub rejects — leaving a Copilot connection stuck once its short-lived token expired and the long-lived refresh path was needed. The provider now resolves its public device-flow `client_id` from the embedded public credential and omits `client_secret` entirely (GitHub's Copilot app is a public client with no secret). (thanks @baslr) --- diff --git a/open-sse/config/providers/registry/github/index.ts b/open-sse/config/providers/registry/github/index.ts index 366fe7b577..6eb80bbf40 100644 --- a/open-sse/config/providers/registry/github/index.ts +++ b/open-sse/config/providers/registry/github/index.ts @@ -1,5 +1,9 @@ import type { RegistryEntry } from "../../shared.ts"; -import { GPT_5_5_CODEX_CAPABILITIES, getGitHubCopilotChatHeaders } from "../../shared.ts"; +import { + GPT_5_5_CODEX_CAPABILITIES, + getGitHubCopilotChatHeaders, + resolvePublicCred, +} from "../../shared.ts"; export const githubProvider: RegistryEntry = { id: "github", @@ -10,6 +14,14 @@ export const githubProvider: RegistryEntry = { responsesBaseUrl: "https://api.githubcopilot.com/responses", authType: "oauth", authHeader: "bearer", + // GitHub Copilot is a public device-flow OAuth client: it has a public client_id but + // NO client_secret. Populate clientId so token refresh carries it (9router#442) — without + // it, refresh requests omit/garble client_id and GitHub rejects them. Embedded via + // resolvePublicCred per Hard Rule #11 (never a string literal). + oauth: { + clientIdEnv: "GITHUB_OAUTH_CLIENT_ID", + clientIdDefault: resolvePublicCred("github_copilot_id"), + }, defaultContextLength: 128000, headers: getGitHubCopilotChatHeaders(), models: [ diff --git a/open-sse/executors/github.ts b/open-sse/executors/github.ts index b3c1ab8b28..98a965f17e 100644 --- a/open-sse/executors/github.ts +++ b/open-sse/executors/github.ts @@ -172,18 +172,24 @@ export class GithubExecutor extends BaseExecutor { async refreshGitHubToken(refreshToken, log) { try { + // GitHub Copilot is a public device-flow client: send the public client_id, and + // only attach client_secret when one is actually configured — never the literal + // "undefined" that new URLSearchParams produces for a missing value (9router#442). + const params = new URLSearchParams({ + grant_type: "refresh_token", + refresh_token: refreshToken, + client_id: this.config.clientId, + }); + if (this.config.clientSecret) { + params.set("client_secret", this.config.clientSecret); + } const response = await fetch(OAUTH_ENDPOINTS.github.token, { method: "POST", headers: { "Content-Type": "application/x-www-form-urlencoded", Accept: "application/json", }, - body: new URLSearchParams({ - grant_type: "refresh_token", - refresh_token: refreshToken, - client_id: this.config.clientId, - client_secret: this.config.clientSecret, - }), + body: params, }); if (!response.ok) return null; const tokens = await response.json(); diff --git a/tests/unit/executor-github.test.ts b/tests/unit/executor-github.test.ts index 7a11ad023e..95cf3bcc2f 100644 --- a/tests/unit/executor-github.test.ts +++ b/tests/unit/executor-github.test.ts @@ -8,6 +8,46 @@ function registerModel(provider, model) { PROVIDER_MODELS[provider] = [...(PROVIDER_MODELS[provider] || []), model]; } +test("GithubExecutor.refreshGitHubToken sends the public client_id and omits client_secret (port from 9router#442)", async () => { + // GitHub Copilot is a public device-flow OAuth client (client_id, no client_secret). + // The previous code sent client_id/client_secret straight from this.config via + // new URLSearchParams, so an undefined config produced the literal + // "client_id=undefined&client_secret=undefined". The fix populates the real client_id + // and only sends client_secret when one actually exists. + const executor = new GithubExecutor(); + const calls: any[] = []; + const originalFetch = globalThis.fetch; + globalThis.fetch = (async (url: any, options: any = {}) => { + calls.push({ url: String(url), options }); + return { + ok: true, + json: async () => ({ + access_token: "gh-access", + refresh_token: "gh-next", + expires_in: 3600, + }), + } as any; + }) as any; + + try { + const result = await executor.refreshGitHubToken("gh-refresh", { info() {}, error() {} }); + assert.deepEqual(result, { + accessToken: "gh-access", + refreshToken: "gh-next", + expiresIn: 3600, + }); + } finally { + globalThis.fetch = originalFetch; + } + + const body = String(calls[0].options.body); + assert.match(body, /client_id=Iv1\./, "the real public github client_id must be sent"); + assert.ok( + !body.includes("client_secret="), + "client_secret must be omitted (never the literal 'undefined')" + ); +}); + test("GithubExecutor.buildUrl routes response-format models to /responses", () => { const originalModels = [...(PROVIDER_MODELS.gh || [])]; registerModel("gh", { diff --git a/tests/unit/token-refresh-service.test.ts b/tests/unit/token-refresh-service.test.ts index adea08462d..1574a79626 100644 --- a/tests/unit/token-refresh-service.test.ts +++ b/tests/unit/token-refresh-service.test.ts @@ -689,40 +689,40 @@ test("refreshQoderToken uses basic auth once qoder oauth settings are configured assert.match(calls[0].options.headers.Authorization, /^Basic /); }); -test("refreshGitHubToken exchanges the refresh token with github oauth", async () => { +test("refreshGitHubToken sends the real public github client_id and no client_secret (port from 9router#442)", async () => { + // GitHub Copilot's OAuth app is a public device-flow client: it has a client_id but + // NO client_secret. PROVIDERS.github.clientId must be populated from the embedded public + // cred so the refresh request actually carries a client_id — a missing one makes GitHub + // reject the refresh. The previous test patched a fake clientId/clientSecret onto + // PROVIDERS.github, masking the fact that the real config had neither. This uses the real + // config and asserts the real client_id is sent and no client_secret leaks out. const log = createLog(); const calls: any[] = []; - await withPatchedProperties( - PROVIDERS.github, - { - clientId: "github-client", - clientSecret: "github-secret", + await withMockedFetch( + async (url, options = {}) => { + calls.push({ url, options }); + return jsonResponse({ + access_token: "github-access", + refresh_token: "github-refresh-next", + expires_in: 3600, + }); }, async () => { - await withMockedFetch( - async (url, options = {}) => { - calls.push({ url, options }); - return jsonResponse({ - access_token: "github-access", - refresh_token: "github-refresh-next", - expires_in: 3600, - }); - }, - async () => { - const result = await refreshGitHubToken("github-refresh", log); - assert.deepEqual(result, { - accessToken: "github-access", - refreshToken: "github-refresh-next", - expiresIn: 3600, - }); - } - ); + const result = await refreshGitHubToken("github-refresh", log); + assert.deepEqual(result, { + accessToken: "github-access", + refreshToken: "github-refresh-next", + expiresIn: 3600, + }); } ); + const body = bodyToString(calls[0].options.body); assert.equal(calls[0].url, OAUTH_ENDPOINTS.github.token); - assert.match(bodyToString(calls[0].options.body), /client_id=github-client/); + assert.ok(PROVIDERS.github.clientId, "PROVIDERS.github.clientId must be populated from the public cred"); + assert.match(body, /client_id=Iv1\./, "the real public github client_id must be sent on refresh"); + assert.ok(!body.includes("client_secret="), "no client_secret for the public github client"); }); test("refreshCopilotToken returns the short-lived copilot token", async () => {