← Watch feed
libplugins: JSON ids are always strings.
Public commit record
80/100 · Strong
What the developer wrote
Authored by Rusty Russell
libplugins: JSON ids are always strings.
We used to handle it being a literal, but this was removed in
73fc9b0c2a162c98703baf618abf944c791bb879 (v25.05) so we don't need to handle that at all.
Not using the raw JSON means we handle weird methodnames by replacement: otherwise we would
not match the responses. Only an issue for commando, where the command would time out
rather than report "Unknown method".
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
✓ Descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Explains rationale or failure mode
What changed, and why it matters
This commit cleans up how Core Lightning plugins handle JSON-RPC request IDs. Previously, IDs could be passed around as raw JSON fragments (with quotes included), which caused mismatches when method names contained unusual characters. The fix forces IDs to be plain strings everywhere. The main user-visible effect mentioned by the author is that commando (a remote-command plugin) would previously time out instead of returning 'Unknown method' for certain weird method names. There is no direct evidence in the commit of a security vulnerability such as injection or remote code execution.
Recommended action
Treat as a normal robustness/correctness patch. Reviewers may want to confirm that all call sites now use idstr and that no plugin still emits raw JSON ids, but no urgent security response is indicated by the supplied materials.
Security signals we found
01
JSON-RPC id handling changed from raw JSON literal to string
02
Method names with special characters are now escaped or replaced to prevent reply mismatch
03
Incoming JSON-RPC ids are validated to be strings
04
No mention of CVE, security advisory, or independent researcher attribution in commit or supplied references
Evidence from the diff
The patch renames cmd->id / out_req->id to cmd->idstr / out_req->idstr and consistently treats JSON-RPC ids as strings rather than raw JSON literals. It removes quote-stripping logic in json_id(), replaces json_add_id() with json_add_string(…, “id”, idstr), and adds validation that incoming JSON-RPC ids are JSMN_STRING. It also escapes or replaces weird method names when constructing outgoing request IDs so replies can be matched. The change is framed as a correctness/robustness fix, not a security fix, and the only functional symptom described is a commando timeout vs. proper error reporting.
Changed components
plugins/libplugin.cplugins/libplugin.hplugins/commando.cplugins/libplugin-pay.cplugins/askrene/askrene.cplugins/askrene/child/child.cInspect captured patch +72 / −61
diff --git a/plugins/askrene/askrene.c b/plugins/askrene/askrene.c
index 70247c1e..b503000c 100644
--- a/plugins/askrene/askrene.c
+++ b/plugins/askrene/askrene.c
@@ -317,7 +317,7 @@ static const char *cmd_log(const tal_t *ctx,
if (level != LOG_DBG)
plugin_log(cmd->plugin,
level == LOG_BROKEN ? level : level - 1,
- "%s: %s", cmd->id, msg);
+ "%s: %s", cmd->idstr, msg);
return msg;
}
@@ -718,7 +718,7 @@ static struct command_result *do_getroutes(struct command *cmd,
deadline, srcnode, dstnode, info->amount,
info->maxfee, info->finalcltv, info->maxdelay, info->maxparts,
include_fees,
- cmd->id, cmd->filter,
+ cmd->idstr, cmd->filter,
include_next_node_id,
include_amount_msat,
include_delay,
@@ -989,7 +989,7 @@ static struct command_result *json_askrene_reserve(struct command *cmd,
json_tok_full_len(params), json_tok_full(buffer, params));
for (size_t i = 0; i < tal_count(path); i++)
- reserve_add(askrene->reserved, &path[i], cmd->id);
+ reserve_add(askrene->reserved, &path[i], cmd->idstr);
response = jsonrpc_stream_success(cmd);
return command_finished(cmd, response);
diff --git a/plugins/askrene/child/child.c b/plugins/askrene/child/child.c
index 72aecd68..7b9dcdc9 100644
--- a/plugins/askrene/child/child.c
+++ b/plugins/askrene/child/child.c
@@ -269,7 +269,7 @@ void run_child(const struct gossmap *gossmap,
struct json_stream *js = new_json_stream(tmpctx, NULL, NULL);
json_object_start(js, NULL);
json_add_string(js, "jsonrpc", "2.0");
- json_add_id(js, cmd_id);
+ json_add_string(js, "id", cmd_id);
json_object_start(js, "result");
if (cmd_filter)
json_stream_attach_filter(js, cmd_filter);
diff --git a/plugins/commando.c b/plugins/commando.c
index a256f0b0..e7eb9b3a 100644
--- a/plugins/commando.c
+++ b/plugins/commando.c
@@ -673,7 +673,7 @@ static struct command_result *json_commando(struct command *cmd,
ocmd = new_commando(cmd, cmd, peer, oid);
ocmd->contents = tal_arr(ocmd, u8, 0);
- ocmd->json_id = tal_strdup(ocmd, cmd->id);
+ ocmd->json_id = tal_fmt(ocmd, "\"%s\"", cmd->idstr);
tal_arr_expand(&outgoing_commands, ocmd);
tal_add_destructor2(ocmd, destroy_commando, &outgoing_commands);
diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c
index 9504950a..2527ec9c 100644
--- a/plugins/libplugin-pay.c
+++ b/plugins/libplugin-pay.c
@@ -181,10 +181,7 @@ struct payment *payment_root(struct payment *p)
static const char *paymod_log_header(const tal_t *ctx,
struct payment *p)
{
- const char *id = payment_cmd(p)->id;
- if (strstarts(id, "\""))
- return tal_strndup(ctx, id+1, strlen(id+1)-1);
- return tal_strdup(ctx, id);
+ return tal_strdup(ctx, payment_cmd(p)->idstr);
}
void
diff --git a/plugins/libplugin.c b/plugins/libplugin.c
index 2bc153f6..d720979c 100644
--- a/plugins/libplugin.c
+++ b/plugins/libplugin.c
@@ -1,5 +1,6 @@
#include "config.h"
#include <ccan/io/io.h>
+#include <ccan/json_escape/json_escape.h>
#include <ccan/json_out/json_out.h>
#include <ccan/read_write_all/read_write_all.h>
#include <ccan/tal/path/path.h>
@@ -175,7 +176,7 @@ static struct command *new_command(const tal_t *ctx,
cmd->type = type;
cmd->filter = NULL;
cmd->methodname = tal_strdup(cmd, methodname);
- cmd->id = tal_strdup(cmd, id);
+ cmd->idstr = tal_strdup(cmd, id);
return cmd;
}
@@ -208,11 +209,11 @@ static void complain_deprecated(const char *feature,
/* Mild log message for disallowing */
plugin_log(cmd->plugin, LOG_DBG,
"Note: disallowing deprecated %s for %s",
- feature, cmd->id);
+ feature, cmd->idstr);
} else {
plugin_log(cmd->plugin, LOG_BROKEN,
"DEPRECATED API USED: %s by %s",
- feature, cmd->id);
+ feature, cmd->idstr);
}
}
@@ -342,27 +343,16 @@ struct command_result *plugin_broken_cb(struct command *cmd,
static const char *json_id(const tal_t *ctx, struct plugin *plugin,
const char *method, const char *prefix)
{
- const char *rawid;
- int rawidlen;
-
- /* Strip quotes! */
- if (strstarts(prefix, "\"")) {
- assert(strlen(prefix) >= 2);
- assert(strends(prefix, "\""));
- rawid = prefix + 1;
- rawidlen = strlen(prefix) - 2;
- } else {
- rawid = prefix;
- rawidlen = strlen(prefix);
- }
-
- return tal_fmt(ctx, "\"%.*s/%s:%s#%"PRIu64"\"",
- rawidlen, rawid, plugin->id, method, plugin->next_outreq_id++);
+ /* Don't create weird IDs, they will get escaped and we won't match the reply. */
+ if (json_escape_needed(method, strlen(method)))
+ method = "!weird!";
+ return tal_fmt(ctx, "%s/%s:%s#%"PRIu64,
+ prefix, plugin->id, method, plugin->next_outreq_id++);
}
static void destroy_out_req(struct out_req *out_req, struct plugin *plugin)
{
- strmap_del(&plugin->out_reqs, out_req->id, NULL);
+ strmap_del(&plugin->out_reqs, out_req->idstr, NULL);
}
/* FIXME: Move lightningd/jsonrpc to common/ ? */
@@ -389,18 +379,18 @@ jsonrpc_request_start_(struct command *cmd,
assert(cmd);
out = tal(cmd, struct out_req);
out->method = tal_strdup(out, method);
- out->id = json_id(out, cmd->plugin, method, id_prefix ? id_prefix : cmd->id);
+ out->idstr = json_id(out, cmd->plugin, method, id_prefix ? id_prefix : cmd->idstr);
out->cmd = cmd;
out->cb = cb;
out->errcb = errcb;
out->arg = arg;
- strmap_add(&cmd->plugin->out_reqs, out->id, out);
+ strmap_add(&cmd->plugin->out_reqs, out->idstr, out);
tal_add_destructor2(out, destroy_out_req, cmd->plugin);
out->js = new_json_stream(NULL, cmd, NULL);
json_object_start(out->js, NULL);
json_add_string(out->js, "jsonrpc", "2.0");
- json_add_id(out->js, out->id);
+ json_add_string(out->js, "id", out->idstr);
json_add_string(out->js, "method", method);
if (filter) {
/* This is raw JSON, so paste, don't escape! */
@@ -432,7 +422,7 @@ static struct json_stream *jsonrpc_stream_start(struct command *cmd)
json_object_start(js, NULL);
json_add_string(js, "jsonrpc", "2.0");
- json_add_id(js, cmd->id);
+ json_add_string(js, "id", cmd->idstr);
return js;
}
@@ -781,8 +771,7 @@ static const jsmntok_t *sync_req(const tal_t *ctx,
json_out_start(jout, NULL, '{');
json_out_addstr(jout, "jsonrpc", "2.0");
- /* Copy in id *literally* */
- memcpy(json_out_member_direct(jout, "id", strlen(id)), id, strlen(id));
+ json_out_addstr(jout, "id", id);
json_out_addstr(jout, "method", method);
if (params)
json_out_add_splice(jout, "params", params);
@@ -1051,9 +1040,16 @@ static void handle_rpc_reply(const tal_t *working_ctx,
/* FIXME: Don't simply ignore notifications! */
return;
+ if (idtok->type != JSMN_STRING) {
+ plugin_log(plugin, LOG_BROKEN, "JSON reply with non-string id '%.*s'",
+ json_tok_full_len(toks),
+ json_tok_full(buf, toks));
+ return;
+ }
+
out = strmap_getn(&plugin->out_reqs,
- json_tok_full(buf, idtok),
- json_tok_full_len(idtok));
+ buf + idtok->start,
+ idtok->end - idtok->start);
if (!out) {
/* This can actually happen, if they free req! */
plugin_log(plugin, LOG_DBG, "JSON reply with unknown id '%.*s'",
@@ -1119,7 +1115,7 @@ send_outreq(const struct out_req *req)
* result to pass to either the error or the success
* callback. */
trace_span_start("jsonrpc", req);
- trace_span_tag(req, "id", req->id);
+ trace_span_tag(req, "id", req->idstr);
trace_span_suspend_may_free(req);
ld_rpc_send(req->cmd->plugin, req->js);
@@ -1824,7 +1820,7 @@ struct plugin_timer *command_timer_(struct command *cmd,
void *cb_arg)
{
return new_timer(cmd, cmd->plugin,
- take(tal_fmt(NULL, "%s-timer", cmd->id)),
+ take(tal_fmt(NULL, "%s-timer", cmd->idstr)),
t, cb, cb_arg);
}
@@ -1907,7 +1903,7 @@ struct json_stream *plugin_notify_start(struct command *cmd, const char *method)
json_add_string(js, "method", method);
json_object_start(js, "params");
- json_add_id(js, cmd->id);
+ json_add_string(js, "id", cmd->idstr);
return js;
}
@@ -2067,7 +2063,7 @@ static void ld_command_handle(struct plugin *plugin,
const char *buffer,
const jsmntok_t *toks)
{
- const jsmntok_t *methtok, *paramstok, *filtertok;
+ const jsmntok_t *methtok, *paramstok, *filtertok, *idtok;
const char *methodname;
struct command *cmd;
const char *id;
@@ -2076,6 +2072,7 @@ static void ld_command_handle(struct plugin *plugin,
methtok = json_get_member(buffer, toks, "method");
paramstok = json_get_member(buffer, toks, "params");
filtertok = json_get_member(buffer, toks, "filter");
+ idtok = json_get_member(buffer, toks, "id");
if (!methtok || !paramstok)
plugin_err(plugin, "Malformed JSON-RPC notification missing "
@@ -2084,14 +2081,21 @@ static void ld_command_handle(struct plugin *plugin,
json_tok_full(buffer, toks));
methodname = json_strdup(NULL, buffer, methtok);
- id = json_get_id(tmpctx, buffer, toks);
- if (!id)
+ if (!idtok) {
type = COMMAND_TYPE_NOTIFICATION;
- else if (streq(methodname, "check"))
- type = COMMAND_TYPE_CHECK;
- else
- type = COMMAND_TYPE_NORMAL;
+ id = NULL;
+ } else {
+ if (idtok->type != JSMN_STRING)
+ plugin_err(plugin, "Malformed JSON-RPC id is not a string: %.*s",
+ json_tok_full_len(toks),
+ json_tok_full(buffer, toks));
+ id = json_strdup(tmpctx, buffer, idtok);
+ if (streq(methodname, "check"))
+ type = COMMAND_TYPE_CHECK;
+ else
+ type = COMMAND_TYPE_NORMAL;
+ }
cmd = new_command(plugin, plugin,
id ? id : tal_fmt(tmpctx, "notification-%s", methodname),
@@ -2662,7 +2666,7 @@ command_hook_success(struct command *cmd)
struct command *aux_command(const struct command *cmd)
{
- return new_command(cmd->plugin, cmd->plugin, cmd->id,
+ return new_command(cmd->plugin, cmd->plugin, cmd->idstr,
cmd->methodname, COMMAND_TYPE_AUX);
}
diff --git a/plugins/libplugin.h b/plugins/libplugin.h
index 9134d2a2..79bb423d 100644
--- a/plugins/libplugin.h
+++ b/plugins/libplugin.h
@@ -21,7 +21,7 @@ enum plugin_restartability {
struct out_req {
/* The unique id of this request. */
- const char *id;
+ const char *idstr;
/* The command which is why we're calling this rpc. */
struct command *cmd;
/* The method this is calling */
@@ -60,7 +60,7 @@ enum command_type {
};
struct command {
- const char *id;
+ const char *idstr;
const char *methodname;
enum command_type type;
struct plugin *plugin;
diff --git a/tests/test_askrene.py b/tests/test_askrene.py
index 4474ec32..1aaa4f73 100644
--- a/tests/test_askrene.py
+++ b/tests/test_askrene.py
@@ -81,7 +81,7 @@ def test_reserve(node_factory):
time.sleep(2)
# Reservations can be in either order.
- with pytest.raises(RpcError, match=rf'We could not find a usable set of paths. The shortest path is {scid12}->{scid23}, but {scid12dir} already reserved 10000000*msat by command ".*" \([0-9]* seconds ago\), 10000000*msat by command ".*" \([0-9]* seconds ago\)'):
+ with pytest.raises(RpcError, match=rf'We could not find a usable set of paths. The shortest path is {scid12}->{scid23}, but {scid12dir} already reserved 10000000*msat by command [-/#:a-zA-Z0-9]* \([0-9]* seconds ago\), 10000000*msat by command [-/#:a-zA-Z0-9]* \([0-9]* seconds ago\)'):
l1.rpc.getroutes(source=l1.info['id'],
destination=l3.info['id'],
amount_msat=1000000,
diff --git a/tests/test_misc.py b/tests/test_misc.py
index 24704c4a..a87f956c 100644
--- a/tests/test_misc.py
+++ b/tests/test_misc.py
@@ -1244,7 +1244,8 @@ def test_cli_commando(node_factory):
'--network={}'.format(TEST_NETWORK),
'--lightning-dir={}'
.format(l1.daemon.lightning_dir),
- 'help'])
+ 'help'],
+ timeout=TIMEOUT)
assert val.returncode == 3
# Valid peer id, but needs rune!
@@ -1253,7 +1254,8 @@ def test_cli_commando(node_factory):
'--network={}'.format(TEST_NETWORK),
'--lightning-dir={}'
.format(l1.daemon.lightning_dir),
- 'help'])
+ 'help'],
+ timeout=TIMEOUT)
assert val.returncode == 1
# This works!
@@ -1262,7 +1264,8 @@ def test_cli_commando(node_factory):
'--network={}'.format(TEST_NETWORK),
'--lightning-dir={}'
.format(l1.daemon.lightning_dir),
- 'help']).decode('utf-8')
+ 'help'],
+ timeout=TIMEOUT).decode('utf-8')
# Test some known output.
assert 'addgossip message\n\naddoutpointwatch' in out
@@ -1279,7 +1282,8 @@ def test_cli_commando(node_factory):
'--lightning-dir={}'
.format(l1.daemon.lightning_dir),
'-J', '-k',
- 'help', 'command=help']).decode('utf-8')
+ 'help', 'command=help'],
+ timeout=TIMEOUT).decode('utf-8')
j, _ = json.JSONDecoder().raw_decode(out)
assert 'help [command]' in j['help'][0]['command']
@@ -1290,7 +1294,8 @@ def test_cli_commando(node_factory):
'--lightning-dir={}'
.format(l1.daemon.lightning_dir),
'-J', '-o',
- 'help', 'help']).decode('utf-8')
+ 'help', 'help'],
+ timeout=TIMEOUT).decode('utf-8')
j, _ = json.JSONDecoder().raw_decode(out)
assert 'help [command]' in j['help'][0]['command']
@@ -1301,7 +1306,8 @@ def test_cli_commando(node_factory):
'--lightning-dir={}'
.format(l1.daemon.lightning_dir),
'-J', '--filter={"help":[{"command":true}]}',
- 'help', 'help']).decode('utf-8')
+ 'help', 'help'],
+ timeout=TIMEOUT).decode('utf-8')
j, _ = json.JSONDecoder().raw_decode(out)
assert j == {'help': [{'command': 'help [command]'}]}
@@ -1315,7 +1321,8 @@ def test_cli_commando(node_factory):
'--lightning-dir={}'
.format(l1.daemon.lightning_dir),
'-J', '-o',
- 'sendpay']).decode('utf-8')
+ 'sendpay'],
+ timeout=TIMEOUT).decode('utf-8')
except Exception:
pass
@@ -1327,7 +1334,8 @@ def test_cli_commando(node_factory):
'--lightning-dir={}'
.format(l1.daemon.lightning_dir),
'x"[]{}'],
- stdout=subprocess.PIPE)
+ stdout=subprocess.PIPE,
+ timeout=TIMEOUT)
assert 'Unknown command' in out.stdout.decode('utf-8')
subprocess.check_output(['cli/lightning-cli',
@@ -1335,7 +1343,8 @@ def test_cli_commando(node_factory):
'--network={}'.format(TEST_NETWORK),
'--lightning-dir={}'
.format(l1.daemon.lightning_dir),
- 'invoice', '123000', 'l"[]{}', 'd"[]{}']).decode('utf-8')
+ 'invoice', '123000', 'l"[]{}', 'd"[]{}'],
+ timeout=TIMEOUT).decode('utf-8')
# Check label is correct, and also that cli's keyword parsing works.
out = subprocess.check_output(['cli/lightning-cli',
'--commando={}:{}'.format(l2.info['id'], rune),
@@ -1343,7 +1352,8 @@ def test_cli_commando(node_factory):
'--lightning-dir={}'
.format(l1.daemon.lightning_dir),
'-k',
- 'listinvoices', 'label=l"[]{}']).decode('utf-8')
+ 'listinvoices', 'label=l"[]{}'],
+ timeout=TIMEOUT).decode('utf-8')
j = json.loads(out)
assert only_one(j['invoices'])['label'] == 'l"[]{}'
Risk score
Our methodology →Why this scored 27/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.