From 243a0fbc06ad1d76cca6c20d28c8adb8e26fbc1c Mon Sep 17 00:00:00 2001 From: Alex Gherghisan Date: Tue, 3 Oct 2023 14:14:02 +0200 Subject: [PATCH] fix: JSON-RPC server returns spec-compliant errors (#2590) Fix #2510 # Checklist: Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge. - [ ] If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag. - [x] I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code. - [x] Every change is related to the PR description. - [x] I have [linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue) this pull request to relevant issues (if any exist). --- .../src/json-rpc/client/json_rpc_client.ts | 4 +- .../json-rpc/server/json_rpc_server.test.ts | 27 ++++++++++ .../src/json-rpc/server/json_rpc_server.ts | 54 ++++++++++++++++--- 3 files changed, 77 insertions(+), 8 deletions(-) diff --git a/yarn-project/foundation/src/json-rpc/client/json_rpc_client.ts b/yarn-project/foundation/src/json-rpc/client/json_rpc_client.ts index 39ddefae1a19..c8f118bdb6cd 100644 --- a/yarn-project/foundation/src/json-rpc/client/json_rpc_client.ts +++ b/yarn-project/foundation/src/json-rpc/client/json_rpc_client.ts @@ -56,9 +56,9 @@ export async function defaultFetch( } if (!resp.ok) { if (noRetry) { - throw new NoRetryError(responseJson.error); + throw new NoRetryError(responseJson.error.message); } else { - throw new Error(responseJson.error); + throw new Error(responseJson.error.message); } } diff --git a/yarn-project/foundation/src/json-rpc/server/json_rpc_server.test.ts b/yarn-project/foundation/src/json-rpc/server/json_rpc_server.test.ts index 23adea9c325c..e8e4d2edc454 100644 --- a/yarn-project/foundation/src/json-rpc/server/json_rpc_server.test.ts +++ b/yarn-project/foundation/src/json-rpc/server/json_rpc_server.test.ts @@ -22,3 +22,30 @@ test('test an RPC function with an array of classes', async () => { expect(response.status).toBe(200); expect(response.text).toBe(JSON.stringify({ result: [{ data: 'a' }, { data: 'b' }, { data: 'c' }] })); }); + +test('test invalid JSON', async () => { + const server = new JsonRpcServer(new TestState([]), { TestNote }, {}, false); + const response = await request(server.getApp().callback()).post('/').send('{'); + expect(response.status).toBe(400); + expect(response.body).toEqual({ + error: { code: -32700, message: 'Parse error' }, + id: null, + jsonrpc: '2.0', + }); +}); + +test('invalid method', async () => { + const server = new JsonRpcServer(new TestState([]), { TestNote }, {}, false); + const response = await request(server.getApp().callback()).post('/').send({ + jsonrpc: '2.0', + method: 'invalid', + params: [], + id: 42, + }); + expect(response.status).toBe(400); + expect(response.body).toEqual({ + error: { code: -32601, message: 'Method not found' }, + id: 42, + jsonrpc: '2.0', + }); +}); diff --git a/yarn-project/foundation/src/json-rpc/server/json_rpc_server.ts b/yarn-project/foundation/src/json-rpc/server/json_rpc_server.ts index 34c7e851c98e..4302a42b657d 100644 --- a/yarn-project/foundation/src/json-rpc/server/json_rpc_server.ts +++ b/yarn-project/foundation/src/json-rpc/server/json_rpc_server.ts @@ -39,14 +39,34 @@ export class JsonRpcServer { await next(); } catch (err: any) { this.log.error(err); - ctx.status = 400; - ctx.body = { error: err.message }; + if (err instanceof SyntaxError) { + ctx.status = 400; + ctx.body = { + jsonrpc: '2.0', + id: null, + error: { + code: -32700, + message: 'Parse error', + }, + }; + } else { + ctx.status = 500; + ctx.body = { + jsonrpc: '2.0', + id: null, + error: { + code: -32603, + message: 'Internal error', + }, + }; + } } }; const app = new Koa(); app.on('error', error => { this.log.error(`Error on API handler: ${error}`); }); + app.use(exceptionHandler); app.use(compress({ br: false } as any)); app.use( bodyParser({ @@ -56,7 +76,6 @@ export class JsonRpcServer { }), ); app.use(cors()); - app.use(exceptionHandler); app.use(router.routes()); app.use(router.allowedMethods()); @@ -98,7 +117,15 @@ export class JsonRpcServer { // Propagate the error message to the client. Plenty of the errors are expected to occur (e.g. adding // a duplicate recipient) so this is necessary. ctx.status = 400; - ctx.body = { error: err.message }; + ctx.body = { + jsonrpc, + id, + error: { + // TODO assign error codes - https://github.com/AztecProtocol/aztec-packages/issues/2633 + code: -32000, + message: err.message, + }, + }; } }); } @@ -113,7 +140,14 @@ export class JsonRpcServer { this.disallowedMethods.includes(method) ) { ctx.status = 400; - ctx.body = { error: `Invalid method name: ${method}` }; + ctx.body = { + jsonrpc, + id, + error: { + code: -32601, + message: 'Method not found', + }, + }; } else { try { const result = await this.proxy.call(method, params); @@ -127,7 +161,15 @@ export class JsonRpcServer { // Propagate the error message to the client. Plenty of the errors are expected to occur (e.g. adding // a duplicate recipient) so this is necessary. ctx.status = 400; - ctx.body = { error: err.message }; + ctx.body = { + jsonrpc, + id, + error: { + // TODO assign error codes - https://github.com/AztecProtocol/aztec-packages/issues/2633 + code: -32000, + message: err.message, + }, + }; } } });