fix(github-copilot): support GUI/RPC wizard auth flow#73290
fix(github-copilot): support GUI/RPC wizard auth flow#73290shanselman merged 2 commits intoopenclaw:mainfrom
Conversation
|
Suggested reviewers: @joshavant (Gateway / Security / Agents — primary, experienced) and @shanselman (pairing with a primary reviewer for context). cc @openclaw/secops for awareness — auth-adjacent though �xtensions/github-copilot/** is not in the secops codeowner list. |
Greptile SummaryThis PR replaces the TTY-gated, Confidence Score: 4/5Safe to merge; the two findings are P2 suggestions with no impact on the correctness of the happy path. Only P2 findings were identified — a minor timing inaccuracy in No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/github-copilot/login.ts
Line: 205-210
Comment:
**`expiresAt` computed after blocking UI calls**
`expiresAt` is calculated with `Date.now()` after both `showCode` and `openUrl` complete. If `ctx.prompter.note` is a wizard step that blocks until the user acknowledges (e.g. clicks "Continue"), the client-side expiry window is shifted forward by however long the user takes, causing the polling loop to run past the actual server-side device-code lifetime. The server will still reply with `expired_token` and the error is handled correctly, but the loop may make unnecessary requests after the code has actually expired. Compute `expiresAt` from immediately after `requestDeviceCode` returns:
```ts
const device = await requestDeviceCode({ scope: "read:user" });
const verificationUrl = normalizeGitHubDeviceVerificationUrl(device.verification_uri);
const userCode = normalizeGitHubDeviceUserCode(device.user_code);
const expiresInMs = device.expires_in * 1000;
const expiresAt = Date.now() + expiresInMs; // <-- anchor to code-issuance time
await io.showCode({ verificationUrl, userCode, expiresInMs });
try { await io.openUrl?.(verificationUrl); } catch {}
const accessToken = await pollForAccessToken({
deviceCode: device.device_code,
intervalMs: Math.max(1000, device.interval * 1000),
expiresAt, // <-- use pre-computed value
});
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/plugin-sdk/test-helpers/provider-auth-contract.ts
Line: 314-321
Comment:
**`Request` object fallback gives `"[object Request]"` instead of the URL**
The `String(input)` fallback for non-string, non-`URL` inputs (i.e. a `Request` object) returns the unhelpful string `"[object Request]"`, which won't match any of the URL checks and will throw `unexpected fetch`. All current callers in `login.ts` pass plain string URLs, so this won't trigger today, but it's a silent footgun if the fetch calls are ever refactored to use `Request` objects.
```ts
const target =
typeof input === "string"
? input
: input instanceof URL
? input.toString()
: input instanceof Request
? input.url
: String(input);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(github-copilot): support GUI/RPC wiz..." | Re-trigger Greptile |
689b656 to
7cc0e63
Compare
|
Pushed
Local validation: |
|
Codex review: needs maintainer review before merge. What this changes: This PR changes GitHub Copilot provider auth to run device-code login through provider prompter/openUrl hooks, return the token profile inline for framework persistence, update provider-auth tests, refresh raw-fetch allowlist line numbers, and add a changelog entry. Maintainer follow-up before merge: This is an active, focused auth-adjacent implementation PR; the remaining action is maintainer/security review and CI resolution, not a separate automated repair PR. Security review: Security review cleared: The diff is auth-adjacent but keeps GitHub device endpoints fixed, validates the displayed URL/user code, adds no dependencies or workflow permission changes, and only shifts existing raw-fetch allowlist line numbers. Review detailsBest possible solution: Land a maintainer-reviewed version of this PR once CI is satisfactory, keeping the terminal-only CLI command intact while allowing provider auth callers to complete GitHub Copilot device login through the existing wizard/RPC prompter and URL-opening contract. Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against d30b8dccfda3. |
|
Pre-empting two of the three flags from the Codex review for reviewer convenience: 1. Allowlist shift correspondence (
|
| Before (origin/main) | After (this PR) | |
|---|---|---|
| Device-code POST | login.ts:48 → await fetch(DEVICE_CODE_URL, { method: "POST", ... |
login.ts:69 → await fetch(DEVICE_CODE_URL, { method: "POST", ... |
| Access-token POST | login.ts:80 → await fetch(ACCESS_TOKEN_URL, { method: "POST", ... |
login.ts:101 → await fetch(ACCESS_TOKEN_URL, { method: "POST", ... |
No new raw fetches were added, and no other files in the allowlist were modified.
2. models.test.ts baseline failures are not from this PR
This PR's diff against origin/main:
extensions/github-copilot/index.ts | 54 +++---
extensions/github-copilot/login.ts | 114 +++++++++++++-
scripts/check-no-raw-channel-fetch.mjs | 4 +-
src/plugin-sdk/test-helpers/provider-auth-contract.ts | 175 +++++++++++++++---
git diff origin/main HEAD -- extensions/github-copilot/models.test.ts extensions/github-copilot/token.ts returns zero lines — neither the failing test file nor the source it imports (token.ts → resolveProviderEndpoint) is touched here. The 3 failures stem from a missing resolveProviderEndpoint export in the vitest mock for @openclaw/plugin-sdk/provider-model-shared and reproduce on a clean origin/main checkout.
3. Auth-adjacent maintainer/security review
Deferring to @joshavant and @openclaw/secops as appropriate — flagging here so it's on their radar.
7cc0e63 to
a10b05b
Compare
The interactive GitHub Copilot device-code provider auth ("github-copilot".auth.run)
short-circuited with { profiles: [] } whenever process.stdin.isTTY was false and
otherwise delegated to githubCopilotLoginCommand, which drives readline against
stdin/stdout. That made the device flow unusable from the gateway RPC bridge
and any GUI wizard frontend (including the WinUI tray app), where there is no
controlling TTY and no shared stdin with the gateway process.
This change adds a high-level runGitHubCopilotDeviceFlow() helper in
extensions/github-copilot/login.ts that:
- validates the GitHub verification URL (must be https://github.com/login/device)
and trims/length-checks user_code before exposing them to the host UI;
- exposes a small io interface (showCode + openUrl) so callers render the
code through their own surface (ctx.prompter.note + ctx.openUrl);
- returns a typed result of { authorized | access_denied | expired }
instead of throwing for normal terminal states.
runGitHubCopilotAuth() in extensions/github-copilot/index.ts is rewritten to
use that helper: it drops the isTTY gate, emits the URL/code/expiry through
ctx.prompter.note, best-effort calls ctx.openUrl, and returns the credential
inline via the returned profiles. The framework then persists it under the
correct agentDir, identical to the existing non-interactive path that Val
introduced in 461c10b.
requestDeviceCode and pollForAccessToken stay private to login.ts (no api.ts
export). The CLI entry githubCopilotLoginCommand is unchanged.
The provider auth contract tests in
src/plugin-sdk/test-helpers/provider-auth-contract.ts are updated to reflect
the new contract: the suite stubs fetch for the github.com device-code and
access-token endpoints and asserts that
- the credential is sourced from the device flow response (not a post-hoc
auth-store readback);
- the provider drives the host UI through ctx.prompter.note + ctx.openUrl
(no stdin/stdout dependency);
- auth completes when process.stdin.isTTY is false (GUI/RPC clients).
Validation
- pnpm tsgo:extensions
- pnpm test:extension github-copilot (all github-copilot tests pass; the
3 failures in models.test.ts are pre-existing on main, unrelated to this
change: vitest mock for openclaw/plugin-sdk/provider-model-shared is missing
the resolveProviderEndpoint export)
- End-to-end manual: ran a Windows tray-app onboarding wizard against a local
gateway, picked GitHub Copilot, browser opened to the verification URL,
authorization succeeded, token persisted under the correct agentDir as
github-copilot:github, chat through the new credential worked.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
a10b05b to
aea7d66
Compare
|
Merged via squash.
Thanks @indierawk2k2! |
Merged via squash. Prepared head SHA: aea7d66 Co-authored-by: indierawk2k2 <18598712+indierawk2k2@users.noreply.github.com> Co-authored-by: shanselman <2892+shanselman@users.noreply.github.com> Reviewed-by: @shanselman
Merged via squash. Prepared head SHA: aea7d66 Co-authored-by: indierawk2k2 <18598712+indierawk2k2@users.noreply.github.com> Co-authored-by: shanselman <2892+shanselman@users.noreply.github.com> Reviewed-by: @shanselman
Problem
The interactive GitHub Copilot device-code provider auth (
"github-copilot".auth.run) short-circuits with{ profiles: [] }wheneverprocess.stdin.isTTYis false, and otherwise delegates togithubCopilotLoginCommand, which drivesreadlineagainststdin/stdout. That makes the device flow unusable from the gateway RPC bridge and any GUI wizard frontend (including downstream OpenClaw GUI clients), because there is no controlling TTY and no shared stdin with the gateway process. Users on those clients see an empty result and cannot complete onboarding for Copilot.This is a complementary follow-up to the non-interactive token path Val added in
461c10bb51(feat(onboard): support non-interactive GitHub Copilot token auth); that change deliberately did not touch the interactiverunGitHubCopilotAuthpath.Fix
A new high-level
runGitHubCopilotDeviceFlow()helper inextensions/github-copilot/login.ts:https://github.com/login/device) and trims/length-checksuser_codebefore exposing them to the host UI.iointerface (showCode+openUrl) so callers render the code through their own surface (ctx.prompter.note+ctx.openUrl).{ status: "authorized", accessToken } | { status: "access_denied" } | { status: "expired" }instead of throwing for normal terminal states.runGitHubCopilotAuth()inextensions/github-copilot/index.tsis rewritten to use that helper:process.stdin.isTTYearly-return guard.githubCopilotLoginCommand(which reads from stdin) and the post-hocensureAuthProfileStorereadback.ctx.prompter.noteand best-effort callsctx.openUrl, exactly like other provider auth flows.profiles. The framework then persists it under the correctagentDir(same persistence path as the non-interactive flow).Deliberate non-changes
githubCopilotLoginCommandis unchanged and remains the right path for terminal users.requestDeviceCodeandpollForAccessTokenremain private tologin.ts; noapi.tsexport.extensions/github-copilot/models.tsand provider-discovery logic are not touched. The constants used (DEFAULT_COPILOT_PROFILE_ID,DEFAULT_COPILOT_MODEL,PROVIDER_ID) already live inindex.ts/models.tsafter #461c10bb51.access_denied/expired) errors continue to propagate rather than being swallowed into{ profiles: [] }. This is intentional: silent failure was painful to debug from GUI clients in pre-fix testing. Network/HTTP failures will surface through the provider's normal error path.Tests
src/plugin-sdk/test-helpers/provider-auth-contract.ts– the github-copilot provider-auth contract. The two existing tests asserted the old behavior (auth-store readback after the SDK login command, and TTY-gated short-circuit), which no longer exists. They are replaced with five tests that drive the real flow viavi.stubGlobal("fetch", ...)against the github.com device-code and access-token endpoints:githubCopilotLoginCommandis not called andensureAuthProfileStoreis not read.ctx.openUrlis invoked with the verification URL andctx.prompter.notereceives the user code and URL.process.stdin.isTTYforced tofalse.error: "access_denied"from the token endpoint produces{ profiles: [] }and a "cancelled" note.error: "expired_token"produces{ profiles: [] }and an "expired" note.Validation
pnpm tsgo:extensions– clean.pnpm test:extension github-copilot– 62 passing, 3 pre-existing failing. The 3 failures are inextensions/github-copilot/models.test.tsand are present onmainindependent of this PR (vi.mockforopenclaw/plugin-sdk/provider-model-sharedis missing theresolveProviderEndpointexport). Confirmed by stashing this PR's edits and re-running againstorigin/main(3 fail / 59 pass on baseline; 3 fail / 62 pass with this PR).--devgateway, selected GitHub Copilot, the gateway opened the verification URL in the default browser, the wizard rendered the user code viactx.prompter.note, GitHub OAuth completed, the credential persisted to the active agent dir asgithub-copilot:github, and chat through the new credential worked end-to-end.Out of scope / follow-ups
normalizeGitHubDeviceVerificationUrl/normalizeGitHubDeviceUserCodevalidation can be added later onceextensions/github-copilot/login.test.tsexists; this PR keeps test scope to the provider-auth contract.Contributing.md checklist
extensions/github-copilot/**is not in the secops codeowner list); courtesy CC@openclaw/secopsbecause this is auth-flow code.pnpm test:extension github-copilot.src/**↔extensions/**seam).cc
@openclaw/secops