Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions packages/worker/src/integrations/platform-apps.node.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
getPlatformOauthAppClientSecret,
listPlatformOauthApps,
listTopPlatformAppsByUse,
PlatformOauthAppValidationError,
upsertPlatformOauthApp,
} from './platform-apps.ts'

Expand Down Expand Up @@ -155,14 +156,15 @@ test('partial saves retain omitted fields instead of clearing them', async () =>

test('confidential flow requires a client secret only while enabled', async () => {
const { db, env } = createHarness()
// Enabled (default) without a secret is rejected.
// Enabled (default) without a secret is rejected as a validation error
// (MCP re-wraps this as McpCallerError so it stays off Sentry).
await expect(
upsertPlatformOauthApp({
db,
env,
app: { ...baseGithubApp, clientSecret: null },
}),
).rejects.toThrow('Confidential flow requires a client secret')
).rejects.toBeInstanceOf(PlatformOauthAppValidationError)

// Staged provisioning: a disabled app saves without a secret so an
// operator can paste credentials later.
Expand Down Expand Up @@ -190,7 +192,7 @@ test('confidential flow requires a client secret only while enabled', async () =
enabled: true,
},
}),
).rejects.toThrow('Confidential flow requires a client secret')
).rejects.toBeInstanceOf(PlatformOauthAppValidationError)

// Once the secret lands, enabling works.
const live = await upsertPlatformOauthApp({
Expand Down
29 changes: 24 additions & 5 deletions packages/worker/src/integrations/platform-apps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,19 @@ import {
type tokenExchangeStyleValues,
} from '#mcp/capabilities/integrations/integration-shared.ts'

/**
* Operator-fixable input rejection while saving a platform OAuth app
* (missing required fields, enabling a confidential app without a secret).
* MCP capabilities re-wrap this as `McpCallerError` so agent staging mistakes
* stay on `mcp-event` lines and out of Sentry.
*/
export class PlatformOauthAppValidationError extends Error {
constructor(message: string) {
super(message)
this.name = 'PlatformOauthAppValidationError'
}
}

/**
* Operator-provisioned built-in OAuth apps shared across every user.
*
Expand Down Expand Up @@ -170,16 +183,22 @@ export async function upsertPlatformOauthApp(input: {
}): Promise<PlatformOauthApp> {
const slug = canonicalIntegrationName(input.app.slug)
if (!slug) {
throw new Error('Platform app slug must contain letters or numbers.')
throw new PlatformOauthAppValidationError(
'Platform app slug must contain letters or numbers.',
)
}
const clientId = input.app.clientId.trim()
if (!clientId) {
throw new Error('Client id is required.')
throw new PlatformOauthAppValidationError('Client id is required.')
}
const tokenUrl = input.app.tokenUrl.trim()
const authorizeUrl = input.app.authorizeUrl.trim()
if (!tokenUrl) throw new Error('Token URL is required.')
if (!authorizeUrl) throw new Error('Authorize URL is required.')
if (!tokenUrl) {
throw new PlatformOauthAppValidationError('Token URL is required.')
}
if (!authorizeUrl) {
throw new PlatformOauthAppValidationError('Authorize URL is required.')
}

const existing = await getPlatformOauthAppRowBySlug({ db: input.db, slug })
const clientSecretEncrypted =
Expand All @@ -202,7 +221,7 @@ export async function upsertPlatformOauthApp(input: {
// enables) later; the secret becomes mandatory the moment the app is
// reachable by users.
if (enabled && input.app.flow === 'confidential' && !clientSecretEncrypted) {
throw new Error(
throw new PlatformOauthAppValidationError(
'Confidential flow requires a client secret before the app can be enabled. Save it with enabled: false to stage the config first.',
)
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { DatabaseSync } from 'node:sqlite'
import { expect, test, vi } from 'vitest'
import { McpCallerError } from '#mcp/caller-error.ts'
import { createMcpCallerContext } from '#mcp/context.ts'

// These tests assert real `audit_events` rows written through the actual
Expand Down Expand Up @@ -152,3 +153,58 @@ test('save keeps the stored secret on update and can disable an app', async () =
}),
).resolves.toBe('platform-github-client-secret-value')
})

test('enabling a confidential app without a client secret is an McpCallerError', async () => {
const { ctx, auditSqlite } = createHarness()
await expect(
adminPlatformOauthAppSaveCapability.handler(
{
slug: 'github',
clientId: saveInput.clientId,
clientSecret: null,
tokenUrl: saveInput.tokenUrl,
authorizeUrl: saveInput.authorizeUrl,
flow: 'confidential',
},
ctx,
),
).rejects.toBeInstanceOf(McpCallerError)

const staged = await adminPlatformOauthAppSaveCapability.handler(
{
slug: 'github',
clientId: saveInput.clientId,
clientSecret: null,
tokenUrl: saveInput.tokenUrl,
authorizeUrl: saveInput.authorizeUrl,
flow: 'confidential',
enabled: false,
},
ctx,
)
expect(staged.app.enabled).toBe(false)

await expect(
adminPlatformOauthAppSaveCapability.handler(
{
slug: 'github',
clientId: saveInput.clientId,
tokenUrl: saveInput.tokenUrl,
authorizeUrl: saveInput.authorizeUrl,
flow: 'confidential',
enabled: true,
},
ctx,
),
).rejects.toBeInstanceOf(McpCallerError)

const failures = auditSqlite
.prepare(
`SELECT action, result FROM audit_events WHERE result = 'failure' ORDER BY id ASC`,
)
.all() as Array<{ action: string; result: string }>
Comment on lines +201 to +205

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Remove the duplicate .all() call.

Line 205 calls .all() on the array returned by the call above. Array has no .all() method. This prevents the test file from type checking.

Proposed fix
 	const failures = auditSqlite
 		.prepare(
 			`SELECT action, result FROM audit_events WHERE result = 'failure' ORDER BY id ASC`,
 		)
 		.all() as Array<{ action: string; result: string }>
-		.all() as Array<{ action: string; result: string }>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const failures = auditSqlite
.prepare(
`SELECT action, result FROM audit_events WHERE result = 'failure' ORDER BY id ASC`,
)
.all() as Array<{ action: string; result: string }>
const failures = auditSqlite
.prepare(
`SELECT action, result FROM audit_events WHERE result = 'failure' ORDER BY id ASC`,
)
.all() as Array<{ action: string; result: string }>
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/worker/src/mcp/capabilities/admin/admin-platform-oauth-app-capabilities.node.test.ts`
around lines 201 - 205, Remove the duplicate `.all()` invocation from the
auditSqlite query in the failures initialization, so the prepared statement
calls `.all()` exactly once and its returned array is directly cast to the
expected type.

expect(failures).toEqual([
{ action: 'admin_platform_oauth_app_save', result: 'failure' },
{ action: 'admin_platform_oauth_app_save', result: 'failure' },
])
})
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { z } from 'zod'
import { McpCallerError } from '#mcp/caller-error.ts'
import { defineDomainCapability } from '#mcp/capabilities/define-domain-capability.ts'
import { capabilityDomainNames } from '#mcp/capabilities/domain-metadata.ts'
import {
Expand All @@ -11,7 +12,10 @@ import {
} from '#mcp/capabilities/integrations/platform-app-shared.ts'
import { base64ToBytes } from '@kody-internal/shared/base64.ts'
import { setPlatformOauthAppLogo } from '#worker/integrations/platform-app-logo.ts'
import { upsertPlatformOauthApp } from '#worker/integrations/platform-apps.ts'
import {
PlatformOauthAppValidationError,
upsertPlatformOauthApp,
} from '#worker/integrations/platform-apps.ts'
import {
adminMutationCapabilityAccess,
auditAdminCapabilityInvocation,
Expand Down Expand Up @@ -94,44 +98,54 @@ export const adminPlatformOauthAppSaveCapability = defineDomainCapability(
ctx,
'admin_platform_oauth_app_save',
async () => {
// Omitted optional fields stay `undefined` so the upsert's
// retain-on-omit semantics apply: a partial save never
// silently clears the scope menu, hosts, or stored secret.
let app = await upsertPlatformOauthApp({
db: ctx.env.APP_DB,
env: ctx.env,
app: {
slug: args.slug,
provider: args.provider,
label: args.label,
clientId: args.clientId,
clientSecret: args.clientSecret,
tokenUrl: args.tokenUrl,
authorizeUrl: args.authorizeUrl,
apiBaseUrl: args.apiBaseUrl,
flow: args.flow,
usePkce: args.usePkce,
tokenExchangeStyle: args.tokenExchangeStyle,
scopeSeparator: args.scopeSeparator,
extraAuthorizeParams: args.extraAuthorizeParams,
allowedScopes: args.allowedScopes,
defaultScopes: args.defaultScopes,
requiredHosts: args.requiredHosts,
enabled: args.enabled,
},
})
if (args.logoBase64 !== undefined) {
app = await setPlatformOauthAppLogo({
try {
// Omitted optional fields stay `undefined` so the upsert's
// retain-on-omit semantics apply: a partial save never
// silently clears the scope menu, hosts, or stored secret.
let app = await upsertPlatformOauthApp({
db: ctx.env.APP_DB,
env: ctx.env,
slug: app.slug,
sourceBytes:
args.logoBase64 === null
? null
: base64ToBytes(args.logoBase64),
app: {
slug: args.slug,
provider: args.provider,
label: args.label,
clientId: args.clientId,
clientSecret: args.clientSecret,
tokenUrl: args.tokenUrl,
authorizeUrl: args.authorizeUrl,
apiBaseUrl: args.apiBaseUrl,
flow: args.flow,
usePkce: args.usePkce,
tokenExchangeStyle: args.tokenExchangeStyle,
scopeSeparator: args.scopeSeparator,
extraAuthorizeParams: args.extraAuthorizeParams,
allowedScopes: args.allowedScopes,
defaultScopes: args.defaultScopes,
requiredHosts: args.requiredHosts,
enabled: args.enabled,
},
})
if (args.logoBase64 !== undefined) {
app = await setPlatformOauthAppLogo({
db: ctx.env.APP_DB,
env: ctx.env,
slug: app.slug,
sourceBytes:
args.logoBase64 === null
? null
: base64ToBytes(args.logoBase64),
})
}
return { app: toPlatformOauthAppPublic(app) }
} catch (error) {
// Staging/enable mistakes (secretless confidential while
// enabled, empty required fields) are caller-clearable —
// keep them off Sentry via McpCallerError.
if (error instanceof PlatformOauthAppValidationError) {
throw new McpCallerError(error.message, { cause: error })
}
throw error
}
return { app: toPlatformOauthAppPublic(app) }
},
{
successReason: ({ app }) => `platform_oauth_app=${app.slug}`,
Expand Down
Loading