You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/06/27 17:00:52 UTC
[PR] fix(rest): reject malformed authorization URLs [iceberg-go]
fallintoplace opened a new pull request, #1336:
URL: https://github.com/apache/iceberg-go/pull/1336
Summary
- reject malformed `rest.authorization-url` values instead of silently ignoring them
- propagate auth URL parse failures from both initial catalog props and fetched REST config
- add regression coverage for invalid local props and invalid server config
Why
`fromProps` previously swallowed `url.Parse` errors and left `authUri` unset. When OAuth credentials were later used, the client fell back to `baseURI.JoinPath("oauth/tokens")`, so a typo in `rest.authorization-url` could quietly send credentials to a different endpoint than intended.
Testing
- `go test ./catalog/rest -run 'Test(LoadRegisteredCatalogRejectsInvalidAuthURL|NewCatalogRejectsInvalidAuthURLFromConfig)$' -count=1`
- `go test ./catalog/rest -count=1`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(rest): reject malformed authorization URLs [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #1336:
URL: https://github.com/apache/iceberg-go/pull/1336
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(rest): reject malformed authorization URLs [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1336:
URL: https://github.com/apache/iceberg-go/pull/1336#discussion_r3491566501
##########
catalog/rest/rest.go:
##########
@@ -464,7 +464,7 @@ func fromProps(props iceberg.Properties, o *options) {
case keyAuthUrl:
u, err := url.Parse(v)
if err != nil {
- continue
+ return fmt.Errorf("invalid %s %q: %w", keyAuthUrl, v, err)
Review Comment:
One subtlety with switching `continue` to `return` here: we iterate `props` as a Go map, so the order is non-deterministic, and returning on the first bad value abandons every key we haven't visited yet.
In practice the only failure here is a bad auth URL so the abandoned-keys part is mostly theoretical, but the non-determinism is real — if a future change adds a second validating case, which error surfaces will vary run to run. I'd lean toward validating-then-applying (or collecting the first error and returning it after the loop) so the behavior is stable. Not blocking on its own, but cheap to make deterministic while we're here.
##########
catalog/rest/rest.go:
##########
@@ -464,7 +464,7 @@ func fromProps(props iceberg.Properties, o *options) {
case keyAuthUrl:
u, err := url.Parse(v)
if err != nil {
- continue
+ return fmt.Errorf("invalid %s %q: %w", keyAuthUrl, v, err)
Review Comment:
I think this only catches part of the problem we're trying to fix. `url.Parse` is permissive enough that it only errors on byte-level malformation — the `http://[::1` fixture, broken percent-encoding, that class. The common typo is a missing scheme, and those slip through.
`url.Parse("localhost:8080")`, `url.Parse("example.com/auth")`, and `url.Parse("")` all succeed and hand back a URL with an empty `Scheme`/`Host`. That sets `o.authUri` non-nil, so it bypasses the `authUri != nil` fallback later and the OAuth exchange targets the wrong endpoint — the exact silent misdirection this PR is closing, just via the typo people are actually likely to make.
I'd validate scheme and host right here:
```go
if u.Scheme == "" || u.Host == "" {
return fmt.Errorf("invalid %s %q: missing scheme or host", keyAuthUrl, v)
}
```
wdyt?
##########
catalog/rest/rest_internal_test.go:
##########
@@ -49,6 +51,40 @@ import (
"golang.org/x/sync/errgroup"
)
+func TestLoadRegisteredCatalogRejectsInvalidAuthURL(t *testing.T) {
+ t.Parallel()
+
+ cat, err := catalog.Load(context.Background(), "rest", iceberg.Properties{
+ "uri": "http://example.com",
+ "rest.authorization-url": "http://[::1",
+ })
+ require.Error(t, err)
+ assert.Nil(t, cat)
+ assert.ErrorContains(t, err, "invalid rest.authorization-url")
+}
+
+func TestNewCatalogRejectsInvalidAuthURLFromConfig(t *testing.T) {
+ t.Parallel()
+
+ mux := http.NewServeMux()
+ srv := httptest.NewServer(mux)
+ defer srv.Close()
+
+ mux.HandleFunc("/v1/config", func(w http.ResponseWriter, r *http.Request) {
+ json.NewEncoder(w).Encode(map[string]any{
+ "defaults": map[string]any{
+ "rest.authorization-url": "http://[::1",
Review Comment:
The bad URL only goes into `defaults`, but the merge precedence is defaults < client props < overrides (`maps.Copy(cfg, rsp.Overrides)`), and `overrides` is the server-controlled case the user can't suppress — the more sensitive one. I'd add a sub-case that puts the malformed URL in `overrides` and asserts the same error, so both paths are pinned.
If you take the "only fatal for overrides / only when a credential is set" suggestion from the `fetchConfig` thread, this test is also where you'd nail down that distinction.
##########
catalog/rest/rest.go:
##########
@@ -765,7 +769,9 @@ func (r *Catalog) fetchConfig(ctx context.Context, opts *options) (*http.Client,
r.endpoints = resolveEndpoints(rsp.Endpoints, cfg.GetBool(keyViewEndpointsSupported, false))
o := *opts
- fromProps(cfg, &o)
+ if err := fromProps(cfg, &o); err != nil {
Review Comment:
Two things on the server-config path here.
A malformed value in the server's `defaults` now hard-fails the whole catalog load, even for a user who never configured a credential and has no OAuth intent at all. `defaults` are meant to be server suggestions a client can override, so failing init on one feels heavier than the threat warrants. I'd consider scoping the fatal behavior to `overrides`, or only erroring when `opts.credential != ""` — no credential, no misdirection risk.
It's also worth a one-line comment on the threat model: `fetchConfig` already ran `setupOAuthManager` from the local opts before this point, so the server-supplied auth-url doesn't actually rebuild the OAuth manager in the current flow. This guard catches a misconfigured server early, which is useful, but the credential-misdirection scenario from the PR description is really the local-props path. Worth being explicit about which case this branch is defending.
##########
catalog/rest/rest_internal_test.go:
##########
@@ -49,6 +51,40 @@ import (
"golang.org/x/sync/errgroup"
)
+func TestLoadRegisteredCatalogRejectsInvalidAuthURL(t *testing.T) {
+ t.Parallel()
+
+ cat, err := catalog.Load(context.Background(), "rest", iceberg.Properties{
+ "uri": "http://example.com",
+ "rest.authorization-url": "http://[::1",
Review Comment:
`http://[::1` is doing a lot of documentation work here as the one malformation the current fix catches — a reader could easily assume `""` or `example.com/auth` are covered too, when they aren't. If we add the scheme/host validation, I'd extend this (or add a table case) to cover a scheme-less value, so the test communicates the actual contract.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org