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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
