clients/cpp: apply audit findings — protocol-error guard + libcurl redirect clamp (bae34a7 → next)
HIGH: - H1: nlohmann::json::exception wrapped as ProtocolError at 5 sites in client.cpp via with_protocol_guard helper. Preserves the documented clawdforge::Error catch-all base contract; nlohmann types never leak into the message (e.what() only). - H2: libcurl MAXREDIRS=5, REDIR_PROTOCOLS_STR="http,https" (CURLOPT_REDIR_PROTOCOLS bitmask fallback for libcurl < 7.85.0), UNRESTRICTED_AUTH=0L. Defense-in-depth on top of libcurl's automatic bearer strip on cross-host redirects (>=7.64.0). MEDIUM: - M1: upload_file resolves the path via std::filesystem::canonical up front. Closes broken-symlink, symlink-loop, and TOCTOU-on-target classes without a doc burden on callers. - M2: README "Linking" section documents the public-ABI nlohmann_json implication. v0.2 wrapper deferred. - M3: README "Threat model" section documents the parse-depth concern on the result field of /run replies. Runtime guard skipped for v0.1 per audit recommendation (low yield, complexity). LOW: - L1: cxx_std_20 → cxx_std_17 in CMakeLists.txt (no C++20-only features in the library source; broader downstream reach). Examples and tests still build via designated initializers (g++ accepts these in C++17 mode). - L2: RunResult struct doc clarifies that missing ok/duration_ms decode to defaults — opt-out forward-compat. - L3: Client class doc clarifies that moved-from instances must not have any non-special-member methods invoked (UB), with explicit callout on base_url() returning an internal reference. Test-only: - cpp-httplib 0.15.3 → 0.20.1. Optional backends (OpenSSL / zlib / brotli / zstd) forced off to keep the dep graph minimal. Test-only, never on the consumer wire path. README "Test deps" section added for transparency. Tests added (12 → 23 cases, 70 → 106 assertions): - protocol_error on malformed response for healthz, run, upload_file, create_token, list_tokens (H1 regression) - redirect_clamp_test (H2 regression — TransportError after 5+ hops) - redirect_protocol_clamp (H2 regression — ftp:// Location rejected) - upload_file_canonicalize: symlink→file works, broken symlink rejected, symlink loop rejected, directory rejected (M1 regression) Verified: - cmake --build build clean (-Wall -Wextra -Wpedantic -Wshadow -Wconversion -Wsign-conversion -Wold-style-cast -Werror) - ctest --output-on-failure all green (Release) - ASan + UBSan: 23/23 cases, 106/106 assertions, zero diagnostics Audit: memory/clawdforge-audits/cpp-bae34a7.md
This commit is contained in:
parent
3c77ef523e
commit
19fe299b3d
7 changed files with 336 additions and 18 deletions
|
|
@ -333,3 +333,211 @@ TEST_CASE("invalid base_url scheme is rejected at construction") {
|
|||
CHECK_THROWS_AS(make_ftp(), cf::ProtocolError);
|
||||
CHECK_THROWS_AS(make_empty(), cf::ProtocolError);
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Regression: malformed JSON bodies must surface as ProtocolError, NOT as
|
||||
// raw nlohmann::json::exception (which would bypass the documented
|
||||
// `clawdforge::Error` catch-all base — H1 fix).
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
TEST_CASE("healthz: malformed response surfaces as ProtocolError") {
|
||||
MockServer mock{[](httplib::Server& s) {
|
||||
s.Get("/healthz", [](const httplib::Request&, httplib::Response& res) {
|
||||
// Well-formed JSON, but wrong shape — `claude_version` is a
|
||||
// number, not a string. Triggers nlohmann type_error during
|
||||
// from_json, which must be wrapped.
|
||||
res.set_content(R"({"ok":true,"claude_version":42})", "application/json");
|
||||
});
|
||||
}};
|
||||
cf::Client c{cf::ClientOptions{.base_url = mock.base_url(), .token = "cf_test"}};
|
||||
CHECK_THROWS_AS(c.healthz(), cf::ProtocolError);
|
||||
// Sanity: the catch-all base must also catch.
|
||||
CHECK_THROWS_AS(c.healthz(), cf::Error);
|
||||
}
|
||||
|
||||
TEST_CASE("run: malformed response surfaces as ProtocolError") {
|
||||
MockServer mock{[](httplib::Server& s) {
|
||||
s.Post("/run", [](const httplib::Request&, httplib::Response& res) {
|
||||
// Missing required `result` field — `from_json(RunResult)` calls
|
||||
// `j.at("result")` which throws out_of_range.
|
||||
res.set_content(R"({"ok":true,"duration_ms":1})", "application/json");
|
||||
});
|
||||
}};
|
||||
cf::Client c{cf::ClientOptions{.base_url = mock.base_url(), .token = "cf_test"}};
|
||||
CHECK_THROWS_AS(c.run(cf::RunRequest{.prompt = "hi"}), cf::ProtocolError);
|
||||
}
|
||||
|
||||
TEST_CASE("upload_file: malformed response surfaces as ProtocolError") {
|
||||
const auto path = write_temp_file("payload");
|
||||
|
||||
MockServer mock{[](httplib::Server& s) {
|
||||
s.Post("/files", [](const httplib::Request&, httplib::Response& res) {
|
||||
// Missing required `file_token`.
|
||||
res.set_content(R"({"ttl_secs":120,"size":7})", "application/json");
|
||||
});
|
||||
}};
|
||||
cf::Client c{cf::ClientOptions{.base_url = mock.base_url(), .token = "cf_test"}};
|
||||
CHECK_THROWS_AS(c.upload_file(path), cf::ProtocolError);
|
||||
|
||||
std::filesystem::remove(path);
|
||||
}
|
||||
|
||||
TEST_CASE("create_token: malformed response surfaces as ProtocolError") {
|
||||
MockServer mock{[](httplib::Server& s) {
|
||||
s.Post("/admin/tokens", [](const httplib::Request&, httplib::Response& res) {
|
||||
// Missing required `name` + `token`.
|
||||
res.set_content(R"({"foo":"bar"})", "application/json");
|
||||
});
|
||||
}};
|
||||
cf::Client c{cf::ClientOptions{.base_url = mock.base_url(),
|
||||
.token = "cf_test",
|
||||
.admin_token = "admin_secret"}};
|
||||
CHECK_THROWS_AS(
|
||||
c.create_token(cf::TokenCreateRequest{.name = "x"}),
|
||||
cf::ProtocolError);
|
||||
}
|
||||
|
||||
TEST_CASE("list_tokens: malformed entries surface as ProtocolError") {
|
||||
MockServer mock{[](httplib::Server& s) {
|
||||
s.Get("/admin/tokens", [](const httplib::Request&, httplib::Response& res) {
|
||||
// Array is present, but entries are missing `name` (required).
|
||||
res.set_content(R"({"tokens":[{"ip_cidrs":["10.0.0.0/8"]}]})",
|
||||
"application/json");
|
||||
});
|
||||
}};
|
||||
cf::Client c{cf::ClientOptions{.base_url = mock.base_url(),
|
||||
.token = "cf_test",
|
||||
.admin_token = "admin_secret"}};
|
||||
CHECK_THROWS_AS(c.list_tokens(), cf::ProtocolError);
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Regression: libcurl redirect clamps (H2). Verify MAXREDIRS=5 caps the chain
|
||||
// and REDIR_PROTOCOLS rejects ftp:// targets.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
TEST_CASE("redirect_clamp: more than 5 redirects -> TransportError") {
|
||||
std::atomic<int> hops{0};
|
||||
MockServer mock{[&](httplib::Server& s) {
|
||||
s.Get("/healthz", [](const httplib::Request&, httplib::Response& res) {
|
||||
res.status = 302;
|
||||
res.set_header("Location", "/loop0");
|
||||
});
|
||||
s.Get(R"(/loop(\d+))", [&](const httplib::Request& req, httplib::Response& res) {
|
||||
hops.fetch_add(1, std::memory_order_relaxed);
|
||||
const int n = std::stoi(req.matches[1]);
|
||||
res.status = 302;
|
||||
res.set_header("Location", "/loop" + std::to_string(n + 1));
|
||||
});
|
||||
}};
|
||||
|
||||
cf::Client c{cf::ClientOptions{
|
||||
.base_url = mock.base_url(),
|
||||
.token = "cf_test",
|
||||
.timeout = std::chrono::seconds{5},
|
||||
.connect_timeout = std::chrono::seconds{2},
|
||||
}};
|
||||
CHECK_THROWS_AS(c.healthz(), cf::TransportError);
|
||||
// libcurl follows up to MAXREDIRS+1 hops total before giving up.
|
||||
CHECK(hops.load() <= 6);
|
||||
}
|
||||
|
||||
TEST_CASE("redirect_protocol_clamp: ftp:// Location is rejected") {
|
||||
MockServer mock{[](httplib::Server& s) {
|
||||
s.Get("/healthz", [](const httplib::Request&, httplib::Response& res) {
|
||||
res.status = 302;
|
||||
res.set_header("Location", "ftp://example.invalid/data");
|
||||
});
|
||||
}};
|
||||
|
||||
cf::Client c{cf::ClientOptions{
|
||||
.base_url = mock.base_url(),
|
||||
.token = "cf_test",
|
||||
.timeout = std::chrono::seconds{5},
|
||||
.connect_timeout = std::chrono::seconds{2},
|
||||
}};
|
||||
CHECK_THROWS_AS(c.healthz(), cf::TransportError);
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Regression: upload_file canonicalizes its path argument (M1). A symlink to
|
||||
// a real file must work; broken / loop symlinks must be rejected up front
|
||||
// with ProtocolError, never reach libcurl.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
TEST_CASE("upload_file: symlink to regular file is accepted") {
|
||||
namespace fs = std::filesystem;
|
||||
const auto target = write_temp_file("symlinked-payload");
|
||||
const auto link = fs::temp_directory_path() /
|
||||
("clawdforge-link-" +
|
||||
std::to_string(std::chrono::steady_clock::now().time_since_epoch().count()));
|
||||
std::error_code ec;
|
||||
fs::create_symlink(target, link, ec);
|
||||
REQUIRE(!ec);
|
||||
|
||||
MockServer mock{[](httplib::Server& s) {
|
||||
s.Post("/files", [](const httplib::Request& req, httplib::Response& res) {
|
||||
REQUIRE(req.has_file("file"));
|
||||
CHECK(req.get_file_value("file").content == "symlinked-payload");
|
||||
res.set_content(
|
||||
R"({"file_token":"ff_link","ttl_secs":120,"size":17})",
|
||||
"application/json");
|
||||
});
|
||||
}};
|
||||
|
||||
cf::Client c{cf::ClientOptions{.base_url = mock.base_url(), .token = "cf_test"}};
|
||||
const auto ft = c.upload_file(link.string(), 120);
|
||||
CHECK(ft.file_token == "ff_link");
|
||||
|
||||
fs::remove(link);
|
||||
fs::remove(target);
|
||||
}
|
||||
|
||||
TEST_CASE("upload_file: broken symlink is rejected with ProtocolError") {
|
||||
namespace fs = std::filesystem;
|
||||
const auto link = fs::temp_directory_path() /
|
||||
("clawdforge-broken-" +
|
||||
std::to_string(std::chrono::steady_clock::now().time_since_epoch().count()));
|
||||
std::error_code ec;
|
||||
fs::create_symlink("/no/such/path/clawdforge-target", link, ec);
|
||||
REQUIRE(!ec);
|
||||
|
||||
cf::Client c{cf::ClientOptions{.base_url = "http://127.0.0.1:1", .token = "cf_test"}};
|
||||
CHECK_THROWS_AS(c.upload_file(link.string()), cf::ProtocolError);
|
||||
|
||||
fs::remove(link);
|
||||
}
|
||||
|
||||
TEST_CASE("upload_file: symlink loop is rejected with ProtocolError") {
|
||||
namespace fs = std::filesystem;
|
||||
const auto stamp =
|
||||
std::to_string(std::chrono::steady_clock::now().time_since_epoch().count());
|
||||
const auto a = fs::temp_directory_path() / ("clawdforge-loopA-" + stamp);
|
||||
const auto b = fs::temp_directory_path() / ("clawdforge-loopB-" + stamp);
|
||||
std::error_code ec;
|
||||
fs::create_symlink(b, a, ec);
|
||||
REQUIRE(!ec);
|
||||
fs::create_symlink(a, b, ec);
|
||||
REQUIRE(!ec);
|
||||
|
||||
cf::Client c{cf::ClientOptions{.base_url = "http://127.0.0.1:1", .token = "cf_test"}};
|
||||
CHECK_THROWS_AS(c.upload_file(a.string()), cf::ProtocolError);
|
||||
|
||||
fs::remove(a);
|
||||
fs::remove(b);
|
||||
}
|
||||
|
||||
TEST_CASE("upload_file: directory is rejected") {
|
||||
namespace fs = std::filesystem;
|
||||
const auto dir = fs::temp_directory_path() /
|
||||
("clawdforge-dir-" +
|
||||
std::to_string(std::chrono::steady_clock::now().time_since_epoch().count()));
|
||||
std::error_code ec;
|
||||
fs::create_directory(dir, ec);
|
||||
REQUIRE(!ec);
|
||||
|
||||
cf::Client c{cf::ClientOptions{.base_url = "http://127.0.0.1:1", .token = "cf_test"}};
|
||||
CHECK_THROWS_AS(c.upload_file(dir.string()), cf::ProtocolError);
|
||||
|
||||
fs::remove(dir);
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue