[mod_json_cdr] Fix malformed JSON in URL-encoded POST bodies (#3152)

`switch_url_encode()` encodes with `double_encode` false, so a `%XX`
already present in `json_text` was emitted unchanged and became
indistinguishable from the escapes the body encoding added itself.
Decoding the body then consumed the values' own escapes as well, and
the document no longer parsed. Values arrive pre-encoded whenever
`encode-values` is on, and can hold percent-hex text regardless of it.

Encode with `switch_url_encode_opt(..., SWITCH_TRUE)` so `%` is encoded
too and the body decodes back to exactly the serialized document.
`mod_xml_cdr` and `mod_format_cdr` already encode their bodies this way.

- Size the escape buffer `* 3 + 1`. `switch_url_encode_opt()` reserves
  the terminator from the length it is given, so `* 3` dropped the last
  escaped character when every byte needed encoding. The base64 call now
  takes that length directly.
- Warn at load when `encode` and `encode-values` are both on, since the
  values stay encoded after the body is decoded.
- Correct both encoding config comments; the `encode` one described only
  `base64`.

Adds core coverage in `tests/unit/switch_utils.c` for both
`switch_url_encode_opt()` modes, its output bounds, and a JSON body
encoded and decoded once.

src/mod/event_handlers/mod_json_cdr/conf/autoload_configs/json_cdr.conf.xml

@@ -6,7 +6,8 @@
 			<param name="log-b-leg" value="true"/>
 			<param name="prefix-a-leg" value="false"/>
 
-			<!-- Whether to URL encode the individual JSON values. Defaults to true, set to false for standard JSON. -->
+			<!-- Whether to URL encode the individual JSON values. Defaults to true; set to false
+			     to leave values as they are. -->
 			<param name="encode-values" value="true"/>
 
 			<!-- Normally if url and log-dir are present, url is attempted first and log-dir second.
@@ -29,7 +30,9 @@
 			<param name="auth-scheme" value="basic"/>
 			<!-- Credentials in the form  username:password  if auth-scheme is used. Leave empty for no authentication. -->
 			<param name="cred" value="string"/>
-			<!-- Whether to base64 encode the entire JSON document before POSTing it. -->
+			<!-- How to encode the POSTed body: 'true' for URL encoding, 'base64' for base64,
+			     'false' or unset for raw JSON. 'true' encodes the whole body on top of any
+			     encoding encode-values already applied to the values. -->
 			<param name="encode" value="base64|true|false"/>
 			<!-- Number of retries in case of failure. Each specified URL is tried in turn. -->
 			<param name="retries" value="0"/>

src/mod/event_handlers/mod_json_cdr/mod_json_cdr.c

@@ -473,15 +473,17 @@ static switch_status_t my_on_reporting(switch_core_session_t *session)
 	json_text = cJSON_PrintUnformatted(json_cdr);
 
 	if (globals.url_count && globals.encode) {
-		switch_size_t need_bytes = strlen(json_text) * 3;
+		switch_size_t json_len = strlen(json_text);
+		switch_size_t need_bytes = json_len * 3 + 1;
 
 		json_text_escaped = malloc(need_bytes);
 		switch_assert(json_text_escaped);
 		memset(json_text_escaped, 0, need_bytes);
 		if (globals.encode == ENCODING_DEFAULT) {
-			switch_url_encode(json_text, json_text_escaped, need_bytes);
+			/* json_text may already hold %XX, so '%' is encoded rather than passed through. */
+			switch_url_encode_opt(json_text, json_text_escaped, need_bytes, SWITCH_TRUE);
 		} else {
-			switch_b64_encode((unsigned char *) json_text, need_bytes / 3, (unsigned char *) json_text_escaped, need_bytes);
+			switch_b64_encode((unsigned char *) json_text, json_len, (unsigned char *) json_text_escaped, need_bytes);
 		}
 	}
 
@@ -723,6 +725,12 @@ SWITCH_MODULE_LOAD_FUNCTION(mod_json_cdr_load)
 		globals.delay = 5;
 	}
 
+	if (globals.encode == ENCODING_DEFAULT && globals.encode_values == ENCODING_DEFAULT) {
+		switch_log_printf(SWITCH_CHANNEL_LOG, SWITCH_LOG_WARNING,
+						  "encode=true with encode-values=true: values stay percent-encoded after the body is "
+						  "decoded, so each value needs decoding too. Set encode-values=false to send them as-is.\n");
+	}
+
 	globals.retries++;
 
 	set_json_cdr_log_dirs();

tests/unit/switch_utils.c

@@ -62,6 +62,147 @@ FST_TEST_BEGIN(benchmark)
 }
 FST_TEST_END()
 
+FST_TEST_BEGIN(url_encode_double_encode)
+{
+	static const struct {
+		const char *in;
+		const char *plain;
+		const char *doubled;
+		const char *rule;
+	} cases[] = {
+		{ "ABCD",     "ABCD",        "ABCD",        "nothing unsafe is copied through unchanged" },
+		{ "50% off",  "50%25%20off", "50%25%20off", "a '%' without two hex digits after it is encoded either way" },
+		{ "50%20off", "50%20off",    "50%2520off",  "a '%' with two hex digits after it is the case the modes differ on" },
+		{ "x%22y",    "x%22y",       "x%2522y",     "an encoded quote is either passed through or protected" },
+		{ "abc%2",    "abc%252",     "abc%252",     "too few characters follow the '%' for it to be an escape" },
+		{ "%2a",      "%252a",       "%252a",       "only uppercase hex counts as an existing escape" }
+	};
+	char plain[64];
+	char doubled[64];
+	char msg[192];
+
+	for (int i = 0; i < (int) (sizeof(cases) / sizeof(cases[0])); i++) {
+		switch_url_encode_opt(cases[i].in, plain, sizeof(plain), SWITCH_FALSE);
+		switch_url_encode_opt(cases[i].in, doubled, sizeof(doubled), SWITCH_TRUE);
+
+		switch_snprintf(msg, sizeof(msg), "[%s] without double_encode: %s", cases[i].in, cases[i].rule);
+		fst_xcheck(!strcmp(plain, cases[i].plain), msg);
+
+		switch_snprintf(msg, sizeof(msg), "[%s] with double_encode: %s", cases[i].in, cases[i].rule);
+		fst_xcheck(!strcmp(doubled, cases[i].doubled), msg);
+	}
+}
+FST_TEST_END()
+
+FST_TEST_BEGIN(url_encode_opt_output_bounds)
+{
+	/* The 0xAA sentinel across the destination catches any write outside the region the
+	   encode call is allowed to touch. */
+	char guarded[32];
+	const char *all_unsafe = "\"\"\"";
+
+	/* Every input character encodes to three bytes, so a buffer of strlen * 3 + 1 is the
+	   smallest that holds the result and its terminator. */
+	memset(guarded, 0xAA, sizeof(guarded));
+	switch_url_encode_opt(all_unsafe, guarded, strlen(all_unsafe) * 3 + 1, SWITCH_FALSE);
+	fst_check_string_equals(guarded, "%22%22%22");
+	fst_xcheck(guarded[9] == '\0', "the terminator must land right after the last encoded byte");
+	for (int i = 10; i < (int) sizeof(guarded); i++) {
+		fst_xcheck(guarded[i] == (char) 0xAA, "encode must not write past the terminator");
+	}
+
+	/* One byte short of that, the last group does not fit and the output stops early
+	   rather than overrunning. */
+	memset(guarded, 0xAA, sizeof(guarded));
+	switch_url_encode_opt(all_unsafe, guarded, strlen(all_unsafe) * 3, SWITCH_FALSE);
+	fst_check_string_equals(guarded, "%22%22");
+	for (int i = 7; i < (int) sizeof(guarded); i++) {
+		fst_xcheck(guarded[i] == (char) 0xAA, "a bounded encode must not write past the terminator");
+	}
+}
+FST_TEST_END()
+
+FST_TEST_BEGIN(url_encoded_json_body_round_trip)
+{
+	/* Mirrors how a CDR body is assembled: each value may be URL encoded, the document is
+	   serialized, the whole body is URL encoded, and the receiver decodes it once. The two
+	   cases differ only in where the %XX inside the value comes from. */
+	static const struct {
+		const char *value;
+		switch_bool_t encode_value;
+		const char *rule;
+	} cases[] = {
+		{ "\"6140\" <sip:6140@203.0.113.10>;tag=x", SWITCH_TRUE,  "value encoded by the value layer" },
+		{ "x%22y",                                  SWITCH_FALSE, "value holding percent-hex text of its own" }
+	};
+	char stored[512];
+	char body[4096];
+	char decoded[4096];
+	char msg[192];
+	cJSON *json = NULL;
+	cJSON *parsed = NULL;
+	char *json_text = NULL;
+
+	for (int i = 0; i < (int) (sizeof(cases) / sizeof(cases[0])); i++) {
+		if (cases[i].encode_value) {
+			switch_url_encode(cases[i].value, stored, sizeof(stored));
+		} else {
+			switch_set_string(stored, cases[i].value);
+		}
+
+		json = cJSON_CreateObject();
+		cJSON_AddItemToObject(json, "v", cJSON_CreateString(stored));
+		json_text = cJSON_PrintUnformatted(json);
+		if (!json_text) {
+			switch_snprintf(msg, sizeof(msg), "failed to serialize the document for a %s", cases[i].rule);
+			fst_fail(msg);
+			goto url_encoded_json_body_round_trip_done;
+		}
+
+		/* double_encode protects the escapes in the value, so one decode returns the document
+		   unchanged and the value keeps its own text. */
+		switch_url_encode_opt(json_text, body, sizeof(body), SWITCH_TRUE);
+		switch_set_string(decoded, body);
+		switch_url_decode(decoded);
+		switch_snprintf(msg, sizeof(msg), "a double encoded body must decode back to the document: %s", cases[i].rule);
+		fst_xcheck(!strcmp(decoded, json_text), msg);
+
+		parsed = cJSON_Parse(decoded);
+		switch_snprintf(msg, sizeof(msg), "a double encoded body must parse after one decode: %s", cases[i].rule);
+		fst_xcheck(parsed != NULL, msg);
+		if (parsed) {
+			switch_snprintf(msg, sizeof(msg), "the value must survive unchanged: %s", cases[i].rule);
+			fst_xcheck(!strcmp(cJSON_GetObjectCstr(parsed, "v"), stored), msg);
+			cJSON_Delete(parsed);
+			parsed = NULL;
+		}
+
+		/* Without it the single decode reaches into the value as well, and the document no
+		   longer parses. */
+		switch_url_encode_opt(json_text, body, sizeof(body), SWITCH_FALSE);
+		switch_set_string(decoded, body);
+		switch_url_decode(decoded);
+		switch_snprintf(msg, sizeof(msg), "a singly encoded body must not decode back to the document: %s", cases[i].rule);
+		fst_xcheck(strcmp(decoded, json_text), msg);
+
+		parsed = cJSON_Parse(decoded);
+		switch_snprintf(msg, sizeof(msg), "a singly encoded body must not survive one decode: %s", cases[i].rule);
+		fst_xcheck(parsed == NULL, msg);
+		cJSON_Delete(parsed);
+		parsed = NULL;
+
+		cJSON_Delete(json);
+		json = NULL;
+		switch_safe_free(json_text);
+	}
+
+url_encoded_json_body_round_trip_done:
+	cJSON_Delete(parsed);
+	cJSON_Delete(json);
+	switch_safe_free(json_text);
+}
+FST_TEST_END()
+
 FST_TEST_BEGIN(b64)
 {
     switch_size_t size;