From 025eab2f7245f1f685d11d8252dd645192143755 Mon Sep 17 00:00:00 2001 From: tx3stn <14163530+tx3stn@users.noreply.github.com> Date: Sun, 16 Aug 2026 00:01:45 +0100 Subject: [PATCH 01/18] test: add example cli app that makes http request to show failure --- apps/cli_http_example/BUILD.bazel | 21 +++++++++++++++ apps/cli_http_example/index.ts | 45 +++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+) create mode 100644 apps/cli_http_example/BUILD.bazel create mode 100644 apps/cli_http_example/index.ts diff --git a/apps/cli_http_example/BUILD.bazel b/apps/cli_http_example/BUILD.bazel new file mode 100644 index 000000000..563092f89 --- /dev/null +++ b/apps/cli_http_example/BUILD.bazel @@ -0,0 +1,21 @@ +load("//bzl/valdi:valdi_cli_application.bzl", "valdi_cli_application") +load("//bzl/valdi:valdi_module.bzl", "valdi_module") + +valdi_module( + name = "cli_http_example", + srcs = glob([ + "**/*.ts", + ]), + visibility = ["//visibility:public"], + deps = [ + "//src/valdi_modules/src/valdi/valdi_core", + "//src/valdi_modules/src/valdi/valdi_http", + "//src/valdi_modules/src/valdi/valdi_standalone", + ], +) + +valdi_cli_application( + name = "cli_http_example_app", + script_path = "cli_http_example/index", + deps = [":cli_http_example"], +) diff --git a/apps/cli_http_example/index.ts b/apps/cli_http_example/index.ts new file mode 100644 index 000000000..e6c9f6529 --- /dev/null +++ b/apps/cli_http_example/index.ts @@ -0,0 +1,45 @@ +import { + beginKeepAlive, + endKeepAlive, +} from "valdi_core/src/utils/KeepAliveCallback"; +import { HTTPClient } from "valdi_http/src/HTTPClient"; +import { ArgumentsParser } from "valdi_standalone/src/ArgumentsParser"; +import { getStandaloneRuntime } from "valdi_standalone/src/ValdiStandalone"; + +const DEFAULT_URL = "https://example.com"; + +const standalone = getStandaloneRuntime(); + +const programArguments = standalone.arguments.slice(); +programArguments.shift(); + +const parser = new ArgumentsParser("cli_http_example", [ + "_", + ...programArguments, +]); +const urlArgument = parser.addString( + "--url", + `URL to fetch (default ${DEFAULT_URL})`, + false, +); +parser.parse(); + +const url = urlArgument.value ?? DEFAULT_URL; + +const keepAlive = beginKeepAlive(); + +console.info(`GET ${url}`); + +new HTTPClient().get(url).then( + (response) => { + const length = response.body ? response.body.byteLength : 0; + console.info(`OK: status ${response.statusCode}, ${length} bytes`); + endKeepAlive(keepAlive); + standalone.exit(0); + }, + (error) => { + console.error(`FAIL: request rejected: ${error}`); + endKeepAlive(keepAlive); + standalone.exit(1); + }, +); From b8b68f7aa94ef0166bcea290d9925acf07331067 Mon Sep 17 00:00:00 2001 From: tx3stn <14163530+tx3stn@users.noreply.github.com> Date: Sun, 16 Aug 2026 00:25:13 +0100 Subject: [PATCH 02/18] fix: add request manager to standalone --- valdi/BUILD.bazel | 15 ++++ valdi/src/valdi/cli_runner/CLIRunner.cpp | 6 +- valdi/src/valdi/cli_runner/CLIRunner.hpp | 13 +++- .../ValdiStandaloneMain.cpp | 4 + .../ValdiStandaloneMain.hpp | 2 + .../StandaloneRequestManager_tests.cpp | 76 +++++++++++++++++++ 6 files changed, 114 insertions(+), 2 deletions(-) create mode 100644 valdi/test/integration/StandaloneRequestManager_tests.cpp diff --git a/valdi/BUILD.bazel b/valdi/BUILD.bazel index c33fc3933..860c20e26 100644 --- a/valdi/BUILD.bazel +++ b/valdi/BUILD.bazel @@ -974,6 +974,21 @@ valdi_test( ], ) +valdi_test( + name = "test_standalone", + srcs = [ + "test/integration/JSBridgeTestFixture.cpp", + "test/integration/StandaloneRequestManager_tests.cpp", + ], + hdrs = glob(["test/integration/**/*.hpp"]), + deps = [ + ":test_utils", + ":valdi_runtime_with_vm", + "//src/valdi_modules/src/cpp/valdi_http:valdi_http_cpp", + "//src/valdi_modules/src/valdi/valdi_core:valdi_core_native", + ], +) + valdi_test( name = "test_hermes", srcs = glob(["test/hermes/**/*.cpp"]), diff --git a/valdi/src/valdi/cli_runner/CLIRunner.cpp b/valdi/src/valdi/cli_runner/CLIRunner.cpp index 4201115df..d0ac64ce6 100644 --- a/valdi/src/valdi/cli_runner/CLIRunner.cpp +++ b/valdi/src/valdi/cli_runner/CLIRunner.cpp @@ -7,12 +7,16 @@ namespace Valdi { -int valdiCLIRun(const char* scriptPath, int argc, const char** argv) { +int valdiCLIRun(const char* scriptPath, + int argc, + const char** argv, + const std::shared_ptr& requestManager) { SignalHandler::install(); StandaloneArguments standaloneArguments; standaloneArguments.scriptPath = StringBox::fromCString(scriptPath); standaloneArguments.enableHotReloader = false; + standaloneArguments.requestManager = requestManager; if constexpr (snap::kIsDevBuild) { standaloneArguments.enableDebuggerService = true; diff --git a/valdi/src/valdi/cli_runner/CLIRunner.hpp b/valdi/src/valdi/cli_runner/CLIRunner.hpp index 09bbe0429..1b7e772ce 100644 --- a/valdi/src/valdi/cli_runner/CLIRunner.hpp +++ b/valdi/src/valdi/cli_runner/CLIRunner.hpp @@ -1,5 +1,16 @@ +#pragma once + +#include + +namespace snap::valdi_core { +class HTTPRequestManager; +} + namespace Valdi { -int valdiCLIRun(const char* scriptPath, int argc, const char** argv); +int valdiCLIRun(const char* scriptPath, + int argc, + const char** argv, + const std::shared_ptr& requestManager = nullptr); } diff --git a/valdi/src/valdi/standalone_runtime/ValdiStandaloneMain.cpp b/valdi/src/valdi/standalone_runtime/ValdiStandaloneMain.cpp index eb583c6bf..e82e8ce45 100644 --- a/valdi/src/valdi/standalone_runtime/ValdiStandaloneMain.cpp +++ b/valdi/src/valdi/standalone_runtime/ValdiStandaloneMain.cpp @@ -80,6 +80,10 @@ Ref createValdiStandaloneRuntime(const StandaloneArgumen runtime->getRuntimeManager().registerModuleFactoriesProvider(moduleFactoriesProvider); } + if (arguments.requestManager != nullptr) { + runtime->getRuntimeManager().setRequestManager(arguments.requestManager); + } + return runtime; } diff --git a/valdi/src/valdi/standalone_runtime/ValdiStandaloneMain.hpp b/valdi/src/valdi/standalone_runtime/ValdiStandaloneMain.hpp index ae74c99fc..75b322f40 100644 --- a/valdi/src/valdi/standalone_runtime/ValdiStandaloneMain.hpp +++ b/valdi/src/valdi/standalone_runtime/ValdiStandaloneMain.hpp @@ -13,6 +13,7 @@ #include namespace snap::valdi_core { +class HTTPRequestManager; class ModuleFactoriesProvider; } @@ -30,6 +31,7 @@ struct StandaloneArguments { LogType logLevel = LogTypeInfo; IJavaScriptBridge* jsBridge = nullptr; std::vector> moduleFactoriesProviders; + std::shared_ptr requestManager = nullptr; bool enableDebuggerService = false; bool enableHotReloader = false; bool enableTSN = false; diff --git a/valdi/test/integration/StandaloneRequestManager_tests.cpp b/valdi/test/integration/StandaloneRequestManager_tests.cpp new file mode 100644 index 000000000..171ddbeeb --- /dev/null +++ b/valdi/test/integration/StandaloneRequestManager_tests.cpp @@ -0,0 +1,76 @@ +#include "RequestManagerMock.hpp" +#include "valdi/runtime/Runtime.hpp" +#include "valdi/standalone_runtime/ValdiStandaloneMain.hpp" +#include "valdi/standalone_runtime/ValdiStandaloneRuntime.hpp" +#include "valdi_core/cpp/Utils/ByteBuffer.hpp" +#include "valdi_core/cpp/Utils/ConsoleLogger.hpp" + +#include "JSBridgeTestFixture.hpp" +#include "gtest/gtest.h" + +using namespace Valdi; + +namespace ValdiTest { + +// A CLI app reaches valdi_http through createValdiStandaloneRuntime, which had no way to install a +// request manager, so performRequest always failed with "No RequestManager set". +class StandaloneRequestManagerFixture : public JSBridgeTestFixture { +protected: + StandaloneArguments makeArguments() { + StandaloneArguments arguments; + arguments.jsBridge = getJsBridge(); + arguments.logLevel = LogTypeError; + return arguments; + } +}; + +TEST_P(StandaloneRequestManagerFixture, isNullWhenNotSupplied) { + auto standaloneRuntime = createValdiStandaloneRuntime(makeArguments()); + + ASSERT_EQ(standaloneRuntime->getRuntime().getRequestManager(), nullptr); +} + +TEST_P(StandaloneRequestManagerFixture, reachesTheRuntimeWhenSupplied) { + auto requestManager = Valdi::makeShared(ConsoleLogger::getLogger()); + + auto arguments = makeArguments(); + arguments.requestManager = requestManager; + + auto standaloneRuntime = createValdiStandaloneRuntime(arguments); + + ASSERT_EQ(standaloneRuntime->getRuntime().getRequestManager(), requestManager); +} + +// The end-to-end shape a CLI app exercises: performRequest resolves instead of throwing. +TEST_P(StandaloneRequestManagerFixture, performRequestSucceedsFromJavaScript) { + auto requestManager = Valdi::makeShared(ConsoleLogger::getLogger()); + requestManager->addMockedResponse(STRING_LITERAL("http://localhost/"), STRING_LITERAL("GET"), BytesView()); + + auto arguments = makeArguments(); + arguments.requestManager = requestManager; + + auto standaloneRuntime = createValdiStandaloneRuntime(arguments); + + std::string js = "var m = global.require('valdi_http/src/NativeHTTPClient');" + "try {" + " m.performRequest({ url: 'http://localhost/', method: 'GET', headers: {} }, function () {});" + " return 'ok';" + "} catch (e) {" + " return String(e);" + "}"; + + auto result = standaloneRuntime->getRuntime().getJavaScriptRuntime()->evaluateScript( + makeShared(js)->toBytesView(), STRING_LITERAL("standalone_request_manager_test.js")); + + ASSERT_TRUE(result) << result.description(); + ASSERT_EQ("ok", result.value().toString()); +} + +INSTANTIATE_TEST_SUITE_P(StandaloneRequestManagerTests, + StandaloneRequestManagerFixture, + ::testing::Values(JavaScriptEngineTestCase::Hermes, + JavaScriptEngineTestCase::QuickJS, + JavaScriptEngineTestCase::JSCore), + PrintJavaScriptEngineType()); + +} // namespace ValdiTest From 74921a31bc6d316911c469df785769f79628274a Mon Sep 17 00:00:00 2001 From: tx3stn <14163530+tx3stn@users.noreply.github.com> Date: Sun, 16 Aug 2026 01:40:57 +0100 Subject: [PATCH 03/18] fix: add curl http request manager --- MODULE.bazel | 4 + valdi/BUILD.bazel | 15 + .../CurlHTTPRequestManager.cpp | 318 ++++++++++++++++++ .../CurlHTTPRequestManager.hpp | 19 ++ .../CurlHTTPRequestManager_tests.cpp | 110 ++++++ 5 files changed, 466 insertions(+) create mode 100644 valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp create mode 100644 valdi/src/valdi/standalone_http/CurlHTTPRequestManager.hpp create mode 100644 valdi/test/integration/CurlHTTPRequestManager_tests.cpp diff --git a/MODULE.bazel b/MODULE.bazel index d338a79da..4604a3412 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -152,6 +152,10 @@ http_archive( bazel_dep(name = "boringssl", version = "0.20250415.0") +# HTTP transport for the standalone runtime. Defaults to the BoringSSL above as its TLS +# backend, and to http_only, so no other protocol is compiled in. +bazel_dep(name = "curl", version = "8.12.0") + # TODO(simon): See if we can use upstream http_archive( name = "boost", diff --git a/valdi/BUILD.bazel b/valdi/BUILD.bazel index 860c20e26..e3a589895 100644 --- a/valdi/BUILD.bazel +++ b/valdi/BUILD.bazel @@ -974,9 +974,23 @@ valdi_test( ], ) +cc_library( + name = "valdi_standalone_http", + srcs = glob(["src/valdi/standalone_http/**/*.cpp"]), + hdrs = glob(["src/valdi/standalone_http/**/*.hpp"]), + copts = CC_COMPILER_FLAGS, + strip_include_prefix = "src", + visibility = ["//visibility:public"], + deps = [ + ":valdi_runtime", + "@curl", + ], +) + valdi_test( name = "test_standalone", srcs = [ + "test/integration/CurlHTTPRequestManager_tests.cpp", "test/integration/JSBridgeTestFixture.cpp", "test/integration/StandaloneRequestManager_tests.cpp", ], @@ -984,6 +998,7 @@ valdi_test( deps = [ ":test_utils", ":valdi_runtime_with_vm", + ":valdi_standalone_http", "//src/valdi_modules/src/cpp/valdi_http:valdi_http_cpp", "//src/valdi_modules/src/valdi/valdi_core:valdi_core_native", ], diff --git a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp new file mode 100644 index 000000000..40e01abc5 --- /dev/null +++ b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp @@ -0,0 +1,318 @@ +#include "valdi/standalone_http/CurlHTTPRequestManager.hpp" + +#include "valdi_core/Cancelable.hpp" +#include "valdi_core/HTTPRequest.hpp" +#include "valdi_core/HTTPRequestManagerCompletion.hpp" +#include "valdi_core/HTTPResponse.hpp" +#include "valdi_core/cpp/Utils/ByteBuffer.hpp" +#include "valdi_core/cpp/Utils/Bytes.hpp" +#include "valdi_core/cpp/Utils/Error.hpp" +#include "valdi_core/cpp/Utils/Result.hpp" +#include "valdi_core/cpp/Utils/StringCache.hpp" +#include "valdi_core/cpp/Utils/Value.hpp" + +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace Valdi { + +namespace { + +constexpr long kMaxRedirects = 10; +constexpr long kConnectTimeoutSeconds = 30; +constexpr int kPollTimeoutMs = 200; + +const char* const kCaBundleCandidates[] = { + "/etc/ssl/cert.pem", // macOS, Alpine + "/etc/ssl/certs/ca-certificates.crt", // Debian, Ubuntu + "/etc/pki/tls/certs/ca-bundle.crt", // RHEL, Fedora + "/etc/ssl/ca-bundle.pem", // openSUSE +}; + +std::string resolveCaBundle(const StringBox& configured) { + if (!configured.isEmpty()) { + return std::string(configured.toStringView()); + } + + for (const char* candidate : kCaBundleCandidates) { + struct stat info; + if (stat(candidate, &info) == 0 && S_ISREG(info.st_mode)) { + return candidate; + } + } + + return {}; +} + +class CurlTask : public snap::valdi_core::Cancelable { +public: + CurlTask(snap::valdi_core::HTTPRequest request, + std::shared_ptr completion) + : request(std::move(request)), _completion(std::move(completion)) {} + + void cancel() override { + cancelled.store(true); + } + + void complete(const Result& result) { + std::shared_ptr completion; + { + std::lock_guard guard(_mutex); + completion = std::move(_completion); + } + + if (completion == nullptr) { + return; + } + + if (result.success()) { + completion->onComplete(result.value()); + } else { + completion->onFail(result.error().toString()); + } + } + + snap::valdi_core::HTTPRequest request; + std::atomic_bool cancelled{false}; + + std::string responseBody; + Value responseHeaders; + curl_slist* requestHeaders = nullptr; + +private: + std::mutex _mutex; + std::shared_ptr _completion; +}; + +size_t writeBodyCallback(char* data, size_t size, size_t count, void* userData) { + auto* task = static_cast(userData); + task->responseBody.append(data, size * count); + return size * count; +} + +size_t writeHeaderCallback(char* data, size_t size, size_t count, void* userData) { + auto* task = static_cast(userData); + + std::string line(data, size * count); + auto separator = line.find(':'); + if (separator != std::string::npos) { + auto name = line.substr(0, separator); + auto value = line.substr(separator + 1); + + auto isTrimmable = [](char c) { return c == ' ' || c == '\t' || c == '\r' || c == '\n'; }; + while (!value.empty() && isTrimmable(value.front())) { + value.erase(value.begin()); + } + while (!value.empty() && isTrimmable(value.back())) { + value.pop_back(); + } + + task->responseHeaders.setMapValue(std::string_view(name), Value(StringBox::fromString(value))); + } + + return size * count; +} + +int progressCallback(void* userData, curl_off_t, curl_off_t, curl_off_t, curl_off_t) { + auto* task = static_cast(userData); + return task->cancelled.load() ? 1 : 0; +} + +class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { +public: + explicit CurlHTTPRequestManager(std::string caBundle) : _caBundle(std::move(caBundle)) { + _multi = curl_multi_init(); + _thread = std::thread([this]() { run(); }); + } + + ~CurlHTTPRequestManager() override { + { + std::lock_guard guard(_mutex); + _stopping = true; + } + curl_multi_wakeup(_multi); + if (_thread.joinable()) { + _thread.join(); + } + curl_multi_cleanup(_multi); + } + + std::shared_ptr performRequest( + const snap::valdi_core::HTTPRequest& request, + const std::shared_ptr& completion) override { + auto task = std::make_shared(request, completion); + + { + std::lock_guard guard(_mutex); + _pending.push_back(task); + } + curl_multi_wakeup(_multi); + + return task; + } + +private: + void run() { + while (true) { + std::vector> pending; + { + std::lock_guard guard(_mutex); + if (_stopping && _active.empty() && _pending.empty()) { + break; + } + pending.swap(_pending); + } + + for (const auto& task : pending) { + addTask(task); + } + + int running = 0; + curl_multi_perform(_multi, &running); + + int numfds = 0; + curl_multi_poll(_multi, nullptr, 0, kPollTimeoutMs, &numfds); + + drainMessages(); + } + + for (auto& entry : _active) { + curl_multi_remove_handle(_multi, entry.first); + finish(entry.first, entry.second, Error(STRING_LITERAL("Request manager shutting down"))); + } + _active.clear(); + } + + void addTask(const std::shared_ptr& task) { + auto* easy = curl_easy_init(); + if (easy == nullptr) { + task->complete(Error(STRING_LITERAL("Failed to create a curl handle"))); + return; + } + + const auto& request = task->request; + + curl_easy_setopt(easy, CURLOPT_URL, std::string(request.url.toStringView()).c_str()); + + auto method = std::string(request.method.toStringView()); + if (method == "HEAD") { + curl_easy_setopt(easy, CURLOPT_NOBODY, 1L); + } else if (!method.empty() && method != "GET") { + curl_easy_setopt(easy, CURLOPT_CUSTOMREQUEST, method.c_str()); + } + + if (request.body) { + const auto& body = request.body.value(); + curl_easy_setopt(easy, CURLOPT_POSTFIELDSIZE_LARGE, static_cast(body.size())); + curl_easy_setopt(easy, CURLOPT_COPYPOSTFIELDS, reinterpret_cast(body.data())); + } + + for (const auto& key : request.headers.sortedMapKeys()) { + auto header = std::string(key.toStringView()) + ": " + + std::string(request.headers.getMapValue(key).toStringBox().toStringView()); + task->requestHeaders = curl_slist_append(task->requestHeaders, header.c_str()); + } + if (task->requestHeaders != nullptr) { + curl_easy_setopt(easy, CURLOPT_HTTPHEADER, task->requestHeaders); + } + + curl_easy_setopt(easy, CURLOPT_WRITEFUNCTION, writeBodyCallback); + curl_easy_setopt(easy, CURLOPT_WRITEDATA, task.get()); + curl_easy_setopt(easy, CURLOPT_HEADERFUNCTION, writeHeaderCallback); + curl_easy_setopt(easy, CURLOPT_HEADERDATA, task.get()); + curl_easy_setopt(easy, CURLOPT_XFERINFOFUNCTION, progressCallback); + curl_easy_setopt(easy, CURLOPT_XFERINFODATA, task.get()); + curl_easy_setopt(easy, CURLOPT_NOPROGRESS, 0L); + + curl_easy_setopt(easy, CURLOPT_FOLLOWLOCATION, 1L); + curl_easy_setopt(easy, CURLOPT_MAXREDIRS, kMaxRedirects); + curl_easy_setopt(easy, CURLOPT_CONNECTTIMEOUT, kConnectTimeoutSeconds); + curl_easy_setopt(easy, CURLOPT_NOSIGNAL, 1L); + + if (!_caBundle.empty()) { + curl_easy_setopt(easy, CURLOPT_CAINFO, _caBundle.c_str()); + } + + _active.emplace(easy, task); + curl_multi_add_handle(_multi, easy); + } + + void drainMessages() { + int remaining = 0; + while (auto* message = curl_multi_info_read(_multi, &remaining)) { + if (message->msg != CURLMSG_DONE) { + continue; + } + + auto* easy = message->easy_handle; + auto found = _active.find(easy); + if (found == _active.end()) { + curl_multi_remove_handle(_multi, easy); + curl_easy_cleanup(easy); + continue; + } + + auto task = found->second; + _active.erase(found); + curl_multi_remove_handle(_multi, easy); + + if (message->data.result == CURLE_OK) { + long statusCode = 0; + curl_easy_getinfo(easy, CURLINFO_RESPONSE_CODE, &statusCode); + + snap::valdi_core::HTTPResponse response(static_cast(statusCode), + task->responseHeaders, + {makeShared(task->responseBody)->toBytesView()}); + finishHandle(easy, task, Result(response)); + } else if (message->data.result == CURLE_ABORTED_BY_CALLBACK) { + finish(easy, task, Error(STRING_LITERAL("Request was cancelled"))); + } else { + finish(easy, task, Error(StringBox::fromCString(curl_easy_strerror(message->data.result)))); + } + } + } + + void finish(CURL* easy, const std::shared_ptr& task, Error&& error) { + finishHandle(easy, task, Result(std::move(error))); + } + + void finishHandle(CURL* easy, + const std::shared_ptr& task, + const Result& result) { + task->complete(result); + + if (task->requestHeaders != nullptr) { + curl_slist_free_all(task->requestHeaders); + task->requestHeaders = nullptr; + } + curl_easy_cleanup(easy); + } + + std::string _caBundle; + CURLM* _multi = nullptr; + std::thread _thread; + std::mutex _mutex; + bool _stopping = false; + std::vector> _pending; + std::unordered_map> _active; +}; + +} // namespace + +Shared makeCurlHTTPRequestManager(const StringBox& caBundlePath) { + static std::once_flag globalInit; + std::call_once(globalInit, []() { curl_global_init(CURL_GLOBAL_DEFAULT); }); + + return std::make_shared(resolveCaBundle(caBundlePath)); +} + +} // namespace Valdi diff --git a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.hpp b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.hpp new file mode 100644 index 000000000..d5fba122a --- /dev/null +++ b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.hpp @@ -0,0 +1,19 @@ +#pragma once + +#include "valdi_core/HTTPRequestManager.hpp" +#include "valdi_core/cpp/Utils/Shared.hpp" +#include "valdi_core/cpp/Utils/StringBox.hpp" + +namespace Valdi { + +/** + * An HTTPRequestManager backed by libcurl, for integrations with no platform user agent of their + * own — the standalone runtime and the CLI apps built on it. + * + * caBundlePath selects the trust store used to verify server certificates. When empty, common + * system locations are probed. If none is found, TLS requests fail rather than silently skipping + * verification. + */ +Shared makeCurlHTTPRequestManager(const StringBox& caBundlePath = StringBox()); + +} // namespace Valdi diff --git a/valdi/test/integration/CurlHTTPRequestManager_tests.cpp b/valdi/test/integration/CurlHTTPRequestManager_tests.cpp new file mode 100644 index 000000000..a402e51e0 --- /dev/null +++ b/valdi/test/integration/CurlHTTPRequestManager_tests.cpp @@ -0,0 +1,110 @@ +#include "valdi/standalone_http/CurlHTTPRequestManager.hpp" + +#include "valdi_core/Cancelable.hpp" +#include "valdi_core/HTTPRequest.hpp" +#include "valdi_core/HTTPRequestManager.hpp" +#include "valdi_core/HTTPRequestManagerCompletion.hpp" +#include "valdi_core/HTTPResponse.hpp" +#include "valdi_core/cpp/Utils/StringCache.hpp" +#include "valdi_core/cpp/Utils/Value.hpp" + +#include "gtest/gtest.h" + +#include +#include +#include +#include +#include + +using namespace Valdi; + +namespace ValdiTest { + +namespace { + +class RecordingCompletion : public snap::valdi_core::HTTPRequestManagerCompletion { +public: + void onComplete(const snap::valdi_core::HTTPResponse& response) override { + std::lock_guard guard(_mutex); + _statusCode = response.statusCode; + _bodySize = response.body ? response.body.value().size() : 0; + _done = true; + _condition.notify_all(); + } + + void onFail(const std::string& error) override { + std::lock_guard guard(_mutex); + _error = error; + _done = true; + _condition.notify_all(); + } + + bool waitForCompletion(std::chrono::milliseconds timeout) { + std::unique_lock lock(_mutex); + return _condition.wait_for(lock, timeout, [this]() { return _done; }); + } + + std::optional statusCode() const { + std::lock_guard guard(_mutex); + return _statusCode; + } + + std::optional error() const { + std::lock_guard guard(_mutex); + return _error; + } + + size_t bodySize() const { + std::lock_guard guard(_mutex); + return _bodySize; + } + +private: + mutable std::mutex _mutex; + std::condition_variable _condition; + bool _done = false; + std::optional _statusCode; + std::optional _error; + size_t _bodySize = 0; +}; + +snap::valdi_core::HTTPRequest makeGet(const char* url) { + return snap::valdi_core::HTTPRequest(StringBox::fromCString(url), STRING_LITERAL("GET"), Value(), std::nullopt, 0); +} + +} // namespace + +TEST(CurlHTTPRequestManagerTests, reportsFailureForAnUnresolvableHost) { + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + + auto cancelable = manager->performRequest(makeGet("http://this-host-does-not-exist.invalid/"), completion); + ASSERT_NE(cancelable, nullptr); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(30))); + ASSERT_FALSE(completion->statusCode().has_value()); + ASSERT_TRUE(completion->error().has_value()); +} + +TEST(CurlHTTPRequestManagerTests, returnsACancelableThatDoesNotDeadlock) { + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + + auto cancelable = manager->performRequest(makeGet("http://this-host-does-not-exist.invalid/"), completion); + ASSERT_NE(cancelable, nullptr); + cancelable->cancel(); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(30))); +} + +TEST(CurlHTTPRequestManagerTests, doesNotBlockShutdownWithRequestsInFlight) { + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + + manager->performRequest(makeGet("http://this-host-does-not-exist.invalid/"), completion); + manager.reset(); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(30))); +} + +} // namespace ValdiTest From 1f5134c4b173cb56ea5b28b80251ff9dc8bd4420 Mon Sep 17 00:00:00 2001 From: tx3stn <14163530+tx3stn@users.noreply.github.com> Date: Sun, 16 Aug 2026 12:35:42 +0100 Subject: [PATCH 04/18] fix: wire up enable_http flag and update example app to test --- apps/cli_http_example/BUILD.bazel | 1 + bzl/valdi/app_templates/cli_main.cpp.tpl | 3 ++- bzl/valdi/valdi_cli_application.bzl | 10 +++++++++- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/apps/cli_http_example/BUILD.bazel b/apps/cli_http_example/BUILD.bazel index 563092f89..2e1865e40 100644 --- a/apps/cli_http_example/BUILD.bazel +++ b/apps/cli_http_example/BUILD.bazel @@ -17,5 +17,6 @@ valdi_module( valdi_cli_application( name = "cli_http_example_app", script_path = "cli_http_example/index", + enable_http = True, deps = [":cli_http_example"], ) diff --git a/bzl/valdi/app_templates/cli_main.cpp.tpl b/bzl/valdi/app_templates/cli_main.cpp.tpl index 67a4c1c05..b6ec14a2f 100644 --- a/bzl/valdi/app_templates/cli_main.cpp.tpl +++ b/bzl/valdi/app_templates/cli_main.cpp.tpl @@ -1,5 +1,6 @@ #include "valdi/cli_runner/CLIRunner.hpp" +@VALDI_HTTP_INCLUDE@ int main(int argc, const char** argv) { - return Valdi::valdiCLIRun("@VALDI_SCRIPT_PATH@", argc, argv); + return Valdi::valdiCLIRun("@VALDI_SCRIPT_PATH@", argc, argv@VALDI_HTTP_MANAGER@); } diff --git a/bzl/valdi/valdi_cli_application.bzl b/bzl/valdi/valdi_cli_application.bzl index c8b84d441..7626f32e8 100644 --- a/bzl/valdi/valdi_cli_application.bzl +++ b/bzl/valdi/valdi_cli_application.bzl @@ -1,10 +1,15 @@ load("//bzl:expand_template.bzl", "expand_template") load("//bzl/valdi:suffixed_deps.bzl", "get_suffixed_deps") +_HTTP_INCLUDE = "#include \"valdi/standalone_http/CurlHTTPRequestManager.hpp\"" + +_HTTP_MANAGER = ", Valdi::makeCurlHTTPRequestManager()" + def valdi_cli_application( name, script_path, visibility = ["//visibility:public"], + enable_http = False, deps = []): main_target = "{}_main".format(name) @@ -14,6 +19,8 @@ def valdi_cli_application( output = "main.cpp", substitutions = { "@VALDI_SCRIPT_PATH@": script_path, + "@VALDI_HTTP_INCLUDE@": _HTTP_INCLUDE if enable_http else "", + "@VALDI_HTTP_MANAGER@": _HTTP_MANAGER if enable_http else "", }, ) @@ -26,5 +33,6 @@ def valdi_cli_application( deps = [ "@valdi//valdi:cli_runner", "@valdi//src/valdi_modules/src/valdi/valdi_core:valdi_core_native", - ] + get_suffixed_deps(deps, "_native"), + ] + (["@valdi//valdi:valdi_standalone_http"] if enable_http else []) + + get_suffixed_deps(deps, "_native"), ) From 1ed5b38375bb5cc0fc3b321492f73cfb78518ef6 Mon Sep 17 00:00:00 2001 From: tx3stn <14163530+tx3stn@users.noreply.github.com> Date: Wed, 19 Aug 2026 23:01:55 +0100 Subject: [PATCH 05/18] fix: test hardening and performance improvements --- apps/cli_http_example/index.ts | 8 +- valdi/BUILD.bazel | 11 +- .../CurlHTTPRequestManager.cpp | 218 ++++- .../CurlHTTPRequestManager.hpp | 19 +- .../CurlHTTPRequestManager_tests.cpp | 110 --- .../CurlHTTPRequestManager_tests.cpp | 881 ++++++++++++++++++ .../StandaloneRequestManager_tests.cpp | 3 +- 7 files changed, 1089 insertions(+), 161 deletions(-) delete mode 100644 valdi/test/integration/CurlHTTPRequestManager_tests.cpp create mode 100644 valdi/test/standalone/CurlHTTPRequestManager_tests.cpp rename valdi/test/{integration => standalone}/StandaloneRequestManager_tests.cpp (98%) diff --git a/apps/cli_http_example/index.ts b/apps/cli_http_example/index.ts index e6c9f6529..9d2807c84 100644 --- a/apps/cli_http_example/index.ts +++ b/apps/cli_http_example/index.ts @@ -10,13 +10,7 @@ const DEFAULT_URL = "https://example.com"; const standalone = getStandaloneRuntime(); -const programArguments = standalone.arguments.slice(); -programArguments.shift(); - -const parser = new ArgumentsParser("cli_http_example", [ - "_", - ...programArguments, -]); +const parser = new ArgumentsParser("cli_http_example", standalone.arguments); const urlArgument = parser.addString( "--url", `URL to fetch (default ${DEFAULT_URL})`, diff --git a/valdi/BUILD.bazel b/valdi/BUILD.bazel index e3a589895..f912969bb 100644 --- a/valdi/BUILD.bazel +++ b/valdi/BUILD.bazel @@ -982,19 +982,20 @@ cc_library( strip_include_prefix = "src", visibility = ["//visibility:public"], deps = [ - ":valdi_runtime", + "//valdi_core:valdi_core_cc", "@curl", ], ) valdi_test( name = "test_standalone", - srcs = [ - "test/integration/CurlHTTPRequestManager_tests.cpp", + srcs = glob(["test/standalone/**/*.cpp"]) + [ "test/integration/JSBridgeTestFixture.cpp", - "test/integration/StandaloneRequestManager_tests.cpp", ], - hdrs = glob(["test/integration/**/*.hpp"]), + hdrs = glob([ + "test/integration/**/*.hpp", + "test/standalone/**/*.hpp", + ]), deps = [ ":test_utils", ":valdi_runtime_with_vm", diff --git a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp index 40e01abc5..be614517b 100644 --- a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp +++ b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp @@ -15,6 +15,7 @@ #include #include +#include #include #include #include @@ -29,11 +30,24 @@ namespace { constexpr long kMaxRedirects = 10; constexpr long kConnectTimeoutSeconds = 30; -constexpr int kPollTimeoutMs = 200; +// An upper bound, not an interval: curl_multi_poll waits for the shorter of this and the multi +// handle's own next timer, so an active transfer still gets serviced on curl's schedule. It only +// governs how long an idle thread sits before rechecking, and new work and shutdown both wake +// the poll explicitly, so there is nothing for a short value to catch. +constexpr int kPollTimeoutMs = 10000; + +// Read by the curl command line tool, not by libcurl, so they have to be honoured here for +// a caller to be able to point at a trust store of their own. +const char* const kCaBundleVariables[] = { + "CURL_CA_BUNDLE", + "SSL_CERT_FILE", +}; +// Only distributions the @curl build defaults miss. The macOS and Debian-family paths are +// deliberately absent: @curl compiles CURL_CA_BUNDLE with exactly those two strings +// (curl+/BUILD.bazel:341-346), so libcurl already applies them, and probing for them here would +// also silently override a deliberate --@curl//:ca_bundle. const char* const kCaBundleCandidates[] = { - "/etc/ssl/cert.pem", // macOS, Alpine - "/etc/ssl/certs/ca-certificates.crt", // Debian, Ubuntu "/etc/pki/tls/certs/ca-bundle.crt", // RHEL, Fedora "/etc/ssl/ca-bundle.pem", // openSUSE }; @@ -43,6 +57,13 @@ std::string resolveCaBundle(const StringBox& configured) { return std::string(configured.toStringView()); } + for (const char* variable : kCaBundleVariables) { + const char* value = std::getenv(variable); + if (value != nullptr && *value != '\0') { + return value; + } + } + for (const char* candidate : kCaBundleCandidates) { struct stat info; if (stat(candidate, &info) == 0 && S_ISREG(info.st_mode)) { @@ -61,6 +82,18 @@ class CurlTask : public snap::valdi_core::Cancelable { void cancel() override { cancelled.store(true); + + // A cancelled request reports nothing back, matching the iOS and Android managers. + dropCompletion(); + } + + // Used at shutdown as well as for cancellation. Completions reach JavaScript directly, with no + // thread hop, so firing one from the curl thread while the thread destroying the manager is + // blocked in join() would enter the engine from two threads at once. Neither platform manager + // guarantees a completion at teardown either, so there is nothing to report. + void dropCompletion() { + std::lock_guard guard(_mutex); + _completion = nullptr; } void complete(const Result& result) { @@ -84,7 +117,11 @@ class CurlTask : public snap::valdi_core::Cancelable { snap::valdi_core::HTTPRequest request; std::atomic_bool cancelled{false}; - std::string responseBody; + // Filled by the write callback and handed straight to the response, so the payload is never + // copied. ByteBuffer grows to the next power of two, so appending stays amortised constant time + // without reserving up front — which would mean sizing an allocation from a Content-Length the + // server chose. + Ref responseBody = makeShared(); Value responseHeaders; curl_slist* requestHeaders = nullptr; @@ -95,7 +132,7 @@ class CurlTask : public snap::valdi_core::Cancelable { size_t writeBodyCallback(char* data, size_t size, size_t count, void* userData) { auto* task = static_cast(userData); - task->responseBody.append(data, size * count); + task->responseBody->append(data, data + size * count); return size * count; } @@ -103,22 +140,42 @@ size_t writeHeaderCallback(char* data, size_t size, size_t count, void* userData auto* task = static_cast(userData); std::string line(data, size * count); + + // Tested before looking for a colon, because a reason phrase is free-form text and may contain + // one. Reading a status line as a header would also skip this reset, and with + // CURLOPT_FOLLOWLOCATION the callback sees every response in the chain, so the redirect's + // headers would be left to leak into the final result. + if (line.rfind("HTTP/", 0) == 0) { + task->responseHeaders = Value(); + return size * count; + } + auto separator = line.find(':'); - if (separator != std::string::npos) { - auto name = line.substr(0, separator); - auto value = line.substr(separator + 1); + if (separator == std::string::npos) { + return size * count; + } - auto isTrimmable = [](char c) { return c == ' ' || c == '\t' || c == '\r' || c == '\n'; }; - while (!value.empty() && isTrimmable(value.front())) { - value.erase(value.begin()); - } - while (!value.empty() && isTrimmable(value.back())) { - value.pop_back(); - } + auto name = line.substr(0, separator); + auto value = line.substr(separator + 1); + + auto isTrimmable = [](char c) { return c == ' ' || c == '\t' || c == '\r' || c == '\n'; }; + while (!value.empty() && isTrimmable(value.front())) { + value.erase(value.begin()); + } + while (!value.empty() && isTrimmable(value.back())) { + value.pop_back(); + } - task->responseHeaders.setMapValue(std::string_view(name), Value(StringBox::fromString(value))); + // Joined rather than replaced, matching what NSURLResponse hands back for a repeated header. + // The response header map is string to string, so there is nowhere else for the earlier + // values to go, and dropping them loses whole Set-Cookie lines. + auto existing = task->responseHeaders.getMapValue(std::string_view(name)); + if (!existing.isNullOrUndefined()) { + value = std::string(existing.toStringBox().toStringView()) + ", " + value; } + task->responseHeaders.setMapValue(std::string_view(name), Value(StringBox::fromString(value))); + return size * count; } @@ -129,8 +186,19 @@ int progressCallback(void* userData, curl_off_t, curl_off_t, curl_off_t, curl_of class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { public: - explicit CurlHTTPRequestManager(std::string caBundle) : _caBundle(std::move(caBundle)) { + CurlHTTPRequestManager(std::string caBundle, int32_t idleTimeoutSeconds, CURLcode globalInit) + : _caBundle(std::move(caBundle)), _idleTimeoutSeconds(idleTimeoutSeconds), _globalInit(globalInit) { + if (_globalInit != CURLE_OK) { + // Going on regardless would leave curl_easy_init handing back handles whose TLS backend + // was never set up, so every HTTPS request would fail with something that looks + // unrelated. + return; + } + _multi = curl_multi_init(); + if (_multi == nullptr) { + return; + } _thread = std::thread([this]() { run(); }); } @@ -151,10 +219,35 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { const std::shared_ptr& completion) override { auto task = std::make_shared(request, completion); + // Checked before the multi handle, so the reported cause is the one that actually failed. + if (_globalInit != CURLE_OK) { + task->complete(Error(StringBox::fromString(std::string("Failed to initialise libcurl: ") + + curl_easy_strerror(_globalInit)))); + return task; + } + + if (_multi == nullptr) { + task->complete(Error(STRING_LITERAL("Failed to create a curl multi handle"))); + return task; + } + + bool stopping = false; { std::lock_guard guard(_mutex); - _pending.push_back(task); + stopping = _stopping; + if (!stopping) { + _pending.push_back(task); + } } + + // Failed outside the lock, because a completion is free to queue another request and + // _mutex is not recursive. Queueing here instead would strand the task: run() has + // already drained _pending for the last time, and nothing will service it again. + if (stopping) { + task->complete(Error(STRING_LITERAL("Request manager shutting down"))); + return task; + } + curl_multi_wakeup(_multi); return task; @@ -166,7 +259,7 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { std::vector> pending; { std::lock_guard guard(_mutex); - if (_stopping && _active.empty() && _pending.empty()) { + if (_stopping) { break; } pending.swap(_pending); @@ -179,17 +272,31 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { int running = 0; curl_multi_perform(_multi, &running); + // Before the poll, not after: perform is what queues CURLMSG_DONE, and once nothing + // is running curl has no timeout to report, so the poll would sleep out its whole + // timeout before a finished transfer was ever reported. + drainMessages(); + int numfds = 0; curl_multi_poll(_multi, nullptr, 0, kPollTimeoutMs, &numfds); - - drainMessages(); } + // Outstanding work is dropped rather than failed; see CurlTask::dropCompletion. for (auto& entry : _active) { + entry.second->dropCompletion(); curl_multi_remove_handle(_multi, entry.first); - finish(entry.first, entry.second, Error(STRING_LITERAL("Request manager shutting down"))); + releaseHandle(entry.first, entry.second); } _active.clear(); + + std::vector> pending; + { + std::lock_guard guard(_mutex); + pending.swap(_pending); + } + for (const auto& task : pending) { + task->dropCompletion(); + } } void addTask(const std::shared_ptr& task) { @@ -203,19 +310,37 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { curl_easy_setopt(easy, CURLOPT_URL, std::string(request.url.toStringView()).c_str()); - auto method = std::string(request.method.toStringView()); - if (method == "HEAD") { - curl_easy_setopt(easy, CURLOPT_NOBODY, 1L); - } else if (!method.empty() && method != "GET") { - curl_easy_setopt(easy, CURLOPT_CUSTOMREQUEST, method.c_str()); - } - if (request.body) { const auto& body = request.body.value(); curl_easy_setopt(easy, CURLOPT_POSTFIELDSIZE_LARGE, static_cast(body.size())); curl_easy_setopt(easy, CURLOPT_COPYPOSTFIELDS, reinterpret_cast(body.data())); } + // Methods curl models itself are set through their own options so that its redirect + // handling knows what the request is. CURLOPT_CUSTOMREQUEST only rewrites the request + // line and leaves behaviour alone, which is why it is reserved for the verbs curl has no + // option for. + auto method = std::string(request.method.toStringView()); + if (method == "HEAD") { + curl_easy_setopt(easy, CURLOPT_NOBODY, 1L); + } else if (method == "POST") { + curl_easy_setopt(easy, CURLOPT_POST, 1L); + if (!request.body) { + // Without fields curl reads the body from the read callback, which is stdin. + curl_easy_setopt(easy, CURLOPT_POSTFIELDSIZE_LARGE, static_cast(0)); + curl_easy_setopt(easy, CURLOPT_COPYPOSTFIELDS, ""); + } + } else if (method.empty() || method == "GET") { + if (request.body) { + // The fields above turned this into a POST; name GET to keep the request line. + curl_easy_setopt(easy, CURLOPT_CUSTOMREQUEST, "GET"); + } else { + curl_easy_setopt(easy, CURLOPT_HTTPGET, 1L); + } + } else { + curl_easy_setopt(easy, CURLOPT_CUSTOMREQUEST, method.c_str()); + } + for (const auto& key : request.headers.sortedMapKeys()) { auto header = std::string(key.toStringView()) + ": " + std::string(request.headers.getMapValue(key).toStringBox().toStringView()); @@ -238,12 +363,29 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { curl_easy_setopt(easy, CURLOPT_CONNECTTIMEOUT, kConnectTimeoutSeconds); curl_easy_setopt(easy, CURLOPT_NOSIGNAL, 1L); + // Abandoning a name lookup otherwise means joining the resolver thread, and getaddrinfo + // cannot be interrupted — so cancelling or shutting down mid-lookup blocks this thread until + // the resolver gives up, taking every other request with it. This makes curl detach that + // thread instead; it frees its own state once the lookup returns. + curl_easy_setopt(easy, CURLOPT_QUICK_EXIT, 1L); + + if (_idleTimeoutSeconds > 0) { + // An inactivity timeout, not an overall one: below one byte a second for this long + // counts as stalled. A slow but progressing download is left alone. + curl_easy_setopt(easy, CURLOPT_LOW_SPEED_LIMIT, 1L); + curl_easy_setopt(easy, CURLOPT_LOW_SPEED_TIME, static_cast(_idleTimeoutSeconds)); + } + if (!_caBundle.empty()) { curl_easy_setopt(easy, CURLOPT_CAINFO, _caBundle.c_str()); } + auto added = curl_multi_add_handle(_multi, easy); + if (added != CURLM_OK) { + finish(easy, task, Error(StringBox::fromCString(curl_multi_strerror(added)))); + return; + } _active.emplace(easy, task); - curl_multi_add_handle(_multi, easy); } void drainMessages() { @@ -271,7 +413,7 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { snap::valdi_core::HTTPResponse response(static_cast(statusCode), task->responseHeaders, - {makeShared(task->responseBody)->toBytesView()}); + {task->responseBody->toBytesView()}); finishHandle(easy, task, Result(response)); } else if (message->data.result == CURLE_ABORTED_BY_CALLBACK) { finish(easy, task, Error(STRING_LITERAL("Request was cancelled"))); @@ -289,7 +431,10 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { const std::shared_ptr& task, const Result& result) { task->complete(result); + releaseHandle(easy, task); + } + void releaseHandle(CURL* easy, const std::shared_ptr& task) { if (task->requestHeaders != nullptr) { curl_slist_free_all(task->requestHeaders); task->requestHeaders = nullptr; @@ -298,6 +443,8 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { } std::string _caBundle; + int32_t _idleTimeoutSeconds = 0; + CURLcode _globalInit = CURLE_OK; CURLM* _multi = nullptr; std::thread _thread; std::mutex _mutex; @@ -308,11 +455,14 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { } // namespace -Shared makeCurlHTTPRequestManager(const StringBox& caBundlePath) { +Shared makeCurlHTTPRequestManager(const StringBox& caBundlePath, + int32_t idleTimeoutSeconds) { + static CURLcode globalInitResult = CURLE_OK; static std::once_flag globalInit; - std::call_once(globalInit, []() { curl_global_init(CURL_GLOBAL_DEFAULT); }); + std::call_once(globalInit, []() { globalInitResult = curl_global_init(CURL_GLOBAL_DEFAULT); }); - return std::make_shared(resolveCaBundle(caBundlePath)); + return std::make_shared( + resolveCaBundle(caBundlePath), idleTimeoutSeconds, globalInitResult); } } // namespace Valdi diff --git a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.hpp b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.hpp index d5fba122a..02c9f7a99 100644 --- a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.hpp +++ b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.hpp @@ -6,14 +6,25 @@ namespace Valdi { +/** Matches NSURLSession's timeoutIntervalForRequest default. */ +constexpr int32_t kDefaultIdleTimeoutSeconds = 60; + /** * An HTTPRequestManager backed by libcurl, for integrations with no platform user agent of their * own — the standalone runtime and the CLI apps built on it. * - * caBundlePath selects the trust store used to verify server certificates. When empty, common - * system locations are probed. If none is found, TLS requests fail rather than silently skipping - * verification. + * caBundlePath selects the trust store used to verify server certificates. When empty, the + * CURL_CA_BUNDLE and SSL_CERT_FILE environment variables are consulted — libcurl does not read + * either itself — and then the few distribution paths the @curl build defaults do not name. When + * nothing here matches, libcurl falls back to the trust store compiled into it, which is what + * covers macOS and the Debian family; set --@curl//:ca_bundle to point that elsewhere. + * + * idleTimeoutSeconds fails a request that goes this long without transferring anything, so that a + * server which accepts a connection and then stalls cannot leave a caller waiting forever. It is + * deliberately an inactivity timeout rather than an overall one: a large asset download is slow + * but never idle, and a hard cap would cut it off. Zero disables it. */ -Shared makeCurlHTTPRequestManager(const StringBox& caBundlePath = StringBox()); +Shared makeCurlHTTPRequestManager( + const StringBox& caBundlePath = StringBox(), int32_t idleTimeoutSeconds = kDefaultIdleTimeoutSeconds); } // namespace Valdi diff --git a/valdi/test/integration/CurlHTTPRequestManager_tests.cpp b/valdi/test/integration/CurlHTTPRequestManager_tests.cpp deleted file mode 100644 index a402e51e0..000000000 --- a/valdi/test/integration/CurlHTTPRequestManager_tests.cpp +++ /dev/null @@ -1,110 +0,0 @@ -#include "valdi/standalone_http/CurlHTTPRequestManager.hpp" - -#include "valdi_core/Cancelable.hpp" -#include "valdi_core/HTTPRequest.hpp" -#include "valdi_core/HTTPRequestManager.hpp" -#include "valdi_core/HTTPRequestManagerCompletion.hpp" -#include "valdi_core/HTTPResponse.hpp" -#include "valdi_core/cpp/Utils/StringCache.hpp" -#include "valdi_core/cpp/Utils/Value.hpp" - -#include "gtest/gtest.h" - -#include -#include -#include -#include -#include - -using namespace Valdi; - -namespace ValdiTest { - -namespace { - -class RecordingCompletion : public snap::valdi_core::HTTPRequestManagerCompletion { -public: - void onComplete(const snap::valdi_core::HTTPResponse& response) override { - std::lock_guard guard(_mutex); - _statusCode = response.statusCode; - _bodySize = response.body ? response.body.value().size() : 0; - _done = true; - _condition.notify_all(); - } - - void onFail(const std::string& error) override { - std::lock_guard guard(_mutex); - _error = error; - _done = true; - _condition.notify_all(); - } - - bool waitForCompletion(std::chrono::milliseconds timeout) { - std::unique_lock lock(_mutex); - return _condition.wait_for(lock, timeout, [this]() { return _done; }); - } - - std::optional statusCode() const { - std::lock_guard guard(_mutex); - return _statusCode; - } - - std::optional error() const { - std::lock_guard guard(_mutex); - return _error; - } - - size_t bodySize() const { - std::lock_guard guard(_mutex); - return _bodySize; - } - -private: - mutable std::mutex _mutex; - std::condition_variable _condition; - bool _done = false; - std::optional _statusCode; - std::optional _error; - size_t _bodySize = 0; -}; - -snap::valdi_core::HTTPRequest makeGet(const char* url) { - return snap::valdi_core::HTTPRequest(StringBox::fromCString(url), STRING_LITERAL("GET"), Value(), std::nullopt, 0); -} - -} // namespace - -TEST(CurlHTTPRequestManagerTests, reportsFailureForAnUnresolvableHost) { - auto manager = makeCurlHTTPRequestManager(); - auto completion = std::make_shared(); - - auto cancelable = manager->performRequest(makeGet("http://this-host-does-not-exist.invalid/"), completion); - ASSERT_NE(cancelable, nullptr); - - ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(30))); - ASSERT_FALSE(completion->statusCode().has_value()); - ASSERT_TRUE(completion->error().has_value()); -} - -TEST(CurlHTTPRequestManagerTests, returnsACancelableThatDoesNotDeadlock) { - auto manager = makeCurlHTTPRequestManager(); - auto completion = std::make_shared(); - - auto cancelable = manager->performRequest(makeGet("http://this-host-does-not-exist.invalid/"), completion); - ASSERT_NE(cancelable, nullptr); - cancelable->cancel(); - - ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(30))); -} - -TEST(CurlHTTPRequestManagerTests, doesNotBlockShutdownWithRequestsInFlight) { - auto manager = makeCurlHTTPRequestManager(); - auto completion = std::make_shared(); - - manager->performRequest(makeGet("http://this-host-does-not-exist.invalid/"), completion); - manager.reset(); - - ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(30))); -} - -} // namespace ValdiTest diff --git a/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp b/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp new file mode 100644 index 000000000..86c3826ad --- /dev/null +++ b/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp @@ -0,0 +1,881 @@ +#include "valdi/standalone_http/CurlHTTPRequestManager.hpp" + +#include "valdi_core/Cancelable.hpp" +#include "valdi_core/HTTPRequest.hpp" +#include "valdi_core/HTTPRequestManager.hpp" +#include "valdi_core/HTTPRequestManagerCompletion.hpp" +#include "valdi_core/HTTPResponse.hpp" +#include "valdi_core/cpp/Utils/ByteBuffer.hpp" +#include "valdi_core/cpp/Utils/StringCache.hpp" +#include "valdi_core/cpp/Utils/Value.hpp" + +#include "gtest/gtest.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +using namespace Valdi; + +namespace ValdiTest { + +namespace { + +class RecordingCompletion : public snap::valdi_core::HTTPRequestManagerCompletion { +public: + void onComplete(const snap::valdi_core::HTTPResponse& response) override { + std::lock_guard guard(_mutex); + _statusCode = response.statusCode; + _headers = response.headers; + _bodySize = response.body ? response.body.value().size() : 0; + _done = true; + _condition.notify_all(); + } + + void onFail(const std::string& error) override { + std::lock_guard guard(_mutex); + _error = error; + _done = true; + _condition.notify_all(); + } + + bool waitForCompletion(std::chrono::milliseconds timeout) { + std::unique_lock lock(_mutex); + return _condition.wait_for(lock, timeout, [this]() { return _done; }); + } + + std::optional statusCode() const { + std::lock_guard guard(_mutex); + return _statusCode; + } + + std::optional error() const { + std::lock_guard guard(_mutex); + return _error; + } + + size_t bodySize() const { + std::lock_guard guard(_mutex); + return _bodySize; + } + + Value headers() const { + std::lock_guard guard(_mutex); + return _headers; + } + +private: + mutable std::mutex _mutex; + std::condition_variable _condition; + bool _done = false; + std::optional _statusCode; + std::optional _error; + size_t _bodySize = 0; + Value _headers; +}; + +// A peer that hangs up mid-response raises SIGPIPE on the serving thread, and its default +// disposition takes down the whole binary, every unrelated test with it. Neither per-socket guard +// is portable — SO_NOSIGPIPE is BSD-only and MSG_NOSIGNAL is Linux-only — whereas ignoring the +// signal works everywhere, and a test binary has no use for it in the first place. +void ignoreSigPipe() { + [[maybe_unused]] static const auto previous = ::signal(SIGPIPE, SIG_IGN); +} + +// Peak resident size is a high-water mark for the whole process, so the delta across one transfer +// is what that transfer newly demanded. +size_t peakResidentBytes() { + rusage usage{}; + ::getrusage(RUSAGE_SELF, &usage); +#ifdef __APPLE__ + return static_cast(usage.ru_maxrss); +#else + return static_cast(usage.ru_maxrss) * 1024; +#endif +} + +uint16_t bindToLoopback(int socketFd) { + sockaddr_in address{}; + address.sin_family = AF_INET; + address.sin_addr.s_addr = htonl(INADDR_LOOPBACK); + ::bind(socketFd, reinterpret_cast(&address), sizeof(address)); + + socklen_t length = sizeof(address); + ::getsockname(socketFd, reinterpret_cast(&address), &length); + return ntohs(address.sin_port); +} + +// Releases a serving thread blocked in accept(). Closing the listener is not portable for this — +// on Linux it leaves accept() blocked — and shutdown() is a no-op on a listening socket, so the +// only reliable release is a connection the thread can actually accept. +void wakeAccept(uint16_t port) { + int socketFd = ::socket(AF_INET, SOCK_STREAM, 0); + sockaddr_in address{}; + address.sin_family = AF_INET; + address.sin_addr.s_addr = htonl(INADDR_LOOPBACK); + address.sin_port = htons(port); + ::connect(socketFd, reinterpret_cast(&address), sizeof(address)); + ::close(socketFd); +} + +// A port nothing listens on, so connecting is refused immediately. +std::string unusedLoopbackUrl() { + int socketFd = ::socket(AF_INET, SOCK_STREAM, 0); + uint16_t port = bindToLoopback(socketFd); + ::close(socketFd); + + return "http://127.0.0.1:" + std::to_string(port) + "/"; +} + +// Accepts a connection and never writes a response, so a transfer stays genuinely in +// flight. A refused or unresolvable address will not do: those complete immediately. +class StallServer { +public: + StallServer() { + _listener = ::socket(AF_INET, SOCK_STREAM, 0); + _port = bindToLoopback(_listener); + ::listen(_listener, 1); + + _thread = std::thread([this]() { serve(); }); + } + + ~StallServer() { + _stopping.store(true); + + int connection = -1; + { + std::lock_guard guard(_mutex); + connection = _connection; + } + if (connection >= 0) { + // Releases the serving thread if it is still blocked reading. + ::shutdown(connection, SHUT_RDWR); + } else { + // Nothing ever connected, so the thread is still in accept(). + wakeAccept(_port); + } + + if (_thread.joinable()) { + _thread.join(); + } + // Closed here rather than on the serving thread, so neither fd can be reused under the + // shutdown above. + if (connection >= 0) { + ::close(connection); + } + ::close(_listener); + } + + bool waitForConnection(std::chrono::milliseconds timeout) { + std::unique_lock lock(_mutex); + return _condition.wait_for(lock, timeout, [this]() { return _connection >= 0; }); + } + + // Nothing is ever written back, so the peer going away is the only thing this server can + // observe — which makes it the one visible effect of a transfer being aborted. + bool waitForDisconnect(std::chrono::milliseconds timeout) { + std::unique_lock lock(_mutex); + return _condition.wait_for(lock, timeout, [this]() { return _disconnected; }); + } + + std::string url() const { + return "http://127.0.0.1:" + std::to_string(_port) + "/"; + } + +private: + void serve() { + int connection = ::accept(_listener, nullptr, nullptr); + if (connection < 0) { + return; + } + if (_stopping.load()) { + ::close(connection); + return; + } + + { + std::lock_guard guard(_mutex); + _connection = connection; + _condition.notify_all(); + } + + char buffer[512]; + while (::recv(connection, buffer, sizeof(buffer), 0) > 0) { + } + + std::lock_guard guard(_mutex); + _disconnected = true; + _condition.notify_all(); + } + + int _listener = -1; + int _connection = -1; + bool _disconnected = false; + uint16_t _port = 0; + std::atomic_bool _stopping{false}; + std::mutex _mutex; + std::condition_variable _condition; + std::thread _thread; +}; + +// Sends its response in pieces with a pause between them, so a transfer can take longer overall +// than an idle timeout without ever actually going idle. +class DribblingServer { +public: + DribblingServer(std::string response, size_t pieces, std::chrono::milliseconds gap) { + ignoreSigPipe(); + + _listener = ::socket(AF_INET, SOCK_STREAM, 0); + _port = bindToLoopback(_listener); + ::listen(_listener, 1); + + _thread = std::thread([this, response = std::move(response), pieces, gap]() { + int connection = ::accept(_listener, nullptr, nullptr); + if (connection < 0) { + return; + } + if (_stopping.load()) { + ::close(connection); + return; + } + + std::string request; + char buffer[512]; + while (request.find("\r\n\r\n") == std::string::npos) { + auto received = ::recv(connection, buffer, sizeof(buffer), 0); + if (received <= 0) { + break; + } + request.append(buffer, static_cast(received)); + } + + auto pieceSize = (response.size() + pieces - 1) / pieces; + for (size_t sent = 0; sent < response.size(); sent += pieceSize) { + auto piece = std::min(pieceSize, response.size() - sent); + if (!sendAll(connection, response.data() + sent, piece)) { + break; + } + std::this_thread::sleep_for(gap); + } + + ::shutdown(connection, SHUT_RDWR); + ::close(connection); + }); + } + + ~DribblingServer() { + _stopping.store(true); + wakeAccept(_port); + + if (_thread.joinable()) { + _thread.join(); + } + ::close(_listener); + } + + std::string url() const { + return "http://127.0.0.1:" + std::to_string(_port) + "/"; + } + +private: + // ::send places only what fits in the socket buffer and returns, so anything above a few + // hundred kilobytes needs the loop. + static bool sendAll(int connection, const char* data, size_t size) { + while (size > 0) { + auto written = ::send(connection, data, size, 0); + if (written <= 0) { + return false; + } + data += written; + size -= static_cast(written); + } + return true; + } + + int _listener = -1; + uint16_t _port = 0; + std::atomic_bool _stopping{false}; + std::thread _thread; +}; + +// Serves canned responses, one per connection, so a redirect chain is deterministic and +// needs no network. Each response should say "Connection: close" to keep curl from +// reusing a connection and leaving a later response unclaimed. +class ScriptedServer { +public: + explicit ScriptedServer(std::vector responses) { + ignoreSigPipe(); + + _listener = ::socket(AF_INET, SOCK_STREAM, 0); + _port = bindToLoopback(_listener); + ::listen(_listener, static_cast(responses.size())); + + _thread = std::thread([this, responses = std::move(responses)]() { + for (const auto& response : responses) { + int connection = ::accept(_listener, nullptr, nullptr); + if (connection < 0) { + return; + } + if (_stopping.load()) { + ::close(connection); + return; + } + + auto request = readRequest(connection); + { + std::lock_guard guard(_mutex); + _requestLines.push_back(request.substr(0, request.find("\r\n"))); + _requests.push_back(std::move(request)); + } + + ::send(connection, response.data(), response.size(), 0); + ::shutdown(connection, SHUT_RDWR); + ::close(connection); + } + }); + } + + ~ScriptedServer() { + _stopping.store(true); + wakeAccept(_port); + + if (_thread.joinable()) { + _thread.join(); + } + // Closed only once the serving thread is done with it, so the fd cannot be reused underneath + // an accept() still in progress. + ::close(_listener); + } + + std::string url(const char* path) const { + return "http://127.0.0.1:" + std::to_string(_port) + path; + } + + // The request line of each request served, in order, so tests can assert on the verb and + // path that actually went over the wire. + std::vector requestLines() const { + std::lock_guard guard(_mutex); + return _requestLines; + } + + // Each request in full, up to the end of its headers, for assertions the request line alone + // cannot carry. + std::vector requests() const { + std::lock_guard guard(_mutex); + return _requests; + } + +private: + // Drained so that closing the connection does not reset it before curl reads back. + static std::string readRequest(int connection) { + std::string request; + char buffer[512]; + while (request.find("\r\n\r\n") == std::string::npos) { + auto received = ::recv(connection, buffer, sizeof(buffer), 0); + if (received <= 0) { + break; + } + request.append(buffer, static_cast(received)); + } + return request; + } + + int _listener = -1; + uint16_t _port = 0; + std::atomic_bool _stopping{false}; + mutable std::mutex _mutex; + std::vector _requestLines; + std::vector _requests; + std::thread _thread; +}; + +snap::valdi_core::HTTPRequest makeGet(const char* url) { + return snap::valdi_core::HTTPRequest(StringBox::fromCString(url), STRING_LITERAL("GET"), Value(), std::nullopt, 0); +} + +snap::valdi_core::HTTPRequest makeRequestWithBody(const char* method, const char* url, const char* body) { + return snap::valdi_core::HTTPRequest(StringBox::fromCString(url), + StringBox::fromCString(method), + Value(), + makeShared(std::string(body))->toBytesView(), + 0); +} + +} // namespace + +TEST(CurlHTTPRequestManagerTests, keepsOnlyTheFinalResponsesHeaders) { + ScriptedServer server({"HTTP/1.1 301 Moved Permanently\r\n" + "Location: /final\r\n" + "X-From-Redirect: yes\r\n" + "Content-Length: 0\r\n" + "Connection: close\r\n" + "\r\n", + "HTTP/1.1 200 OK\r\n" + "X-From-Final: yes\r\n" + "Content-Length: 2\r\n" + "Connection: close\r\n" + "\r\n" + "ok"}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGet(server.url("/start").c_str()), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + ASSERT_EQ(completion->statusCode(), 200); + EXPECT_EQ(completion->bodySize(), 2u); + + auto headers = completion->headers(); + EXPECT_FALSE(headers.getMapValue("X-From-Final").isNullOrUndefined()) + << "the final response's own headers are missing"; + EXPECT_TRUE(headers.getMapValue("X-From-Redirect").isNullOrUndefined()) + << "a header sent only by the redirect survived into the result"; + EXPECT_TRUE(headers.getMapValue("Location").isNullOrUndefined()) + << "the redirect's Location survived into the result"; +} + +TEST(CurlHTTPRequestManagerTests, resetsHeadersOnAStatusLineWhoseReasonPhraseHasAColon) { + // A colon in the reason phrase is legal — RFC 7230 makes it free-form text — and it makes the + // final status line parse as a header if the colon is looked for before the HTTP/ prefix, which + // skips the reset that discards the redirect's headers. + ScriptedServer server({"HTTP/1.1 301 Moved Permanently\r\n" + "Location: /final\r\n" + "X-From-Redirect: yes\r\n" + "Content-Length: 0\r\n" + "Connection: close\r\n" + "\r\n", + "HTTP/1.1 200 Enhance your calm: relax\r\n" + "X-From-Final: yes\r\n" + "Content-Length: 2\r\n" + "Connection: close\r\n" + "\r\n" + "ok"}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGet(server.url("/start").c_str()), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + ASSERT_EQ(completion->statusCode(), 200); + + auto headers = completion->headers(); + EXPECT_FALSE(headers.getMapValue("X-From-Final").isNullOrUndefined()) + << "the final response's own headers are missing"; + EXPECT_TRUE(headers.getMapValue("X-From-Redirect").isNullOrUndefined()) + << "a header sent only by the redirect survived, so the final status line was mistaken for a " + "header and never reset them"; + EXPECT_TRUE(headers.getMapValue("Location").isNullOrUndefined()) + << "the redirect's Location survived into the result"; + EXPECT_TRUE(headers.getMapValue("HTTP/1.1 200 Enhance your calm").isNullOrUndefined()) + << "the status line was stored as though it were a header"; +} + +TEST(CurlHTTPRequestManagerTests, joinsRepeatedResponseHeaders) { + ScriptedServer server({"HTTP/1.1 200 OK\r\n" + "Set-Cookie: session=abc\r\n" + "Set-Cookie: csrf=def\r\n" + "Set-Cookie: prefs=ghi\r\n" + "Content-Length: 2\r\n" + "Connection: close\r\n" + "\r\n" + "ok"}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGet(server.url("/").c_str()), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + ASSERT_EQ(completion->statusCode(), 200); + + auto cookies = completion->headers().getMapValue("Set-Cookie"); + ASSERT_FALSE(cookies.isNullOrUndefined()); + EXPECT_EQ(cookies.toStringBox().toStringView(), "session=abc, csrf=def, prefs=ghi") + << "repeated response headers must be joined the way NSURLResponse joins them, not " + "collapsed to whichever one arrived last"; +} + +TEST(CurlHTTPRequestManagerTests, sendsAGetCarryingABodyAsGet) { + ScriptedServer server({"HTTP/1.1 200 OK\r\n" + "Content-Length: 2\r\n" + "Connection: close\r\n" + "\r\n" + "ok"}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeRequestWithBody("GET", server.url("/query").c_str(), "{}"), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + ASSERT_EQ(completion->statusCode(), 200); + + auto requests = server.requestLines(); + ASSERT_EQ(requests.size(), 1u); + EXPECT_EQ(requests[0], "GET /query HTTP/1.1") << "a GET carrying a body must still be sent as GET"; +} + +TEST(CurlHTTPRequestManagerTests, sendsAnEmptyButPresentBodyAsAnEmptyPost) { + ScriptedServer server({"HTTP/1.1 200 OK\r\n" + "Content-Length: 2\r\n" + "Connection: close\r\n" + "\r\n" + "ok"}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + // An engaged body of size zero, as httpClient.post(url, new Uint8Array(0)) produces. An empty + // ByteBuffer never allocates, so its BytesView::data() is null. + manager->performRequest(makeRequestWithBody("POST", server.url("/submit").c_str(), ""), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))) + << "the request never completed, so curl was left waiting for a body from somewhere else"; + ASSERT_EQ(completion->statusCode(), 200); + + auto requests = server.requests(); + ASSERT_EQ(requests.size(), 1u); + EXPECT_EQ(requests[0].substr(0, requests[0].find("\r\n")), "POST /submit HTTP/1.1"); + EXPECT_NE(requests[0].find("Content-Length: 0\r\n"), std::string::npos) + << "an empty body must still declare a zero length. Request was:\n" + << requests[0]; +} + +TEST(CurlHTTPRequestManagerTests, followsASeeOtherWithGet) { + ScriptedServer server({"HTTP/1.1 303 See Other\r\n" + "Location: /final\r\n" + "Content-Length: 0\r\n" + "Connection: close\r\n" + "\r\n", + "HTTP/1.1 200 OK\r\n" + "Content-Length: 2\r\n" + "Connection: close\r\n" + "\r\n" + "ok"}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeRequestWithBody("POST", server.url("/submit").c_str(), "a=1"), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + ASSERT_EQ(completion->statusCode(), 200); + + auto requests = server.requestLines(); + ASSERT_EQ(requests.size(), 2u); + EXPECT_EQ(requests[0], "POST /submit HTTP/1.1"); + EXPECT_EQ(requests[1], "GET /final HTTP/1.1") << "a 303 must be followed with GET, not the original verb"; +} + +TEST(CurlHTTPRequestManagerTests, reportsALoopbackResponseWithoutWaitingOutThePollTimeout) { + ScriptedServer server({"HTTP/1.1 200 OK\r\n" + "Content-Length: 2\r\n" + "Connection: close\r\n" + "\r\n" + "ok"}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + + auto start = std::chrono::steady_clock::now(); + manager->performRequest(makeGet(server.url("/").c_str()), completion); + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + auto elapsed = std::chrono::duration_cast(std::chrono::steady_clock::now() - start); + + ASSERT_EQ(completion->statusCode(), 200); + EXPECT_LT(elapsed.count(), 100) << "a loopback request was reported after " << elapsed.count() + << " ms; the finished transfer is not drained until curl_multi_poll " + "has slept out its whole timeout"; +} + +TEST(CurlHTTPRequestManagerTests, servesARequestPromptlyAfterSittingIdle) { + ScriptedServer server({"HTTP/1.1 200 OK\r\n" + "Content-Length: 2\r\n" + "Connection: close\r\n" + "\r\n" + "ok"}); + + auto manager = makeCurlHTTPRequestManager(); + + // Long enough that the curl thread is certainly parked in its poll. An idle multi handle has + // no timer for curl to report, so nothing but the wakeup in performRequest can serve this + // inside the deadline below. + std::this_thread::sleep_for(std::chrono::milliseconds(300)); + + auto completion = std::make_shared(); + auto start = std::chrono::steady_clock::now(); + manager->performRequest(makeGet(server.url("/").c_str()), completion); + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(30))); + auto elapsed = std::chrono::duration_cast(std::chrono::steady_clock::now() - start); + + ASSERT_EQ(completion->statusCode(), 200); + EXPECT_LT(elapsed.count(), 500) << "an idle curl thread took " << elapsed.count() + << " ms to pick up new work, so it was not woken and waited out " + "kPollTimeoutMs instead"; +} + +TEST(CurlHTTPRequestManagerTests, reportsFailureForAnUnreachableHost) { + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + + auto cancelable = manager->performRequest(makeGet(unusedLoopbackUrl().c_str()), completion); + ASSERT_NE(cancelable, nullptr); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(30))); + ASSERT_FALSE(completion->statusCode().has_value()); + ASSERT_TRUE(completion->error().has_value()); +} + +TEST(CurlHTTPRequestManagerTests, failsATransferThatStopsMakingProgress) { + StallServer server; + + // A one second idle timeout, so the test does not sit out the default. + auto manager = makeCurlHTTPRequestManager(StringBox(), 1); + auto completion = std::make_shared(); + manager->performRequest(makeGet(server.url().c_str()), completion); + + ASSERT_TRUE(server.waitForConnection(std::chrono::seconds(5))) + << "curl never connected, so a stalled transfer is not being exercised"; + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(20))) + << "a server that accepts and then never answers leaves the request pending forever"; + EXPECT_TRUE(completion->error().has_value()) << "a stalled transfer should be reported as a failure"; +} + +TEST(CurlHTTPRequestManagerTests, leavesASlowButProgressingTransferAlone) { + std::string body(400, 'x'); + // Ten pieces 200 ms apart, so about two seconds in total — well past the idle timeout below, + // but never a whole second without data. An overall CURLOPT_TIMEOUT would cut this off. + DribblingServer server("HTTP/1.1 200 OK\r\n" + "Content-Length: 400\r\n" + "Connection: close\r\n" + "\r\n" + + body, + 10, + std::chrono::milliseconds(200)); + + auto manager = makeCurlHTTPRequestManager(StringBox(), 1); + auto completion = std::make_shared(); + manager->performRequest(makeGet(server.url().c_str()), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(30))); + EXPECT_FALSE(completion->error().has_value()) + << "a slow but progressing transfer was cut off: " << completion->error().value_or(""); + EXPECT_EQ(completion->statusCode(), 200); + EXPECT_EQ(completion->bodySize(), 400u); +} + +TEST(CurlHTTPRequestManagerTests, scriptedServerShutsDownWithAResponseUnclaimed) { + static const char* kResponse = "HTTP/1.1 200 OK\r\n" + "Content-Length: 2\r\n" + "Connection: close\r\n" + "\r\n" + "ok"; + + // The work runs on a detached thread so that a destructor which never returns fails this test + // rather than wedging the whole binary. + std::promise destroyed; + auto finished = destroyed.get_future(); + + std::thread([destroyed = std::move(destroyed)]() mutable { + { + // Primed with two responses but given one request. That is exactly the state + // followsASeeOtherWithGet is left in when a redirect is not followed — the regression it + // exists to catch — and it leaves the serving thread blocked in accept() for a second + // connection that never comes. + ScriptedServer server({kResponse, kResponse}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGet(server.url("/only").c_str()), completion); + completion->waitForCompletion(std::chrono::seconds(10)); + } + destroyed.set_value(); + }).detach(); + + EXPECT_EQ(finished.wait_for(std::chrono::seconds(10)), std::future_status::ready) + << "~ScriptedServer never returned. Its thread is blocked in accept() for a connection that " + "will never arrive, so a test that fails this way times out instead of reporting which " + "assertion failed"; +} + +TEST(CurlHTTPRequestManagerTests, survivesAPeerHangingUpMidResponse) { + std::string body(4000, 'x'); + // Forty pieces 50 ms apart, so there are plenty of writes left to attempt after the peer goes. + DribblingServer server("HTTP/1.1 200 OK\r\n" + "Content-Length: 4000\r\n" + "Connection: close\r\n" + "\r\n" + + body, + 40, + std::chrono::milliseconds(50)); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + auto cancelable = manager->performRequest(makeGet(server.url().c_str()), completion); + + // Cancelling partway through makes curl close the connection while the server still has most of + // the response to write, so every later ::send is against a socket the peer has gone from. If + // SIGPIPE is not suppressed that terminates this binary, taking every unrelated test with it. + std::this_thread::sleep_for(std::chrono::milliseconds(200)); + cancelable->cancel(); + + EXPECT_FALSE(completion->waitForCompletion(std::chrono::seconds(1))) + << "a cancelled request must leave its completion uncalled"; +} + +TEST(CurlHTTPRequestManagerTests, doesNotHoldTheResponseBodyTwice) { + constexpr size_t kBodySize = 32 * 1024 * 1024; + + DribblingServer server("HTTP/1.1 200 OK\r\n" + "Content-Length: " + + std::to_string(kBodySize) + + "\r\n" + "Connection: close\r\n" + "\r\n" + + std::string(kBodySize, 'x'), + 1, + std::chrono::milliseconds(0)); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + + auto before = peakResidentBytes(); + manager->performRequest(makeGet(server.url().c_str()), completion); + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(60))); + + ASSERT_EQ(completion->statusCode(), 200); + ASSERT_EQ(completion->bodySize(), kBodySize) << "the large body did not arrive intact"; + + // Measured at 1.03x holding the payload once and 2.03x holding it twice, so the threshold sits + // midway rather than just under the failing value. + auto growth = peakResidentBytes() - before; + EXPECT_LT(growth, kBodySize + kBodySize / 2) + << "peak memory grew by " << growth / (1024 * 1024) << " MiB to receive a " + << kBodySize / (1024 * 1024) + << " MiB body, so the payload is accumulated in one buffer and then copied whole into another"; +} + +TEST(CurlHTTPRequestManagerTests, cancellingARequestDropsItsCompletion) { + StallServer server; + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + + auto cancelable = manager->performRequest(makeGet(server.url().c_str()), completion); + ASSERT_NE(cancelable, nullptr); + + ASSERT_TRUE(server.waitForConnection(std::chrono::seconds(5))) + << "curl never connected, so there is no live transfer to cancel"; + cancelable->cancel(); + + // curl hanging up means the abort has been through drainMessages, which is where a completion + // that was merely detached from curl rather than dropped would have fired. The manager is left + // alive on purpose, so that this proves cancel() dropped the completion rather than shutdown + // doing it. + ASSERT_TRUE(server.waitForDisconnect(std::chrono::seconds(30))) + << "curl never hung up, so the cancelled transfer was never aborted"; + + EXPECT_FALSE(completion->waitForCompletion(std::chrono::seconds(1))) + << "a cancelled request must leave its completion uncalled, as on iOS and Android"; +} + +TEST(CurlHTTPRequestManagerTests, abortsACancelledTransferWithoutWaitingOutThePollTimeout) { + StallServer server; + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + auto cancelable = manager->performRequest(makeGet(server.url().c_str()), completion); + + ASSERT_TRUE(server.waitForConnection(std::chrono::seconds(5))) + << "curl never connected, so there is no live transfer to cancel"; + + auto start = std::chrono::steady_clock::now(); + cancelable->cancel(); + ASSERT_TRUE(server.waitForDisconnect(std::chrono::seconds(30))) + << "curl never hung up, so the cancelled transfer was never aborted"; + auto elapsed = std::chrono::duration_cast(std::chrono::steady_clock::now() - start); + + EXPECT_LT(elapsed.count(), 100) << "a cancelled transfer was aborted after " << elapsed.count() + << " ms; cancelling does not wake the curl thread, so it sleeps out its " + "poll timeout before consulting the progress callback"; +} + +TEST(CurlHTTPRequestManagerTests, aCancelledLookupDoesNotStallOtherRequests) { + ScriptedServer server({"HTTP/1.1 200 OK\r\n" + "Content-Length: 2\r\n" + "Connection: close\r\n" + "\r\n" + "ok"}); + + auto manager = makeCurlHTTPRequestManager(); + + // A name the resolver takes a long time to give up on, cancelled while the lookup is still + // running. Where the resolver answers quickly there is no stall to observe and this test simply + // passes, so it can never fail spuriously. + auto abandoned = std::make_shared(); + auto cancelable = manager->performRequest(makeGet("http://this-host-does-not-exist.invalid/"), abandoned); + std::this_thread::sleep_for(std::chrono::milliseconds(150)); + cancelable->cancel(); + + auto completion = std::make_shared(); + auto start = std::chrono::steady_clock::now(); + manager->performRequest(makeGet(server.url("/after").c_str()), completion); + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(60))); + auto elapsed = std::chrono::duration_cast(std::chrono::steady_clock::now() - start); + + ASSERT_EQ(completion->statusCode(), 200); + EXPECT_LT(elapsed.count(), 1000) + << "an unrelated request waited " << elapsed.count() + << " ms because abandoning the cancelled lookup blocked the one curl thread until the " + "resolver gave up, stalling every other request with it"; +} + +TEST(CurlHTTPRequestManagerTests, reportsNothingWhenShutDownBeforeConnecting) { + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + + manager->performRequest(makeGet("http://this-host-does-not-exist.invalid/"), completion); + manager.reset(); + + EXPECT_FALSE(completion->waitForCompletion(std::chrono::seconds(1))) + << "shutdown reported a request that never got off the ground; it should be dropped, the " + "same as a cancellation"; +} + +TEST(CurlHTTPRequestManagerTests, shutdownCancelsRequestsInFlight) { + StallServer server; + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGet(server.url().c_str()), completion); + + ASSERT_TRUE(server.waitForConnection(std::chrono::seconds(5))) + << "curl never connected, so shutdown-with-a-live-transfer is not being exercised"; + + // The manager is moved into a detached thread so that a destructor which never returns + // fails this test instead of wedging the whole binary. + std::promise destroyed; + auto finished = destroyed.get_future(); + std::thread([manager = std::move(manager), destroyed = std::move(destroyed)]() mutable { + manager.reset(); + destroyed.set_value(); + }).detach(); + + EXPECT_EQ(finished.wait_for(std::chrono::seconds(5)), std::future_status::ready) + << "~CurlHTTPRequestManager waited for the in-flight transfer instead of cancelling it"; + EXPECT_FALSE(completion->waitForCompletion(std::chrono::seconds(1))) + << "shutdown reported an in-flight request. Completions reach JavaScript with no thread " + "hop, so firing one from the curl thread while the destroying thread is inside join() " + "enters the engine from two threads at once"; +} + +} // namespace ValdiTest diff --git a/valdi/test/integration/StandaloneRequestManager_tests.cpp b/valdi/test/standalone/StandaloneRequestManager_tests.cpp similarity index 98% rename from valdi/test/integration/StandaloneRequestManager_tests.cpp rename to valdi/test/standalone/StandaloneRequestManager_tests.cpp index 171ddbeeb..8019e08ae 100644 --- a/valdi/test/integration/StandaloneRequestManager_tests.cpp +++ b/valdi/test/standalone/StandaloneRequestManager_tests.cpp @@ -5,7 +5,8 @@ #include "valdi_core/cpp/Utils/ByteBuffer.hpp" #include "valdi_core/cpp/Utils/ConsoleLogger.hpp" -#include "JSBridgeTestFixture.hpp" +#include "valdi/test/integration/JSBridgeTestFixture.hpp" + #include "gtest/gtest.h" using namespace Valdi; From e7d9abaefe2cc9e3b7f8f276b70272823693f190 Mon Sep 17 00:00:00 2001 From: tx3stn <14163530+tx3stn@users.noreply.github.com> Date: Wed, 19 Aug 2026 23:20:09 +0100 Subject: [PATCH 06/18] fix: http errors never actualy throwing --- .../HTTPRequestManagerModuleFactory.cpp | 7 +- valdi/BUILD.bazel | 3 + .../runtime/JavaScript/JavaScriptUtils.cpp | 6 + .../StandaloneRequestManager_tests.cpp | 103 ++++++++++++++++++ 4 files changed, 118 insertions(+), 1 deletion(-) diff --git a/src/valdi_modules/src/cpp/valdi_http/HTTPRequestManagerModuleFactory.cpp b/src/valdi_modules/src/cpp/valdi_http/HTTPRequestManagerModuleFactory.cpp index 35a330f92..32dfd1ef5 100644 --- a/src/valdi_modules/src/cpp/valdi_http/HTTPRequestManagerModuleFactory.cpp +++ b/src/valdi_modules/src/cpp/valdi_http/HTTPRequestManagerModuleFactory.cpp @@ -85,7 +85,12 @@ Value HTTPRequestManagerModuleFactory::loadModule() { parameters[1] = Value::undefined(); } else { parameters[0] = Value::undefined(); - parameters[1] = Value(result.error()); + // Stringified, not Value(Error): converting a Value holding an Error raises it + // into the exception tracker rather than marshalling an argument + // (JavaScriptUtils.cpp:337), so the callback would never be invoked and the + // promise behind it would stay pending forever. Matches + // PersistentStoreModuleFactory. + parameters[1] = Value(result.error().toString()); } (*completion)(parameters.data(), parameters.size()); diff --git a/valdi/BUILD.bazel b/valdi/BUILD.bazel index f912969bb..d266c9dae 100644 --- a/valdi/BUILD.bazel +++ b/valdi/BUILD.bazel @@ -1001,6 +1001,9 @@ valdi_test( ":valdi_runtime_with_vm", ":valdi_standalone_http", "//src/valdi_modules/src/cpp/valdi_http:valdi_http_cpp", + # The JavaScript side of valdi_http, so tests can drive HTTPClient and assert on the promise + # it hands back, not just the native performRequest binding. + "//src/valdi_modules/src/valdi/valdi_http:valdi_http_native", "//src/valdi_modules/src/valdi/valdi_core:valdi_core_native", ], ) diff --git a/valdi/src/valdi/runtime/JavaScript/JavaScriptUtils.cpp b/valdi/src/valdi/runtime/JavaScript/JavaScriptUtils.cpp index e9ac1dad2..296c7820c 100644 --- a/valdi/src/valdi/runtime/JavaScript/JavaScriptUtils.cpp +++ b/valdi/src/valdi/runtime/JavaScript/JavaScriptUtils.cpp @@ -335,6 +335,12 @@ JSValueRef valueToJSValue(IJavaScriptContext& jsContext, jsContext, typedArray.getType(), typedArray.getBuffer(), exceptionTracker); } case ValueType::Error: { + // Raises rather than converting, so an Error reaching here as a *return* value throws in + // JavaScript. That makes it unusable for a callback *argument*: the raise happens while + // the arguments are being marshalled, so the callback is never invoked and any promise + // waiting on it stays pending. Pass error text instead, as + // HTTPRequestManagerModuleFactory and PersistentStoreModuleFactory do. Note the ObjC + // conversion differs and does produce a value (SCValdiError). exceptionTracker.onError(value.getError()); return JSValueRef(); } diff --git a/valdi/test/standalone/StandaloneRequestManager_tests.cpp b/valdi/test/standalone/StandaloneRequestManager_tests.cpp index 8019e08ae..638e4a598 100644 --- a/valdi/test/standalone/StandaloneRequestManager_tests.cpp +++ b/valdi/test/standalone/StandaloneRequestManager_tests.cpp @@ -67,6 +67,109 @@ TEST_P(StandaloneRequestManagerFixture, performRequestSucceedsFromJavaScript) { ASSERT_EQ("ok", result.value().toString()); } +TEST_P(StandaloneRequestManagerFixture, performRequestReportsSuccessToJavaScript) { + auto requestManager = Valdi::makeShared(ConsoleLogger::getLogger()); + requestManager->addMockedResponse(STRING_LITERAL("http://localhost/"), STRING_LITERAL("GET"), BytesView()); + + auto arguments = makeArguments(); + arguments.requestManager = requestManager; + + auto standaloneRuntime = createValdiStandaloneRuntime(arguments); + auto* jsRuntime = standaloneRuntime->getRuntime().getJavaScriptRuntime(); + + auto evaluate = [&](const std::string& source) { + return jsRuntime->evaluateScript(makeShared(source)->toBytesView(), + STRING_LITERAL("standalone_request_manager_test.js")); + }; + + auto started = evaluate("var m = global.require('valdi_http/src/NativeHTTPClient');" + "global.__outcome = '';" + "m.performRequest({ url: 'http://localhost/', method: 'GET', headers: {} }," + " function (response, error) {" + " global.__outcome = response ? 'status ' + response.statusCode : 'error: ' + error;" + " });" + "return 'started';"); + ASSERT_TRUE(started) << started.description(); + + requestManager->getAllPerformedTasks(); + + auto outcome = evaluate("return global.__outcome;"); + ASSERT_TRUE(outcome) << outcome.description(); + // The counterpart to the failure test below: nothing else asserts that a successful response + // actually reaches the callback, so stringifying the wrong parameter would go unnoticed. + EXPECT_EQ("status 200", outcome.value().toString()) << "a successful response must reach the JavaScript callback"; +} + +TEST_P(StandaloneRequestManagerFixture, performRequestReportsFailureToJavaScript) { + auto requestManager = Valdi::makeShared(ConsoleLogger::getLogger()); + // Deliberately no mocked response, so the mock fails the request. + + auto arguments = makeArguments(); + arguments.requestManager = requestManager; + + auto standaloneRuntime = createValdiStandaloneRuntime(arguments); + auto* jsRuntime = standaloneRuntime->getRuntime().getJavaScriptRuntime(); + + auto evaluate = [&](const std::string& source) { + return jsRuntime->evaluateScript(makeShared(source)->toBytesView(), + STRING_LITERAL("standalone_request_manager_test.js")); + }; + + auto started = evaluate("var m = global.require('valdi_http/src/NativeHTTPClient');" + "global.__outcome = '';" + "m.performRequest({ url: 'http://localhost/missing', method: 'GET', headers: {} }," + " function (response, error) {" + " global.__outcome = response ? 'response' : 'error: ' + error;" + " });" + "return 'started';"); + ASSERT_TRUE(started) << started.description(); + ASSERT_EQ("started", started.value().toString()); + + // Drains the mock's queue, so the completion — and with it the JavaScript callback — has run. + requestManager->getAllPerformedTasks(); + + auto outcome = evaluate("return global.__outcome;"); + ASSERT_TRUE(outcome) << outcome.description(); + EXPECT_EQ("error: No mocked response for given request", outcome.value().toString()) + << "a failed request must reach the JavaScript callback. Handing the error over as a Value " + "holding an Error raises it while the arguments are being marshalled instead, so the " + "callback is never invoked at all and the promise behind it stays pending forever"; +} + +// The contract callers actually depend on, and the one that hung: a failed request must settle the +// promise HTTPClient hands back, not abandon it. +TEST_P(StandaloneRequestManagerFixture, httpClientRejectsItsPromiseOnFailure) { + auto requestManager = Valdi::makeShared(ConsoleLogger::getLogger()); + // Deliberately no mocked response, so the mock fails the request. + + auto arguments = makeArguments(); + arguments.requestManager = requestManager; + + auto standaloneRuntime = createValdiStandaloneRuntime(arguments); + auto* jsRuntime = standaloneRuntime->getRuntime().getJavaScriptRuntime(); + + auto evaluate = [&](const std::string& source) { + return jsRuntime->evaluateScript(makeShared(source)->toBytesView(), + STRING_LITERAL("standalone_request_manager_test.js")); + }; + + auto started = evaluate("var HTTPClient = global.require('valdi_http/src/HTTPClient').HTTPClient;" + "global.__outcome = '';" + "new HTTPClient().get('http://localhost/missing').then(" + " function () { global.__outcome = 'resolved'; }," + " function (e) { global.__outcome = 'rejected: ' + e; });" + "return 'started';"); + ASSERT_TRUE(started) << started.description(); + + requestManager->getAllPerformedTasks(); + + auto outcome = evaluate("return global.__outcome;"); + ASSERT_TRUE(outcome) << outcome.description(); + EXPECT_EQ("rejected: No mocked response for given request", outcome.value().toString()) + << "the promise was left pending, which is what hangs a CLI: beginKeepAlive holds the runtime " + "open waiting for a settlement that never comes"; +} + INSTANTIATE_TEST_SUITE_P(StandaloneRequestManagerTests, StandaloneRequestManagerFixture, ::testing::Values(JavaScriptEngineTestCase::Hermes, From bcb650401d891c0eefc2dcc7e6cdcf6b27b9158f Mon Sep 17 00:00:00 2001 From: tx3stn <14163530+tx3stn@users.noreply.github.com> Date: Wed, 19 Aug 2026 23:36:11 +0100 Subject: [PATCH 07/18] chore: tidy up comments --- .../HTTPRequestManagerModuleFactory.cpp | 10 +-- valdi/BUILD.bazel | 4 +- .../runtime/JavaScript/JavaScriptUtils.cpp | 12 +-- .../CurlHTTPRequestManager.cpp | 79 +++++++++---------- .../CurlHTTPRequestManager.hpp | 16 ++-- .../CurlHTTPRequestManager_tests.cpp | 39 +++++---- .../StandaloneRequestManager_tests.cpp | 14 ++-- 7 files changed, 85 insertions(+), 89 deletions(-) diff --git a/src/valdi_modules/src/cpp/valdi_http/HTTPRequestManagerModuleFactory.cpp b/src/valdi_modules/src/cpp/valdi_http/HTTPRequestManagerModuleFactory.cpp index 32dfd1ef5..b8718aba4 100644 --- a/src/valdi_modules/src/cpp/valdi_http/HTTPRequestManagerModuleFactory.cpp +++ b/src/valdi_modules/src/cpp/valdi_http/HTTPRequestManagerModuleFactory.cpp @@ -85,11 +85,11 @@ Value HTTPRequestManagerModuleFactory::loadModule() { parameters[1] = Value::undefined(); } else { parameters[0] = Value::undefined(); - // Stringified, not Value(Error): converting a Value holding an Error raises it - // into the exception tracker rather than marshalling an argument - // (JavaScriptUtils.cpp:337), so the callback would never be invoked and the - // promise behind it would stay pending forever. Matches - // PersistentStoreModuleFactory. + // Pass the text, not the Error itself. Converting a Value holding an Error + // raises it into the exception tracker instead of marshalling an argument + // (JavaScriptUtils.cpp:337), so the callback would never run at all and the + // promise behind it would stay pending. PersistentStoreModuleFactory does + // the same. parameters[1] = Value(result.error().toString()); } diff --git a/valdi/BUILD.bazel b/valdi/BUILD.bazel index d266c9dae..f25ce878d 100644 --- a/valdi/BUILD.bazel +++ b/valdi/BUILD.bazel @@ -1001,8 +1001,8 @@ valdi_test( ":valdi_runtime_with_vm", ":valdi_standalone_http", "//src/valdi_modules/src/cpp/valdi_http:valdi_http_cpp", - # The JavaScript side of valdi_http, so tests can drive HTTPClient and assert on the promise - # it hands back, not just the native performRequest binding. + # The JavaScript side of valdi_http. Tests need this to drive HTTPClient and assert on the + # promise it hands back, rather than only the native performRequest binding. "//src/valdi_modules/src/valdi/valdi_http:valdi_http_native", "//src/valdi_modules/src/valdi/valdi_core:valdi_core_native", ], diff --git a/valdi/src/valdi/runtime/JavaScript/JavaScriptUtils.cpp b/valdi/src/valdi/runtime/JavaScript/JavaScriptUtils.cpp index 296c7820c..8c6d6eee9 100644 --- a/valdi/src/valdi/runtime/JavaScript/JavaScriptUtils.cpp +++ b/valdi/src/valdi/runtime/JavaScript/JavaScriptUtils.cpp @@ -335,12 +335,12 @@ JSValueRef valueToJSValue(IJavaScriptContext& jsContext, jsContext, typedArray.getType(), typedArray.getBuffer(), exceptionTracker); } case ValueType::Error: { - // Raises rather than converting, so an Error reaching here as a *return* value throws in - // JavaScript. That makes it unusable for a callback *argument*: the raise happens while - // the arguments are being marshalled, so the callback is never invoked and any promise - // waiting on it stays pending. Pass error text instead, as - // HTTPRequestManagerModuleFactory and PersistentStoreModuleFactory do. Note the ObjC - // conversion differs and does produce a value (SCValdiError). + // This raises instead of converting, so an Error arriving here as a return value + // throws in JavaScript. That makes it unusable for a callback argument: the raise + // happens while the arguments are still being marshalled, so the callback never runs + // and any promise waiting on it stays pending. Pass error text instead, as + // HTTPRequestManagerModuleFactory and PersistentStoreModuleFactory do. The ObjC + // conversion behaves differently and does produce a value (SCValdiError). exceptionTracker.onError(value.getError()); return JSValueRef(); } diff --git a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp index be614517b..5d4815cb0 100644 --- a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp +++ b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp @@ -30,23 +30,21 @@ namespace { constexpr long kMaxRedirects = 10; constexpr long kConnectTimeoutSeconds = 30; -// An upper bound, not an interval: curl_multi_poll waits for the shorter of this and the multi -// handle's own next timer, so an active transfer still gets serviced on curl's schedule. It only -// governs how long an idle thread sits before rechecking, and new work and shutdown both wake -// the poll explicitly, so there is nothing for a short value to catch. +// curl_multi_poll waits for the shorter of this and the multi handle's own next timer, so an +// active transfer is still serviced on curl's schedule. This only bounds how long an idle thread +// sits before rechecking. New work and shutdown both wake the poll, so a short value buys nothing. constexpr int kPollTimeoutMs = 10000; -// Read by the curl command line tool, not by libcurl, so they have to be honoured here for -// a caller to be able to point at a trust store of their own. +// The curl command line tool reads these, libcurl does not, so we honour them here to let a +// caller point at their own trust store. const char* const kCaBundleVariables[] = { "CURL_CA_BUNDLE", "SSL_CERT_FILE", }; -// Only distributions the @curl build defaults miss. The macOS and Debian-family paths are -// deliberately absent: @curl compiles CURL_CA_BUNDLE with exactly those two strings -// (curl+/BUILD.bazel:341-346), so libcurl already applies them, and probing for them here would -// also silently override a deliberate --@curl//:ca_bundle. +// Only distributions the @curl build defaults miss. @curl already compiles CURL_CA_BUNDLE with +// the macOS and Debian-family paths (curl+/BUILD.bazel:341-346), so libcurl applies those itself, +// and probing for them here would override a deliberate --@curl//:ca_bundle. const char* const kCaBundleCandidates[] = { "/etc/pki/tls/certs/ca-bundle.crt", // RHEL, Fedora "/etc/ssl/ca-bundle.pem", // openSUSE @@ -87,10 +85,10 @@ class CurlTask : public snap::valdi_core::Cancelable { dropCompletion(); } - // Used at shutdown as well as for cancellation. Completions reach JavaScript directly, with no - // thread hop, so firing one from the curl thread while the thread destroying the manager is - // blocked in join() would enter the engine from two threads at once. Neither platform manager - // guarantees a completion at teardown either, so there is nothing to report. + // Shutdown uses this too. Completions reach JavaScript directly with no thread hop, so firing + // one from the curl thread while another thread sits in join() destroying the manager would + // enter the engine twice over. Neither platform manager promises a completion at teardown, so + // there is nothing to report. void dropCompletion() { std::lock_guard guard(_mutex); _completion = nullptr; @@ -117,9 +115,9 @@ class CurlTask : public snap::valdi_core::Cancelable { snap::valdi_core::HTTPRequest request; std::atomic_bool cancelled{false}; - // Filled by the write callback and handed straight to the response, so the payload is never - // copied. ByteBuffer grows to the next power of two, so appending stays amortised constant time - // without reserving up front — which would mean sizing an allocation from a Content-Length the + // The write callback fills this and the response takes it directly, so the payload is never + // copied. ByteBuffer grows to the next power of two, so appending stays amortised constant + // time with no reserve up front. Reserving would size an allocation from a Content-Length the // server chose. Ref responseBody = makeShared(); Value responseHeaders; @@ -141,10 +139,10 @@ size_t writeHeaderCallback(char* data, size_t size, size_t count, void* userData std::string line(data, size * count); - // Tested before looking for a colon, because a reason phrase is free-form text and may contain - // one. Reading a status line as a header would also skip this reset, and with - // CURLOPT_FOLLOWLOCATION the callback sees every response in the chain, so the redirect's - // headers would be left to leak into the final result. + // Check this before looking for a colon, because a reason phrase is free-form text and may + // contain one. Misreading a status line as a header also skips the reset below, and with + // CURLOPT_FOLLOWLOCATION the callback sees every response in the chain, so a redirect's + // headers would leak into the final result. if (line.rfind("HTTP/", 0) == 0) { task->responseHeaders = Value(); return size * count; @@ -166,9 +164,9 @@ size_t writeHeaderCallback(char* data, size_t size, size_t count, void* userData value.pop_back(); } - // Joined rather than replaced, matching what NSURLResponse hands back for a repeated header. - // The response header map is string to string, so there is nowhere else for the earlier - // values to go, and dropping them loses whole Set-Cookie lines. + // Join repeated headers, matching what NSURLResponse hands back. The response header map is + // string to string, so earlier values have nowhere else to go, and dropping them loses whole + // Set-Cookie lines. auto existing = task->responseHeaders.getMapValue(std::string_view(name)); if (!existing.isNullOrUndefined()) { value = std::string(existing.toStringBox().toStringView()) + ", " + value; @@ -189,9 +187,8 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { CurlHTTPRequestManager(std::string caBundle, int32_t idleTimeoutSeconds, CURLcode globalInit) : _caBundle(std::move(caBundle)), _idleTimeoutSeconds(idleTimeoutSeconds), _globalInit(globalInit) { if (_globalInit != CURLE_OK) { - // Going on regardless would leave curl_easy_init handing back handles whose TLS backend - // was never set up, so every HTTPS request would fail with something that looks - // unrelated. + // Carrying on would leave curl_easy_init handing back handles whose TLS backend was + // never set up, so every HTTPS request would fail with an unrelated-looking error. return; } @@ -219,7 +216,7 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { const std::shared_ptr& completion) override { auto task = std::make_shared(request, completion); - // Checked before the multi handle, so the reported cause is the one that actually failed. + // Check this before the multi handle, so the reported cause is the one that actually failed. if (_globalInit != CURLE_OK) { task->complete(Error(StringBox::fromString(std::string("Failed to initialise libcurl: ") + curl_easy_strerror(_globalInit)))); @@ -240,9 +237,9 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { } } - // Failed outside the lock, because a completion is free to queue another request and - // _mutex is not recursive. Queueing here instead would strand the task: run() has - // already drained _pending for the last time, and nothing will service it again. + // Fail outside the lock, because a completion is free to queue another request and _mutex + // is not recursive. Queueing the task instead would strand it: run() has already drained + // _pending for the last time and nothing will service it again. if (stopping) { task->complete(Error(STRING_LITERAL("Request manager shutting down"))); return task; @@ -272,16 +269,16 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { int running = 0; curl_multi_perform(_multi, &running); - // Before the poll, not after: perform is what queues CURLMSG_DONE, and once nothing - // is running curl has no timeout to report, so the poll would sleep out its whole - // timeout before a finished transfer was ever reported. + // This has to run before the poll. perform is what queues CURLMSG_DONE, and once + // nothing is running curl has no timeout to report, so the poll would sleep out its + // full timeout before a finished transfer got reported. drainMessages(); int numfds = 0; curl_multi_poll(_multi, nullptr, 0, kPollTimeoutMs, &numfds); } - // Outstanding work is dropped rather than failed; see CurlTask::dropCompletion. + // Drop outstanding work instead of failing it; see CurlTask::dropCompletion. for (auto& entry : _active) { entry.second->dropCompletion(); curl_multi_remove_handle(_multi, entry.first); @@ -363,15 +360,15 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { curl_easy_setopt(easy, CURLOPT_CONNECTTIMEOUT, kConnectTimeoutSeconds); curl_easy_setopt(easy, CURLOPT_NOSIGNAL, 1L); - // Abandoning a name lookup otherwise means joining the resolver thread, and getaddrinfo - // cannot be interrupted — so cancelling or shutting down mid-lookup blocks this thread until - // the resolver gives up, taking every other request with it. This makes curl detach that - // thread instead; it frees its own state once the lookup returns. + // Without this, abandoning a name lookup joins the resolver thread, and getaddrinfo cannot + // be interrupted. Cancelling or shutting down mid-lookup would block this thread until the + // resolver gave up, taking every other request with it. curl detaches the thread instead, + // and it frees its own state once the lookup returns. curl_easy_setopt(easy, CURLOPT_QUICK_EXIT, 1L); if (_idleTimeoutSeconds > 0) { - // An inactivity timeout, not an overall one: below one byte a second for this long - // counts as stalled. A slow but progressing download is left alone. + // An inactivity timeout rather than an overall cap: below one byte a second for this + // long counts as stalled, so a slow but progressing download is left alone. curl_easy_setopt(easy, CURLOPT_LOW_SPEED_LIMIT, 1L); curl_easy_setopt(easy, CURLOPT_LOW_SPEED_TIME, static_cast(_idleTimeoutSeconds)); } diff --git a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.hpp b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.hpp index 02c9f7a99..017ae6c22 100644 --- a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.hpp +++ b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.hpp @@ -11,18 +11,18 @@ constexpr int32_t kDefaultIdleTimeoutSeconds = 60; /** * An HTTPRequestManager backed by libcurl, for integrations with no platform user agent of their - * own — the standalone runtime and the CLI apps built on it. + * own: the standalone runtime and the CLI apps built on it. * - * caBundlePath selects the trust store used to verify server certificates. When empty, the - * CURL_CA_BUNDLE and SSL_CERT_FILE environment variables are consulted — libcurl does not read - * either itself — and then the few distribution paths the @curl build defaults do not name. When - * nothing here matches, libcurl falls back to the trust store compiled into it, which is what - * covers macOS and the Debian family; set --@curl//:ca_bundle to point that elsewhere. + * caBundlePath selects the trust store used to verify server certificates. When empty, we check + * the CURL_CA_BUNDLE and SSL_CERT_FILE environment variables, which libcurl does not read itself, + * then the few distribution paths the @curl build defaults do not name. If nothing matches, + * libcurl falls back to the trust store compiled into it, covering macOS and the Debian family. + * Set --@curl//:ca_bundle to point that elsewhere. * * idleTimeoutSeconds fails a request that goes this long without transferring anything, so that a * server which accepts a connection and then stalls cannot leave a caller waiting forever. It is - * deliberately an inactivity timeout rather than an overall one: a large asset download is slow - * but never idle, and a hard cap would cut it off. Zero disables it. + * deliberately measures inactivity, not total elapsed time: a large asset download is slow but + * never idle, and a hard cap would cut it off. Zero disables it. */ Shared makeCurlHTTPRequestManager( const StringBox& caBundlePath = StringBox(), int32_t idleTimeoutSeconds = kDefaultIdleTimeoutSeconds); diff --git a/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp b/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp index 86c3826ad..62d0f8ce7 100644 --- a/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp +++ b/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp @@ -88,8 +88,8 @@ class RecordingCompletion : public snap::valdi_core::HTTPRequestManagerCompletio // A peer that hangs up mid-response raises SIGPIPE on the serving thread, and its default // disposition takes down the whole binary, every unrelated test with it. Neither per-socket guard -// is portable — SO_NOSIGPIPE is BSD-only and MSG_NOSIGNAL is Linux-only — whereas ignoring the -// signal works everywhere, and a test binary has no use for it in the first place. +// is portable, since SO_NOSIGPIPE is BSD-only and MSG_NOSIGNAL is Linux-only. Ignoring the signal +// works everywhere, and a test binary has no use for it anyway. void ignoreSigPipe() { [[maybe_unused]] static const auto previous = ::signal(SIGPIPE, SIG_IGN); } @@ -117,8 +117,8 @@ uint16_t bindToLoopback(int socketFd) { return ntohs(address.sin_port); } -// Releases a serving thread blocked in accept(). Closing the listener is not portable for this — -// on Linux it leaves accept() blocked — and shutdown() is a no-op on a listening socket, so the +// Releases a serving thread blocked in accept(). Closing the listener does not do it portably, +// because on Linux accept() stays blocked, and shutdown() is a no-op on a listening socket. The // only reliable release is a connection the thread can actually accept. void wakeAccept(uint16_t port) { int socketFd = ::socket(AF_INET, SOCK_STREAM, 0); @@ -183,8 +183,8 @@ class StallServer { return _condition.wait_for(lock, timeout, [this]() { return _connection >= 0; }); } - // Nothing is ever written back, so the peer going away is the only thing this server can - // observe — which makes it the one visible effect of a transfer being aborted. + // Nothing is ever written back, so the peer going away is all this server can observe. That + // makes it the one visible effect of a transfer being aborted. bool waitForDisconnect(std::chrono::milliseconds timeout) { std::unique_lock lock(_mutex); return _condition.wait_for(lock, timeout, [this]() { return _disconnected; }); @@ -447,9 +447,9 @@ TEST(CurlHTTPRequestManagerTests, keepsOnlyTheFinalResponsesHeaders) { } TEST(CurlHTTPRequestManagerTests, resetsHeadersOnAStatusLineWhoseReasonPhraseHasAColon) { - // A colon in the reason phrase is legal — RFC 7230 makes it free-form text — and it makes the - // final status line parse as a header if the colon is looked for before the HTTP/ prefix, which - // skips the reset that discards the redirect's headers. + // A colon in the reason phrase is legal, since RFC 7230 makes it free-form text. It also makes + // the final status line parse as a header if the colon is looked for before the HTTP/ prefix, + // which skips the reset that discards the redirect's headers. ScriptedServer server({"HTTP/1.1 301 Moved Permanently\r\n" "Location: /final\r\n" "X-From-Redirect: yes\r\n" @@ -652,8 +652,8 @@ TEST(CurlHTTPRequestManagerTests, failsATransferThatStopsMakingProgress) { TEST(CurlHTTPRequestManagerTests, leavesASlowButProgressingTransferAlone) { std::string body(400, 'x'); - // Ten pieces 200 ms apart, so about two seconds in total — well past the idle timeout below, - // but never a whole second without data. An overall CURLOPT_TIMEOUT would cut this off. + // Ten pieces 200 ms apart, so about two seconds in total. That is well past the idle timeout + // below, but never a whole second without data. An overall CURLOPT_TIMEOUT would cut it off. DribblingServer server("HTTP/1.1 200 OK\r\n" "Content-Length: 400\r\n" "Connection: close\r\n" @@ -687,10 +687,10 @@ TEST(CurlHTTPRequestManagerTests, scriptedServerShutsDownWithAResponseUnclaimed) std::thread([destroyed = std::move(destroyed)]() mutable { { - // Primed with two responses but given one request. That is exactly the state - // followsASeeOtherWithGet is left in when a redirect is not followed — the regression it - // exists to catch — and it leaves the serving thread blocked in accept() for a second - // connection that never comes. + // Two responses primed, one request given. That is exactly the state + // followsASeeOtherWithGet is left in when a redirect is not followed, which is the + // regression it exists to catch, and it leaves the serving thread blocked in accept() + // waiting for a second connection that never comes. ScriptedServer server({kResponse, kResponse}); auto manager = makeCurlHTTPRequestManager(); @@ -755,8 +755,8 @@ TEST(CurlHTTPRequestManagerTests, doesNotHoldTheResponseBodyTwice) { ASSERT_EQ(completion->statusCode(), 200); ASSERT_EQ(completion->bodySize(), kBodySize) << "the large body did not arrive intact"; - // Measured at 1.03x holding the payload once and 2.03x holding it twice, so the threshold sits - // midway rather than just under the failing value. + // Holding the payload once measured 1.03x and holding it twice measured 2.03x, so the threshold + // sits midway instead of just under the failing value. auto growth = peakResidentBytes() - before; EXPECT_LT(growth, kBodySize + kBodySize / 2) << "peak memory grew by " << growth / (1024 * 1024) << " MiB to receive a " @@ -778,9 +778,8 @@ TEST(CurlHTTPRequestManagerTests, cancellingARequestDropsItsCompletion) { cancelable->cancel(); // curl hanging up means the abort has been through drainMessages, which is where a completion - // that was merely detached from curl rather than dropped would have fired. The manager is left - // alive on purpose, so that this proves cancel() dropped the completion rather than shutdown - // doing it. + // that was merely detached from curl would have fired if it had not been dropped. The manager + // is left alive on purpose, so this pins the drop on cancel() and not on shutdown. ASSERT_TRUE(server.waitForDisconnect(std::chrono::seconds(30))) << "curl never hung up, so the cancelled transfer was never aborted"; diff --git a/valdi/test/standalone/StandaloneRequestManager_tests.cpp b/valdi/test/standalone/StandaloneRequestManager_tests.cpp index 638e4a598..2451d2b19 100644 --- a/valdi/test/standalone/StandaloneRequestManager_tests.cpp +++ b/valdi/test/standalone/StandaloneRequestManager_tests.cpp @@ -13,8 +13,8 @@ using namespace Valdi; namespace ValdiTest { -// A CLI app reaches valdi_http through createValdiStandaloneRuntime, which had no way to install a -// request manager, so performRequest always failed with "No RequestManager set". +// A CLI app reaches valdi_http through createValdiStandaloneRuntime, so the request manager it is +// given has to arrive that way. Without one, performRequest fails with "No RequestManager set". class StandaloneRequestManagerFixture : public JSBridgeTestFixture { protected: StandaloneArguments makeArguments() { @@ -42,7 +42,7 @@ TEST_P(StandaloneRequestManagerFixture, reachesTheRuntimeWhenSupplied) { ASSERT_EQ(standaloneRuntime->getRuntime().getRequestManager(), requestManager); } -// The end-to-end shape a CLI app exercises: performRequest resolves instead of throwing. +// The shape a CLI app exercises end to end: performRequest resolves instead of throwing. TEST_P(StandaloneRequestManagerFixture, performRequestSucceedsFromJavaScript) { auto requestManager = Valdi::makeShared(ConsoleLogger::getLogger()); requestManager->addMockedResponse(STRING_LITERAL("http://localhost/"), STRING_LITERAL("GET"), BytesView()); @@ -95,7 +95,7 @@ TEST_P(StandaloneRequestManagerFixture, performRequestReportsSuccessToJavaScript auto outcome = evaluate("return global.__outcome;"); ASSERT_TRUE(outcome) << outcome.description(); - // The counterpart to the failure test below: nothing else asserts that a successful response + // The counterpart to the failure test below. Nothing else asserts that a successful response // actually reaches the callback, so stringifying the wrong parameter would go unnoticed. EXPECT_EQ("status 200", outcome.value().toString()) << "a successful response must reach the JavaScript callback"; } @@ -125,7 +125,7 @@ TEST_P(StandaloneRequestManagerFixture, performRequestReportsFailureToJavaScript ASSERT_TRUE(started) << started.description(); ASSERT_EQ("started", started.value().toString()); - // Drains the mock's queue, so the completion — and with it the JavaScript callback — has run. + // Drains the mock's queue, so the completion has run and with it the JavaScript callback. requestManager->getAllPerformedTasks(); auto outcome = evaluate("return global.__outcome;"); @@ -136,8 +136,8 @@ TEST_P(StandaloneRequestManagerFixture, performRequestReportsFailureToJavaScript "callback is never invoked at all and the promise behind it stays pending forever"; } -// The contract callers actually depend on, and the one that hung: a failed request must settle the -// promise HTTPClient hands back, not abandon it. +// The contract callers actually depend on, and the one that hung. A failed request has to settle +// the promise HTTPClient hands back instead of abandoning it. TEST_P(StandaloneRequestManagerFixture, httpClientRejectsItsPromiseOnFailure) { auto requestManager = Valdi::makeShared(ConsoleLogger::getLogger()); // Deliberately no mocked response, so the mock fails the request. From 1e5392163609c217ed2b3117cf578fdc194a20b7 Mon Sep 17 00:00:00 2001 From: tx3stn <14163530+tx3stn@users.noreply.github.com> Date: Fri, 21 Aug 2026 18:44:14 +0100 Subject: [PATCH 08/18] fix: re-direct handling --- .../CurlHTTPRequestManager.cpp | 52 +++++- .../CurlHTTPRequestManager_tests.cpp | 176 +++++++++++++++++- 2 files changed, 220 insertions(+), 8 deletions(-) diff --git a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp index 5d4815cb0..52ab52f60 100644 --- a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp +++ b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp @@ -28,7 +28,7 @@ namespace Valdi { namespace { -constexpr long kMaxRedirects = 10; +constexpr int kMaxRedirects = 10; constexpr long kConnectTimeoutSeconds = 30; // curl_multi_poll waits for the shorter of this and the multi handle's own next timer, so an // active transfer is still serviced on curl's schedule. This only bounds how long an idle thread @@ -112,7 +112,10 @@ class CurlTask : public snap::valdi_core::Cancelable { } } + // Rewritten in place as redirects are followed, so that each hop is issued from the same + // record the first one was. snap::valdi_core::HTTPRequest request; + int redirects = 0; std::atomic_bool cancelled{false}; // The write callback fills this and the response takes it directly, so the payload is never @@ -140,9 +143,8 @@ size_t writeHeaderCallback(char* data, size_t size, size_t count, void* userData std::string line(data, size * count); // Check this before looking for a colon, because a reason phrase is free-form text and may - // contain one. Misreading a status line as a header also skips the reset below, and with - // CURLOPT_FOLLOWLOCATION the callback sees every response in the chain, so a redirect's - // headers would leak into the final result. + // contain one. The reset discards anything an informational response carried, since curl hands + // a 1xx header block to this callback too and a 103 Early Hints brings real headers with it. if (line.rfind("HTTP/", 0) == 0) { task->responseHeaders = Value(); return size * count; @@ -355,8 +357,6 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { curl_easy_setopt(easy, CURLOPT_XFERINFODATA, task.get()); curl_easy_setopt(easy, CURLOPT_NOPROGRESS, 0L); - curl_easy_setopt(easy, CURLOPT_FOLLOWLOCATION, 1L); - curl_easy_setopt(easy, CURLOPT_MAXREDIRS, kMaxRedirects); curl_easy_setopt(easy, CURLOPT_CONNECTTIMEOUT, kConnectTimeoutSeconds); curl_easy_setopt(easy, CURLOPT_NOSIGNAL, 1L); @@ -385,6 +385,29 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { _active.emplace(easy, task); } + // Redirects are followed here rather than by CURLOPT_FOLLOWLOCATION, because that option keeps + // CURLOPT_CUSTOMREQUEST for the whole chain: Curl_http_method takes the request line from it + // unconditionally, and the redirect handlers only ever touch curl's own idea of the method. A + // DELETE would stay a DELETE across a 303 that has to become a GET, and a PUT would keep its + // verb while curl dropped the body out from under it. The rewriting below is RFC 9110 15.4. + static void retarget(CurlTask& task, long statusCode, const char* location) { + task.request.url = StringBox::fromCString(location); + + auto method = std::string(task.request.method.toStringView()); + bool toGet = statusCode == 303 ? (method != "GET" && method != "HEAD") + : ((statusCode == 301 || statusCode == 302) && method == "POST"); + if (toGet) { + task.request.method = STRING_LITERAL("GET"); + task.request.body.reset(); + } + + // curl only withholds a redirect's body from the write callback when it is following the + // redirect itself, so this hop's is ours to discard. + task.responseBody->clear(); + task.responseHeaders = Value(); + task.redirects++; + } + void drainMessages() { int remaining = 0; while (auto* message = curl_multi_info_read(_multi, &remaining)) { @@ -408,6 +431,23 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { long statusCode = 0; curl_easy_getinfo(easy, CURLINFO_RESPONSE_CODE, &statusCode); + // Only set for a 3xx carrying a Location, and curl has already resolved it against + // the request URL, so a relative target arrives absolute. + char* location = nullptr; + curl_easy_getinfo(easy, CURLINFO_REDIRECT_URL, &location); + if (location != nullptr) { + if (task->redirects >= kMaxRedirects) { + finish(easy, task, Error(STRING_LITERAL("Maximum number of redirects followed"))); + continue; + } + retarget(*task, statusCode, location); + releaseHandle(easy, task); + // curl_multi_add_handle asks for this handle to run immediately, so the next hop + // is serviced on the following pass rather than after a poll timeout. + addTask(task); + continue; + } + snap::valdi_core::HTTPResponse response(static_cast(statusCode), task->responseHeaders, {task->responseBody->toBytesView()}); diff --git a/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp b/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp index 62d0f8ce7..d37f38a63 100644 --- a/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp +++ b/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp @@ -378,11 +378,22 @@ class ScriptedServer { } private: - // Drained so that closing the connection does not reset it before curl reads back. + // Drained so that closing the connection does not reset it before curl reads back. A declared + // body is drained too, so that asserting on what was sent does not depend on the body having + // arrived in the same packet as the headers. static std::string readRequest(int connection) { std::string request; char buffer[512]; while (request.find("\r\n\r\n") == std::string::npos) { + auto received = ::recv(connection, buffer, sizeof(buffer), 0); + if (received <= 0) { + return request; + } + request.append(buffer, static_cast(received)); + } + + auto expected = request.find("\r\n\r\n") + 4 + declaredBodySize(request); + while (request.size() < expected) { auto received = ::recv(connection, buffer, sizeof(buffer), 0); if (received <= 0) { break; @@ -392,6 +403,16 @@ class ScriptedServer { return request; } + static size_t declaredBodySize(const std::string& request) { + static const std::string kField = "\r\nContent-Length: "; + + auto at = request.find(kField); + if (at == std::string::npos) { + return 0; + } + return static_cast(std::stoul(request.substr(at + kField.size()))); + } + int _listener = -1; uint16_t _port = 0; std::atomic_bool _stopping{false}; @@ -401,8 +422,13 @@ class ScriptedServer { std::thread _thread; }; +snap::valdi_core::HTTPRequest makeRequest(const char* method, const char* url) { + return snap::valdi_core::HTTPRequest( + StringBox::fromCString(url), StringBox::fromCString(method), Value(), std::nullopt, 0); +} + snap::valdi_core::HTTPRequest makeGet(const char* url) { - return snap::valdi_core::HTTPRequest(StringBox::fromCString(url), STRING_LITERAL("GET"), Value(), std::nullopt, 0); + return makeRequest("GET", url); } snap::valdi_core::HTTPRequest makeRequestWithBody(const char* method, const char* url, const char* body) { @@ -575,6 +601,152 @@ TEST(CurlHTTPRequestManagerTests, followsASeeOtherWithGet) { EXPECT_EQ(requests[1], "GET /final HTTP/1.1") << "a 303 must be followed with GET, not the original verb"; } +TEST(CurlHTTPRequestManagerTests, followsASeeOtherFromACustomVerbWithGet) { + ScriptedServer server({"HTTP/1.1 303 See Other\r\n" + "Location: /final\r\n" + "Content-Length: 0\r\n" + "Connection: close\r\n" + "\r\n", + "HTTP/1.1 200 OK\r\n" + "Content-Length: 2\r\n" + "Connection: close\r\n" + "\r\n" + "ok"}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + + auto start = std::chrono::steady_clock::now(); + manager->performRequest(makeRequest("DELETE", server.url("/thing").c_str()), completion); + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(30))); + auto elapsed = std::chrono::duration_cast(std::chrono::steady_clock::now() - start); + + ASSERT_EQ(completion->statusCode(), 200); + + auto requests = server.requestLines(); + ASSERT_EQ(requests.size(), 2u); + EXPECT_EQ(requests[0], "DELETE /thing HTTP/1.1"); + EXPECT_EQ(requests[1], "GET /final HTTP/1.1") + << "a 303 must be followed with GET, including for a verb curl only knows as a custom " + "request line"; + + EXPECT_LT(elapsed.count(), 500) << "a two hop redirect took " << elapsed.count() + << " ms, so a hop is not picked up until the poll has slept out " + "its whole timeout"; +} + +TEST(CurlHTTPRequestManagerTests, dropsThePutBodyFollowingASeeOtherWithGet) { + ScriptedServer server({"HTTP/1.1 303 See Other\r\n" + "Location: /final\r\n" + "Content-Length: 0\r\n" + "Connection: close\r\n" + "\r\n", + "HTTP/1.1 200 OK\r\n" + "Content-Length: 2\r\n" + "Connection: close\r\n" + "\r\n" + "ok"}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeRequestWithBody("PUT", server.url("/thing").c_str(), "a=1"), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(30))); + ASSERT_EQ(completion->statusCode(), 200); + + auto requests = server.requests(); + ASSERT_EQ(requests.size(), 2u); + EXPECT_EQ(requests[0].substr(0, requests[0].find("\r\n")), "PUT /thing HTTP/1.1"); + EXPECT_EQ(requests[1].substr(0, requests[1].find("\r\n")), "GET /final HTTP/1.1") + << "a 303 must be followed with GET. Request was:\n" + << requests[1]; + EXPECT_EQ(requests[1].find("Content-Length"), std::string::npos) + << "the verb became GET but the body came along with it. Request was:\n" + << requests[1]; +} + +TEST(CurlHTTPRequestManagerTests, keepsThePutBodyAcrossAMovedPermanently) { + ScriptedServer server({"HTTP/1.1 301 Moved Permanently\r\n" + "Location: /final\r\n" + "Content-Length: 0\r\n" + "Connection: close\r\n" + "\r\n", + "HTTP/1.1 200 OK\r\n" + "Content-Length: 2\r\n" + "Connection: close\r\n" + "\r\n" + "ok"}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeRequestWithBody("PUT", server.url("/thing").c_str(), "a=1"), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(30))); + ASSERT_EQ(completion->statusCode(), 200); + + auto requests = server.requests(); + ASSERT_EQ(requests.size(), 2u); + EXPECT_EQ(requests[1].substr(0, requests[1].find("\r\n")), "PUT /final HTTP/1.1") + << "only POST turns into GET on a 301. Request was:\n" + << requests[1]; + EXPECT_NE(requests[1].find("Content-Length: 3\r\n"), std::string::npos) + << "the redirected PUT was sent with no body at all. Request was:\n" + << requests[1]; + EXPECT_NE(requests[1].find("\r\n\r\na=1"), std::string::npos) + << "the redirected PUT declared a body but did not send it. Request was:\n" + << requests[1]; +} + +TEST(CurlHTTPRequestManagerTests, keepsThePostBodyAcrossATemporaryRedirect) { + ScriptedServer server({"HTTP/1.1 307 Temporary Redirect\r\n" + "Location: /final\r\n" + "Content-Length: 0\r\n" + "Connection: close\r\n" + "\r\n", + "HTTP/1.1 200 OK\r\n" + "Content-Length: 2\r\n" + "Connection: close\r\n" + "\r\n" + "ok"}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeRequestWithBody("POST", server.url("/submit").c_str(), "a=1"), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(30))); + ASSERT_EQ(completion->statusCode(), 200); + + auto requests = server.requests(); + ASSERT_EQ(requests.size(), 2u); + EXPECT_EQ(requests[1].substr(0, requests[1].find("\r\n")), "POST /final HTTP/1.1") + << "a 307 must be repeated with the original verb. Request was:\n" + << requests[1]; + EXPECT_NE(requests[1].find("\r\n\r\na=1"), std::string::npos) + << "a 307 must be repeated with the original body. Request was:\n" + << requests[1]; +} + +TEST(CurlHTTPRequestManagerTests, failsARedirectChainThatNeverEnds) { + // One response more than kMaxRedirects permits follows, so the last has to be refused rather + // than followed. This pins the follow count, which the manager now keeps itself instead of + // leaving to CURLOPT_MAXREDIRS. + ScriptedServer server(std::vector(11, + "HTTP/1.1 302 Found\r\n" + "Location: /loop\r\n" + "Content-Length: 0\r\n" + "Connection: close\r\n" + "\r\n")); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGet(server.url("/loop").c_str()), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(30))); + EXPECT_TRUE(completion->error().has_value()) << "an endless redirect chain was reported as a response"; + EXPECT_EQ(server.requestLines().size(), 11u) + << "the original request plus ten follows is what kMaxRedirects allows"; +} + TEST(CurlHTTPRequestManagerTests, reportsALoopbackResponseWithoutWaitingOutThePollTimeout) { ScriptedServer server({"HTTP/1.1 200 OK\r\n" "Content-Length: 2\r\n" From 2f202ab950012e80bc3442f176ee9e56a0841dc1 Mon Sep 17 00:00:00 2001 From: tx3stn <14163530+tx3stn@users.noreply.github.com> Date: Fri, 21 Aug 2026 19:03:40 +0100 Subject: [PATCH 09/18] docs: add some docs around enable_http flag --- bzl/valdi/valdi_cli_application.bzl | 12 ++++++++++++ docs/docs/stdlib-http.md | 1 + docs/docs/workflow-cli-application.md | 23 +++++++++++++++++++++++ 3 files changed, 36 insertions(+) diff --git a/bzl/valdi/valdi_cli_application.bzl b/bzl/valdi/valdi_cli_application.bzl index 7626f32e8..c764e1633 100644 --- a/bzl/valdi/valdi_cli_application.bzl +++ b/bzl/valdi/valdi_cli_application.bzl @@ -11,6 +11,18 @@ def valdi_cli_application( visibility = ["//visibility:public"], enable_http = False, deps = []): + """ Builds a Valdi CLI application into a single self contained binary. + + Args: + name: The name of the generated binary. + script_path: The entry point script, as /. + visibility: The visibility of the Bazel target. + enable_http: Links a libcurl backed HTTP client into the binary, which valdi_http needs at + runtime. Off by default because it pulls in curl and BoringSSL, which an application + making no network requests should not have to carry. Left off, requests reject with + "No RequestManager set". + deps: The Valdi modules to include in the build. + """ main_target = "{}_main".format(name) expand_template( diff --git a/docs/docs/stdlib-http.md b/docs/docs/stdlib-http.md index d3b012069..06e365fa5 100644 --- a/docs/docs/stdlib-http.md +++ b/docs/docs/stdlib-http.md @@ -368,6 +368,7 @@ The `valdi_http` module works on: - ✅ iOS - ✅ Android - ✅ Web (via polyfill) +- ✅ CLI (requires `enable_http = True`, see [Building a CLI application](./workflow-cli-application.md#making-network-requests)) Network requests are always performed asynchronously and will not block the JavaScript thread. diff --git a/docs/docs/workflow-cli-application.md b/docs/docs/workflow-cli-application.md index 971c54ac7..0257c7012 100644 --- a/docs/docs/workflow-cli-application.md +++ b/docs/docs/workflow-cli-application.md @@ -27,5 +27,28 @@ valdi_cli_application( ) ``` +## Making network requests + +The [`valdi_http`](./stdlib-http.md) module needs a native HTTP client, and a CLI application only links one in when you ask for it. Set `enable_http = True` on `valdi_cli_application()` as well as adding `valdi_http` to your module's `deps`: + +```python +valdi_module( + name = "cli_example", + srcs = glob([ + "**/*.ts", + ]), + deps = ["//src/valdi_modules/src/valdi/valdi_http"], +) + +valdi_cli_application( + name = "cli_example_app", + script_path = "cli_example/index", + enable_http = True, + deps = [":cli_example"], +) +``` + +It is off by default because it links libcurl and BoringSSL into the binary, which an application that makes no network requests should not have to carry. Leaving it off still builds, and every request rejects at runtime with `No RequestManager set`. + You can then run your application using `valdi install cli`. You can package your application into a single binary using `valdi package cli`. \ No newline at end of file From 0370c880ac61ff5e2a20fdc967b75fa064ab2945 Mon Sep 17 00:00:00 2001 From: tx3stn <14163530+tx3stn@users.noreply.github.com> Date: Fri, 21 Aug 2026 23:33:01 +0100 Subject: [PATCH 10/18] fix: more test hardening --- apps/cli_http_example/index.ts | 43 +- .../HTTPRequestManagerModuleFactory.cpp | 9 +- .../CurlHTTPRequestManager.cpp | 233 ++++++++-- .../CurlHTTPRequestManager_tests.cpp | 433 +++++++++++++++++- 4 files changed, 636 insertions(+), 82 deletions(-) diff --git a/apps/cli_http_example/index.ts b/apps/cli_http_example/index.ts index 9d2807c84..953ea319f 100644 --- a/apps/cli_http_example/index.ts +++ b/apps/cli_http_example/index.ts @@ -1,21 +1,14 @@ -import { - beginKeepAlive, - endKeepAlive, -} from "valdi_core/src/utils/KeepAliveCallback"; -import { HTTPClient } from "valdi_http/src/HTTPClient"; -import { ArgumentsParser } from "valdi_standalone/src/ArgumentsParser"; -import { getStandaloneRuntime } from "valdi_standalone/src/ValdiStandalone"; +import { beginKeepAlive, endKeepAlive } from 'valdi_core/src/utils/KeepAliveCallback'; +import { HTTPClient } from 'valdi_http/src/HTTPClient'; +import { ArgumentsParser } from 'valdi_standalone/src/ArgumentsParser'; +import { getStandaloneRuntime } from 'valdi_standalone/src/ValdiStandalone'; -const DEFAULT_URL = "https://example.com"; +const DEFAULT_URL = 'https://example.com'; const standalone = getStandaloneRuntime(); -const parser = new ArgumentsParser("cli_http_example", standalone.arguments); -const urlArgument = parser.addString( - "--url", - `URL to fetch (default ${DEFAULT_URL})`, - false, -); +const parser = new ArgumentsParser('cli_http_example', standalone.arguments); +const urlArgument = parser.addString('--url', `URL to fetch (default ${DEFAULT_URL})`, false); parser.parse(); const url = urlArgument.value ?? DEFAULT_URL; @@ -25,15 +18,15 @@ const keepAlive = beginKeepAlive(); console.info(`GET ${url}`); new HTTPClient().get(url).then( - (response) => { - const length = response.body ? response.body.byteLength : 0; - console.info(`OK: status ${response.statusCode}, ${length} bytes`); - endKeepAlive(keepAlive); - standalone.exit(0); - }, - (error) => { - console.error(`FAIL: request rejected: ${error}`); - endKeepAlive(keepAlive); - standalone.exit(1); - }, + response => { + const length = response.body ? response.body.byteLength : 0; + console.info(`OK: status ${response.statusCode}, ${length} bytes`); + endKeepAlive(keepAlive); + standalone.exit(0); + }, + error => { + console.error(`FAIL: request rejected: ${error}`); + endKeepAlive(keepAlive); + standalone.exit(1); + }, ); diff --git a/src/valdi_modules/src/cpp/valdi_http/HTTPRequestManagerModuleFactory.cpp b/src/valdi_modules/src/cpp/valdi_http/HTTPRequestManagerModuleFactory.cpp index b8718aba4..a24570bb7 100644 --- a/src/valdi_modules/src/cpp/valdi_http/HTTPRequestManagerModuleFactory.cpp +++ b/src/valdi_modules/src/cpp/valdi_http/HTTPRequestManagerModuleFactory.cpp @@ -85,11 +85,10 @@ Value HTTPRequestManagerModuleFactory::loadModule() { parameters[1] = Value::undefined(); } else { parameters[0] = Value::undefined(); - // Pass the text, not the Error itself. Converting a Value holding an Error - // raises it into the exception tracker instead of marshalling an argument - // (JavaScriptUtils.cpp:337), so the callback would never run at all and the - // promise behind it would stay pending. PersistentStoreModuleFactory does - // the same. + // Pass the text, not the Error itself. valueToJSValue raises a + // ValueType::Error into the exception tracker instead of marshalling it as + // an argument, so the callback would never run and the promise behind it + // would stay pending. PersistentStoreModuleFactory does the same. parameters[1] = Value(result.error().toString()); } diff --git a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp index 52ab52f60..2e44ba1fc 100644 --- a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp +++ b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp @@ -10,15 +10,17 @@ #include "valdi_core/cpp/Utils/Result.hpp" #include "valdi_core/cpp/Utils/StringCache.hpp" #include "valdi_core/cpp/Utils/Value.hpp" +#include "valdi_core/cpp/Utils/ValueMap.hpp" #include #include -#include #include #include #include +#include #include +#include #include #include #include @@ -30,9 +32,8 @@ namespace { constexpr int kMaxRedirects = 10; constexpr long kConnectTimeoutSeconds = 30; -// curl_multi_poll waits for the shorter of this and the multi handle's own next timer, so an -// active transfer is still serviced on curl's schedule. This only bounds how long an idle thread -// sits before rechecking. New work and shutdown both wake the poll, so a short value buys nothing. +// Only bounds how long an idle thread sits: curl_multi_poll returns at the sooner of this and the +// multi handle's own next timer, and new work, cancellation and shutdown all wake it early. constexpr int kPollTimeoutMs = 10000; // The curl command line tool reads these, libcurl does not, so we honour them here to let a @@ -42,9 +43,8 @@ const char* const kCaBundleVariables[] = { "SSL_CERT_FILE", }; -// Only distributions the @curl build defaults miss. @curl already compiles CURL_CA_BUNDLE with -// the macOS and Debian-family paths (curl+/BUILD.bazel:341-346), so libcurl applies those itself, -// and probing for them here would override a deliberate --@curl//:ca_bundle. +// Only the distributions @curl's own compiled-in CURL_CA_BUNDLE misses, since libcurl applies that +// itself and probing for the same paths here would override a deliberate --@curl//:ca_bundle. const char* const kCaBundleCandidates[] = { "/etc/pki/tls/certs/ca-bundle.crt", // RHEL, Fedora "/etc/ssl/ca-bundle.pem", // openSUSE @@ -72,23 +72,67 @@ std::string resolveCaBundle(const StringBox& configured) { return {}; } +// Always a map, never null: HTTPTypes.d.ts declares headers as StringMap, and iOS, Android +// and web all hand back {} when a response carried none. +Value emptyHeaderMap() { + return Value(makeShared()); +} + +// A cancelable can outlive the manager, since JavaScript may hold one and cancel after teardown. +// Tasks reach the multi handle through this rather than a back-pointer: the manager clears it once +// its thread has joined and before curl_multi_cleanup, so a late cancel finds nothing to poke. +class PollWaker { +public: + explicit PollWaker(CURLM* multi) : _multi(multi) {} + + void wake() { + std::lock_guard guard(_mutex); + if (_multi != nullptr) { + curl_multi_wakeup(_multi); + } + } + + void detach() { + std::lock_guard guard(_mutex); + _multi = nullptr; + } + +private: + std::mutex _mutex; + CURLM* _multi; +}; + class CurlTask : public snap::valdi_core::Cancelable { public: CurlTask(snap::valdi_core::HTTPRequest request, - std::shared_ptr completion) - : request(std::move(request)), _completion(std::move(completion)) {} + std::shared_ptr completion, + std::shared_ptr waker) + : request(std::move(request)), _completion(std::move(completion)), _waker(std::move(waker)) { + // The caller's body views a live JavaScript ArrayBuffer, so it is copied here, on the thread + // that called performRequest. Reading it from the curl thread would race whatever + // JavaScript writes next. The view is then dropped so nothing downstream can reach for it. + if (this->request.body) { + const auto& body = this->request.body.value(); + requestBody = makeShared(body.begin(), body.end()); + this->request.body.reset(); + } + } void cancel() override { + // Dropped before the flag goes up, because the curl thread turns an aborted transfer into a + // "Request was cancelled" failure and must not find a completion still attached. + dropCompletion(); + cancelled.store(true); - // A cancelled request reports nothing back, matching the iOS and Android managers. - dropCompletion(); + // Nothing reads that flag until curl next runs the progress callback, and an idle transfer + // leaves the poll parked for up to kPollTimeoutMs. Waking it frees the socket now. + _waker->wake(); } - // Shutdown uses this too. Completions reach JavaScript directly with no thread hop, so firing - // one from the curl thread while another thread sits in join() destroying the manager would - // enter the engine twice over. Neither platform manager promises a completion at teardown, so - // there is nothing to report. + // Used by cancel() and by shutdown. Neither the iOS nor the Android manager promises a + // completion for a cancelled or torn-down request, so there is nothing to report. Dropping is + // the contract, not a thread-safety measure: a JS backed completion dispatches to the JS thread. void dropCompletion() { std::lock_guard guard(_mutex); _completion = nullptr; @@ -118,17 +162,21 @@ class CurlTask : public snap::valdi_core::Cancelable { int redirects = 0; std::atomic_bool cancelled{false}; + // Null when the request has no body. This owns the payload for the life of the task, which + // outlives every easy handle made from it, so curl can read it in place. + Ref requestBody; + // The write callback fills this and the response takes it directly, so the payload is never - // copied. ByteBuffer grows to the next power of two, so appending stays amortised constant - // time with no reserve up front. Reserving would size an allocation from a Content-Length the - // server chose. + // copied. No reserve up front: that would size an allocation from a Content-Length the server + // chose. Ref responseBody = makeShared(); - Value responseHeaders; + Value responseHeaders = emptyHeaderMap(); curl_slist* requestHeaders = nullptr; private: std::mutex _mutex; std::shared_ptr _completion; + std::shared_ptr _waker; }; size_t writeBodyCallback(char* data, size_t size, size_t count, void* userData) { @@ -146,7 +194,7 @@ size_t writeHeaderCallback(char* data, size_t size, size_t count, void* userData // contain one. The reset discards anything an informational response carried, since curl hands // a 1xx header block to this callback too and a 103 Early Hints brings real headers with it. if (line.rfind("HTTP/", 0) == 0) { - task->responseHeaders = Value(); + task->responseHeaders = emptyHeaderMap(); return size * count; } @@ -166,9 +214,8 @@ size_t writeHeaderCallback(char* data, size_t size, size_t count, void* userData value.pop_back(); } - // Join repeated headers, matching what NSURLResponse hands back. The response header map is - // string to string, so earlier values have nowhere else to go, and dropping them loses whole - // Set-Cookie lines. + // Join repeated headers as NSURLResponse does. The map is string to string, so dropping earlier + // values would lose whole Set-Cookie lines. auto existing = task->responseHeaders.getMapValue(std::string_view(name)); if (!existing.isNullOrUndefined()) { value = std::string(existing.toStringBox().toStringView()) + ", " + value; @@ -179,6 +226,63 @@ size_t writeHeaderCallback(char* data, size_t size, size_t count, void* userData return size * count; } +// curl writes custom headers and a custom request line out verbatim, and curl_slist_append takes a +// const char*, so a CR or LF injects a header or a whole second request, and a NUL truncates +// silently. Rejected rather than stripped, so a request never quietly means something else. +bool hasRequestControlCharacters(std::string_view text) { + return text.find_first_of(std::string_view("\r\n\0", 3)) != std::string_view::npos; +} + +// The message to fail with, or nullopt when every field is safe to serialize. +std::optional findUnserializableField(const snap::valdi_core::HTTPRequest& request) { + if (hasRequestControlCharacters(request.method.toStringView())) { + return "The request method contains a carriage return, newline or NUL"; + } + + for (const auto& key : request.headers.sortedMapKeys()) { + // Deliberately not naming it: the name is the thing carrying the newline. + if (hasRequestControlCharacters(key.toStringView())) { + return "A request header name contains a carriage return, newline or NUL"; + } + + if (hasRequestControlCharacters(request.headers.getMapValue(key).toStringBox().toStringView())) { + return "The value of request header " + std::string(key.toStringView()) + + " contains a carriage return, newline or NUL"; + } + } + + return std::nullopt; +} + +bool equalsIgnoringCase(std::string_view value, std::string_view lowercase) { + return value.size() == lowercase.size() && strncasecmp(value.data(), lowercase.data(), value.size()) == 0; +} + +// This build has no compression codecs, so curl neither asks for an encoded response nor checks +// whether it got one: it only parses Content-Encoding when CURLOPT_ACCEPT_ENCODING is set. Matched +// case-insensitively, since the casing is the origin's and nothing here canonicalizes it. +std::optional undecodableContentEncoding(const Value& headers) { + if (!headers.isMap()) { + return std::nullopt; + } + + for (const auto& entry : *headers.getMap()) { + if (!equalsIgnoringCase(entry.first.toStringView(), "content-encoding")) { + continue; + } + + auto encoding = std::string(entry.second.toStringBox().toStringView()); + + // identity is a no-op, and the only codec this build has. + if (encoding.empty() || equalsIgnoringCase(encoding, "identity")) { + return std::nullopt; + } + return encoding; + } + + return std::nullopt; +} + int progressCallback(void* userData, curl_off_t, curl_off_t, curl_off_t, curl_off_t) { auto* task = static_cast(userData); return task->cancelled.load() ? 1 : 0; @@ -198,6 +302,7 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { if (_multi == nullptr) { return; } + _waker = std::make_shared(_multi); _thread = std::thread([this]() { run(); }); } @@ -210,13 +315,16 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { if (_thread.joinable()) { _thread.join(); } + // After the join, so the run loop still has a handle to poll, and before the cleanup, so a + // cancel arriving from JavaScript from here on finds nothing rather than a freed handle. + _waker->detach(); curl_multi_cleanup(_multi); } std::shared_ptr performRequest( const snap::valdi_core::HTTPRequest& request, const std::shared_ptr& completion) override { - auto task = std::make_shared(request, completion); + auto task = std::make_shared(request, completion, _waker); // Check this before the multi handle, so the reported cause is the one that actually failed. if (_globalInit != CURLE_OK) { @@ -299,38 +407,45 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { } void addTask(const std::shared_ptr& task) { + const auto& request = task->request; + + // Checked before a handle exists, so a rejected request leaves nothing to clean up. + if (auto rejection = findUnserializableField(request)) { + task->complete(Error(StringBox::fromString(*rejection))); + return; + } + auto* easy = curl_easy_init(); if (easy == nullptr) { task->complete(Error(STRING_LITERAL("Failed to create a curl handle"))); return; } - const auto& request = task->request; - curl_easy_setopt(easy, CURLOPT_URL, std::string(request.url.toStringView()).c_str()); - if (request.body) { - const auto& body = request.body.value(); - curl_easy_setopt(easy, CURLOPT_POSTFIELDSIZE_LARGE, static_cast(body.size())); - curl_easy_setopt(easy, CURLOPT_COPYPOSTFIELDS, reinterpret_cast(body.data())); + if (task->requestBody != nullptr) { + curl_easy_setopt(easy, CURLOPT_POSTFIELDSIZE_LARGE, static_cast(task->requestBody->size())); + // Not COPYPOSTFIELDS: the task already owns this buffer for longer than the handle + // lives, so letting curl copy it again would hold the payload twice. + curl_easy_setopt(easy, CURLOPT_POSTFIELDS, reinterpret_cast(task->requestBody->data())); } - // Methods curl models itself are set through their own options so that its redirect - // handling knows what the request is. CURLOPT_CUSTOMREQUEST only rewrites the request - // line and leaves behaviour alone, which is why it is reserved for the verbs curl has no - // option for. + // Methods curl models itself go through their own options, so it sets the transfer up to + // match: CURLOPT_POST arranges a body reader, CURLOPT_NOBODY suppresses reading one. + // CUSTOMREQUEST only rewrites the request line, so it is left for verbs curl has no option + // for. auto method = std::string(request.method.toStringView()); if (method == "HEAD") { curl_easy_setopt(easy, CURLOPT_NOBODY, 1L); } else if (method == "POST") { curl_easy_setopt(easy, CURLOPT_POST, 1L); - if (!request.body) { + if (task->requestBody == nullptr) { // Without fields curl reads the body from the read callback, which is stdin. curl_easy_setopt(easy, CURLOPT_POSTFIELDSIZE_LARGE, static_cast(0)); curl_easy_setopt(easy, CURLOPT_COPYPOSTFIELDS, ""); } } else if (method.empty() || method == "GET") { - if (request.body) { + if (task->requestBody != nullptr) { // The fields above turned this into a POST; name GET to keep the request line. curl_easy_setopt(easy, CURLOPT_CUSTOMREQUEST, "GET"); } else { @@ -341,8 +456,15 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { } for (const auto& key : request.headers.sortedMapKeys()) { - auto header = std::string(key.toStringView()) + ": " + - std::string(request.headers.getMapValue(key).toStringBox().toStringView()); + auto value = std::string(request.headers.getMapValue(key).toStringBox().toStringView()); + + // curl steps over the whitespace after a colon and then drops an empty valued header + // entirely. Its "name;" form is the documented way to send one, and iOS and web both + // do. The blank set mirrors curl's ISSPACE, so this catches exactly what it would drop. + auto blank = value.find_first_not_of(" \t\n\v\f\r") == std::string::npos; + auto header = blank ? std::string(key.toStringView()) + ";" + : std::string(key.toStringView()) + ": " + value; + task->requestHeaders = curl_slist_append(task->requestHeaders, header.c_str()); } if (task->requestHeaders != nullptr) { @@ -361,9 +483,8 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { curl_easy_setopt(easy, CURLOPT_NOSIGNAL, 1L); // Without this, abandoning a name lookup joins the resolver thread, and getaddrinfo cannot - // be interrupted. Cancelling or shutting down mid-lookup would block this thread until the - // resolver gave up, taking every other request with it. curl detaches the thread instead, - // and it frees its own state once the lookup returns. + // be interrupted, so a cancel or shutdown mid-lookup would block this thread and every + // other request with it. curl detaches the thread instead and it frees its own state. curl_easy_setopt(easy, CURLOPT_QUICK_EXIT, 1L); if (_idleTimeoutSeconds > 0) { @@ -385,11 +506,11 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { _active.emplace(easy, task); } - // Redirects are followed here rather than by CURLOPT_FOLLOWLOCATION, because that option keeps - // CURLOPT_CUSTOMREQUEST for the whole chain: Curl_http_method takes the request line from it - // unconditionally, and the redirect handlers only ever touch curl's own idea of the method. A - // DELETE would stay a DELETE across a 303 that has to become a GET, and a PUT would keep its - // verb while curl dropped the body out from under it. The rewriting below is RFC 9110 15.4. + // Followed here rather than by CURLOPT_FOLLOWLOCATION, which keeps CURLOPT_CUSTOMREQUEST for + // the whole chain: Curl_http_method takes the request line from it unconditionally while the + // redirect handlers only touch curl's own idea of the method, so a DELETE would stay a DELETE + // across a 303 and a PUT would keep its verb while curl dropped the body. Rewriting per RFC + // 9110 15.4. static void retarget(CurlTask& task, long statusCode, const char* location) { task.request.url = StringBox::fromCString(location); @@ -398,13 +519,13 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { : ((statusCode == 301 || statusCode == 302) && method == "POST"); if (toGet) { task.request.method = STRING_LITERAL("GET"); - task.request.body.reset(); + task.requestBody = nullptr; } // curl only withholds a redirect's body from the write callback when it is following the // redirect itself, so this hop's is ours to discard. task.responseBody->clear(); - task.responseHeaders = Value(); + task.responseHeaders = emptyHeaderMap(); task.redirects++; } @@ -448,6 +569,20 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { continue; } + // Nothing to decode if nothing arrived, which is also what keeps a HEAD or a 204 + // against a compressing origin from being reported as a failure. + if (!task->responseBody->empty()) { + if (auto encoding = undecodableContentEncoding(task->responseHeaders)) { + finish(easy, + task, + Error(StringBox::fromString( + "Cannot decode a response sent with Content-Encoding: " + *encoding + + ". This build has no decompression support, so an Accept-Encoding request " + "header must not be set."))); + continue; + } + } + snap::valdi_core::HTTPResponse response(static_cast(statusCode), task->responseHeaders, {task->responseBody->toBytesView()}); @@ -483,6 +618,8 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { int32_t _idleTimeoutSeconds = 0; CURLcode _globalInit = CURLE_OK; CURLM* _multi = nullptr; + // Never null, so that a cancel is safe whether or not the run loop ever came up. + std::shared_ptr _waker = std::make_shared(nullptr); std::thread _thread; std::mutex _mutex; bool _stopping = false; diff --git a/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp b/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp index d37f38a63..15b1ead15 100644 --- a/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp +++ b/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -431,6 +432,18 @@ snap::valdi_core::HTTPRequest makeGet(const char* url) { return makeRequest("GET", url); } +Value makeHeaders(std::initializer_list> entries) { + Value headers; + for (const auto& entry : entries) { + headers.setMapValue(std::string_view(entry.first), Value(StringBox::fromCString(entry.second))); + } + return headers; +} + +snap::valdi_core::HTTPRequest makeGetWithHeaders(const char* url, const Value& headers) { + return snap::valdi_core::HTTPRequest(StringBox::fromCString(url), STRING_LITERAL("GET"), headers, std::nullopt, 0); +} + snap::valdi_core::HTTPRequest makeRequestWithBody(const char* method, const char* url, const char* body) { return snap::valdi_core::HTTPRequest(StringBox::fromCString(url), StringBox::fromCString(method), @@ -576,6 +589,409 @@ TEST(CurlHTTPRequestManagerTests, sendsAnEmptyButPresentBodyAsAnEmptyPost) { << requests[0]; } +// A canned 200 for tests that only care about what went out on the request. +std::string okResponse() { + return "HTTP/1.1 200 OK\r\n" + "Content-Length: 2\r\n" + "Connection: close\r\n" + "\r\n" + "ok"; +} + +TEST(CurlHTTPRequestManagerTests, reportsAnEmptyMapForAResponseWithNoHeaders) { + // No colon anywhere but the status line, which is the only way the header map is never promoted + // from null. 204 so that curl needs no Content-Length, since any header added to help would + // itself carry a colon and defeat the test. RFC 9110 makes Date a MUST for an origin with a + // clock, so this shape comes from hand rolled and embedded servers rather than mainstream ones. + ScriptedServer server({"HTTP/1.1 204 No Content\r\n\r\n"}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGet(server.url("/").c_str()), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + ASSERT_EQ(completion->statusCode(), 204); + + auto headers = completion->headers(); + EXPECT_FALSE(headers.isNullOrUndefined()) + << "headers reached JavaScript as null, but HTTPTypes.d.ts declares them as a " + "StringMap and iOS, Android and web all hand back {}"; + EXPECT_TRUE(headers.isMap()) << "headers must always be a map, even for a response that carried none"; +} + +TEST(CurlHTTPRequestManagerTests, sendsARequestHeader) { + ScriptedServer server({okResponse()}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGetWithHeaders(server.url("/").c_str(), makeHeaders({{"X-Test", "value"}})), + completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + ASSERT_EQ(completion->statusCode(), 200); + + auto requests = server.requests(); + ASSERT_EQ(requests.size(), 1u); + EXPECT_NE(requests[0].find("X-Test: value\r\n"), std::string::npos) + << "a caller header never reached the wire, or was not formatted as \"name: value\". Request was:\n" + << requests[0]; +} + +TEST(CurlHTTPRequestManagerTests, sendsHeadersInAnOrderThatDoesNotDependOnInsertion) { + ScriptedServer server({okResponse()}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + // Inserted in an order that is neither alphabetical nor reverse alphabetical, so agreeing with + // the map's own iteration order by chance is unlikely across five keys. + manager->performRequest(makeGetWithHeaders(server.url("/").c_str(), + makeHeaders({{"X-Delta", "4"}, + {"X-Alpha", "1"}, + {"X-Echo", "5"}, + {"X-Bravo", "2"}, + {"X-Charlie", "3"}})), + completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + ASSERT_EQ(completion->statusCode(), 200); + + auto requests = server.requests(); + ASSERT_EQ(requests.size(), 1u); + + std::vector positions; + for (const auto* header : {"X-Alpha: 1\r\n", "X-Bravo: 2\r\n", "X-Charlie: 3\r\n", "X-Delta: 4\r\n", "X-Echo: 5\r\n"}) { + auto at = requests[0].find(header); + EXPECT_NE(at, std::string::npos) << header << " never reached the wire. Request was:\n" << requests[0]; + positions.push_back(at); + } + + // sortedMapKeys imposes this. The guarantee worth having is that the bytes do not depend on the + // order the caller happened to build the map in, and sorting is how that is achieved. + EXPECT_TRUE(std::is_sorted(positions.begin(), positions.end())) + << "headers were not sent in sorted order, so the request bytes depend on map iteration " + "order. Request was:\n" + << requests[0]; +} + +TEST(CurlHTTPRequestManagerTests, overridesACurlDefaultHeader) { + ScriptedServer server({okResponse()}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest( + makeGetWithHeaders(server.url("/").c_str(), makeHeaders({{"Accept", "application/json"}})), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + ASSERT_EQ(completion->statusCode(), 200); + + auto requests = server.requests(); + ASSERT_EQ(requests.size(), 1u); + EXPECT_NE(requests[0].find("Accept: application/json\r\n"), std::string::npos) + << "the caller's Accept did not reach the wire. Request was:\n" + << requests[0]; + EXPECT_EQ(requests[0].find("Accept: */*"), std::string::npos) + << "curl's own Accept was sent alongside the caller's, so the server sees two. Request was:\n" + << requests[0]; +} + +TEST(CurlHTTPRequestManagerTests, coercesNonStringHeaderValues) { + ScriptedServer server({okResponse()}); + + // JavaScript is not held to HTTPTypes.d.ts, so a number or a boolean can arrive here. The loop + // runs every value through toStringBox, and this pins what that produces. + Value headers; + headers.setMapValue(std::string_view("X-Count"), Value(42)); + headers.setMapValue(std::string_view("X-Flag"), Value(true)); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGetWithHeaders(server.url("/").c_str(), headers), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + ASSERT_EQ(completion->statusCode(), 200); + + auto requests = server.requests(); + ASSERT_EQ(requests.size(), 1u); + EXPECT_NE(requests[0].find("X-Count: 42\r\n"), std::string::npos) + << "a numeric header value was not coerced to its digits. Request was:\n" + << requests[0]; + EXPECT_NE(requests[0].find("X-Flag: true\r\n"), std::string::npos) + << "a boolean header value was not coerced to true/false. Request was:\n" + << requests[0]; +} + +// curl writes custom headers and a custom request line verbatim, so a CR or LF in caller-supplied +// text ends one header and starts another. With two of them it ends the whole request and starts a +// second one on the same connection. A NUL truncates instead, since curl_slist_append takes a +// const char*. None of it must reach the wire. +TEST(CurlHTTPRequestManagerTests, rejectsAHeaderValueContainingCrlf) { + ScriptedServer server({okResponse()}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest( + makeGetWithHeaders(server.url("/").c_str(), makeHeaders({{"X-Test", "a\r\nX-Evil: 1"}})), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + EXPECT_TRUE(completion->error().has_value()) << "a header value carrying CRLF was accepted"; + EXPECT_TRUE(server.requestLines().empty()) + << "the request went out anyway, so a caller-supplied string injected a header"; +} + +TEST(CurlHTTPRequestManagerTests, rejectsAHeaderValueThatWouldSplitTheRequest) { + ScriptedServer server({okResponse()}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest( + makeGetWithHeaders(server.url("/").c_str(), + makeHeaders({{"X-Test", "a\r\n\r\nGET /smuggled HTTP/1.1\r\nHost: evil\r\n"}})), + completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + EXPECT_TRUE(completion->error().has_value()) << "a header value that ends the request was accepted"; + EXPECT_TRUE(server.requestLines().empty()) + << "a second request was written onto the connection from a header value"; +} + +TEST(CurlHTTPRequestManagerTests, rejectsAHeaderValueContainingANul) { + ScriptedServer server({okResponse()}); + + Value headers; + headers.setMapValue(std::string_view("X-Test"), Value(StringBox::fromString(std::string("a\0b", 3)))); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGetWithHeaders(server.url("/").c_str(), headers), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + EXPECT_TRUE(completion->error().has_value()) + << "a NUL in a header value truncates it at the C string boundary, which must not pass silently"; + EXPECT_TRUE(server.requestLines().empty()) << "a truncated header value was sent"; +} + +TEST(CurlHTTPRequestManagerTests, rejectsAHeaderNameContainingCrlf) { + ScriptedServer server({okResponse()}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest( + makeGetWithHeaders(server.url("/").c_str(), makeHeaders({{"X-Test\r\nX-Evil", "1"}})), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + EXPECT_TRUE(completion->error().has_value()) << "a header name carrying CRLF was accepted"; + EXPECT_TRUE(server.requestLines().empty()) << "the request went out anyway"; +} + +TEST(CurlHTTPRequestManagerTests, rejectsAMethodContainingCrlf) { + ScriptedServer server({okResponse()}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeRequest("PURGE\r\nX-Evil: 1", server.url("/").c_str()), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + EXPECT_TRUE(completion->error().has_value()) << "a method carrying CRLF was accepted"; + EXPECT_TRUE(server.requestLines().empty()) << "the request line was split across two lines on the wire"; +} + +TEST(CurlHTTPRequestManagerTests, rejectsAUrlContainingCrlf) { + // curl parses the URL itself, so this is a guard on curl keeping that promise rather than on + // anything this manager does. Pointed at a server that would otherwise answer, so passing on a + // connection failure instead of a rejected URL is not possible. + ScriptedServer server({okResponse()}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGet((server.url("/") + "\r\nX-Evil: 1").c_str()), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + EXPECT_TRUE(completion->error().has_value()) << "a URL carrying CRLF was accepted"; + EXPECT_TRUE(server.requestLines().empty()) << "the request reached the wire with CRLF in its target"; +} + +TEST(CurlHTTPRequestManagerTests, sendsAnEmptyValuedRequestHeader) { + ScriptedServer server({okResponse()}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGetWithHeaders(server.url("/").c_str(), makeHeaders({{"X-Trace", ""}})), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + ASSERT_EQ(completion->statusCode(), 200); + + auto requests = server.requests(); + ASSERT_EQ(requests.size(), 1u); + EXPECT_NE(requests[0].find("X-Trace:\r\n"), std::string::npos) + << "an empty valued header was dropped, so the same JavaScript sends different bytes here " + "than it does on iOS and web. Request was:\n" + << requests[0]; +} + +TEST(CurlHTTPRequestManagerTests, sendsAWhitespaceOnlyRequestHeaderAsEmpty) { + ScriptedServer server({okResponse()}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGetWithHeaders(server.url("/").c_str(), makeHeaders({{"X-Trace", " "}})), + completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + ASSERT_EQ(completion->statusCode(), 200); + + auto requests = server.requests(); + ASSERT_EQ(requests.size(), 1u); + EXPECT_NE(requests[0].find("X-Trace:\r\n"), std::string::npos) + << "curl drops a whitespace only value just as it drops an empty one, since it steps over " + "the whitespace before testing. Request was:\n" + << requests[0]; +} + +TEST(CurlHTTPRequestManagerTests, sendsAnEmptyValuedOverrideOfACurlDefault) { + ScriptedServer server({okResponse()}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGetWithHeaders(server.url("/").c_str(), makeHeaders({{"Accept", ""}})), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + ASSERT_EQ(completion->statusCode(), 200); + + auto requests = server.requests(); + ASSERT_EQ(requests.size(), 1u); + EXPECT_EQ(requests[0].find("Accept: */*"), std::string::npos) + << "curl's own default was sent even though the caller overrode Accept. Request was:\n" + << requests[0]; + EXPECT_NE(requests[0].find("Accept:\r\n"), std::string::npos) + << "overriding a header curl generates itself with an empty value loses it twice over: " + "curl suppresses its default because a custom one exists, then drops the custom one. " + "Request was:\n" + << requests[0]; +} + +TEST(CurlHTTPRequestManagerTests, failsAResponseItCannotDecode) { + ScriptedServer server({"HTTP/1.1 200 OK\r\n" + "Content-Encoding: gzip\r\n" + "Content-Length: 3\r\n" + "Connection: close\r\n" + "\r\n" + "\x1f\x8b\x08"}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGet(server.url("/").c_str()), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + ASSERT_TRUE(completion->error().has_value()) + << "a body this build cannot decode was handed back as a successful response"; + EXPECT_NE(completion->error().value().find("gzip"), std::string::npos) + << "the failure should name the encoding it could not decode, but said: " << completion->error().value(); +} + +TEST(CurlHTTPRequestManagerTests, failsAResponseItCannotDecodeWhateverTheHeaderCasing) { + // Casing is the origin's choice and nothing here canonicalizes it, so the check has to be + // insensitive on the header name and on its value. + ScriptedServer server({"HTTP/1.1 200 OK\r\n" + "content-encoding: GZIP\r\n" + "Content-Length: 3\r\n" + "Connection: close\r\n" + "\r\n" + "\x1f\x8b\x08"}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGet(server.url("/").c_str()), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + ASSERT_TRUE(completion->error().has_value()) + << "a lowercase Content-Encoding went unnoticed, so the response was handed back undecoded"; + EXPECT_NE(completion->error().value().find("GZIP"), std::string::npos) + << "failed, but not over the encoding, so this passes for the wrong reason: " + << completion->error().value(); +} + +TEST(CurlHTTPRequestManagerTests, acceptsAnIdentityContentEncoding) { + ScriptedServer server({"HTTP/1.1 200 OK\r\n" + "Content-Encoding: identity\r\n" + "Content-Length: 2\r\n" + "Connection: close\r\n" + "\r\n" + "ok"}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGet(server.url("/").c_str()), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + EXPECT_FALSE(completion->error().has_value()) + << "identity means the body is unencoded, so there is nothing to reject: " + << completion->error().value_or(""); + EXPECT_EQ(completion->statusCode(), 200); + EXPECT_EQ(completion->bodySize(), 2u); +} + +TEST(CurlHTTPRequestManagerTests, ignoresContentEncodingOnABodilessResponse) { + // A HEAD against an origin that compresses still advertises the encoding the body would have + // had. There is no body to decode, so it must not be treated as a failure. + ScriptedServer server({"HTTP/1.1 200 OK\r\n" + "Content-Encoding: gzip\r\n" + "Content-Length: 569\r\n" + "Connection: close\r\n" + "\r\n"}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeRequest("HEAD", server.url("/").c_str()), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + EXPECT_FALSE(completion->error().has_value()) + << "a HEAD carries no body, so its advertised encoding is nothing to reject: " + << completion->error().value_or(""); + EXPECT_EQ(completion->statusCode(), 200); +} + +TEST(CurlHTTPRequestManagerTests, sendsTheBodyAsItWasWhenTheRequestWasMade) { + // Big enough to span several socket writes, so a body streamed out of the caller's memory shows + // the overwrite partway through rather than not at all, and under curl's 1 MiB Expect: + // 100-continue threshold, which a ScriptedServer would never answer. + constexpr size_t kBodySize = 512 * 1024; + + ScriptedServer server({"HTTP/1.1 200 OK\r\n" + "Content-Length: 2\r\n" + "Connection: close\r\n" + "\r\n" + "ok"}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + + // The body a caller hands over is a view over a live JavaScript ArrayBuffer, and JavaScript is + // free to write to that array the moment performRequest returns. Overwriting it here stands in + // for that, so what reaches the wire has to be what the request was made with. + auto source = makeShared(std::string(kBodySize, 'a')); + manager->performRequest(snap::valdi_core::HTTPRequest(StringBox::fromCString(server.url("/submit").c_str()), + STRING_LITERAL("POST"), + Value(), + source->toBytesView(), + 0), + completion); + std::memset(source->data(), 'b', source->size()); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(30))); + ASSERT_EQ(completion->statusCode(), 200); + + auto requests = server.requests(); + ASSERT_EQ(requests.size(), 1u); + + auto body = requests[0].substr(requests[0].find("\r\n\r\n") + 4); + ASSERT_EQ(body.size(), kBodySize) << "the whole body did not arrive, so there is nothing to judge"; + + auto unchanged = static_cast(std::count(body.begin(), body.end(), 'a')); + EXPECT_EQ(unchanged, kBodySize) << unchanged << " of " << kBodySize + << " body bytes were the ones the request was made with; the rest were read " + "on the curl thread, after performRequest had already returned to its caller"; +} + TEST(CurlHTTPRequestManagerTests, followsASeeOtherWithGet) { ScriptedServer server({"HTTP/1.1 303 See Other\r\n" "Location: /final\r\n" @@ -962,13 +1378,23 @@ TEST(CurlHTTPRequestManagerTests, cancellingARequestDropsItsCompletion) { TEST(CurlHTTPRequestManagerTests, abortsACancelledTransferWithoutWaitingOutThePollTimeout) { StallServer server; - auto manager = makeCurlHTTPRequestManager(); + // The idle timeout off, so curl arms no timer of its own and the poll really does sit for + // kPollTimeoutMs. Under the sixty second default, CURLOPT_LOW_SPEED_TIME has curl re-arm a one + // second timer that caps the poll, which hides all but a second of the wait and makes this a + // coin toss rather than a measurement. + auto manager = makeCurlHTTPRequestManager(StringBox(), 0); auto completion = std::make_shared(); auto cancelable = manager->performRequest(makeGet(server.url().c_str()), completion); ASSERT_TRUE(server.waitForConnection(std::chrono::seconds(5))) << "curl never connected, so there is no live transfer to cancel"; + // waitForConnection returns at accept(), which is the same event that makes curl's socket + // writable and ends its poll. Cancelling straight away is therefore observed by a + // curl_multi_perform pass that was going to run anyway. Settling first is what puts the curl + // thread back inside curl_multi_poll, which is the state this test is about. + std::this_thread::sleep_for(std::chrono::milliseconds(300)); + auto start = std::chrono::steady_clock::now(); cancelable->cancel(); ASSERT_TRUE(server.waitForDisconnect(std::chrono::seconds(30))) @@ -1044,9 +1470,8 @@ TEST(CurlHTTPRequestManagerTests, shutdownCancelsRequestsInFlight) { EXPECT_EQ(finished.wait_for(std::chrono::seconds(5)), std::future_status::ready) << "~CurlHTTPRequestManager waited for the in-flight transfer instead of cancelling it"; EXPECT_FALSE(completion->waitForCompletion(std::chrono::seconds(1))) - << "shutdown reported an in-flight request. Completions reach JavaScript with no thread " - "hop, so firing one from the curl thread while the destroying thread is inside join() " - "enters the engine from two threads at once"; + << "shutdown reported an in-flight request. Neither the iOS nor the Android manager promises " + "a completion at teardown, so it should be dropped, the same as a cancellation"; } } // namespace ValdiTest From 9628105837bc94eb3de09dfdb3788332232aab2f Mon Sep 17 00:00:00 2001 From: tx3stn <14163530+tx3stn@users.noreply.github.com> Date: Thu, 27 Aug 2026 23:39:19 +0100 Subject: [PATCH 11/18] fix: review findings --- bzl/valdi/app_templates/BUILD.bazel | 1 + bzl/valdi/app_templates/cli_main.cpp.tpl | 5 +- bzl/valdi/app_templates/cli_main_http.cpp.tpl | 7 + bzl/valdi/valdi_cli_application.bzl | 11 +- .../src/valdi/valdi_http/src/HTTPClient.ts | 9 +- valdi/src/valdi/cli_runner/CLIRunner.hpp | 4 +- .../runtime/JavaScript/JavaScriptUtils.cpp | 6 - valdi/src/valdi/standalone_http/CaStore.cpp | 60 ++++ valdi/src/valdi/standalone_http/CaStore.hpp | 32 ++ .../CurlHTTPRequestManager.cpp | 252 ++++++++------- .../CurlHTTPRequestManager.hpp | 9 +- valdi/test/standalone/CaStore_tests.cpp | 149 +++++++++ .../CurlHTTPRequestManager_tests.cpp | 289 ++++++++++++++++-- .../StandaloneRequestManager_tests.cpp | 36 +++ 14 files changed, 711 insertions(+), 159 deletions(-) create mode 100644 bzl/valdi/app_templates/cli_main_http.cpp.tpl create mode 100644 valdi/src/valdi/standalone_http/CaStore.cpp create mode 100644 valdi/src/valdi/standalone_http/CaStore.hpp create mode 100644 valdi/test/standalone/CaStore_tests.cpp diff --git a/bzl/valdi/app_templates/BUILD.bazel b/bzl/valdi/app_templates/BUILD.bazel index 41fac804f..613b9c2b8 100644 --- a/bzl/valdi/app_templates/BUILD.bazel +++ b/bzl/valdi/app_templates/BUILD.bazel @@ -7,5 +7,6 @@ exports_files([ "AndroidLibManifest.xml", "StartActivity.kt.tpl", "cli_main.cpp.tpl", + "cli_main_http.cpp.tpl", "linux_main.cpp.tpl", ]) diff --git a/bzl/valdi/app_templates/cli_main.cpp.tpl b/bzl/valdi/app_templates/cli_main.cpp.tpl index b6ec14a2f..9a38074ff 100644 --- a/bzl/valdi/app_templates/cli_main.cpp.tpl +++ b/bzl/valdi/app_templates/cli_main.cpp.tpl @@ -1,6 +1,7 @@ +// Paired with cli_main_http.cpp.tpl, which valdi_cli_application picks when enable_http is set. #include "valdi/cli_runner/CLIRunner.hpp" -@VALDI_HTTP_INCLUDE@ int main(int argc, const char** argv) { - return Valdi::valdiCLIRun("@VALDI_SCRIPT_PATH@", argc, argv@VALDI_HTTP_MANAGER@); + // No backend, so valdi_http rejects every request with "No RequestManager set". + return Valdi::valdiCLIRun("@VALDI_SCRIPT_PATH@", argc, argv, nullptr); } diff --git a/bzl/valdi/app_templates/cli_main_http.cpp.tpl b/bzl/valdi/app_templates/cli_main_http.cpp.tpl new file mode 100644 index 000000000..c3b191cd1 --- /dev/null +++ b/bzl/valdi/app_templates/cli_main_http.cpp.tpl @@ -0,0 +1,7 @@ +// Paired with cli_main.cpp.tpl, which valdi_cli_application picks when enable_http is not set. +#include "valdi/cli_runner/CLIRunner.hpp" +#include "valdi/standalone_http/CurlHTTPRequestManager.hpp" + +int main(int argc, const char** argv) { + return Valdi::valdiCLIRun("@VALDI_SCRIPT_PATH@", argc, argv, Valdi::makeCurlHTTPRequestManager()); +} diff --git a/bzl/valdi/valdi_cli_application.bzl b/bzl/valdi/valdi_cli_application.bzl index c764e1633..1eb873305 100644 --- a/bzl/valdi/valdi_cli_application.bzl +++ b/bzl/valdi/valdi_cli_application.bzl @@ -1,10 +1,6 @@ load("//bzl:expand_template.bzl", "expand_template") load("//bzl/valdi:suffixed_deps.bzl", "get_suffixed_deps") -_HTTP_INCLUDE = "#include \"valdi/standalone_http/CurlHTTPRequestManager.hpp\"" - -_HTTP_MANAGER = ", Valdi::makeCurlHTTPRequestManager()" - def valdi_cli_application( name, script_path, @@ -25,14 +21,15 @@ def valdi_cli_application( """ main_target = "{}_main".format(name) + # Two whole templates rather than one with the manager spliced in, so both stay readable C++. + template = "cli_main_http.cpp.tpl" if enable_http else "cli_main.cpp.tpl" + expand_template( name = main_target, - src = "@valdi//bzl/valdi/app_templates:cli_main.cpp.tpl", + src = "@valdi//bzl/valdi/app_templates:" + template, output = "main.cpp", substitutions = { "@VALDI_SCRIPT_PATH@": script_path, - "@VALDI_HTTP_INCLUDE@": _HTTP_INCLUDE if enable_http else "", - "@VALDI_HTTP_MANAGER@": _HTTP_MANAGER if enable_http else "", }, ) diff --git a/src/valdi_modules/src/valdi/valdi_http/src/HTTPClient.ts b/src/valdi_modules/src/valdi/valdi_http/src/HTTPClient.ts index ce0edc11f..d00a42f1c 100644 --- a/src/valdi_modules/src/valdi/valdi_http/src/HTTPClient.ts +++ b/src/valdi_modules/src/valdi/valdi_http/src/HTTPClient.ts @@ -31,7 +31,9 @@ export class HTTPClient implements IHTTPClient { body: ArrayBuffer | Uint8Array | undefined, ): CancelablePromise { let cancelFn: (() => void) | undefined; + let rejectFn: ((reason: unknown) => void) | undefined; const promise = new Promise((resolve, reject) => { + rejectFn = reject; try { const request: HTTPRequest = { url: makeURL(this.baseUrl, pathOrUrl), @@ -52,7 +54,12 @@ export class HTTPClient implements IHTTPClient { } }); - return promiseToCancelablePromise(promise, () => cancelFn?.()); + // No request manager reports a cancelled request, so without this the promise never settles. + // Rejecting an already settled one is a no-op, so a late cancel changes nothing. + return promiseToCancelablePromise(promise, () => { + cancelFn?.(); + rejectFn?.(new Error('Request was cancelled')); + }); } get(pathOrUrl: string, headers?: StringMap | undefined): CancelablePromise { diff --git a/valdi/src/valdi/cli_runner/CLIRunner.hpp b/valdi/src/valdi/cli_runner/CLIRunner.hpp index 1b7e772ce..6b3206f9c 100644 --- a/valdi/src/valdi/cli_runner/CLIRunner.hpp +++ b/valdi/src/valdi/cli_runner/CLIRunner.hpp @@ -8,9 +8,11 @@ class HTTPRequestManager; namespace Valdi { +// requestManager is null for an application built without an HTTP backend. Not defaulted, so that +// each cli_main template says which one it is. int valdiCLIRun(const char* scriptPath, int argc, const char** argv, - const std::shared_ptr& requestManager = nullptr); + const std::shared_ptr& requestManager); } diff --git a/valdi/src/valdi/runtime/JavaScript/JavaScriptUtils.cpp b/valdi/src/valdi/runtime/JavaScript/JavaScriptUtils.cpp index 8c6d6eee9..e9ac1dad2 100644 --- a/valdi/src/valdi/runtime/JavaScript/JavaScriptUtils.cpp +++ b/valdi/src/valdi/runtime/JavaScript/JavaScriptUtils.cpp @@ -335,12 +335,6 @@ JSValueRef valueToJSValue(IJavaScriptContext& jsContext, jsContext, typedArray.getType(), typedArray.getBuffer(), exceptionTracker); } case ValueType::Error: { - // This raises instead of converting, so an Error arriving here as a return value - // throws in JavaScript. That makes it unusable for a callback argument: the raise - // happens while the arguments are still being marshalled, so the callback never runs - // and any promise waiting on it stays pending. Pass error text instead, as - // HTTPRequestManagerModuleFactory and PersistentStoreModuleFactory do. The ObjC - // conversion behaves differently and does produce a value (SCValdiError). exceptionTracker.onError(value.getError()); return JSValueRef(); } diff --git a/valdi/src/valdi/standalone_http/CaStore.cpp b/valdi/src/valdi/standalone_http/CaStore.cpp new file mode 100644 index 000000000..72e603dd2 --- /dev/null +++ b/valdi/src/valdi/standalone_http/CaStore.cpp @@ -0,0 +1,60 @@ +#include "valdi/standalone_http/CaStore.hpp" + +#include +#include +#include + +namespace Valdi { + +namespace { + +std::string firstSetVariable(std::initializer_list variables) { + for (const char* variable : variables) { + const char* value = std::getenv(variable); + if (value != nullptr && *value != '\0') { + return value; + } + } + return {}; +} + +std::string firstMatching(std::initializer_list candidates, mode_t kind) { + for (const char* candidate : candidates) { + struct stat info; + if (stat(candidate, &info) == 0 && (info.st_mode & S_IFMT) == kind) { + return candidate; + } + } + return {}; +} + +} // namespace + +CaStore requestedCaStore(const StringBox& configured) { + if (!configured.isEmpty()) { + return {std::string(configured.toStringView()), {}}; + } + + // Both, not the first that answers: curl takes a CAINFO and a CAPATH together. + return {firstSetVariable({"CURL_CA_BUNDLE", "SSL_CERT_FILE"}), + firstSetVariable({"CURL_CA_PATH", "SSL_CERT_DIR"})}; +} + +CaStore installedCaStore() { + return {firstMatching( + { + "/etc/ssl/certs/ca-certificates.crt", // Debian, Ubuntu, Alpine, Gentoo + "/etc/pki/tls/certs/ca-bundle.crt", // RHEL, Fedora, CentOS + "/etc/ssl/ca-bundle.pem", // openSUSE + "/etc/ssl/cert.pem", // Alpine, FreeBSD, macOS + }, + S_IFREG), + firstMatching( + { + "/etc/ssl/certs", + "/etc/pki/tls/certs", + }, + S_IFDIR)}; +} + +} // namespace Valdi diff --git a/valdi/src/valdi/standalone_http/CaStore.hpp b/valdi/src/valdi/standalone_http/CaStore.hpp new file mode 100644 index 000000000..1d48a8886 --- /dev/null +++ b/valdi/src/valdi/standalone_http/CaStore.hpp @@ -0,0 +1,32 @@ +#pragma once + +#include "valdi_core/cpp/Utils/StringBox.hpp" + +#include + +namespace Valdi { + +/** A bundle file for CURLOPT_CAINFO and a hashed directory for CURLOPT_CAPATH. */ +struct CaStore { + std::string file; + std::string path; + + bool empty() const { + return file.empty() && path.empty(); + } +}; + +/** + * The configured path, or else CURL_CA_BUNDLE, SSL_CERT_FILE, CURL_CA_PATH and SSL_CERT_DIR. + * libcurl reads none of those itself. + */ +CaStore requestedCaStore(const StringBox& configured); + +/** + * Where the distributions keep their trust stores. Load bearing on Linux: @curl compiles in no CA + * bundle unless --@curl//:ca_bundle says so and is built without CURL_CA_FALLBACK, so BoringSSL + * would otherwise verify against an empty store. macOS gets SecureTransport and the keychain. + */ +CaStore installedCaStore(); + +} // namespace Valdi diff --git a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp index 2e44ba1fc..abfb2c053 100644 --- a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp +++ b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp @@ -1,5 +1,7 @@ #include "valdi/standalone_http/CurlHTTPRequestManager.hpp" +#include "valdi/standalone_http/CaStore.hpp" + #include "valdi_core/Cancelable.hpp" #include "valdi_core/HTTPRequest.hpp" #include "valdi_core/HTTPRequestManagerCompletion.hpp" @@ -14,14 +16,16 @@ #include +#include #include +#include #include +#include #include #include #include #include #include -#include #include #include #include @@ -32,44 +36,29 @@ namespace { constexpr int kMaxRedirects = 10; constexpr long kConnectTimeoutSeconds = 30; -// Only bounds how long an idle thread sits: curl_multi_poll returns at the sooner of this and the -// multi handle's own next timer, and new work, cancellation and shutdown all wake it early. +// A backstop while a transfer is in flight, where curl has a timer of its own and curl_multi_poll +// returns at the sooner of the two. constexpr int kPollTimeoutMs = 10000; -// The curl command line tool reads these, libcurl does not, so we honour them here to let a -// caller point at their own trust store. -const char* const kCaBundleVariables[] = { - "CURL_CA_BUNDLE", - "SSL_CERT_FILE", -}; - -// Only the distributions @curl's own compiled-in CURL_CA_BUNDLE misses, since libcurl applies that -// itself and probing for the same paths here would override a deliberate --@curl//:ca_bundle. -const char* const kCaBundleCandidates[] = { - "/etc/pki/tls/certs/ca-bundle.crt", // RHEL, Fedora - "/etc/ssl/ca-bundle.pem", // openSUSE -}; +// With nothing in flight curl has no timer, so this is the whole wait, and at kPollTimeoutMs the +// thread would wake for no reason every ten seconds for the life of the process. New work, +// cancellation and shutdown all wake the poll, and curl latches a wakeup arriving before the poll +// begins, so sleeping this long strands nothing. Finite only so a lost wakeup would right itself. +constexpr int kIdlePollTimeoutMs = 24 * 60 * 60 * 1000; -std::string resolveCaBundle(const StringBox& configured) { - if (!configured.isEmpty()) { - return std::string(configured.toStringView()); +bool curlHasItsOwnCaStore() { + auto* easy = curl_easy_init(); + if (easy == nullptr) { + return false; } - for (const char* variable : kCaBundleVariables) { - const char* value = std::getenv(variable); - if (value != nullptr && *value != '\0') { - return value; - } - } + char* file = nullptr; + char* path = nullptr; + curl_easy_getinfo(easy, CURLINFO_CAINFO, &file); + curl_easy_getinfo(easy, CURLINFO_CAPATH, &path); + curl_easy_cleanup(easy); - for (const char* candidate : kCaBundleCandidates) { - struct stat info; - if (stat(candidate, &info) == 0 && S_ISREG(info.st_mode)) { - return candidate; - } - } - - return {}; + return file != nullptr || path != nullptr; } // Always a map, never null: HTTPTypes.d.ts declares headers as StringMap, and iOS, Android @@ -79,8 +68,8 @@ Value emptyHeaderMap() { } // A cancelable can outlive the manager, since JavaScript may hold one and cancel after teardown. -// Tasks reach the multi handle through this rather than a back-pointer: the manager clears it once -// its thread has joined and before curl_multi_cleanup, so a late cancel finds nothing to poke. +// Tasks reach the multi handle through this rather than a back-pointer, so a cancel arriving before +// the manager has started one or after it has torn it down finds nothing to poke. class PollWaker { public: explicit PollWaker(CURLM* multi) : _multi(multi) {} @@ -125,8 +114,8 @@ class CurlTask : public snap::valdi_core::Cancelable { cancelled.store(true); - // Nothing reads that flag until curl next runs the progress callback, and an idle transfer - // leaves the poll parked for up to kPollTimeoutMs. Waking it frees the socket now. + // Nothing reads that flag until curl next runs the progress callback, and the poll is parked + // until something wakes it. Waking it frees the socket now. _waker->wake(); } @@ -156,15 +145,13 @@ class CurlTask : public snap::valdi_core::Cancelable { } } - // Rewritten in place as redirects are followed, so that each hop is issued from the same - // record the first one was. snap::valdi_core::HTTPRequest request; - int redirects = 0; std::atomic_bool cancelled{false}; - // Null when the request has no body. This owns the payload for the life of the task, which - // outlives every easy handle made from it, so curl can read it in place. + // Null when the request has no body. Owned for the life of the task, which outlives the easy + // handle made from it, so curl can read it in place. Ref requestBody; + size_t requestBodyOffset = 0; // The write callback fills this and the response takes it directly, so the payload is never // copied. No reserve up front: that would size an allocation from a Content-Length the server @@ -179,6 +166,31 @@ class CurlTask : public snap::valdi_core::Cancelable { std::shared_ptr _waker; }; +// Not CURLOPT_POSTFIELDS, which would pin curl's own idea of the method to POST for every verb and +// take the RFC 9110 redirect rewrite with it. +size_t readBodyCallback(char* buffer, size_t size, size_t count, void* userData) { + auto* task = static_cast(userData); + + auto sending = std::min(task->requestBody->size() - task->requestBodyOffset, size * count); + if (sending > 0) { + std::memcpy(buffer, task->requestBody->data() + task->requestBodyOffset, sending); + task->requestBodyOffset += sending; + } + return sending; +} + +// curl rewinds through this when it repeats a request, as it does for a redirect that keeps the body. +int seekBodyCallback(void* userData, curl_off_t offset, int origin) { + auto* task = static_cast(userData); + + if (origin != SEEK_SET || offset < 0 || static_cast(task->requestBody->size()) < offset) { + return CURL_SEEKFUNC_CANTSEEK; + } + + task->requestBodyOffset = static_cast(offset); + return CURL_SEEKFUNC_OK; +} + size_t writeBodyCallback(char* data, size_t size, size_t count, void* userData) { auto* task = static_cast(userData); task->responseBody->append(data, data + size * count); @@ -190,9 +202,9 @@ size_t writeHeaderCallback(char* data, size_t size, size_t count, void* userData std::string line(data, size * count); - // Check this before looking for a colon, because a reason phrase is free-form text and may - // contain one. The reset discards anything an informational response carried, since curl hands - // a 1xx header block to this callback too and a 103 Early Hints brings real headers with it. + // Checked before looking for a colon, because a reason phrase is free-form text and may contain + // one. The reset is what leaves only the last response's headers: curl hands over every hop of a + // redirect chain and every 1xx block, and a 103 Early Hints carries real headers. if (line.rfind("HTTP/", 0) == 0) { task->responseHeaders = emptyHeaderMap(); return size * count; @@ -290,14 +302,20 @@ int progressCallback(void* userData, curl_off_t, curl_off_t, curl_off_t, curl_of class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { public: - CurlHTTPRequestManager(std::string caBundle, int32_t idleTimeoutSeconds, CURLcode globalInit) - : _caBundle(std::move(caBundle)), _idleTimeoutSeconds(idleTimeoutSeconds), _globalInit(globalInit) { + CurlHTTPRequestManager(const StringBox& caBundlePath, int32_t idleTimeoutSeconds, CURLcode globalInit) + : _idleTimeoutSeconds(idleTimeoutSeconds), _globalInit(globalInit) { if (_globalInit != CURLE_OK) { // Carrying on would leave curl_easy_init handing back handles whose TLS backend was // never set up, so every HTTPS request would fail with an unrelated-looking error. return; } + // Probed last, so a deliberate --@curl//:ca_bundle is never overridden. + _caStore = requestedCaStore(caBundlePath); + if (_caStore.empty() && !curlHasItsOwnCaStore()) { + _caStore = installedCaStore(); + } + _multi = curl_multi_init(); if (_multi == nullptr) { return; @@ -311,10 +329,12 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { std::lock_guard guard(_mutex); _stopping = true; } + curl_multi_wakeup(_multi); if (_thread.joinable()) { _thread.join(); } + // After the join, so the run loop still has a handle to poll, and before the cleanup, so a // cancel arriving from JavaScript from here on finds nothing rather than a freed handle. _waker->detach(); @@ -326,7 +346,7 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { const std::shared_ptr& completion) override { auto task = std::make_shared(request, completion, _waker); - // Check this before the multi handle, so the reported cause is the one that actually failed. + // Checked before the multi handle, so the reported cause is the one that actually failed. if (_globalInit != CURLE_OK) { task->complete(Error(StringBox::fromString(std::string("Failed to initialise libcurl: ") + curl_easy_strerror(_globalInit)))); @@ -348,8 +368,7 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { } // Fail outside the lock, because a completion is free to queue another request and _mutex - // is not recursive. Queueing the task instead would strand it: run() has already drained - // _pending for the last time and nothing will service it again. + // is not recursive. if (stopping) { task->complete(Error(STRING_LITERAL("Request manager shutting down"))); return task; @@ -385,7 +404,7 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { drainMessages(); int numfds = 0; - curl_multi_poll(_multi, nullptr, 0, kPollTimeoutMs, &numfds); + curl_multi_poll(_multi, nullptr, 0, _active.empty() ? kIdlePollTimeoutMs : kPollTimeoutMs, &numfds); } // Drop outstanding work instead of failing it; see CurlTask::dropCompletion. @@ -407,6 +426,11 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { } void addTask(const std::shared_ptr& task) { + // cancel() only raises the flag, so a task cancelled while queued arrives here anyway. + if (task->cancelled.load()) { + return; + } + const auto& request = task->request; // Checked before a handle exists, so a rejected request leaves nothing to clean up. @@ -423,39 +447,57 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { curl_easy_setopt(easy, CURLOPT_URL, std::string(request.url.toStringView()).c_str()); - if (task->requestBody != nullptr) { - curl_easy_setopt(easy, CURLOPT_POSTFIELDSIZE_LARGE, static_cast(task->requestBody->size())); - // Not COPYPOSTFIELDS: the task already owns this buffer for longer than the handle - // lives, so letting curl copy it again would hold the payload twice. - curl_easy_setopt(easy, CURLOPT_POSTFIELDS, reinterpret_cast(task->requestBody->data())); + // Methods curl models itself go through their own options, so it rewrites them across a + // redirect per RFC 9110 15.4. CUSTOMREQUEST only rewrites the request line and curl keeps it + // for the whole chain, so a verb left to it survives a 303 instead of becoming GET, which is + // the one place this diverges from iOS and Android. + auto method = std::string(request.method.toStringView()); + if (method.empty()) { + method = "GET"; } - // Methods curl models itself go through their own options, so it sets the transfer up to - // match: CURLOPT_POST arranges a body reader, CURLOPT_NOBODY suppresses reading one. - // CUSTOMREQUEST only rewrites the request line, so it is left for verbs curl has no option - // for. - auto method = std::string(request.method.toStringView()); + bool hasBody = task->requestBody != nullptr; + if (method == "HEAD") { curl_easy_setopt(easy, CURLOPT_NOBODY, 1L); } else if (method == "POST") { curl_easy_setopt(easy, CURLOPT_POST, 1L); - if (task->requestBody == nullptr) { - // Without fields curl reads the body from the read callback, which is stdin. - curl_easy_setopt(easy, CURLOPT_POSTFIELDSIZE_LARGE, static_cast(0)); - curl_easy_setopt(easy, CURLOPT_COPYPOSTFIELDS, ""); - } - } else if (method.empty() || method == "GET") { - if (task->requestBody != nullptr) { - // The fields above turned this into a POST; name GET to keep the request line. - curl_easy_setopt(easy, CURLOPT_CUSTOMREQUEST, "GET"); - } else { - curl_easy_setopt(easy, CURLOPT_HTTPGET, 1L); + // Set even with no body, since curl otherwise reads one from stdin. + curl_easy_setopt(easy, + CURLOPT_POSTFIELDSIZE_LARGE, + static_cast(hasBody ? task->requestBody->size() : 0)); + // Not COPYPOSTFIELDS: the task already owns this buffer for longer than the handle + // lives, so letting curl copy it again would hold the payload twice. + curl_easy_setopt(easy, + CURLOPT_POSTFIELDS, + hasBody ? reinterpret_cast(task->requestBody->data()) : ""); + } else if (hasBody) { + curl_easy_setopt(easy, CURLOPT_UPLOAD, 1L); + curl_easy_setopt(easy, CURLOPT_INFILESIZE_LARGE, static_cast(task->requestBody->size())); + curl_easy_setopt(easy, CURLOPT_READFUNCTION, readBodyCallback); + curl_easy_setopt(easy, CURLOPT_READDATA, task.get()); + curl_easy_setopt(easy, CURLOPT_SEEKFUNCTION, seekBodyCallback); + curl_easy_setopt(easy, CURLOPT_SEEKDATA, task.get()); + + // CURLOPT_UPLOAD already names the request PUT. + if (method != "PUT") { + curl_easy_setopt(easy, CURLOPT_CUSTOMREQUEST, method.c_str()); } + } else if (method == "GET") { + curl_easy_setopt(easy, CURLOPT_HTTPGET, 1L); } else { curl_easy_setopt(easy, CURLOPT_CUSTOMREQUEST, method.c_str()); } for (const auto& key : request.headers.sortedMapKeys()) { + // The transport's to set, not the caller's: curl frames the body itself, and asking for + // an encoding would get one back that this build has no codec to decode. fetch forbids + // both header names, so the same JavaScript already loses them on web. + if (equalsIgnoringCase(key.toStringView(), "content-length") || + equalsIgnoringCase(key.toStringView(), "accept-encoding")) { + continue; + } + auto value = std::string(request.headers.getMapValue(key).toStringBox().toStringView()); // curl steps over the whitespace after a colon and then drops an empty valued header @@ -479,6 +521,14 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { curl_easy_setopt(easy, CURLOPT_XFERINFODATA, task.get()); curl_easy_setopt(easy, CURLOPT_NOPROGRESS, 0L); + // Also withholds Authorization and Cookie from any host but the one the chain started on. + curl_easy_setopt(easy, CURLOPT_FOLLOWLOCATION, 1L); + curl_easy_setopt(easy, CURLOPT_MAXREDIRS, static_cast(kMaxRedirects)); + + // Redirects have their own allow-list, already these two, but a first request is checked + // against this one, which otherwise admits every protocol compiled in. + curl_easy_setopt(easy, CURLOPT_PROTOCOLS_STR, "http,https"); + curl_easy_setopt(easy, CURLOPT_CONNECTTIMEOUT, kConnectTimeoutSeconds); curl_easy_setopt(easy, CURLOPT_NOSIGNAL, 1L); @@ -494,8 +544,11 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { curl_easy_setopt(easy, CURLOPT_LOW_SPEED_TIME, static_cast(_idleTimeoutSeconds)); } - if (!_caBundle.empty()) { - curl_easy_setopt(easy, CURLOPT_CAINFO, _caBundle.c_str()); + if (!_caStore.file.empty()) { + curl_easy_setopt(easy, CURLOPT_CAINFO, _caStore.file.c_str()); + } + if (!_caStore.path.empty()) { + curl_easy_setopt(easy, CURLOPT_CAPATH, _caStore.path.c_str()); } auto added = curl_multi_add_handle(_multi, easy); @@ -506,29 +559,6 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { _active.emplace(easy, task); } - // Followed here rather than by CURLOPT_FOLLOWLOCATION, which keeps CURLOPT_CUSTOMREQUEST for - // the whole chain: Curl_http_method takes the request line from it unconditionally while the - // redirect handlers only touch curl's own idea of the method, so a DELETE would stay a DELETE - // across a 303 and a PUT would keep its verb while curl dropped the body. Rewriting per RFC - // 9110 15.4. - static void retarget(CurlTask& task, long statusCode, const char* location) { - task.request.url = StringBox::fromCString(location); - - auto method = std::string(task.request.method.toStringView()); - bool toGet = statusCode == 303 ? (method != "GET" && method != "HEAD") - : ((statusCode == 301 || statusCode == 302) && method == "POST"); - if (toGet) { - task.request.method = STRING_LITERAL("GET"); - task.requestBody = nullptr; - } - - // curl only withholds a redirect's body from the write callback when it is following the - // redirect itself, so this hop's is ours to discard. - task.responseBody->clear(); - task.responseHeaders = emptyHeaderMap(); - task.redirects++; - } - void drainMessages() { int remaining = 0; while (auto* message = curl_multi_info_read(_multi, &remaining)) { @@ -552,23 +582,6 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { long statusCode = 0; curl_easy_getinfo(easy, CURLINFO_RESPONSE_CODE, &statusCode); - // Only set for a 3xx carrying a Location, and curl has already resolved it against - // the request URL, so a relative target arrives absolute. - char* location = nullptr; - curl_easy_getinfo(easy, CURLINFO_REDIRECT_URL, &location); - if (location != nullptr) { - if (task->redirects >= kMaxRedirects) { - finish(easy, task, Error(STRING_LITERAL("Maximum number of redirects followed"))); - continue; - } - retarget(*task, statusCode, location); - releaseHandle(easy, task); - // curl_multi_add_handle asks for this handle to run immediately, so the next hop - // is serviced on the following pass rather than after a poll timeout. - addTask(task); - continue; - } - // Nothing to decode if nothing arrived, which is also what keeps a HEAD or a 204 // against a compressing origin from being reported as a failure. if (!task->responseBody->empty()) { @@ -611,10 +624,16 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { curl_slist_free_all(task->requestHeaders); task->requestHeaders = nullptr; } + + // JavaScript holds the task for as long as it holds the CancelablePromise, so a kept handle + // should cost the handle rather than the payload. + task->requestBody = nullptr; + task->request = snap::valdi_core::HTTPRequest(StringBox(), StringBox(), Value(), std::nullopt, 0); + curl_easy_cleanup(easy); } - std::string _caBundle; + CaStore _caStore; int32_t _idleTimeoutSeconds = 0; CURLcode _globalInit = CURLE_OK; CURLM* _multi = nullptr; @@ -635,8 +654,7 @@ Shared makeCurlHTTPRequestManager(const St static std::once_flag globalInit; std::call_once(globalInit, []() { globalInitResult = curl_global_init(CURL_GLOBAL_DEFAULT); }); - return std::make_shared( - resolveCaBundle(caBundlePath), idleTimeoutSeconds, globalInitResult); + return std::make_shared(caBundlePath, idleTimeoutSeconds, globalInitResult); } } // namespace Valdi diff --git a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.hpp b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.hpp index 017ae6c22..8e394699d 100644 --- a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.hpp +++ b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.hpp @@ -13,14 +13,11 @@ constexpr int32_t kDefaultIdleTimeoutSeconds = 60; * An HTTPRequestManager backed by libcurl, for integrations with no platform user agent of their * own: the standalone runtime and the CLI apps built on it. * - * caBundlePath selects the trust store used to verify server certificates. When empty, we check - * the CURL_CA_BUNDLE and SSL_CERT_FILE environment variables, which libcurl does not read itself, - * then the few distribution paths the @curl build defaults do not name. If nothing matches, - * libcurl falls back to the trust store compiled into it, covering macOS and the Debian family. - * Set --@curl//:ca_bundle to point that elsewhere. + * caBundlePath selects the trust store used to verify server certificates. When empty, see + * CaStore.hpp for what is consulted instead and why. Set --@curl//:ca_bundle to compile one in. * * idleTimeoutSeconds fails a request that goes this long without transferring anything, so that a - * server which accepts a connection and then stalls cannot leave a caller waiting forever. It is + * server which accepts a connection and then stalls cannot leave a caller waiting forever. It * deliberately measures inactivity, not total elapsed time: a large asset download is slow but * never idle, and a hard cap would cut it off. Zero disables it. */ diff --git a/valdi/test/standalone/CaStore_tests.cpp b/valdi/test/standalone/CaStore_tests.cpp new file mode 100644 index 000000000..52f13aee0 --- /dev/null +++ b/valdi/test/standalone/CaStore_tests.cpp @@ -0,0 +1,149 @@ +#include "valdi/standalone_http/CaStore.hpp" + +#include "valdi_core/cpp/Utils/StringBox.hpp" + +#include "gtest/gtest.h" + +#include +#include +#include +#include +#include +#include +#include + +using namespace Valdi; + +namespace ValdiTest { + +namespace { + +// Every variable the resolution consults, cleared around each case so that whatever the developer +// or the CI image happens to have set cannot decide what these assert. +const char* const kVariables[] = { + "CURL_CA_BUNDLE", + "SSL_CERT_FILE", + "CURL_CA_PATH", + "SSL_CERT_DIR", +}; + +class CaStoreFixture : public ::testing::Test { +protected: + void SetUp() override { + for (const char* variable : kVariables) { + const char* value = std::getenv(variable); + _saved.emplace_back(variable, + value != nullptr ? std::optional(value) : std::nullopt); + ::unsetenv(variable); + } + } + + void TearDown() override { + for (const auto& entry : _saved) { + if (entry.second) { + ::setenv(entry.first, entry.second->c_str(), 1); + } else { + ::unsetenv(entry.first); + } + } + } + +private: + std::vector>> _saved; +}; + +} // namespace + +TEST_F(CaStoreFixture, prefersTheConfiguredPathOverTheEnvironment) { + ::setenv("SSL_CERT_FILE", "/from/the/environment.pem", 1); + + auto store = requestedCaStore(StringBox::fromCString("/from/the/caller.pem")); + EXPECT_EQ(store.file, "/from/the/caller.pem"); + EXPECT_EQ(store.path, ""); +} + +TEST_F(CaStoreFixture, readsTheCurlCommandLineToolsBundleVariable) { + ::setenv("CURL_CA_BUNDLE", "/curl/bundle.pem", 1); + + auto store = requestedCaStore(StringBox()); + EXPECT_EQ(store.file, "/curl/bundle.pem") << "libcurl does not read this itself, so a caller " + "pointing at their own trust store is ignored " + "without it"; +} + +TEST_F(CaStoreFixture, readsOpenSslsCertificateFileVariable) { + ::setenv("SSL_CERT_FILE", "/openssl/bundle.pem", 1); + + auto store = requestedCaStore(StringBox()); + EXPECT_EQ(store.file, "/openssl/bundle.pem"); +} + +TEST_F(CaStoreFixture, prefersCurlsBundleVariableOverOpenSsls) { + ::setenv("CURL_CA_BUNDLE", "/curl/bundle.pem", 1); + ::setenv("SSL_CERT_FILE", "/openssl/bundle.pem", 1); + + auto store = requestedCaStore(StringBox()); + EXPECT_EQ(store.file, "/curl/bundle.pem"); +} + +// The trust store on Alpine and on any image that points at a hashed directory rather than a single +// bundle. There is no file to hand CURLOPT_CAINFO, so a resolution that only ever produces one +// leaves those machines with an empty store and every HTTPS request failing. +TEST_F(CaStoreFixture, readsOpenSslsCertificateDirectoryVariable) { + ::setenv("SSL_CERT_DIR", "/etc/ssl/certs", 1); + + auto store = requestedCaStore(StringBox()); + EXPECT_EQ(store.path, "/etc/ssl/certs") + << "SSL_CERT_DIR went unread, so a caller whose trust store is a hashed directory has no " + "way to say so and every certificate is rejected"; + EXPECT_EQ(store.file, ""); +} + +TEST_F(CaStoreFixture, readsTheCurlCommandLineToolsPathVariable) { + ::setenv("CURL_CA_PATH", "/curl/certs", 1); + + auto store = requestedCaStore(StringBox()); + EXPECT_EQ(store.path, "/curl/certs"); +} + +TEST_F(CaStoreFixture, reportsBothAFileAndADirectoryWhenBothAreSet) { + ::setenv("SSL_CERT_FILE", "/openssl/bundle.pem", 1); + ::setenv("SSL_CERT_DIR", "/openssl/certs", 1); + + auto store = requestedCaStore(StringBox()); + EXPECT_EQ(store.file, "/openssl/bundle.pem"); + EXPECT_EQ(store.path, "/openssl/certs") << "curl takes a CAINFO and a CAPATH together, and the " + "tool these variables come from honours both"; +} + +TEST_F(CaStoreFixture, findsNothingWhenNothingIsAskedFor) { + auto store = requestedCaStore(StringBox()); + EXPECT_TRUE(store.empty()); +} + +// The @curl dep compiles in no trust store and is built without CURL_CA_FALLBACK, so on Linux +// whatever this finds is the only thing standing between BoringSSL and rejecting every certificate. +// Debian and Ubuntu are the common case and neither is served by the RHEL or openSUSE paths. +TEST_F(CaStoreFixture, namesTheTrustStoreOfTheCommonDistributions) { + // Not a filesystem probe: this asserts the list, since the machine running the test has at most + // one of these and the point is that all of them are covered. + for (const char* expected : { + "/etc/ssl/certs/ca-certificates.crt", // Debian, Ubuntu, Alpine, Gentoo + "/etc/pki/tls/certs/ca-bundle.crt", // RHEL, Fedora, CentOS + "/etc/ssl/ca-bundle.pem", // openSUSE + "/etc/ssl/cert.pem", // Alpine, FreeBSD + }) { + struct stat info; + if (stat(expected, &info) != 0 || !S_ISREG(info.st_mode)) { + continue; + } + EXPECT_EQ(installedCaStore().file, expected) + << expected << " exists on this machine but is not the bundle that was resolved, so a " + << "build on this distribution verifies against nothing"; + return; + } + + GTEST_SKIP() << "no distribution bundle on this machine to resolve"; +} + +} // namespace ValdiTest diff --git a/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp b/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp index 15b1ead15..8fae8abb8 100644 --- a/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp +++ b/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -311,6 +312,92 @@ class DribblingServer { std::thread _thread; }; +// Streams a body of a given size out of one small buffer. A server holding the whole thing would +// take peak resident size past anything the transfer goes on to ask for, leaving a peak measured +// across that transfer meaningless. +class BulkServer { +public: + explicit BulkServer(size_t bodySize) { + ignoreSigPipe(); + + _listener = ::socket(AF_INET, SOCK_STREAM, 0); + _port = bindToLoopback(_listener); + ::listen(_listener, 1); + + _thread = std::thread([this, bodySize]() { serve(bodySize); }); + } + + ~BulkServer() { + _stopping.store(true); + wakeAccept(_port); + + if (_thread.joinable()) { + _thread.join(); + } + ::close(_listener); + } + + std::string url() const { + return "http://127.0.0.1:" + std::to_string(_port) + "/"; + } + +private: + void serve(size_t bodySize) { + int connection = ::accept(_listener, nullptr, nullptr); + if (connection < 0) { + return; + } + if (_stopping.load()) { + ::close(connection); + return; + } + + std::vector buffer(64 * 1024); + std::string head; + while (head.find("\r\n\r\n") == std::string::npos) { + auto received = ::recv(connection, buffer.data(), buffer.size(), 0); + if (received <= 0) { + ::close(connection); + return; + } + head.append(buffer.data(), static_cast(received)); + } + + auto response = + "HTTP/1.1 200 OK\r\nContent-Length: " + std::to_string(bodySize) + "\r\nConnection: close\r\n\r\n"; + if (sendAll(connection, response.data(), response.size())) { + std::fill(buffer.begin(), buffer.end(), 'x'); + for (size_t sent = 0; sent < bodySize;) { + auto size = std::min(buffer.size(), bodySize - sent); + if (!sendAll(connection, buffer.data(), size)) { + break; + } + sent += size; + } + } + + ::shutdown(connection, SHUT_RDWR); + ::close(connection); + } + + static bool sendAll(int connection, const char* data, size_t size) { + while (size > 0) { + auto written = ::send(connection, data, size, 0); + if (written <= 0) { + return false; + } + data += written; + size -= static_cast(written); + } + return true; + } + + int _listener = -1; + uint16_t _port = 0; + std::atomic_bool _stopping{false}; + std::thread _thread; +}; + // Serves canned responses, one per connection, so a redirect chain is deterministic and // needs no network. Each response should say "Connection: close" to keep curl from // reusing a connection and leaving a later response unclaimed. @@ -869,6 +956,32 @@ TEST(CurlHTTPRequestManagerTests, sendsAnEmptyValuedOverrideOfACurlDefault) { << requests[0]; } +// This build has no decompression codecs, so a caller asking on its behalf gets back a response +// nothing can read. fetch drops the header too, so the same JavaScript already loses it on web. +TEST(CurlHTTPRequestManagerTests, ignoresACallerSuppliedAcceptEncoding) { + ScriptedServer server({okResponse()}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest( + makeGetWithHeaders(server.url("/").c_str(), makeHeaders({{"Accept-Encoding", "gzip, deflate"}})), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + EXPECT_FALSE(completion->error().has_value()) + << "asking for compression should cost the caller nothing but the compression: " + << completion->error().value_or(""); + EXPECT_EQ(completion->statusCode(), 200); + EXPECT_EQ(completion->bodySize(), 2u); + + auto requests = server.requests(); + ASSERT_EQ(requests.size(), 1u); + EXPECT_EQ(requests[0].find("Accept-Encoding"), std::string::npos) + << "the caller's Accept-Encoding reached the origin, so a conforming origin will compress a " + "response this build cannot decode and the request fails where it succeeds everywhere " + "else. Request was:\n" + << requests[0]; +} + TEST(CurlHTTPRequestManagerTests, failsAResponseItCannotDecode) { ScriptedServer server({"HTTP/1.1 200 OK\r\n" "Content-Encoding: gzip\r\n" @@ -1017,7 +1130,10 @@ TEST(CurlHTTPRequestManagerTests, followsASeeOtherWithGet) { EXPECT_EQ(requests[1], "GET /final HTTP/1.1") << "a 303 must be followed with GET, not the original verb"; } -TEST(CurlHTTPRequestManagerTests, followsASeeOtherFromACustomVerbWithGet) { +// Curl_http_method takes the request line from CUSTOMREQUEST unconditionally, so a verb sent that +// way survives the chain, while the body still goes because that follows curl's own idea of the +// method. iOS and Android send GET here. Pinned so the divergence stays a decision. +TEST(CurlHTTPRequestManagerTests, keepsACustomVerbAcrossASeeOtherButDropsItsBody) { ScriptedServer server({"HTTP/1.1 303 See Other\r\n" "Location: /final\r\n" "Content-Length: 0\r\n" @@ -1033,18 +1149,19 @@ TEST(CurlHTTPRequestManagerTests, followsASeeOtherFromACustomVerbWithGet) { auto completion = std::make_shared(); auto start = std::chrono::steady_clock::now(); - manager->performRequest(makeRequest("DELETE", server.url("/thing").c_str()), completion); + manager->performRequest(makeRequestWithBody("PATCH", server.url("/thing").c_str(), "a=1"), completion); ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(30))); auto elapsed = std::chrono::duration_cast(std::chrono::steady_clock::now() - start); ASSERT_EQ(completion->statusCode(), 200); - auto requests = server.requestLines(); + auto requests = server.requests(); ASSERT_EQ(requests.size(), 2u); - EXPECT_EQ(requests[0], "DELETE /thing HTTP/1.1"); - EXPECT_EQ(requests[1], "GET /final HTTP/1.1") - << "a 303 must be followed with GET, including for a verb curl only knows as a custom " - "request line"; + EXPECT_EQ(requests[0].substr(0, requests[0].find("\r\n")), "PATCH /thing HTTP/1.1"); + EXPECT_EQ(requests[1].substr(0, requests[1].find("\r\n")), "PATCH /final HTTP/1.1"); + EXPECT_EQ(requests[1].find("\r\n\r\na=1"), std::string::npos) + << "the body survived a 303, which drops it whatever the verb. Request was:\n" + << requests[1]; EXPECT_LT(elapsed.count(), 500) << "a two hop redirect took " << elapsed.count() << " ms, so a hop is not picked up until the poll has slept out " @@ -1142,6 +1259,128 @@ TEST(CurlHTTPRequestManagerTests, keepsThePostBodyAcrossATemporaryRedirect) { << requests[1]; } +// @curl is built http_only, so these pass on build configuration alone. They exist to hold +// CURLOPT_PROTOCOLS_STR in place, without which the guarantee is one build flag from a file read. +TEST(CurlHTTPRequestManagerTests, refusesAUrlThatIsNotHttp) { + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGet("file:///etc/hosts"), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + EXPECT_TRUE(completion->error().has_value()) << "a file:// URL from JavaScript was fetched"; + EXPECT_EQ(completion->bodySize(), 0u) << "the contents of a local file reached the caller"; +} + +TEST(CurlHTTPRequestManagerTests, refusesARedirectToAUrlThatIsNotHttp) { + ScriptedServer server({"HTTP/1.1 302 Found\r\n" + "Location: file:///etc/hosts\r\n" + "Content-Length: 0\r\n" + "Connection: close\r\n" + "\r\n"}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGet(server.url("/start").c_str()), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + EXPECT_TRUE(completion->error().has_value()) + << "an origin redirected the request at a local file and it was followed"; + EXPECT_EQ(completion->bodySize(), 0u) << "the contents of a local file reached the caller"; +} + +TEST(CurlHTTPRequestManagerTests, ignoresACallerSuppliedContentLength) { + ScriptedServer server({"HTTP/1.1 303 See Other\r\n" + "Location: /final\r\n" + "Content-Length: 0\r\n" + "Connection: close\r\n" + "\r\n", + okResponse()}); + + // JavaScript is not held to HTTPTypes.d.ts, so a caller can set this. NSURLSession ignores it. + auto request = makeRequestWithBody("POST", server.url("/submit").c_str(), "a=1"); + request.headers.setMapValue(std::string_view("Content-Length"), Value(StringBox::fromCString("3"))); + request.headers.setMapValue(std::string_view("Content-Type"), Value(StringBox::fromCString("text/plain"))); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(request, completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + ASSERT_EQ(completion->statusCode(), 200); + + auto requests = server.requests(); + ASSERT_EQ(requests.size(), 2u); + EXPECT_NE(requests[0].find("Content-Length: 3\r\n"), std::string::npos) + << "the body's own length must still be declared. Request was:\n" + << requests[0]; + EXPECT_EQ(requests[1].find("Content-Length"), std::string::npos) + << "a 303 drops the body, but the caller's Content-Length came along, so the server waits " + "for three bytes that will never arrive. Request was:\n" + << requests[1]; +} + +TEST(CurlHTTPRequestManagerTests, withholdsCredentialHeadersFromARedirectToAnotherOrigin) { + // Two loopback servers differ by port, which is part of an origin, so this is the same shape as + // api.example.com answering 302 Location: https://attacker.test/. + ScriptedServer elsewhere({okResponse()}); + + ScriptedServer origin({"HTTP/1.1 302 Found\r\n" + "Location: " + + elsewhere.url("/collect") + + "\r\n" + "Content-Length: 0\r\n" + "Connection: close\r\n" + "\r\n"}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGetWithHeaders(origin.url("/start").c_str(), + makeHeaders({{"Authorization", "Bearer secret"}, + {"Cookie", "session=abc"}, + {"X-Trace", "keep-me"}})), + completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + ASSERT_EQ(completion->statusCode(), 200); + + auto forwarded = elsewhere.requests(); + ASSERT_EQ(forwarded.size(), 1u) << "the redirect was not followed, so there is nothing to judge"; + EXPECT_EQ(forwarded[0].find("Authorization:"), std::string::npos) + << "the caller's bearer token was handed to the host the first one redirected to. Request was:\n" + << forwarded[0]; + EXPECT_EQ(forwarded[0].find("Cookie:"), std::string::npos) + << "the caller's cookies were handed to the host the first one redirected to. Request was:\n" + << forwarded[0]; + EXPECT_NE(forwarded[0].find("X-Trace: keep-me\r\n"), std::string::npos) + << "only Authorization and Cookie are withheld; an ordinary header still crosses. Request was:\n" + << forwarded[0]; +} + +TEST(CurlHTTPRequestManagerTests, keepsCredentialHeadersOnARedirectWithinTheSameOrigin) { + ScriptedServer server({"HTTP/1.1 302 Found\r\n" + "Location: /final\r\n" + "Content-Length: 0\r\n" + "Connection: close\r\n" + "\r\n", + okResponse()}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest( + makeGetWithHeaders(server.url("/start").c_str(), makeHeaders({{"Authorization", "Bearer secret"}})), + completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + ASSERT_EQ(completion->statusCode(), 200); + + auto requests = server.requests(); + ASSERT_EQ(requests.size(), 2u); + EXPECT_NE(requests[1].find("Authorization: Bearer secret\r\n"), std::string::npos) + << "a redirect within the same origin must keep the caller's credentials, or every " + "authenticated request that redirects starts failing. Request was:\n" + << requests[1]; +} + TEST(CurlHTTPRequestManagerTests, failsARedirectChainThatNeverEnds) { // One response more than kMaxRedirects permits follows, so the last has to be refused rather // than followed. This pins the follow count, which the manager now keeps itself instead of @@ -1323,15 +1562,9 @@ TEST(CurlHTTPRequestManagerTests, survivesAPeerHangingUpMidResponse) { TEST(CurlHTTPRequestManagerTests, doesNotHoldTheResponseBodyTwice) { constexpr size_t kBodySize = 32 * 1024 * 1024; - DribblingServer server("HTTP/1.1 200 OK\r\n" - "Content-Length: " + - std::to_string(kBodySize) + - "\r\n" - "Connection: close\r\n" - "\r\n" + - std::string(kBodySize, 'x'), - 1, - std::chrono::milliseconds(0)); + // Streaming, so nothing here has held a body this size when the baseline below is taken. + // Otherwise the peak is already past anything the transfer reaches and the comparison is vacuous. + BulkServer server(kBodySize); auto manager = makeCurlHTTPRequestManager(); auto completion = std::make_shared(); @@ -1343,10 +1576,12 @@ TEST(CurlHTTPRequestManagerTests, doesNotHoldTheResponseBodyTwice) { ASSERT_EQ(completion->statusCode(), 200); ASSERT_EQ(completion->bodySize(), kBodySize) << "the large body did not arrive intact"; - // Holding the payload once measured 1.03x and holding it twice measured 2.03x, so the threshold - // sits midway instead of just under the failing value. + // 2x is inherent: ByteBuffer grows geometrically, so the last reallocation holds the old buffer + // and the written part of the new one at once, and sizing up front would mean trusting a + // server-chosen Content-Length. A second copy of the finished payload is 3x, which is the thing + // this catches, so the threshold sits between them. auto growth = peakResidentBytes() - before; - EXPECT_LT(growth, kBodySize + kBodySize / 2) + EXPECT_LT(growth, kBodySize * 5 / 2) << "peak memory grew by " << growth / (1024 * 1024) << " MiB to receive a " << kBodySize / (1024 * 1024) << " MiB body, so the payload is accumulated in one buffer and then copied whole into another"; @@ -1375,6 +1610,22 @@ TEST(CurlHTTPRequestManagerTests, cancellingARequestDropsItsCompletion) { << "a cancelled request must leave its completion uncalled, as on iOS and Android"; } +TEST(CurlHTTPRequestManagerTests, doesNotConnectForARequestCancelledBeforeItStarts) { + StallServer server; + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + + // performRequest only queues the task, so cancelling straight back lands before the curl thread + // has been scheduled to pick it up. + auto cancelable = manager->performRequest(makeGet(server.url().c_str()), completion); + cancelable->cancel(); + + EXPECT_FALSE(server.waitForConnection(std::chrono::seconds(1))) + << "a request cancelled before it ever started was still handed to curl, which resolved the " + "name and completed a handshake before the progress callback got a say"; +} + TEST(CurlHTTPRequestManagerTests, abortsACancelledTransferWithoutWaitingOutThePollTimeout) { StallServer server; diff --git a/valdi/test/standalone/StandaloneRequestManager_tests.cpp b/valdi/test/standalone/StandaloneRequestManager_tests.cpp index 2451d2b19..558a7ce01 100644 --- a/valdi/test/standalone/StandaloneRequestManager_tests.cpp +++ b/valdi/test/standalone/StandaloneRequestManager_tests.cpp @@ -170,6 +170,42 @@ TEST_P(StandaloneRequestManagerFixture, httpClientRejectsItsPromiseOnFailure) { "open waiting for a settlement that never comes"; } +// No manager reports a cancelled request, so the settlement has to come from JavaScript. Without it +// the runtime never runs down. +TEST_P(StandaloneRequestManagerFixture, httpClientRejectsItsPromiseOnCancel) { + auto requestManager = Valdi::makeShared(ConsoleLogger::getLogger()); + requestManager->addMockedResponse(STRING_LITERAL("http://localhost/"), STRING_LITERAL("GET"), BytesView()); + + auto arguments = makeArguments(); + arguments.requestManager = requestManager; + + auto standaloneRuntime = createValdiStandaloneRuntime(arguments); + auto* jsRuntime = standaloneRuntime->getRuntime().getJavaScriptRuntime(); + + auto evaluate = [&](const std::string& source) { + return jsRuntime->evaluateScript(makeShared(source)->toBytesView(), + STRING_LITERAL("standalone_request_manager_test.js")); + }; + + auto started = evaluate("var HTTPClient = global.require('valdi_http/src/HTTPClient').HTTPClient;" + "global.__outcome = '';" + "var request = new HTTPClient().get('http://localhost/');" + "request.then(" + " function () { global.__outcome = 'resolved'; }," + " function (e) { global.__outcome = 'rejected: ' + e; });" + "request.cancel();" + "return 'started';"); + ASSERT_TRUE(started) << started.description(); + + requestManager->getAllPerformedTasks(); + + auto outcome = evaluate("return global.__outcome;"); + ASSERT_TRUE(outcome) << outcome.description(); + EXPECT_EQ("rejected: Error: Request was cancelled", outcome.value().toString()) + << "cancelling left the promise pending, so a CLI that called beginKeepAlive waits forever " + "for a settlement no manager is going to send"; +} + INSTANTIATE_TEST_SUITE_P(StandaloneRequestManagerTests, StandaloneRequestManagerFixture, ::testing::Values(JavaScriptEngineTestCase::Hermes, From d19ff666a6bc7bf2bc65279033ae7612f306a834 Mon Sep 17 00:00:00 2001 From: tx3stn <14163530+tx3stn@users.noreply.github.com> Date: Fri, 28 Aug 2026 11:48:01 +0100 Subject: [PATCH 12/18] fix: re-word error message --- valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp index abfb2c053..ece234e1d 100644 --- a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp +++ b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp @@ -589,9 +589,9 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { finish(easy, task, Error(StringBox::fromString( - "Cannot decode a response sent with Content-Encoding: " + *encoding + - ". This build has no decompression support, so an Accept-Encoding request " - "header must not be set."))); + "The server sent Content-Encoding: " + *encoding + + ", which this build cannot decode. No Accept-Encoding was requested, so the " + "response was compressed unasked."))); continue; } } From 9a3226d2f836b2e299c02a53a9cd5f6550854c70 Mon Sep 17 00:00:00 2001 From: tx3stn <14163530+tx3stn@users.noreply.github.com> Date: Fri, 28 Aug 2026 12:50:03 +0100 Subject: [PATCH 13/18] fix: rejection error and make the ca store verify paths --- .../src/valdi/valdi_http/src/HTTPClient.ts | 4 ++- valdi/src/valdi/standalone_http/CaStore.cpp | 13 ++++++-- valdi/src/valdi/standalone_http/CaStore.hpp | 10 ++++-- .../CurlHTTPRequestManager.cpp | 22 ++++++++++--- valdi/test/standalone/CaStore_tests.cpp | 32 +++++++++++++++++-- .../StandaloneRequestManager_tests.cpp | 2 +- 6 files changed, 68 insertions(+), 15 deletions(-) diff --git a/src/valdi_modules/src/valdi/valdi_http/src/HTTPClient.ts b/src/valdi_modules/src/valdi/valdi_http/src/HTTPClient.ts index d00a42f1c..e4d3be012 100644 --- a/src/valdi_modules/src/valdi/valdi_http/src/HTTPClient.ts +++ b/src/valdi_modules/src/valdi/valdi_http/src/HTTPClient.ts @@ -46,7 +46,9 @@ export class HTTPClient implements IHTTPClient { if (response) { resolve(response); } else { - reject(error); + // A request manager reports its failure as text, so wrap it: callers should not have to + // ask whether the rejection they caught is a string or an Error. + reject(error instanceof Error ? error : new Error(String(error))); } }); } catch (err: unknown) { diff --git a/valdi/src/valdi/standalone_http/CaStore.cpp b/valdi/src/valdi/standalone_http/CaStore.cpp index 72e603dd2..557f21e55 100644 --- a/valdi/src/valdi/standalone_http/CaStore.cpp +++ b/valdi/src/valdi/standalone_http/CaStore.cpp @@ -18,10 +18,14 @@ std::string firstSetVariable(std::initializer_list variables) { return {}; } +bool isKind(const char* candidate, mode_t kind) { + struct stat info; + return stat(candidate, &info) == 0 && (info.st_mode & S_IFMT) == kind; +} + std::string firstMatching(std::initializer_list candidates, mode_t kind) { for (const char* candidate : candidates) { - struct stat info; - if (stat(candidate, &info) == 0 && (info.st_mode & S_IFMT) == kind) { + if (isKind(candidate, kind)) { return candidate; } } @@ -40,6 +44,11 @@ CaStore requestedCaStore(const StringBox& configured) { firstSetVariable({"CURL_CA_PATH", "SSL_CERT_DIR"})}; } +bool caStoreExists(const CaStore& store) { + return (!store.file.empty() && isKind(store.file.c_str(), S_IFREG)) || + (!store.path.empty() && isKind(store.path.c_str(), S_IFDIR)); +} + CaStore installedCaStore() { return {firstMatching( { diff --git a/valdi/src/valdi/standalone_http/CaStore.hpp b/valdi/src/valdi/standalone_http/CaStore.hpp index 1d48a8886..987b5c591 100644 --- a/valdi/src/valdi/standalone_http/CaStore.hpp +++ b/valdi/src/valdi/standalone_http/CaStore.hpp @@ -22,10 +22,14 @@ struct CaStore { */ CaStore requestedCaStore(const StringBox& configured); +/** Whether a store is actually on this machine: a bundle that is a file, or a path that is a directory. */ +bool caStoreExists(const CaStore& store); + /** - * Where the distributions keep their trust stores. Load bearing on Linux: @curl compiles in no CA - * bundle unless --@curl//:ca_bundle says so and is built without CURL_CA_FALLBACK, so BoringSSL - * would otherwise verify against an empty store. macOS gets SecureTransport and the keychain. + * Where the distributions keep their trust stores, for when the one curl compiled in is not on this + * machine. @curl hardcodes that per OS — Debian's bundle path on every Linux — so a build on RHEL or + * openSUSE is pointed at a file that does not exist, and BoringSSL, which is the backend on macOS + * too, would verify against an empty store. Compare with caStoreExists before relying on it. */ CaStore installedCaStore(); diff --git a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp index ece234e1d..4dd07bdf5 100644 --- a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp +++ b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp @@ -46,19 +46,24 @@ constexpr int kPollTimeoutMs = 10000; // begins, so sleeping this long strands nothing. Finite only so a lost wakeup would right itself. constexpr int kIdlePollTimeoutMs = 24 * 60 * 60 * 1000; -bool curlHasItsOwnCaStore() { +// What curl compiled in as its default. @curl hardcodes this per OS rather than probing, so on any +// Linux it names Debian's bundle whether or not this machine is Debian, and reports it whether or not +// the file is there. +CaStore curlsOwnCaStore() { auto* easy = curl_easy_init(); if (easy == nullptr) { - return false; + return {}; } char* file = nullptr; char* path = nullptr; curl_easy_getinfo(easy, CURLINFO_CAINFO, &file); curl_easy_getinfo(easy, CURLINFO_CAPATH, &path); + + CaStore store{file != nullptr ? file : "", path != nullptr ? path : ""}; curl_easy_cleanup(easy); - return file != nullptr || path != nullptr; + return store; } // Always a map, never null: HTTPTypes.d.ts declares headers as StringMap, and iOS, Android @@ -310,9 +315,11 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { return; } - // Probed last, so a deliberate --@curl//:ca_bundle is never overridden. + // Probed last, so a deliberate --@curl//:ca_bundle is never overridden. Its existence is what + // decides, not merely that curl reports one: a compiled-in default naming a file this machine + // does not have must not suppress the probe, or the store that is here goes unfound. _caStore = requestedCaStore(caBundlePath); - if (_caStore.empty() && !curlHasItsOwnCaStore()) { + if (_caStore.empty() && !caStoreExists(curlsOwnCaStore())) { _caStore = installedCaStore(); } @@ -544,6 +551,11 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { curl_easy_setopt(easy, CURLOPT_LOW_SPEED_TIME, static_cast(_idleTimeoutSeconds)); } + // curl's own floor is TLS 1.0, and iOS (App Transport Security) and Android both refuse + // anything below 1.2. Without this the same request gets a weaker floor on the CLI than in + // the app it shares its code with. + curl_easy_setopt(easy, CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1_2); + if (!_caStore.file.empty()) { curl_easy_setopt(easy, CURLOPT_CAINFO, _caStore.file.c_str()); } diff --git a/valdi/test/standalone/CaStore_tests.cpp b/valdi/test/standalone/CaStore_tests.cpp index 52f13aee0..b7f421d0a 100644 --- a/valdi/test/standalone/CaStore_tests.cpp +++ b/valdi/test/standalone/CaStore_tests.cpp @@ -121,9 +121,9 @@ TEST_F(CaStoreFixture, findsNothingWhenNothingIsAskedFor) { EXPECT_TRUE(store.empty()); } -// The @curl dep compiles in no trust store and is built without CURL_CA_FALLBACK, so on Linux -// whatever this finds is the only thing standing between BoringSSL and rejecting every certificate. -// Debian and Ubuntu are the common case and neither is served by the RHEL or openSUSE paths. +// @curl compiles in Debian's bundle path on every Linux, so on RHEL, Fedora or openSUSE what this +// finds is the only thing standing between BoringSSL and rejecting every certificate. All four are +// listed because no one of them serves the others. TEST_F(CaStoreFixture, namesTheTrustStoreOfTheCommonDistributions) { // Not a filesystem probe: this asserts the list, since the machine running the test has at most // one of these and the point is that all of them are covered. @@ -146,4 +146,30 @@ TEST_F(CaStoreFixture, namesTheTrustStoreOfTheCommonDistributions) { GTEST_SKIP() << "no distribution bundle on this machine to resolve"; } +// The reason the resolution checks existence rather than trusting what curl reports. A build on RHEL +// or Fedora carries Debian's bundle path, and taking that as a trust store skips the probe that would +// have found the one this machine actually has. +TEST_F(CaStoreFixture, doesNotCountABundleThatIsNotThere) { + EXPECT_FALSE(caStoreExists({"/no/such/bundle.pem", ""})); + EXPECT_FALSE(caStoreExists({"", "/no/such/certs"})); + EXPECT_FALSE(caStoreExists({})); +} + +TEST_F(CaStoreFixture, countsAStoreThatIsThere) { + EXPECT_TRUE(caStoreExists({"", "/etc"})) << "a directory that exists was not taken as a CAPATH"; + + // Whichever of the two kinds this machine has; both branches matter and neither is guaranteed. + auto installed = installedCaStore(); + if (installed.empty()) { + GTEST_SKIP() << "no distribution trust store on this machine to check against"; + } + EXPECT_TRUE(caStoreExists(installed)); +} + +// A bundle is a file and a path is a directory, so the kinds are not interchangeable: handing curl a +// directory as CAINFO fails to load, and a file as CAPATH is ignored. +TEST_F(CaStoreFixture, doesNotAcceptADirectoryAsABundle) { + EXPECT_FALSE(caStoreExists({"/etc", ""})); +} + } // namespace ValdiTest diff --git a/valdi/test/standalone/StandaloneRequestManager_tests.cpp b/valdi/test/standalone/StandaloneRequestManager_tests.cpp index 558a7ce01..a4bc5e05a 100644 --- a/valdi/test/standalone/StandaloneRequestManager_tests.cpp +++ b/valdi/test/standalone/StandaloneRequestManager_tests.cpp @@ -165,7 +165,7 @@ TEST_P(StandaloneRequestManagerFixture, httpClientRejectsItsPromiseOnFailure) { auto outcome = evaluate("return global.__outcome;"); ASSERT_TRUE(outcome) << outcome.description(); - EXPECT_EQ("rejected: No mocked response for given request", outcome.value().toString()) + EXPECT_EQ("rejected: Error: No mocked response for given request", outcome.value().toString()) << "the promise was left pending, which is what hangs a CLI: beginKeepAlive holds the runtime " "open waiting for a settlement that never comes"; } From 8a2ff0856a90d45fecfd6eed66adc2ca8c05c79a Mon Sep 17 00:00:00 2001 From: tx3stn <14163530+tx3stn@users.noreply.github.com> Date: Fri, 28 Aug 2026 13:12:13 +0100 Subject: [PATCH 14/18] fix: set user agent to avoid no ua rejections --- docs/docs/stdlib-http.md | 14 +++++++ .../CurlHTTPRequestManager.cpp | 11 +++++ .../CurlHTTPRequestManager_tests.cpp | 40 +++++++++++++++++++ 3 files changed, 65 insertions(+) diff --git a/docs/docs/stdlib-http.md b/docs/docs/stdlib-http.md index 06e365fa5..0c9eac30a 100644 --- a/docs/docs/stdlib-http.md +++ b/docs/docs/stdlib-http.md @@ -372,3 +372,17 @@ The `valdi_http` module works on: Network requests are always performed asynchronously and will not block the JavaScript thread. +### CLI differences + +The CLI's HTTP client is libcurl rather than a platform user agent, so a few things differ from iOS +and Android: + +- **No response compression.** Responses arrive uncompressed, and a response that carries a + `Content-Encoding` this build cannot decode fails rather than handing back unreadable bytes. +- **`Accept-Encoding` and `Content-Length` request headers are ignored**, as `fetch()` ignores them + on the web. The transport sets both itself. +- **A non-standard method survives a 303 redirect** instead of becoming `GET`. Standard methods are + rewritten per RFC 9110; its body is dropped either way. +- **Certificates verify against the system trust store**, found via `SSL_CERT_FILE`, `SSL_CERT_DIR`, + `CURL_CA_BUNDLE` or `CURL_CA_PATH`, or else the usual distribution paths. If none is found, every + HTTPS request fails rather than succeeding unverified. diff --git a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp index 4dd07bdf5..85b216a75 100644 --- a/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp +++ b/valdi/src/valdi/standalone_http/CurlHTTPRequestManager.cpp @@ -66,6 +66,14 @@ CaStore curlsOwnCaStore() { return store; } +// Neither the iOS nor the Android manager sets a User-Agent: NSURLSession and HttpURLConnection each +// send the transport's own, and Valdi never names itself. libcurl is the exception in sending none at +// all, which a fair number of CDNs answer with a 403, so it names itself the way the curl tool does. +const char* defaultUserAgent() { + static const std::string agent = std::string("curl/") + curl_version_info(CURLVERSION_NOW)->version; + return agent.c_str(); +} + // Always a map, never null: HTTPTypes.d.ts declares headers as StringMap, and iOS, Android // and web all hand back {} when a response carried none. Value emptyHeaderMap() { @@ -496,6 +504,9 @@ class CurlHTTPRequestManager : public snap::valdi_core::HTTPRequestManager { curl_easy_setopt(easy, CURLOPT_CUSTOMREQUEST, method.c_str()); } + // Set before the caller's headers so that one of their own still replaces it. + curl_easy_setopt(easy, CURLOPT_USERAGENT, defaultUserAgent()); + for (const auto& key : request.headers.sortedMapKeys()) { // The transport's to set, not the caller's: curl frames the body itself, and asking for // an encoding would get one back that this build has no codec to decode. fetch forbids diff --git a/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp b/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp index 8fae8abb8..7aa208655 100644 --- a/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp +++ b/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp @@ -781,6 +781,46 @@ TEST(CurlHTTPRequestManagerTests, overridesACurlDefaultHeader) { << requests[0]; } +// libcurl sends no User-Agent unless told to, where NSURLSession and HttpURLConnection both send the +// transport's own. A request without one is refused outright by a fair number of CDNs. +TEST(CurlHTTPRequestManagerTests, sendsAUserAgentNamingTheTransport) { + ScriptedServer server({okResponse()}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest(makeGet(server.url("/").c_str()), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + ASSERT_EQ(completion->statusCode(), 200); + + auto requests = server.requests(); + ASSERT_EQ(requests.size(), 1u); + EXPECT_NE(requests[0].find("User-Agent: curl/"), std::string::npos) + << "no User-Agent reached the wire, so a CDN that requires one refuses this. Request was:\n" + << requests[0]; +} + +TEST(CurlHTTPRequestManagerTests, overridesTheDefaultUserAgent) { + ScriptedServer server({okResponse()}); + + auto manager = makeCurlHTTPRequestManager(); + auto completion = std::make_shared(); + manager->performRequest( + makeGetWithHeaders(server.url("/").c_str(), makeHeaders({{"User-Agent", "Atolla/1.0"}})), completion); + + ASSERT_TRUE(completion->waitForCompletion(std::chrono::seconds(10))); + ASSERT_EQ(completion->statusCode(), 200); + + auto requests = server.requests(); + ASSERT_EQ(requests.size(), 1u); + EXPECT_NE(requests[0].find("User-Agent: Atolla/1.0\r\n"), std::string::npos) + << "an app naming itself was ignored. Request was:\n" + << requests[0]; + EXPECT_EQ(requests[0].find("User-Agent: curl/"), std::string::npos) + << "the default went out alongside the caller's, so the server sees two. Request was:\n" + << requests[0]; +} + TEST(CurlHTTPRequestManagerTests, coercesNonStringHeaderValues) { ScriptedServer server({okResponse()}); From fb19fecdefa55b39e9532c1c720de3318f4e2a12 Mon Sep 17 00:00:00 2001 From: tx3stn <14163530+tx3stn@users.noreply.github.com> Date: Fri, 28 Aug 2026 13:32:09 +0100 Subject: [PATCH 15/18] fix: move tests back to integration target now that's all working again --- valdi/BUILD.bazel | 24 ++++--------------- .../CaStore_tests.cpp | 0 .../CurlHTTPRequestManager_tests.cpp | 0 .../StandaloneRequestManager_tests.cpp | 0 4 files changed, 4 insertions(+), 20 deletions(-) rename valdi/test/{standalone => integration}/CaStore_tests.cpp (100%) rename valdi/test/{standalone => integration}/CurlHTTPRequestManager_tests.cpp (100%) rename valdi/test/{standalone => integration}/StandaloneRequestManager_tests.cpp (100%) diff --git a/valdi/BUILD.bazel b/valdi/BUILD.bazel index f25ce878d..91c2bd634 100644 --- a/valdi/BUILD.bazel +++ b/valdi/BUILD.bazel @@ -930,9 +930,13 @@ valdi_test( deps = [ ":test_utils", ":valdi_runtime_with_vm", + ":valdi_standalone_http", "//src/valdi_modules/src/cpp/valdi_http:valdi_http_cpp", "//src/valdi_modules/src/valdi/persistence:persistence_native", "//src/valdi_modules/src/valdi/valdi_core:valdi_core_native", + # The JavaScript side of valdi_http, for driving HTTPClient and asserting on the promise it + # hands back rather than only the native performRequest binding. + "//src/valdi_modules/src/valdi/valdi_http:valdi_http_native", "//src/valdi_modules/src/valdi/valdi_protobuf:valdi_protobuf_native", "//tsn", "//valdi/testdata/resources/modules/local:local_native", @@ -987,26 +991,6 @@ cc_library( ], ) -valdi_test( - name = "test_standalone", - srcs = glob(["test/standalone/**/*.cpp"]) + [ - "test/integration/JSBridgeTestFixture.cpp", - ], - hdrs = glob([ - "test/integration/**/*.hpp", - "test/standalone/**/*.hpp", - ]), - deps = [ - ":test_utils", - ":valdi_runtime_with_vm", - ":valdi_standalone_http", - "//src/valdi_modules/src/cpp/valdi_http:valdi_http_cpp", - # The JavaScript side of valdi_http. Tests need this to drive HTTPClient and assert on the - # promise it hands back, rather than only the native performRequest binding. - "//src/valdi_modules/src/valdi/valdi_http:valdi_http_native", - "//src/valdi_modules/src/valdi/valdi_core:valdi_core_native", - ], -) valdi_test( name = "test_hermes", diff --git a/valdi/test/standalone/CaStore_tests.cpp b/valdi/test/integration/CaStore_tests.cpp similarity index 100% rename from valdi/test/standalone/CaStore_tests.cpp rename to valdi/test/integration/CaStore_tests.cpp diff --git a/valdi/test/standalone/CurlHTTPRequestManager_tests.cpp b/valdi/test/integration/CurlHTTPRequestManager_tests.cpp similarity index 100% rename from valdi/test/standalone/CurlHTTPRequestManager_tests.cpp rename to valdi/test/integration/CurlHTTPRequestManager_tests.cpp diff --git a/valdi/test/standalone/StandaloneRequestManager_tests.cpp b/valdi/test/integration/StandaloneRequestManager_tests.cpp similarity index 100% rename from valdi/test/standalone/StandaloneRequestManager_tests.cpp rename to valdi/test/integration/StandaloneRequestManager_tests.cpp From f32d2efae66a6ecfa7f7a64540e3e49515724a8a Mon Sep 17 00:00:00 2001 From: tx3stn <14163530+tx3stn@users.noreply.github.com> Date: Fri, 28 Aug 2026 16:40:43 +0100 Subject: [PATCH 16/18] fix: bump test timeout --- valdi/BUILD.bazel | 1 + 1 file changed, 1 insertion(+) diff --git a/valdi/BUILD.bazel b/valdi/BUILD.bazel index 91c2bd634..0ca17a5d5 100644 --- a/valdi/BUILD.bazel +++ b/valdi/BUILD.bazel @@ -1012,6 +1012,7 @@ valdi_test( cc_test( name = "test", + timeout = "long", linkstatic = 1, visibility = ["//visibility:public"], deps = [ From 401efa8baaa3619bf05e6bf9b13eeaf52813d8de Mon Sep 17 00:00:00 2001 From: tx3stn <14163530+tx3stn@users.noreply.github.com> Date: Thu, 3 Sep 2026 21:49:41 +0100 Subject: [PATCH 17/18] chore: move standalone tests into their own tragte to get curl out of integration tests --- valdi/BUILD.bazel | 11 ++++++++++- .../CaStore_tests.cpp | 0 .../CurlHTTPRequestManager_tests.cpp | 0 3 files changed, 10 insertions(+), 1 deletion(-) rename valdi/test/{integration => standalone_http}/CaStore_tests.cpp (100%) rename valdi/test/{integration => standalone_http}/CurlHTTPRequestManager_tests.cpp (100%) diff --git a/valdi/BUILD.bazel b/valdi/BUILD.bazel index 0ca17a5d5..898c5d7ba 100644 --- a/valdi/BUILD.bazel +++ b/valdi/BUILD.bazel @@ -930,7 +930,6 @@ valdi_test( deps = [ ":test_utils", ":valdi_runtime_with_vm", - ":valdi_standalone_http", "//src/valdi_modules/src/cpp/valdi_http:valdi_http_cpp", "//src/valdi_modules/src/valdi/persistence:persistence_native", "//src/valdi_modules/src/valdi/valdi_core:valdi_core_native", @@ -991,6 +990,16 @@ cc_library( ], ) +# Kept out of :test_integration and :test so those stay curl-free: Snap's internal build does not +# consume this repo's MODULE.bazel, so it cannot resolve @curl. +valdi_test( + name = "test_standalone_http", + srcs = glob(["test/standalone_http/**/*.cpp"]), + deps = [ + ":valdi_standalone_http", + "//valdi_core:valdi_core_cc", + ], +) valdi_test( name = "test_hermes", diff --git a/valdi/test/integration/CaStore_tests.cpp b/valdi/test/standalone_http/CaStore_tests.cpp similarity index 100% rename from valdi/test/integration/CaStore_tests.cpp rename to valdi/test/standalone_http/CaStore_tests.cpp diff --git a/valdi/test/integration/CurlHTTPRequestManager_tests.cpp b/valdi/test/standalone_http/CurlHTTPRequestManager_tests.cpp similarity index 100% rename from valdi/test/integration/CurlHTTPRequestManager_tests.cpp rename to valdi/test/standalone_http/CurlHTTPRequestManager_tests.cpp From af48040c07ceea65016932c9c25109bc1315e3b3 Mon Sep 17 00:00:00 2001 From: tx3stn <14163530+tx3stn@users.noreply.github.com> Date: Thu, 3 Sep 2026 21:50:32 +0100 Subject: [PATCH 18/18] test: add CI test target that runs standalone tests --- .github/workflows/bzl-changes.yml | 9 +++++++++ tools/ci/test_standalone.sh | 13 +++++++++++++ 2 files changed, 22 insertions(+) create mode 100755 tools/ci/test_standalone.sh diff --git a/.github/workflows/bzl-changes.yml b/.github/workflows/bzl-changes.yml index adb89b4ce..cf146bd52 100644 --- a/.github/workflows/bzl-changes.yml +++ b/.github/workflows/bzl-changes.yml @@ -176,6 +176,9 @@ jobs: # iOS/macOS ObjC/Swift suites that never execute on the Linux runner. run: ./tools/ci/run_tests.sh + - name: "Run: Standalone Tests" + run: ./tools/ci/test_standalone.sh + # Note: Linux CLI distribution tests moved to dedicated workflow: # .github/workflows/test-cli-linux.yml # This avoids duplication and provides better test organization. @@ -191,6 +194,8 @@ jobs: include: - name: C++ Tests task: test-cpp + - name: Standalone Tests + task: test-standalone - name: Build Compiler task: build-compiler - name: Build & Export @@ -242,6 +247,10 @@ jobs: if: matrix.task == 'test-cpp' run: ./tools/ci/run_tests.sh + - name: "Run: Standalone Tests" + if: matrix.task == 'test-standalone' + run: ./tools/ci/test_standalone.sh + - name: "Run: Build Compiler" if: matrix.task == 'build-compiler' run: ./tools/ci/build_compiler.sh diff --git a/tools/ci/test_standalone.sh b/tools/ci/test_standalone.sh new file mode 100755 index 000000000..250d01552 --- /dev/null +++ b/tools/ci/test_standalone.sh @@ -0,0 +1,13 @@ +#!/usr/bin/env bash + +# Tests for the standalone runtime whose deps come from this repo's MODULE.bazel. +set -eux + +( + + # Intended to be run from open_source/ + cd "$(dirname "$0")/../.." + + bzl test //valdi:test_standalone_http --test_output=errors + +)