mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-29 21:07:40 +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>
435 lines
16 KiB
TypeScript
435 lines
16 KiB
TypeScript
import { HttpError, unprocessable } from "../../../errors.js";
|
|
import { asRecord, originOf } from "./metadata.js";
|
|
import { normalizeOAuthProviderError, oauthProviderErrorMessage } from "./provider-errors.js";
|
|
import type { OAuthProviderEndpoints, OAuthTokenEndpointAuthMethod, OAuthTransport } from "./types.js";
|
|
|
|
const MAX_OAUTH_DCR_CLIENT_ID_LENGTH = 4_096;
|
|
|
|
const MAX_OAUTH_DCR_CLIENT_SECRET_LENGTH = 16_384;
|
|
|
|
function invalidOAuthDcrResponse(field: string, reason: string): HttpError {
|
|
return new HttpError(
|
|
502,
|
|
"OAuth provider returned incompatible dynamic client metadata",
|
|
{
|
|
code: "oauth_dcr_response_invalid",
|
|
field,
|
|
reason,
|
|
},
|
|
);
|
|
}
|
|
|
|
function parseOAuthDcrString(
|
|
record: Record<string, unknown>,
|
|
field: "client_id" | "client_secret",
|
|
input: { required: boolean; maxLength: number },
|
|
): string | null {
|
|
const value = record[field];
|
|
if (value === undefined || value === null) {
|
|
if (input.required) throw invalidOAuthDcrResponse(field, "missing");
|
|
return null;
|
|
}
|
|
if (
|
|
typeof value !== "string" ||
|
|
value.length === 0 ||
|
|
value.length > input.maxLength
|
|
) {
|
|
throw invalidOAuthDcrResponse(field, "invalid_string");
|
|
}
|
|
if (field === "client_id" && value.trim() !== value) {
|
|
throw invalidOAuthDcrResponse(field, "invalid_string");
|
|
}
|
|
return value;
|
|
}
|
|
|
|
function assertOAuthDcrArray(
|
|
record: Record<string, unknown>,
|
|
field: "redirect_uris" | "grant_types" | "response_types",
|
|
expected: string[],
|
|
options: { allowAdditional?: boolean; allowOmitted?: boolean } = {},
|
|
) {
|
|
if (record[field] === undefined) {
|
|
if (options.allowOmitted) return;
|
|
throw invalidOAuthDcrResponse(field, "missing");
|
|
}
|
|
const value = record[field];
|
|
if (
|
|
!Array.isArray(value) ||
|
|
value.length < expected.length ||
|
|
value.length > 32 ||
|
|
value.some(
|
|
(entry) =>
|
|
typeof entry !== "string" ||
|
|
entry.length === 0 ||
|
|
entry.length > 2_048,
|
|
)
|
|
) {
|
|
throw invalidOAuthDcrResponse(field, "invalid_array");
|
|
}
|
|
const actual = new Set(value);
|
|
if (
|
|
expected.some((entry) => !actual.has(entry)) ||
|
|
(!options.allowAdditional && actual.size !== expected.length)
|
|
) {
|
|
throw invalidOAuthDcrResponse(field, "registered_value_mismatch");
|
|
}
|
|
}
|
|
|
|
function parseOAuthDcrTimestamp(
|
|
record: Record<string, unknown>,
|
|
field: string,
|
|
): number | null {
|
|
const value = record[field];
|
|
if (value === undefined || value === null) return null;
|
|
if (
|
|
typeof value !== "number" ||
|
|
!Number.isSafeInteger(value) ||
|
|
value < 0
|
|
) {
|
|
throw invalidOAuthDcrResponse(field, "invalid_timestamp");
|
|
}
|
|
return value;
|
|
}
|
|
|
|
function selectOAuthDcrTokenEndpointAuthMethod(
|
|
supported: string[] | undefined,
|
|
): OAuthTokenEndpointAuthMethod {
|
|
if (!supported?.length || supported.includes("none")) return "none";
|
|
// When a confidential client is required, preserve the provider's advertised
|
|
// ordering. Miro advertises both methods but only completes its MCP exchange
|
|
// for the first one (`client_secret_post`), while Supabase and Hugging Face
|
|
// advertise `client_secret_basic` first. Treating Basic as a global preference
|
|
// creates a valid-looking registration that fails only after user consent.
|
|
for (const method of supported) {
|
|
if (method === "client_secret_basic" || method === "client_secret_post")
|
|
return method;
|
|
}
|
|
throw unprocessable(
|
|
"OAuth provider does not support a compatible dynamic client authentication method",
|
|
{
|
|
code: "oauth_dcr_client_auth_unsupported",
|
|
supportedMethods: supported,
|
|
},
|
|
);
|
|
}
|
|
|
|
export function storedOAuthTokenEndpointAuthMethod(
|
|
oauth: Record<string, unknown>,
|
|
clientSecret: string | null | undefined,
|
|
): OAuthTokenEndpointAuthMethod {
|
|
const method = oauth.clientTokenEndpointAuthMethod;
|
|
if (
|
|
method === "none" ||
|
|
method === "client_secret_basic" ||
|
|
method === "client_secret_post"
|
|
) {
|
|
return method;
|
|
}
|
|
const advertised = Array.isArray(oauth.tokenEndpointAuthMethodsSupported)
|
|
? oauth.tokenEndpointAuthMethodsSupported.filter(
|
|
(value): value is string => typeof value === "string",
|
|
)
|
|
: [];
|
|
// Manual OAuth clients do not carry a DCR-selected method. Follow the
|
|
// authorization server's advertised preference when it offers a
|
|
// confidential-client method; Xero, for example, documents Basic auth and
|
|
// rejects an otherwise valid code when the secret is posted in the body.
|
|
if (clientSecret && advertised.includes("client_secret_basic"))
|
|
return "client_secret_basic";
|
|
if (clientSecret && advertised.includes("client_secret_post"))
|
|
return "client_secret_post";
|
|
// Existing manually configured and preconfigured clients predate the
|
|
// persisted method field and already use client_secret_post successfully.
|
|
return clientSecret ? "client_secret_post" : "none";
|
|
}
|
|
|
|
export function createOAuthClientProtocol({ fetchRemoteHttpUrl, assertOAuthEndpointUrl }: OAuthTransport) {
|
|
async function requestOAuthClientRegistration(input: { endpoints: OAuthProviderEndpoints; redirectUri: string }) {
|
|
if (!input.endpoints.registrationUrl) {
|
|
throw unprocessable(
|
|
"OAuth provider does not advertise dynamic client registration",
|
|
{
|
|
code: "oauth_dcr_not_supported",
|
|
},
|
|
);
|
|
}
|
|
if (
|
|
input.endpoints.codeChallengeMethodsSupported?.length &&
|
|
!input.endpoints.codeChallengeMethodsSupported.includes("S256")
|
|
) {
|
|
throw unprocessable(
|
|
"OAuth provider does not support the required PKCE S256 method",
|
|
{
|
|
code: "oauth_pkce_s256_required",
|
|
},
|
|
);
|
|
}
|
|
const tokenEndpointAuthMethod = selectOAuthDcrTokenEndpointAuthMethod(
|
|
input.endpoints.tokenEndpointAuthMethodsSupported,
|
|
);
|
|
|
|
const host = new URL(input.redirectUri).host;
|
|
const clientNameHost = host
|
|
.replace(/[^A-Za-z0-9-]/g, "-")
|
|
.replace(/-+/g, "-")
|
|
.replace(/^-|-$/g, "");
|
|
const requestedMetadata = {
|
|
// Some DCR servers restrict client_name to alphanumeric characters,
|
|
// hyphens and spaces. Keep the deployment host recognizable without
|
|
// sending punctuation such as dots, colons or parentheses.
|
|
client_name: `Paperclip ${clientNameHost}`,
|
|
redirect_uris: [input.redirectUri],
|
|
grant_types: [
|
|
"authorization_code",
|
|
...(!input.endpoints.grantTypesSupported?.length ||
|
|
input.endpoints.grantTypesSupported.includes("refresh_token")
|
|
? ["refresh_token"]
|
|
: []),
|
|
],
|
|
response_types: ["code"],
|
|
token_endpoint_auth_method: tokenEndpointAuthMethod,
|
|
// RFC 7591: Paperclip's callback is a server-side HTTPS endpoint, so this
|
|
// is a `web` client, not a `native` one. Some authorization servers reject
|
|
// an https redirect URI when the default (`web`) is left implicit, and
|
|
// others apply native-client redirect rules without it.
|
|
application_type: "web",
|
|
};
|
|
const response = await fetchRemoteHttpUrl(
|
|
assertOAuthEndpointUrl("registration", input.endpoints.registrationUrl),
|
|
{
|
|
method: "POST",
|
|
headers: { "content-type": "application/json" },
|
|
body: JSON.stringify(requestedMetadata),
|
|
},
|
|
);
|
|
const record = asRecord(
|
|
(await response.json().catch(() => ({}))) as unknown,
|
|
);
|
|
if (!response.ok) {
|
|
const providerError = normalizeOAuthProviderError(record.error);
|
|
throw new HttpError(
|
|
502,
|
|
oauthProviderErrorMessage(
|
|
providerError,
|
|
"OAuth dynamic client registration failed",
|
|
),
|
|
{
|
|
code: "oauth_dynamic_client_registration_failed",
|
|
providerError,
|
|
status: response.status,
|
|
},
|
|
);
|
|
}
|
|
const clientId = parseOAuthDcrString(record, "client_id", {
|
|
required: true,
|
|
maxLength: MAX_OAUTH_DCR_CLIENT_ID_LENGTH,
|
|
})!;
|
|
const clientSecret = parseOAuthDcrString(record, "client_secret", {
|
|
required: tokenEndpointAuthMethod !== "none",
|
|
maxLength: MAX_OAUTH_DCR_CLIENT_SECRET_LENGTH,
|
|
});
|
|
// Some authorization servers add provider-owned metadata to the registered
|
|
// client. Paperclip still uses only the exact redirect, grant and response
|
|
// types it requested, so accept bounded supersets while requiring every
|
|
// requested value to remain present. Hugging Face, for example, adds the
|
|
// device-code grant to an otherwise valid authorization-code registration.
|
|
assertOAuthDcrArray(
|
|
record,
|
|
"redirect_uris",
|
|
requestedMetadata.redirect_uris,
|
|
{ allowAdditional: true },
|
|
);
|
|
// RFC 7591 registration responses do not consistently echo every accepted
|
|
// request field. Supabase, for example, returns only the client material and
|
|
// redirect URIs. Redirect binding remains mandatory; omitted grant/response
|
|
// metadata inherits the values Paperclip requested. Additional provider-owned
|
|
// values do not widen Paperclip's behavior because they are never persisted as
|
|
// a flow choice or sent in authorization/token requests.
|
|
assertOAuthDcrArray(record, "grant_types", requestedMetadata.grant_types, {
|
|
allowAdditional: true,
|
|
allowOmitted: true,
|
|
});
|
|
assertOAuthDcrArray(
|
|
record,
|
|
"response_types",
|
|
requestedMetadata.response_types,
|
|
{
|
|
allowAdditional: true,
|
|
allowOmitted: true,
|
|
},
|
|
);
|
|
if (
|
|
record.token_endpoint_auth_method !== undefined &&
|
|
record.token_endpoint_auth_method !==
|
|
requestedMetadata.token_endpoint_auth_method
|
|
) {
|
|
throw invalidOAuthDcrResponse(
|
|
"token_endpoint_auth_method",
|
|
"registered_value_mismatch",
|
|
);
|
|
}
|
|
const clientIdIssuedAt = parseOAuthDcrTimestamp(
|
|
record,
|
|
"client_id_issued_at",
|
|
);
|
|
const returnedClientSecretExpiresAt = parseOAuthDcrTimestamp(
|
|
record,
|
|
"client_secret_expires_at",
|
|
);
|
|
// A few public-client registrars (including Mixpanel) return the RFC 7591
|
|
// `0` sentinel even though they issued no secret. It carries no credential
|
|
// lifetime in that case, so normalize it away. A positive expiry without a
|
|
// secret is still contradictory and remains a hard failure.
|
|
if (
|
|
returnedClientSecretExpiresAt !== null &&
|
|
returnedClientSecretExpiresAt > 0 &&
|
|
clientSecret === null
|
|
) {
|
|
throw invalidOAuthDcrResponse(
|
|
"client_secret_expires_at",
|
|
"client_secret_missing",
|
|
);
|
|
}
|
|
const clientSecretExpiresAt = clientSecret
|
|
? returnedClientSecretExpiresAt
|
|
: null;
|
|
return { clientId, clientSecret, tokenEndpointAuthMethod, clientIdIssuedAt, clientSecretExpiresAt };
|
|
}
|
|
|
|
async function exchangeOAuthToken(input: {
|
|
tokenUrl: string;
|
|
clientId: string;
|
|
clientSecret?: string | null;
|
|
tokenEndpointAuthMethod?: OAuthTokenEndpointAuthMethod;
|
|
grantType?: "authorization_code" | "refresh_token" | "client_credentials";
|
|
scopes?: string[];
|
|
redirectUri?: string | null;
|
|
codeVerifier?: string | null;
|
|
code?: string | null;
|
|
refreshToken?: string | null;
|
|
/** RFC 8707 resource indicator: the MCP server this token is for. */
|
|
resource?: string | null;
|
|
}) {
|
|
const body = new URLSearchParams();
|
|
if (input.grantType === "client_credentials") {
|
|
body.set("grant_type", "client_credentials");
|
|
if (input.scopes && input.scopes.length > 0)
|
|
body.set("scope", input.scopes.join(" "));
|
|
} else if (input.refreshToken) {
|
|
body.set("grant_type", "refresh_token");
|
|
body.set("refresh_token", input.refreshToken);
|
|
} else {
|
|
body.set("grant_type", "authorization_code");
|
|
body.set("code", input.code ?? "");
|
|
body.set("redirect_uri", input.redirectUri ?? "");
|
|
body.set("code_verifier", input.codeVerifier ?? "");
|
|
}
|
|
const tokenEndpointAuthMethod =
|
|
input.tokenEndpointAuthMethod ??
|
|
(input.clientSecret ? "client_secret_post" : "none");
|
|
if (tokenEndpointAuthMethod !== "client_secret_basic")
|
|
body.set("client_id", input.clientId);
|
|
if (tokenEndpointAuthMethod === "client_secret_post") {
|
|
if (!input.clientSecret) {
|
|
throw unprocessable(
|
|
"OAuth client secret is missing for client_secret_post authentication",
|
|
{
|
|
code: "oauth_client_secret_missing",
|
|
},
|
|
);
|
|
}
|
|
body.set("client_secret", input.clientSecret);
|
|
}
|
|
// RFC 8707: repeat the resource indicator on the token request so the
|
|
// authorization server audience-restricts the access token (and any refresh
|
|
// exchange) to this MCP server.
|
|
if (input.resource) body.set("resource", input.resource);
|
|
|
|
// The token URL can come from a connection row written before the endpoint
|
|
// gate existed, so a client secret / authorization code never leaves
|
|
// Paperclip without re-checking the transport it would leave over.
|
|
const tokenUrl = assertOAuthEndpointUrl("token", input.tokenUrl, {
|
|
// Paperclip's own callback origin, so a first-party token endpoint keeps
|
|
// working on a deployment that is itself served over plaintext HTTP.
|
|
firstPartyOrigin: originOf(input.redirectUri),
|
|
});
|
|
const headers: Record<string, string> = {
|
|
"content-type": "application/x-www-form-urlencoded",
|
|
// Some providers otherwise return a legacy form-encoded token response.
|
|
accept: "application/json",
|
|
};
|
|
if (tokenEndpointAuthMethod === "client_secret_basic") {
|
|
if (!input.clientSecret) {
|
|
throw unprocessable(
|
|
"OAuth client secret is missing for client_secret_basic authentication",
|
|
{
|
|
code: "oauth_client_secret_missing",
|
|
},
|
|
);
|
|
}
|
|
const formEncode = (value: string) =>
|
|
new URLSearchParams({ value }).toString().slice("value=".length);
|
|
headers.Authorization = `Basic ${Buffer.from(
|
|
`${formEncode(input.clientId)}:${formEncode(input.clientSecret)}`,
|
|
"utf8",
|
|
).toString("base64")}`;
|
|
}
|
|
const response = await fetchRemoteHttpUrl(tokenUrl, {
|
|
method: "POST",
|
|
headers,
|
|
body,
|
|
});
|
|
const payload = (await response.json().catch(() => ({}))) as unknown;
|
|
const record = asRecord(payload);
|
|
if (!response.ok || record.ok === false) {
|
|
const providerError = normalizeOAuthProviderError(record.error);
|
|
const message = oauthProviderErrorMessage(
|
|
providerError,
|
|
"OAuth token exchange failed",
|
|
);
|
|
if (
|
|
input.grantType === "refresh_token" &&
|
|
providerError === "invalid_grant"
|
|
) {
|
|
throw new HttpError(
|
|
422,
|
|
"OAuth authorization has expired. Reconnect this app to continue.",
|
|
{
|
|
code: "oauth_reauthorization_required",
|
|
providerError,
|
|
status: response.status,
|
|
},
|
|
);
|
|
}
|
|
throw new HttpError(502, message, {
|
|
code: "oauth_token_exchange_failed",
|
|
providerError,
|
|
status: response.status,
|
|
});
|
|
}
|
|
const accessToken =
|
|
typeof record.access_token === "string" ? record.access_token : null;
|
|
if (!accessToken)
|
|
throw new HttpError(
|
|
502,
|
|
"OAuth provider did not return an access token",
|
|
{ code: "oauth_access_token_missing" },
|
|
);
|
|
const expiresIn =
|
|
typeof record.expires_in === "number"
|
|
? record.expires_in
|
|
: Number(record.expires_in);
|
|
return {
|
|
accessToken,
|
|
refreshToken:
|
|
typeof record.refresh_token === "string" ? record.refresh_token : null,
|
|
expiresIn: Number.isFinite(expiresIn) && expiresIn > 0 ? expiresIn : null,
|
|
scope: typeof record.scope === "string" ? record.scope : null,
|
|
tokenType:
|
|
typeof record.token_type === "string" ? record.token_type : "Bearer",
|
|
};
|
|
}
|
|
|
|
return { requestOAuthClientRegistration, exchangeOAuthToken };
|
|
}
|