From 9b7f1986f152c59439efca3ea06598dc3f753637 Mon Sep 17 00:00:00 2001 From: "shuxu.li" Date: Mon, 19 Oct 2026 14:43:10 +0800 Subject: [PATCH 1/2] feat(rest): add opt-in client-only-properties to protect oauth2-server-uri Introduce rest.client-only-properties, an opt-in mechanism that lets clients prevent server-provided defaults/overrides from affecting specified configuration keys. Keys listed in this property are resolved from client configuration only. When oauth2-server-uri appears in the list: - Server-provided values for it are ignored during config merge - The endpoint is resolved to an absolute URL using the client's uri before merging, preventing indirect override via uri changes - InitSession and CatalogSession now use consistent endpoints This addresses the risk of OAuth2 credentials being sent to an unintended endpoint when a REST catalog server overrides oauth2-server-uri or uri. The protection is opt-in to maintain backward compatibility and align with REST spec defaults. Changes: - Add RestCatalogProperties::kClientOnlyProperties - Export auth::ResolveOAuth2ServerUri for reuse - Add MergeServerConfig() and ParseClientOnlyProperties() in rest_util - Replace MergeConfigs with MergeServerConfig in RestCatalog::Make - Warn when oauth2-server-uri is neither explicit nor protected while using credential auth (following Java's warnIfOAuthServerUriNotSet) - Add unit tests covering direct/indirect override scenarios Default behavior unchanged: without setting rest.client-only-properties, server overrides apply as before, consistent with Java and Rust clients. Co-Authored-By: Claude --- .../catalog/rest/auth/auth_properties.cc | 4 +- .../catalog/rest/auth/auth_properties.h | 16 ++ src/iceberg/catalog/rest/catalog_properties.h | 4 + src/iceberg/catalog/rest/rest_catalog.cc | 31 +++- src/iceberg/catalog/rest/rest_util.cc | 67 ++++++++ src/iceberg/catalog/rest/rest_util.h | 26 +++ src/iceberg/test/rest_catalog_unit_test.cc | 109 +++++++++++++ src/iceberg/test/rest_util_test.cc | 149 ++++++++++++++++++ 8 files changed, 402 insertions(+), 4 deletions(-) diff --git a/src/iceberg/catalog/rest/auth/auth_properties.cc b/src/iceberg/catalog/rest/auth/auth_properties.cc index f373df617e..c183e9b278 100644 --- a/src/iceberg/catalog/rest/auth/auth_properties.cc +++ b/src/iceberg/catalog/rest/auth/auth_properties.cc @@ -36,6 +36,8 @@ std::pair ParseCredential(const std::string& credentia return {credential.substr(0, colon_pos), credential.substr(colon_pos + 1)}; } +} // namespace + Result ResolveOAuth2ServerUri( const std::unordered_map& properties) { auto endpoint_it = properties.find(AuthProperties::kOAuth2ServerUri.key()); @@ -61,8 +63,6 @@ Result ResolveOAuth2ServerUri( return base_uri + "/" + endpoint; } -} // namespace - std::unordered_map AuthProperties::optional_oauth_params() const { std::unordered_map params; diff --git a/src/iceberg/catalog/rest/auth/auth_properties.h b/src/iceberg/catalog/rest/auth/auth_properties.h index 8784194cce..1e0b460a0c 100644 --- a/src/iceberg/catalog/rest/auth/auth_properties.h +++ b/src/iceberg/catalog/rest/auth/auth_properties.h @@ -128,4 +128,20 @@ class ICEBERG_REST_EXPORT AuthProperties : public ConfigBase { std::optional expires_at_millis_; }; +/// \brief Resolve the OAuth2 server URI from a properties map. +/// +/// This function resolves the OAuth2 server URI by examining the +/// `oauth2-server-uri` key in the provided properties map. The resolution logic: +/// - If the value starts with `http://` or `https://`, it is returned as-is +/// (absolute URL). +/// - If the value is empty, it is returned as-is. +/// - Otherwise, the value is treated as a relative path and resolved against the +/// `uri` key from the properties map. If the relative path starts with `/`, it +/// is appended directly; otherwise, a `/` separator is inserted. +/// +/// \param properties Properties map containing `oauth2-server-uri` and `uri` keys. +/// \return Resolved OAuth2 server URI. +Result ResolveOAuth2ServerUri( + const std::unordered_map& properties); + } // namespace iceberg::rest::auth diff --git a/src/iceberg/catalog/rest/catalog_properties.h b/src/iceberg/catalog/rest/catalog_properties.h index a1f205a605..e9833ddbae 100644 --- a/src/iceberg/catalog/rest/catalog_properties.h +++ b/src/iceberg/catalog/rest/catalog_properties.h @@ -77,6 +77,10 @@ class ICEBERG_REST_EXPORT RestCatalogProperties "rest.client.connection-timeout-ms", 0}; /// \brief HTTP socket/read timeout in milliseconds, matching Java REST clients. inline static Entry kClientSocketTimeoutMs{"rest.client.socket-timeout-ms", 0}; + /// \brief Comma-separated keys that are resolved from client configuration + /// only; server-provided defaults and overrides for them are ignored. + inline static Entry kClientOnlyProperties{"rest.client-only-properties", + ""}; /// \brief Create a default RestCatalogProperties instance. static RestCatalogProperties default_properties(); diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index a945e25d4c..a21a90a232 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -35,6 +35,7 @@ #include #include "iceberg/catalog/rest/auth/auth_managers.h" +#include "iceberg/catalog/rest/auth/auth_properties.h" #include "iceberg/catalog/rest/auth/auth_session.h" #include "iceberg/catalog/rest/catalog_properties.h" #include "iceberg/catalog/rest/constant.h" @@ -481,8 +482,10 @@ Result> RestCatalog::Make( ICEBERG_ASSIGN_OR_RAISE(auto server_config, FetchServerConfig(*init_client, *paths, config, *init_session)); - RestCatalogProperties final_config = RestCatalogProperties::FromMap( - MergeConfigs(server_config.defaults, config.configs(), server_config.overrides)); + ICEBERG_ASSIGN_OR_RAISE(auto merged_config, + MergeServerConfig(server_config.defaults, + server_config.overrides, config.configs())); + RestCatalogProperties final_config = RestCatalogProperties::FromMap(merged_config); std::unordered_set endpoints; if (!server_config.endpoints.empty()) { @@ -511,6 +514,30 @@ Result> RestCatalog::Make( ICEBERG_ASSIGN_OR_RAISE(auto client, client_factory(final_config)); ICEBERG_PRECHECK(client != nullptr, "HTTP client factory returned null"); + + // Warn if oauth2-server-uri is neither explicitly configured nor protected from + // server overrides while using credential auth. + if (auto cred_it = final_config.configs().find(auth::AuthProperties::kCredential.key()); + cred_it != final_config.configs().end() && !cred_it->second.empty()) { + auto client_uri_it = + config.configs().find(auth::AuthProperties::kOAuth2ServerUri.key()); + bool endpoint_explicit = + client_uri_it != config.configs().end() && !client_uri_it->second.empty() && + client_uri_it->second != auth::AuthProperties::kOAuth2ServerUri.value(); + bool endpoint_protected = + ParseClientOnlyProperties( + config.Get(RestCatalogProperties::kClientOnlyProperties)) + .contains(auth::AuthProperties::kOAuth2ServerUri.key()); + if (!endpoint_explicit && !endpoint_protected) { + ICEBERG_LOG_WARN( + "OAuth2 server URI not explicitly configured; token endpoint will be derived " + "from catalog URI and may be affected by server overrides. Set '{}' explicitly " + "or add it to '{}' for stronger isolation.", + auth::AuthProperties::kOAuth2ServerUri.key(), + RestCatalogProperties::kClientOnlyProperties.key()); + } + } + ICEBERG_ASSIGN_OR_RAISE(auto catalog_session, auth_manager->CatalogSession(client, final_config.configs())); diff --git a/src/iceberg/catalog/rest/rest_util.cc b/src/iceberg/catalog/rest/rest_util.cc index cc69abe642..dd43efdad7 100644 --- a/src/iceberg/catalog/rest/rest_util.cc +++ b/src/iceberg/catalog/rest/rest_util.cc @@ -20,11 +20,15 @@ #include "iceberg/catalog/rest/rest_util.h" #include +#include #include #include +#include "iceberg/catalog/rest/auth/auth_properties.h" +#include "iceberg/catalog/rest/catalog_properties.h" #include "iceberg/catalog/rest/endpoint.h" +#include "iceberg/logging/log_macros.h" #include "iceberg/table_identifier.h" #include "iceberg/util/macros.h" @@ -135,6 +139,69 @@ std::unordered_map MergeConfigs( return merged; } +std::unordered_set ParseClientOnlyProperties(std::string_view value) { + constexpr std::string_view kWhitespace = " \t\n\r\f\v"; + + std::unordered_set result; + for (auto part : std::views::split(value, ',')) { + std::string_view key(part.begin(), part.size()); + if (auto first = key.find_first_not_of(kWhitespace); + first != std::string_view::npos) { + key = key.substr(first, key.find_last_not_of(kWhitespace) - first + 1); + result.emplace(key); + } + } + return result; +} + +Result> MergeServerConfig( + const std::unordered_map& server_defaults, + const std::unordered_map& server_overrides, + const std::unordered_map& client_configs) { + // Make mutable copies of server configs + auto defaults = server_defaults; + auto overrides = server_overrides; + + // Parse client-only properties list from client config only + std::string_view client_only_value; + if (auto it = client_configs.find(RestCatalogProperties::kClientOnlyProperties.key()); + it != client_configs.end()) { + client_only_value = it->second; + } + const auto client_only = ParseClientOnlyProperties(client_only_value); + + // Remove rest.client-only-properties itself from server configs + defaults.erase(RestCatalogProperties::kClientOnlyProperties.key()); + overrides.erase(RestCatalogProperties::kClientOnlyProperties.key()); + + // Remove client-only keys from server configs + for (const auto& key : client_only) { + if (auto it = defaults.find(key); it != defaults.end()) { + ICEBERG_LOG_INFO("Ignoring server-provided '{}' in defaults: listed in {}", key, + RestCatalogProperties::kClientOnlyProperties.key()); + defaults.erase(it); + } + if (auto it = overrides.find(key); it != overrides.end()) { + ICEBERG_LOG_INFO("Ignoring server-provided '{}' in overrides: listed in {}", key, + RestCatalogProperties::kClientOnlyProperties.key()); + overrides.erase(it); + } + } + + // If oauth2-server-uri is protected, resolve it to an absolute URL before merging + auto merged_client_configs = client_configs; + if (client_only.contains(auth::AuthProperties::kOAuth2ServerUri.key())) { + ICEBERG_ASSIGN_OR_RAISE(auto resolved_uri, + auth::ResolveOAuth2ServerUri(client_configs)); + if (!resolved_uri.empty()) { + merged_client_configs[auth::AuthProperties::kOAuth2ServerUri.key()] = + std::move(resolved_uri); + } + } + + return MergeConfigs(defaults, merged_client_configs, overrides); +} + std::string GetStandardReasonPhrase(int32_t status_code) { switch (status_code) { case 100: diff --git a/src/iceberg/catalog/rest/rest_util.h b/src/iceberg/catalog/rest/rest_util.h index 556b2557ad..fccafc178c 100644 --- a/src/iceberg/catalog/rest/rest_util.h +++ b/src/iceberg/catalog/rest/rest_util.h @@ -92,6 +92,32 @@ ICEBERG_REST_EXPORT std::unordered_map MergeConfigs( const std::unordered_map& client_configs, const std::unordered_map& server_overrides); +/// \brief Parse the rest.client-only-properties value. +/// +/// \details Parses a comma-separated list of property keys. Each key is taken from +/// client configuration only; server-provided values for them are ignored. Leading +/// and trailing whitespace is stripped from each key, and empty entries are ignored. +/// \param value The raw string value of rest.client-only-properties. +/// \return A set of property keys to protect from server override. +ICEBERG_REST_EXPORT std::unordered_set ParseClientOnlyProperties( + std::string_view value); + +/// \brief Resolve the effective catalog configuration from server-provided +/// defaults/overrides and client configuration. +/// +/// \details Keys listed in rest.client-only-properties are taken from client +/// configuration only: server-provided values for them are ignored. If +/// oauth2-server-uri is listed, it is resolved to an absolute URL before merging +/// to prevent indirect override via the uri property. +/// \param server_defaults Default properties provided by the server. +/// \param server_overrides Override properties enforced by the server. +/// \param client_configs Configuration properties from the client. +/// \return The merged configuration map with client-only properties protected. +ICEBERG_REST_EXPORT Result> +MergeServerConfig(const std::unordered_map& server_defaults, + const std::unordered_map& server_overrides, + const std::unordered_map& client_configs); + /// \brief Get the standard HTTP reason phrase for a status code. /// /// \details Returns the standard English reason phrase for common HTTP status codes. diff --git a/src/iceberg/test/rest_catalog_unit_test.cc b/src/iceberg/test/rest_catalog_unit_test.cc index 86865a6638..4755887251 100644 --- a/src/iceberg/test/rest_catalog_unit_test.cc +++ b/src/iceberg/test/rest_catalog_unit_test.cc @@ -29,6 +29,7 @@ #include #include "iceberg/catalog.h" +#include "iceberg/catalog/rest/auth/auth_properties.h" #include "iceberg/catalog/rest/catalog_properties.h" #include "iceberg/catalog/rest/endpoint.h" #include "iceberg/catalog/rest/error_handlers.h" @@ -76,6 +77,13 @@ class MockHttpClient : public HttpClient { const ErrorHandler& error_handler, auth::AuthSession& session), (override)); + MOCK_METHOD(Result, PostForm, + (const std::string& path, + (const std::unordered_map&)form_data, + (const std::unordered_map&)headers, + const ErrorHandler& error_handler, auth::AuthSession& session), + (override)); + MOCK_METHOD(Result, Delete, (const std::string& path, (const std::unordered_map&)params, @@ -559,6 +567,107 @@ TEST_P(RestCatalogScanLifetimeTest, ScanKeepsCredentialRefreshAlive) { INSTANTIATE_TEST_SUITE_P(ScanModes, RestCatalogScanLifetimeTest, ::testing::Bool()); +// -------------------------------------------------------------------------- +// Client-only properties tests +// -------------------------------------------------------------------------- + +class RestCatalogClientOnlyPropertiesTest : public ::testing::Test { + protected: + void SetUp() override { + mock_client_ = std::make_shared(); + FileIORegistry::Register( + "rest-catalog-unit-test-noop", + {.create = [](const std::unordered_map&) + -> Result> { + return std::make_unique(); + }}); + } + + // Runs RestCatalog::Make against a server that returns `server_config` from + // /v1/config, and returns the configurations handed to the HTTP client + // factory. The second entry is the merged configuration. + std::vector MakeWithServerConfig( + const CatalogConfig& server_config, + std::unordered_map client_properties) { + client_properties["io-impl"] = "rest-catalog-unit-test-noop"; + EXPECT_CALL(*mock_client_, Get(::testing::EndsWith("/v1/config"), _, _, _, _)) + .WillOnce(Return(HttpResponse(200, ToJson(server_config).dump()))); + EXPECT_CALL(*mock_client_, PostForm(_, _, _, _, _)) + .WillRepeatedly( + [](const std::string&, const std::unordered_map&, + const std::unordered_map&, const ErrorHandler&, + auth::AuthSession&) -> Result { + return HttpResponse(200, R"({"access_token":"tok","token_type":"bearer"})"); + }); + + auto config = RestCatalogProperties::FromMap(std::move(client_properties)); + std::vector captured; + auto result = + RestCatalog::Make(config, /*metrics_executor=*/nullptr, + [&captured, this](const RestCatalogProperties& properties) { + captured.push_back(properties); + return mock_client_; + }); + EXPECT_THAT(result, IsOk()); + return captured; + } + + std::shared_ptr mock_client_; +}; + +TEST_F(RestCatalogClientOnlyPropertiesTest, + ServerOverrideAppliesWhenNoClientOnlyPropertiesAreSet) { + auto configs = MakeWithServerConfig( + CatalogConfig{.defaults = {}, + .overrides = {{"oauth2-server-uri", "https://server.example/token"}}, + .endpoints = {}}, + {{"uri", "https://catalog.example"}, {"credential", "client:secret"}}); + + ASSERT_EQ(configs.size(), 2); + // Upstream REST behavior is unchanged when the list is empty. + EXPECT_EQ( + configs[1].configs().at(std::string(auth::AuthProperties::kOAuth2ServerUri.key())), + "https://server.example/token"); +} + +TEST_F(RestCatalogClientOnlyPropertiesTest, + ClientOnlyPropertiesProtectsExplicitOAuth2ServerUri) { + auto configs = MakeWithServerConfig( + CatalogConfig{.defaults = {}, + .overrides = {{"oauth2-server-uri", "https://server.example/token"}}, + .endpoints = {}}, + {{"uri", "https://catalog.example"}, + {"credential", "client:secret"}, + {"oauth2-server-uri", "https://idp.example/token"}, + {"rest.client-only-properties", "oauth2-server-uri"}}); + + ASSERT_EQ(configs.size(), 2); + EXPECT_EQ( + configs[1].configs().at(std::string(auth::AuthProperties::kOAuth2ServerUri.key())), + "https://idp.example/token"); +} + +TEST_F(RestCatalogClientOnlyPropertiesTest, + ClientOnlyPropertiesProtectsDefaultOAuth2ServerUriFromIndirectOverride) { + auto configs = MakeWithServerConfig( + CatalogConfig{.defaults = {}, + .overrides = {{"uri", "https://collector.example"}, + {"oauth2-server-uri", "https://server.example/token"}}, + .endpoints = {}}, + {{"uri", "https://catalog.example"}, + {"credential", "client:secret"}, + {"rest.client-only-properties", "oauth2-server-uri"}}); + + ASSERT_EQ(configs.size(), 2); + // The endpoint is pinned against the client URI before merging, so a server + // override of `uri` cannot move it. + EXPECT_EQ( + configs[1].configs().at(std::string(auth::AuthProperties::kOAuth2ServerUri.key())), + "https://catalog.example/v1/oauth/tokens"); + // `uri` itself is still overridable. + EXPECT_EQ(configs[1].Get(RestCatalogProperties::kUri), "https://collector.example"); +} + } // namespace } // namespace iceberg::rest diff --git a/src/iceberg/test/rest_util_test.cc b/src/iceberg/test/rest_util_test.cc index 0035afca08..ce7443994f 100644 --- a/src/iceberg/test/rest_util_test.cc +++ b/src/iceberg/test/rest_util_test.cc @@ -23,6 +23,7 @@ #include +#include "iceberg/catalog/rest/auth/auth_properties.h" #include "iceberg/catalog/rest/catalog_properties.h" #include "iceberg/catalog/rest/endpoint.h" #include "iceberg/catalog/rest/resource_paths.h" @@ -232,4 +233,152 @@ TEST(RestCatalogPropertiesTest, SnapshotLoadingModeDefaultIsAll) { EXPECT_EQ(result.value(), SnapshotMode::kAll); } +TEST(RestUtilTest, ParseClientOnlyProperties) { + // Empty string + auto result = ParseClientOnlyProperties(""); + EXPECT_TRUE(result.empty()); + + // Single key + result = ParseClientOnlyProperties("oauth2-server-uri"); + EXPECT_EQ(result.size(), 1); + EXPECT_TRUE(result.contains("oauth2-server-uri")); + + // Multiple keys with spaces + result = ParseClientOnlyProperties(" a , b , c "); + EXPECT_EQ(result.size(), 3); + EXPECT_TRUE(result.contains("a")); + EXPECT_TRUE(result.contains("b")); + EXPECT_TRUE(result.contains("c")); + + // Keys with leading/trailing spaces and empty entries + result = ParseClientOnlyProperties(" a , b ,, a "); + EXPECT_EQ(result.size(), 2); // "a" deduplicated + EXPECT_TRUE(result.contains("a")); + EXPECT_TRUE(result.contains("b")); + + // Only commas + result = ParseClientOnlyProperties(",,,"); + EXPECT_TRUE(result.empty()); + + // Only spaces + result = ParseClientOnlyProperties(" "); + EXPECT_TRUE(result.empty()); +} + +TEST(RestUtilTest, MergeServerConfigWithoutClientOnlyProperties) { + std::unordered_map server_defaults = { + {"default1", "value1"}, + {"oauth2-server-uri", "https://server-default.example/token"}}; + std::unordered_map server_overrides = { + {"override1", "value1"}, + {"oauth2-server-uri", "https://server-override.example/token"}}; + std::unordered_map client_configs = { + {"client1", "value1"}, {"uri", "https://catalog.example"}}; + + auto result = MergeServerConfig(server_defaults, server_overrides, client_configs); + ASSERT_THAT(result, IsOk()); + + auto merged = result.value(); + EXPECT_EQ(merged["default1"], "value1"); + EXPECT_EQ(merged["override1"], "value1"); + EXPECT_EQ(merged["client1"], "value1"); + // Server override wins when rest.client-only-properties is not set + EXPECT_EQ(merged["oauth2-server-uri"], "https://server-override.example/token"); +} + +TEST(RestUtilTest, MergeServerConfigWithClientOnlyPropertiesProtectsOAuth2ServerUri) { + std::unordered_map server_defaults = {{"default1", "value1"}}; + std::unordered_map server_overrides = { + {"oauth2-server-uri", "https://malicious.example/token"}}; + std::unordered_map client_configs = { + {"uri", "https://catalog.example"}, + {"oauth2-server-uri", "https://idp.example/token"}, + {RestCatalogProperties::kClientOnlyProperties.key(), "oauth2-server-uri"}}; + + auto result = MergeServerConfig(server_defaults, server_overrides, client_configs); + ASSERT_THAT(result, IsOk()); + + auto merged = result.value(); + EXPECT_EQ(merged["default1"], "value1"); + // Client value is preserved + EXPECT_EQ(merged["oauth2-server-uri"], "https://idp.example/token"); +} + +TEST(RestUtilTest, MergeServerConfigResolvesRelativeOAuth2ServerUri) { + std::unordered_map server_defaults = {}; + std::unordered_map server_overrides = { + {"uri", "https://collector.example"}, + {"oauth2-server-uri", "https://malicious.example/token"}}; + std::unordered_map client_configs = { + {"uri", "https://catalog.example"}, + // oauth2-server-uri not set, defaults to relative path "v1/oauth/tokens" + {RestCatalogProperties::kClientOnlyProperties.key(), "oauth2-server-uri"}}; + + auto result = MergeServerConfig(server_defaults, server_overrides, client_configs); + ASSERT_THAT(result, IsOk()); + + auto merged = result.value(); + // oauth2-server-uri should be resolved against client uri before merging + EXPECT_EQ(merged["oauth2-server-uri"], "https://catalog.example/v1/oauth/tokens"); + // uri can still be overridden by server (but token endpoint is already fixed) + EXPECT_EQ(merged["uri"], "https://collector.example"); +} + +TEST(RestUtilTest, MergeServerConfigIgnoresServerProvidedClientOnlyProperties) { + std::unordered_map server_defaults = { + {RestCatalogProperties::kClientOnlyProperties.key(), "ignored-key"}}; + std::unordered_map server_overrides = { + {RestCatalogProperties::kClientOnlyProperties.key(), "another-ignored-key"}}; + std::unordered_map client_configs = { + {"uri", "https://catalog.example"}, + {RestCatalogProperties::kClientOnlyProperties.key(), "oauth2-server-uri"}}; + + auto result = MergeServerConfig(server_defaults, server_overrides, client_configs); + ASSERT_THAT(result, IsOk()); + + auto merged = result.value(); + // Client value is preserved, server values ignored + EXPECT_EQ(merged[RestCatalogProperties::kClientOnlyProperties.key()], + "oauth2-server-uri"); +} + +TEST(RestUtilTest, MergeServerConfigWithAbsoluteOAuth2ServerUri) { + std::unordered_map server_defaults = {}; + std::unordered_map server_overrides = { + {"uri", "https://collector.example"}, + {"oauth2-server-uri", "https://malicious.example/token"}}; + std::unordered_map client_configs = { + {"uri", "https://catalog.example"}, + {"oauth2-server-uri", "https://idp.example/token"}, + {RestCatalogProperties::kClientOnlyProperties.key(), "oauth2-server-uri"}}; + + auto result = MergeServerConfig(server_defaults, server_overrides, client_configs); + ASSERT_THAT(result, IsOk()); + + auto merged = result.value(); + // Absolute client endpoint is preserved as-is + EXPECT_EQ(merged["oauth2-server-uri"], "https://idp.example/token"); + // uri can still be overridden + EXPECT_EQ(merged["uri"], "https://collector.example"); +} + +TEST(RestUtilTest, MergeServerConfigWithLeadingSlashOAuth2ServerUri) { + std::unordered_map server_defaults = {}; + std::unordered_map server_overrides = { + {"uri", "https://collector.example"}}; + std::unordered_map client_configs = { + {"uri", "https://catalog.example"}, + {"oauth2-server-uri", "/custom/oauth/tokens"}, + {RestCatalogProperties::kClientOnlyProperties.key(), "oauth2-server-uri"}}; + + auto result = MergeServerConfig(server_defaults, server_overrides, client_configs); + ASSERT_THAT(result, IsOk()); + + auto merged = result.value(); + // Leading-slash path is resolved against client uri + EXPECT_EQ(merged["oauth2-server-uri"], "https://catalog.example/custom/oauth/tokens"); + // uri can still be overridden + EXPECT_EQ(merged["uri"], "https://collector.example"); +} + } // namespace iceberg::rest From 95e9cea4d23b64d731cfb40f0e68047c8e1007d4 Mon Sep 17 00:00:00 2001 From: "shuxu.li" Date: Tue, 20 Oct 2026 16:11:29 +0800 Subject: [PATCH 2/2] fix(rest): fix MSVC build of ParseClientOnlyProperties - Construct string_view from an iterator pair; MSVC string_view iterators are not pointers. - Parenthesize std::min in StringUtils so the Windows min() macro pulled in through cpr does not expand there. --- src/iceberg/catalog/rest/rest_util.cc | 2 +- src/iceberg/util/string_util.h | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/iceberg/catalog/rest/rest_util.cc b/src/iceberg/catalog/rest/rest_util.cc index dd43efdad7..a8f77c1dd1 100644 --- a/src/iceberg/catalog/rest/rest_util.cc +++ b/src/iceberg/catalog/rest/rest_util.cc @@ -144,7 +144,7 @@ std::unordered_set ParseClientOnlyProperties(std::string_view value std::unordered_set result; for (auto part : std::views::split(value, ',')) { - std::string_view key(part.begin(), part.size()); + std::string_view key(part.begin(), part.end()); if (auto first = key.find_first_not_of(kWhitespace); first != std::string_view::npos) { key = key.substr(first, key.find_last_not_of(kWhitespace) - first + 1); diff --git a/src/iceberg/util/string_util.h b/src/iceberg/util/string_util.h index f1431e49d4..b4a0540e56 100644 --- a/src/iceberg/util/string_util.h +++ b/src/iceberg/util/string_util.h @@ -196,7 +196,9 @@ class ICEBERG_EXPORT StringUtils { // knows. static std::optional AsciiEqualsIgnoreCase(std::string_view a, std::string_view b) { - const size_t n = std::min(a.size(), b.size()); + // Parenthesized so a Windows min() macro (e.g. from via cpr) does + // not expand here. + const size_t n = (std::min)(a.size(), b.size()); for (size_t i = 0; i < n; ++i) { if (!IsAsciiByte(a[i]) || !IsAsciiByte(b[i])) { return std::nullopt;