From 4e412eef262fb25a8f24be05dd0e93bc781cf705 Mon Sep 17 00:00:00 2001 From: Michael Schmoock Date: Sat, 30 Jan 2021 14:35:52 +0100 Subject: [PATCH 1/4] plugins: make rpc_command hook chainable Changelog-Changed: The `rpc_command` hook is now chainable. --- lightningd/jsonrpc.c | 136 ++++++++++++++++++++++++++++++------------- 1 file changed, 95 insertions(+), 41 deletions(-) diff --git a/lightningd/jsonrpc.c b/lightningd/jsonrpc.c index 585fa401456a..1d42e0f22246 100644 --- a/lightningd/jsonrpc.c +++ b/lightningd/jsonrpc.c @@ -665,6 +665,12 @@ struct rpc_command_hook_payload { struct command *cmd; const char *buffer; const jsmntok_t *request; + + /* custom response/replace/error options plugins can have */ + const char *custom_result; + const char *custom_error; + const jsmntok_t *custom_replace; + const char *custom_buffer; }; static void rpc_command_hook_serialize(struct rpc_command_hook_payload *p, @@ -734,50 +740,89 @@ static void replace_command(struct rpc_command_hook_payload *p, "Bad response to 'rpc_command' hook: %s", bad)); } -static void -rpc_command_hook_callback(struct rpc_command_hook_payload *p STEALS, - const char *buffer, const jsmntok_t *resulttok) +static void rpc_command_hook_final(struct rpc_command_hook_payload *p STEALS) { - const jsmntok_t *tok, *params, *custom_return; - const jsmntok_t *innerresulttok; - struct json_stream *response; + const jsmntok_t *params; /* Free payload with cmd */ tal_steal(p->cmd, p); + if (p->custom_result != NULL) { + struct json_stream *s = json_start(p->cmd); + json_add_jsonstr(s, "result", p->custom_result); + json_object_compat_end(s); + return was_pending(command_raw_complete(p->cmd, s)); + } + if (p->custom_error != NULL) { + struct json_stream *s = json_start(p->cmd); + json_add_jsonstr(s, "error", p->custom_error); + json_object_compat_end(s); + return was_pending(command_raw_complete(p->cmd, s)); + } + if (p->custom_replace != NULL) + return replace_command(p, p->custom_buffer, p->custom_replace); + + /* If no plugin requested a change, just continue command execution. */ params = json_get_member(p->buffer, p->request, "params"); + return was_pending(command_exec(p->cmd->jcon, + p->cmd, + p->buffer, + p->request, + params)); +} + +static bool +rpc_command_hook_callback(struct rpc_command_hook_payload *p, + const char *buffer, const jsmntok_t *resulttok) +{ + const struct lightningd *ld = p->cmd->ld; + const jsmntok_t *tok, *custom_return; + static char *error = ""; + char *method; + + if (!resulttok || !buffer) + return true; - /* If no plugin registered, just continue command execution. Same if - * the registered plugin tells us to do so. */ - if (buffer == NULL) - return was_pending(command_exec(p->cmd->jcon, p->cmd, p->buffer, - p->request, params)); - - innerresulttok = json_get_member(buffer, resulttok, "result"); - if (innerresulttok) { - if (json_tok_streq(buffer, innerresulttok, "continue")) { - return was_pending(command_exec(p->cmd->jcon, p->cmd, p->buffer, - p->request, params)); + tok = json_get_member(buffer, resulttok, "result"); + if (tok) { + if (!json_tok_streq(buffer, tok, "continue")) { + error = "'result' should only be 'continue'."; + goto log_error_and_skip; } - return was_pending(command_fail(p->cmd, JSONRPC2_INVALID_REQUEST, - "Bad 'result' to 'rpc_command' hook.")); + /* plugin tells us to do nothing. just pass. */ + return true; + } + + /* didn't just continue but hook was already modified by prior plugin */ + if (p->custom_result != NULL || + p->custom_error != NULL || + p->custom_replace != NULL) { + /* get method name and log error (only the first time). */ + tok = json_get_member(p->buffer, p->request, "method"); + method = tal_strndup(p, p->buffer + tok->start, tok->end - tok->start); + log_unusual(ld->log, "rpc_command hook '%s' already modified, ignoring.", method ); + rpc_command_hook_final(p); + return false; } /* If the registered plugin did not respond with continue, * it wants either to replace the request... */ tok = json_get_member(buffer, resulttok, "replace"); - if (tok) - return replace_command(p, buffer, tok); + if (tok) { + /* We need to make copies here, as buffer and tokens + * can be reused. */ + p->custom_replace = json_tok_copy(p, tok); + p->custom_buffer = tal_dup_talarr(p, char, buffer); + return true; + } /* ...or return a custom JSONRPC response. */ tok = json_get_member(buffer, resulttok, "return"); if (tok) { custom_return = json_get_member(buffer, tok, "result"); if (custom_return) { - response = json_start(p->cmd); - json_add_tok(response, "result", custom_return, buffer); - json_object_compat_end(response); - return was_pending(command_raw_complete(p->cmd, response)); + p->custom_result = json_strdup(p, buffer, custom_return); + return true; } custom_return = json_get_member(buffer, tok, "error"); @@ -786,29 +831,32 @@ rpc_command_hook_callback(struct rpc_command_hook_payload *p STEALS, const char *errmsg; if (!json_to_errcode(buffer, json_get_member(buffer, custom_return, "code"), - &code)) - return was_pending(command_fail(p->cmd, JSONRPC2_INVALID_REQUEST, - "Bad response to 'rpc_command' hook: " - "'error' object does not contain a code.")); + &code)) { + error = "'error' object does not contain a code."; + goto log_error_and_skip; + } errmsg = json_strdup(tmpctx, buffer, json_get_member(buffer, custom_return, "message")); - if (!errmsg) - return was_pending(command_fail(p->cmd, JSONRPC2_INVALID_REQUEST, - "Bad response to 'rpc_command' hook: " - "'error' object does not contain a message.")); - response = json_stream_fail_nodata(p->cmd, code, errmsg); - return was_pending(command_failed(p->cmd, response)); + if (!errmsg) { + error = "'error' object does not contain a message."; + goto log_error_and_skip; + } + p->custom_error = json_strdup(p, buffer, custom_return); + return true; } } - was_pending(command_fail(p->cmd, JSONRPC2_INVALID_REQUEST, - "Bad response to 'rpc_command' hook.")); +log_error_and_skip: + /* Just log BROKEN errors. Give other plugins a chance. */ + log_broken(ld->log, "Bad response to 'rpc_command' hook. %s", error); + return true; } -REGISTER_SINGLE_PLUGIN_HOOK(rpc_command, - rpc_command_hook_callback, - rpc_command_hook_serialize, - struct rpc_command_hook_payload *); +REGISTER_PLUGIN_HOOK(rpc_command, + rpc_command_hook_callback, + rpc_command_hook_final, + rpc_command_hook_serialize, + struct rpc_command_hook_payload *); /* We return struct command_result so command_fail return value has a natural * sink; we don't actually use the result. */ @@ -884,6 +932,12 @@ parse_request(struct json_connection *jcon, const jsmntok_t tok[]) rpc_hook->buffer = tal_dup_talarr(rpc_hook, char, jcon->buffer); rpc_hook->request = tal_dup_talarr(rpc_hook, jsmntok_t, tok); + /* NULL the custom_ values for the hooks */ + rpc_hook->custom_result = NULL; + rpc_hook->custom_error = NULL; + rpc_hook->custom_replace = NULL; + rpc_hook->custom_buffer = NULL; + db_begin_transaction(jcon->ld->wallet->db); completed = plugin_hook_call_rpc_command(jcon->ld, rpc_hook); db_commit_transaction(jcon->ld->wallet->db); From 5aab3f71af06a8cf31ac9bbd7db5c79b118d54d1 Mon Sep 17 00:00:00 2001 From: Michael Schmoock Date: Sat, 30 Jan 2021 14:36:26 +0100 Subject: [PATCH 2/4] doc: make rpc_command hook chainable --- doc/PLUGINS.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/PLUGINS.md b/doc/PLUGINS.md index 644f3a254295..ac115d8c20c9 100644 --- a/doc/PLUGINS.md +++ b/doc/PLUGINS.md @@ -1198,6 +1198,9 @@ Return a custom error to the request sender: } ``` +Note: The `rpc_command` hook is chainable. If two or more plugins try to +replace/result/error the same `method`, only the first plugin in the chain +will be respected. Others will be ignored and a warning will be logged. ### `custommsg` From 95dca3391610bdfded1237fdada88f0f30cc8c31 Mon Sep 17 00:00:00 2001 From: Michael Schmoock Date: Thu, 11 Feb 2021 16:18:41 +0100 Subject: [PATCH 3/4] pytest: test rpc_command hook chain --- .../{rpc_command.py => rpc_command_1.py} | 10 ++---- tests/plugins/rpc_command_2.py | 24 +++++++++++++ tests/test_plugin.py | 35 +++++++++++++------ 3 files changed, 52 insertions(+), 17 deletions(-) rename tests/plugins/{rpc_command.py => rpc_command_1.py} (58%) create mode 100755 tests/plugins/rpc_command_2.py diff --git a/tests/plugins/rpc_command.py b/tests/plugins/rpc_command_1.py similarity index 58% rename from tests/plugins/rpc_command.py rename to tests/plugins/rpc_command_1.py index a993a8d6a045..29724dabbb78 100755 --- a/tests/plugins/rpc_command.py +++ b/tests/plugins/rpc_command_1.py @@ -1,6 +1,6 @@ #!/usr/bin/env python3 """ -This plugin is used to test the `rpc_command` hook. +This plugin is used to test the chained `rpc_command` hook. """ from pyln.client import Plugin @@ -12,15 +12,11 @@ def on_rpc_command(plugin, rpc_command, **kwargs): request = rpc_command if request["method"] == "invoice": # Replace part of this command - request["params"]["description"] = "A plugin modified this description" + request["params"]["description"] = "rpc_command_1 modified this description" return {"replace": request} elif request["method"] == "listfunds": # Return a custom result to the command - return {"return": {"result": ["Custom result"]}} - elif request["method"] == "sendpay": - # Don't allow this command to be executed - return {"return": {"error": {"code": -1, - "message": "You cannot do this"}}} + return {"return": {"result": ["Custom rpc_command_1 result"]}} elif request["method"] == "help": request["method"] = "autocleaninvoice" return {"replace": request} diff --git a/tests/plugins/rpc_command_2.py b/tests/plugins/rpc_command_2.py new file mode 100755 index 000000000000..f167c1ce5a88 --- /dev/null +++ b/tests/plugins/rpc_command_2.py @@ -0,0 +1,24 @@ +#!/usr/bin/env python3 +""" +This plugin is used to test the chained `rpc_command` hook. +""" +from pyln.client import Plugin + +plugin = Plugin() + + +@plugin.hook("rpc_command") +def on_rpc_command(plugin, rpc_command, **kwargs): + request = rpc_command + if request["method"] == "invoice": + # Replace part of this command + request["params"]["description"] = "rpc_command_2 modified this description" + return {"replace": request} + elif request["method"] == "sendpay": + # Don't allow this command to be executed + return {"return": {"error": {"code": -1, + "message": "rpc_command_2 cannot do this"}}} + return {"result": "continue"} + + +plugin.run() diff --git a/tests/test_plugin.py b/tests/test_plugin.py index baec3e09ce33..9699de37c8db 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -16,6 +16,7 @@ import json import os import pytest +import random import re import signal import sqlite3 @@ -1339,28 +1340,42 @@ def test_sendpay_notifications_nowaiter(node_factory): def test_rpc_command_hook(node_factory): - """Test the `sensitive_command` hook""" - plugin = os.path.join(os.getcwd(), "tests/plugins/rpc_command.py") + """Test the `rpc_command` hook chain""" + plugin = [ + os.path.join(os.getcwd(), "tests/plugins/rpc_command_1.py"), + os.path.join(os.getcwd(), "tests/plugins/rpc_command_2.py") + ] l1 = node_factory.get_node(options={"plugin": plugin}) - # Usage of "sendpay" has been restricted by the plugin - with pytest.raises(RpcError, match=r"You cannot do this"): + # rpc_command_2 plugin restricts using "sendpay" + with pytest.raises(RpcError, match=r"rpc_command_2 cannot do this"): l1.rpc.call("sendpay") - # The plugin replaces a call made for the "invoice" command + # Both plugins will replace calls made for the "invoice" command + # The first will win, for the second a warning should be logged invoice = l1.rpc.invoice(10**6, "test_side", "test_input") decoded = l1.rpc.decodepay(invoice["bolt11"]) - assert decoded["description"] == "A plugin modified this description" + assert decoded["description"] == "rpc_command_1 modified this description" + l1.daemon.wait_for_log("rpc_command hook 'invoice' already modified, ignoring.") - # The plugin sends a custom response to "listfunds" + # rpc_command_1 plugin sends a custom response to "listfunds" funds = l1.rpc.listfunds() - assert funds[0] == "Custom result" + assert funds[0] == "Custom rpc_command_1 result" # Test command redirection to a plugin l1.rpc.call('help', [0]) - # Test command which removes plugin itself! - l1.rpc.plugin_stop('rpc_command.py') + # Check the 'already modified' warning is not logged on just 'continue' + assert not l1.daemon.is_in_log("rpc_command hook 'listfunds' already modified, ignoring.") + + # Tests removing a chained hook in random order. + # Note: This will get flaky by design if theres a problem. + if bool(random.getrandbits(1)): + l1.rpc.plugin_stop('rpc_command_2.py') + l1.rpc.plugin_stop('rpc_command_1.py') + else: + l1.rpc.plugin_stop('rpc_command_1.py') + l1.rpc.plugin_stop('rpc_command_2.py') def test_libplugin(node_factory): From fec9aa1bfea742cd94f41edc3b52a1f4c10d8703 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 26 Feb 2021 10:11:28 +0100 Subject: [PATCH 4/4] json: fix oom when adding a long string --- common/json_stream.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/common/json_stream.c b/common/json_stream.c index 45ac30ebd330..20af11a29aec 100644 --- a/common/json_stream.c +++ b/common/json_stream.c @@ -194,8 +194,9 @@ void json_add_jsonstr(struct json_stream *js, size_t len = strlen(jsonstr); p = json_member_direct(js, fieldname, len); - - memcpy(p, jsonstr, len); + /* Could be OOM! */ + if (p) + memcpy(p, jsonstr, len); } /* This is where we read the json_stream and write it to conn */