Skip to content

feat(auth): OAuth 2.0 client credentials support for CLI + library#746

Merged
bokelley merged 6 commits into
mainfrom
bokelley/cli-issue-2677
Apr 22, 2026
Merged

feat(auth): OAuth 2.0 client credentials support for CLI + library#746
bokelley merged 6 commits into
mainfrom
bokelley/cli-issue-2677

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Summary

  • Adds OAuth 2.0 client credentials (RFC 6749 §4.4) support to @adcp/client so machine-to-machine compliance runs no longer need manual token management. Addresses adcontextprotocol/adcp#2677.
  • Library-level auto-refresh in ProtocolClient.callTool (pre-flight + mid-session 401 retry, coalesced across concurrent callers). Storyboard/fuzz/grade pick it up via the new auth: { type: 'oauth_client_credentials', credentials, tokens? } option on TestOptions.
  • CLI: adcp --save-auth <alias> <url> --oauth-token-url <url> --client-id ... --client-secret ..., with --client-id-env / --client-secret-env for CI (stores $ENV:VAR references — nothing sensitive on disk). Dedicated adcp --save-auth --help subcommand help.
  • Security: HTTPS-only token_endpoint (localhost carved out for dev), userinfo rejected, private-IP SSRF guard with operator opt-in, RFC 6749 §2.3.1 Basic-auth encoding, control-char sanitization on AS error_description (defends against ANSI/CRLF injection from a hostile AS).
  • RFC 8707 resource / audience forwarding for audience-validating authorization servers (Auth0, Okta, Azure AD, Keycloak).
  • Typed errors: ClientCredentialsExchangeError with kind: 'oauth' | 'malformed' | 'network'; MissingEnvSecretError with reason: 'unset' | 'empty'.
  • Bug fix caught by the new integration test: is401Error now recognizes err.code === 401 (MCP SDK's StreamableHTTPClientTransport error shape), so the 401-retry actually fires end-to-end.

Relates to adcontextprotocol/adcp#2738 — that PR fixed the auth-code handoff from ComplianceDatabase.resolveOwnerAuth to the SDK. This PR adds the client-credentials leg on the SDK side. A follow-up issue will file the dashboard-side work for save_agent to accept CC config.

Test plan

  • 33 unit tests in test/oauth-client-credentials.test.js (mocked fetch): exchange shape, RFC 6749 §2.3.1 encoding, RFC 8707 resource/audience, TLS/userinfo/SSRF rejection, error kind discriminator, control-char sanitization, coalescing, force + concurrency interaction, getAuthToken integration, createTestClient wiring.
  • 5 integration tests in test/oauth-client-credentials-integration.test.js (real MCP stub + real token server on 127.0.0.1:0): initial exchange + bearer attach, warm cache, mid-session token rotation → 401 → force-refresh → retry succeeds, unbounded-retry protection, concurrent-call coalescing through the real call path.
  • 84 tests green across related suites (oauth-*, oauth-storyboard-plumbing, server-auth, cli-multi-instance-strategy).
  • error-extraction.test.js + test/unit/mcp-*.test.js (61 tests) still pass — is401Error change didn't regress anything.
  • Manual e2e against a local fake token server: --save-auth happy path, TLS rejection, userinfo rejection, localhost.attacker.com prefix bypass blocked, empty env var surfaces a distinct error.
  • adcp --save-auth --help prints dedicated subcommand help, unshadowed by the main --help handler.

Reviewer passes

Ran four specialist reviews (security, code, DX, testing) — two rounds. All must-fix and should-fix feedback is addressed in-PR:

  • Security: userinfo reject, library-level SSRF guard with opt-in, CLI URL parser (closes http://localhost.attacker.com prefix bypass), coalesce key includes token_endpoint + client_id.
  • Code: runFullAssessment uses buildResolvedAuthOption helper, MissingEnvSecretError.reason discriminator, URLSearchParams-based test assertions, force+coalesce buckets so a 401-triggered forced refresh doesn't piggyback on a stale in-flight exchange, A2A 401-retry symmetric with MCP (wraps with rethrowAsNeedsAuthorization).
  • DX: renamed --oauth-endpoint--oauth-token-url, three extra error hints (invalid_grant, invalid_request, non-JSON/404/405 → issuer-URL paste hint), CC symbols re-exported from @adcp/client top level for IntelliSense discovery.
  • Testing: integration test against real MCP + token server added — catches the 401-retry path that unit tests missed.

🤖 Generated with Claude Code

Adds RFC 6749 §4.4 machine-to-machine OAuth to @adcp/client so CI compliance
runs can point at an OAuth-protected agent without manual token management.
Addresses adcontextprotocol/adcp#2677.

Library auto-refreshes tokens before every call via
`ensureClientCredentialsTokens` (coalesced across concurrent callers) and
retries once on mid-session 401. CLI exposes `--save-auth --oauth-token-url`
with `--client-secret-env` env-var indirection for CI. Includes RFC 8707
`resource`/`audience` support, RFC 6749 §2.3.1 Basic-auth encoding, TLS
enforcement with userinfo/SSRF guards, and control-char sanitization on
AS-supplied error messages.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread bin/adcp.js
bindAgentStorage,
NeedsAuthorizationError,
exchangeClientCredentials,
ensureClientCredentialsTokens,
Comment thread bin/adcp.js Fixed
Prettier reformat across new CC files. In bin/adcp.js `--list-agents`,
pull the env-var name via `extractEnvSecretName` so the raw `client_secret`
field never enters the log format string. CodeQL's clear-text-logging rule
considers any `*_secret` field access tainted regardless of branch guards;
routing through a helper that returns only the safe label avoids the
false positive without losing the DX of showing which env var holds the
secret.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread bin/adcp.js Fixed
Comment thread bin/adcp.js Fixed
CodeQL's clear-text-logging rule treats any field access on
`oauth_client_credentials` as tainted, even when the branch proves the
value is a non-sensitive label (env-var name). Drop the secret-source
breakdown from --list-agents and surface only "OAuth: client credentials";
users who want the full config can run --show-config.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley

Copy link
Copy Markdown
Contributor Author

CodeQL alerts — 27 pre-existing false positives

The CodeQL PR check fails on 27 new alerts, all of which are pre-existing code paths flagged only because this PR added oauth_client_credentials (containing a client_secret field) to the shared AgentConfig type. CodeQL's taint-flow analysis treats any access to a *_secret-named field as a sensitive source and propagates through any object that can transitively contain one.

Breakdown:

  • 14 alerts in scripts/manual-testing/*.ts — pre-existing console.log(result) / console.log(agent) in ad-hoc manual test scripts that never populate oauth_client_credentials.
  • 7 alerts in examples/*.ts — pre-existing console.log('Test Result:', result) sites in example files; ditto.
  • 4 library alerts (src/lib/core/SingleAgentClient.ts:2614, src/lib/core/TaskExecutor.ts:1111, 1382, src/lib/conformance/oracle.ts:190) — all pre-existing logging of error.message / generic result payloads, plus one unrelated js/polynomial-redos warning.
  • 2 hash warnings (src/lib/protocols/mcp.ts:73, src/lib/protocols/a2a.ts:49) — createHash('sha256').update(authToken) used as a connection-cache key, not password hashing. Pre-existing.

Main had 0 open alerts before this PR — I confirmed via gh api .../code-scanning/alerts?state=open&ref=refs/heads/main. Every alert is a taint-propagation artifact from the type-surface expansion, not a real credential leak.

What I fixed in my own code:

  • bin/adcp.js --list-agents no longer prints the secret-source label for CC agents. Reading any field off oauth_client_credentials here trips the rule regardless of whether the value is actually sensitive (env-var names are not). Trade-off: minor DX loss — users who want the full breakdown run --show-config.

Options for the remaining 27:

  1. Dismiss as false positives via the Code Scanning UI.
  2. Adopt Advanced CodeQL config (.github/codeql/codeql-config.yml) to downgrade js/clear-text-logging on examples/ and scripts/manual-testing/ paths, and add a proper sanitizer for the cache-key hash pattern. Out of scope for this PR.
  3. Rename AgentOAuthClientCredentials.client_secret → something without "secret" in the name, which fights RFC 6749 naming. Not recommended.

I'd suggest (1) for this PR and (2) as follow-up work on the repo's code-scanning config.

All other checks pass: Test & Build, Code Quality, Schema Sync, Publish Dry Run, Security Audit, CodeQL Analyze (javascript-typescript + actions), Commitlint, GitGuardian, Changeset, ipr-check.

Make --oauth-token-url optional. When omitted from a client-credentials
--save-auth invocation, walk the RFC 9728 protected-resource metadata and
RFC 8414 authorization-server metadata chain (the same probe that powers
adcp diagnose-auth) and use the advertised token_endpoint. Falls back to
a clear error with guidance if discovery can't resolve it.

Also relaxes the auth-mode mutex so that --client-id/--client-secret
alongside --oauth is accepted (credentials already imply M2M; the flags
are harmless redundancy rather than a mode conflict).

New integration test spins up an agent that serves both well-known
metadata documents and asserts the discovered token endpoint matches
the advertised one.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread bin/adcp.js Fixed
Library:
- discoverAuthorizationRequirements now falls back to OpenID Connect
  Discovery 1.0 (/.well-known/openid-configuration) when RFC 8414's
  /.well-known/oauth-authorization-server misses. Covers Keycloak-in-OIDC,
  Ping, ADFS. Results carry `metadataSource` so callers can tell which
  doc they read.
- AuthorizationRequirements exposes the full `authorization_servers` list
  and `grantTypesSupported` array so callers can log multi-issuer
  deployments and make informed grant-compatibility decisions.
- diagnose-auth grows H7: informational check for DCR (RFC 7591)
  readiness for client_credentials.

CLI:
- `adcp --save-auth` with client credentials:
    * Pre-checks `grant_types_supported` when present (only when the AS
      published it — RFC 8414 says absent defaults to
      ['authorization_code', 'implicit'], so absence is not proof of
      non-support).
    * Prints a multi-AS warning when PRM lists > 1 authorization server.
    * Says "--oauth flag ignored; client credentials imply M2M" rather
      than silently absorbing a redundant --oauth flag.
    * Shows the discovered authorization server alongside the token URL.
    * --dry-run validates + prints the plan without exchanging tokens or
      writing config.
    * Arg-shape mutex (--client-id vs --client-id-env etc.) now runs
      before network discovery so bad-flags cases fail fast.
- Bare `--oauth` (browser) now pre-checks the discovered AS's
  `grant_types_supported` and bails with guidance ("use client
  credentials instead") when authorization_code isn't advertised.
- Help: --oauth-token-url labeled "(optional; auto-discovered)";
  subcommand help lists exit codes.
- Inline require for discoverAuthorizationRequirements moved to the
  top-of-file bulk import.

Tests:
- test/oauth-client-credentials-cli.test.js — new. Subprocess-spawned
  smoke tests for the CLI wire: discovery + persist, --dry-run, OIDC
  fallback, no-metadata error path, flag mutex, $ENV:VAR unset.
- Extended integration test for OIDC fallback.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread bin/adcp.js
discoveredRequirements = await discoverAuthorizationRequirements(url, { allowPrivateIp: true });
if (discoveredRequirements && discoveredRequirements.tokenEndpoint) {
resolvedOauthEndpoint = discoveredRequirements.tokenEndpoint;
console.log(` Found token endpoint: ${resolvedOauthEndpoint}`);
* don't touch the user's real config.
*/

const { test, describe, before, after } = require('node:test');
* don't touch the user's real config.
*/

const { test, describe, before, after } = require('node:test');
Adjusts the diagnose-auth report-shape assertion. H7 was added in the
previous commit; the test was still pinned to the pre-H7 count.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants