From a1e894a4455fc59c1818016d396fe0739493a8ce Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 3 Jan 2023 14:52:42 +1030 Subject: [PATCH] lightningd: treat JSON ids as direct tokens. This avoids any confusion between primitive and string ids, and in particular stops an issue with commando once it starts chaining ids, that weird ids can be double-escaped and commando will not recognize the response, leaving the client hanging. It's the client's fault for using a weird id, but it's still rude (and triggered by our tests!). It also makes substituting the id in passthrough simpler, FTW. Signed-off-by: Rusty Russell --- lightningd/jsonrpc.c | 46 ++++++++++++++++++++--------------- lightningd/plugin.c | 54 ++++++++++++++++-------------------------- tests/test_invoices.py | 2 +- tests/test_misc.py | 2 +- tests/test_plugin.py | 2 +- tests/test_wallet.py | 2 +- 6 files changed, 52 insertions(+), 56 deletions(-) diff --git a/lightningd/jsonrpc.c b/lightningd/jsonrpc.c index 9aec11114..fe4e2d759 100644 --- a/lightningd/jsonrpc.c +++ b/lightningd/jsonrpc.c @@ -203,7 +203,9 @@ static struct command_result *json_stop(struct command *cmd, jout = json_out_new(tmpctx); json_out_start(jout, NULL, '{'); json_out_addstr(jout, "jsonrpc", "2.0"); - json_out_add(jout, "id", cmd->id_is_string, "%s", cmd->id); + /* Copy input id token exactly */ + memcpy(json_out_member_direct(jout, "id", strlen(cmd->id)), + cmd->id, strlen(cmd->id)); json_out_addstr(jout, "result", "Shutdown complete"); json_out_end(jout, '}'); json_out_finished(jout); @@ -557,10 +559,7 @@ void json_notify_fmt(struct command *cmd, json_add_string(js, "jsonrpc", "2.0"); json_add_string(js, "method", "message"); json_object_start(js, "params"); - if (cmd->id_is_string) - json_add_string(js, "id", cmd->id); - else - json_add_jsonstr(js, "id", cmd->id, strlen(cmd->id)); + json_add_id(js, cmd->id); json_add_string(js, "level", log_level_name(level)); json_add_string(js, "message", tal_vfmt(tmpctx, fmt, ap)); json_object_end(js); @@ -605,10 +604,7 @@ static struct json_stream *json_start(struct command *cmd) json_object_start(js, NULL); json_add_string(js, "jsonrpc", "2.0"); - if (cmd->id_is_string) - json_add_string(js, "id", cmd->id); - else - json_add_jsonstr(js, "id", cmd->id, strlen(cmd->id)); + json_add_id(js, cmd->id); return js; } @@ -934,7 +930,10 @@ parse_request(struct json_connection *jcon, const jsmntok_t tok[]) c->pending = false; c->json_stream = NULL; c->id_is_string = (id->type == JSMN_STRING); - c->id = json_strdup(c, jcon->buffer, id); + /* Include "" around string */ + c->id = tal_strndup(c, + json_tok_full(jcon->buffer, id), + json_tok_full_len(id)); c->mode = CMD_NORMAL; c->filter = NULL; list_add_tail(&jcon->commands, &c->list); @@ -1068,7 +1067,7 @@ again: json_command_malformed( jcon, "null", tal_fmt(tmpctx, "Invalid token in json input: '%s'", - tal_strndup(tmpctx, jcon->buffer, jcon->used))); + tal_hexstr(tmpctx, jcon->buffer, jcon->used))); if (in_transaction) db_commit_transaction(jcon->ld->wallet->db); return io_halfclose(conn); @@ -1400,11 +1399,23 @@ struct jsonrpc_request *jsonrpc_request_start_( r->id_is_string = id_as_string; if (r->id_is_string) { - if (id_prefix) - r->id = tal_fmt(r, "%s/cln:%s#%"PRIu64, + if (id_prefix) { + /* Strip "" and otherwise sanity-check */ + if (strstarts(id_prefix, "\"") + && strlen(id_prefix) > 1 + && strends(id_prefix, "\"")) { + id_prefix = tal_strndup(tmpctx, id_prefix + 1, + strlen(id_prefix) - 2); + } + /* We could try escaping, but TBH they're + * messing with us at this point! */ + if (json_escape_needed(id_prefix, strlen(id_prefix))) + id_prefix = "weird-id"; + + r->id = tal_fmt(r, "\"%s/cln:%s#%"PRIu64"\"", id_prefix, method, next_request_id); - else - r->id = tal_fmt(r, "cln:%s#%"PRIu64, method, next_request_id); + } else + r->id = tal_fmt(r, "\"cln:%s#%"PRIu64"\"", method, next_request_id); } else { r->id = tal_fmt(r, "%"PRIu64, next_request_id); } @@ -1423,10 +1434,7 @@ struct jsonrpc_request *jsonrpc_request_start_( if (add_header) { json_object_start(r->stream, NULL); json_add_string(r->stream, "jsonrpc", "2.0"); - if (r->id_is_string) - json_add_string(r->stream, "id", r->id); - else - json_add_primitive(r->stream, "id", r->id); + json_add_id(r->stream, r->id); json_add_string(r->stream, "method", method); json_object_start(r->stream, "params"); } diff --git a/lightningd/plugin.c b/lightningd/plugin.c index 8a883e220..fcd6caef9 100644 --- a/lightningd/plugin.c +++ b/lightningd/plugin.c @@ -448,13 +448,16 @@ static const char *plugin_notify_handle(struct plugin *plugin, "JSON-RPC notify \"id\"-field is not present"); } + /* Include any "" in id */ request = strmap_getn(&plugin->plugins->pending_requests, - plugin->buffer + idtok->start, - idtok->end - idtok->start); + json_tok_full(plugin->buffer, idtok), + json_tok_full_len(idtok)); if (!request) { return tal_fmt( plugin, - "Received a JSON-RPC notify for non-existent request"); + "Received a JSON-RPC notify for non-existent request '%.*s'", + json_tok_full_len(idtok), + json_tok_full(plugin->buffer, idtok)); } /* Ignore if they don't have a callback */ @@ -572,12 +575,14 @@ static const char *plugin_response_handle(struct plugin *plugin, struct jsonrpc_request *request; request = strmap_getn(&plugin->plugins->pending_requests, - plugin->buffer + idtok->start, - idtok->end - idtok->start); + json_tok_full(plugin->buffer, idtok), + json_tok_full_len(idtok)); if (!request) { return tal_fmt( plugin, - "Received a JSON-RPC response for non-existent request"); + "Received a JSON-RPC response for non-existent request '%.*s'", + json_tok_full_len(idtok), + json_tok_full(plugin->buffer, idtok)); } /* We expect the request->cb to copy if needed */ @@ -1028,37 +1033,23 @@ static void json_stream_forward_change_id(struct json_stream *stream, const char *buffer, const jsmntok_t *toks, const jsmntok_t *idtok, - const char *new_id, - bool new_id_is_str) + /* Full token, including "" */ + const char *new_id) { /* We copy everything, but replace the id. Special care has to * be taken when the id that is being replaced is a string. If * we don't crop the quotes off we'll transform a numeric * new_id into a string, or even worse, quote a string id * twice. */ - size_t offset = 0; - bool add_quotes = false; + const char *id_start, *id_end; - if (idtok->type == JSMN_STRING) { - if (new_id_is_str) - add_quotes = false; - else - offset = 1; - } else { - if (new_id_is_str) - add_quotes = true; - } + id_start = json_tok_full(buffer, idtok); + id_end = id_start + json_tok_full_len(idtok); json_stream_append(stream, buffer + toks->start, - idtok->start - toks->start - offset); - - if (add_quotes) - json_stream_append(stream, "\"", 1); + id_start - (buffer + toks->start)); json_stream_append(stream, new_id, strlen(new_id)); - if (add_quotes) - json_stream_append(stream, "\"", 1); - json_stream_append(stream, buffer + idtok->end + offset, - toks->end - idtok->end - offset); + json_stream_append(stream, id_end, (buffer + toks->end) - id_end); } static void plugin_rpcmethod_cb(const char *buffer, @@ -1070,8 +1061,7 @@ static void plugin_rpcmethod_cb(const char *buffer, struct json_stream *response; response = json_stream_raw_for_cmd(cmd); - json_stream_forward_change_id(response, buffer, toks, idtok, cmd->id, - cmd->id_is_string); + json_stream_forward_change_id(response, buffer, toks, idtok, cmd->id); json_stream_double_cr(response); command_raw_complete(cmd, response); @@ -1097,8 +1087,7 @@ static void plugin_notify_cb(const char *buffer, json_add_tok(response, "method", methodtok, buffer); json_stream_append(response, ",\"params\":", strlen(",\"params\":")); json_stream_forward_change_id(response, buffer, - paramtoks, idtok, cmd->id, - cmd->id_is_string); + paramtoks, idtok, cmd->id); json_object_end(response); json_stream_double_cr(response); @@ -1155,8 +1144,7 @@ static struct command_result *plugin_rpcmethod_dispatch(struct command *cmd, call->plugin = plugin; list_add_tail(&plugin->pending_rpccalls, &call->list); - json_stream_forward_change_id(req->stream, buffer, toks, idtok, req->id, - req->id_is_string); + json_stream_forward_change_id(req->stream, buffer, toks, idtok, req->id); json_stream_double_cr(req->stream); plugin_request_send(plugin, req); req->stream = NULL; diff --git a/tests/test_invoices.py b/tests/test_invoices.py index 195864d44..5df57e843 100644 --- a/tests/test_invoices.py +++ b/tests/test_invoices.py @@ -21,7 +21,7 @@ def test_invoice(node_factory, chainparams): # Side note: invoice calls out to listincoming, so check JSON id is as expected myname = os.path.splitext(os.path.basename(sys.argv[0]))[0] - l1.daemon.wait_for_log(r": {}:invoice#[0-9]*/cln:listincoming#[0-9]*\[OUT\]".format(myname)) + l1.daemon.wait_for_log(r': "{}:invoice#[0-9]*/cln:listincoming#[0-9]*"\[OUT\]'.format(myname)) after = int(time.time()) b11 = l1.rpc.decodepay(inv['bolt11']) diff --git a/tests/test_misc.py b/tests/test_misc.py index 8dfe56c46..13316dd32 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -874,7 +874,7 @@ def test_cli(node_factory): assert 'help [command]\n List available commands, or give verbose help on one {command}' in out # Check JSON id is as expected - l1.daemon.wait_for_log(r"jsonrpc#[0-9]*: cli:help#[0-9]*\[IN\]") + l1.daemon.wait_for_log(r'jsonrpc#[0-9]*: "cli:help#[0-9]*"\[IN\]') # Test JSON output. out = subprocess.check_output(['cli/lightning-cli', diff --git a/tests/test_plugin.py b/tests/test_plugin.py index aeb54cef5..7f127d75d 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -1539,7 +1539,7 @@ def test_libplugin(node_factory): # Test hooks and notifications (add plugin, so we can test hook id) l2 = node_factory.get_node(options={"plugin": plugin, 'log-level': 'io'}) l2.connect(l1) - l2.daemon.wait_for_log(r": {}:connect#[0-9]*/cln:peer_connected#[0-9]*\[OUT\]".format(myname)) + l2.daemon.wait_for_log(r': "{}:connect#[0-9]*/cln:peer_connected#[0-9]*"\[OUT\]'.format(myname)) l1.daemon.wait_for_log("{} peer_connected".format(l2.info["id"])) l1.daemon.wait_for_log("{} connected".format(l2.info["id"])) diff --git a/tests/test_wallet.py b/tests/test_wallet.py index a90fca091..be6275a17 100644 --- a/tests/test_wallet.py +++ b/tests/test_wallet.py @@ -62,7 +62,7 @@ def test_withdraw(node_factory, bitcoind): # Side note: sendrawtransaction will trace back to withdrawl myname = os.path.splitext(os.path.basename(sys.argv[0]))[0] - l1.daemon.wait_for_log(r": {}:withdraw#[0-9]*/cln:withdraw#[0-9]*/txprepare:sendpsbt#[0-9]*/cln:sendrawtransaction#[0-9]*\[OUT\]".format(myname)) + l1.daemon.wait_for_log(r': "{}:withdraw#[0-9]*/cln:withdraw#[0-9]*/txprepare:sendpsbt#[0-9]*/cln:sendrawtransaction#[0-9]*"\[OUT\]'.format(myname)) # Make sure bitcoind received the withdrawal unspent = l1.bitcoin.rpc.listunspent(0)