Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions include/heidi-kernel/http.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,13 @@ class HttpServer {
void register_handler(std::string_view path, RequestHandler handler);
void serve_forever();

private:
void handle_client(int client_fd);
// Exposed for testing
HttpRequest parse_request(const std::string& data) const;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The HttpRequest struct uses std::string_view for its method, path, and body members, which means it does not own the underlying string data. By making parse_request public, you are exposing an API where the returned HttpRequest object contains pointers into the data string passed as an argument.

If a caller passes a temporary string (e.g., server.parse_request(std::string(buffer))) or if the source string is destroyed while the HttpRequest is still in use, these members will become dangling pointers. This leads to a Use-After-Free (UAF) vulnerability, which can cause crashes or memory corruption.

To fix this, consider changing the HttpRequest struct members to std::string so they own their data, or clearly document the lifetime requirements if performance is critical.

std::string format_response(const HttpResponse& resp) const;

private:
void handle_client(int client_fd);

std::string address_;
uint16_t port_;
int server_fd_ = -1;
Expand Down
17 changes: 15 additions & 2 deletions src/dashd/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,9 +1,22 @@
# Library for HTTP server (shared between daemon and tests)
add_library(heidi-dashd-lib STATIC
http.cpp
)

target_link_libraries(heidi-dashd-lib PRIVATE heidi-kernel-lib pthread)

target_include_directories(heidi-dashd-lib PUBLIC
${CMAKE_CURRENT_SOURCE_DIR}/../../include
)

target_compile_options(heidi-dashd-lib PRIVATE -Wall -Wextra -Wpedantic)

# Daemon executable
add_executable(heidi-dashd
main.cpp
http.cpp
)

target_link_libraries(heidi-dashd PRIVATE heidi-kernel-lib pthread)
target_link_libraries(heidi-dashd PRIVATE heidi-dashd-lib heidi-kernel-lib pthread)

target_include_directories(heidi-dashd PRIVATE
${CMAKE_CURRENT_SOURCE_DIR}/../../include
Expand Down
2 changes: 2 additions & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ add_executable(unit_tests
test_ipc.cpp
test_metrics.cpp
test_job.cpp
http_test.cpp
../src/config.cpp
)

Expand All @@ -27,6 +28,7 @@ target_link_libraries(unit_tests
heidi-ipc
heidi-metrics
heidi-kernel-job
heidi-dashd-lib
)

include(GoogleTest)
Expand Down
104 changes: 104 additions & 0 deletions tests/http_test.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
#include "heidi-kernel/http.h"
#include <gtest/gtest.h>

using namespace heidi;

// Test fixture that creates an HttpServer instance
class HttpServerTest : public ::testing::Test {
protected:
HttpServer server{"127.0.0.1", 0}; // Port 0 = let OS assign
};

TEST_F(HttpServerTest, ParseRequest_ValidGet) {
std::string data = "GET /api/status HTTP/1.1\r\nHost: localhost\r\n\r\n";
HttpRequest req = server.parse_request(data);

EXPECT_EQ(std::string(req.method), "GET");
EXPECT_EQ(std::string(req.path), "/api/status");
EXPECT_TRUE(req.body.empty());
}

TEST_F(HttpServerTest, ParseRequest_ValidPostWithBody) {
std::string data = "POST /submit HTTP/1.1\r\nContent-Length: 5\r\n\r\nhello";
HttpRequest req = server.parse_request(data);

EXPECT_EQ(std::string(req.method), "POST");
EXPECT_EQ(std::string(req.path), "/submit");
EXPECT_EQ(std::string(req.body), "hello");
}

TEST_F(HttpServerTest, ParseRequest_Incomplete) {
std::string data = "GET /"; // Missing CRLFs
HttpRequest req = server.parse_request(data);

EXPECT_TRUE(req.method.empty());
EXPECT_TRUE(req.path.empty());
EXPECT_TRUE(req.body.empty());
}

TEST_F(HttpServerTest, ParseRequest_Empty) {
std::string data = "";
HttpRequest req = server.parse_request(data);

EXPECT_TRUE(req.method.empty());
EXPECT_TRUE(req.path.empty());
}

TEST_F(HttpServerTest, FormatResponse_Simple) {
HttpResponse resp;
resp.status_code = 200;
resp.status_text = "OK";
resp.body = "hello";
resp.headers["Content-Type"] = "text/plain";

std::string s = server.format_response(resp);

EXPECT_NE(s.find("HTTP/1.1 200 OK\r\n"), std::string::npos);
EXPECT_NE(s.find("Content-Type: text/plain\r\n"), std::string::npos);
EXPECT_NE(s.find("Content-Length: 5\r\n"), std::string::npos);
// Check end of headers and body
EXPECT_NE(s.find("\r\n\r\nhello"), std::string::npos);
Comment on lines +56 to +60
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current assertions using s.find(...) are not strict enough, as they only check for the presence of substrings and not their order or the overall structure of the HTTP response. This could allow formatting errors to go unnoticed. It's better to assert against the exact expected response string for a more robust test.

Suggested change
EXPECT_NE(s.find("HTTP/1.1 200 OK\r\n"), std::string::npos);
EXPECT_NE(s.find("Content-Type: text/plain\r\n"), std::string::npos);
EXPECT_NE(s.find("Content-Length: 5\r\n"), std::string::npos);
// Check end of headers and body
EXPECT_NE(s.find("\r\n\r\nhello"), std::string::npos);
EXPECT_EQ(s, "HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nContent-Length: 5\r\n\r\nhello");

}

TEST_F(HttpServerTest, FormatResponse_404) {
HttpResponse resp;
resp.status_code = 404;
resp.status_text = "Not Found";

std::string s = server.format_response(resp);

EXPECT_NE(s.find("HTTP/1.1 404 Not Found\r\n"), std::string::npos);
EXPECT_NE(s.find("Content-Length: 0\r\n"), std::string::npos);
Comment on lines +70 to +71
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to the other response formatting tests, using s.find(...) is not very strict. Asserting against the exact expected string ensures the response is perfectly formatted and makes the test more robust against future changes.

Suggested change
EXPECT_NE(s.find("HTTP/1.1 404 Not Found\r\n"), std::string::npos);
EXPECT_NE(s.find("Content-Length: 0\r\n"), std::string::npos);
EXPECT_EQ(s, "HTTP/1.1 404 Not Found\r\nContent-Length: 0\r\n\r\n");

}

TEST_F(HttpServerTest, FormatResponse_WithHeaders) {
HttpResponse resp;
resp.status_code = 201;
resp.status_text = "Created";
resp.body = "{\"id\": 123}";
resp.headers["Content-Type"] = "application/json";
resp.headers["Location"] = "/items/123";

std::string s = server.format_response(resp);

EXPECT_NE(s.find("HTTP/1.1 201 Created\r\n"), std::string::npos);
EXPECT_NE(s.find("Content-Type: application/json\r\n"), std::string::npos);
EXPECT_NE(s.find("Location: /items/123\r\n"), std::string::npos);
EXPECT_NE(s.find("\r\n\r\n{\"id\": 123}"), std::string::npos);
Comment on lines +84 to +87
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

These checks are not very strict. They don't verify the order of headers or the exact structure of the response. Also, the automatically added Content-Length header is not being checked. A single, exact match assertion would be more robust. Since resp.headers is a std::map, header order is predictable ("Content-Type" then "Location").

Suggested change
EXPECT_NE(s.find("HTTP/1.1 201 Created\r\n"), std::string::npos);
EXPECT_NE(s.find("Content-Type: application/json\r\n"), std::string::npos);
EXPECT_NE(s.find("Location: /items/123\r\n"), std::string::npos);
EXPECT_NE(s.find("\r\n\r\n{\"id\": 123}"), std::string::npos);
EXPECT_EQ(s, "HTTP/1.1 201 Created\r\nContent-Type: application/json\r\nLocation: /items/123\r\nContent-Length: 12\r\n\r\n{\"id\": 123}");

}

TEST_F(HttpServerTest, ParseRequest_PathWithQuery) {
std::string data = "GET /api/status?verbose=true HTTP/1.1\r\n\r\n";
HttpRequest req = server.parse_request(data);

EXPECT_EQ(std::string(req.method), "GET");
EXPECT_EQ(std::string(req.path), "/api/status?verbose=true");
}

TEST_F(HttpServerTest, ParseRequest_MethodCasePreserved) {
std::string data = "post /data HTTP/1.1\r\n\r\n";
HttpRequest req = server.parse_request(data);

EXPECT_EQ(std::string(req.method), "post"); // Lowercase preserved
EXPECT_EQ(std::string(req.path), "/data");
}
Loading