mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-02 16:58:52 +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>
312 lines
10 KiB
TypeScript
312 lines
10 KiB
TypeScript
import type { OAuthConnectionSettings } from "./types.js";
|
|
import { remoteEndpoint } from "../connection-methods.js";
|
|
|
|
export function asRecord(value: unknown): Record<string, unknown> {
|
|
if (value && typeof value === "object" && !Array.isArray(value))
|
|
return value as Record<string, unknown>;
|
|
return {};
|
|
}
|
|
|
|
/**
|
|
* RFC 8414 §3.1 requires the well-known path to be *inserted between* the
|
|
* issuer host and its path component, but a large amount of deployed software
|
|
* only serves the naive suffix form. OpenID Connect Discovery 1.0 in turn
|
|
* specifies the suffix form. Try all of them for an issuer that has a path, and
|
|
* the plain origin form when it doesn't.
|
|
*/
|
|
export function wellKnownMetadataUrls(issuer: string): string[] {
|
|
let parsed: URL;
|
|
try {
|
|
parsed = new URL(issuer);
|
|
} catch {
|
|
return [];
|
|
}
|
|
const suffixes = ["oauth-authorization-server", "openid-configuration"];
|
|
const path = parsed.pathname.replace(/\/+$/, "");
|
|
const urls: string[] = [];
|
|
for (const suffix of suffixes) {
|
|
if (path) {
|
|
// RFC 8414: https://host/.well-known/<suffix><path>
|
|
urls.push(
|
|
new URL(`/.well-known/${suffix}${path}`, parsed.origin).toString(),
|
|
);
|
|
// OIDC Discovery / widely deployed: https://host<path>/.well-known/<suffix>
|
|
urls.push(
|
|
new URL(`${path}/.well-known/${suffix}`, parsed.origin).toString(),
|
|
);
|
|
}
|
|
urls.push(new URL(`/.well-known/${suffix}`, parsed.origin).toString());
|
|
}
|
|
return [...new Set(urls)];
|
|
}
|
|
|
|
/**
|
|
* Protected-resource metadata lives at `/.well-known/oauth-protected-resource`
|
|
* with the resource's path appended (RFC 9728 §3.1). Probe the path-aware form
|
|
* first so a multi-tenant host that serves several MCP servers resolves to the
|
|
* right one, then fall back to the origin form.
|
|
*/
|
|
export function protectedResourceMetadataUrls(endpoint: URL): string[] {
|
|
const path = endpoint.pathname.replace(/\/+$/, "");
|
|
const urls: string[] = [];
|
|
if (path)
|
|
urls.push(
|
|
new URL(
|
|
`/.well-known/oauth-protected-resource${path}`,
|
|
endpoint.origin,
|
|
).toString(),
|
|
);
|
|
urls.push(
|
|
new URL(
|
|
"/.well-known/oauth-protected-resource",
|
|
endpoint.origin,
|
|
).toString(),
|
|
);
|
|
return [...new Set(urls)];
|
|
}
|
|
|
|
/**
|
|
* Canonical RFC 8707 resource indicator for an MCP endpoint: origin + path, with
|
|
* query, fragment and any trailing slash removed. Vendor query parameters belong
|
|
* to the connection config, not to the token audience.
|
|
*/
|
|
export function canonicalResourceIndicator(endpoint: string): string | null {
|
|
try {
|
|
const parsed = new URL(endpoint);
|
|
const path = parsed.pathname.replace(/\/+$/, "");
|
|
return `${parsed.origin}${path}`;
|
|
} catch {
|
|
return null;
|
|
}
|
|
}
|
|
|
|
/**
|
|
* Two issuers are the same authorization server only when scheme, host, port and
|
|
* path match exactly (a trailing slash is not significant). Used for `iss`
|
|
* validation on the callback and for detecting that a stored registration is
|
|
* bound to a different server than the one we just discovered.
|
|
*/
|
|
export function sameOAuthIssuer(
|
|
a: string | null | undefined,
|
|
b: string | null | undefined,
|
|
): boolean {
|
|
if (!a || !b) return false;
|
|
try {
|
|
const left = new URL(a);
|
|
const right = new URL(b);
|
|
return (
|
|
left.protocol === right.protocol &&
|
|
left.host === right.host &&
|
|
left.pathname.replace(/\/+$/, "") === right.pathname.replace(/\/+$/, "")
|
|
);
|
|
} catch {
|
|
return false;
|
|
}
|
|
}
|
|
|
|
export function originOf(value: string | null | undefined): string | null {
|
|
if (!value) return null;
|
|
try {
|
|
return new URL(value).origin;
|
|
} catch {
|
|
return null;
|
|
}
|
|
}
|
|
|
|
export function oauthConfig(connection: Pick<OAuthConnectionSettings, "config">) {
|
|
const oauth = asRecord(connection.config).oauth
|
|
? asRecord(asRecord(connection.config).oauth)
|
|
: {};
|
|
const {
|
|
access_token: _accessToken,
|
|
refresh_token: _refreshToken,
|
|
accessToken: _camelAccessToken,
|
|
refreshToken: _camelRefreshToken,
|
|
...metadata
|
|
} = oauth;
|
|
return metadata;
|
|
}
|
|
|
|
export function normalizeOauthScopes(value: unknown): string[] {
|
|
if (Array.isArray(value))
|
|
return value.filter(
|
|
(item): item is string =>
|
|
typeof item === "string" && item.trim().length > 0,
|
|
);
|
|
if (typeof value === "string")
|
|
return value
|
|
.split(/\s+/)
|
|
.map((item) => item.trim())
|
|
.filter(Boolean);
|
|
return [];
|
|
}
|
|
|
|
/**
|
|
* Decide what a token grant actually carries, keeping the provider's assertion separate from
|
|
* our own request.
|
|
*
|
|
* RFC 6749 §5.1 permits a provider to omit `scope` only when the grant is identical to the
|
|
* request, so reading the request as the grant is the specified fallback. A provider that
|
|
* over-grants *and* omits `scope` breaks that contract, and recording the request unmarked
|
|
* turns what we asked for into a confident-looking record of what we got. `scopeSource` keeps
|
|
* the two readable apart so nothing downstream can mistake an inference for an assertion.
|
|
*
|
|
* `unrequestedScopes` is only meaningful when we asked for something specific; with no
|
|
* requested scopes there is no baseline to compare against, so it stays empty rather than
|
|
* flagging every scope on connections that keep their scopes in provider-side app config.
|
|
*
|
|
* `previous` is the provenance already recorded for this grant. A refresh response that omits
|
|
* `scope` asserts nothing at all — least of all that a permission was taken away — so the
|
|
* earlier record is carried forward verbatim instead of being overwritten with a clean-looking
|
|
* inference. Only a fresh provider assertion replaces a recorded over-grant.
|
|
*/
|
|
export function resolveGrantedOauthScopes(input: {
|
|
tokenScope: unknown;
|
|
requestedScopes: unknown;
|
|
previous?: {
|
|
scopes?: unknown;
|
|
scopeSource?: unknown;
|
|
unrequestedScopes?: unknown;
|
|
};
|
|
}): {
|
|
scopes: string[];
|
|
scopeSource: "provider" | "requested_fallback";
|
|
unrequestedScopes: string[];
|
|
} {
|
|
const requested = normalizeOauthScopes(input.requestedScopes);
|
|
const asserted =
|
|
input.tokenScope === undefined || input.tokenScope === null
|
|
? []
|
|
: normalizeOauthScopes(input.tokenScope);
|
|
if (asserted.length === 0) {
|
|
const previousScopes = normalizeOauthScopes(input.previous?.scopes);
|
|
const previousUnrequested = normalizeOauthScopes(
|
|
input.previous?.unrequestedScopes,
|
|
);
|
|
if (previousScopes.length > 0) {
|
|
return {
|
|
scopes: previousScopes,
|
|
scopeSource:
|
|
input.previous?.scopeSource === "provider"
|
|
? "provider"
|
|
: "requested_fallback",
|
|
unrequestedScopes: previousUnrequested,
|
|
};
|
|
}
|
|
return {
|
|
scopes: requested,
|
|
scopeSource: "requested_fallback",
|
|
unrequestedScopes: previousUnrequested,
|
|
};
|
|
}
|
|
return {
|
|
scopes: asserted,
|
|
scopeSource: "provider",
|
|
unrequestedScopes:
|
|
requested.length === 0
|
|
? []
|
|
: asserted.filter((scope) => !requested.includes(scope)),
|
|
};
|
|
}
|
|
|
|
export function isSmokeLabOAuthFixture(
|
|
connection: Pick<OAuthConnectionSettings, "config">,
|
|
) {
|
|
const config = asRecord(connection.config);
|
|
const oauth = oauthConfig(connection);
|
|
return (
|
|
config.smokeLabFixture === "oauth-http" && oauth.smokeLabFixture === true
|
|
);
|
|
}
|
|
|
|
export function isSmokeLabOAuthUrl(value: string | null | undefined) {
|
|
if (!value) return false;
|
|
try {
|
|
return /\/smoke-lab\/oauth(?:\/|$)/.test(new URL(value).pathname);
|
|
} catch {
|
|
return false;
|
|
}
|
|
}
|
|
|
|
export function oauthProviderForConnection(
|
|
connection: Pick<OAuthConnectionSettings, "config">,
|
|
metadataUrl?: string | null,
|
|
): string {
|
|
const oauth = oauthConfig(connection);
|
|
if (typeof oauth.provider === "string" && oauth.provider.trim())
|
|
return oauth.provider.trim();
|
|
const sourceTemplateKey =
|
|
typeof connection.config.sourceTemplateKey === "string"
|
|
? connection.config.sourceTemplateKey.trim()
|
|
: "";
|
|
if (sourceTemplateKey) return sourceTemplateKey;
|
|
const url = metadataUrl ?? remoteEndpoint(connection.config);
|
|
try {
|
|
return (
|
|
new URL(url).hostname
|
|
.replace(/[^a-z0-9]+/gi, "_")
|
|
.replace(/^_+|_+$/g, "")
|
|
.toLowerCase() || "generic"
|
|
);
|
|
} catch {
|
|
return "generic";
|
|
}
|
|
}
|
|
|
|
export function parseWwwAuthenticateParams(value: string): Record<string, string> {
|
|
const params: Record<string, string> = {};
|
|
const input = value.replace(/^\s*Bearer\s+/i, "");
|
|
const re = /([a-zA-Z_][a-zA-Z0-9_-]*)=(?:"([^"]*)"|([^,\s]+))/g;
|
|
let match: RegExpExecArray | null;
|
|
while ((match = re.exec(input))) {
|
|
params[match[1]!.toLowerCase()] = match[2] ?? match[3] ?? "";
|
|
}
|
|
return params;
|
|
}
|
|
|
|
export function challengeOAuthHints(wwwAuthenticate: string) {
|
|
const params = parseWwwAuthenticateParams(wwwAuthenticate);
|
|
return {
|
|
metadataUrl:
|
|
params.resource_metadata ??
|
|
params.resource_metadata_url ??
|
|
params.metadata_url ??
|
|
null,
|
|
authorizationUrl:
|
|
params.authorization_uri ?? params.authorization_url ?? null,
|
|
tokenUrl: params.token_uri ?? params.token_url ?? null,
|
|
scope: params.scope ?? null,
|
|
};
|
|
}
|
|
|
|
/**
|
|
* Candidate authorization-server metadata URLs advertised by protected-resource
|
|
* metadata, paired with the issuer that advertised them so the caller can bind
|
|
* the resulting client material to a canonical issuer.
|
|
*/
|
|
export function authServerMetadataUrls(
|
|
metadata: Record<string, unknown>,
|
|
): Array<{ issuer: string; metadataUrl: string }> {
|
|
const candidates: Array<{ issuer: string; metadataUrl: string }> = [];
|
|
const issuers: string[] = [];
|
|
if (Array.isArray(metadata.authorization_servers)) {
|
|
for (const server of metadata.authorization_servers) {
|
|
if (typeof server === "string" && server.trim())
|
|
issuers.push(server.trim());
|
|
}
|
|
}
|
|
if (typeof metadata.issuer === "string" && metadata.issuer.trim())
|
|
issuers.push(metadata.issuer.trim());
|
|
for (const issuer of [...new Set(issuers)]) {
|
|
for (const metadataUrl of wellKnownMetadataUrls(issuer)) {
|
|
candidates.push({ issuer, metadataUrl });
|
|
}
|
|
}
|
|
const seen = new Set<string>();
|
|
return candidates.filter((candidate) => {
|
|
if (seen.has(candidate.metadataUrl)) return false;
|
|
seen.add(candidate.metadataUrl);
|
|
return true;
|
|
});
|
|
}
|