fix(core): reject invalid JSON-RPC params#181
Conversation
| request: { | ||
| apiNamespace: parsed.method.substring(0, dotIndex), | ||
| method: parsed.method.substring(dotIndex + 1), | ||
| // JSON-RPC 2.0 §4.2: params may be an array (positional) or object (named). |
There was a problem hiding this comment.
The comment only mentions array (positional) and object (named), but after the refactor args is also [] when params is omitted entirely. Suggest updating so the third path is documented:
| // JSON-RPC 2.0 §4.2: params may be an array (positional) or object (named). | |
| // JSON-RPC 2.0 §4.2: params may be array (positional), object (named), or omitted (empty). |
| } | ||
| }); | ||
|
|
||
| for (const params of ['abc', 42, true, null]) { |
There was a problem hiding this comment.
false is the only boolean not covered here — it's rejected by the same typeof !== 'object' guard but is absent from the set. Adding it completes coverage of all four primitive types (string, number, boolean, null):
| for (const params of ['abc', 42, true, null]) { | |
| for (const params of ['abc', 42, true, false, null]) { |
| const response = JSON.parse(result.response); | ||
| assert.strictEqual(response.error.code, RpcErrorCode.InvalidParams); | ||
| assert.strictEqual(response.error.data.name, 'InvalidParams'); | ||
| assert.strictEqual(response.id, 'request-1'); |
There was a problem hiding this comment.
These tests assert error.code and error.data.name but not error.message. The existing -32600 tests assert message content; the user-facing -32602 message could regress silently. One assertion would close the gap:
assert.ok(response.error.message.includes('expected an array or object'));
| } | ||
| }); | ||
|
|
||
| it('accepts named object params using their value order', () => { |
There was a problem hiding this comment.
nit: "using their value order" leans on an Object.values() implementation detail. JSON-RPC doesn't guarantee param ordering for named objects, so a neutral name like 'accepts named object params' avoids setting a false reader expectation.
ahmedhamouda78
left a comment
ahmedhamouda78
left a comment
There was a problem hiding this comment.
Approving. This is a clean, spec-correct fix for #179 — the -32602 Invalid Params guard is exactly right: Object.hasOwn distinguishes absent from present params, and the params === null || typeof params !== 'object' check correctly covers all four primitive types (the typeof null === 'object' trap is handled by the explicit === null arm). Preserving the request id in the error response is spec-required and done correctly. Extracting let args from the inline ternary is a nice readability win, and patch is the right changeset level.
Left a few non-blocking notes: add false to the rejection test loop for full boolean coverage, assert error.message in one -32602 case to guard against silent message regressions, refresh the args comment at rpc.ts:130 to mention the omitted-params path, and a naming nit on the named-object test. None block merge.
bea9774 to
0852303
Compare
|
@ahmedhamouda78 |
2 participants
Problem
JSON-RPC requests with primitive or
nullparamsare currently accepted instead of returning the standard-32602 Invalid Paramserror. TheObject.values(parsed.params ?? {})fallback turns strings into character arguments and numbers, booleans, andnullinto an empty argument list, so malformed requests can reach application methods with unexpected arguments.Closes #179.
Changes
paramsproperty before constructing method arguments.RpcErrorCode.InvalidParamswithdata.name: "InvalidParams"for primitive andnullvalues while preserving the request id.params.@aws-blocks/core.Validation
npm cinpm run build:packagesnpm run build -w @aws-blocks/corenode --test --test-concurrency=1 dist/*.test.js dist/**/*.test.jsfrompackages/core(588 passed, 0 failed)npm run lintnpm run lint:depsnpm run check:exports-consistencynpm run check:apinpx @changesets/cli status --since=origin/mainnpx tsx scripts/verify-changeset-coverage.tsgit diff --cached --checkChecklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.