From dd4a2c17593283456884db18fa32ea01851fd2c5 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Wed, 29 May 2024 20:51:37 +0200 Subject: [PATCH] libstore: fix http abuses no longer working while refactoring the curl wrapper we inadvertently broken the immutable flake protocol, because the immutable flake protocol accumulates headers across the entire redirect chain instead of using only the headers given in the final response of the chain. this is a problem because Some Known Providers Of Flake Infrastructure set rel=immutable link headers only in the penultimate entry of the redirect chain, and curl does not regard it as worth returning to us via its response header enumeration mechanisms. fixes https://git.lix.systems/lix-project/lix/issues/358 Change-Id: I645c3932b465cde848bd6a3565925a1e3cbcdda0 --- src/libstore/filetransfer.cc | 70 +++++++++++------------------ tests/unit/libstore/filetransfer.cc | 53 +++++++++++++++++++--- 2 files changed, 73 insertions(+), 50 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 492463a61..076a5ca56 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -51,7 +51,6 @@ struct curlFileTransfer : public FileTransfer std::function dataCallback; CURL * req = 0; bool active = false; // whether the handle has been added to the multi object - bool headersProcessed = false; std::string statusMsg; unsigned int attempt = 0; @@ -136,35 +135,11 @@ struct curlFileTransfer : public FileTransfer std::exception_ptr writeException; - std::optional getHeader(const char * name) - { - curl_header * result; - auto e = curl_easy_header(req, name, 0, CURLH_HEADER, -1, &result); - if (e == CURLHE_OK) { - return result->value; - } else if (e == CURLHE_MISSING || e == CURLHE_NOHEADERS) { - return std::nullopt; - } else { - throw nix::Error("unexpected error from curl_easy_header(): %i", e); - } - } - size_t writeCallback(void * contents, size_t size, size_t nmemb) { const size_t realSize = size * nmemb; try { - if (!headersProcessed) { - if (auto h = getHeader("content-encoding")) { - encoding = std::move(*h); - } - if (auto h = getHeader("accept-ranges"); h && *h == "bytes") { - acceptRanges = true; - } - - headersProcessed = true; - } - result.bodySize += realSize; if (successfulStatuses.count(getHTTPStatus()) && this->dataCallback) { @@ -200,7 +175,31 @@ struct curlFileTransfer : public FileTransfer statusMsg = trim(match.str(1)); acceptRanges = false; encoding = ""; - headersProcessed = false; + } else { + auto i = line.find(':'); + if (i != std::string::npos) { + std::string name = toLower(trim(line.substr(0, i))); + + if (name == "etag") { + result.etag = trim(line.substr(i + 1)); + } + + else if (name == "content-encoding") + encoding = trim(line.substr(i + 1)); + + else if (name == "accept-ranges" && toLower(trim(line.substr(i + 1))) == "bytes") + acceptRanges = true; + + else if (name == "link" || name == "x-amz-meta-link") { + auto value = trim(line.substr(i + 1)); + static std::regex linkRegex("<([^>]*)>; rel=\"immutable\"", std::regex::extended | std::regex::icase); + if (std::smatch match; std::regex_match(value, match, linkRegex)) + result.immutableUrl = match.str(1); + else + debug("got invalid link header '%s'", value); + warn("foo %s", value); + } + } } return realSize; } @@ -336,25 +335,6 @@ struct curlFileTransfer : public FileTransfer debug("finished %s of '%s'; curl status = %d, HTTP status = %d, body = %d bytes", request.verb(), request.uri, code, httpStatus, result.bodySize); - auto link = getHeader("link"); - if (!link) { - link = getHeader("x-amz-meta-link"); - } - if (link) { - static std::regex linkRegex( - "<([^>]*)>; rel=\"immutable\"", std::regex::extended | std::regex::icase - ); - if (std::smatch match; std::regex_match(*link, match, linkRegex)) { - result.immutableUrl = match.str(1); - } else { - debug("got invalid link header '%s'", *link); - } - } - - if (auto etag = getHeader("etag")) { - result.etag = std::move(*etag); - } - // this has to happen here until we can return an actual future. // wrapping user `callback`s instead is not possible because the // Callback api expects std::functions, and copying Callbacks is diff --git a/tests/unit/libstore/filetransfer.cc b/tests/unit/libstore/filetransfer.cc index 684697c69..0e5c0965e 100644 --- a/tests/unit/libstore/filetransfer.cc +++ b/tests/unit/libstore/filetransfer.cc @@ -23,10 +23,19 @@ using namespace std::chrono_literals; +namespace { + +struct Reply { + std::string status, headers; + std::function content; +}; + +} + namespace nix { static std::tuple -serveHTTP(std::string_view status, std::string_view headers, std::function content) +serveHTTP(std::vector replies) { AutoCloseFD listener(::socket(AF_INET6, SOCK_STREAM, 0)); if (!listener) { @@ -52,7 +61,7 @@ serveHTTP(std::string_view status, std::string_view headers, std::function +serveHTTP(std::string status, std::string headers, std::function content) +{ + return serveHTTP({{{status, headers, content}}}); +} + TEST(FileTransfer, exceptionAbortsDownload) { struct Done @@ -166,4 +183,30 @@ TEST(FileTransfer, NOT_ON_DARWIN(handlesContentEncoding)) ft->download(FileTransferRequest(fmt("http://[::1]:%d/index", port)), sink); EXPECT_EQ(sink.s, original); } + +TEST(FileTransfer, usesIntermediateLinkHeaders) +{ + auto [port, srv] = serveHTTP({ + {"301 ok", + "location: /second\r\n" + "content-length: 0\r\n", + [] { return ""; }}, + {"307 ok", + "location: /third\r\n" + "content-length: 0\r\n", + [] { return ""; }}, + {"307 ok", + "location: /fourth\r\n" + "link: ; rel=\"immutable\"\r\n" + "content-length: 0\r\n", + [] { return ""; }}, + {"200 ok", "content-length: 1\r\n", [] { return "a"; }}, + }); + auto ft = makeFileTransfer(); + FileTransferRequest req(fmt("http://[::1]:%d/first", port)); + req.baseRetryTimeMs = 0; + auto result = ft->download(req); + ASSERT_EQ(result.immutableUrl, "http://foo"); +} + }