Repository navigation
feat(rest): add an option to ignore server OAuth2 endpoints - #1041
Merged
Merged
Conversation
…r-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 <noreply@anthropic.com>
- 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.
wgtmac
changed the title
shangxinli
reviewed
Oct 22, 2026
shangxinli
reviewed
Oct 22, 2026
- 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.
…-properties # Conflicts: # src/iceberg/test/rest_catalog_unit_test.cc
wgtmac
changed the title
wgtmac
approved these changes
Oct 28, 2026
wgtmac
left a comment
wgtmac
left a comment
Member
There was a problem hiding this comment.
As we discussed offline, my earlier guidance made this PR more complicated than necessary. I've pushed a simpler version that replaces the generic client-only property list with a dedicated option for the OAuth2 endpoint. Sorry for the confusion, and thanks for your patience! @lishuxu
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduce
rest.auth.oauth2.trust-server-uri(default:true) to control whether server configuration can select the OAuth2 token endpoint.When set to
false:oauth2-server-uristarting withhttp://orhttps://.uri.Also align OAuth2 endpoint path handling with Java and reuse the resolved endpoint for refresh and child sessions. Add focused tests for these behaviors.
The default config merge precedence remains unchanged.