mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-02 02:07:25 +08:00
fix(tools): treat OAuth sign-in challenges as client errors (#13786)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Connected apps can require OAuth sign-in before they list their tools. > - Remote discovery recognizes this condition as `oauth_challenge`. > - Discovery and catalog refresh currently return it as HTTP 502. > - The server error handler reports that response as a crash. > - This pull request returns HTTP 422 for the explicit sign-in challenge. > - The operator keeps the sign-in instructions, while unexpected upstream failures remain reportable. ## Linked Issues or Issue Description **What happened?** Connecting a remote MCP app that answers with a recognized OAuth challenge returns HTTP 502. Reading an empty catalog or explicitly refreshing it does the same. The server error handler then sends the expected sign-in condition to error monitoring. **Expected behavior** A known sign-in requirement returns HTTP 422 with the existing `oauth_challenge` code, message, and setup/reconnect links. An unexplained upstream HTTP 400 or an unavailable upstream service still returns 502 and reaches error monitoring. **Steps to reproduce** 1. Configure a remote MCP app that returns HTTP 401 with a Bearer challenge. 2. Connect the app, read its empty catalog, or request a catalog refresh. 3. Observe HTTP 502 and a server error report before this change. **Paperclip version or commit** Reproduced on `6de50ba594b15efaa3eae6ed869cd39b3a436456`. **Deployment mode** Server with remote MCP connections. The regression coverage uses local PostgreSQL and mocked upstream HTTP responses. Related: #9750 addresses MCP initialization and session recovery. It does not change the classification of this recognized sign-in condition. Targeted searches found no duplicate classification PR. ## What Changed - Return 422 for `oauth_challenge` from discovery and from catalog health-error normalization. - Preserve the existing structured error and remediation links. - Test automatic empty-catalog reads and explicit refreshes. Verify that OAuth challenges produce no Sentry capture and that upstream 400/503 failures still do. - Update the direct-connect and blocked-redirect expectations and document the monitoring behavior. ## Verification - Before the fix, three sign-in route regressions fail with 502 instead of 422; all four upstream-error controls pass. - After the fix, all 339 tool-access and error-handler tests pass, including authorization and redirect protections. - These suites ran against disposable Homebrew PostgreSQL 16.14 through the existing test-constructor seam. The temporary setup and config remain outside the repository. CI uses the ordinary embedded PostgreSQL setup. - Direct server `tsc --noEmit` passes. - Full build and recursive typecheck were attempted; the Runner Rust step cannot run because `cargo` is absent on this machine. - Full `pnpm test:run`: 8,211 passed, 14 failed, 4,759 skipped. The 36 failed files match the existing embedded PostgreSQL startup/cleanup and macOS runtime-cache `EACCES` limitations. The changed database-backed service suite passed separately with local PostgreSQL. - Greptile: 5/5 with no unresolved comments. Seven CI workers received a simultaneous shutdown signal; the failed jobs are being retried through the normal workflow. Other completed checks passed. ## Risks Low risk. Clients now receive 422 instead of 502 for the explicit `oauth_challenge` condition. The code, message, and remediation links remain available. No permissions, credential handling, OAuth discovery rules, retry policy, or schema change. Other upstream failures retain their existing behavior. ## Model Used OpenAI GPT-6 via Codex, with reasoning, repository inspection, code editing, and test execution. The session does not expose an exact model snapshot or context-window size. ## 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 (339 service and error-handler tests; full workspace limitations are documented above) - [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 - [ ] 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>
This commit is contained in:
@@ -479,6 +479,10 @@ These fields contain build identifiers; they add no tenant or user identity.
|
||||
|
||||
- A Zod validation error, which answers 400.
|
||||
- Each `HttpError` below status 500, such as 401, 403, 404, 409, and 422.
|
||||
- A remote app's recognized OAuth sign-in challenge. Connecting an app or
|
||||
refreshing its catalog returns 422 with `oauth_challenge` and the existing
|
||||
setup/reconnect links. Other upstream failures still return 502 and are
|
||||
reported, including an unexplained upstream HTTP 400.
|
||||
- A performance trace and a profile, because `tracesSampleRate` is 0.
|
||||
|
||||
### Operator responsibilities
|
||||
|
||||
@@ -85,6 +85,7 @@ import {
|
||||
} from "../services/tool-gateway.js";
|
||||
import { toolAccessRoutes } from "../routes/tool-access.js";
|
||||
import { errorHandler } from "../middleware/index.js";
|
||||
import * as sentry from "../sentry.js";
|
||||
import type { VercelConnectClient } from "../services/vercel-connect.js";
|
||||
import { invalidatePaperclipCloudConnectorCapabilities, type PaperclipCloudConnector } from "../services/paperclip-cloud-connector.js";
|
||||
|
||||
@@ -13758,7 +13759,7 @@ describeEmbeddedPostgres("tool access service", () => {
|
||||
name: "Sign-in app",
|
||||
});
|
||||
|
||||
expect(res.status).toBe(502);
|
||||
expect(res.status).toBe(422);
|
||||
expect(res.body).toMatchObject({
|
||||
error: "This app needs you to sign in.",
|
||||
details: expect.objectContaining({ code: "oauth_challenge" }),
|
||||
@@ -13767,6 +13768,77 @@ describeEmbeddedPostgres("tool access service", () => {
|
||||
await expect(db.select().from(toolConnections)).resolves.toHaveLength(0);
|
||||
});
|
||||
|
||||
it.each([
|
||||
["catalog", 401, 'Bearer realm="app"', 422, false],
|
||||
["catalog/refresh", 401, 'Bearer realm="app"', 422, false],
|
||||
["catalog", 400, null, 502, true],
|
||||
["catalog/refresh", 400, null, 502, true],
|
||||
["catalog", 503, null, 502, true],
|
||||
["catalog/refresh", 503, null, 502, true],
|
||||
] as const)(
|
||||
"classifies %s upstream HTTP %i without hiding provider failures",
|
||||
async (path, upstreamStatus, challenge, expectedStatus, reportable) => {
|
||||
const company = await createCompany(db);
|
||||
const [application] = await db
|
||||
.insert(toolApplications)
|
||||
.values({
|
||||
companyId: company.id,
|
||||
applicationKey: `catalog-status-${randomUUID()}`,
|
||||
name: "Catalog status fixture",
|
||||
type: "mcp_http",
|
||||
status: "active",
|
||||
})
|
||||
.returning();
|
||||
const [connection] = await db
|
||||
.insert(toolConnections)
|
||||
.values({
|
||||
companyId: company.id,
|
||||
applicationId: application!.id,
|
||||
name: "Catalog status fixture",
|
||||
uid: `test/${randomUUID()}`,
|
||||
transport: "mcp_remote",
|
||||
status: "draft",
|
||||
enabled: false,
|
||||
config: { url: "https://catalog-status.example.test/mcp" },
|
||||
transportConfig: { url: "https://catalog-status.example.test/mcp" },
|
||||
credentialSecretRefs: [],
|
||||
})
|
||||
.returning();
|
||||
vi.spyOn(globalThis, "fetch").mockImplementation(async () =>
|
||||
new Response(JSON.stringify({ error: "upstream request rejected" }), {
|
||||
status: upstreamStatus,
|
||||
headers: challenge ? { "www-authenticate": challenge } : {},
|
||||
}),
|
||||
);
|
||||
const capture = vi
|
||||
.spyOn(sentry, "captureException")
|
||||
.mockImplementation(() => {});
|
||||
const app = createRouteApp(db);
|
||||
const url = `/api/tool-connections/${connection!.id}/${path}`;
|
||||
const res = await (path === "catalog"
|
||||
? request(app).get(url)
|
||||
: request(app).post(url));
|
||||
|
||||
expect(res.status).toBe(expectedStatus);
|
||||
if (challenge) {
|
||||
expect(res.body).toMatchObject({
|
||||
error: "This app needs you to sign in.",
|
||||
code: "oauth_challenge",
|
||||
details: {
|
||||
code: "oauth_challenge",
|
||||
setupUrl: expect.any(String),
|
||||
reconnectUrl: expect.any(String),
|
||||
},
|
||||
});
|
||||
}
|
||||
expect(capture).toHaveBeenCalledTimes(reportable ? 1 : 0);
|
||||
await expect(
|
||||
db.select().from(toolCatalogEntries)
|
||||
.where(eq(toolCatalogEntries.connectionId, connection!.id)),
|
||||
).resolves.toHaveLength(0);
|
||||
},
|
||||
);
|
||||
|
||||
it.each([
|
||||
[
|
||||
"local_trusted",
|
||||
@@ -13833,7 +13905,7 @@ describeEmbeddedPostgres("tool access service", () => {
|
||||
.post(`/api/companies/${company.id}/tools/apps/connect`)
|
||||
.send({ link: "https://8.8.8.8/mcp", name: "Redirect OAuth MCP" });
|
||||
|
||||
expect(res.status).toBe(502);
|
||||
expect(res.status).toBe(422);
|
||||
expect(fetchMock).toHaveBeenCalledWith(
|
||||
"https://8.8.8.8/.well-known/oauth-protected-resource",
|
||||
expect.objectContaining({ redirect: "manual" }),
|
||||
|
||||
@@ -2803,6 +2803,7 @@ function healthFailureHttpStatus(failure: {
|
||||
code: string;
|
||||
}): number {
|
||||
if (failure.status === "missing_secret") return 422;
|
||||
if (failure.code === "oauth_challenge") return 422;
|
||||
if (failure.code === "user_authorization_required") return 422;
|
||||
if (failure.code === "composio_broker_retired") return 422;
|
||||
if (failure.code === "tool_connection_transport_unsupported") return 422;
|
||||
@@ -6903,7 +6904,7 @@ export function toolAccessService(
|
||||
})
|
||||
.where(eq(toolConnections.id, connection.id));
|
||||
}
|
||||
throw new HttpError(502, "This app needs you to sign in.", {
|
||||
throw unprocessable("This app needs you to sign in.", {
|
||||
code: "oauth_challenge",
|
||||
status: response.status,
|
||||
setupUrl: connectionSetupUrl(connection),
|
||||
|
||||
Reference in New Issue
Block a user