This is an automated email from the ASF dual-hosted git repository.
wgtmac pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-cpp.git
The following commit(s) were added to refs/heads/main by this push:
new 059a1164 fix(rest): do not follow HTTP redirects in HttpClient (#1024)
059a1164 is described below
commit 059a116435698db0397594abf35c104331f8a234
Author: lishuxu <[email protected]>
AuthorDate: Mon Oct 19 13:05:41 2026 +0800
fix(rest): do not follow HTTP redirects in HttpClient (#1024)
Signed requests are bound to the exact request URL and cannot be
transparently re-signed for a redirect target, and custom headers set
on the request are forwarded verbatim when cpr follows a redirect.
Disable redirect following for all HttpClient requests so that a 3xx
response is surfaced to the caller as an error.
---
src/iceberg/catalog/rest/http_client.cc | 30 +++--
src/iceberg/test/http_client_test.cc | 189 ++++++++++++++++++++++++++++++++
2 files changed, 211 insertions(+), 8 deletions(-)
diff --git a/src/iceberg/catalog/rest/http_client.cc
b/src/iceberg/catalog/rest/http_client.cc
index 871a21e6..718a5b73 100644
--- a/src/iceberg/catalog/rest/http_client.cc
+++ b/src/iceberg/catalog/rest/http_client.cc
@@ -75,6 +75,17 @@ namespace {
/// \brief Default error type for unparseable REST responses.
constexpr std::string_view kRestExceptionType = "RESTException";
+/// \brief Do not follow HTTP redirects on catalog requests.
+///
+/// cpr follows redirects by default. Signed requests are bound to the exact
+/// request URL and cannot be re-signed for a redirect target, and headers set
+/// on the request (e.g. the relocated `Original-Authorization` header used by
+/// SigV4) would be sent unchanged to the new location. A redirect response is
+/// therefore surfaced to the caller as an error instead of being followed.
+const cpr::Redirect kNoFollowRedirects{/*maximum=*/0L, /*follow=*/false,
+ /*cont_send_cred=*/false,
+ cpr::PostRedirectFlags::POST_ALL};
+
/// \brief Merge default headers with per-request headers (per-request wins).
HttpHeaders MergeHeaders(
const std::unordered_map<std::string, std::string>& default_headers,
@@ -204,7 +215,8 @@ Result<HttpResponse> HttpClient::Get(
AuthenticateRequest(session, HttpMethod::kGet,
std::move(url),
MergeHeaders(default_headers_,
headers)));
cpr::Response response =
- cpr::Get(cpr::Url{authenticated.url}, ToCprHeader(authenticated),
*connection_pool_,
+ cpr::Get(cpr::Url{authenticated.url}, ToCprHeader(authenticated),
+ kNoFollowRedirects, *connection_pool_,
cpr::ConnectTimeout{connection_timeout_},
cpr::Timeout{socket_timeout_});
ICEBERG_RETURN_UNEXPECTED(HandleFailureResponse(response, error_handler));
@@ -223,7 +235,7 @@ Result<HttpResponse> HttpClient::Post(
MergeHeaders(default_headers_, headers), body));
cpr::Response response =
cpr::Post(cpr::Url{authenticated.url}, cpr::Body{authenticated.body},
- ToCprHeader(authenticated), *connection_pool_,
+ ToCprHeader(authenticated), kNoFollowRedirects,
*connection_pool_,
cpr::ConnectTimeout{connection_timeout_},
cpr::Timeout{socket_timeout_});
ICEBERG_RETURN_UNEXPECTED(HandleFailureResponse(response, error_handler));
@@ -254,7 +266,7 @@ Result<HttpResponse> HttpClient::PostForm(
std::move(encoded_body)));
cpr::Response response =
cpr::Post(cpr::Url{authenticated.url}, cpr::Body{authenticated.body},
- ToCprHeader(authenticated), *connection_pool_,
+ ToCprHeader(authenticated), kNoFollowRedirects,
*connection_pool_,
cpr::ConnectTimeout{connection_timeout_},
cpr::Timeout{socket_timeout_});
ICEBERG_RETURN_UNEXPECTED(HandleFailureResponse(response, error_handler));
@@ -269,9 +281,10 @@ Result<HttpResponse> HttpClient::Head(
ICEBERG_ASSIGN_OR_RAISE(auto authenticated,
AuthenticateRequest(session, HttpMethod::kHead, path,
MergeHeaders(default_headers_,
headers)));
- cpr::Response response = cpr::Head(
- cpr::Url{authenticated.url}, ToCprHeader(authenticated),
*connection_pool_,
- cpr::ConnectTimeout{connection_timeout_}, cpr::Timeout{socket_timeout_});
+ cpr::Response response =
+ cpr::Head(cpr::Url{authenticated.url}, ToCprHeader(authenticated),
+ kNoFollowRedirects, *connection_pool_,
+ cpr::ConnectTimeout{connection_timeout_},
cpr::Timeout{socket_timeout_});
ICEBERG_RETURN_UNEXPECTED(HandleFailureResponse(response, error_handler));
HttpResponse http_response;
@@ -289,8 +302,9 @@ Result<HttpResponse> HttpClient::Delete(
AuthenticateRequest(session, HttpMethod::kDelete, std::move(url),
MergeHeaders(default_headers_, headers)));
cpr::Response response = cpr::Delete(
- cpr::Url{authenticated.url}, ToCprHeader(authenticated),
*connection_pool_,
- cpr::ConnectTimeout{connection_timeout_}, cpr::Timeout{socket_timeout_});
+ cpr::Url{authenticated.url}, ToCprHeader(authenticated),
kNoFollowRedirects,
+ *connection_pool_, cpr::ConnectTimeout{connection_timeout_},
+ cpr::Timeout{socket_timeout_});
ICEBERG_RETURN_UNEXPECTED(HandleFailureResponse(response, error_handler));
HttpResponse http_response;
diff --git a/src/iceberg/test/http_client_test.cc
b/src/iceberg/test/http_client_test.cc
index 4203e742..9e503729 100644
--- a/src/iceberg/test/http_client_test.cc
+++ b/src/iceberg/test/http_client_test.cc
@@ -20,17 +20,22 @@
#include "iceberg/catalog/rest/http_client.h"
#include <array>
+#include <atomic>
+#include <cctype>
#include <chrono>
+#include <cstddef>
#include <cstdint>
#include <format>
#include <string>
#include <string_view>
#include <thread>
+#include <utility>
#include <gmock/gmock.h>
#include <gtest/gtest.h>
#ifndef _WIN32
+# include <poll.h>
# include <unistd.h>
# include <arpa/inet.h>
@@ -49,6 +54,143 @@ namespace iceberg::rest {
namespace {
+/// \brief Minimal HTTP/1.1 server on 127.0.0.1 that answers every request with
+/// a fixed response and closes the connection.
+class LoopbackHttpServer {
+ public:
+ explicit LoopbackHttpServer(std::string response) :
response_(std::move(response)) {}
+
+ ~LoopbackHttpServer() {
+ stop_ = true;
+ if (thread_.joinable()) {
+ thread_.join();
+ }
+ if (listen_fd_ >= 0) {
+ close(listen_fd_);
+ }
+ }
+
+ LoopbackHttpServer(const LoopbackHttpServer&) = delete;
+ LoopbackHttpServer& operator=(const LoopbackHttpServer&) = delete;
+
+ /// \brief Bind an ephemeral port and start serving. Returns false on
failure.
+ bool Start() {
+ listen_fd_ = socket(AF_INET, SOCK_STREAM, 0);
+ if (listen_fd_ < 0) {
+ return false;
+ }
+ int reuse = 1;
+ setsockopt(listen_fd_, SOL_SOCKET, SO_REUSEADDR, &reuse, sizeof(reuse));
+
+ sockaddr_in addr{};
+ addr.sin_family = AF_INET;
+ addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
+ addr.sin_port = 0;
+ if (bind(listen_fd_, reinterpret_cast<sockaddr*>(&addr), sizeof(addr)) !=
0 ||
+ listen(listen_fd_, 8) != 0) {
+ return false;
+ }
+ socklen_t len = sizeof(addr);
+ if (getsockname(listen_fd_, reinterpret_cast<sockaddr*>(&addr), &len) !=
0) {
+ return false;
+ }
+ port_ = ntohs(addr.sin_port);
+ thread_ = std::thread([this] { Serve(); });
+ return true;
+ }
+
+ int port() const { return port_; }
+ int request_count() const { return request_count_.load(); }
+ std::string url(std::string_view path) const {
+ return std::format("http://127.0.0.1:{}{}";, port_, path);
+ }
+
+ private:
+ void Serve() {
+ while (!stop_) {
+ pollfd pfd{.fd = listen_fd_, .events = POLLIN, .revents = 0};
+ if (poll(&pfd, 1, /*timeout_ms=*/50) <= 0) {
+ continue;
+ }
+ int conn = accept(listen_fd_, nullptr, nullptr);
+ if (conn < 0) {
+ continue;
+ }
+ ReadRequest(conn);
+ ++request_count_;
+ SendAll(conn, response_);
+ close(conn);
+ }
+ }
+
+ /// \brief Consume the request headers and any Content-Length body so the
+ /// client does not see a connection reset before it reads the response.
+ static void ReadRequest(int conn) {
+ timeval timeout{.tv_sec = 10, .tv_usec = 0};
+ setsockopt(conn, SOL_SOCKET, SO_RCVTIMEO, &timeout, sizeof(timeout));
+ setsockopt(conn, SOL_SOCKET, SO_SNDTIMEO, &timeout, sizeof(timeout));
+
+ std::string request;
+ std::array<char, 4096> buf{};
+ size_t header_end = std::string::npos;
+ size_t content_length = 0;
+ while (true) {
+ ssize_t n = recv(conn, buf.data(), buf.size(), 0);
+ if (n <= 0) {
+ return;
+ }
+ request.append(buf.data(), static_cast<size_t>(n));
+ if (header_end == std::string::npos) {
+ header_end = request.find("\r\n\r\n");
+ if (header_end == std::string::npos) {
+ continue;
+ }
+ content_length = ParseContentLength(request.substr(0, header_end));
+ }
+ if (request.size() >= header_end + 4 + content_length) {
+ return;
+ }
+ }
+ }
+
+ static size_t ParseContentLength(std::string headers) {
+ for (auto& c : headers) {
+ c = static_cast<char>(std::tolower(static_cast<unsigned char>(c)));
+ }
+ constexpr std::string_view kHeader = "content-length:";
+ size_t pos = headers.find(kHeader);
+ if (pos == std::string::npos) {
+ return 0;
+ }
+ return std::stoul(headers.substr(pos + kHeader.size()));
+ }
+
+ static void SendAll(int conn, const std::string& data) {
+# ifdef MSG_NOSIGNAL
+ constexpr int kFlags = MSG_NOSIGNAL;
+# else
+ constexpr int kFlags = 0;
+ int no_sigpipe = 1;
+ setsockopt(conn, SOL_SOCKET, SO_NOSIGPIPE, &no_sigpipe,
sizeof(no_sigpipe));
+# endif
+ size_t sent = 0;
+ while (sent < data.size()) {
+ ssize_t n = send(conn, data.data() + sent, data.size() - sent, kFlags);
+ if (n <= 0) {
+ return;
+ }
+ sent += static_cast<size_t>(n);
+ }
+ }
+
+ std::string response_;
+ int listen_fd_ = -1;
+ int port_ = 0;
+ std::atomic<bool> stop_{false};
+ std::atomic<int> request_count_{0};
+ std::thread thread_;
+};
+
// A loopback socket listening on an ephemeral port.
struct Listener {
int fd = -1;
@@ -172,4 +314,51 @@ TEST(HttpClientTest,
PostReportsTransferFailureAfterSuccessfulStatus) {
#endif
+// Redirect responses must be reported without replaying the request at the
target.
+TEST(HttpClientTest, DoesNotFollowRedirects) {
+#ifdef _WIN32
+ GTEST_SKIP() << "Loopback test server is only implemented for POSIX";
+#else
+ for (const int status_code : {301, 302, 303, 307, 308}) {
+ SCOPED_TRACE(status_code);
+ LoopbackHttpServer target(
+ "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\n"
+ "Content-Length: 2\r\nConnection: close\r\n\r\n{}");
+ ASSERT_TRUE(target.Start());
+
+ LoopbackHttpServer redirector(
+ std::format("HTTP/1.1 {} Redirect\r\nLocation: {}\r\nContent-Length:
0\r\n"
+ "Connection: close\r\n\r\n",
+ status_code, target.url("/v1/config")));
+ ASSERT_TRUE(redirector.Start());
+
+ HttpClient client({}, std::chrono::seconds(10), std::chrono::seconds(10));
+ auto session = auth::AuthSession::MakeDefault(
+ {{"Authorization", "Bearer token"},
+ {"Original-Authorization", "Bearer delegate-token"},
+ {"X-Amz-Security-Token", "session-token"}});
+ const auto& handler = *DefaultErrorHandler::Instance();
+ const std::string url = redirector.url("/v1/config");
+
+ auto expect_not_followed = [&](const Result<HttpResponse>& result,
+ std::string_view method) {
+ SCOPED_TRACE(method);
+ EXPECT_THAT(result, IsError(ErrorKind::kRestError));
+ EXPECT_THAT(result, HasErrorMessage(std::format("code: {}",
status_code)));
+ EXPECT_EQ(target.request_count(), 0);
+ };
+
+ expect_not_followed(client.Get(url, {}, {}, handler, *session), "GET");
+ expect_not_followed(client.Post(url, "{}", {}, handler, *session), "POST");
+ expect_not_followed(client.PostForm(url, {{"grant_type",
"client_credentials"}}, {},
+ handler, *session),
+ "POST form");
+ expect_not_followed(client.Head(url, {}, handler, *session), "HEAD");
+ expect_not_followed(client.Delete(url, {}, {}, handler, *session),
"DELETE");
+
+ EXPECT_EQ(redirector.request_count(), 5);
+ }
+#endif
+}
+
} // namespace iceberg::rest
