From 9b7f1986f152c59439efca3ea06598dc3f753637 Mon Sep 17 00:00:00 2001 From: "shuxu.li" Date: Mon, 19 Oct 2026 14:43:10 +0800 Subject: [PATCH 1/5] 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/5] 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; From bedeab758ef6ddf70ace78b1221f5ab423153c87 Mon Sep 17 00:00:00 2001 From: "shuxu.li" Date: Fri, 23 Oct 2026 15:08:18 +0800 Subject: [PATCH 3/5] fix(rest): address review comments on client-only properties - Warn about an unset oauth2-server-uri when a token is set as well as a credential, matching Java's OAuth2Manager.warnIfOAuthServerUriNotSet. - Export ResolveOAuth2ServerUri from the public auth_properties.h header. --- src/iceberg/catalog/rest/auth/auth_properties.h | 2 +- src/iceberg/catalog/rest/rest_catalog.cc | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/iceberg/catalog/rest/auth/auth_properties.h b/src/iceberg/catalog/rest/auth/auth_properties.h index 1e0b460a0c..c4327c76c5 100644 --- a/src/iceberg/catalog/rest/auth/auth_properties.h +++ b/src/iceberg/catalog/rest/auth/auth_properties.h @@ -141,7 +141,7 @@ class ICEBERG_REST_EXPORT AuthProperties : public ConfigBase { /// /// \param properties Properties map containing `oauth2-server-uri` and `uri` keys. /// \return Resolved OAuth2 server URI. -Result ResolveOAuth2ServerUri( +ICEBERG_REST_EXPORT Result ResolveOAuth2ServerUri( const std::unordered_map& properties); } // namespace iceberg::rest::auth diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index a21a90a232..0a38a87ba5 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -516,9 +516,13 @@ Result> RestCatalog::Make( 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()) { + // server overrides while a credential or a token is set, matching Java's + // OAuth2Manager.warnIfOAuthServerUriNotSet. + const auto& final_props = final_config.configs(); + auto cred_it = final_props.find(auth::AuthProperties::kCredential.key()); + bool has_credential = cred_it != final_props.end() && !cred_it->second.empty(); + bool has_token = final_props.contains(auth::AuthProperties::kToken.key()); + if (has_credential || has_token) { auto client_uri_it = config.configs().find(auth::AuthProperties::kOAuth2ServerUri.key()); bool endpoint_explicit = From 10a4ce60ecd40d59576c7bc4a392b685ede8f88e Mon Sep 17 00:00:00 2001 From: Gang Wu Date: Wed, 28 Oct 2026 11:45:59 +0800 Subject: [PATCH 4/5] fix(rest): simplify OAuth endpoint protection --- src/iceberg/catalog/rest/auth/auth_manager.cc | 4 + .../catalog/rest/auth/auth_manager_internal.h | 4 + .../catalog/rest/auth/auth_properties.cc | 41 ++-- .../catalog/rest/auth/auth_properties.h | 16 -- src/iceberg/catalog/rest/catalog_properties.h | 10 +- src/iceberg/catalog/rest/rest_catalog.cc | 52 ++--- src/iceberg/catalog/rest/rest_util.cc | 67 ------- src/iceberg/catalog/rest/rest_util.h | 26 --- src/iceberg/test/auth_manager_test.cc | 116 +++++++---- src/iceberg/test/rest_catalog_unit_test.cc | 186 +++++++++--------- src/iceberg/test/rest_util_test.cc | 149 -------------- src/iceberg/util/string_util.h | 4 +- 12 files changed, 226 insertions(+), 449 deletions(-) diff --git a/src/iceberg/catalog/rest/auth/auth_manager.cc b/src/iceberg/catalog/rest/auth/auth_manager.cc index df5d7304a8..e19eaca9d6 100644 --- a/src/iceberg/catalog/rest/auth/auth_manager.cc +++ b/src/iceberg/catalog/rest/auth/auth_manager.cc @@ -150,6 +150,8 @@ class OAuth2Manager : public AuthManager { ICEBERG_PRECHECK(init_client != nullptr, "OAuth2 initialization HTTP client must not be null"); ICEBERG_ASSIGN_OR_RAISE(auto config, AuthProperties::FromProperties(properties)); + ICEBERG_ASSIGN_OR_RAISE(auto endpoint, ResolveOAuth2ServerUri(properties)); + config.Set(AuthProperties::kOAuth2ServerUri, std::move(endpoint)); // No token refresh during init (short-lived session). config.Set(AuthProperties::kKeepRefreshed, false); @@ -177,6 +179,8 @@ class OAuth2Manager : public AuthManager { std::shared_ptr shared_client, const std::unordered_map& properties) override { ICEBERG_ASSIGN_OR_RAISE(auto config, AuthProperties::FromProperties(properties)); + ICEBERG_ASSIGN_OR_RAISE(auto endpoint, ResolveOAuth2ServerUri(properties)); + config.Set(AuthProperties::kOAuth2ServerUri, std::move(endpoint)); ICEBERG_PRECHECK(shared_client != nullptr, "OAuth2 catalog session HTTP client must not be null"); refresh_client_ = std::move(shared_client); diff --git a/src/iceberg/catalog/rest/auth/auth_manager_internal.h b/src/iceberg/catalog/rest/auth/auth_manager_internal.h index 36671a39ff..d62d88925e 100644 --- a/src/iceberg/catalog/rest/auth/auth_manager_internal.h +++ b/src/iceberg/catalog/rest/auth/auth_manager_internal.h @@ -32,6 +32,10 @@ namespace iceberg::rest::auth { +/// \brief Resolve an OAuth2 token endpoint. +Result ResolveOAuth2ServerUri( + const std::unordered_map& properties); + /// \brief Create a no-op authentication manager (no authentication). Result> MakeNoopAuthManager( std::string_view name, diff --git a/src/iceberg/catalog/rest/auth/auth_properties.cc b/src/iceberg/catalog/rest/auth/auth_properties.cc index c183e9b278..5b16cb2e6b 100644 --- a/src/iceberg/catalog/rest/auth/auth_properties.cc +++ b/src/iceberg/catalog/rest/auth/auth_properties.cc @@ -21,6 +21,7 @@ #include +#include "iceberg/catalog/rest/auth/auth_manager_internal.h" #include "iceberg/catalog/rest/catalog_properties.h" #include "iceberg/catalog/rest/rest_util.h" @@ -41,26 +42,25 @@ std::pair ParseCredential(const std::string& credentia Result ResolveOAuth2ServerUri( const std::unordered_map& properties) { auto endpoint_it = properties.find(AuthProperties::kOAuth2ServerUri.key()); - std::string endpoint = endpoint_it == properties.end() - ? AuthProperties::kOAuth2ServerUri.value() - : endpoint_it->second; - - if (endpoint.starts_with("http://") || endpoint.starts_with("https://")) { - return endpoint; - } - if (endpoint.empty()) { - return endpoint; - } - auto uri_it = properties.find(RestCatalogProperties::kUri.key()); - if (uri_it == properties.end() || uri_it->second.empty()) { - return endpoint; + const std::string& endpoint = endpoint_it == properties.end() + ? AuthProperties::kOAuth2ServerUri.value() + : endpoint_it->second; + + // Match Java HTTPRequest's path checks and string joining, without URIBuilder + // validation. Full URL syntax is left to the HTTP transport. + ICEBERG_PRECHECK(!endpoint.starts_with('/'), + "OAuth2 token endpoint paths must not start with '/'"); + std::string full_path; + if (endpoint.starts_with("https://") || endpoint.starts_with("http://")) { + full_path = endpoint; + } else { + auto uri_it = properties.find(RestCatalogProperties::kUri.key()); + ICEBERG_PRECHECK(uri_it != properties.end() && !uri_it->second.empty(), + "Relative OAuth2 token endpoint requires a catalog URI"); + auto base_uri = TrimTrailingSlash(uri_it->second); + full_path = std::string(TrimTrailingSlash(std::string(base_uri) + "/" + endpoint)); } - - auto base_uri = std::string(TrimTrailingSlash(uri_it->second)); - if (endpoint.starts_with('/')) { - return base_uri + endpoint; - } - return base_uri + "/" + endpoint; + return full_path; } std::unordered_map AuthProperties::optional_oauth_params() @@ -87,9 +87,6 @@ Result AuthProperties::FromProperties( config.client_secret_ = std::move(secret); } - ICEBERG_ASSIGN_OR_RAISE(auto oauth2_server_uri, ResolveOAuth2ServerUri(properties)); - config.Set(kOAuth2ServerUri, std::move(oauth2_server_uri)); - // TODO(lishuxu): Parse JWT exp claim from token to set expires_at_millis_. return config; diff --git a/src/iceberg/catalog/rest/auth/auth_properties.h b/src/iceberg/catalog/rest/auth/auth_properties.h index c4327c76c5..8784194cce 100644 --- a/src/iceberg/catalog/rest/auth/auth_properties.h +++ b/src/iceberg/catalog/rest/auth/auth_properties.h @@ -128,20 +128,4 @@ 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. -ICEBERG_REST_EXPORT 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 e9833ddbae..0b24af58ca 100644 --- a/src/iceberg/catalog/rest/catalog_properties.h +++ b/src/iceberg/catalog/rest/catalog_properties.h @@ -77,10 +77,12 @@ 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 Whether server configuration may select the OAuth2 token endpoint. + /// When false, the client must explicitly configure an absolute http:// or https:// + /// oauth2-server-uri. This setting is read from client configuration only. + inline static Entry kTrustServerOAuth2Uri{"rest.auth.oauth2.trust-server-uri", + true}; /// \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 acd734ac11..56f7617a75 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -465,6 +465,18 @@ Result> RestCatalog::Make( } ICEBERG_ASSIGN_OR_RAISE(auto uri, config.Uri()); + const bool trust_server_oauth2_uri = + config.Get(RestCatalogProperties::kTrustServerOAuth2Uri); + const auto endpoint = + config.configs().find(auth::AuthProperties::kOAuth2ServerUri.key()); + if (!trust_server_oauth2_uri) { + ICEBERG_PRECHECK( + endpoint != config.configs().end() && (endpoint->second.starts_with("https://") || + endpoint->second.starts_with("http://")), + "An explicit absolute http:// or https:// oauth2-server-uri is required when " + "rest.auth.oauth2.trust-server-uri is false"); + } + std::string catalog_name = config.Get(RestCatalogProperties::kName); ICEBERG_ASSIGN_OR_RAISE(auto auth_manager, auth::AuthManagers::Load(catalog_name, config.configs())); @@ -483,10 +495,14 @@ Result> RestCatalog::Make( ICEBERG_ASSIGN_OR_RAISE(auto server_config, FetchServerConfig(*init_client, *paths, config, *init_session)); - ICEBERG_ASSIGN_OR_RAISE(auto merged_config, - MergeServerConfig(server_config.defaults, - server_config.overrides, config.configs())); - RestCatalogProperties final_config = RestCatalogProperties::FromMap(merged_config); + RestCatalogProperties final_config = RestCatalogProperties::FromMap( + MergeConfigs(server_config.defaults, config.configs(), server_config.overrides)); + // The server cannot change the client's endpoint trust policy or, when disabled, + // move the OAuth endpoint directly or through an override of the catalog URI. + final_config.Set(RestCatalogProperties::kTrustServerOAuth2Uri, trust_server_oauth2_uri); + if (!trust_server_oauth2_uri) { + final_config.mutable_configs().insert_or_assign(endpoint->first, endpoint->second); + } std::unordered_set endpoints; if (!server_config.endpoints.empty()) { @@ -515,34 +531,6 @@ 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 a credential or a token is set, matching Java's - // OAuth2Manager.warnIfOAuthServerUriNotSet. - const auto& final_props = final_config.configs(); - auto cred_it = final_props.find(auth::AuthProperties::kCredential.key()); - bool has_credential = cred_it != final_props.end() && !cred_it->second.empty(); - bool has_token = final_props.contains(auth::AuthProperties::kToken.key()); - if (has_credential || has_token) { - 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 a8f77c1dd1..cc69abe642 100644 --- a/src/iceberg/catalog/rest/rest_util.cc +++ b/src/iceberg/catalog/rest/rest_util.cc @@ -20,15 +20,11 @@ #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" @@ -139,69 +135,6 @@ 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.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); - 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 fccafc178c..556b2557ad 100644 --- a/src/iceberg/catalog/rest/rest_util.h +++ b/src/iceberg/catalog/rest/rest_util.h @@ -92,32 +92,6 @@ 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/auth_manager_test.cc b/src/iceberg/test/auth_manager_test.cc index f6f1400b2b..ed4a1ebd2f 100644 --- a/src/iceberg/test/auth_manager_test.cc +++ b/src/iceberg/test/auth_manager_test.cc @@ -34,6 +34,7 @@ #include #include +#include "iceberg/catalog/rest/auth/auth_manager_internal.h" #include "iceberg/catalog/rest/auth/auth_managers.h" #include "iceberg/catalog/rest/auth/auth_properties.h" #include "iceberg/catalog/rest/auth/auth_session.h" @@ -75,78 +76,107 @@ class AuthManagerTest : public ::testing::Test { std::shared_ptr client_ = std::make_shared(); }; -TEST(AuthPropertiesTest, ResolvesDefaultOAuth2ServerUri) { +TEST(OAuth2UriTest, ResolvesDefaultOAuth2ServerUri) { ICEBERG_UNWRAP_OR_FAIL( - auto config, - AuthProperties::FromProperties({ + auto resolved, + ResolveOAuth2ServerUri({ {RestCatalogProperties::kUri.key(), "https://catalog.example.com/api/"}, {RestCatalogProperties::kPrefix.key(), "warehouse"}, })); - EXPECT_EQ(config.oauth2_server_uri(), - "https://catalog.example.com/api/v1/oauth/tokens"); + EXPECT_EQ(resolved, "https://catalog.example.com/api/v1/oauth/tokens"); } -TEST(AuthPropertiesTest, PreservesEmptyOAuth2ServerUri) { +TEST(OAuth2UriTest, ResolvesEmptyOAuth2ServerUriToCatalogUri) { ICEBERG_UNWRAP_OR_FAIL( - auto config, AuthProperties::FromProperties({ - {RestCatalogProperties::kUri.key(), "https://catalog.example.com"}, - {AuthProperties::kOAuth2ServerUri.key(), ""}, - })); + auto resolved, + ResolveOAuth2ServerUri({ + {RestCatalogProperties::kUri.key(), "https://catalog.example.com"}, + {AuthProperties::kOAuth2ServerUri.key(), ""}, + })); - EXPECT_TRUE(config.oauth2_server_uri().empty()); + EXPECT_EQ(resolved, "https://catalog.example.com"); } -TEST(AuthPropertiesTest, ResolvesExplicitRelativeOAuth2ServerUri) { +TEST(OAuth2UriTest, ResolvesExplicitRelativeOAuth2ServerUri) { ICEBERG_UNWRAP_OR_FAIL( - auto config, - AuthProperties::FromProperties({ + auto resolved, + ResolveOAuth2ServerUri({ {RestCatalogProperties::kUri.key(), "https://catalog.example.com/api/"}, {AuthProperties::kOAuth2ServerUri.key(), "oauth/token/"}, })); - EXPECT_EQ(config.oauth2_server_uri(), "https://catalog.example.com/api/oauth/token/"); + EXPECT_EQ(resolved, "https://catalog.example.com/api/oauth/token"); } -TEST(AuthPropertiesTest, PreservesExplicitAbsoluteOAuth2ServerUri) { +TEST(OAuth2UriTest, PreservesExplicitAbsoluteOAuth2ServerUri) { ICEBERG_UNWRAP_OR_FAIL( - auto config, - AuthProperties::FromProperties({ + auto resolved, + ResolveOAuth2ServerUri({ {RestCatalogProperties::kUri.key(), "https://catalog.example.com"}, {AuthProperties::kOAuth2ServerUri.key(), "https://auth.example.com/token/"}, })); - EXPECT_EQ(config.oauth2_server_uri(), "https://auth.example.com/token/"); + EXPECT_EQ(resolved, "https://auth.example.com/token/"); } -TEST(AuthPropertiesTest, PreservesRelativeOAuth2ServerUriWithoutCatalogUri) { - ICEBERG_UNWRAP_OR_FAIL(auto config, - AuthProperties::FromProperties({ - {AuthProperties::kOAuth2ServerUri.key(), "oauth/token"}, - })); - - EXPECT_EQ(config.oauth2_server_uri(), "oauth/token"); +TEST(OAuth2UriTest, RejectsRelativeOAuth2ServerUriWithoutCatalogUri) { + for (const auto& properties : + {std::unordered_map{}, + std::unordered_map{ + {"oauth2-server-uri", "oauth/token"}}, + std::unordered_map{ + {"uri", ""}, {"oauth2-server-uri", "oauth/token"}}}) { + EXPECT_THAT(ResolveOAuth2ServerUri(properties), IsError(ErrorKind::kInvalidArgument)); + } } -TEST(AuthPropertiesTest, PreservesAbsolutePathWithoutCatalogUri) { - ICEBERG_UNWRAP_OR_FAIL(auto config, - AuthProperties::FromProperties({ - {AuthProperties::kOAuth2ServerUri.key(), "/oauth/token"}, - })); - - EXPECT_EQ(config.oauth2_server_uri(), "/oauth/token"); +TEST(OAuth2UriTest, RejectsOAuth2ServerUriWithLeadingSlash) { + for (const auto* endpoint : {"/oauth/token", "//auth.example.com/token"}) { + for (const auto* uri : {"", "https://catalog.example.com/api/"}) { + SCOPED_TRACE(std::string(uri) + " | " + endpoint); + EXPECT_THAT(ResolveOAuth2ServerUri({{"uri", uri}, {"oauth2-server-uri", endpoint}}), + IsError(ErrorKind::kInvalidArgument)); + } + } } -TEST(AuthPropertiesTest, ResolvesOAuth2ServerUriWithLeadingSlash) { +TEST(OAuth2UriTest, PreservesAbsoluteOAuth2ServerUriWithoutCatalogUri) { ICEBERG_UNWRAP_OR_FAIL( - auto config, - AuthProperties::FromProperties({ - {RestCatalogProperties::kUri.key(), "https://catalog.example.com/api/"}, - {AuthProperties::kOAuth2ServerUri.key(), "/v1/oauth/tokens"}, - })); + auto resolved, + ResolveOAuth2ServerUri({{"oauth2-server-uri", "https://auth.example.com/token/"}})); + EXPECT_EQ(resolved, "https://auth.example.com/token/"); +} - EXPECT_EQ(config.oauth2_server_uri(), - "https://catalog.example.com/api/v1/oauth/tokens"); +TEST(OAuth2UriTest, ReusesResolvedEndpointInSessions) { + struct TestCase { + const char* uri; + const char* endpoint; + const char* expected; + }; + // These resolved URLs must not be treated as relative paths a second time. + for (const auto& [uri, endpoint, expected] : { + TestCase{"HTTPS://catalog.example/api", "v1/oauth/tokens", + "HTTPS://catalog.example/api/v1/oauth/tokens"}, + TestCase{"https://catalog.example/api", "HTTPS://auth.example/token", + "https://catalog.example/api/HTTPS://auth.example/token"}, + }) { + SCOPED_TRACE(std::string(uri) + " | " + endpoint); + const std::unordered_map properties = { + {"uri", uri}, + {"oauth2-server-uri", endpoint}, + {"token", "parent"}, + {"token-refresh-enabled", "false"}}; + auto client = std::make_shared(); + ICEBERG_UNWRAP_OR_FAIL(auto manager, AuthManagers::Load("test", properties)); + ICEBERG_UNWRAP_OR_FAIL(auto parent, manager->CatalogSession(client, properties)); + EXPECT_EQ(parent->OAuth2Info()->oauth2_server_uri, expected); + ICEBERG_UNWRAP_OR_FAIL( + auto child, + manager->ContextualSession( + {.session_id = "child", .credentials = {{"token", "child"}}}, parent)); + EXPECT_EQ(child->OAuth2Info()->oauth2_server_uri, expected); + } } // Verifies loading NoopAuthManager with explicit "none" auth type @@ -437,6 +467,7 @@ TEST_F(AuthManagerTest, OAuth2StaticToken) { TEST_F(AuthManagerTest, OAuth2InferredFromToken) { std::unordered_map properties = { {AuthProperties::kToken.key(), "inferred-token"}, + {RestCatalogProperties::kUri.key(), "https://catalog.example.com"}, }; auto manager_result = AuthManagers::Load("test-catalog", properties); @@ -455,6 +486,7 @@ TEST_F(AuthManagerTest, OAuth2InferredFromToken) { TEST_F(AuthManagerTest, OAuth2MissingCredentials) { std::unordered_map properties = { {AuthProperties::kAuthType, "oauth2"}, + {RestCatalogProperties::kUri.key(), "https://catalog.example.com"}, }; auto manager_result = AuthManagers::Load("test-catalog", properties); @@ -514,6 +546,7 @@ TEST_F(AuthManagerTest, OAuth2ContextTokenCreatesChildAndHasPriority) { TEST_F(AuthManagerTest, OAuth2ContextTypedTokenOnlyUsesCredentials) { std::unordered_map properties = { {AuthProperties::kAuthType, "oauth2"}, + {RestCatalogProperties::kUri.key(), "https://catalog.example.com"}, }; ICEBERG_UNWRAP_OR_FAIL(auto manager, AuthManagers::Load("test-catalog", properties)); ICEBERG_UNWRAP_OR_FAIL(auto parent, manager->CatalogSession(client_, properties)); @@ -557,6 +590,7 @@ TEST_F(AuthManagerTest, OAuth2TableTokenCreatesChild) { TEST_F(AuthManagerTest, OAuth2TableIgnoresCredential) { std::unordered_map properties = { {AuthProperties::kAuthType, "oauth2"}, + {RestCatalogProperties::kUri.key(), "https://catalog.example.com"}, }; ICEBERG_UNWRAP_OR_FAIL(auto manager, AuthManagers::Load("test-catalog", properties)); ICEBERG_UNWRAP_OR_FAIL(auto parent, manager->CatalogSession(client_, properties)); diff --git a/src/iceberg/test/rest_catalog_unit_test.cc b/src/iceberg/test/rest_catalog_unit_test.cc index ad3b8985b4..9a84ce8667 100644 --- a/src/iceberg/test/rest_catalog_unit_test.cc +++ b/src/iceberg/test/rest_catalog_unit_test.cc @@ -17,6 +17,8 @@ * under the License. */ +#include +#include #include #include #include @@ -29,7 +31,6 @@ #include #include "iceberg/catalog.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/endpoint.h" @@ -40,6 +41,7 @@ #include "iceberg/catalog/rest/rest_table.h" #include "iceberg/catalog/rest/rest_table_scan.h" #include "iceberg/catalog/rest/types.h" +#include "iceberg/catalog/session_context.h" #include "iceberg/file_io.h" #include "iceberg/file_io_registry.h" #include "iceberg/labels.h" @@ -615,107 +617,113 @@ TEST_P(RestCatalogScanLifetimeTest, ScanKeepsCredentialRefreshAlive) { INSTANTIATE_TEST_SUITE_P(ScanModes, RestCatalogScanLifetimeTest, ::testing::Bool()); -// -------------------------------------------------------------------------- -// Client-only properties tests -// -------------------------------------------------------------------------- - -class RestCatalogClientOnlyPropertiesTest : public ::testing::Test { +class RestCatalogOAuth2UriTest : public RestCatalogLoadTableTest { 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(); - }}); + RestCatalogLoadTableTest::SetUp(); + init_client_ = std::make_shared<::testing::StrictMock>(); + mock_client_ = std::make_shared<::testing::StrictMock>(); + properties_ = {{"uri", "https://catalog.example/api"}, + {"credential", "client:secret"}, + {"oauth2-server-uri", "https://auth.example/token/"}, + {"token-refresh-enabled", "false"}, + {"io-impl", "rest-catalog-unit-test-noop"}}; } - // 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"; - // These tests inspect the merged configuration only; disable refresh so no auth - // executor is required. - client_properties["token-refresh-enabled"] = "false"; - EXPECT_CALL(*mock_client_, Get(::testing::EndsWith("/v1/config"), _, _, _, _)) + void ExpectInit(const CatalogConfig& server_config, + const std::string& token_response = + R"({"access_token":"init","token_type":"bearer"})") { + EXPECT_CALL(*init_client_, PostForm("https://auth.example/token/", _, _, _, _)) + .WillOnce(Return(HttpResponse(200, token_response))); + EXPECT_CALL(*init_client_, Get("https://catalog.example/api/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, [&captured, this](const RestCatalogProperties& properties) { - captured.push_back(properties); - return mock_client_; - }); - EXPECT_THAT(result, IsOk()); - return captured; } - std::shared_ptr mock_client_; + Result> MakeOAuth2Catalog(Executor* executor = nullptr) { + return RestCatalog::Make( + RestCatalogProperties::FromMap(properties_), + [this](const RestCatalogProperties&) -> Result> { + return ++factory_calls_ == 1 ? init_client_ : mock_client_; + }, + executor); + } + + void CheckContextualRequest(const std::shared_ptr& root, + const std::string& endpoint, + const std::string& catalog_uri) { + EXPECT_CALL(*mock_client_, PostForm(endpoint, _, _, _, _)) + .WillOnce(Return( + HttpResponse(200, R"({"access_token":"context","token_type":"bearer"})"))); + EXPECT_CALL(*mock_client_, Get(catalog_uri + "/v1/namespaces", _, _, _, _)) + .WillOnce(Return(HttpResponse(200, R"({"namespaces":[]})"))); + ICEBERG_UNWRAP_OR_FAIL( + auto scoped, + root->WithContext({.session_id = "context", + .credentials = {{"credential", "user:context-secret"}}})); + EXPECT_THAT(scoped->ListNamespaces({}), IsOk()); + } + + std::shared_ptr init_client_; + std::unordered_map properties_; + int factory_calls_ = 0; }; -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(RestCatalogOAuth2UriTest, TrustsServerOverridesByDefault) { + ExpectInit({.overrides = {{"uri", "https://other-catalog.example/base/"}, + {"oauth2-server-uri", "oauth/token///"}, + {"rest.auth.oauth2.trust-server-uri", "false"}}}); + ICEBERG_UNWRAP_OR_FAIL(auto root, MakeOAuth2Catalog()); + CheckContextualRequest(root, "https://other-catalog.example/base/oauth/token", + "https://other-catalog.example/base"); } -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(RestCatalogOAuth2UriTest, RequiresAbsoluteClientEndpointBeforeAnyIoWhenUntrusted) { + properties_["rest.auth.oauth2.trust-server-uri"] = "false"; + properties_.erase("oauth2-server-uri"); + EXPECT_THAT(MakeOAuth2Catalog(), IsError(ErrorKind::kInvalidArgument)); + for (const auto* endpoint : + {"", "token", "/token", "//auth.example/token", "ftp://auth.example/token", + "HTTPS://auth.example/token"}) { + SCOPED_TRACE(endpoint); + properties_["oauth2-server-uri"] = endpoint; + EXPECT_THAT(MakeOAuth2Catalog(), IsError(ErrorKind::kInvalidArgument)); + } + EXPECT_EQ(factory_calls_, 0); } -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"); +TEST_F(RestCatalogOAuth2UriTest, RefreshUsesTheClientEndpointWhenUntrusted) { + test::MockExecutor executor; + std::promise queued; + auto ready = queued.get_future(); + EXPECT_CALL(executor, Submit).WillOnce([&](ExecutorTask task) -> Status { + queued.set_value(std::move(task)); + return {}; + }); + properties_["rest.auth.oauth2.trust-server-uri"] = "false"; + properties_["token-refresh-enabled"] = "true"; + ExpectInit({.overrides = {{"uri", "https://other-catalog.example"}, + {"oauth2-server-uri", "https://server-auth.example/token"}, + {"rest.auth.oauth2.trust-server-uri", "true"}}}, + R"({"access_token":"expired","token_type":"bearer","expires_in":0})"); + EXPECT_CALL(*mock_client_, PostForm("https://auth.example/token/", _, _, _, _)) + .WillOnce(Return( + HttpResponse(200, R"({"access_token":"refreshed","token_type":"bearer"})"))); + ICEBERG_UNWRAP_OR_FAIL(auto root, MakeOAuth2Catalog(&executor)); + ASSERT_EQ(ready.wait_for(std::chrono::seconds(5)), std::future_status::ready); + std::move(ready.get())(); + EXPECT_CALL(*mock_client_, + Get("https://other-catalog.example/v1/namespaces", _, _, _, _)) + .WillOnce( + [](const std::string&, const std::unordered_map&, + const std::unordered_map&, const ErrorHandler&, + auth::AuthSession& session) -> Result { + EXPECT_EQ(session.OAuth2Info()->token, "refreshed"); + return HttpResponse(200, R"({"namespaces":[]})"); + }); + ICEBERG_UNWRAP_OR_FAIL(auto catalog, root->AsCatalog()); + EXPECT_THAT(catalog->ListNamespaces({}), IsOk()); + CheckContextualRequest(root, "https://auth.example/token/", + "https://other-catalog.example"); } } // namespace diff --git a/src/iceberg/test/rest_util_test.cc b/src/iceberg/test/rest_util_test.cc index ce7443994f..0035afca08 100644 --- a/src/iceberg/test/rest_util_test.cc +++ b/src/iceberg/test/rest_util_test.cc @@ -23,7 +23,6 @@ #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" @@ -233,152 +232,4 @@ 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 diff --git a/src/iceberg/util/string_util.h b/src/iceberg/util/string_util.h index b4a0540e56..f1431e49d4 100644 --- a/src/iceberg/util/string_util.h +++ b/src/iceberg/util/string_util.h @@ -196,9 +196,7 @@ class ICEBERG_EXPORT StringUtils { // knows. static std::optional AsciiEqualsIgnoreCase(std::string_view a, std::string_view b) { - // 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()); + 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; From ae0be00844eb5f27452211958268c6e268bf872f Mon Sep 17 00:00:00 2001 From: Gang Wu Date: Wed, 28 Oct 2026 14:47:39 +0800 Subject: [PATCH 5/5] fix(rest): resolve request URIs in HTTP client --- src/iceberg/catalog/rest/auth/auth_manager.cc | 4 - .../catalog/rest/auth/auth_manager_internal.h | 4 - .../catalog/rest/auth/auth_properties.cc | 28 ----- src/iceberg/catalog/rest/http_client.cc | 47 ++++++-- src/iceberg/catalog/rest/http_client.h | 7 +- src/iceberg/catalog/rest/rest_catalog.cc | 3 +- src/iceberg/catalog/rest/rest_catalog.h | 3 +- src/iceberg/test/auth_manager_test.cc | 104 ------------------ src/iceberg/test/http_client_test.cc | 28 +++++ src/iceberg/test/rest_catalog_unit_test.cc | 13 ++- 10 files changed, 81 insertions(+), 160 deletions(-) diff --git a/src/iceberg/catalog/rest/auth/auth_manager.cc b/src/iceberg/catalog/rest/auth/auth_manager.cc index e19eaca9d6..df5d7304a8 100644 --- a/src/iceberg/catalog/rest/auth/auth_manager.cc +++ b/src/iceberg/catalog/rest/auth/auth_manager.cc @@ -150,8 +150,6 @@ class OAuth2Manager : public AuthManager { ICEBERG_PRECHECK(init_client != nullptr, "OAuth2 initialization HTTP client must not be null"); ICEBERG_ASSIGN_OR_RAISE(auto config, AuthProperties::FromProperties(properties)); - ICEBERG_ASSIGN_OR_RAISE(auto endpoint, ResolveOAuth2ServerUri(properties)); - config.Set(AuthProperties::kOAuth2ServerUri, std::move(endpoint)); // No token refresh during init (short-lived session). config.Set(AuthProperties::kKeepRefreshed, false); @@ -179,8 +177,6 @@ class OAuth2Manager : public AuthManager { std::shared_ptr shared_client, const std::unordered_map& properties) override { ICEBERG_ASSIGN_OR_RAISE(auto config, AuthProperties::FromProperties(properties)); - ICEBERG_ASSIGN_OR_RAISE(auto endpoint, ResolveOAuth2ServerUri(properties)); - config.Set(AuthProperties::kOAuth2ServerUri, std::move(endpoint)); ICEBERG_PRECHECK(shared_client != nullptr, "OAuth2 catalog session HTTP client must not be null"); refresh_client_ = std::move(shared_client); diff --git a/src/iceberg/catalog/rest/auth/auth_manager_internal.h b/src/iceberg/catalog/rest/auth/auth_manager_internal.h index d62d88925e..36671a39ff 100644 --- a/src/iceberg/catalog/rest/auth/auth_manager_internal.h +++ b/src/iceberg/catalog/rest/auth/auth_manager_internal.h @@ -32,10 +32,6 @@ namespace iceberg::rest::auth { -/// \brief Resolve an OAuth2 token endpoint. -Result ResolveOAuth2ServerUri( - const std::unordered_map& properties); - /// \brief Create a no-op authentication manager (no authentication). Result> MakeNoopAuthManager( std::string_view name, diff --git a/src/iceberg/catalog/rest/auth/auth_properties.cc b/src/iceberg/catalog/rest/auth/auth_properties.cc index 5b16cb2e6b..2f6d1d9c3d 100644 --- a/src/iceberg/catalog/rest/auth/auth_properties.cc +++ b/src/iceberg/catalog/rest/auth/auth_properties.cc @@ -21,10 +21,6 @@ #include -#include "iceberg/catalog/rest/auth/auth_manager_internal.h" -#include "iceberg/catalog/rest/catalog_properties.h" -#include "iceberg/catalog/rest/rest_util.h" - namespace iceberg::rest::auth { namespace { @@ -39,30 +35,6 @@ std::pair ParseCredential(const std::string& credentia } // namespace -Result ResolveOAuth2ServerUri( - const std::unordered_map& properties) { - auto endpoint_it = properties.find(AuthProperties::kOAuth2ServerUri.key()); - const std::string& endpoint = endpoint_it == properties.end() - ? AuthProperties::kOAuth2ServerUri.value() - : endpoint_it->second; - - // Match Java HTTPRequest's path checks and string joining, without URIBuilder - // validation. Full URL syntax is left to the HTTP transport. - ICEBERG_PRECHECK(!endpoint.starts_with('/'), - "OAuth2 token endpoint paths must not start with '/'"); - std::string full_path; - if (endpoint.starts_with("https://") || endpoint.starts_with("http://")) { - full_path = endpoint; - } else { - auto uri_it = properties.find(RestCatalogProperties::kUri.key()); - ICEBERG_PRECHECK(uri_it != properties.end() && !uri_it->second.empty(), - "Relative OAuth2 token endpoint requires a catalog URI"); - auto base_uri = TrimTrailingSlash(uri_it->second); - full_path = std::string(TrimTrailingSlash(std::string(base_uri) + "/" + endpoint)); - } - return full_path; -} - std::unordered_map AuthProperties::optional_oauth_params() const { std::unordered_map params; diff --git a/src/iceberg/catalog/rest/http_client.cc b/src/iceberg/catalog/rest/http_client.cc index 718a5b7371..019f78209d 100644 --- a/src/iceberg/catalog/rest/http_client.cc +++ b/src/iceberg/catalog/rest/http_client.cc @@ -192,25 +192,45 @@ Status HandleFailureResponse(const cpr::Response& response, HttpClient::HttpClient(std::unordered_map default_headers, std::chrono::milliseconds connection_timeout, - std::chrono::milliseconds socket_timeout) - : default_headers_{std::move(default_headers)}, + std::chrono::milliseconds socket_timeout, + std::optional base_uri) + : base_uri_{std::move(base_uri)}, + default_headers_{std::move(default_headers)}, connection_timeout_{connection_timeout}, socket_timeout_{socket_timeout}, connection_pool_{std::make_unique()} { // Set default Content-Type for all requests (including GET/HEAD/DELETE). - // Many systems require that content type is set regardless and will fail, - // even on an empty bodied request. + // Many systems require that content type is set regardless, even on an empty body. default_headers_[kHeaderContentType] = kMimeTypeApplicationJson; default_headers_[kHeaderUserAgent] = kUserAgent; } HttpClient::~HttpClient() = default; +Result HttpClient::RequestUri(const std::string& path) const { + // Match Java HTTPRequest.check, then requestUri. URI syntax validation remains + // the transport's responsibility rather than emulating java.net.URI. + ICEBERG_PRECHECK(!path.starts_with('/'), "Request paths must not start with '/'"); + const bool has_absolute_path = + path.starts_with("https://") || path.starts_with("http://"); + ICEBERG_PRECHECK(base_uri_.has_value() || has_absolute_path, + "Relative request path requires a base URI"); + std::string full_path; + if (has_absolute_path) { + full_path = path; + } else { + auto base_uri = TrimTrailingSlash(*base_uri_); + full_path = std::string(TrimTrailingSlash(std::string(base_uri) + "/" + path)); + } + return full_path; +} + Result HttpClient::Get( const std::string& path, const std::unordered_map& params, const std::unordered_map& headers, const ErrorHandler& error_handler, auth::AuthSession& session) { - ICEBERG_ASSIGN_OR_RAISE(auto url, AppendQueryString(path, params)); + ICEBERG_ASSIGN_OR_RAISE(auto request_uri, RequestUri(path)); + ICEBERG_ASSIGN_OR_RAISE(auto url, AppendQueryString(request_uri, params)); ICEBERG_ASSIGN_OR_RAISE(auto authenticated, AuthenticateRequest(session, HttpMethod::kGet, std::move(url), MergeHeaders(default_headers_, headers))); @@ -229,9 +249,10 @@ Result HttpClient::Post( const std::string& path, const std::string& body, const std::unordered_map& headers, const ErrorHandler& error_handler, auth::AuthSession& session) { + ICEBERG_ASSIGN_OR_RAISE(auto request_uri, RequestUri(path)); ICEBERG_ASSIGN_OR_RAISE( auto authenticated, - AuthenticateRequest(session, HttpMethod::kPost, path, + AuthenticateRequest(session, HttpMethod::kPost, std::move(request_uri), MergeHeaders(default_headers_, headers), body)); cpr::Response response = cpr::Post(cpr::Url{authenticated.url}, cpr::Body{authenticated.body}, @@ -249,6 +270,7 @@ Result HttpClient::PostForm( const std::unordered_map& form_data, const std::unordered_map& headers, const ErrorHandler& error_handler, auth::AuthSession& session) { + ICEBERG_ASSIGN_OR_RAISE(auto request_uri, RequestUri(path)); std::unordered_map form_headers(headers); form_headers.insert_or_assign(kHeaderContentType, kMimeTypeFormUrlEncoded); std::vector pair_list; @@ -261,7 +283,7 @@ Result HttpClient::PostForm( cpr::Payload(pair_list.begin(), pair_list.end()).GetContent(); ICEBERG_ASSIGN_OR_RAISE( auto authenticated, - AuthenticateRequest(session, HttpMethod::kPost, path, + AuthenticateRequest(session, HttpMethod::kPost, std::move(request_uri), MergeHeaders(default_headers_, form_headers), std::move(encoded_body))); cpr::Response response = @@ -278,9 +300,11 @@ Result HttpClient::PostForm( Result HttpClient::Head( const std::string& path, const std::unordered_map& headers, const ErrorHandler& error_handler, auth::AuthSession& session) { - ICEBERG_ASSIGN_OR_RAISE(auto authenticated, - AuthenticateRequest(session, HttpMethod::kHead, path, - MergeHeaders(default_headers_, headers))); + ICEBERG_ASSIGN_OR_RAISE(auto request_uri, RequestUri(path)); + ICEBERG_ASSIGN_OR_RAISE( + auto authenticated, + AuthenticateRequest(session, HttpMethod::kHead, std::move(request_uri), + MergeHeaders(default_headers_, headers))); cpr::Response response = cpr::Head(cpr::Url{authenticated.url}, ToCprHeader(authenticated), kNoFollowRedirects, *connection_pool_, @@ -296,7 +320,8 @@ Result HttpClient::Delete( const std::string& path, const std::unordered_map& params, const std::unordered_map& headers, const ErrorHandler& error_handler, auth::AuthSession& session) { - ICEBERG_ASSIGN_OR_RAISE(auto url, AppendQueryString(path, params)); + ICEBERG_ASSIGN_OR_RAISE(auto request_uri, RequestUri(path)); + ICEBERG_ASSIGN_OR_RAISE(auto url, AppendQueryString(request_uri, params)); ICEBERG_ASSIGN_OR_RAISE( auto authenticated, AuthenticateRequest(session, HttpMethod::kDelete, std::move(url), diff --git a/src/iceberg/catalog/rest/http_client.h b/src/iceberg/catalog/rest/http_client.h index a185064041..9a24593878 100644 --- a/src/iceberg/catalog/rest/http_client.h +++ b/src/iceberg/catalog/rest/http_client.h @@ -75,7 +75,8 @@ class ICEBERG_REST_EXPORT HttpClient { /// \param connection_timeout,socket_timeout Zero leaves requests unbounded. explicit HttpClient(std::unordered_map default_headers = {}, std::chrono::milliseconds connection_timeout = {}, - std::chrono::milliseconds socket_timeout = {}); + std::chrono::milliseconds socket_timeout = {}, + std::optional base_uri = std::nullopt); virtual ~HttpClient(); HttpClient(const HttpClient&) = delete; @@ -115,6 +116,10 @@ class ICEBERG_REST_EXPORT HttpClient { const ErrorHandler& error_handler, auth::AuthSession& session); private: + /// \brief Build a request URI from this client's base URI and a path. + Result RequestUri(const std::string& path) const; + + std::optional base_uri_; std::unordered_map default_headers_; std::chrono::milliseconds connection_timeout_; std::chrono::milliseconds socket_timeout_; diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index 56f7617a75..a25d28eb47 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -457,10 +457,11 @@ Result> RestCatalog::Make( const auto timeout_of = [&properties](const auto& entry) { return std::chrono::milliseconds(std::max(0, properties.Get(entry))); }; + ICEBERG_ASSIGN_OR_RAISE(auto uri, properties.Uri()); return std::make_shared( properties.ExtractHeaders(), timeout_of(RestCatalogProperties::kClientConnectionTimeoutMs), - timeout_of(RestCatalogProperties::kClientSocketTimeoutMs)); + timeout_of(RestCatalogProperties::kClientSocketTimeoutMs), std::string(uri)); }; } ICEBERG_ASSIGN_OR_RAISE(auto uri, config.Uri()); diff --git a/src/iceberg/catalog/rest/rest_catalog.h b/src/iceberg/catalog/rest/rest_catalog.h index d648fa7628..561ff34716 100644 --- a/src/iceberg/catalog/rest/rest_catalog.h +++ b/src/iceberg/catalog/rest/rest_catalog.h @@ -59,7 +59,8 @@ class ICEBERG_REST_EXPORT RestCatalog final /// \brief Create a RestCatalog instance. /// \param client_factory Optional HTTP client factory, called with the initial config - /// and then the merged server config. When empty, uses the default HTTP client. + /// and then the merged server config. It should configure the HTTP client's base URI + /// from the supplied properties. When empty, uses the default HTTP client. /// \param auth_executor Borrowed OAuth2 refresh executor. With OAuth2 credentials, /// token-refresh-enabled=true (default) requires it; null returns InvalidArgument. /// Must enqueue asynchronously and outlive the catalog, its auth sessions and their diff --git a/src/iceberg/test/auth_manager_test.cc b/src/iceberg/test/auth_manager_test.cc index ed4a1ebd2f..0bae430613 100644 --- a/src/iceberg/test/auth_manager_test.cc +++ b/src/iceberg/test/auth_manager_test.cc @@ -34,7 +34,6 @@ #include #include -#include "iceberg/catalog/rest/auth/auth_manager_internal.h" #include "iceberg/catalog/rest/auth/auth_managers.h" #include "iceberg/catalog/rest/auth/auth_properties.h" #include "iceberg/catalog/rest/auth/auth_session.h" @@ -76,109 +75,6 @@ class AuthManagerTest : public ::testing::Test { std::shared_ptr client_ = std::make_shared(); }; -TEST(OAuth2UriTest, ResolvesDefaultOAuth2ServerUri) { - ICEBERG_UNWRAP_OR_FAIL( - auto resolved, - ResolveOAuth2ServerUri({ - {RestCatalogProperties::kUri.key(), "https://catalog.example.com/api/"}, - {RestCatalogProperties::kPrefix.key(), "warehouse"}, - })); - - EXPECT_EQ(resolved, "https://catalog.example.com/api/v1/oauth/tokens"); -} - -TEST(OAuth2UriTest, ResolvesEmptyOAuth2ServerUriToCatalogUri) { - ICEBERG_UNWRAP_OR_FAIL( - auto resolved, - ResolveOAuth2ServerUri({ - {RestCatalogProperties::kUri.key(), "https://catalog.example.com"}, - {AuthProperties::kOAuth2ServerUri.key(), ""}, - })); - - EXPECT_EQ(resolved, "https://catalog.example.com"); -} - -TEST(OAuth2UriTest, ResolvesExplicitRelativeOAuth2ServerUri) { - ICEBERG_UNWRAP_OR_FAIL( - auto resolved, - ResolveOAuth2ServerUri({ - {RestCatalogProperties::kUri.key(), "https://catalog.example.com/api/"}, - {AuthProperties::kOAuth2ServerUri.key(), "oauth/token/"}, - })); - - EXPECT_EQ(resolved, "https://catalog.example.com/api/oauth/token"); -} - -TEST(OAuth2UriTest, PreservesExplicitAbsoluteOAuth2ServerUri) { - ICEBERG_UNWRAP_OR_FAIL( - auto resolved, - ResolveOAuth2ServerUri({ - {RestCatalogProperties::kUri.key(), "https://catalog.example.com"}, - {AuthProperties::kOAuth2ServerUri.key(), "https://auth.example.com/token/"}, - })); - - EXPECT_EQ(resolved, "https://auth.example.com/token/"); -} - -TEST(OAuth2UriTest, RejectsRelativeOAuth2ServerUriWithoutCatalogUri) { - for (const auto& properties : - {std::unordered_map{}, - std::unordered_map{ - {"oauth2-server-uri", "oauth/token"}}, - std::unordered_map{ - {"uri", ""}, {"oauth2-server-uri", "oauth/token"}}}) { - EXPECT_THAT(ResolveOAuth2ServerUri(properties), IsError(ErrorKind::kInvalidArgument)); - } -} - -TEST(OAuth2UriTest, RejectsOAuth2ServerUriWithLeadingSlash) { - for (const auto* endpoint : {"/oauth/token", "//auth.example.com/token"}) { - for (const auto* uri : {"", "https://catalog.example.com/api/"}) { - SCOPED_TRACE(std::string(uri) + " | " + endpoint); - EXPECT_THAT(ResolveOAuth2ServerUri({{"uri", uri}, {"oauth2-server-uri", endpoint}}), - IsError(ErrorKind::kInvalidArgument)); - } - } -} - -TEST(OAuth2UriTest, PreservesAbsoluteOAuth2ServerUriWithoutCatalogUri) { - ICEBERG_UNWRAP_OR_FAIL( - auto resolved, - ResolveOAuth2ServerUri({{"oauth2-server-uri", "https://auth.example.com/token/"}})); - EXPECT_EQ(resolved, "https://auth.example.com/token/"); -} - -TEST(OAuth2UriTest, ReusesResolvedEndpointInSessions) { - struct TestCase { - const char* uri; - const char* endpoint; - const char* expected; - }; - // These resolved URLs must not be treated as relative paths a second time. - for (const auto& [uri, endpoint, expected] : { - TestCase{"HTTPS://catalog.example/api", "v1/oauth/tokens", - "HTTPS://catalog.example/api/v1/oauth/tokens"}, - TestCase{"https://catalog.example/api", "HTTPS://auth.example/token", - "https://catalog.example/api/HTTPS://auth.example/token"}, - }) { - SCOPED_TRACE(std::string(uri) + " | " + endpoint); - const std::unordered_map properties = { - {"uri", uri}, - {"oauth2-server-uri", endpoint}, - {"token", "parent"}, - {"token-refresh-enabled", "false"}}; - auto client = std::make_shared(); - ICEBERG_UNWRAP_OR_FAIL(auto manager, AuthManagers::Load("test", properties)); - ICEBERG_UNWRAP_OR_FAIL(auto parent, manager->CatalogSession(client, properties)); - EXPECT_EQ(parent->OAuth2Info()->oauth2_server_uri, expected); - ICEBERG_UNWRAP_OR_FAIL( - auto child, - manager->ContextualSession( - {.session_id = "child", .credentials = {{"token", "child"}}}, parent)); - EXPECT_EQ(child->OAuth2Info()->oauth2_server_uri, expected); - } -} - // Verifies loading NoopAuthManager with explicit "none" auth type TEST_F(AuthManagerTest, LoadNoopAuthManagerExplicit) { std::unordered_map properties = { diff --git a/src/iceberg/test/http_client_test.cc b/src/iceberg/test/http_client_test.cc index 9e5037299b..6cc653131e 100644 --- a/src/iceberg/test/http_client_test.cc +++ b/src/iceberg/test/http_client_test.cc @@ -314,6 +314,34 @@ TEST(HttpClientTest, PostReportsTransferFailureAfterSuccessfulStatus) { #endif +class CapturingAuthSession final : public auth::AuthSession { + public: + Result Authenticate(HttpRequest request) override { + request_ = std::move(request); + return InvalidArgument("captured"); + } + + const HttpRequest& request() const { return request_; } + + private: + HttpRequest request_; +}; + +TEST(HttpClientTest, BuildsRequestUriLikeJavaHttpRequest) { + HttpClient client({}, {}, {}, std::string("https://catalog.example/api/")); + CapturingAuthSession session; + + auto relative = client.PostForm("v1/oauth/tokens///", {}, {}, + *DefaultErrorHandler::Instance(), session); + EXPECT_THAT(relative, IsError(ErrorKind::kInvalidArgument)); + EXPECT_EQ(session.request().url, "https://catalog.example/api/v1/oauth/tokens"); + + auto absolute = client.PostForm("https://auth.example/token/", {}, {}, + *DefaultErrorHandler::Instance(), session); + EXPECT_THAT(absolute, IsError(ErrorKind::kInvalidArgument)); + EXPECT_EQ(session.request().url, "https://auth.example/token/"); +} + // Redirect responses must be reported without replaying the request at the target. TEST(HttpClientTest, DoesNotFollowRedirects) { #ifdef _WIN32 diff --git a/src/iceberg/test/rest_catalog_unit_test.cc b/src/iceberg/test/rest_catalog_unit_test.cc index 9a84ce8667..1b162763b4 100644 --- a/src/iceberg/test/rest_catalog_unit_test.cc +++ b/src/iceberg/test/rest_catalog_unit_test.cc @@ -31,6 +31,7 @@ #include #include "iceberg/catalog.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/endpoint.h" @@ -649,8 +650,8 @@ class RestCatalogOAuth2UriTest : public RestCatalogLoadTableTest { } void CheckContextualRequest(const std::shared_ptr& root, - const std::string& endpoint, - const std::string& catalog_uri) { + const std::string& endpoint, const std::string& catalog_uri, + const std::string& credential_type = "credential") { EXPECT_CALL(*mock_client_, PostForm(endpoint, _, _, _, _)) .WillOnce(Return( HttpResponse(200, R"({"access_token":"context","token_type":"bearer"})"))); @@ -659,7 +660,7 @@ class RestCatalogOAuth2UriTest : public RestCatalogLoadTableTest { ICEBERG_UNWRAP_OR_FAIL( auto scoped, root->WithContext({.session_id = "context", - .credentials = {{"credential", "user:context-secret"}}})); + .credentials = {{credential_type, "user:context-secret"}}})); EXPECT_THAT(scoped->ListNamespaces({}), IsOk()); } @@ -669,12 +670,12 @@ class RestCatalogOAuth2UriTest : public RestCatalogLoadTableTest { }; TEST_F(RestCatalogOAuth2UriTest, TrustsServerOverridesByDefault) { - ExpectInit({.overrides = {{"uri", "https://other-catalog.example/base/"}, + ExpectInit({.overrides = {{"uri", "HTTPS://other-catalog.example/base/"}, {"oauth2-server-uri", "oauth/token///"}, {"rest.auth.oauth2.trust-server-uri", "false"}}}); ICEBERG_UNWRAP_OR_FAIL(auto root, MakeOAuth2Catalog()); - CheckContextualRequest(root, "https://other-catalog.example/base/oauth/token", - "https://other-catalog.example/base"); + CheckContextualRequest(root, "oauth/token///", "HTTPS://other-catalog.example/base", + auth::AuthProperties::kIdTokenType); } TEST_F(RestCatalogOAuth2UriTest, RequiresAbsoluteClientEndpointBeforeAnyIoWhenUntrusted) {