Skip to content

fix(copilot-oauth): bound OAuth and API endpoint response reads#96877

Closed
solodmd wants to merge 1 commit into
openclaw:mainfrom
solodmd:fix/bound-copilot-oauth-response
Closed

fix(copilot-oauth): bound OAuth and API endpoint response reads#96877
solodmd wants to merge 1 commit into
openclaw:mainfrom
solodmd:fix/bound-copilot-oauth-response

Conversation

@solodmd

@solodmd solodmd commented Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

fetchJson in src/llm/utils/oauth/github-copilot.ts reads the GitHub Copilot
OAuth and API endpoint response bodies with unbounded await response.text()
and await response.json(). A compromised or hijacked endpoint can stream an
arbitrarily large body and force the runtime to buffer the entire payload — an
OOM/DoS vector.

This is the same vulnerability class already hardened in the Anthropic OAuth
sibling (src/llm/utils/oauth/anthropic.ts, PR #96644).

Changes

  • src/llm/utils/oauth/github-copilot.ts — import readResponseWithLimit
    from @openclaw/media-core/read-response-with-limit; replace both
    await response.text() and await response.json() with
    readResponseWithLimit at 16 MiB cap + new TextDecoder().decode(buffer) +
    JSON.parse. Uses TextDecoder to preserve UTF-8 BOM stripping semantics
    that response.text() provides.

Evidence

Real behavior proof

A real node:http server was started on 127.0.0.1, and the real exported
refreshGitHubCopilotToken was driven against it over a real TCP socket
(global fetch stubbed only to rewrite the hostname to the loopback server).
Node v22.22.0.

$ node --import tsx _proof_copilot_overflow.mts
[proof] cap=16777216 (16 MiB)

[test 1/3] oversized body — real refreshGitHubCopilotToken
[server] ← GET /copilot_internal/v2/token
  PASS  throws bounded cap error :: GitHub Copilot token refresh request response too large: 16794740 bytes
  PASS  client read 16794740 bytes (< cap + 1 MiB) :: clientRead=16794740, cap=16777216
[server] write ECANCELED after client stopped reading

[test 2/3] negative control — raw response.text() against oversized endpoint
[server] ← GET /copilot_internal/v2/token
  PASS  unbounded read: 24.0 MiB > 16 MiB cap :: buffered=25165824 bytes

[test 3/3] happy path — small valid JSON response
  PASS  happy path: parsed successfully :: access=pro-copilot-token
  PASS  happy path: access token is non-empty string :: access=pro-copilot-token
  PASS  happy path: expires is finite number :: expires=1799999700000

[proof] ALL PASS (pass=6 fail=0)
  • Oversized case: refreshGitHubCopilotToken threw
    GitHub Copilot token refresh request response too large: 16794740 bytes;
    the server write was cancelled (ECANCELED) after the client stopped reading,
    confirming the stream was cancelled, not drained.
  • Negative control: the OLD unbounded response.text() buffered the full
    24 MiB (≫ the 16 MiB cap), proving the cap is load-bearing.
  • Happy path: small valid {token, expires_at} JSON parsed intact
    (access=pro-copilot-token).

Tests

  • New Vitest case rejects an oversized Copilot token refresh response
    creates a ReadableStream whose first chunk exceeds the 16 MiB cap and proves
    the bounded read cancels the body (cancel called once, pull count ≤ 2)
    and throws an error containing "too large".
  • Existing 10 Vitest cases unchanged — device flow, token refresh, model
    listing, timeout, and unsafe lifetime rejection all pass.
  • pnpm test src/llm/utils/oauth/github-copilot.test.ts → 11 passed (11)

@clawsweeper

clawsweeper Bot commented Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 26, 2026, 1:24 AM ET / 05:24 UTC.

Summary
The PR changes the core GitHub Copilot OAuth/API fetchJson helper to read response bodies through a 16 MiB bounded reader and adds an oversized token-refresh cancellation test.

PR surface: Source +13, Tests +22. Total +35 across 2 files.

Reproducibility: yes. for source-level review: current main and v2026.6.10 use unbounded response.text() and response.json() in the shared Copilot helper, and the PR body includes terminal proof over a real local TCP server.

Review metrics: 1 noteworthy metric.

  • Copilot response cap: 1 shared helper capped at 16 MiB. The helper covers device-code, device-token polling, token-refresh, and model-list responses, so maintainers should notice the new fail-closed provider boundary.

Stored data model
Persistent data-model change detected: serialized state: src/llm/utils/oauth/github-copilot.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • none.

Risk before merge

  • [P1] The 16 MiB cap changes oversized Copilot device-code, token-refresh, and model-list responses from unbounded buffering to fail-closed errors, so maintainers should accept or tune that auth-provider boundary before merge.

Maintainer options:

  1. Accept the 16 MiB Copilot auth cap (recommended)
    Maintainers can accept the cap because OAuth token and model-list payloads should be small and the same byte limit is already used by shared provider JSON handling.
  2. Tune the cap before merge
    If legitimate Copilot OAuth or model-list responses can exceed 16 MiB, adjust the limit and refresh the overflow proof before landing.
  3. Coordinate with adjacent OAuth hardening
    Maintainers can pause this PR if they want the Copilot and Anthropic OAuth response-read caps accepted together.

Next step before merge

  • [P2] A human maintainer should accept or tune the 16 MiB Copilot auth/API cap before merge; there is no narrow contributor-facing repair for ClawSweeper to queue.

Security
Cleared: The diff narrows an unbounded OAuth/API response-read availability exposure and does not change dependencies, workflows, package metadata, permissions, or secret handling.

Review details

Best possible solution:

Land the narrow core helper cap after maintainer acceptance of the 16 MiB auth-provider boundary, and keep adjacent OAuth/provider response-read sweeps separate unless maintainers want a coordinated batch.

Do we have a high-confidence way to reproduce the issue?

Yes for source-level review: current main and v2026.6.10 use unbounded response.text() and response.json() in the shared Copilot helper, and the PR body includes terminal proof over a real local TCP server.

Is this the best way to solve the issue?

Yes. Applying the existing bounded reader at the shared Copilot fetchJson helper is the narrowest maintainable fix for this core path; broader sibling OAuth and plugin hardening is adjacent follow-up, not a blocker for this patch.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 751a6c23f098.

Label changes

Label justifications:

  • P2: This is normal-priority security and availability hardening for one provider OAuth/API helper with limited blast radius and no active outage evidence.
  • merge-risk: 🚨 compatibility: The PR changes oversized Copilot OAuth/API JSON responses from unbounded parsing to a 16 MiB failure.
  • merge-risk: 🚨 auth-provider: The changed helper handles Copilot login, token refresh, and model-list responses, so cap behavior can affect provider auth and catalog setup flows.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes terminal output from a real local node:http server over TCP showing overflow cancellation, a negative control, and a small valid JSON success path.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes terminal output from a real local node:http server over TCP showing overflow cancellation, a negative control, and a small valid JSON success path.
Evidence reviewed

PR surface:

Source +13, Tests +22. Total +35 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 15 2 +13
Tests 1 22 0 +22
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 37 2 +35

What I checked:

Likely related people:

  • vincentkoc: git blame ties the current Copilot OAuth helper and shared bounded-reader helper in this checkout to commit 497d53d821e6. (role: recent current-main committer; confidence: medium; commits: 497d53d821e6; files: src/llm/utils/oauth/github-copilot.ts, packages/media-core/src/read-response-with-limit.ts, src/agents/provider-http-errors.ts)
  • Alix-007: Authored merged PR fix(github-copilot): bound usage response reads #96607, which hardened neighboring GitHub Copilot usage response reads with the same bounded JSON pattern. (role: adjacent Copilot response-hardening contributor; confidence: medium; commits: 646e54ae3578; files: extensions/github-copilot/usage.ts, extensions/github-copilot/models.test.ts)
  • hugenshen: Authored merged PR fix(github-copilot): bound model discovery and embeddings JSON response #96499, which bounded GitHub Copilot model discovery and embeddings JSON response reads. (role: adjacent Copilot response-hardening contributor; confidence: medium; commits: 1aa7cafc35a1; files: extensions/github-copilot/embeddings.ts, extensions/github-copilot/embeddings.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@solodmd
solodmd marked this pull request as draft June 26, 2026 00:49
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 26, 2026
@solodmd
solodmd force-pushed the fix/bound-copilot-oauth-response branch from 2f9fa1a to f76caf0 Compare June 26, 2026 00:55
@solodmd
solodmd marked this pull request as ready for review June 26, 2026 00:55
@solodmd

solodmd commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper review

@clawsweeper

clawsweeper Bot commented Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jun 26, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 28, 2026
Merged
@vincentkoc

vincentkoc commented Jun 28, 2026

Copy link
Copy Markdown
Member

Closing this as superseded by the narrower canonical fix in #97499, which landed as bd0c052 and uses the shared bounded provider readers with overflow/cancel coverage. Thanks for pushing this boundary forward.

@vincentkoc vincentkoc closed this Jun 28, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 29, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@solodmd @vincentkoc