Skip to content

Commit 96ac18f

Browse files
committed
Fix validation findings: bounds check, DRY, const refs, empty guard
- Add bounds checking for url_pieces[chunks[i]] to prevent potential buffer overflow when cached endpoint chunks don't match request pieces - Extract URL parameter extraction from cache hit/miss paths into single block after match resolution (DRY fix) - Use const references for vectors instead of by-value copies to avoid heap allocations under cache lock - Add empty-string guard to standardize_url before calling back() - Extract ensure_path_pieces_cached() helper to deduplicate caching logic in get_path_pieces() and get_path_piece() - Add RFC 7230 comment documenting case-sensitive strcmp intent
1 parent a2afe2b commit 96ac18f

File tree

3 files changed

+31
-30
lines changed

3 files changed

+31
-30
lines changed

src/http_utils.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,8 @@ std::vector<std::string> http_utils::tokenize_url(const std::string& str, const
218218
}
219219

220220
std::string http_utils::standardize_url(const std::string& url) {
221+
if (url.empty()) return url;
222+
221223
std::string result = url;
222224

223225
auto new_end = std::unique(result.begin(), result.end(), [](char a, char b) { return (a == b) && (a == '/'); });

src/httpserver/http_request.hpp

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -93,10 +93,7 @@ class http_request {
9393
* @return a vector of strings containing all pieces
9494
**/
9595
const std::vector<std::string> get_path_pieces() const {
96-
if (!cache->path_pieces_cached) {
97-
cache->path_pieces = http::http_utils::tokenize_url(path);
98-
cache->path_pieces_cached = true;
99-
}
96+
ensure_path_pieces_cached();
10097
return cache->path_pieces;
10198
}
10299

@@ -106,10 +103,7 @@ class http_request {
106103
* @return the selected piece in form of string
107104
**/
108105
const std::string get_path_piece(int index) const {
109-
if (!cache->path_pieces_cached) {
110-
cache->path_pieces = http::http_utils::tokenize_url(path);
111-
cache->path_pieces_cached = true;
112-
}
106+
ensure_path_pieces_cached();
113107
if (static_cast<int>(cache->path_pieces.size()) > index) {
114108
return cache->path_pieces[index];
115109
}
@@ -453,6 +447,13 @@ class http_request {
453447
bool path_pieces_cached = false;
454448
};
455449
std::unique_ptr<http_request_data_cache> cache = std::make_unique<http_request_data_cache>();
450+
void ensure_path_pieces_cached() const {
451+
if (!cache->path_pieces_cached) {
452+
cache->path_pieces = http::http_utils::tokenize_url(path);
453+
cache->path_pieces_cached = true;
454+
}
455+
}
456+
456457
// Populate the data cache unescaped_args
457458
void populate_args() const;
458459

src/webserver.cpp

Lines changed: 20 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -758,21 +758,15 @@ MHD_Result webserver::finalize_answer(MHD_Connection* connection, struct details
758758
details::http_endpoint endpoint(st_url, false, false, false);
759759

760760
// Check the LRU route cache first
761+
const details::http_endpoint* matched_ep = nullptr;
761762
{
762763
std::lock_guard<std::mutex> cache_lock(route_cache_mutex);
763764
auto cache_it = route_cache_map.find(mr->standardized_url);
764765
if (cache_it != route_cache_map.end()) {
765766
// Cache hit — move to front of LRU list
766767
route_cache_list.splice(route_cache_list.begin(), route_cache_list, cache_it->second);
767768
const route_cache_entry& cached = cache_it->second->second;
768-
769-
vector<string> url_pars = cached.matched_endpoint.get_url_pars();
770-
vector<string> url_pieces = endpoint.get_url_pieces();
771-
vector<int> chunks = cached.matched_endpoint.get_chunk_positions();
772-
for (unsigned int i = 0; i < url_pars.size(); i++) {
773-
mr->dhr->set_arg(url_pars[i], url_pieces[chunks[i]]);
774-
}
775-
769+
matched_ep = &cached.matched_endpoint;
776770
hrm = cached.resource;
777771
found = true;
778772
}
@@ -782,15 +776,13 @@ MHD_Result webserver::finalize_answer(MHD_Connection* connection, struct details
782776
// Cache miss — perform regex scan
783777
map<details::http_endpoint, http_resource*>::iterator found_endpoint;
784778

785-
map<details::http_endpoint, http_resource*>::iterator it;
786-
787779
size_t len = 0;
788780
size_t tot_len = 0;
789-
for (it = registered_resources_regex.begin(); it != registered_resources_regex.end(); ++it) {
790-
size_t endpoint_pieces_len = (*it).first.get_url_pieces().size();
791-
size_t endpoint_tot_len = (*it).first.get_url_complete().size();
781+
for (auto it = registered_resources_regex.begin(); it != registered_resources_regex.end(); ++it) {
782+
size_t endpoint_pieces_len = it->first.get_url_pieces().size();
783+
size_t endpoint_tot_len = it->first.get_url_complete().size();
792784
if (!found || endpoint_pieces_len > len || (endpoint_pieces_len == len && endpoint_tot_len > tot_len)) {
793-
if ((*it).first.match(endpoint)) {
785+
if (it->first.match(endpoint)) {
794786
found = true;
795787
len = endpoint_pieces_len;
796788
tot_len = endpoint_tot_len;
@@ -800,14 +792,7 @@ MHD_Result webserver::finalize_answer(MHD_Connection* connection, struct details
800792
}
801793

802794
if (found) {
803-
vector<string> url_pars = found_endpoint->first.get_url_pars();
804-
805-
vector<string> url_pieces = endpoint.get_url_pieces();
806-
vector<int> chunks = found_endpoint->first.get_chunk_positions();
807-
for (unsigned int i = 0; i < url_pars.size(); i++) {
808-
mr->dhr->set_arg(url_pars[i], url_pieces[chunks[i]]);
809-
}
810-
795+
matched_ep = &found_endpoint->first;
811796
hrm = found_endpoint->second;
812797

813798
// Store in LRU cache
@@ -823,6 +808,18 @@ MHD_Result webserver::finalize_answer(MHD_Connection* connection, struct details
823808
}
824809
}
825810
}
811+
812+
// Extract URL parameters from matched endpoint
813+
if (found && matched_ep != nullptr) {
814+
const auto& url_pars = matched_ep->get_url_pars();
815+
const auto& url_pieces = endpoint.get_url_pieces();
816+
const auto& chunks = matched_ep->get_chunk_positions();
817+
for (unsigned int i = 0; i < url_pars.size(); i++) {
818+
if (chunks[i] >= 0 && static_cast<size_t>(chunks[i]) < url_pieces.size()) {
819+
mr->dhr->set_arg(url_pars[i], url_pieces[chunks[i]]);
820+
}
821+
}
822+
}
826823
}
827824
} else {
828825
hrm = fe->second;
@@ -939,6 +936,7 @@ MHD_Result webserver::answer_to_connection(void* cls, MHD_Connection* connection
939936

940937
access_log(static_cast<webserver*>(cls), mr->complete_uri + " METHOD: " + method);
941938

939+
// Case-sensitive per RFC 7230 §3.1.1: HTTP method is case-sensitive.
942940
if (0 == strcmp(method, http_utils::http_method_get)) {
943941
mr->callback = &http_resource::render_GET;
944942
} else if (0 == strcmp(method, http_utils::http_method_post)) {

0 commit comments

Comments
 (0)