Compare commits

...
Author SHA1 Message Date
Peter Steinberger fa3849069d fix(api): centralize v1 soft-delete error mapping 2026-02-14 21:52:43 +01:00
Sergiy Dybskiy ed4c191dfc test: add e2e test for delete error handling
Verifies that deleting a non-existent skill returns a proper 'not found'
error instead of a generic 'Unauthorized' message.
2026-02-14 21:49:21 +01:00
Sergiy Dybskiy 9e704e0a81 fix: return proper HTTP status codes for delete/undelete errors
The delete and undelete handlers for skills and souls were catching all
errors and returning 401 Unauthorized, even for errors like:
- 'Skill not found' (should be 404)
- 'Forbidden' (should be 403)
- Other validation errors (should be 400)

This change updates the error handling to return appropriate status codes:
- 401 Unauthorized: authentication failures
- 403 Forbidden: authorization failures (not owner/admin/moderator)
- 404 Not Found: skill/soul/user not found
- 400 Bad Request: other errors with descriptive message

Fixes #34
2026-02-14 21:48:52 +01:00
3 changed files with 127 additions and 8 deletions
+57
View File
@@ -807,6 +807,63 @@ describe('httpApiV1 handlers', () => {
expect(response2.status).toBe(200)
})
it('delete/undelete map forbidden/not-found/unknown to 403/404/500', async () => {
vi.mocked(requireApiTokenUser).mockResolvedValue({
userId: 'users:1',
user: { handle: 'p' },
} as never)
const runMutationForbidden = vi.fn(async (_query: unknown, args: Record<string, unknown>) => {
if ('key' in args) return okRate()
throw new Error('Forbidden')
})
const forbidden = await __handlers.skillsDeleteRouterV1Handler(
makeCtx({ runMutation: runMutationForbidden }),
new Request('https://example.com/api/v1/skills/demo', {
method: 'DELETE',
headers: { Authorization: 'Bearer clh_test' },
}),
)
expect(forbidden.status).toBe(403)
expect(await forbidden.text()).toBe('Forbidden')
vi.mocked(requireApiTokenUser).mockResolvedValue({
userId: 'users:1',
user: { handle: 'p' },
} as never)
const runMutationNotFound = vi.fn(async (_query: unknown, args: Record<string, unknown>) => {
if ('key' in args) return okRate()
throw new Error('Skill not found')
})
const notFound = await __handlers.skillsPostRouterV1Handler(
makeCtx({ runMutation: runMutationNotFound }),
new Request('https://example.com/api/v1/skills/demo/undelete', {
method: 'POST',
headers: { Authorization: 'Bearer clh_test' },
}),
)
expect(notFound.status).toBe(404)
expect(await notFound.text()).toBe('Skill not found')
vi.mocked(requireApiTokenUser).mockResolvedValue({
userId: 'users:1',
user: { handle: 'p' },
} as never)
const runMutationUnknown = vi.fn(async (_query: unknown, args: Record<string, unknown>) => {
if ('key' in args) return okRate()
throw new Error('boom')
})
const unknown = await __handlers.soulsDeleteRouterV1Handler(
makeCtx({ runMutation: runMutationUnknown }),
new Request('https://example.com/api/v1/souls/demo-soul', {
method: 'DELETE',
headers: { Authorization: 'Bearer clh_test' },
}),
)
expect(unknown.status).toBe(500)
expect(await unknown.text()).toBe('Internal Server Error')
})
it('ban user requires auth', async () => {
vi.mocked(requireApiTokenUser).mockRejectedValueOnce(new Error('Unauthorized'))
const runMutation = vi.fn().mockResolvedValue(okRate())
+25 -8
View File
@@ -538,8 +538,8 @@ async function skillsPostRouterV1Handler(ctx: ActionCtx, request: Request) {
deleted: false,
})
return json({ ok: true }, 200, rate.headers)
} catch {
return text('Unauthorized', 401, rate.headers)
} catch (error) {
return softDeleteErrorToResponse('skill', error, rate.headers)
}
}
@@ -560,8 +560,8 @@ async function skillsDeleteRouterV1Handler(ctx: ActionCtx, request: Request) {
deleted: true,
})
return json({ ok: true }, 200, rate.headers)
} catch {
return text('Unauthorized', 401, rate.headers)
} catch (error) {
return softDeleteErrorToResponse('skill', error, rate.headers)
}
}
@@ -1235,8 +1235,8 @@ async function soulsPostRouterV1Handler(ctx: ActionCtx, request: Request) {
deleted: false,
})
return json({ ok: true }, 200, rate.headers)
} catch {
return text('Unauthorized', 401, rate.headers)
} catch (error) {
return softDeleteErrorToResponse('soul', error, rate.headers)
}
}
@@ -1257,13 +1257,30 @@ async function soulsDeleteRouterV1Handler(ctx: ActionCtx, request: Request) {
deleted: true,
})
return json({ ok: true }, 200, rate.headers)
} catch {
return text('Unauthorized', 401, rate.headers)
} catch (error) {
return softDeleteErrorToResponse('soul', error, rate.headers)
}
}
export const soulsDeleteRouterV1Http = httpAction(soulsDeleteRouterV1Handler)
function softDeleteErrorToResponse(
entity: 'skill' | 'soul',
error: unknown,
headers: HeadersInit,
) {
const message = error instanceof Error ? error.message : `${entity} delete failed`
const lower = message.toLowerCase()
if (lower.includes('unauthorized')) return text('Unauthorized', 401, headers)
if (lower.includes('forbidden')) return text('Forbidden', 403, headers)
if (lower.includes('not found')) return text(message, 404, headers)
if (lower.includes('slug required')) return text('Slug required', 400, headers)
// Unknown: server-side failure. Keep body generic.
return text('Internal Server Error', 500, headers)
}
async function starsPostRouterV1Handler(ctx: ActionCtx, request: Request) {
const rate = await applyRateLimit(ctx, request, 'write')
if (!rate.ok) return rate.response
+45
View File
@@ -504,4 +504,49 @@ describe('clawhub e2e', () => {
await rm(cfg.dir, { recursive: true, force: true })
}
}, 180_000)
it('delete returns proper error for non-existent skill', async () => {
const registry = process.env.CLAWDHUB_REGISTRY?.trim() || 'https://clawdhub.com'
const site = process.env.CLAWDHUB_SITE?.trim() || 'https://clawdhub.com'
const token = mustGetToken() ?? (await readGlobalConfig())?.token ?? null
if (!token) {
throw new Error('Missing token. Set CLAWDHUB_E2E_TOKEN or run: bun clawdhub auth login')
}
const cfg = await makeTempConfig(registry, token)
const workdir = await mkdtemp(join(tmpdir(), 'clawdhub-e2e-delete-'))
const nonExistentSlug = `non-existent-skill-${Date.now()}`
try {
const del = spawnSync(
'bun',
[
'clawdhub',
'delete',
nonExistentSlug,
'--yes',
'--site',
site,
'--registry',
registry,
'--workdir',
workdir,
],
{
cwd: process.cwd(),
env: { ...process.env, CLAWDHUB_CONFIG_PATH: cfg.path, CLAWDHUB_DISABLE_TELEMETRY: '1' },
encoding: 'utf8',
},
)
// Should fail with non-zero exit code
expect(del.status).not.toBe(0)
// Error should mention "not found" - not generic "Unauthorized"
const output = (del.stdout + del.stderr).toLowerCase()
expect(output).toMatch(/not found|404|does not exist/i)
expect(output).not.toMatch(/unauthorized/i)
} finally {
await rm(workdir, { recursive: true, force: true })
await rm(cfg.dir, { recursive: true, force: true })
}
}, 30_000)
})