Skip to content

Commit d18c514

Browse files
Viccodes111nonlypiececlaude
authored
fix(api): return JSON 4xx errors for malformed bodies and invalid query parameters (#1764)
Co-authored-by: 1nonlypiece <1nonlypiece@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent c70dc18 commit d18c514

8 files changed

Lines changed: 441 additions & 17 deletions

File tree

‎api/__tests__/errors.test.mjs‎

Lines changed: 216 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,216 @@
1+
import { test, describe } from 'node:test'
2+
import assert from 'node:assert/strict'
3+
import request from 'supertest'
4+
import { createApp } from '../index.mjs'
5+
import { MemoryStore } from '../../apilib/store.mjs'
6+
import { OPENAPI } from '../../apilib/openapi.mjs'
7+
8+
function buildApp(customStore) {
9+
const store = customStore || new MemoryStore()
10+
return { app: createApp(async () => store), store }
11+
}
12+
13+
describe('Error Handling Middleware and Query Validation', () => {
14+
test('GET /api/vaults?status=bogus -> 400 JSON naming status; no Postgres query is issued', async () => {
15+
let listVaultsCalled = false
16+
const customStore = new MemoryStore()
17+
customStore.listVaults = async (...args) => {
18+
listVaultsCalled = true
19+
return MemoryStore.prototype.listVaults.apply(customStore, args)
20+
}
21+
22+
const { app } = buildApp(customStore)
23+
const res = await request(app)
24+
.get('/api/vaults?status=bogus')
25+
.expect('Content-Type', /application\/json/)
26+
.expect(400)
27+
28+
assert.equal(res.status, 400)
29+
assert.match(res.body.error, /status/i)
30+
assert(Array.isArray(res.body.details))
31+
assert.match(res.body.details[0], /status/i)
32+
assert.equal(listVaultsCalled, false, 'Store listVaults should not be called on validation failure')
33+
})
34+
35+
test('GET /api/vaults?creator=notakey -> 400; ?creator=a&creator=b -> 400', async () => {
36+
const { app } = buildApp()
37+
38+
const res1 = await request(app)
39+
.get('/api/vaults?creator=notakey')
40+
.expect('Content-Type', /application\/json/)
41+
.expect(400)
42+
43+
assert.equal(res1.status, 400)
44+
assert.match(res1.body.error, /creator/i)
45+
assert(Array.isArray(res1.body.details))
46+
47+
const res2 = await request(app)
48+
.get('/api/vaults?creator=a&creator=b')
49+
.expect('Content-Type', /application\/json/)
50+
.expect(400)
51+
52+
assert.equal(res2.status, 400)
53+
assert.match(res2.body.error, /creator/i)
54+
assert(Array.isArray(res2.body.details))
55+
assert.match(res2.body.details[0], /array/i)
56+
})
57+
58+
test('GET /api/vaults?status= (empty string) is treated as absent', async () => {
59+
const { app } = buildApp()
60+
61+
const res = await request(app)
62+
.get('/api/vaults?status=')
63+
.expect('Content-Type', /application\/json/)
64+
.expect(200)
65+
66+
assert.equal(res.status, 200)
67+
assert(Array.isArray(res.body.data))
68+
})
69+
70+
test('GET /api/transactions?vaultId=../../x -> 400', async () => {
71+
const { app } = buildApp()
72+
73+
const res = await request(app)
74+
.get('/api/transactions?vaultId=../../x')
75+
.expect('Content-Type', /application\/json/)
76+
.expect(400)
77+
78+
assert.equal(res.status, 400)
79+
assert.match(res.body.error, /vaultId/i)
80+
assert(Array.isArray(res.body.details))
81+
})
82+
83+
test('POST /api/vaults with malformed JSON body -> 400 application/json { error: "Malformed JSON body" }', async () => {
84+
const { app } = buildApp()
85+
86+
const res = await request(app)
87+
.post('/api/vaults')
88+
.set('Content-Type', 'application/json')
89+
.send('{bad')
90+
.expect('Content-Type', /application\/json/)
91+
.expect(400)
92+
93+
assert.equal(res.status, 400)
94+
assert.deepEqual(res.body, { error: 'Malformed JSON body' })
95+
})
96+
97+
test('A 101kb body -> 413 JSON', async () => {
98+
const { app } = buildApp()
99+
const largeBody = JSON.stringify({ data: 'x'.repeat(1024 * 101) })
100+
101+
const res = await request(app)
102+
.post('/api/vaults')
103+
.set('Content-Type', 'application/json')
104+
.send(largeBody)
105+
.expect('Content-Type', /application\/json/)
106+
.expect(413)
107+
108+
assert.equal(res.status, 413)
109+
assert.equal(res.body.error, 'Payload Too Large')
110+
})
111+
112+
test('A text/plain POST -> 415 JSON', async () => {
113+
const { app } = buildApp()
114+
115+
const res = await request(app)
116+
.post('/api/vaults')
117+
.set('Content-Type', 'text/plain')
118+
.send('hello world')
119+
.expect('Content-Type', /application\/json/)
120+
.expect(415)
121+
122+
assert.equal(res.status, 415)
123+
assert.match(res.body.error, /Unsupported Media Type/i)
124+
})
125+
126+
test('DELETE /api/vaults -> 405 JSON with an Allow header', async () => {
127+
const { app } = buildApp()
128+
129+
const res = await request(app)
130+
.delete('/api/vaults')
131+
.expect('Content-Type', /application\/json/)
132+
.expect(405)
133+
134+
assert.equal(res.status, 405)
135+
assert.equal(res.headers.allow, 'GET, POST')
136+
assert.match(res.body.error, /Method Not Allowed/i)
137+
})
138+
139+
test('DELETE /api/transactions -> 405 JSON with an Allow header', async () => {
140+
const { app } = buildApp()
141+
142+
const res = await request(app)
143+
.delete('/api/transactions')
144+
.expect('Content-Type', /application\/json/)
145+
.expect(405)
146+
147+
assert.equal(res.status, 405)
148+
assert.equal(res.headers.allow, 'GET')
149+
assert.match(res.body.error, /Method Not Allowed/i)
150+
})
151+
152+
test('Thrown non-Error values (strings) inside handlers still yield JSON 500 without leaking stack', async () => {
153+
const customStore = new MemoryStore()
154+
customStore.listVaults = async () => {
155+
throw 'A non-Error string failure'
156+
}
157+
158+
const { app } = buildApp(customStore)
159+
const res = await request(app)
160+
.get('/api/vaults')
161+
.expect('Content-Type', /application\/json/)
162+
.expect(500)
163+
164+
assert.equal(res.status, 500)
165+
assert.deepEqual(res.body, { error: 'Internal error' })
166+
})
167+
168+
test('Thrown Error inside handlers does not leak err.message in responses', async () => {
169+
const customStore = new MemoryStore()
170+
customStore.listVaults = async () => {
171+
throw new Error('Secret postgres connection error or stack trace')
172+
}
173+
174+
const { app } = buildApp(customStore)
175+
const res = await request(app)
176+
.get('/api/vaults')
177+
.expect('Content-Type', /application\/json/)
178+
.expect(500)
179+
180+
assert.equal(res.status, 500)
181+
assert.deepEqual(res.body, { error: 'Internal error' })
182+
})
183+
184+
test('Parametrised test: every route in OPENAPI.paths with garbage input asserts JSON content-type and status < 500', async () => {
185+
process.env.CRON_SECRET = 'test-secret'
186+
const { app } = buildApp()
187+
const paths = Object.keys(OPENAPI.paths)
188+
189+
for (const p of paths) {
190+
// Substitute path variables like :id, :mid with test string
191+
const testPath = '/api' + p.replace(/\{([^}]+)\}/g, 'test-$1')
192+
193+
// 1. GET with garbage query params
194+
const resGet = await request(app)
195+
.get(`${testPath}?status=bogus&creator=bogus&vaultId=bogus/bad/path&junk=<script>`)
196+
.set('Authorization', 'Bearer test-secret')
197+
198+
const getContentType = resGet.headers['content-type'] || ''
199+
assert(!/text\/html/i.test(getContentType), `Route GET ${testPath} should not return HTML`)
200+
assert.match(getContentType, /application\/json/, `Route GET ${testPath} should return JSON`)
201+
assert(resGet.status < 500, `Route GET ${testPath} returned ${resGet.status} >= 500 with body: ${JSON.stringify(resGet.body)}`)
202+
203+
// 2. POST with garbage body
204+
const resPost = await request(app)
205+
.post(testPath)
206+
.set('Authorization', 'Bearer test-secret')
207+
.set('Content-Type', 'application/json')
208+
.send({ garbage: true, status: 'invalid', creator: 'not-a-key' })
209+
210+
const postContentType = resPost.headers['content-type'] || ''
211+
assert(!/text\/html/i.test(postContentType), `Route POST ${testPath} should not return HTML`)
212+
assert.match(postContentType, /application\/json/, `Route POST ${testPath} should return JSON`)
213+
assert(resPost.status < 500, `Route POST ${testPath} returned ${resPost.status} >= 500 with body: ${JSON.stringify(resPost.body)}`)
214+
}
215+
})
216+
})

‎api/index.mjs‎

Lines changed: 70 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { checkReadiness, isDatabaseUnavailable } from '../apilib/store-bootstrap
77
import { OPENAPI } from '../apilib/openapi.mjs'
88
import { OnchainError, isEnabled, getConfig, getServer, buildCreateVaultTx, buildStakeTx, verifySignedTx, txHashOf, submitAndWait, validateForChain } from '../apilib/soroban.mjs'
99
import { safeCompare, expireOverdueVaults } from '../apilib/expiry.mjs'
10+
import { QueryValidationError, validateStatus, validateCreator, validateVaultId } from '../apilib/query.mjs'
1011
import { parseAmount, formatAmount } from '../apilib/amount.mjs'
1112
import { encodeCursor, decodeCursor, parseLimit } from '../apilib/pagination.mjs'
1213
import { failVault, validateMilestone } from '../shared/vaultMachine.mjs'
@@ -57,15 +58,7 @@ export function createApp(getStore = defaultGetStore, env = process.env) {
5758
const STELLAR = /^G[A-Z2-7]{55}$/
5859
const hex = (n) => randomBytes(n).toString('hex')
5960
const bad = (res, msg, code = 400) => res.status(code).json({ error: msg })
60-
const wrap = (fn) => (req, res) => fn(req, res).catch((e) => {
61-
if (isDatabaseUnavailable(e)) {
62-
return res.set('Retry-After', '1').status(503).json({ error: 'Database unavailable' })
63-
}
64-
if (e instanceof StoreConflictError) return res.status(409).json({ error: e.message })
65-
if (e instanceof OnchainError) return res.status(e.status).json({ error: e.message, ...(e.contractError && { contractError: e.contractError }) })
66-
console.error('API request failed')
67-
res.status(500).json({ error: 'Internal error' })
68-
})
61+
const wrap = (fn) => (req, res, next) => Promise.resolve().then(() => fn(req, res, next)).catch(next)
6962

7063
const router = express.Router()
7164

@@ -89,8 +82,10 @@ export function createApp(getStore = defaultGetStore, env = process.env) {
8982
if (!cursor) return bad(res, 'Invalid cursor')
9083
}
9184

85+
const creator = validateCreator(req.query.creator)
86+
const status = validateStatus(req.query.status)
9287
const s = await getStore()
93-
const result = await s.listVaults({ creator: req.query.creator, status: req.query.status, limit, cursor })
88+
const result = await s.listVaults({ creator, status, limit, cursor })
9489

9590
const nextCursor = result.hasMore && result.lastItem
9691
? encodeCursor(result.lastItem.created_at || result.lastItem.createdAt, result.lastItem.id)
@@ -348,8 +343,9 @@ export function createApp(getStore = defaultGetStore, env = process.env) {
348343
if (!cursor) return bad(res, 'Invalid cursor')
349344
}
350345

346+
const vaultId = validateVaultId(req.query.vaultId)
351347
const s = await getStore()
352-
const result = await s.listActivity({ vaultId: req.query.vaultId, limit, cursor })
348+
const result = await s.listActivity({ vaultId, limit, cursor })
353349

354350
const nextCursor = result.hasMore && result.lastItem
355351
? encodeCursor(result.lastItem.created_at || result.lastItem.createdAt, result.lastItem.id)
@@ -363,10 +359,11 @@ export function createApp(getStore = defaultGetStore, env = process.env) {
363359
}))
364360

365361
router.get('/stats', wrap(async (req, res) => {
362+
const creator = validateCreator(req.query.creator)
366363
const s = await getStore()
367364
// Use aggregateStats if available (Postgres or Memory store)
368365
const stats = typeof s.aggregateStats === 'function'
369-
? await s.aggregateStats({ creator: req.query.creator, verifier: req.query.verifier })
366+
? await s.aggregateStats({ creator, verifier: req.query.verifier })
370367
: { totalVaults: 0, active: 0, completed: 0, failed: 0, totalLocked: '0', totalReleased: '0', totalRedirected: '0' }
371368
const { totalVaults, active, completed, failed, totalLocked, totalReleased, totalRedirected } = stats
372369
const done = completed + failed
@@ -406,11 +403,69 @@ export function createApp(getStore = defaultGetStore, env = process.env) {
406403
res.json({ processed: result.processed, remaining: result.remaining })
407404
}))
408405

406+
app.use(['/api/v1', '/api'], (req, res, next) => {
407+
if (req.method === 'POST' && !req.is('application/json')) {
408+
return res.status(415).json({ error: 'Unsupported Media Type: Content-Type must be application/json' })
409+
}
410+
next()
411+
})
412+
409413
app.use(['/api/v1', '/api'], router)
414+
415+
// 405 Method Not Allowed for known API endpoints
416+
const methodMap = [
417+
{ pattern: /^\/api(?:\/v1)?\/vaults\/?$/, allow: 'GET, POST' },
418+
{ pattern: /^\/api(?:\/v1)?\/vaults\/[^\/]+\/?$/, allow: 'GET' },
419+
{ pattern: /^\/api(?:\/v1)?\/vaults\/[^\/]+\/sign\/?$/, allow: 'POST' },
420+
{ pattern: /^\/api(?:\/v1)?\/vaults\/[^\/]+\/milestones\/[^\/]+\/validate\/?$/, allow: 'POST' },
421+
{ pattern: /^\/api(?:\/v1)?\/vaults\/[^\/]+\/fail\/?$/, allow: 'POST' },
422+
{ pattern: /^\/api(?:\/v1)?\/transactions\/?$/, allow: 'GET' },
423+
{ pattern: /^\/api(?:\/v1)?\/stats\/?$/, allow: 'GET' },
424+
{ pattern: /^\/api(?:\/v1)?\/health\/?$/, allow: 'GET' },
425+
{ pattern: /^\/api(?:\/v1)?\/ready\/?$/, allow: 'GET' },
426+
{ pattern: /^\/api(?:\/v1)?\/openapi\.json\/?$/, allow: 'GET' },
427+
{ pattern: /^\/api(?:\/v1)?\/cron\/expire-vaults\/?$/, allow: 'GET' },
428+
]
429+
app.use((req, res, next) => {
430+
const match = methodMap.find((m) => m.pattern.test(req.path))
431+
if (match) {
432+
res.setHeader('Allow', match.allow)
433+
return res.status(405).json({ error: `Method Not Allowed: use ${match.allow}` })
434+
}
435+
next()
436+
})
437+
438+
// 404 Fallback
410439
app.use((_req, res) => res.status(404).json({ error: 'Not found' }))
411-
app.use((err, _req, res, _next) => {
412-
console.error('API request error:', err)
413-
res.status(500).json({ error: 'Internal error' })
440+
441+
// Error-handling middleware (4 arguments required by Express)
442+
// Maps Express entity.parse.failed -> 400, entity.too.large -> 413, charset/encoding -> 415,
443+
// QueryValidationError -> 400, isDatabaseUnavailable -> 503, StoreConflictError -> 409,
444+
// OnchainError -> e.status, and everything else -> safe 500 JSON without stack leak.
445+
app.use((err, req, res, _next) => {
446+
if (err?.type === 'entity.parse.failed') {
447+
return res.status(400).json({ error: 'Malformed JSON body' })
448+
}
449+
if (err?.type === 'entity.too.large' || err?.status === 413) {
450+
return res.status(413).json({ error: 'Payload Too Large' })
451+
}
452+
if (err?.type === 'charset.unsupported' || err?.type === 'encoding.unsupported' || err?.status === 415) {
453+
return res.status(415).json({ error: 'Unsupported Media Type' })
454+
}
455+
if (err instanceof QueryValidationError) {
456+
return res.status(err.status || 400).json({ error: err.message, ...(err.details && { details: err.details }) })
457+
}
458+
if (isDatabaseUnavailable(err)) {
459+
return res.set('Retry-After', '1').status(503).json({ error: 'Database unavailable' })
460+
}
461+
if (err instanceof StoreConflictError) {
462+
return res.status(409).json({ error: err.message })
463+
}
464+
if (err instanceof OnchainError) {
465+
return res.status(err.status).json({ error: err.message, ...(err.contractError && { contractError: err.contractError }) })
466+
}
467+
console.error('API request failed')
468+
return res.status(500).json({ error: 'Internal error' })
414469
})
415470

416471
return app

0 commit comments

Comments
 (0)