Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions src/adapters/providers/discord.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export const discordManifest: ProviderManifest = {
// there is no scope list to ask for here.
scopes: [],
auth: { type: 'api_key', in: 'header', name: 'authorization', prefix: 'Bot ' },
validate: { request: 'GET /users/@me' },
pagination: {
// Discord sends no cursor back. The caller asks for what came before the
// oldest id it holds, so the cursor is a message id read off the page.
Expand Down
35 changes: 35 additions & 0 deletions src/adapters/providers/manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,8 @@ export type ProviderManifest = {
baseUrl: string
scopes: string[]
auth: AuthScheme
/** Optional request made before an api key is stored in the vault. */
validate?: { request: `${Method} /${string}` }
/** Merged under the headers the executor sets, never over them. */
headers?: Record<string, string>
pagination?: Pagination
Expand Down Expand Up @@ -176,6 +178,39 @@ export function manifestProvider(manifest: ProviderManifest): ProviderAdapter {
credential: manifest.auth.type === 'api_key' ? 'api_key' : 'oauth',
listTools: () => manifest.tools.map(toolDef),

...(manifest.validate
? {
async validateKey(ctx: AdapterContext): Promise<void> {
const space = manifest.validate!.request.indexOf(' ')
const method = manifest.validate!.request.slice(0, space)
const path = manifest.validate!.request.slice(space + 1)
let res: Response
try {
res = await ctx.fetch(
withKeyInQuery(`${manifest.baseUrl}${path}`, manifest.auth, ctx.accessToken ?? ''),
{
method,
headers: {
...manifest.headers,
...authHeader(manifest.auth, ctx.accessToken ?? ''),
},
},
)
Comment thread
fajarhide marked this conversation as resolved.
Outdated
} catch {
throw fail('upstream_error', `${manifest.prefix} validation request failed`)
}
if (!res.ok) {
const code = res.status === 401 || res.status === 403 ? 'invalid_credential' : 'upstream_error'
const message =
code === 'invalid_credential'
? `${manifest.prefix} refused the api key`
: `${manifest.prefix} validation request failed`
throw fail(code, message)
}
Comment thread
fajarhide marked this conversation as resolved.
},
}
: {}),

async callTool(ctx: AdapterContext, name: string, rawArgs: unknown): Promise<ToolResult> {
const tool = byName.get(name)
if (!tool) throw fail('tool_not_found', `${manifest.prefix} has no tool ${name}`)
Expand Down
1 change: 1 addition & 0 deletions src/adapters/providers/registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export interface ProviderAdapter {
*/
credential?: 'oauth' | 'api_key' | 'none'
listTools(): ToolDef[]
validateKey?(ctx: AdapterContext): Promise<void>
callTool(ctx: AdapterContext, tool: string, args: unknown): Promise<ToolResult>
mapError(err: unknown): GatewayError
}
Expand Down
10 changes: 10 additions & 0 deletions src/application/connections.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export type ConnectionDeps = {
states: StateStore
grants: GrantStore
enablement: EnablementStore
fetch?: typeof fetch
exchange?: typeof exchangeCode
}

Expand Down Expand Up @@ -136,6 +137,15 @@ export async function setApiKey(
const key = input.key.trim()
if (!key) throw new GatewayError('invalid_arguments', 'api_key must not be empty')

if (adapter.validateKey) {
await adapter.validateKey({
workspaceId: input.workspaceId,
requestId: `key-validation-${input.prefix}`,
accessToken: key,
fetch: deps.fetch ?? fetch,
})
}
Comment on lines +140 to +147

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

deps.fetch is never wired at any call site, so this falls through to the global fetch and the suite reaches discord.com for real. That is what turns admin-connections.test.ts:210 and connections-http.test.ts:124 red. Both fixtures build their deps without a fetch, so passing the existing double through ConnectionDeps in those two files fixes it and keeps the suite offline. No suggestion here because those files are not in this diff.

Separately, this call has no timeout budget. Every tool call goes through withTimeout (call-tool.ts:160, module-private), so a vendor that accepts the connection and never answers holds the handler for undici's default. Either export that helper or hand the validate request an AbortSignal.timeout(...).


await deps.grants.save(input.workspaceId, adapter.grantId, {
accessToken: key,
refreshToken: null,
Expand Down
23 changes: 23 additions & 0 deletions test/connections.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
beginConnection,
completeConnection,
disconnect,
setApiKey,
type ConnectionDeps,
} from '../src/application/connections.ts'
import { createRegistry, type ProviderAdapter } from '../src/adapters/providers/registry.ts'
Expand Down Expand Up @@ -185,3 +186,25 @@ describe('a grant shared by several providers', () => {
expect(new URL(url).searchParams.get('scope')).toBe('cal.read')
})
})

describe('api key validation', () => {
it('does not save or enable a provider when validation refuses the key', async () => {
const base = fakeProvider()
const rejected: ProviderAdapter = {
...base,
id: 'keyed',
prefix: 'keyed',
credential: 'api_key',
validateKey: async () => {
throw new Error('refused')
},
}
const shared = deps({ registry: createRegistry([rejected]) })

await expect(setApiKey(shared, { workspaceId, prefix: 'keyed', key: 'wrong' })).rejects.toThrow(
'refused',
)
expect(await shared.grants.load(workspaceId, 'keyed')).toBeNull()
expect(await shared.enablement.enabledPrefixes(workspaceId)).not.toContain('keyed')
})
})
49 changes: 49 additions & 0 deletions test/providers/manifest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,55 @@ function withTools(tools: ProviderManifest['tools'], over: Partial<ProviderManif
const demo = manifestProvider(base)

describe('manifest executor: requests', () => {
it('validates an api key without storing or exposing it', async () => {
const validator = withTools([], { validate: { request: 'GET /users/@me' }, auth: { type: 'api_key', in: 'header', name: 'authorization', prefix: 'Bot ' } })
const upstream = fakeUpstream([{ match: /users\/@me/, body: { id: 'bot-1' } }])

await validator.validateKey?.({ ...ctx(upstream), accessToken: 'secret-token' })

expect(upstream.calls[0]?.url).toBe('https://api.demo.test/users/@me')
expect((upstream.calls[0]?.init?.headers as Record<string, string>).authorization).toBe('Bot secret-token')
})

it('rejects an api key when its validation request fails', async () => {
const validator = withTools([], { validate: { request: 'GET /users/@me' } })
const upstream = fakeUpstream([{ match: /users\/@me/, status: 401, body: { message: 'bad token' } }])

await expect(validator.validateKey?.({ ...ctx(upstream), accessToken: 'wrong' })).rejects.toMatchObject({
code: 'invalid_credential',

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Follows from the error code change you took above.

Suggested change
code: 'invalid_credential',
code: 'invalid_arguments',

})
})

it('puts query API keys on the validation request', async () => {
const validator = withTools([], {
validate: { request: 'GET /users/@me' },
auth: { type: 'api_key', in: 'query', name: 'api_key' },
})
const upstream = fakeUpstream([{ match: /users\/@me\?api_key=secret-token/, body: { id: 'bot-1' } }])

await validator.validateKey?.({ ...ctx(upstream), accessToken: 'secret-token' })

expect(upstream.calls[0]?.url).toContain('api_key=secret-token')
})

it('reports validation service failures as upstream errors', async () => {
const validator = withTools([], { validate: { request: 'GET /users/@me' } })
const upstream = fakeUpstream([{ match: /users\/@me/, status: 503, body: { message: 'busy' } }])

await expect(validator.validateKey?.({ ...ctx(upstream), accessToken: 'secret-token' })).rejects.toMatchObject({
code: 'upstream_error',
})
})

it('reports validation network failures as upstream errors', async () => {
const validator = withTools([], { validate: { request: 'GET /users/@me' } })
const unreachable = { ...ctx(fakeUpstream([])), fetch: (async () => { throw new Error('offline') }) as typeof fetch }

await expect(validator.validateKey?.({ ...unreachable, accessToken: 'secret-token' })).rejects.toMatchObject({
code: 'upstream_error',
})
})

it('fills path placeholders and escapes every segment', async () => {
const upstream = fakeUpstream([{ match: /boxes/, body: itemsPage(1) }])
await demo.callTool(ctx(upstream), 'list_things', { box: '../../admin' })
Expand Down
Loading