Skip to content

Commit 1916516

Browse files
Easton97-JensEaston97-Jens
authored
Merge pull request #68 from Easton97-Jens/codex/improve-sonarqube-compliance-in-request-body-processor
Refactor JSON processing: use smart pointers, unify error/sink handling, and tidy instrumention
2 parents bbde416 + fb9a852 commit 1916516

7 files changed

Lines changed: 215 additions & 264 deletions

File tree

‎src/request_body_processor/json.cc‎

Lines changed: 66 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
#include <chrono>
2323
#include <cstdint>
24+
#include <memory>
2425
#include <string>
2526

2627
#include "src/request_body_processor/json_adapter.h"
@@ -34,10 +35,44 @@ static const char* json_depth_limit_exceeded_msg = ". Parsing depth limit exceed
3435

3536
namespace {
3637

37-
JsonSinkStatus startContainer(std::deque<JSONContainer *> *containers,
38-
JSONContainer *container, int64_t *current_depth, double max_depth,
39-
bool *depth_limit_exceeded) {
40-
containers->push_back(container);
38+
void assignJsonErrorMessage(std::string *err, JsonParseStatus parse_status,
39+
const std::string &detail) {
40+
if (err == nullptr) {
41+
return;
42+
}
43+
44+
if (!detail.empty()) {
45+
err->assign(detail);
46+
return;
47+
}
48+
49+
switch (parse_status) {
50+
case JsonParseStatus::ParseError:
51+
err->assign("Invalid JSON body.");
52+
break;
53+
case JsonParseStatus::TruncatedInput:
54+
err->assign("Incomplete JSON body.");
55+
break;
56+
case JsonParseStatus::Utf8Error:
57+
err->assign("Invalid UTF-8 in JSON body.");
58+
break;
59+
case JsonParseStatus::EngineAbort:
60+
err->assign("JSON traversal aborted by ModSecurity.");
61+
break;
62+
case JsonParseStatus::InternalError:
63+
err->assign("Internal JSON backend failure.");
64+
break;
65+
case JsonParseStatus::Ok:
66+
err->clear();
67+
break;
68+
}
69+
}
70+
71+
JsonSinkStatus startContainer(
72+
std::deque<std::unique_ptr<JSONContainer>> *containers,
73+
std::unique_ptr<JSONContainer> container, int64_t *current_depth,
74+
double max_depth, bool *depth_limit_exceeded) {
75+
containers->push_back(std::move(container));
4176
(*current_depth)++;
4277
if (*current_depth > max_depth) {
4378
*depth_limit_exceeded = true;
@@ -46,18 +81,17 @@ JsonSinkStatus startContainer(std::deque<JSONContainer *> *containers,
4681
return JsonSinkStatus::Continue;
4782
}
4883

49-
JsonSinkStatus endContainer(std::deque<JSONContainer *> *containers,
84+
JsonSinkStatus endContainer(
85+
std::deque<std::unique_ptr<JSONContainer>> *containers,
5086
int64_t *current_depth) {
5187
if (containers->empty()) {
5288
return JsonSinkStatus::InternalError;
5389
}
5490

55-
JSONContainer *container = containers->back();
5691
containers->pop_back();
57-
delete container;
5892

59-
if (containers->empty() == false) {
60-
auto *array = dynamic_cast<JSONContainerArray *>(containers->back());
93+
if (!containers->empty()) {
94+
auto *array = dynamic_cast<JSONContainerArray *>(containers->back().get());
6195
if (array != nullptr) {
6296
array->m_elementCounter++;
6397
}
@@ -72,6 +106,16 @@ JsonSinkStatus endContainer(std::deque<JSONContainer *> *containers,
72106
return JsonSinkStatus::Continue;
73107
}
74108

109+
JsonSinkStatus addArgumentAsSinkStatus(JSON *json,
110+
const std::string &argument_value) {
111+
return json->addArgument(argument_value) != 0 ? JsonSinkStatus::Continue
112+
: JsonSinkStatus::EngineAbort;
113+
}
114+
115+
JsonSinkStatus addStringViewAsSinkStatus(JSON *json, std::string_view value) {
116+
return addArgumentAsSinkStatus(json, std::string(value.data(), value.size()));
117+
}
118+
75119
} // namespace
76120

77121
JSON::JSON(Transaction *transaction) : m_transaction(transaction),
@@ -129,48 +173,7 @@ bool JSON::complete(std::string *err) {
129173
if (result.sink_status == JsonSinkStatus::DepthLimitExceeded) {
130174
m_depth_limit_exceeded = true;
131175
}
132-
if (err != nullptr) {
133-
switch (result.parse_status) {
134-
case JsonParseStatus::ParseError:
135-
if (result.detail.empty()) {
136-
err->assign("Invalid JSON body.");
137-
} else {
138-
err->assign(result.detail);
139-
}
140-
break;
141-
case JsonParseStatus::TruncatedInput:
142-
if (result.detail.empty()) {
143-
err->assign("Incomplete JSON body.");
144-
} else {
145-
err->assign(result.detail);
146-
}
147-
break;
148-
case JsonParseStatus::Utf8Error:
149-
if (result.detail.empty()) {
150-
err->assign("Invalid UTF-8 in JSON body.");
151-
} else {
152-
err->assign(result.detail);
153-
}
154-
break;
155-
case JsonParseStatus::EngineAbort:
156-
if (result.detail.empty()) {
157-
err->assign("JSON traversal aborted by ModSecurity.");
158-
} else {
159-
err->assign(result.detail);
160-
}
161-
break;
162-
case JsonParseStatus::InternalError:
163-
if (result.detail.empty()) {
164-
err->assign("Internal JSON backend failure.");
165-
} else {
166-
err->assign(result.detail);
167-
}
168-
break;
169-
case JsonParseStatus::Ok:
170-
err->clear();
171-
break;
172-
}
173-
}
176+
assignJsonErrorMessage(err, result.parse_status, result.detail);
174177
if (m_depth_limit_exceeded && err != nullptr) {
175178
err->append(json_depth_limit_exceeded_msg);
176179
}
@@ -187,9 +190,9 @@ int JSON::addArgument(const std::string& value) {
187190

188191
for (size_t i = 0; i < m_containers.size(); i++) {
189192
const JSONContainerArray *a = dynamic_cast<JSONContainerArray *>(
190-
m_containers[i]);
193+
m_containers[i].get());
191194
path = path + m_containers[i]->m_name;
192-
if (a != NULL) {
195+
if (a != nullptr) {
193196
path = path + ".array_" + std::to_string(a->m_elementCounter);
194197
} else {
195198
path = path + ".";
@@ -198,7 +201,7 @@ int JSON::addArgument(const std::string& value) {
198201

199202
if (m_containers.size() > 0) {
200203
JSONContainerArray *a = dynamic_cast<JSONContainerArray *>(
201-
m_containers.back());
204+
m_containers.back().get());
202205
if (a) {
203206
a->m_elementCounter++;
204207
} else {
@@ -225,35 +228,28 @@ JsonSinkStatus JSON::on_key(std::string_view value) {
225228

226229

227230
JsonSinkStatus JSON::on_null() {
228-
return addArgument("") != 0 ? JsonSinkStatus::Continue
229-
: JsonSinkStatus::EngineAbort;
231+
return addArgumentAsSinkStatus(this, "");
230232
}
231233

232234

233235
JsonSinkStatus JSON::on_boolean(bool value) {
234-
if (value) {
235-
return addArgument("true") != 0 ? JsonSinkStatus::Continue
236-
: JsonSinkStatus::EngineAbort;
237-
}
238-
return addArgument("false") != 0 ? JsonSinkStatus::Continue
239-
: JsonSinkStatus::EngineAbort;
236+
return addArgumentAsSinkStatus(this, value ? "true" : "false");
240237
}
241238

242239

243240
JsonSinkStatus JSON::on_string(std::string_view value) {
244-
return addArgument(std::string(value.data(), value.size())) != 0
245-
? JsonSinkStatus::Continue : JsonSinkStatus::EngineAbort;
241+
return addStringViewAsSinkStatus(this, value);
246242
}
247243

248244

249245
JsonSinkStatus JSON::on_number(std::string_view value) {
250-
return addArgument(std::string(value.data(), value.size())) != 0
251-
? JsonSinkStatus::Continue : JsonSinkStatus::EngineAbort;
246+
return addStringViewAsSinkStatus(this, value);
252247
}
253248

254249

255250
JsonSinkStatus JSON::on_start_array() {
256-
return startContainer(&m_containers, new JSONContainerArray(getCurrentKey()),
251+
return startContainer(&m_containers,
252+
std::make_unique<JSONContainerArray>(getCurrentKey()),
257253
&m_current_depth, m_max_depth, &m_depth_limit_exceeded);
258254
}
259255

@@ -264,7 +260,8 @@ JsonSinkStatus JSON::on_end_array() {
264260

265261

266262
JsonSinkStatus JSON::on_start_object() {
267-
return startContainer(&m_containers, new JSONContainerMap(getCurrentKey()),
263+
return startContainer(&m_containers,
264+
std::make_unique<JSONContainerMap>(getCurrentKey()),
268265
&m_current_depth, m_max_depth, &m_depth_limit_exceeded);
269266
}
270267

@@ -274,11 +271,7 @@ JsonSinkStatus JSON::on_end_object() {
274271
}
275272

276273
void JSON::clearContainers() {
277-
while (m_containers.size() > 0) {
278-
JSONContainer *a = m_containers.back();
279-
m_containers.pop_back();
280-
delete a;
281-
}
274+
m_containers.clear();
282275
}
283276

284277
} // namespace modsecurity::RequestBodyProcessor

‎src/request_body_processor/json.h‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@
1616
#ifndef SRC_REQUEST_BODY_PROCESSOR_JSON_H_
1717
#define SRC_REQUEST_BODY_PROCESSOR_JSON_H_
1818

19+
#include <cstdint>
1920
#include <deque>
21+
#include <memory>
2022
#include <string>
2123

2224
#include "modsecurity/transaction.h"
@@ -53,6 +55,10 @@ class JSON : public JsonEventSink {
5355
public:
5456
explicit JSON(Transaction *transaction);
5557
~JSON() override;
58+
JSON(const JSON &) = delete;
59+
JSON &operator=(const JSON &) = delete;
60+
JSON(JSON &&) = delete;
61+
JSON &operator=(JSON &&) = delete;
5662

5763
bool init();
5864
bool processChunk(const char *buf, unsigned int size,
@@ -72,22 +78,21 @@ class JSON : public JsonEventSink {
7278
JsonSinkStatus on_null() override;
7379

7480
bool isPreviousArray() const {
75-
const JSONContainerArray *prev = NULL;
76-
if (m_containers.size() < 1) {
81+
if (m_containers.empty()) {
7782
return false;
7883
}
79-
prev = dynamic_cast<JSONContainerArray *>(
80-
m_containers[m_containers.size() - 1]);
81-
return prev != NULL;
84+
const JSONContainerArray *prev = dynamic_cast<JSONContainerArray *>(
85+
m_containers.back().get());
86+
return prev != nullptr;
8287
}
8388

8489
std::string getCurrentKey(bool emptyIsNull = false) {
8590
std::string ret(m_current_key);
8691
if (m_containers.size() == 0) {
8792
return "json";
8893
}
89-
if (m_current_key.empty() == true) {
90-
if (isPreviousArray() || emptyIsNull == true) {
94+
if (m_current_key.empty()) {
95+
if (isPreviousArray() || emptyIsNull) {
9196
return "";
9297
}
9398
return "empty-key";
@@ -103,7 +108,7 @@ class JSON : public JsonEventSink {
103108
private:
104109
void clearContainers();
105110

106-
std::deque<JSONContainer *> m_containers;
111+
std::deque<std::unique_ptr<JSONContainer>> m_containers;
107112
Transaction *m_transaction;
108113
std::string m_current_key;
109114
std::string m_data;

‎src/request_body_processor/json_adapter.cc‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -53,11 +53,9 @@ JsonParseResult normalizeResult(JsonParseResult result) {
5353
return result;
5454
}
5555

56-
} // namespace
57-
58-
JsonParseResult JSONAdapter::parseImpl(const std::string &input,
59-
JsonEventSink *sink,
60-
const JsonBackendParseOptions &options [[maybe_unused]]) const {
56+
template <typename InputType>
57+
JsonParseResult parseImplCommon(InputType &input, JsonEventSink *sink,
58+
const JsonBackendParseOptions &options [[maybe_unused]]) {
6159
if (sink == nullptr) {
6260
return makeResult(JsonParseStatus::InternalError,
6361
JsonSinkStatus::InternalError, "JSON event sink is null.");
@@ -78,6 +76,20 @@ JsonParseResult JSONAdapter::parseImpl(const std::string &input,
7876
#endif
7977
}
8078

79+
} // namespace
80+
81+
JsonParseResult JSONAdapter::parseImpl(std::string &input,
82+
JsonEventSink *sink,
83+
const JsonBackendParseOptions &options [[maybe_unused]]) const {
84+
return parseImplCommon(input, sink, options);
85+
}
86+
87+
JsonParseResult JSONAdapter::parseImpl(const std::string &input,
88+
JsonEventSink *sink,
89+
const JsonBackendParseOptions &options [[maybe_unused]]) const {
90+
return parseImplCommon(input, sink, options);
91+
}
92+
8193
JsonParseResult JSONAdapter::parse(std::string &input,
8294
JsonEventSink *sink,
8395
const JsonBackendParseOptions &options) const {

‎src/request_body_processor/json_adapter.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,9 @@ class JSONAdapter {
3333
= JsonBackendParseOptions()) const;
3434

3535
private:
36+
JsonParseResult parseImpl(std::string &input, JsonEventSink *sink,
37+
const JsonBackendParseOptions &options [[maybe_unused]]) const;
38+
3639
JsonParseResult parseImpl(const std::string &input, JsonEventSink *sink,
3740
const JsonBackendParseOptions &options [[maybe_unused]]) const;
3841
};

0 commit comments

Comments
 (0)