From 0313961d45fa155bce461f9de921c76c4457fa72 Mon Sep 17 00:00:00 2001
From: daywalker90 <daywalker990@gmx.de>
Date: Fri, 24 Jul 2026 13:32:25 +0200
Subject: [PATCH] cln-plugin: return rpc error instead of exiting on invalid
 json input

CLN is too permissive for serde_json when validating json:

`lightning-cli -k myplugin-cmd channels='[123456x1x0]'`

is valid for lightning-cli but is actually invalid json (bare token in array).

The cln-plugin decoder would error and end the FramedRead stream, causing the
PluginDriver loop to exit, and therefore exiting the plugin itself.

We need to recover the id from the invalid json with a separate parser to return a
json rpc error to CLN with the correct id so the rpc command does not hang.

Changelog-None
---
 plugins/src/codec.rs    | 378 +++++++++++++++++++++++++++++++++++++++-
 plugins/src/lib.rs      |   4 +
 plugins/src/messages.rs |   1 +
 tests/test_cln_rs.py    |  30 ++++
 4 files changed, 405 insertions(+), 8 deletions(-)

diff --git a/plugins/src/codec.rs b/plugins/src/codec.rs
index 75068e7e5f75..78983177ea3d 100644
--- a/plugins/src/codec.rs
+++ b/plugins/src/codec.rs
@@ -3,8 +3,7 @@
 /// exchange JSON formatted messages. Each message is separated by an
 /// empty line and we're guaranteed that no other empty line is
 /// present in the messages.
-use crate::Error;
-use anyhow::anyhow;
+use crate::{Error, RpcError};
 use bytes::{BufMut, BytesMut};
 use serde_json::value::Value;
 use std::str::FromStr;
@@ -85,7 +84,7 @@ where
 }
 
 impl Decoder for JsonCodec {
-    type Item = Value;
+    type Item = Result<Value, Value>;
     type Error = Error;
 
     fn decode(&mut self, buf: &mut BytesMut) -> Result<Option<Self::Item>, Error> {
@@ -94,9 +93,19 @@ impl Decoder for JsonCodec {
             Err(e) => Err(e),
             Ok(Some(s)) => {
                 if let Ok(v) = Value::from_str(&s) {
-                    Ok(Some(v))
+                    Ok(Some(Ok(v)))
                 } else {
-                    Err(anyhow!("failed to parse JSON"))
+                    let id = recover_id(&s).unwrap();
+
+                    Ok(Some(Err(json!({
+                        "jsonrpc": "2.0",
+                        "id": id,
+                        "error": RpcError {
+                            code: Some(-32700),
+                            message: format!("failed to parse as JSON: `{s}`"),
+                            data: None,
+                    }
+                    }))))
                 }
             }
         }
@@ -119,16 +128,159 @@ impl Decoder for JsonRpcCodec {
         match self.inner.decode(buf) {
             Ok(None) => Ok(None),
             Err(e) => Err(e),
-            Ok(Some(s)) => {
-                let req: Self::Item = serde_json::from_value(s)?;
+            Ok(Some(Err(rpc_err))) => Ok(Some(JsonRpc::Error(rpc_err))),
+            Ok(Some(Ok(v))) => {
+                let req: Self::Item = serde_json::from_value(v)?;
                 Ok(Some(req))
             }
         }
     }
 }
 
+fn recover_id(input: &str) -> Option<serde_json::Value> {
+    let bytes = input.as_bytes();
+    let len = bytes.len();
+    let mut i = 0;
+    let mut depth: i32 = 0;
+
+    while i < len {
+        let c = bytes[i] as char;
+
+        match c {
+            '"' => {
+                let content_start = i + 1;
+                let mut j = content_start;
+                let mut escaped = false;
+                let end = loop {
+                    if j >= len {
+                        return None;
+                    }
+                    let cj = bytes[j] as char;
+                    if escaped {
+                        escaped = false;
+                    } else if cj == '\\' {
+                        escaped = true;
+                    } else if cj == '"' {
+                        break j;
+                    }
+                    j += 1;
+                };
+
+                let key = &input[content_start..end];
+                if depth == 1 && key == "id" {
+                    let mut k = end + 1;
+                    k += skip_ws(&bytes[k..]);
+                    if k < len && bytes[k] as char == ':' {
+                        k += 1;
+                        k += skip_ws(&bytes[k..]);
+                        if let Some((value, consumed)) = parse_value_prefix(&input[k..]) {
+                            let after = k + consumed;
+                            let ws = skip_ws(&bytes[after..]);
+                            let next = bytes.get(after + ws).map(|b| *b as char);
+                            // A well-formed object only ever has ',' or
+                            // '}' right after a field's value. Anything
+                            // else means we accidentally consumed part
+                            // of the next key/token, and the value we
+                            // extracted can't be trusted.
+                            if matches!(next, None | Some(',' | '}')) {
+                                return Some(value);
+                            }
+                        }
+                        return None;
+                    }
+                }
+                i = end + 1;
+            }
+            '{' | '[' => {
+                depth += 1;
+                i += 1;
+            }
+            '}' | ']' => {
+                depth -= 1;
+                i += 1;
+            }
+            _ => {
+                i += 1;
+            }
+        }
+    }
+    None
+}
+
+fn skip_ws(bytes: &[u8]) -> usize {
+    bytes
+        .iter()
+        .take_while(|b| (**b as char).is_ascii_whitespace())
+        .count()
+}
+
+/// Parse a single JSON value from the start of `input`. Returns the value
+/// plus the number of bytes consumed, so the caller can validate what
+/// immediately follows it.
+fn parse_value_prefix(input: &str) -> Option<(serde_json::Value, usize)> {
+    let bytes = input.as_bytes();
+
+    for (lit, val) in [
+        ("null", serde_json::Value::Null),
+        ("true", serde_json::Value::Bool(true)),
+        ("false", serde_json::Value::Bool(false)),
+    ] {
+        if input.starts_with(lit) {
+            // Guard against "nullish" / "trueish" / "falseX" being
+            // mistaken for the real keyword.
+            let boundary_ok = match bytes.get(lit.len()) {
+                None => true,
+                Some(b) => {
+                    let c = *b as char;
+                    !(c.is_ascii_alphanumeric() || c == '_')
+                }
+            };
+            return boundary_ok.then_some((val, lit.len()));
+        }
+    }
+
+    if let Some(rest) = input.strip_prefix('"') {
+        let rbytes = rest.as_bytes();
+        let mut escaped = false;
+        for (idx, b) in rbytes.iter().enumerate() {
+            let c = *b as char;
+            if escaped {
+                escaped = false;
+                continue;
+            }
+            if c == '\\' {
+                escaped = true;
+                continue;
+            }
+            if c == '"' {
+                let literal = format!("\"{}\"", &rest[..idx]);
+                return serde_json::from_str::<serde_json::Value>(&literal)
+                    .ok()
+                    .map(|v| (v, idx + 2)); // opening quote + content + closing quote
+            }
+        }
+        return None; // unterminated
+    }
+
+    let end = input
+        .char_indices()
+        .take_while(|(_, c)| c.is_ascii_digit() || matches!(c, '-' | '+' | '.' | 'e' | 'E'))
+        .map(|(idx, c)| idx + c.len_utf8())
+        .last()
+        .unwrap_or(0);
+
+    if end == 0 {
+        return None;
+    }
+    serde_json::from_str::<serde_json::Value>(&input[..end])
+        .ok()
+        .map(|v| (v, end))
+}
+
 #[cfg(test)]
 mod test {
+    use crate::codec::recover_id;
+
     use super::{JsonCodec, MultiLineCodec};
     use bytes::{BufMut, BytesMut};
     use serde_json::json;
@@ -188,8 +340,218 @@ mod test {
             let mut codec = JsonCodec::default();
             let mut buf = BytesMut::new();
             codec.encode(t.clone(), &mut buf).unwrap();
-            let decoded = codec.decode(&mut buf).unwrap().unwrap();
+            let decoded = codec.decode(&mut buf).unwrap().unwrap().unwrap();
             assert_eq!(&decoded, t);
         }
     }
+
+    #[test]
+    fn string_id_basic() {
+        let msg = r#"{"id":"abc123","method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!("abc123")));
+    }
+
+    #[test]
+    fn string_id_with_colon_and_hash() {
+        let msg = r#"{"id":"cli:testmethod#2078394/cln:testmethod#65","method":"foo"}"#;
+        assert_eq!(
+            recover_id(msg),
+            Some(json!("cli:testmethod#2078394/cln:testmethod#65"))
+        );
+    }
+
+    #[test]
+    fn string_id_empty() {
+        let msg = r#"{"id":"","method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!("")));
+    }
+
+    #[test]
+    fn string_id_with_escaped_quote() {
+        let msg = r#"{"id":"say \"hi\"","method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!("say \"hi\"")));
+    }
+
+    #[test]
+    fn string_id_with_escaped_backslash() {
+        let msg = r#"{"id":"back\\slash","method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!("back\\slash")));
+    }
+
+    #[test]
+    fn string_id_with_newline_escape() {
+        let msg = r#"{"id":"line1\nline2","method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!("line1\nline2")));
+    }
+
+    #[test]
+    fn string_id_with_unicode_escape() {
+        let msg = r#"{"id":"caf\u00e9","method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!("café")));
+    }
+
+    #[test]
+    fn string_id_with_literal_unicode() {
+        let msg = r#"{"id":"héllo-世界","method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!("héllo-世界")));
+    }
+
+    #[test]
+    fn string_id_with_embedded_braces_and_brackets() {
+        let msg = r#"{"id":"{not real} [json] here","method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!("{not real} [json] here")));
+    }
+
+    #[test]
+    fn string_id_with_embedded_comma_and_colon() {
+        let msg = r#"{"id":"a, b: c","method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!("a, b: c")));
+    }
+
+    #[test]
+    fn string_id_whitespace_before_colon() {
+        let msg = r#"{"id"   :   "abc","method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!("abc")));
+    }
+
+    #[test]
+    fn string_id_no_whitespace() {
+        let msg = r#"{"id":"abc","method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!("abc")));
+    }
+
+    #[test]
+    fn string_id_tabs_and_newlines_around_colon() {
+        let msg = "{\"id\"\t:\n\"abc\",\"method\":\"foo\"}";
+        assert_eq!(recover_id(msg), Some(json!("abc")));
+    }
+
+    #[test]
+    fn string_id_unterminated_is_unrecoverable() {
+        let msg = r#"{"id":"abc,"method":"foo"}"#;
+        let result = recover_id(msg);
+        assert!(result.is_none());
+    }
+
+    #[test]
+    fn string_id_containing_the_word_id() {
+        // "id" appearing inside a string ID value itself.
+        let msg = r#"{"id":"this-is-my-id-value","method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!("this-is-my-id-value")));
+    }
+
+    #[test]
+    fn string_id_after_malformed_params() {
+        let msg = r#"{"jsonrpc":"2.0","method":"testmethod","params":{"channels":[123x22x12]},"id":"cli:testmethod#99"}"#;
+        assert_eq!(recover_id(msg), Some(json!("cli:testmethod#99")));
+    }
+
+    #[test]
+    fn string_id_before_malformed_params() {
+        let msg = r#"{"jsonrpc":"2.0","id":"cli:testmethod#99","method":"testmethod","params":{"channels":[123x22x12]}}"#;
+        assert_eq!(recover_id(msg), Some(json!("cli:testmethod#99")));
+    }
+
+    #[test]
+    fn number_id_positive_int() {
+        let msg = r#"{"id":42,"method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!(42)));
+    }
+
+    #[test]
+    fn number_id_zero() {
+        let msg = r#"{"id":0,"method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!(0)));
+    }
+
+    #[test]
+    fn number_id_negative_int() {
+        let msg = r#"{"id":-17,"method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!(-17)));
+    }
+
+    #[test]
+    fn number_id_float() {
+        let msg = r#"{"id":3.15,"method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!(3.15)));
+    }
+
+    #[test]
+    fn number_id_negative_float() {
+        let msg = r#"{"id":-0.5,"method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!(-0.5)));
+    }
+
+    #[test]
+    fn number_id_with_exponent() {
+        let msg = r#"{"id":1e10,"method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!(1e10)));
+    }
+
+    #[test]
+    fn number_id_with_signed_exponent() {
+        let msg = r#"{"id":2.5e-3,"method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!(2.5e-3)));
+    }
+
+    #[test]
+    fn number_id_large_integer() {
+        let msg = r#"{"id":9007199254740993,"method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!(9_007_199_254_740_993_i64)));
+    }
+
+    #[test]
+    fn number_id_whitespace_before_colon() {
+        let msg = r#"{"id"  :  42,"method":"foo"}"#;
+        assert_eq!(recover_id(msg), Some(json!(42)));
+    }
+
+    #[test]
+    fn number_id_followed_directly_by_brace() {
+        let msg = r#"{"id":7}"#;
+        assert_eq!(recover_id(msg), Some(json!(7)));
+    }
+
+    #[test]
+    fn number_id_followed_directly_by_bracket_in_params() {
+        let msg = r#"{"method":"foo","id":7,"params":[bad,tokens,here]}"#;
+        assert_eq!(recover_id(msg), Some(json!(7)));
+    }
+
+    #[test]
+    fn number_id_invalid_leading_plus_is_unrecoverable() {
+        let msg = r#"{"id":+42,"method":"foo"}"#;
+        assert_eq!(recover_id(msg), None);
+    }
+
+    #[test]
+    fn number_id_bare_minus_is_unrecoverable() {
+        let msg = r#"{"id":-,"method":"foo"}"#;
+        assert_eq!(recover_id(msg), None);
+    }
+
+    #[test]
+    fn number_id_trailing_dot_no_digits_is_unrecoverable() {
+        let msg = r#"{"id":1.,"method":"foo"}"#;
+        assert_eq!(recover_id(msg), None);
+    }
+
+    #[test]
+    fn number_id_double_leading_zero_still_parses_prefix() {
+        let msg = r#"{"id":007,"method":"foo"}"#;
+        assert_eq!(recover_id(msg), None);
+    }
+
+    #[test]
+    fn number_id_mixed_with_bad_downstream_object() {
+        let msg =
+            r#"{"jsonrpc":"2.0","id":123,"method":"testmethod","params":{"channels":[123x22x12]}}"#;
+        assert_eq!(recover_id(msg), Some(json!(123)));
+    }
+
+    #[test]
+    fn number_id_adjacent_to_nested_id_key_ignored() {
+        let msg = r#"{"method":"foo","params":{"id":"decoy"},"id":555}"#;
+        assert_eq!(recover_id(msg), Some(json!(555)));
+    }
 }
diff --git a/plugins/src/lib.rs b/plugins/src/lib.rs
index 101c2d51db28..4ccdbf67cd1c 100644
--- a/plugins/src/lib.rs
+++ b/plugins/src/lib.rs
@@ -1030,6 +1030,10 @@ where
                         };
                         Ok(())
                     }
+                    messages::JsonRpc::Error(rpc_error) => {
+                        plugin.sender.send(rpc_error).await?;
+                        Ok(())
+                    }
                 }
             }
             Some(Err(e)) => Err(anyhow!("Error reading command: {}", e)),
diff --git a/plugins/src/messages.rs b/plugins/src/messages.rs
index 30d4d22eff1f..459fa5997499 100644
--- a/plugins/src/messages.rs
+++ b/plugins/src/messages.rs
@@ -99,6 +99,7 @@ pub(crate) enum JsonRpc<N, R> {
     Notification(N),
     CustomRequest(serde_json::Value, Value),
     CustomNotification(Value),
+    Error(serde_json::Value),
 }
 
 /// This function disentangles the various cases:
diff --git a/tests/test_cln_rs.py b/tests/test_cln_rs.py
index 37af56cfdba6..b4edbb681b10 100644
--- a/tests/test_cln_rs.py
+++ b/tests/test_cln_rs.py
@@ -82,6 +82,36 @@ def test_plugin_start(node_factory):
         l1.rpc.test_error()
 
 
+def test_plugin_bad_json(node_factory):
+    bin_path = Path.cwd() / "target" / RUST_PROFILE / "examples" / "cln-plugin-startup"
+    l1 = node_factory.get_node(options={"plugin": str(bin_path)})
+
+    names = [p["name"] for p in l1.rpc.plugin_list()["plugins"]]
+    assert any("cln-plugin-startup" in n for n in names)
+
+    cli_cmd = [
+        "cli/lightning-cli",
+        f"--lightning-dir={l1.daemon.lightning_dir}",
+        f"--network={l1.daemon.opts.get('network', 'regtest')}",
+        "-k",
+        "testmethod",
+        "channels=[123456x1x0]",  # unquoted scid in array -> malformed JSON
+    ]
+    result = subprocess.run(cli_cmd, capture_output=True, text=True, check=False)
+
+    # We don't care whether this particular call succeeds -- a parse
+    # error for the bad request is fine. We care that it doesn't take
+    # the plugin down.
+    assert "Plugin terminated before replying" not in result.stdout + result.stderr
+
+    # Plugin must still be alive and answering ordinary requests.
+    names_after = [p["name"] for p in l1.rpc.plugin_list()["plugins"]]
+    assert any("cln-plugin-startup" in n for n in names_after)
+    assert l1.rpc.call("testmethod", {"channels": []}) is not None
+
+    assert not l1.daemon.is_in_log(r"Killing plugin.*exited during normal operation")
+
+
 def test_plugin_options_handle_defaults(node_factory):
     """Start a minimal plugin and ensure it is well-behaved
     """