From 3af21e69e551a73e580551e774f2c2132cdb01ae Mon Sep 17 00:00:00 2001 From: Dmitry Verenitsin Date: Wed, 9 Sep 2026 20:47:24 +0500 Subject: [PATCH] [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. --- .../conf/autoload_configs/json_cdr.conf.xml | 7 +- .../mod_json_cdr/mod_json_cdr.c | 14 +- tests/unit/switch_utils.c | 141 ++++++++++++++++++ 3 files changed, 157 insertions(+), 5 deletions(-) diff --git a/src/mod/event_handlers/mod_json_cdr/conf/autoload_configs/json_cdr.conf.xml b/src/mod/event_handlers/mod_json_cdr/conf/autoload_configs/json_cdr.conf.xml index af30c67c07..ea5cc06caa 100644 --- a/src/mod/event_handlers/mod_json_cdr/conf/autoload_configs/json_cdr.conf.xml +++ b/src/mod/event_handlers/mod_json_cdr/conf/autoload_configs/json_cdr.conf.xml @@ -6,7 +6,8 @@ - + - + diff --git a/src/mod/event_handlers/mod_json_cdr/mod_json_cdr.c b/src/mod/event_handlers/mod_json_cdr/mod_json_cdr.c index 7f66aa425a..ed216841d2 100644 --- a/src/mod/event_handlers/mod_json_cdr/mod_json_cdr.c +++ b/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(); diff --git a/tests/unit/switch_utils.c b/tests/unit/switch_utils.c index bbd250bb11..07657141d5 100644 --- a/tests/unit/switch_utils.c +++ b/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\" ;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;