diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4431b64d..24ae6967 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -99,6 +99,8 @@ Developers may need to change their code
### Corrected Bugs
+* Fixed: [RPC edit-config payloads are not fully validated](https://github.com/clicon/clixon/issues/337)
+
## 5.7.0
17 May 2022
diff --git a/apps/backend/backend_client.c b/apps/backend/backend_client.c
index 45dda83a..ba48ba52 100644
--- a/apps/backend/backend_client.c
+++ b/apps/backend/backend_client.c
@@ -274,7 +274,15 @@ clixon_stats_module_get(clicon_handle h,
* @param[in] regarg User argument given at rpc_callback_register()
* @retval 0 OK
* @retval -1 Error
- */
+ * @note According to RFC 7950 8.3: constraints MUST be enforced:
+ * o during parsing of RPC payloads
+ * o during processing of the operation
+ * But Clixon is flexible in allowing invalid XML payload here, only some specialized
+ * validations are made.
+ * However, what really should be done is to apply the change to the datastore and then
+ * validate, if error, discard to previous state.
+ * But this could discard other previous changes to candidate.
+ */
static int
from_client_edit_config(clicon_handle h,
cxobj *xn,
@@ -383,14 +391,19 @@ from_client_edit_config(clicon_handle h,
goto done;
goto ok;
}
+ /* Limited validation of incoming payload
+ */
+ if ((ret = xml_yang_check_list_unique_minmax(xc, &xret)) < 0)
+ goto done;
/* xmldb_put (difflist handling) requires list keys */
- if ((ret = xml_yang_validate_list_key_only(xc, &xret)) < 0)
+ if (ret==1 && (ret = xml_yang_validate_list_key_only(xc, &xret)) < 0)
goto done;
if (ret == 0){
if (clixon_xml2cbuf(cbret, xret, 0, 0, -1, 0) < 0)
goto done;
goto ok;
}
+
/* Cant do this earlier since we dont have a yang spec to
* the upper part of the tree, until we get the "config" tree.
*/
@@ -1234,7 +1247,9 @@ from_client_msg(clicon_handle h,
clicon_err(OE_XML, errno, "cbuf_new");
goto done;
}
- /* Decode msg from client -> xml top (ct) and session id */
+ /* Decode msg from client -> xml top (ct) and session id
+ * Bind is a part of the decode function
+ */
if ((ret = clicon_msg_decode(msg, yspec, &id, &xt, &xret)) < 0){
if (netconf_malformed_message(cbret, "XML parse error") < 0)
goto done;
diff --git a/apps/cli/cli_common.c b/apps/cli/cli_common.c
index 06aa3586..430868cc 100644
--- a/apps/cli/cli_common.c
+++ b/apps/cli/cli_common.c
@@ -797,6 +797,7 @@ compare_dbs(clicon_handle h,
/*! Load a configuration file to candidate database
* Utility function used by cligen spec file
+ * Note that the CLI function makes no Validation of the XML sent to the backend
* @param[in] h CLICON handle
* @param[in] cvv Vector of variables (where is found)
* @param[in] argv A string: " []"
diff --git a/lib/clixon/clixon_validate.h b/lib/clixon/clixon_validate.h
index 10cc8a5f..aedc378d 100644
--- a/lib/clixon/clixon_validate.h
+++ b/lib/clixon/clixon_validate.h
@@ -45,6 +45,7 @@
*/
int xml_yang_validate_rpc(clicon_handle h, cxobj *xrpc, cxobj **xret);
int xml_yang_validate_rpc_reply(clicon_handle h, cxobj *xrpc, cxobj **xret);
+int xml_yang_check_list_unique_minmax(cxobj *xt, cxobj **xret);
int xml_yang_validate_add(clicon_handle h, cxobj *xt, cxobj **xret);
int xml_yang_validate_list_key_only(cxobj *xt, cxobj **xret);
int xml_yang_validate_all(clicon_handle h, cxobj *xt, cxobj **xret);
diff --git a/lib/src/clixon_validate.c b/lib/src/clixon_validate.c
index 01b92ba8..c9db689f 100644
--- a/lib/src/clixon_validate.c
+++ b/lib/src/clixon_validate.c
@@ -1093,9 +1093,9 @@ check_min_max(cxobj *xp,
* XML node. This may not be a large problem since it would mean empty configs
* are not allowed.
*/
-static int
-check_list_unique_minmax(cxobj *xt,
- cxobj **xret)
+int
+xml_yang_check_list_unique_minmax(cxobj *xt,
+ cxobj **xret)
{
int retval = -1;
cxobj *x = NULL;
@@ -1458,7 +1458,6 @@ xml_yang_validate_leaf_union(clicon_handle h,
* @endcode
* @see xml_yang_validate_add
* @see xml_yang_validate_rpc
- * @note Should need a variant accepting cxobj **xret
*/
int
xml_yang_validate_all(clicon_handle h,
@@ -1601,7 +1600,7 @@ xml_yang_validate_all(clicon_handle h,
/* Check unique and min-max after choice test for example*/
if (yang_config(yt) != 0){
/* Checks if next level contains any unique list constraints */
- if ((ret = check_list_unique_minmax(xt, xret)) < 0)
+ if ((ret = xml_yang_check_list_unique_minmax(xt, xret)) < 0)
goto done;
if (ret == 0)
goto fail;
@@ -1618,7 +1617,8 @@ xml_yang_validate_all(clicon_handle h,
retval = 0;
goto done;
}
-/*! Translate a single xml node to a cligen variable vector. Note not recursive
+
+/*! Validate a single XML node with yang specification
* @param[out] xret Error XML tree (if ret == 0). Free with xml_free after use
* @retval 1 Validation OK
* @retval 0 Validation failed (xret set)
@@ -1637,7 +1637,7 @@ xml_yang_validate_all_top(clicon_handle h,
if ((ret = xml_yang_validate_all(h, x, xret)) < 1)
return ret;
}
- if ((ret = check_list_unique_minmax(xt, xret)) < 1)
+ if ((ret = xml_yang_check_list_unique_minmax(xt, xret)) < 1)
return ret;
return 1;
}
diff --git a/test/lib.sh b/test/lib.sh
index 2539c766..7014f624 100755
--- a/test/lib.sh
+++ b/test/lib.sh
@@ -779,7 +779,7 @@ EOF
r=$(echo "$ret" | grep --null -Go "$i")
match=$?
if [ $match -ne 0 ]; then
- err "$i" "$ret"
+ err "$expectenc" "$ret"
fi
done <<< "$expectenc"
fi
diff --git a/test/test_datastore_format.sh b/test/test_datastore_format.sh
index 821b2248..56ace059 100755
--- a/test/test_datastore_format.sh
+++ b/test/test_datastore_format.sh
@@ -136,7 +136,7 @@ if [ $BE -ne 0 ]; then
err
fi
new "start backend -s init -f $cfg"
- start_backend -s startup -f $cfg
+ start_backend -s init -f $cfg
fi
new "wait backend"
diff --git a/test/test_db.sh b/test/test_db.sh
index 0cf9eee7..3abf4a84 100755
--- a/test/test_db.sh
+++ b/test/test_db.sh
@@ -2,6 +2,7 @@
# Datastore tests:
# - XML and JSON
# - save and load config files
+# Pretty and not
# Magic line must be first in script (see README.md)
s="$_" ; . ./lib.sh || if [ "$s" = $0 ]; then exit 0; else return 0; fi
@@ -47,6 +48,11 @@ module clixon-example{
leaf value{
type string;
}
+ container two{
+ leaf a{
+ type string;
+ }
+ }
}
}
}
@@ -69,6 +75,7 @@ discard("Discard edits (rollback 0)"), discard_changes();
load("Load configuration from XML file") ("Filename (local filename)"){
xml("Replace candidate with file containing XML"), load_config_file("","filename", "replace", "xml");
json("Replace candidate with file containing JSON"), load_config_file("","filename", "replace", "json");
+ merge("Merge file with existent candidate"), load_config_file("filename", "merge");
}
save("Save candidate configuration to XML file") ("Filename (local filename)"){
xml("Save configuration as XML"), save_config_file("candidate","filename", "xml");
@@ -181,17 +188,79 @@ EOF
new "test params: -f $cfg"
-new "test db xml"
-testrun xml false
+for format in xml json; do
+ for pretty in false true json; do
+ new "test db $format pretty=$pretty"
+ testrun xml false
+ done
+done
-new "test db xml pretty"
-testrun xml true
+# Negative test, load yang-invalid xml
+if [ $BE -ne 0 ]; then
+ new "kill old backend"
+ sudo clixon_backend -z -f $cfg
+ if [ $? -ne 0 ]; then
+ err
+ fi
+ new "start backend -s init -f $cfg -o CLICON_XMLDB_FORMAT=$format -o CLICON_XMLDB_PRETTY=$pretty"
+ start_backend -s init -f $cfg -o CLICON_XMLDB_FORMAT=$format -o CLICON_XMLDB_PRETTY=$pretty
+fi
-new "test db json"
-testrun json false
+new "wait backend"
+wait_backend
-new "test db json pretty"
-testrun json true
+# Wrong: two toplevels
+cat < $dir/myconfig
+<${DATASTORE_TOP}>
+