mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-03 17:51:58 +01:00
## Thinking Path > - Paperclip manages AI agents and their access to external tools. > - Tool connections use OAuth to obtain credentials from remote providers. > - The tool-access service mixed provider protocol parsing with database and secret writes. > - This made protocol changes hard to review without reading the full coordinator. > - This PR gives discovery, endpoint policy, and client exchange explicit owners. > - The coordinator keeps authorization, leases, persistence, and audit decisions. > - A real local provider fixture checks the boundary through public routes. ## Linked Issues or Issue Description **What existing behavior does this improve?** The ownership of OAuth discovery, registration, token exchange, and client metadata. **Current behavior** The tool-access service contains over 20,000 lines. Provider responses, endpoint policy, and stateful operations share the same closure. **Proposed behavior** Small protocol modules accept settings and named request capabilities. They return normalized results. The service owns durable state and existing public exports. This is part 3 of the stack: #17144, then #17146, then this PR. It targets #17146's branch. Review only this PR's diff. ## What Changed - Extract endpoint policy, metadata parsing/discovery, registration/token exchange, provider error normalization, and the public client metadata document. - Validate remote registration responses in the protocol module. Return client material to the coordinator for secret and connection writes. - Remove the unused raw provider response from the internal token result. Keep every consumed field. - Preserve process-wide registration single-flight, service-local maps, refresh leases, membership checks, callback checks, and transaction ownership. - Reuse connection-method URL normalization. Keep the endpoint setting and clock callbacks live. - Add a real local HTTP OAuth/MCP route fixture for public and Basic-authenticated clients. - Reduce `tool-access.ts` from 20,917 lines / 808,777 bytes to 19,293 lines / 749,158 bytes. The seven OAuth modules total 1,697 lines; the largest has 582 lines. The stack removes 2,232 lines from the original service. - Record the next profile-resolution and token-broker boundaries in the dated plan. ## Verification - Baseline: 466 service cases and 80 protocol/provider-policy cases pass. The new two-case HTTP fixture passes before moving production code. - After: `pnpm exec vitest run server/src/__tests__/tool-access-oauth-protocol.integration.test.ts server/src/__tests__/tool-access-service.test.ts server/src/__tests__/remote-mcp-connectors.test.ts server/src/__tests__/remote-http-rebinding.test.ts server/src/__tests__/oauth-client-metadata.test.ts` — 508 tests pass with one worker and disposable PostgreSQL. - The fixture drives connect, metadata discovery, DCR, PKCE code exchange, callback, catalog review, and finish. It checks persisted active state, secret references, issuer/auth-method binding, and wire-level authentication. - `pnpm -r --workspace-concurrency=2 typecheck` passes. The sandbox-only attempt could not open a local IPC socket; the same command passed with local process permissions. - `node scripts/check-module-boundaries.mjs` and `git diff --check` pass. - AST comparison confirms 37 moved bodies are unchanged. Registration protocol and persistence statements match their original halves. Token exchange only drops its unused raw result. Preflight uses explicit defaults and a clock callback. - Initial fixture authoring used a nonexistent catalog enabled field; the corrected assertion checks persisted status and reviewer identity. An initial typecheck found optional defaults at the new boundary; the caller now passes its already-validated defaults. - Final-stack local verification: `pnpm -r --workspace-concurrency=2 typecheck` and `pnpm build` pass. Remote CI is green at `3dc46c132408556ecb31038a9b9621ac20aacefc`. Greptile is 5/5 with no actionable findings or open inline threads. - Full local-suite limitation: `pnpm test:run` was interrupted after 79 minutes during the serial general-server phase, after CI had completed the full test matrix. Before interruption, it flagged the unchanged `company-search-service.test.ts` suite (26 database cases skipped). One isolated rerun passed all 28 cases without code changes. The original log and a pre-interruption copy are preserved. The original failure cause was not reproduced. The full local suite and its later groups are not claimed as passed; the complete CI matrix is green on this stack. ## Risks - OAuth endpoint checks and issuer/resource binding are security boundaries. Existing denial, redirect, scope, DCR, secret-redaction, and refresh tests cover the preserved paths. - A moved closure could capture stale policy or split registration/refresh ownership. The new modules hold no mutable state. Policy is read through the existing callback, and state stays in the service. - The local fixture proves real HTTP and database behavior. It does not claim a live third-party provider sign-in. - No schema, API, authorization policy, or persisted-data migration changes are intended. ## Model Used OpenAI Codex, GPT-6 family, with code execution and repository tools. The exact serving model ID and context-window size are not exposed in this session. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip <noreply@paperclip.ing>
127 lines
5.5 KiB
TypeScript
127 lines
5.5 KiB
TypeScript
|
|
|
|
/**
|
|
* Upstream OAuth error redaction (PAP-17108).
|
|
*
|
|
* A generic remote MCP connection points at an arbitrary authorization server,
|
|
* so everything that server says about a failure is attacker-chosen: `error`,
|
|
* `error_description`, `error_uri`, and the response body. Paperclip surfaces
|
|
* connection failures to the operator through API responses, board UI copy,
|
|
* audit rows and logs, so reflecting any of that text would let a hostile
|
|
* provider plant secrets, ANSI escapes, or instructions ("paste your recovery
|
|
* key here") into Paperclip's own voice.
|
|
*
|
|
* The rule is therefore: the operator only ever reads text Paperclip authored.
|
|
* The provider's `error` code survives — as a *label* in structured `details`,
|
|
* never in a message — and only when it is one of the codes the RFCs define,
|
|
* because a label is still untrusted input. Everything else is dropped, and an
|
|
* unrecognized code collapses to `unrecognized` rather than being echoed.
|
|
*
|
|
* `error_description` and the response body are never read at all: no call site
|
|
* below parses them, which is what keeps a future edit from quietly
|
|
* reintroducing the reflection.
|
|
*/
|
|
const OAUTH_PROVIDER_ERROR_CODES = new Set([
|
|
// RFC 6749 §4.1.2.1 — authorization endpoint (the callback-denial path).
|
|
"access_denied",
|
|
"invalid_request",
|
|
"invalid_scope",
|
|
"server_error",
|
|
"temporarily_unavailable",
|
|
"unauthorized_client",
|
|
"unsupported_response_type",
|
|
// RFC 6749 §5.2 — token endpoint (authorization-code and refresh exchanges).
|
|
"invalid_client",
|
|
"invalid_grant",
|
|
"unsupported_grant_type",
|
|
// RFC 7591 §3.2.2 — dynamic client registration.
|
|
"invalid_client_metadata",
|
|
"invalid_redirect_uri",
|
|
"invalid_software_statement",
|
|
"unapproved_software_statement",
|
|
// OpenID Connect Core §3.1.2.6 — interactive re-authentication prompts.
|
|
"account_selection_required",
|
|
"consent_required",
|
|
"interaction_required",
|
|
"login_required",
|
|
]);
|
|
|
|
/** What an `error` that is absent, malformed, or off the allowlist becomes. */
|
|
const UNRECOGNIZED_OAUTH_PROVIDER_ERROR = "unrecognized";
|
|
|
|
const MAX_OAUTH_PROVIDER_ERROR_LENGTH = 64;
|
|
|
|
const OAUTH_PROVIDER_ERROR_PATTERN = /^[a-z0-9_-]+$/;
|
|
|
|
/**
|
|
* Stable, Paperclip-authored operator copy for each allowlisted provider error.
|
|
* Deliberately keyed on the code alone: the calling context is already carried
|
|
* by the Paperclip `code` in `details`, so one table serves the callback,
|
|
* token-exchange and registration paths without any of them composing a message
|
|
* out of provider text.
|
|
*/
|
|
const OAUTH_PROVIDER_ERROR_MESSAGES: Record<string, string> = {
|
|
access_denied: "The authorization server denied the request.",
|
|
account_selection_required:
|
|
"The authorization server needs an account to be selected. Try connecting again.",
|
|
consent_required:
|
|
"The authorization server needs consent to be granted. Try connecting again.",
|
|
interaction_required:
|
|
"The authorization server needs to be signed in to interactively. Try connecting again.",
|
|
invalid_client: "The authorization server rejected Paperclip's OAuth client.",
|
|
invalid_client_metadata:
|
|
"The authorization server rejected Paperclip's client registration details.",
|
|
invalid_grant:
|
|
"The authorization server rejected the authorization code or refresh token.",
|
|
invalid_redirect_uri:
|
|
"The authorization server rejected Paperclip's callback URL.",
|
|
invalid_request:
|
|
"The authorization server rejected the request as malformed.",
|
|
invalid_scope: "The authorization server rejected the requested permissions.",
|
|
invalid_software_statement:
|
|
"The authorization server rejected Paperclip's client registration details.",
|
|
login_required:
|
|
"The authorization server needs to be signed in to. Try connecting again.",
|
|
server_error:
|
|
"The authorization server reported an internal error. Try again shortly.",
|
|
temporarily_unavailable:
|
|
"The authorization server is temporarily unavailable. Try again shortly.",
|
|
unapproved_software_statement:
|
|
"The authorization server rejected Paperclip's client registration details.",
|
|
unauthorized_client:
|
|
"The authorization server refused to authorize Paperclip's OAuth client.",
|
|
unsupported_grant_type:
|
|
"The authorization server does not support the grant Paperclip uses.",
|
|
unsupported_response_type:
|
|
"The authorization server does not support the sign-in flow Paperclip uses.",
|
|
};
|
|
|
|
/**
|
|
* Reduce a provider-supplied `error` to a bounded, allowlisted label safe to
|
|
* keep in structured `details`. Returns `null` only when the provider sent no
|
|
* `error` at all, so the caller can tell "silent failure" from "said something
|
|
* Paperclip does not recognize".
|
|
*/
|
|
export function normalizeOAuthProviderError(value: unknown): string | null {
|
|
if (typeof value !== "string" || value.length === 0) return null;
|
|
// Bound length and character class before the allowlist even though
|
|
// membership implies both: these limits are what keeps the label safe if the
|
|
// allowlist above ever grows a pattern-matched entry.
|
|
if (value.length > MAX_OAUTH_PROVIDER_ERROR_LENGTH)
|
|
return UNRECOGNIZED_OAUTH_PROVIDER_ERROR;
|
|
if (!OAUTH_PROVIDER_ERROR_PATTERN.test(value))
|
|
return UNRECOGNIZED_OAUTH_PROVIDER_ERROR;
|
|
return OAUTH_PROVIDER_ERROR_CODES.has(value)
|
|
? value
|
|
: UNRECOGNIZED_OAUTH_PROVIDER_ERROR;
|
|
}
|
|
|
|
/** Paperclip's own message for a provider failure, never the provider's. */
|
|
export function oauthProviderErrorMessage(
|
|
providerError: string | null,
|
|
fallback: string,
|
|
): string {
|
|
if (!providerError) return fallback;
|
|
return OAUTH_PROVIDER_ERROR_MESSAGES[providerError] ?? fallback;
|
|
}
|