Skip to content

Commit 2f68456

Browse files
committed
Fix audit findings: plugin budgets, fail-closed hooks, secret params, dup IDs, dead config, input validation
- Per-request plugin budgets: PluginRegistry.beginRequest() mints a RequestPlugins with its own budget map, so concurrent requests no longer clobber each other's allowances (was a shared mutable map reset per request). - Mutation hooks fail-closed on a thrown error (onHttpRequest rejects; onBeforeApiCall/onAfterApiCall/onBeforeSign drop), matching the documented contract instead of silently skipping a broken plugin. - Secret-param detection runs before env interpolation: a `fixed: ${VAR}` parameter is now flagged `secret: true` in parseConfig so the resolved value stays out of the public endpoint ID (the implicit `${...}` check in isSecretParameter was dead post-interpolation). - buildEndpointMap throws on colliding endpoint IDs and validateConfig reports them, instead of new Map() silently last-wins. - Removed the unimplemented `cache.delay` field from the schema, docs, and example config. - parseRequestBody rejects a non-object `parameters` (400) but passes nested values through untouched so body parameters can still be nested JSON; resolveEncoding ignores non-string `_type`/`_path`/`_times` (clean 400, not a 502); body-size check counts bytes. Documented the limitation. - x402 `amount` must be a non-negative integer string (config error, not a BigInt throw at request time).
1 parent 5a41090 commit 2f68456

16 files changed

Lines changed: 532 additions & 128 deletions

File tree

book/docs/config/apis.md

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -139,10 +139,10 @@ cache:
139139
| Field | Type | Required | Description |
140140
| -------- | -------- | -------- | ---------------------------------------------------------- |
141141
| `maxAge` | `number` | Yes | Cache TTL in milliseconds for responses. Positive integer. |
142-
| `delay` | `number` | No | Delay in milliseconds before cached responses are served. |
143142

144143
`maxAge` controls response caching — repeated `POST /endpoints/{id}` requests with the same parameters return the cached
145-
response until the TTL expires.
144+
response until the TTL expires. A cached response replays the same signature and timestamp, so within `maxAge` every
145+
caller (and every on-chain submission) gets the identical signed payload.
146146

147147
## Endpoint-level fields
148148

@@ -222,6 +222,20 @@ parameters:
222222
default: usd
223223
```
224224

225+
:::note Parameter values in requests
226+
227+
Clients send parameter values in the request body as `{"parameters": {"name": value, ...}}`. `parameters` must be a JSON
228+
**object** (a non-object value is rejected with `400`). The individual values are handled per parameter `in`:
229+
230+
- `query`, `path`, `header`, `cookie` — the value is coerced to a string. **Pass a string** (or a number/boolean);
231+
passing a nested object or array here produces a stringified placeholder, not what you want.
232+
- `body` — the value is serialized whole into the upstream request body, so **nested JSON is supported and expected**
233+
here (e.g. a JSON-RPC `params: [{ "to": "0x...", "data": "0x..." }, "latest"]`).
234+
235+
In short: only `body` parameters may be nested; everything else should be a primitive.
236+
237+
:::
238+
225239
### Parameter fields
226240

227241
| Field | Type | Required | Default | Description |

examples/configs/complete/config.yaml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,6 @@ apis:
4747
times: '1e18'
4848
cache:
4949
maxAge: 30000
50-
delay: 60000 # public responses delayed by 60s (OEV window)
5150
description: Get the current price of a coin
5251

5352
- name: coinMarketData

src/config/parser.test.ts

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { beforeAll, describe, expect, test } from 'bun:test';
2+
import { deriveEndpointId } from '../endpoint';
23
import { loadEnvFile } from './env';
34
import { interpolateEnvironment, parseConfig } from './parser';
45

@@ -99,6 +100,84 @@ describe('parseConfig', () => {
99100
expect(() => parseConfig('not: valid: yaml: [')).toThrow();
100101
});
101102

103+
test('flags parameters with env-backed fixed values as secret', () => {
104+
process.env['SECRET_PARAM_VALUE'] = 'super-secret-key';
105+
const raw = `
106+
version: '1.0'
107+
server:
108+
port: 3000
109+
settings:
110+
proof: none
111+
apis:
112+
- name: Test
113+
url: https://api.example.com
114+
endpoints:
115+
- name: getData
116+
path: /data
117+
parameters:
118+
- name: apikey
119+
in: query
120+
fixed: \${SECRET_PARAM_VALUE}
121+
- name: coin
122+
in: query
123+
required: true
124+
`;
125+
const params = parseConfig(raw).apis[0]?.endpoints[0]?.parameters;
126+
const apikey = params?.find((p) => p.name === 'apikey');
127+
expect(apikey?.fixed).toBe('super-secret-key'); // resolved from the environment
128+
expect(apikey?.secret).toBe(true); // and marked secret so it stays out of the endpoint ID
129+
expect(params?.find((p) => p.name === 'coin')?.secret).toBe(false);
130+
});
131+
132+
test('env-backed fixed params do not change the endpoint ID', () => {
133+
process.env['SECRET_PARAM_VALUE'] = 'super-secret-key';
134+
const withSecretParam = `
135+
version: '1.0'
136+
server:
137+
port: 3000
138+
settings:
139+
proof: none
140+
apis:
141+
- name: Test
142+
url: https://api.example.com
143+
endpoints:
144+
- name: getData
145+
path: /data
146+
parameters:
147+
- name: apikey
148+
in: query
149+
fixed: \${SECRET_PARAM_VALUE}
150+
- name: coin
151+
in: query
152+
required: true
153+
`;
154+
const withoutSecretParam = `
155+
version: '1.0'
156+
server:
157+
port: 3000
158+
settings:
159+
proof: none
160+
apis:
161+
- name: Test
162+
url: https://api.example.com
163+
endpoints:
164+
- name: getData
165+
path: /data
166+
parameters:
167+
- name: coin
168+
in: query
169+
required: true
170+
`;
171+
const apiWith = parseConfig(withSecretParam).apis[0];
172+
const apiWithout = parseConfig(withoutSecretParam).apis[0];
173+
const endpointWith = apiWith?.endpoints[0];
174+
const endpointWithout = apiWithout?.endpoints[0];
175+
if (!apiWith || !endpointWith || !apiWithout || !endpointWithout) {
176+
throw new Error('config did not parse as expected');
177+
}
178+
expect(deriveEndpointId(apiWith, endpointWith)).toBe(deriveEndpointId(apiWithout, endpointWithout));
179+
});
180+
102181
test('interpolates header values from environment variables', async () => {
103182
const raw = await loadExample();
104183
const config = parseConfig(raw);

src/config/parser.ts

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,52 @@ export function interpolateEnvironment(raw: string): string {
1414
});
1515
}
1616

17+
// =============================================================================
18+
// Secret parameter detection (must run before interpolation)
19+
//
20+
// A parameter whose `fixed` value references an env var (e.g. `fixed: ${API_KEY}`)
21+
// is treated as secret: its resolved value is excluded from the endpoint ID. This
22+
// has to be detected on the *un-interpolated* config — once `${VAR}` has been
23+
// replaced with the value, there is nothing left to recognise. Interpolation only
24+
// rewrites scalar text (it never adds, removes, or reorders keys), so each
25+
// parameter occupies the same position in the raw and interpolated parses, and we
26+
// can mark `secret: true` on the corresponding entry in the interpolated tree.
27+
// =============================================================================
28+
function asRecord(value: unknown): Record<string, unknown> | undefined {
29+
return typeof value === 'object' && value !== null && !Array.isArray(value)
30+
? (value as Record<string, unknown>)
31+
: undefined;
32+
}
33+
34+
function asArray(value: unknown): readonly unknown[] {
35+
return Array.isArray(value) ? value : [];
36+
}
37+
38+
function markEnvBackedFixedParamsSecret(raw: unknown, interpolated: unknown): void {
39+
const rawApis = asArray(asRecord(raw)?.['apis']);
40+
const apis = asArray(asRecord(interpolated)?.['apis']);
41+
// eslint-disable-next-line functional/no-loop-statements
42+
for (const [apiIndex, rawApi] of rawApis.entries()) {
43+
const rawEndpoints = asArray(asRecord(rawApi)?.['endpoints']);
44+
const endpoints = asArray(asRecord(apis[apiIndex])?.['endpoints']);
45+
// eslint-disable-next-line functional/no-loop-statements
46+
for (const [endpointIndex, rawEndpoint] of rawEndpoints.entries()) {
47+
const rawParams = asArray(asRecord(rawEndpoint)?.['parameters']);
48+
const params = asArray(asRecord(endpoints[endpointIndex])?.['parameters']);
49+
// eslint-disable-next-line functional/no-loop-statements
50+
for (const [paramIndex, rawParam] of rawParams.entries()) {
51+
const fixed = asRecord(rawParam)?.['fixed'];
52+
if (typeof fixed !== 'string' || !fixed.startsWith('${')) continue;
53+
const target = asRecord(params[paramIndex]);
54+
if (target) target['secret'] = true; // eslint-disable-line functional/immutable-data
55+
}
56+
}
57+
}
58+
}
59+
1760
export function parseConfig(raw: string): Config {
1861
const interpolated = interpolateEnvironment(raw);
1962
const data: unknown = parse(interpolated);
63+
markEnvBackedFixedParamsSecret(parse(raw), data);
2064
return configSchema.parse(data);
2165
}

src/config/schema.test.ts

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,24 @@ apis:
129129
expect(auth.expiry).toBe(300_000);
130130
});
131131

132+
test('rejects a non-numeric x402 amount', () => {
133+
const raw = MINIMAL_CONFIG.replace(
134+
'url: https://api.example.com',
135+
`url: https://api.example.com
136+
auth:
137+
type: x402
138+
network: 8453
139+
rpc: https://mainnet.base.org
140+
token: '0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48'
141+
amount: '1 USDC'
142+
recipient: '0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266'`
143+
);
144+
expect(() => configSchema.parse(parseYaml(raw))).toThrow();
145+
146+
const withDecimal = raw.replace("amount: '1 USDC'", "amount: '1.5'");
147+
expect(() => configSchema.parse(parseYaml(withDecimal))).toThrow();
148+
});
149+
132150
test('validates auth as array (multi-method)', () => {
133151
const raw = MINIMAL_CONFIG.replace(
134152
'url: https://api.example.com',
@@ -197,7 +215,7 @@ apis:
197215
expect(result.apis[0]?.endpoints[0]?.cache).toEqual({ maxAge: 5000 });
198216
});
199217

200-
test('validates cache with delay', () => {
218+
test('strips unknown cache keys', () => {
201219
const raw = MINIMAL_CONFIG.replace(
202220
'path: /test',
203221
`path: /test
@@ -206,7 +224,7 @@ apis:
206224
delay: 60000`
207225
);
208226
const result = configSchema.parse(parseYaml(raw));
209-
expect(result.apis[0]?.endpoints[0]?.cache).toEqual({ maxAge: 5000, delay: 60_000 });
227+
expect(result.apis[0]?.endpoints[0]?.cache).toEqual({ maxAge: 5000 });
210228
});
211229

212230
test('validates parameters with secret flag', () => {

src/config/schema.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,10 @@ const x402ClientAuthSchema = z.object({
7373
network: z.number().int().positive(),
7474
rpc: z.url(),
7575
token: evmAddressSchema,
76-
amount: z.string().min(1),
76+
// Base-unit integer amount (e.g. wei, or token base units) — kept as a string
77+
// to preserve precision and parsed with BigInt at request time, so it must be
78+
// a plain non-negative integer with no decimal point, sign, or units.
79+
amount: z.string().regex(/^\d+$/, 'Must be a non-negative integer string (token base units)'),
7780
recipient: evmAddressSchema,
7881
expiry: z.number().int().positive().default(300_000),
7982
});
@@ -92,7 +95,6 @@ export const clientAuthSchema = z.union([clientAuthMethodSchema, z.array(clientA
9295
// =============================================================================
9396
export const cacheSchema = z.object({
9497
maxAge: z.number().int().positive(),
95-
delay: z.number().int().nonnegative().optional(),
9698
});
9799

98100
// =============================================================================

src/config/validate.test.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,29 @@ apis:
146146
expect(result.errors[0]).toContain('Duplicate API name');
147147
});
148148

149+
test('rejects endpoints with identical specifications (colliding endpoint IDs)', () => {
150+
const yaml = `
151+
version: '1.0'
152+
server:
153+
port: 3000
154+
settings:
155+
proof: none
156+
apis:
157+
- name: Test
158+
url: https://api.example.com
159+
endpoints:
160+
- name: first
161+
path: /data
162+
- name: second
163+
path: /data
164+
`;
165+
const result = validateConfig(yaml);
166+
167+
expect(result.success).toBe(false);
168+
if (result.success) return;
169+
expect(result.errors.some((e) => e.includes('identical specifications'))).toBe(true);
170+
});
171+
149172
// ===========================================================================
150173
// Multiple errors
151174
// ===========================================================================

src/config/validate.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { goSync } from '@api3/promise-utils';
22
import { parse } from 'yaml';
33
import type { z } from 'zod/v4';
4+
import { deriveEndpointId } from '../endpoint';
45
import { interpolateEnvironment } from './parser';
56
import { configSchema } from './schema';
67

@@ -42,7 +43,19 @@ function crossFieldErrors(config: z.infer<typeof configSchema>): readonly string
4243
? [`Duplicate plugin source(s): ${[...new Set(duplicatePluginSources)].join(', ')}`]
4344
: [];
4445

45-
return [...apiNameErrors, ...endpointNameErrors, ...pluginErrors];
46+
// Two endpoints with identical specifications derive the same endpoint ID,
47+
// which would silently shadow one another in the endpoint map.
48+
const endpointEntries = config.apis.flatMap((api) =>
49+
api.endpoints.map((endpoint) => ({ id: deriveEndpointId(api, endpoint), label: `${api.name}/${endpoint.name}` }))
50+
);
51+
const endpointIds = endpointEntries.map((e) => e.id);
52+
const duplicateEndpointIds = endpointIds.filter((id, i, all) => all.indexOf(id) !== i);
53+
const endpointIdErrors = [...new Set(duplicateEndpointIds)].map((id) => {
54+
const labels = endpointEntries.filter((e) => e.id === id).map((e) => e.label);
55+
return `Endpoints with identical specifications (same endpoint ID ${id}): ${labels.join(', ')}`;
56+
});
57+
58+
return [...apiNameErrors, ...endpointNameErrors, ...pluginErrors, ...endpointIdErrors];
4659
}
4760

4861
// =============================================================================

src/endpoint.test.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,4 +259,29 @@ describe('buildEndpointMap', () => {
259259
const map = buildEndpointMap(config);
260260
expect(map.size).toBe(3);
261261
});
262+
263+
test('throws when two endpoints derive the same ID', () => {
264+
const api = makeApi({
265+
url: 'https://api.example.com',
266+
// Same path + method + (no) params + (no) encoding → identical endpoint ID.
267+
endpoints: [makeEndpoint({ name: 'first', path: '/data' }), makeEndpoint({ name: 'second', path: '/data' })],
268+
});
269+
270+
expect(() => buildEndpointMap(makeConfig([api]))).toThrow(/same endpoint ID/);
271+
});
272+
273+
test('throws when endpoints across different APIs with the same url collide', () => {
274+
const api1 = makeApi({
275+
name: 'A',
276+
url: 'https://shared.example.com',
277+
endpoints: [makeEndpoint({ name: 'x', path: '/p' })],
278+
});
279+
const api2 = makeApi({
280+
name: 'B',
281+
url: 'https://shared.example.com',
282+
endpoints: [makeEndpoint({ name: 'y', path: '/p' })],
283+
});
284+
285+
expect(() => buildEndpointMap(makeConfig([api1, api2]))).toThrow(/same endpoint ID/);
286+
});
262287
});

src/endpoint.ts

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -59,15 +59,28 @@ function deriveEndpointId(api: Api, endpoint: Endpoint): Hex {
5959

6060
// =============================================================================
6161
// Endpoint map
62+
//
63+
// Two endpoints with identical specifications (URL, path, method, parameters,
64+
// encoding, encryption) derive the same ID. Building the map silently with
65+
// `new Map(entries)` would let the later one shadow the earlier — so we detect
66+
// the collision and fail loudly instead.
6267
// =============================================================================
6368
function buildEndpointMap(config: Config): ReadonlyMap<Hex, ResolvedEndpoint> {
6469
const entries = config.apis.flatMap((api) =>
65-
api.endpoints.map((endpoint) => {
66-
const endpointId = deriveEndpointId(api, endpoint);
67-
return [endpointId, { api, endpoint }] as const;
68-
})
70+
api.endpoints.map((endpoint) => [deriveEndpointId(api, endpoint), { api, endpoint }] as const)
6971
);
7072

73+
const ids = entries.map(([id]) => id);
74+
const duplicateId = ids.find((id, index) => ids.indexOf(id) !== index);
75+
if (duplicateId) {
76+
const colliding = entries
77+
.filter(([id]) => id === duplicateId)
78+
.map(([, resolved]) => `"${resolved.api.name}/${resolved.endpoint.name}"`);
79+
throw new Error(
80+
`Endpoints ${colliding.join(' and ')} derive the same endpoint ID ${duplicateId} — their specifications are identical`
81+
);
82+
}
83+
7184
return new Map(entries);
7285
}
7386

0 commit comments

Comments
 (0)