Skip to content

feat(mcp): CIMD client identification for OAuth MCP - #84050

Closed
rob-maron wants to merge 1 commit into
mainfrom
rm/mcp-cimd-auth
Closed

feat(mcp): CIMD client identification for OAuth MCP#84050
rob-maron wants to merge 1 commit into
mainfrom
rm/mcp-cimd-auth

Conversation

@rob-maron

Copy link
Copy Markdown
Contributor

No description provided.

@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

૮ >ﻌ< ა ci review

ran on d31d671 — MCP CIMD auth

⚠️ Warnings

OSV vulnerability scan · View job

2 known vulnerabilities found in pinned dependencies.

How to fix:

Review the findings in the Security tab. Update the affected dependencies if a patched version is available.


debug info

CI timings

CI timings · View report · View job

Wall time 5m8s vs 6m44s (-23.8%). 10 job(s) slower, 13 faster, 3 unchanged.

  • Python tests / Run tests slice 7/12: +98.0s
  • OS-specific tests / macOS-only tests: +36.0s
  • Python tests / Run tests slice 5/12: -17.0s
  • Python tests / Run tests slice 12/12: -14.0s
  • Python tests / Run tests slice 11/12: -12.0s

@alt-glitch alt-glitch added type/feature New feature or request tool/mcp MCP client and OAuth area/auth Authentication, OAuth, credential pools P3 Low — cosmetic, nice to have sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation labels Aug 11, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: implements the CIMD/client-identification portion of #75576 with published metadata and callback-port consistency coverage.

@rob-maron
rob-maron requested a review from teknium1 August 12, 2026 15:34
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

feat(mcp): CIMD client identification for OAuth MCP

  1. tools/mcp_oauth.py (_park_reserved_socket): the eviction cap (_MAX_RESERVED_SOCKETS = 8) applies to pinned CIMD sockets as well. Under heavy concurrency (more than 8 simultaneous reservations), an earlier CIMD port's parked socket can be closed before _wait_for_callback adopts it, silently converting a pinned flow into an unbound one — the exact stealability race the pinning exists to prevent (fix: TOCTOU port race between _find_free_port() and HTTPServer bind #22161). Consider a separate, higher cap for the pinned range or refusing to evict CIMD pins.

  2. tools/mcp_oauth.py (_pick_cimd_port): once this process holds all five ports, the range wraps and returns _assigned_cimd_ports[0], so a 6th+ server reuses server Terminal tool #1's port. The docstring acknowledges this, but two servers authorizing at the same moment would then share one listener, and the "reports that collision clearly" claim depends on the waiter's error path. Consider recording the owning server per port and falling back to DCR (rather than sharing) when the range is exhausted.

  3. tools/mcp_oauth.py (_maybe_use_cimd / _configure_callback_port): the optimistic pin is taken for every unknown server at provider-build time and _assigned_cimd_ports is never released. A long-lived gateway serving many OAuth MCP servers (several of which turn out to be DCR-only) burns through the five-port range quickly and reaches the wrap case above. If the SDK's 401 branch later reveals the server does not advertise CIMD, the assignment is never reclaimed; releasing it in that path (when feasible) would keep the range available for genuinely unknown servers.

No blocking issues found.

@alt-glitch alt-glitch added the comp/tools Tool registry, model_tools, toolsets label Aug 16, 2026
teknium1 added a commit that referenced this pull request Aug 19, 2026
The _MAX_RESERVED_SOCKETS cap applied to pinned CIMD sockets too, so under
heavy concurrency an ephemeral-reservation churn could close a parked pinned
socket before _wait_for_callback adopted it, silently reopening the
port-stealing window the pin exists to prevent (#22161). Eviction now skips
the pinned range; it is already bounded by _CIMD_PORTS.

Follow-up to the #84050 salvage.
teknium1 added a commit that referenced this pull request Aug 19, 2026
The _MAX_RESERVED_SOCKETS cap applied to pinned CIMD sockets too, so under
heavy concurrency an ephemeral-reservation churn could close a parked pinned
socket before _wait_for_callback adopted it, silently reopening the
port-stealing window the pin exists to prevent (#22161). Eviction now skips
the pinned range; it is already bounded by _CIMD_PORTS.

Follow-up to the #84050 salvage.
@teknium1

Copy link
Copy Markdown
Contributor

Merged via PR #89566 — your commit was cherry-picked onto current main (post mcp 2.x SDK migration) with your authorship preserved in git history, plus a small follow-up hardening the pinned-socket reservation against the eviction cap. Thanks for the excellent, thoroughly-tested implementation!

lisajlau pushed a commit to lisajlau/hermes-agent that referenced this pull request Aug 20, 2026
The _MAX_RESERVED_SOCKETS cap applied to pinned CIMD sockets too, so under
heavy concurrency an ephemeral-reservation churn could close a parked pinned
socket before _wait_for_callback adopted it, silently reopening the
port-stealing window the pin exists to prevent (NousResearch#22161). Eviction now skips
the pinned range; it is already bounded by _CIMD_PORTS.

Follow-up to the NousResearch#84050 salvage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/mcp MCP client and OAuth type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants