diff --git a/.env.example b/.env.example index d20e31ac1..f910b6a28 100644 --- a/.env.example +++ b/.env.example @@ -136,23 +136,22 @@ HUB_STATIC_DIR=../web/dist # in-room connect-github card) — a SEPARATE OAuth App from GITHUB_CLIENT_ID # above, which only signs people in to Workbench itself. Create a second # OAuth app at https://github.com/settings/developers, with an -# authorization callback URL of -# /api/tenants/:tenantId/connections/github/callback and the -# "repo" scope. Leave both unset and Connect GitHub falls back to a -# guided personal-access-token paste — no dead end either way. NOTE: as of -# CL-6386, `@workbench/connections`' generic oauth-pkce/oauth-code route -# factory (`createOAuthConnectRoutes`) is exported but not yet mounted in -# `apps/hub` — see CL-6386's PR report. Setting these two vars registers -# the app and flips the Plugins UI over to it, but the redirect itself -# will not complete until that mount lands (a pre-existing gap that also -# affects OpenRouter/Hugging Face's own hosted-connect buttons). +# authorization callback URL of /api/tenants/ — GitHub matches +# any redirect under that prefix, and the flow's actual callback is the +# tenant-scoped /api/tenants//connections/oauth/github/callback +# (CL-6394). The connect asks for the "repo" scope. Leave both unset and +# Connect GitHub falls back to a guided personal-access-token paste — no +# dead end either way. # GITHUB_APP_CLIENT_ID= # GITHUB_APP_CLIENT_SECRET= # Onboarding's Hugging Face connect card (a public OAuth app — no # secret): create one at https://huggingface.co/settings/applications/new # with a redirect URI of /api/onboarding/oauth/huggingface/callback -# and scope "openid inference-api". See +# (first-login onboarding) — the settings/plugins Connect buttons ride +# the tenant-scoped /api/tenants//connections/oauth/huggingface/callback +# instead (CL-6394), so register that shape too if your HF app supports +# it — and scope "openid inference-api". See # docs/onboarding-huggingface-connect.md for the full setup. Leave unset # and Hugging Face stays available only as a paste-a-token provider card. # HUGGINGFACE_OAUTH_CLIENT_ID= diff --git a/apps/hub/src/index.ts b/apps/hub/src/index.ts index c77256ceb..744f6dd0a 100644 --- a/apps/hub/src/index.ts +++ b/apps/hub/src/index.ts @@ -1879,7 +1879,7 @@ export async function createHub(config: HubConfig) { // through to `createTenantConnectCredential`. app.route( `${TENANT_PREFIX}/connections/oauth`, - createOAuthConnectRoutes({ + createOAuthConnectRoutes({ hubUrl: config.baseUrl, log: (line) => log.info`${line}`, credentialCipher, @@ -3121,10 +3121,6 @@ export async function createHub(config: HubConfig) { onboardingDeps.seedModel = config.seedModel; if (config.huggingfaceOAuthClientId !== undefined) onboardingDeps.huggingfaceClientId = config.huggingfaceOAuthClientId; - if (config.githubAppClientId !== undefined) - onboardingDeps.githubAppClientId = config.githubAppClientId; - if (config.githubAppClientSecret !== undefined) - onboardingDeps.githubAppClientSecret = config.githubAppClientSecret; app.route("/api/onboarding", createOnboardingRoutes(onboardingDeps)); diff --git a/apps/web/src/app.css b/apps/web/src/app.css index 96177a33f..c76c18915 100644 --- a/apps/web/src/app.css +++ b/apps/web/src/app.css @@ -65,13 +65,6 @@ select:disabled, cursor: not-allowed; } -.app-mark { - height: 1.375rem; - width: auto; - flex-shrink: 0; - color: var(--primary); -} - .app-boot-frame { position: relative; display: flex; @@ -1828,13 +1821,6 @@ select:disabled, cursor: pointer; } -.app-wordmark { - font-size: 0.875rem; - font-weight: 600; - letter-spacing: -0.01em; - white-space: nowrap; -} - /* Onboarding: full-screen guided wizard, gated above the shell entirely (see app.tsx) so nothing implying an existing workbench — sidebar, bench dock — is ever on screen while it runs. Same two-column shape as diff --git a/packages/connections/package.json b/packages/connections/package.json index 95fab694f..4595df7ef 100644 --- a/packages/connections/package.json +++ b/packages/connections/package.json @@ -8,6 +8,7 @@ "exports": { ".": "./src/index.ts", "./registry": "./src/registry.ts", + "./persist-credential": "./src/persist-credential.ts", "./plugins": "./src/plugins.ts", "./provider-health": "./src/provider-health.ts", "./mcp-presets": "./src/mcp-presets.ts" diff --git a/packages/connections/src/index.ts b/packages/connections/src/index.ts index 846f5ca83..8ed710757 100644 --- a/packages/connections/src/index.ts +++ b/packages/connections/src/index.ts @@ -79,6 +79,12 @@ export { createTenantConnectCredential, type CreateTenantConnectCredentialDeps, } from "./oauth-tenant-connect"; +export { + isInferenceProvider, + persistConnectorCredential, + type PersistConnectorCredentialArgs, + type PersistConnectorCredentialFns, +} from "./persist-credential"; export { CONNECT_STATE_TTL_MS as OPENROUTER_CONNECT_STATE_TTL_MS, exchangeCodeForKey, diff --git a/packages/connections/src/oauth-routes.ts b/packages/connections/src/oauth-routes.ts index eaf1cdbca..5ae50909c 100644 --- a/packages/connections/src/oauth-routes.ts +++ b/packages/connections/src/oauth-routes.ts @@ -138,7 +138,7 @@ export type OAuthStoreOutcome = | { readonly kind: "invalid-credential"; readonly message: string } | { readonly kind: "no-personal-bench" }; -export type CreateOAuthConnectRoutesDeps = { +export type CreateOAuthConnectRoutesDeps = { readonly hubUrl: string; readonly log: (line: string) => void; /** Seals the PKCE+state cookie parked between `/start` and @@ -156,13 +156,14 @@ export type CreateOAuthConnectRoutesDeps = { * Required: without a caller-supplied store step, a successful * exchange would have nowhere to land. */ readonly connectCredential: (args: { - /** Given so a tenant-scoped caller (mounted inside the platform's - * tenant middleware, same as `afterConnected` below) can read - * `c.get("tenant")`/`c.get("principal")` directly instead of - * re-deriving them — see `createTenantConnectCredential` in + /** Typed by the factory's own env parameter, so a tenant-scoped + * caller (`E = TenantEnv`, mounted inside the platform's tenant + * middleware) reads `c.get("tenant")`/`c.get("principal")` directly + * with no cast — see `createTenantConnectCredential` in * `./oauth-tenant-connect.ts`. A caller with no tenant middleware - * (`packages/onboarding`'s own mount) is free to ignore it. */ - c: Context; + * (`packages/onboarding`'s own mount, `E = AppEnv`) is free to + * ignore it. */ + c: Context; connectorId: string; userId: string; userEmail: string; @@ -185,7 +186,7 @@ export type CreateOAuthConnectRoutesDeps = { * pending-seed sealing lives here, entirely outside this package. * Given the Hono `Context` directly so it can set its own cookie. */ readonly afterConnected?: (args: { - c: Context; + c: Context; connectorId: string; userId: string; apiKey: string; @@ -207,10 +208,10 @@ export type CreateOAuthConnectRoutesDeps = { const CONNECT_STATE_TTL_MS = 10 * 60 * 1000; const CONNECT_START_RATE_LIMIT_MS = 10_000; -export function createOAuthConnectRoutes( - deps: CreateOAuthConnectRoutesDeps, -): Hono { - const app = new Hono(); +export function createOAuthConnectRoutes( + deps: CreateOAuthConnectRoutesDeps, +): Hono { + const app = new Hono(); const registry = deps.registry ?? CONNECTOR_REGISTRY; const oauthEnv = deps.oauthEnv ?? {}; const defaultReturnPath = deps.defaultReturnPath ?? "/onboarding"; diff --git a/packages/connections/src/oauth-tenant-connect.ts b/packages/connections/src/oauth-tenant-connect.ts index c27b5e1b5..9a1a88de7 100644 --- a/packages/connections/src/oauth-tenant-connect.ts +++ b/packages/connections/src/oauth-tenant-connect.ts @@ -7,87 +7,52 @@ // inside the platform's tenant middleware — the same one // `createConnectionRoutes` and `createMcpOAuthRoutes` (#115) run // inside — so `c.get("tenant")`/`c.get("principal")` are already -// resolved and this never re-derives a tenant of its own. +// resolved, typed by the factory's own `TenantEnv` parameter rather +// than a cast. // -// Persists exactly the way `routes.ts`'s `POST /:connectorId/complete` -// does: `ensureProvider` + `ensureCredential`, then `seedCatalog` for an -// inference connector so a just-connected provider's models are -// launchable immediately, not just stored. The credential is already -// proven by the OAuth exchange itself, so there is no separate probe -// step here (unlike `/complete`'s pasted-key path, which has nothing -// else vouching for the secret). -import type { Context } from "hono"; +// Persists through the one shared sequence every connect surface runs +// (`./persist-credential.ts`): provider + credential rows always, the +// curated model catalog only for an inference connector — a +// non-inference connector (GitHub) stores its token and stops there. +// The credential is already proven by the OAuth exchange itself, so +// there is no separate probe step here (unlike `/complete`'s pasted-key +// path, which has nothing else vouching for the secret). import type { TenantEnv } from "@intx/hub-api"; -import { - createHubAPI, - ensureCredential, - ensureProvider, - PROVIDER_TEST_CONFIG, - seedCatalog, - type ApiCall, - type EnsureCredentialArgs, - type EnsureProviderArgs, - type SeedCatalogArgs, - type SupportedCredentialProvider, -} from "@workbench/hub-client"; +import { createHubAPI } from "@workbench/hub-client"; import type { ConnectorDescriptor } from "./descriptor"; import type { ProviderHealthStore } from "./provider-health"; import { CONNECTOR_REGISTRY } from "./registry"; import type { CreateOAuthConnectRoutesDeps } from "./oauth-routes"; +import { + persistConnectorCredential, + type PersistConnectorCredentialFns, +} from "./persist-credential"; -export type CreateTenantConnectCredentialDeps = { - readonly hubUrl: string; - readonly log: (line: string) => void; - /** Test-only override, matching every other route factory here. */ - readonly registry?: Readonly>; - /** Cleared on a successful connect, same store `createConnectionRoutes`' - * `/complete` and `GET /provider-health` share (CL-6092). */ - readonly providerHealth?: ProviderHealthStore; - /** Test-only override, matching `routes.ts`'s own seam — lets this - * module's own test prove the persist/seed sequencing without - * reaching for module mocking or a real hub HTTP server. */ - readonly ensureProviderFn?: ( - api: ApiCall, - cookies: string[], - args: EnsureProviderArgs, - log: (line: string) => void, - ) => ReturnType; - readonly ensureCredentialFn?: ( - api: ApiCall, - cookies: string[], - args: EnsureCredentialArgs, - log: (line: string) => void, - ) => ReturnType; - readonly seedCatalogFn?: ( - args: SeedCatalogArgs, - ) => ReturnType; -}; - -function isInferenceProvider(id: string): id is SupportedCredentialProvider { - return Object.hasOwn(PROVIDER_TEST_CONFIG, id); -} +export type CreateTenantConnectCredentialDeps = + PersistConnectorCredentialFns & { + readonly hubUrl: string; + readonly log: (line: string) => void; + /** Test-only override, matching every other route factory here. */ + readonly registry?: Readonly>; + /** Cleared on a successful connect, same store `createConnectionRoutes`' + * `/complete` and `GET /provider-health` share (CL-6092). */ + readonly providerHealth?: ProviderHealthStore; + }; /** - * Builds the `connectCredential` dep `createOAuthConnectRoutes` needs, - * scoped to whatever tenant the request's own middleware already - * resolved. `args.c` is cast to `Context` — safe only - * because this is wired exclusively into a mount reached through the - * platform's tenant middleware (see this module's own header); a caller - * mounting outside that middleware must not use this. + * Builds the `connectCredential` dep a `TenantEnv`-typed + * `createOAuthConnectRoutes` mount needs, scoped to whatever tenant the + * request's own middleware already resolved. */ export function createTenantConnectCredential( deps: CreateTenantConnectCredentialDeps, -): CreateOAuthConnectRoutesDeps["connectCredential"] { +): CreateOAuthConnectRoutesDeps["connectCredential"] { const api = createHubAPI(deps.hubUrl); const registry = deps.registry ?? CONNECTOR_REGISTRY; - const runEnsureProvider = deps.ensureProviderFn ?? ensureProvider; - const runEnsureCredential = deps.ensureCredentialFn ?? ensureCredential; - const runSeedCatalog = deps.seedCatalogFn ?? seedCatalog; return async (args) => { - const c = args.c as Context; - const tenant = c.get("tenant"); - const principal = c.get("principal"); + const tenant = args.c.get("tenant"); + const principal = args.c.get("principal"); const descriptor = registry[args.connectorId]; if (descriptor === undefined) { return { @@ -97,40 +62,27 @@ export function createTenantConnectCredential( } try { - const providerId = await runEnsureProvider( + const persistArgs: Parameters[0] = { api, - args.cookies, - { - tenantId: tenant.id, - name: descriptor.id, - plugin: descriptor.credentialPlugin, - }, - deps.log, - ); - await runEnsureCredential( - api, - args.cookies, - { - tenantId: tenant.id, - providerId, - name: descriptor.displayName, - secret: args.apiKey, - type: "api_key", - verified: true, - }, - deps.log, - ); - if (isInferenceProvider(descriptor.id)) { - await runSeedCatalog({ - api, - cookies: args.cookies, - tenantId: tenant.id, - log: deps.log, - provider: descriptor.id, - apiKey: args.apiKey, - credentialVerified: true, - }); - } + cookies: args.cookies, + tenantId: tenant.id, + descriptor, + secret: args.apiKey, + log: deps.log, + ...(args.credentialMetadata !== undefined + ? { credentialMetadata: args.credentialMetadata } + : {}), + ...(deps.ensureProviderFn !== undefined + ? { ensureProviderFn: deps.ensureProviderFn } + : {}), + ...(deps.ensureCredentialFn !== undefined + ? { ensureCredentialFn: deps.ensureCredentialFn } + : {}), + ...(deps.seedCatalogFn !== undefined + ? { seedCatalogFn: deps.seedCatalogFn } + : {}), + }; + await persistConnectorCredential(persistArgs); deps.providerHealth?.clear(tenant.id, descriptor.id); return { kind: "connected", diff --git a/packages/connections/src/persist-credential.test.ts b/packages/connections/src/persist-credential.test.ts new file mode 100644 index 000000000..eb53fb686 --- /dev/null +++ b/packages/connections/src/persist-credential.test.ts @@ -0,0 +1,170 @@ +// The shared persist-and-seed sequence (CL-6394): one place decides +// which connectors seed a model catalog — a non-inference connector +// (GitHub) must never reach `CATALOG_SEEDS`, the exact fall-through +// that crashed the hosted one-click connect when three parallel copies +// of this sequence disagreed. +import { describe, expect, test } from "bun:test"; +import type { + EnsureCredentialArgs, + EnsureProviderArgs, + SeedCatalogArgs, +} from "@workbench/hub-client"; +import type { ConnectorDescriptor } from "./descriptor"; +import { + isInferenceProvider, + persistConnectorCredential, +} from "./persist-credential"; +import { CONNECTOR_REGISTRY } from "./registry"; + +const noApi = () => { + throw new Error("the api must only be reached through the injected fns"); +}; + +function recordingFns() { + const providers: EnsureProviderArgs[] = []; + const credentials: EnsureCredentialArgs[] = []; + const seeds: SeedCatalogArgs[] = []; + return { + providers, + credentials, + seeds, + fns: { + ensureProviderFn: async ( + _api: unknown, + _cookies: string[], + args: EnsureProviderArgs, + ) => { + providers.push(args); + return `prv_${args.name}`; + }, + ensureCredentialFn: async ( + _api: unknown, + _cookies: string[], + args: EnsureCredentialArgs, + ) => { + credentials.push(args); + return `cred_${args.providerId}`; + }, + seedCatalogFn: async (args: SeedCatalogArgs) => { + seeds.push(args); + return { hasCompletionCapableModel: true }; + }, + }, + }; +} + +function descriptorOrThrow(id: string): ConnectorDescriptor { + const descriptor = CONNECTOR_REGISTRY[id]; + if (descriptor === undefined) throw new Error(`no descriptor for ${id}`); + return descriptor; +} + +describe("persistConnectorCredential", () => { + test("github (non-inference) persists provider + credential and never seeds a catalog", async () => { + const { providers, credentials, seeds, fns } = recordingFns(); + const result = await persistConnectorCredential({ + api: noApi as never, + cookies: [], + tenantId: "tnt_1", + descriptor: descriptorOrThrow("github"), + secret: "gho_exchanged-token", + log: () => undefined, + ...fns, + }); + + expect(providers).toEqual([ + { tenantId: "tnt_1", name: "github", plugin: "http" }, + ]); + expect(credentials).toEqual([ + { + tenantId: "tnt_1", + providerId: "prv_github", + name: "GitHub", + secret: "gho_exchanged-token", + type: "api_key", + verified: true, + }, + ]); + expect(seeds).toEqual([]); + expect(result.credentialId).toBe("cred_prv_github"); + }); + + test("an inference provider seeds its catalog under the same credential name", async () => { + const { credentials, seeds, fns } = recordingFns(); + await persistConnectorCredential({ + api: noApi as never, + cookies: [], + tenantId: "tnt_1", + descriptor: descriptorOrThrow("openrouter"), + secret: "sk-or-abc", + log: () => undefined, + ...fns, + }); + + expect(credentials).toHaveLength(1); + expect(seeds).toHaveLength(1); + expect(seeds[0]).toMatchObject({ + tenantId: "tnt_1", + provider: "openrouter", + apiKey: "sk-or-abc", + credentialName: "OpenRouter", + credentialType: "api_key", + credentialVerified: true, + }); + }); + + test("credential metadata types the row oauth_token and rides into the seed", async () => { + const { credentials, seeds, fns } = recordingFns(); + await persistConnectorCredential({ + api: noApi as never, + cookies: [], + tenantId: "tnt_1", + descriptor: descriptorOrThrow("huggingface"), + secret: "hf_token", + credentialMetadata: { expiresAt: "2026-09-01T00:00:00Z" }, + log: () => undefined, + ...fns, + }); + + expect(credentials[0]).toMatchObject({ + type: "oauth_token", + metadata: { expiresAt: "2026-09-01T00:00:00Z" }, + }); + expect(seeds[0]).toMatchObject({ + credentialType: "oauth_token", + credentialMetadata: { expiresAt: "2026-09-01T00:00:00Z" }, + }); + }); + + test("a url-kind connect stores the URL on the provider row and threads the base-URL seam", async () => { + const { providers, seeds, fns } = recordingFns(); + await persistConnectorCredential({ + api: noApi as never, + cookies: [], + tenantId: "tnt_1", + descriptor: descriptorOrThrow("ollama"), + secret: "placeholder-not-a-real-key", + baseURLOverride: "http://localhost:11434", + log: () => undefined, + ...fns, + }); + + expect(providers[0]).toMatchObject({ + name: "ollama", + apiBaseUrl: "http://localhost:11434", + }); + expect(seeds[0]).toMatchObject({ + provider: "ollama", + baseURLOverride: "http://localhost:11434", + }); + }); +}); + +describe("isInferenceProvider", () => { + test("splits inference providers from tool connectors", () => { + expect(isInferenceProvider("anthropic")).toBe(true); + expect(isInferenceProvider("openrouter")).toBe(true); + expect(isInferenceProvider("github")).toBe(false); + expect(isInferenceProvider("linear")).toBe(false); + }); +}); diff --git a/packages/connections/src/persist-credential.ts b/packages/connections/src/persist-credential.ts new file mode 100644 index 000000000..e00c5ca92 --- /dev/null +++ b/packages/connections/src/persist-credential.ts @@ -0,0 +1,174 @@ +// The one persist-and-seed sequence every connect surface runs once a +// secret is proven (by probe or by OAuth exchange): ensureProvider → +// ensureCredential → seedCatalog-if-inference. Before CL-6394 this +// sequence existed as three parallel copies (`routes.ts`'s +// `/:connectorId/complete`, `oauth-tenant-connect.ts`, and +// `@workbench/onboarding`'s `testAndPersistCredential`) and their +// divergence is exactly what let a GitHub callback fall into an +// inference-only `seedCatalog` — a non-inference connector must never +// reach `CATALOG_SEEDS`, and here that rule lives in one place. +// +// The provider row is named by the connector's lowercase `id` (the +// canonical name `credentialBindings` resolve against); the credential +// row carries the human-facing `displayName` — the exact name the +// Plugins gallery's resolver looks up. `seedCatalog` is passed that same +// `credentialName` so its own internal `ensureCredential` resolves to +// the row planted here rather than a second `-default` row. +import { + ensureCredential, + ensureProvider, + PROVIDER_TEST_CONFIG, + seedCatalog, + type ApiCall, + type EnsureCredentialArgs, + type EnsureProviderArgs, + type SeedCatalogArgs, + type SupportedCredentialProvider, +} from "@workbench/hub-client"; +import type { ConnectorDescriptor } from "./descriptor"; + +export function isInferenceProvider( + id: string, +): id is SupportedCredentialProvider { + return Object.hasOwn(PROVIDER_TEST_CONFIG, id); +} + +/** The test seams every route factory in this package already exposes, + * shared verbatim so a caller can thread its own overrides through. */ +export type PersistConnectorCredentialFns = { + readonly ensureProviderFn?: ( + api: ApiCall, + cookies: string[], + args: EnsureProviderArgs, + log: (line: string) => void, + ) => ReturnType; + readonly ensureCredentialFn?: ( + api: ApiCall, + cookies: string[], + args: EnsureCredentialArgs, + log: (line: string) => void, + ) => ReturnType; + readonly seedCatalogFn?: ( + args: SeedCatalogArgs, + ) => ReturnType; +}; + +export type PersistConnectorCredentialArgs = PersistConnectorCredentialFns & { + readonly api: ApiCall; + readonly cookies: string[]; + readonly tenantId: string; + readonly descriptor: ConnectorDescriptor; + /** The credential secret to store: a probed pasted key, an + * OAuth-exchanged token, or — for a url-kind connector (Ollama) — the + * fixed placeholder secret, with the instance URL in + * `baseURLOverride`. Always already proven by the caller, so the row + * is stored `verified: true`. */ + readonly secret: string; + /** Free-form data stored on the credential's `metadata` field — the + * extension point an expiring OAuth token's expiry lives in. Its + * presence is also what types the row `oauth_token` instead of + * `api_key`. */ + readonly credentialMetadata?: Record; + /** The instance origin a url-kind connector actually points at — + * stored as the provider row's `apiBaseUrl` and threaded into + * `seedCatalog`'s own base-URL seam. */ + readonly baseURLOverride?: string; + readonly log: (line: string) => void; +}; + +export async function persistConnectorCredential( + args: PersistConnectorCredentialArgs, +): Promise<{ + credentialId: string; + /** The catalog seed's own report (CL-6351's model-capability read + * included) — absent for a non-inference connector, which never + * seeds a catalog. */ + seedResult?: Awaited>; +}> { + const runEnsureProvider = args.ensureProviderFn ?? ensureProvider; + const runEnsureCredential = args.ensureCredentialFn ?? ensureCredential; + const runSeedCatalog = args.seedCatalogFn ?? seedCatalog; + const credentialType = + args.credentialMetadata !== undefined + ? ("oauth_token" as const) + : ("api_key" as const); + + const providerArgs: EnsureProviderArgs = + args.baseURLOverride !== undefined + ? { + tenantId: args.tenantId, + name: args.descriptor.id, + plugin: args.descriptor.credentialPlugin, + apiBaseUrl: args.baseURLOverride, + } + : { + tenantId: args.tenantId, + name: args.descriptor.id, + plugin: args.descriptor.credentialPlugin, + }; + const providerId = await runEnsureProvider( + args.api, + args.cookies, + providerArgs, + args.log, + ); + + const credentialArgs: EnsureCredentialArgs = + args.credentialMetadata !== undefined + ? { + tenantId: args.tenantId, + providerId, + name: args.descriptor.displayName, + secret: args.secret, + type: credentialType, + verified: true, + metadata: args.credentialMetadata, + } + : { + tenantId: args.tenantId, + providerId, + name: args.descriptor.displayName, + secret: args.secret, + type: credentialType, + verified: true, + }; + const credentialId = await runEnsureCredential( + args.api, + args.cookies, + credentialArgs, + args.log, + ); + + // An inference provider connected anywhere must become usable, not + // just stored: plant its curated model catalog so the models show up + // in Inference and a workbench can actually run on them. A + // non-inference connector (GitHub, Linear, ...) has no catalog seed + // and must never reach `CATALOG_SEEDS`. + if (isInferenceProvider(args.descriptor.id)) { + const seedArgs: SeedCatalogArgs = { + api: args.api, + cookies: args.cookies, + tenantId: args.tenantId, + log: args.log, + provider: args.descriptor.id, + apiKey: args.secret, + credentialName: args.descriptor.displayName, + credentialType, + credentialVerified: true, + // The row planted above is the one and only credential write — + // seedCatalog plants the catalog side against it instead of + // ensuring (and re-rotating) a row of its own. + existingCredentialId: credentialId, + }; + if (args.credentialMetadata !== undefined) { + seedArgs.credentialMetadata = args.credentialMetadata; + } + if (args.baseURLOverride !== undefined) { + seedArgs.baseURLOverride = args.baseURLOverride; + } + const seedResult = await runSeedCatalog(seedArgs); + return { credentialId, seedResult }; + } + + return { credentialId }; +} diff --git a/packages/connections/src/pkce.ts b/packages/connections/src/pkce.ts index 9e5db637f..0c6423138 100644 --- a/packages/connections/src/pkce.ts +++ b/packages/connections/src/pkce.ts @@ -61,7 +61,11 @@ export async function s256Challenge(codeVerifier: string): Promise { const ConnectStatePayload = type({ userId: "string > 0", - codeVerifier: "string > 0", + // Empty for a non-PKCE flow (GitHub's confidential-client web flow + // seals `codeVerifier: ""`), so this must accept the empty string — + // `string > 0` here silently expired every non-PKCE callback + // (CL-6394). + codeVerifier: "string", nonce: "string > 0", expiresAt: "number", }); diff --git a/packages/connections/src/routes.ts b/packages/connections/src/routes.ts index 300e39e98..2eebdcc58 100644 --- a/packages/connections/src/routes.ts +++ b/packages/connections/src/routes.ts @@ -27,15 +27,14 @@ import { ensureProvider, parseAs, OLLAMA_PLACEHOLDER_SECRET, - PROVIDER_TEST_CONFIG, seedCatalog, type ApiCall, - type SupportedCredentialProvider, type EnsureCredentialArgs, type EnsureProviderArgs, type SeedCatalogArgs, } from "@workbench/hub-client"; import type { ConnectorDescriptor } from "./descriptor"; +import { persistConnectorCredential } from "./persist-credential"; import type { ProviderHealthStore } from "./provider-health"; import { CONNECTOR_REGISTRY } from "./registry"; @@ -245,9 +244,6 @@ export function createConnectionRoutes( const app = new Hono(); const api = createHubAPI(deps.hubUrl); const registry = deps.registry ?? CONNECTOR_REGISTRY; - const runEnsureProvider = deps.ensureProviderFn ?? ensureProvider; - const runEnsureCredential = deps.ensureCredentialFn ?? ensureCredential; - const runSeedCatalog = deps.seedCatalogFn ?? seedCatalog; const runDisconnectConnector = deps.disconnectConnectorFn ?? disconnectConnector; @@ -297,10 +293,6 @@ export function createConnectionRoutes( }, ); - function isInferenceProvider(id: string): id is SupportedCredentialProvider { - return Object.hasOwn(PROVIDER_TEST_CONFIG, id); - } - function findApiKeyDescriptor(connectorId: string) { const descriptor = registry[connectorId]; if (descriptor === undefined || descriptor.probe === undefined) { @@ -364,64 +356,43 @@ export function createConnectionRoutes( // in the same wire field every other connector uses for a secret — // it stores the fixed placeholder secret instead, and the URL // itself as the provider row's `apiBaseUrl` (the same seam MCP - // servers use). + // servers use). The persist-and-seed sequence itself is the one + // shared `persistConnectorCredential` every connect surface runs + // (CL-6394). const isUrlCredential = descriptor.credentialInputKind === "url"; - const providerArgs: EnsureProviderArgs = isUrlCredential - ? { - tenantId: tenant.id, - name: descriptor.id, - plugin: descriptor.credentialPlugin, - apiBaseUrl: parsed.apiKey, - } - : { - tenantId: tenant.id, - name: descriptor.id, - plugin: descriptor.credentialPlugin, - }; try { - const providerId = await runEnsureProvider( + const { credentialId, seedResult } = await persistConnectorCredential({ api, cookies, - providerArgs, - deps.log, - ); - const credentialId = await runEnsureCredential( - api, - cookies, - { - tenantId: tenant.id, - providerId, - name: descriptor.displayName, - secret: isUrlCredential ? OLLAMA_PLACEHOLDER_SECRET : parsed.apiKey, - type: "api_key", - // `test` above already proved `parsed.apiKey` against - // `descriptor.probe`, so a name conflict here (a - // regenerated key, or a retry after a bad paste) is safe - // to rotate rather than silently keeping the stale secret. - verified: true, - }, - deps.log, - ); - // An inference provider connected here must become usable, not - // just stored: plant its curated model catalog (and Ollama's live - // model list) exactly the way onboarding does, so the models show - // up in Inference and a workbench can actually run on them. - let modelGuidance: string | undefined; - if (isInferenceProvider(descriptor.id)) { - const seeded = await runSeedCatalog({ - api, - cookies, - tenantId: tenant.id, - log: deps.log, - provider: descriptor.id, - apiKey: isUrlCredential ? OLLAMA_PLACEHOLDER_SECRET : parsed.apiKey, - credentialVerified: true, - ...(isUrlCredential ? { baseURLOverride: parsed.apiKey } : {}), - }); - if (descriptor.id === "ollama" && !seeded.hasCompletionCapableModel) { - modelGuidance = OLLAMA_NO_CHAT_MODEL_GUIDANCE; - } - } + tenantId: tenant.id, + descriptor, + // `test` above already proved `parsed.apiKey` against + // `descriptor.probe`, so a name conflict on the credential row + // (a regenerated key, or a retry after a bad paste) is safe to + // rotate rather than silently keeping the stale secret. + secret: isUrlCredential ? OLLAMA_PLACEHOLDER_SECRET : parsed.apiKey, + log: deps.log, + ...(isUrlCredential ? { baseURLOverride: parsed.apiKey } : {}), + ...(deps.ensureProviderFn !== undefined + ? { ensureProviderFn: deps.ensureProviderFn } + : {}), + ...(deps.ensureCredentialFn !== undefined + ? { ensureCredentialFn: deps.ensureCredentialFn } + : {}), + ...(deps.seedCatalogFn !== undefined + ? { seedCatalogFn: deps.seedCatalogFn } + : {}), + }); + // CL-6351: a fresh Ollama connect whose instance serves no + // completion-capable model gets guided copy, not a silent dead + // end — read off the catalog seed the shared persist sequence + // just ran. + const modelGuidance = + descriptor.id === "ollama" && + seedResult !== undefined && + !seedResult.hasCompletionCapableModel + ? OLLAMA_NO_CHAT_MODEL_GUIDANCE + : undefined; // Only clear once the credential is actually durable — a storage // failure below (the `catch`) must leave a prior needs-attention // record standing rather than clearing it on a test pass whose diff --git a/packages/connections/test/github-oauth-connect.test.ts b/packages/connections/test/github-oauth-connect.test.ts new file mode 100644 index 000000000..1abffcff6 --- /dev/null +++ b/packages/connections/test/github-oauth-connect.test.ts @@ -0,0 +1,213 @@ +// CL-6394 regression: the hosted GitHub one-click connect, driven +// through the exact tenant-scoped start URL the UI now emits +// (`@corbits/settings-ui`'s `oauthStartHref` — its output is pinned +// literally in that package's own tests, and repeated literally here so +// the two suites cannot drift apart silently). Before the fix, the only +// UI entry point targeted `/api/onboarding/oauth/github/start`, whose +// mount binary-dispatched openrouter/huggingface and sent github into +// inference-only seeding — a TypeError on `CATALOG_SEEDS["github"]` +// AFTER a successful token exchange. This proves the full chain the UI +// actually drives: start → GitHub callback → token exchange (against a +// fake exchange server) → credential persisted, and — the crash's exact +// shape — no catalog seeding for a non-inference connector. +import { describe, expect, test } from "bun:test"; +import { Hono } from "hono"; +import type { MiddlewareHandler } from "hono"; +import type { TenantEnv } from "@intx/hub-api"; +import { createNoopCredentialCipher } from "@intx/crypto"; +import type { + EnsureCredentialArgs, + EnsureProviderArgs, + SeedCatalogArgs, +} from "@workbench/hub-client"; +import type { ConnectorDescriptor } from "../src/descriptor"; +import { + exchangeCodeForGithubToken, + GITHUB_TOKEN_EXCHANGE_URL, +} from "../src/github-connect"; +import { + createOAuthConnectRoutes, + DEFAULT_RETURN_PATH_ALLOWLIST, +} from "../src/oauth-routes"; +import { createTenantConnectCredential } from "../src/oauth-tenant-connect"; +import { CONNECTOR_REGISTRY } from "../src/registry"; + +// What `oauthStartHref("tnt_1", "github", "/plugins")` renders into the +// plugins gallery's Connect link. +const UI_START_URL = + "/api/tenants/tnt_1/connections/oauth/github/start?return=%2Fplugins"; + +const TENANT = { + id: "tnt_1", + name: "Acme", + slug: "acme", + domain: "acme.example", + parentId: null, + config: null, + createdAt: new Date(), + updatedAt: new Date(), +}; + +const PRINCIPAL = { + id: "prn_alice", + tenantId: TENANT.id, + kind: "user" as const, + refId: "prn_alice", + status: "active" as const, + createdAt: new Date(), + updatedAt: new Date(), +}; + +function fakeGithubExchangeServer(exchangeBodies: unknown[]) { + return async ( + url: string, + init: { method: "POST"; headers: Record; body: string }, + ): Promise => { + expect(url).toBe(GITHUB_TOKEN_EXCHANGE_URL); + exchangeBodies.push(JSON.parse(init.body)); + return Response.json({ access_token: "gho_exchanged_token" }); + }; +} + +function mountHubShaped() { + const realGithub = CONNECTOR_REGISTRY["github"]; + if (realGithub?.oauth === undefined) { + throw new Error("registry is missing the github oauth entry"); + } + const exchangeBodies: unknown[] = []; + // The real descriptor, with its exchange pointed at the fake GitHub + // token server instead of github.com — same seam the descriptor's own + // `fetchImpl` exposes. + const github: ConnectorDescriptor = { + ...realGithub, + oauth: { + ...realGithub.oauth, + exchange: async ({ code, redirectUri, clientId, clientSecret }) => { + if (clientId === undefined || clientSecret === undefined) { + return { ok: false, message: "github app connect is not configured" }; + } + const result = await exchangeCodeForGithubToken({ + code, + redirectUri, + clientId, + clientSecret, + fetchImpl: fakeGithubExchangeServer(exchangeBodies), + }); + return result.ok ? { ok: true, apiKey: result.key } : result; + }, + }, + }; + + const providers: EnsureProviderArgs[] = []; + const credentials: EnsureCredentialArgs[] = []; + const seeds: SeedCatalogArgs[] = []; + const routes = createOAuthConnectRoutes({ + hubUrl: "https://bench.example.com", + log: () => undefined, + credentialCipher: createNoopCredentialCipher(), + registry: { github }, + oauthEnv: { + githubAppClientId: "iv_client_id", + githubAppClientSecret: "app-secret", + }, + connectCredential: createTenantConnectCredential({ + hubUrl: "https://bench.example.com", + log: () => undefined, + registry: { github }, + ensureProviderFn: async (_api, _cookies, args) => { + providers.push(args); + return `prv_${args.name}`; + }, + ensureCredentialFn: async (_api, _cookies, args) => { + credentials.push(args); + return `cred_${args.providerId}`; + }, + seedCatalogFn: async (args) => { + seeds.push(args); + return { hasCompletionCapableModel: true }; + }, + }), + defaultReturnPath: "/settings/connections", + returnPathAllowlist: [...DEFAULT_RETURN_PATH_ALLOWLIST, "/plugins"], + }); + + const asTenant: MiddlewareHandler = async (c, next) => { + c.set("user", { + id: "user_1", + email: "user_1@example.com", + } as never); + c.set("tenant", TENANT); + c.set("principal", PRINCIPAL); + await next(); + }; + const app = new Hono(); + app.use("*", asTenant); + app.route("/api/tenants/tnt_1/connections/oauth", routes); + return { app, providers, credentials, seeds, exchangeBodies }; +} + +function cookieHeaderFrom(response: Response): string { + return response.headers + .getSetCookie() + .map((sc) => sc.split(";")[0]) + .join("; "); +} + +describe("hosted GitHub one-click connect through the UI's start URL", () => { + test("start -> callback persists the exchanged token and lands back on /plugins", async () => { + const { app, providers, credentials, seeds, exchangeBodies } = + mountHubShaped(); + + const started = await app.request(UI_START_URL); + expect(started.status).toBe(302); + const authorizeUrl = new URL(started.headers.get("location") ?? ""); + expect(authorizeUrl.origin).toBe("https://github.com"); + expect(authorizeUrl.searchParams.get("client_id")).toBe("iv_client_id"); + expect(authorizeUrl.searchParams.get("redirect_uri")).toBe( + "https://bench.example.com/api/tenants/tnt_1/connections/oauth/github/callback", + ); + const state = authorizeUrl.searchParams.get("state") ?? ""; + expect(state).not.toBe(""); + const cookie = cookieHeaderFrom(started); + + // GitHub echoes `state` back on the callback (echoesState: true). + const callback = await app.request( + `/api/tenants/tnt_1/connections/oauth/github/callback?code=gh_code_1&state=${encodeURIComponent(state)}`, + { headers: { cookie } }, + ); + expect(callback.status).toBe(302); + const redirect = new URL( + callback.headers.get("location") ?? "", + "https://x", + ); + expect(redirect.pathname).toBe("/plugins"); + expect(redirect.searchParams.get("outcome")).toBe("connected"); + expect(redirect.searchParams.get("tenantSlug")).toBe(TENANT.slug); + + expect(exchangeBodies).toEqual([ + { + client_id: "iv_client_id", + client_secret: "app-secret", + code: "gh_code_1", + redirect_uri: + "https://bench.example.com/api/tenants/tnt_1/connections/oauth/github/callback", + }, + ]); + expect(providers).toEqual([ + { tenantId: TENANT.id, name: "github", plugin: "http" }, + ]); + expect(credentials).toEqual([ + { + tenantId: TENANT.id, + providerId: "prv_github", + name: "GitHub", + secret: "gho_exchanged_token", + type: "api_key", + verified: true, + }, + ]); + // The crash's exact shape: github has no catalog seed, so the + // persist sequence must never reach seedCatalog for it. + expect(seeds).toEqual([]); + }); +}); diff --git a/packages/hub-client/src/seed.ts b/packages/hub-client/src/seed.ts index 77ab0e75a..6cd87256a 100644 --- a/packages/hub-client/src/seed.ts +++ b/packages/hub-client/src/seed.ts @@ -1339,6 +1339,15 @@ export type SeedCatalogArgs = { * own. */ credentialVerified?: boolean; + /** + * A credential row the caller already planted (the shared + * persist-and-seed sequence, `@workbench/connections`' + * `persistConnectorCredential`). When set, this function plants only + * the catalog side — provider/credential ensure is skipped entirely, + * so the caller's single `ensureCredential` stays the one write (no + * second rotation PATCH against the same row). + */ + existingCredentialId?: string; /** * Overrides `CATALOG_SEEDS[provider].provider.baseURL` for this seed * run — the configurable-base-URL seam every other curated provider @@ -1424,7 +1433,41 @@ export async function seedCatalog( (args.placeholderCredential === true ? PLACEHOLDER_CATALOG_API_KEY : undefined); - if (credentialSecret === undefined) { + + async function plantCredential(secret: string): Promise { + const providerArgs = + provider === "ollama" + ? { + tenantId, + name: seed.provider.name, + plugin: seed.provider.plugin, + apiBaseUrl: providerBaseURL, + } + : { tenantId, name: seed.provider.name, plugin: seed.provider.plugin }; + const providerId = await ensureProvider(api, cookies, providerArgs, log); + const baseCredentialArgs = { + tenantId, + providerId, + name: args.credentialName ?? inferenceCredentialName(seed.provider.name), + secret, + type: args.credentialType ?? ("api_key" as const), + verified: args.credentialVerified ?? false, + }; + return ensureCredential( + api, + cookies, + args.credentialMetadata !== undefined + ? { ...baseCredentialArgs, metadata: args.credentialMetadata } + : baseCredentialArgs, + log, + ); + } + let credentialId: string; + if (args.existingCredentialId !== undefined) { + credentialId = args.existingCredentialId; + } else if (credentialSecret !== undefined) { + credentialId = await plantCredential(credentialSecret); + } else { log( `catalog models for ${seed.provider.name} seeded without a credential; ` + `no workbench or workflow can launch against them until a ${seed.provider.name} API key is set — set it in the hub's own environment and restart (the env-key auto-plant, CL-6101, then plants it with no other step), or set it here and re-run: workbench seed`, @@ -1437,33 +1480,6 @@ export async function seedCatalog( ), }; } - - const providerArgs = - provider === "ollama" - ? { - tenantId, - name: seed.provider.name, - plugin: seed.provider.plugin, - apiBaseUrl: providerBaseURL, - } - : { tenantId, name: seed.provider.name, plugin: seed.provider.plugin }; - const providerId = await ensureProvider(api, cookies, providerArgs, log); - const baseCredentialArgs = { - tenantId, - providerId, - name: args.credentialName ?? inferenceCredentialName(seed.provider.name), - secret: credentialSecret, - type: args.credentialType ?? ("api_key" as const), - verified: args.credentialVerified ?? false, - }; - const credentialId = await ensureCredential( - api, - cookies, - args.credentialMetadata !== undefined - ? { ...baseCredentialArgs, metadata: args.credentialMetadata } - : baseCredentialArgs, - log, - ); const catalogProviderId = await ensureCatalogProvider( api, cookies, diff --git a/packages/onboarding/src/complete-credential.ts b/packages/onboarding/src/complete-credential.ts index 667422d2d..b6211b2f4 100644 --- a/packages/onboarding/src/complete-credential.ts +++ b/packages/onboarding/src/complete-credential.ts @@ -58,18 +58,20 @@ import { isSidecarUnavailableError, ollamaOpenAICompatBaseURL, parseAs, - seedCatalog, seedTenant, - supportedCredentialProviders, type ApiCall, type ModelSource, - type SeedCatalogArgs, type SeedTenantArgs, type SupportedCredentialProvider, type ToolRegistryPublisher, type WorkflowPusher, } from "@workbench/hub-client"; import { preferCompletionCapable } from "@workbench/hub-client/model-capability"; +import { + persistConnectorCredential, + type PersistConnectorCredentialFns, +} from "@workbench/connections/persist-credential"; +import { CONNECTOR_REGISTRY } from "@workbench/connections/registry"; import { personalTenantSlug, seededWorkflowStatus } from "./provision"; /** The onboarding UI's copy for a partial seed: every durable step @@ -153,23 +155,23 @@ type CommonArgs = { log: (line: string) => void; }; -export type TestAndPersistCredentialArgs = CommonArgs & { - userId: string; - userEmail: string; - provider: SupportedCredentialProvider; - apiKey: string; - /** - * Free-form data stored on the credential's `metadata` field — the - * extension point an OAuth connect flow's token expiry lives in (see - * `huggingface-connect.ts`'s `exchangeCodeForToken`). Absent for a - * pasted key or a durable-key connect flow (OpenRouter). - */ - credentialMetadata?: Record; - /** The configurable-base-URL seam `ollama` uses (see `modelSourceFor`); - * ignored for every other provider. */ - baseURLOverride?: string; - seedCatalogFn?: (args: SeedCatalogArgs) => ReturnType; -}; +export type TestAndPersistCredentialArgs = CommonArgs & + PersistConnectorCredentialFns & { + userId: string; + userEmail: string; + provider: SupportedCredentialProvider; + apiKey: string; + /** + * Free-form data stored on the credential's `metadata` field — the + * extension point an OAuth connect flow's token expiry lives in (see + * `huggingface-connect.ts`'s `exchangeCodeForToken`). Absent for a + * pasted key or a durable-key connect flow (OpenRouter). + */ + credentialMetadata?: Record; + /** The configurable-base-URL seam `ollama` uses (see `modelSourceFor`); + * ignored for every other provider. */ + baseURLOverride?: string; + }; export type EnsureSeededArgs = CommonArgs & { tenant: PersonalTenant; @@ -179,34 +181,16 @@ export type EnsureSeededArgs = CommonArgs & { seedTenantFn?: (args: SeedTenantArgs) => ReturnType; }; -export type CompleteCredentialArgs = CommonArgs & { - userId: string; - userEmail: string; - provider: SupportedCredentialProvider; - apiKey: string; - credentialMetadata?: Record; - baseURLOverride?: string; - seedCatalogFn?: (args: SeedCatalogArgs) => ReturnType; - seedTenantFn?: (args: SeedTenantArgs) => ReturnType; -}; - -/** - * The exact name the Plugins gallery's resolver - * (`@workbench/connections/plugins`'s `resolveOne`) looks a credential up - * by: a connector's `descriptor.displayName`, itself sourced from this - * same `PROVIDER_TEST_CONFIG` table (see `packages/connections/src/ - * registry.ts`). Seeding the credential under any other name — the - * catalog-seed convention `inferenceCredentialName` still uses for the - * hub-owned CLI seed and the env-key auto-plant — leaves a self-served - * connect flow's credential invisible to that gallery. - */ -function credentialDisplayName(provider: SupportedCredentialProvider): string { - const match = supportedCredentialProviders().find((p) => p.id === provider); - if (match === undefined) { - throw new Error(`No display name registered for provider ${provider}`); - } - return match.displayName; -} +export type CompleteCredentialArgs = CommonArgs & + PersistConnectorCredentialFns & { + userId: string; + userEmail: string; + provider: SupportedCredentialProvider; + apiKey: string; + credentialMetadata?: Record; + baseURLOverride?: string; + seedTenantFn?: (args: SeedTenantArgs) => ReturnType; + }; export async function findPersonalTenant( api: ApiCall, @@ -405,41 +389,48 @@ export async function modelSourceFor( export async function testAndPersistCredential( args: TestAndPersistCredentialArgs, ): Promise { - const runSeedCatalog = args.seedCatalogFn ?? seedCatalog; - const expectedSlug = personalTenantSlug(args.userEmail, args.userId); const tenant = await findPersonalTenant(args.api, args.cookies, expectedSlug); if (!tenant) return { kind: "no-personal-bench" }; - const seedCatalogArgs = { + const descriptor = CONNECTOR_REGISTRY[args.provider]; + if (descriptor === undefined) { + throw new Error( + `no connector descriptor registered for provider ${args.provider}`, + ); + } + + // The one shared persist-and-seed sequence (CL-6394): provider + + // credential rows named the way the Plugins gallery's resolver reads + // them back (`descriptor.id` / `descriptor.displayName`), then the + // curated model catalog. An explicit user submission through a connect + // UI — a pasted key or a completed OAuth exchange — always rotates a + // name-conflicting credential (a regenerated key, or a retry after a + // bad paste): see `ensureCredential`'s own `verified` doc comment in + // `@workbench/hub-client`'s `seed.ts` for the full rotation rule. + await persistConnectorCredential({ api: args.api, cookies: args.cookies, tenantId: tenant.tenantId, - provider: args.provider, - apiKey: args.apiKey, + descriptor, + secret: args.apiKey, log: args.log, - credentialName: credentialDisplayName(args.provider), - credentialType: - args.credentialMetadata !== undefined - ? ("oauth_token" as const) - : ("api_key" as const), - // An explicit user submission through a connect UI — a pasted key or - // a completed OAuth exchange — always rotates a name-conflicting - // api_key credential (a regenerated key, or a retry after a bad - // paste), independent of whether the key was ever probed: see - // `ensureCredential`'s own `verified` doc comment in - // `@workbench/hub-client`'s `seed.ts` for the full rotation rule. - credentialVerified: true, - }; - const withMetadata = - args.credentialMetadata !== undefined - ? { ...seedCatalogArgs, credentialMetadata: args.credentialMetadata } - : seedCatalogArgs; - await runSeedCatalog( - args.baseURLOverride !== undefined - ? { ...withMetadata, baseURLOverride: args.baseURLOverride } - : withMetadata, - ); + ...(args.credentialMetadata !== undefined + ? { credentialMetadata: args.credentialMetadata } + : {}), + ...(args.baseURLOverride !== undefined + ? { baseURLOverride: args.baseURLOverride } + : {}), + ...(args.ensureProviderFn !== undefined + ? { ensureProviderFn: args.ensureProviderFn } + : {}), + ...(args.ensureCredentialFn !== undefined + ? { ensureCredentialFn: args.ensureCredentialFn } + : {}), + ...(args.seedCatalogFn !== undefined + ? { seedCatalogFn: args.seedCatalogFn } + : {}), + }); return { kind: "connected", ...tenant }; } diff --git a/packages/onboarding/src/routes.ts b/packages/onboarding/src/routes.ts index ebb4c8bb6..e91314d15 100644 --- a/packages/onboarding/src/routes.ts +++ b/packages/onboarding/src/routes.ts @@ -134,14 +134,6 @@ export type CreateOnboardingRoutesDeps = { exchange?: typeof exchangeHuggingFaceCodeForToken; connectCredential?: typeof testAndPersistCredential; }; - /** The GitHub OAuth App id/secret from github.com/settings/apps (see - * .env.example). Absent leaves the one-click GitHub connect path - * reporting `not_configured` — the PAT paste form stays available - * either way (CL-6386). Read here so this package's own `/oauth` - * mount decides the same thing `@workbench/connections`' tenant- - * scoped `GET .../oauth-configured` route already reports. */ - githubAppClientId?: string; - githubAppClientSecret?: string; /** Test seam for `POST /complete-setup`'s slow-path deploy step. */ ensureSeededFn?: typeof ensureSeeded; /** Test seam for `POST /complete`'s own success path — defaults to the @@ -555,8 +547,13 @@ export function createOnboardingRoutes( "@workbench/connections' registry is missing the huggingface oauth-pkce entry", ); } + // ONLY the two providers onboarding's own first-login flow offers. + // Every other OAuth-capable connector (the GitHub App connect + // included) belongs to the tenant-scoped `connections/oauth` mount in + // `apps/hub` — a `/oauth/github/start` here answers the factory's own + // 404, never a silent fall-through into onboarding's inference-only + // persistence (CL-6394). const oauthRegistry: Readonly> = { - ...CONNECTOR_REGISTRY, openrouter: { ...openrouterDescriptor, oauth: { @@ -573,6 +570,20 @@ export function createOnboardingRoutes( }, }; + /** Everything onboarding's own OAuth mount may ever persist for — + * enforced twice: `oauthRegistry` above keeps any other connector from + * even starting a flow here (a loud 404), and this narrowing refuses + * one that somehow reached persistence anyway, instead of an `as` + * cast letting it fall into inference-only seeding (CL-6394). */ + function onboardingOAuthProvider( + connectorId: string, + ): "openrouter" | "huggingface" | undefined { + if (connectorId === "openrouter" || connectorId === "huggingface") { + return connectorId; + } + return undefined; + } + /** The fast half only — persists the exchanged material, no probe, * never deploys a workflow. Dispatches to whichever provider's own * test-seam override (`deps.openrouterConnect`/`deps.huggingfaceConnect`) @@ -585,7 +596,13 @@ export function createOnboardingRoutes( apiKey: string; credentialMetadata?: Record; }): Promise { - const provider = args.connectorId as SupportedCredentialProvider; + const provider = onboardingOAuthProvider(args.connectorId); + if (provider === undefined) { + return { + kind: "invalid-credential", + message: `onboarding does not connect ${args.connectorId} — use the workbench's own Connections surface`, + }; + } const impl = provider === "openrouter" ? (deps.openrouterConnect?.connectCredential ?? @@ -620,10 +637,12 @@ export function createOnboardingRoutes( cookies: string[]; withinMs: number; }): Promise { + const provider = onboardingOAuthProvider(args.connectorId); + if (provider === undefined) return undefined; return recentlyConnectedCredential(api, args.cookies, { userId: args.userId, userEmail: args.userEmail, - provider: args.connectorId as SupportedCredentialProvider, + provider, withinMs: args.withinMs, log: deps.log, }); @@ -645,7 +664,12 @@ export function createOnboardingRoutes( principalId: string; tenantDomain: string; }): Promise { - const provider = args.connectorId as SupportedCredentialProvider; + const provider = onboardingOAuthProvider(args.connectorId); + if (provider === undefined) { + throw new Error( + `onboarding's pending-seed store only holds its own providers, not ${args.connectorId}`, + ); + } await deps.pendingSeedStore.put({ userId: args.userId, tenantId: args.tenantId, @@ -665,8 +689,6 @@ export function createOnboardingRoutes( registry: oauthRegistry, oauthEnv: { huggingfaceClientId: deps.huggingfaceClientId, - githubAppClientId: deps.githubAppClientId, - githubAppClientSecret: deps.githubAppClientSecret, }, connectCredential, recentlyConnected, diff --git a/packages/onboarding/test/complete-credential.test.ts b/packages/onboarding/test/complete-credential.test.ts index 573883f00..460c4a2a4 100644 --- a/packages/onboarding/test/complete-credential.test.ts +++ b/packages/onboarding/test/complete-credential.test.ts @@ -34,6 +34,23 @@ const noopPush: WorkflowPusher = async () => ({ }); const noopPublishToolRegistry: ToolRegistryPublisher = async () => undefined; +// Stubs for the provider/credential half of the shared persist-and-seed +// sequence (CL-6394) — paired with every stubbed `seedCatalogFn` so a +// test that fakes the catalog side never dials the real credential +// endpoints either. +const stubPersistFns = { + ensureProviderFn: async ( + _api: unknown, + _cookies: string[], + args: { name: string }, + ) => `prv_${args.name}`, + ensureCredentialFn: async ( + _api: unknown, + _cookies: string[], + args: { providerId: string }, + ) => `cred_${args.providerId}`, +}; + function collector() { const lines: string[] = []; return { lines, log: (line: string) => lines.push(line) }; @@ -278,6 +295,7 @@ describe("completeCredentialSetup", () => { pushWorkflow: noopPush, publishToolRegistry: noopPublishToolRegistry, log: collector().log, + ...stubPersistFns, seedCatalogFn: async (args) => { seedCatalogCalls.push(args); return { hasCompletionCapableModel: true }; @@ -344,6 +362,7 @@ describe("completeCredentialSetup", () => { pushWorkflow: noopPush, publishToolRegistry: noopPublishToolRegistry, log: collector().log, + ...stubPersistFns, seedCatalogFn: async (args) => { seedCatalogCalls.push(args); return { hasCompletionCapableModel: true }; @@ -395,6 +414,7 @@ describe("completeCredentialSetup", () => { pushWorkflow: noopPush, publishToolRegistry: noopPublishToolRegistry, log: collector().log, + ...stubPersistFns, seedCatalogFn: async (args) => { seedCatalogCalls.push(args); return { hasCompletionCapableModel: true }; @@ -446,6 +466,7 @@ describe("completeCredentialSetup", () => { pushWorkflow: noopPush, publishToolRegistry: noopPublishToolRegistry, log: collector().log, + ...stubPersistFns, seedCatalogFn: async (args) => { seedCatalogCalls.push(args as never); return { hasCompletionCapableModel: true }; @@ -501,6 +522,7 @@ describe("completeCredentialSetup", () => { pushWorkflow: noopPush, publishToolRegistry: noopPublishToolRegistry, log: collector().log, + ...stubPersistFns, seedCatalogFn: async (args) => { seedCatalogCalls.push(args as never); return { hasCompletionCapableModel: true }; @@ -811,6 +833,7 @@ describe("completeCredentialSetup", () => { pushWorkflow: noopPush, publishToolRegistry: noopPublishToolRegistry, log: collector().log, + ...stubPersistFns, seedCatalogFn: async () => ({ hasCompletionCapableModel: true }), }); @@ -1299,6 +1322,7 @@ describe("completeCredentialSetup", () => { pushWorkflow: noopPush, publishToolRegistry: noopPublishToolRegistry, log: collector().log, + ...stubPersistFns, seedCatalogFn: async (args) => { seedCatalogCalls.push(args as never); return { hasCompletionCapableModel: true }; @@ -1351,6 +1375,7 @@ describe("completeCredentialSetup", () => { pushWorkflow: noopPush, publishToolRegistry: noopPublishToolRegistry, log: collector().log, + ...stubPersistFns, seedCatalogFn: async () => ({ hasCompletionCapableModel: true }), seedTenantFn: async () => { throw new SidecarUnavailableError( @@ -1411,6 +1436,7 @@ describe("testAndPersistCredential (the fast half)", () => { return { outcome: "pushed" as const, commitSha: "a".repeat(40) }; }, log: collector().log, + ...stubPersistFns, seedCatalogFn: async (args) => { seedCatalogCalls.push(args); return { hasCompletionCapableModel: true }; @@ -1451,6 +1477,7 @@ describe("testAndPersistCredential (the fast half)", () => { pushWorkflow: noopPush, publishToolRegistry: noopPublishToolRegistry, log: collector().log, + ...stubPersistFns, seedCatalogFn: async (args) => { seedCatalogCalls.push(args); return { hasCompletionCapableModel: true }; @@ -1493,6 +1520,7 @@ describe("testAndPersistCredential (the fast half)", () => { pushWorkflow: noopPush, publishToolRegistry: noopPublishToolRegistry, log: collector().log, + ...stubPersistFns, seedCatalogFn: async (args) => { seedCatalogCalls.push(args); return { hasCompletionCapableModel: true }; diff --git a/packages/onboarding/test/openrouter-connect-routes.test.ts b/packages/onboarding/test/openrouter-connect-routes.test.ts index 34374e6f9..a18304407 100644 --- a/packages/onboarding/test/openrouter-connect-routes.test.ts +++ b/packages/onboarding/test/openrouter-connect-routes.test.ts @@ -250,6 +250,23 @@ describe("GET /oauth/openrouter/start", () => { expect(setCookie).toContain("HttpOnly"); }); + // CL-6394: onboarding's OAuth mount serves ONLY its own first-login + // providers. A GitHub start here must refuse loudly — before this, + // github fell through onboarding's inference-only persistence and + // crashed AFTER a successful token exchange. The GitHub App connect + // lives on the tenant-scoped `connections/oauth` mount instead. + test("a github start on the onboarding mount is a loud 404, never a fall-through", async () => { + const app = connectRoutes(); + + const response = await app.request("/api/onboarding/oauth/github/start"); + + expect(response.status).toBe(404); + const body = (await response.json()) as { + error: { code: string; message: string }; + }; + expect(body.error.code).toBe("not_found"); + }); + test("derives the callback origin from configuration, not the request host", async () => { const app = connectRoutes({ hubUrl: "http://localhost:3000" }); diff --git a/packages/plugins-ui/src/plugin-card.tsx b/packages/plugins-ui/src/plugin-card.tsx index 69a899eba..111e297bc 100644 --- a/packages/plugins-ui/src/plugin-card.tsx +++ b/packages/plugins-ui/src/plugin-card.tsx @@ -7,7 +7,6 @@ import { Button } from "@corbits/react-ui"; import type { ResolvedPlugin } from "@workbench/connections/plugins"; -import { Plus } from "@corbits/icons"; import { pluginIcon, pluginOutcome } from "./plugin-meta"; import { PluginLogo } from "./plugin-logo"; @@ -57,6 +56,9 @@ export function PluginCard({ {caption} {plugin.status === "not_connected" ? ( + // The same single Connect verb the MCP preset rows use — one + // idiom for "not connected yet" across the whole gallery, never + // a bare "+" glyph that hides the action. ) : (