commando: make rune alternatives a JSON array.

This avoids having to escape | or &, though we still allow that for the deprecation period. To detect deprecated usage, we insist that alternatives are *always* an array (which could be loosened later), but that also means that restrictions must *always* be an array for now. Before: ``` # invoice, description either A or B lightning-cli commando-rune '["method=invoice","pnamedescription=A|pnamedescription=B"]' # invoice, description literally 'A|B' lightning-cli commando-rune '["method=invoice","pnamedescription=A\\|B"]' ``` After: ``` # invoice, description either A or B lightning-cli commando-rune '[["method=invoice"],["pnamedescription=A", "pnamedescription=B"]]' # invoice, description literally 'A|B' lightning-cli commando-rune '[["method=invoice"],["pnamedescription=A|B"]]' ``` Changelog-Deprecated: JSON-RPC: `commando-rune` restrictions is always an array, each element an array of alternatives. Replaces a string with `|`-separators, so no escaping necessary except for `\\`.
ElementsProject · Aug 25, 2022 · 9ecaa28 · 9ecaa28
1 parent ae53e51
commit 9ecaa28
Show file tree

Hide file tree

Showing 7 changed files with 128 additions and 51 deletions.
diff --git a/ccan/README b/ccan/README
@@ -1,3 +1,3 @@
 CCAN imported from http://ccodearchive.net.
 
-CCAN version: init-2547-g17a0d069
+CCAN version: init-2548-gab87e56b
diff --git a/ccan/ccan/rune/coding.c b/ccan/ccan/rune/coding.c
@@ -184,7 +184,7 @@ static bool pull_char(const char **data, size_t *len, char *c)
 	return true;
 }
 
-static bool is_valid_cond(enum rune_condition cond)
+bool rune_condition_is_valid(enum rune_condition cond)
 {
 	switch (cond) {
 	case RUNE_COND_IF_MISSING:
@@ -203,31 +203,35 @@ static bool is_valid_cond(enum rune_condition cond)
 	return false;
 }
 
+size_t rune_altern_fieldname_len(const char *alternstr, size_t alternstrlen)
+{
+	for (size_t i = 0; i < alternstrlen; i++) {
+		if (cispunct(alternstr[i]))
+			return i;
+	}
+	return alternstrlen;
+}
+
 /* Sets *more on success: true if another altern follows */
 static struct rune_altern *rune_altern_decode(const tal_t *ctx,
 					      const char **data, size_t *len,
 					      bool *more)
 {
 	struct rune_altern *alt = tal(ctx, struct rune_altern);
-	const char *strstart = *data;
 	char *value;
-	size_t strlen = 0;
+	size_t strlen;
 	char c;
 
-        /* Swallow field up to conditional */
-	for (;;) {
-		if (!pull_char(data, len, &c))
-			return tal_free(alt);
-		if (cispunct(c))
-			break;
-		strlen++;
-	}
+        /* Swallow field up to possible conditional */
+	strlen = rune_altern_fieldname_len(*data, *len);
+	alt->fieldname = tal_strndup(alt, *data, strlen);
+	*data += strlen;
+	*len -= strlen;
 
-	alt->fieldname = tal_strndup(alt, strstart, strlen);
-	if (!is_valid_cond(c)) {
-		pull_invalid(data, len);
+	/* Grab conditional */
+	if (!pull_char(data, len, &c) || !rune_condition_is_valid(c))
 		return tal_free(alt);
-	}
+
 	alt->condition = c;
 
 	/* Assign worst case. */

diff --git a/ccan/ccan/rune/rune.h b/ccan/ccan/rune/rune.h
@@ -376,4 +376,25 @@ char *rune_to_string(const tal_t *ctx, const struct rune *rune);
 struct rune_restr *rune_restr_from_string(const tal_t *ctx,
 					  const char *str,
 					  size_t len);
+
+/**
+ * rune_condition_is_valid: is this a valid condition?
+ * @cond: potential condition character.
+ *
+ * Returns true if it's one of enum rune_condition.
+ */
+bool rune_condition_is_valid(enum rune_condition cond);
+
+/**
+ * rune_altern_fieldname_len: how much of this string is condition?
+ * @alternstr: potential alternative string
+ * @alternstrlen: length
+ *
+ * This helps parsing your own runes.
+ *
+ * Returns the first possible condition (check with rune_condition_is_valid)
+ * or alternstrlen if none found.
+ */
+size_t rune_altern_fieldname_len(const char *alternstr, size_t alternstrlen);
+
 #endif /* CCAN_RUNE_RUNE_H */
diff --git a/doc/lightning-commando-rune.7.md b/doc/lightning-commando-rune.7.md
@@ -23,11 +23,10 @@ If *rune* is supplied, the restrictions are simple appended to that
 
 *restrictions* can be the string "readonly" (creates a rune which
 allows most *get* and *list* commands, and the *summary* command), or
-an array of restrictions, or a single resriction.
+an array of restrictions.
 
-Each restriction is a set of one or more alternatives, such as "method
-is listpeers", or "method is listpeers OR time is before 2023".
-Alternatives use a simple language to examine the command which is
+Each restriction is an array of one or more alternatives, such as "method
+is listpeers", or "method is listpeers OR time is before 2023".  Alternatives use a simple language to examine the command which is
 being run:
 
 * time: the current UNIX time, e.g. "time<1656759180".
@@ -41,10 +40,10 @@ being run:
 RESTRICTION FORMAT
 ------------------
 
-Restrictions are one or more alternatives, separated by `|`.  Each
+Restrictions are one or more alternatives.  Each
 alternative is *name* *operator* *value*.  The valid names are shown
-above.  If a value contains `|`, `&` or `\\`, it must be preceeded by
-a `\\`.
+above.  Note that if a value contains `\\`, it must be preceeded by another `\\`
+to form valid JSON:
 
 * `=`: passes if equal ie. identical. e.g. `method=withdraw`
 * `/`: not equals, e.g. `method/withdraw`
@@ -80,12 +79,12 @@ We can add restrictions to that rune, like so:
 
 The "readonly" restriction is a short-cut for two restrictions:
 
-1. `method^list|method^get|method=summary`: You may call list, get or summary.
-2. `method/listdatastore`: But not listdatastore: that contains sensitive stuff!
+1. `["method^list", "method^get", "method=summary"]`: You may call list, get or summary.
+2. `["method/listdatastore"]`: But not listdatastore: that contains sensitive stuff!
 
 We can do the same manually, like so:
 
-    $ lightning-cli commando-rune rune=KUhZzNlECC7pYsz3QVbF1TqjIUYi3oyESTI7n60hLMs9MA== restrictions='["method^list|method^get|method=summary","method/listdatastore"]'
+    $ lightning-cli commando-rune rune=KUhZzNlECC7pYsz3QVbF1TqjIUYi3oyESTI7n60hLMs9MA== restrictions='[["method^list", "method^get", "method=summary"],["method/listdatastore"]]'
     {
        "rune": "NbL7KkXcPQsVseJ9TdJNjJK2KsPjnt_q4cE_wvc873I9MCZtZXRob2RebGlzdHxtZXRob2ReZ2V0fG1ldGhvZD1zdW1tYXJ5Jm1ldGhvZC9saXN0ZGF0YXN0b3Jl",
        "unique_id": "0"
@@ -95,15 +94,15 @@ Let's create a rune which lets a specific peer
 (024b9a1fa8e006f1e3937f65f66c408e6da8e1ca728ea43222a7381df1cc449605)
 run "listpeers" on themselves:
 
-    $ lightning-cli commando-rune restrictions='["id=024b9a1fa8e006f1e3937f65f66c408e6da8e1ca728ea43222a7381df1cc449605","method=listpeers","pnum=1","pnameid=024b9a1fa8e006f1e3937f65f66c408e6da8e1ca728ea43222a7381df1cc449605|parr0=024b9a1fa8e006f1e3937f65f66c408e6da8e1ca728ea43222a7381df1cc449605"]'
+    $ lightning-cli commando-rune restrictions='[["id=024b9a1fa8e006f1e3937f65f66c408e6da8e1ca728ea43222a7381df1cc449605"],["method=listpeers"],["pnum=1"],["pnameid=024b9a1fa8e006f1e3937f65f66c408e6da8e1ca728ea43222a7381df1cc449605","parr0=024b9a1fa8e006f1e3937f65f66c408e6da8e1ca728ea43222a7381df1cc449605"]]'
     {
        "rune": "FE8GHiGVvxcFqCQcClVRRiNE_XEeLYQzyG2jmqto4jM9MiZpZD0wMjRiOWExZmE4ZTAwNmYxZTM5MzdmNjVmNjZjNDA4ZTZkYThlMWNhNzI4ZWE0MzIyMmE3MzgxZGYxY2M0NDk2MDUmbWV0aG9kPWxpc3RwZWVycyZwbnVtPTEmcG5hbWVpZD0wMjRiOWExZmE4ZTAwNmYxZTM5MzdmNjVmNjZjNDA4ZTZkYThlMWNhNzI4ZWE0MzIyMmE3MzgxZGYxY2M0NDk2MDV8cGFycjA9MDI0YjlhMWZhOGUwMDZmMWUzOTM3ZjY1ZjY2YzQwOGU2ZGE4ZTFjYTcyOGVhNDMyMjJhNzM4MWRmMWNjNDQ5NjA1",
        "unique_id": "2"
     }
 
 This allows `listpeers` with 1 argument (`pnum=1`), which is either by name (`pnameid`), or position (`parr0`).  We could shorten this in several ways: either allowing only positional or named parameters, or by testing the start of the parameters only.  Here's an example which only checks the first 9 bytes of the `listpeers` parameter:
 
-    $ lightning-cli commando-rune restrictions='["id=024b9a1fa8e006f1e3937f65f66c408e6da8e1ca728ea43222a7381df1cc449605","method=listpeers","pnum=1","pnameid^024b9a1fa8e006f1e393|parr0^024b9a1fa8e006f1e393"]'
+    $ lightning-cli commando-rune restrictions='[["id=024b9a1fa8e006f1e3937f65f66c408e6da8e1ca728ea43222a7381df1cc449605"],["method=listpeers"],["pnum=1"],["pnameid^024b9a1fa8e006f1e393", "parr0^024b9a1fa8e006f1e393"]'
      {
        "rune": "fTQnfL05coEbiBO8SS0cvQwCcPLxE9c02pZCC6HRVEY9MyZpZD0wMjRiOWExZmE4ZTAwNmYxZTM5MzdmNjVmNjZjNDA4ZTZkYThlMWNhNzI4ZWE0MzIyMmE3MzgxZGYxY2M0NDk2MDUmbWV0aG9kPWxpc3RwZWVycyZwbnVtPTEmcG5hbWVpZF4wMjRiOWExZmE4ZTAwNmYxZTM5M3xwYXJyMF4wMjRiOWExZmE4ZTAwNmYxZTM5Mw==",
        "unique_id": "3"
@@ -114,7 +113,7 @@ it only be usable for 24 hours from now (`time<`), and that it can only
 be used twice a minute (`rate=2`).  `date +%s` can give us the current
 time in seconds:
 
-    $ lightning-cli commando-rune rune=fTQnfL05coEbiBO8SS0cvQwCcPLxE9c02pZCC6HRVEY9MyZpZD0wMjRiOWExZmE4ZTAwNmYxZTM5MzdmNjVmNjZjNDA4ZTZkYThlMWNhNzI4ZWE0MzIyMmE3MzgxZGYxY2M0NDk2MDUmbWV0aG9kPWxpc3RwZWVycyZwbnVtPTEmcG5hbWVpZF4wMjRiOWExZmE4ZTAwNmYxZTM5M3xwYXJyMF4wMjRiOWExZmE4ZTAwNmYxZTM5Mw== restrictions='["time<'$(($(date +%s) + 24*60*60))'","rate=2"]'
+    $ lightning-cli commando-rune rune=fTQnfL05coEbiBO8SS0cvQwCcPLxE9c02pZCC6HRVEY9MyZpZD0wMjRiOWExZmE4ZTAwNmYxZTM5MzdmNjVmNjZjNDA4ZTZkYThlMWNhNzI4ZWE0MzIyMmE3MzgxZGYxY2M0NDk2MDUmbWV0aG9kPWxpc3RwZWVycyZwbnVtPTEmcG5hbWVpZF4wMjRiOWExZmE4ZTAwNmYxZTM5M3xwYXJyMF4wMjRiOWExZmE4ZTAwNmYxZTM5Mw== restrictions='[["time<'$(($(date +%s) + 24*60*60))'","rate=2"]]'
     {
        "rune": "tU-RLjMiDpY2U0o3W1oFowar36RFGpWloPbW9-RuZdo9MyZpZD0wMjRiOWExZmE4ZTAwNmYxZTM5MzdmNjVmNjZjNDA4ZTZkYThlMWNhNzI4ZWE0MzIyMmE3MzgxZGYxY2M0NDk2MDUmbWV0aG9kPWxpc3RwZWVycyZwbnVtPTEmcG5hbWVpZF4wMjRiOWExZmE4ZTAwNmYxZTM5M3xwYXJyMF4wMjRiOWExZmE4ZTAwNmYxZTM5MyZ0aW1lPDE2NTY5MjA1MzgmcmF0ZT0y",
        "unique_id": "3"

diff --git a/doc/schemas/commando-rune.request.json b/doc/schemas/commando-rune.request.json
@@ -14,12 +14,18 @@
           "type": "array",
           "description": "array of restrictions to add to rune",
           "items": {
-            "type": "string"
+            "type": "array",
+            "items": {
+              "type": "string"
+            }
           }
         },
         {
           "type": "string",
-          "description": "single restrictions to add to rune, or readonly."
+          "enum": [
+            "readonly"
+          ],
+          "description": "readonly string to indicate standard readonly restrictions."
         }
       ]
     }

diff --git a/plugins/commando.c b/plugins/commando.c
@@ -762,19 +762,66 @@ static struct rune_restr **readonly_restrictions(const tal_t *ctx)
 	return restrs;
 }
 
-/* \| is not valid JSON, so they use \\|: undo it! */
-static struct rune_restr *rune_restr_from_json(const tal_t *ctx,
-					       const char *buffer,
-					       const jsmntok_t *tok)
+static struct rune_altern *rune_altern_from_json(const tal_t *ctx,
+						 const char *buffer,
+						 const jsmntok_t *tok)
 {
+	struct rune_altern *alt;
+	size_t condoff;
+	/* We still need to unescape here, for \\ -> \.  JSON doesn't
+	 * allow unnecessary \ */
 	const char *unescape;
 	struct json_escape *e = json_escape_string_(tmpctx,
 						    buffer + tok->start,
 						    tok->end - tok->start);
 	unescape = json_escape_unescape(tmpctx, e);
 	if (!unescape)
 		return NULL;
-	return rune_restr_from_string(ctx, unescape, strlen(unescape));
+
+	condoff = rune_altern_fieldname_len(unescape, strlen(unescape));
+	if (!rune_condition_is_valid(unescape[condoff]))
+		return NULL;
+
+	alt = tal(ctx, struct rune_altern);
+	alt->fieldname = tal_strndup(alt, unescape, condoff);
+	alt->condition = unescape[condoff];
+	alt->value = tal_strdup(alt, unescape + condoff + 1);
+	return alt;
+}
+
+static struct rune_restr *rune_restr_from_json(const tal_t *ctx,
+					       const char *buffer,
+					       const jsmntok_t *tok)
+{
+	const jsmntok_t *t;
+	size_t i;
+	struct rune_restr *restr;
+
+	/* \| is not valid JSON, so they use \\|: undo it! */
+	if (deprecated_apis && tok->type == JSMN_STRING) {
+		const char *unescape;
+		struct json_escape *e = json_escape_string_(tmpctx,
+							    buffer + tok->start,
+							    tok->end - tok->start);
+		unescape = json_escape_unescape(tmpctx, e);
+		if (!unescape)
+			return NULL;
+		return rune_restr_from_string(ctx, unescape, strlen(unescape));
+	}
+
+	restr = tal(ctx, struct rune_restr);
+	/* FIXME: after deprecation removed, allow singletons again! */
+	if (tok->type != JSMN_ARRAY)
+		return NULL;
+
+	restr->alterns = tal_arr(restr, struct rune_altern *, tok->size);
+	json_for_each_arr(i, t, tok) {
+		restr->alterns[i] = rune_altern_from_json(restr->alterns,
+							  buffer, t);
+		if (!restr->alterns[i])
+			return tal_free(restr);
+	}
+	return restr;
 }
 
 static struct command_result *param_restrictions(struct command *cmd,
@@ -794,15 +841,15 @@ static struct command_result *param_restrictions(struct command *cmd,
 			(*restrs)[i] = rune_restr_from_json(*restrs, buffer, t);
 			if (!(*restrs)[i]) {
 				return command_fail_badparam(cmd, name, buffer, t,
-							     "not a valid restriction");
+							     "not a valid restriction (should be array)");
 			}
 		}
 	} else {
 		*restrs = tal_arr(cmd, struct rune_restr *, 1);
 		(*restrs)[0] = rune_restr_from_json(*restrs, buffer, tok);
 		if (!(*restrs)[0])
 			return command_fail_badparam(cmd, name, buffer, tok,
-						     "not a valid restriction");
+						     "not a valid restriction (should be array)");
 	}
 	return NULL;
 }

diff --git a/tests/test_plugin.py b/tests/test_plugin.py
@@ -2652,11 +2652,11 @@ def test_commando_rune(node_factory):
     #   "rune": "zKc2W88jopslgUBl0UE77aEe5PNCLn5WwqSusU_Ov3A9MA=="
     # $ l1-cli commando-rune restrictions=readonly
     #   "rune": "1PJnoR9a7u4Bhglj2s7rVOWqRQnswIwUoZrDVMKcLTY9MSZtZXRob2RebGlzdHxtZXRob2ReZ2V0fG1ldGhvZD1zdW1tYXJ5Jm1ldGhvZC9saXN0ZGF0YXN0b3Jl"
-    # $ l1-cli commando-rune restrictions='time>1656675211'
+    # $ l1-cli commando-rune restrictions='[[time>1656675211]]'
     #   "rune": "RnlWC4lwBULFaObo6ZP8jfqYRyTbfWPqcMT3qW-Wmso9MiZ0aW1lPjE2NTY2NzUyMTE="
-    # $ l1-cli commando-rune restrictions='["id^022d223620a359a47ff7","method=listpeers"]'
+    # $ l1-cli commando-rune restrictions='[["id^022d223620a359a47ff7"],["method=listpeers"]]'
     #   "rune": "lXFWzb51HjWxKV5TmfdiBgd74w0moeyChj3zbLoxmws9MyZpZF4wMjJkMjIzNjIwYTM1OWE0N2ZmNyZtZXRob2Q9bGlzdHBlZXJz"
-    # $ l1-cli commando-rune lXFWzb51HjWxKV5TmfdiBgd74w0moeyChj3zbLoxmws9MyZpZF4wMjJkMjIzNjIwYTM1OWE0N2ZmNyZtZXRob2Q9bGlzdHBlZXJz 'pnamelevel!|pnamelevel/io'
+    # $ l1-cli commando-rune lXFWzb51HjWxKV5TmfdiBgd74w0moeyChj3zbLoxmws9MyZpZF4wMjJkMjIzNjIwYTM1OWE0N2ZmNyZtZXRob2Q9bGlzdHBlZXJz '[pnamelevel!,pnamelevel/io]'
     #   "rune": "Dw2tzGCoUojAyT0JUw7fkYJYqExpEpaDRNTkyvWKoJY9MyZpZF4wMjJkMjIzNjIwYTM1OWE0N2ZmNyZtZXRob2Q9bGlzdHBlZXJzJnBuYW1lbGV2ZWwhfHBuYW1lbGV2ZWwvaW8="
 
     rune1 = l1.rpc.commando_rune()
@@ -2665,31 +2665,31 @@ def test_commando_rune(node_factory):
     rune2 = l1.rpc.commando_rune(restrictions="readonly")
     assert rune2['rune'] == '1PJnoR9a7u4Bhglj2s7rVOWqRQnswIwUoZrDVMKcLTY9MSZtZXRob2RebGlzdHxtZXRob2ReZ2V0fG1ldGhvZD1zdW1tYXJ5Jm1ldGhvZC9saXN0ZGF0YXN0b3Jl'
     assert rune2['unique_id'] == '1'
-    rune3 = l1.rpc.commando_rune(restrictions="time>1656675211")
+    rune3 = l1.rpc.commando_rune(restrictions=[["time>1656675211"]])
     assert rune3['rune'] == 'RnlWC4lwBULFaObo6ZP8jfqYRyTbfWPqcMT3qW-Wmso9MiZ0aW1lPjE2NTY2NzUyMTE='
     assert rune3['unique_id'] == '2'
-    rune4 = l1.rpc.commando_rune(restrictions=["id^022d223620a359a47ff7", "method=listpeers"])
+    rune4 = l1.rpc.commando_rune(restrictions=[["id^022d223620a359a47ff7"], ["method=listpeers"]])
     assert rune4['rune'] == 'lXFWzb51HjWxKV5TmfdiBgd74w0moeyChj3zbLoxmws9MyZpZF4wMjJkMjIzNjIwYTM1OWE0N2ZmNyZtZXRob2Q9bGlzdHBlZXJz'
     assert rune4['unique_id'] == '3'
-    rune5 = l1.rpc.commando_rune(rune4['rune'], "pnamelevel!|pnamelevel/io")
+    rune5 = l1.rpc.commando_rune(rune4['rune'], [["pnamelevel!", "pnamelevel/io"]])
     assert rune5['rune'] == 'Dw2tzGCoUojAyT0JUw7fkYJYqExpEpaDRNTkyvWKoJY9MyZpZF4wMjJkMjIzNjIwYTM1OWE0N2ZmNyZtZXRob2Q9bGlzdHBlZXJzJnBuYW1lbGV2ZWwhfHBuYW1lbGV2ZWwvaW8='
     assert rune5['unique_id'] == '3'
-    rune6 = l1.rpc.commando_rune(rune5['rune'], "parr1!|parr1/io")
+    rune6 = l1.rpc.commando_rune(rune5['rune'], [["parr1!", "parr1/io"]])
     assert rune6['rune'] == '2Wh6F4R51D3esZzp-7WWG51OhzhfcYKaaI8qiIonaHE9MyZpZF4wMjJkMjIzNjIwYTM1OWE0N2ZmNyZtZXRob2Q9bGlzdHBlZXJzJnBuYW1lbGV2ZWwhfHBuYW1lbGV2ZWwvaW8mcGFycjEhfHBhcnIxL2lv'
     assert rune6['unique_id'] == '3'
-    rune7 = l1.rpc.commando_rune(restrictions="pnum=0")
+    rune7 = l1.rpc.commando_rune(restrictions=[["pnum=0"]])
     assert rune7['rune'] == 'QJonN6ySDFw-P5VnilZxlOGRs_tST1ejtd-bAYuZfjk9NCZwbnVtPTA='
     assert rune7['unique_id'] == '4'
-    rune8 = l1.rpc.commando_rune(rune7['rune'], "rate=3")
+    rune8 = l1.rpc.commando_rune(rune7['rune'], [["rate=3"]])
     assert rune8['rune'] == 'kSYFx6ON9hr_ExcQLwVkm1ABnvc1TcMFBwLrAVee0EA9NCZwbnVtPTAmcmF0ZT0z'
     assert rune8['unique_id'] == '4'
-    rune9 = l1.rpc.commando_rune(rune8['rune'], "rate=1")
+    rune9 = l1.rpc.commando_rune(rune8['rune'], [["rate=1"]])
     assert rune9['rune'] == 'O8Zr-ULTBKO3_pKYz0QKE9xYl1vQ4Xx9PtlHuist9Rk9NCZwbnVtPTAmcmF0ZT0zJnJhdGU9MQ=='
     assert rune9['unique_id'] == '4'
 
     # Test rune with \|.
-    weirdrune = l1.rpc.commando_rune(restrictions=["method=invoice",
-                                                   "pnamedescription=@tipjar\\|jb55@sendsats.lol"])
+    weirdrune = l1.rpc.commando_rune(restrictions=[["method=invoice"],
+                                                   ["pnamedescription=@tipjar|jb55@sendsats.lol"]])
     with pytest.raises(RpcError, match='Not authorized:'):
         l2.rpc.call(method='commando',
                     payload={'peer_id': l1.info['id'],
@@ -2761,7 +2761,7 @@ def test_commando_rune(node_factory):
 
     # Replace rune3 with a more useful timestamp!
     expiry = int(time.time()) + 15
-    rune3 = l1.rpc.commando_rune(restrictions="time<{}".format(expiry))
+    rune3 = l1.rpc.commando_rune(restrictions=[["time<{}".format(expiry)]])
 
     successes = ((rune1, "listpeers", {}),
                  (rune2, "listpeers", {}),