fix(rest): reject malformed authorization URLs

[fallintoplace]

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

[tanmayrauth]

Clean fix, the silent fallback to the default OAuth endpoint on a typo'd auth URL was a real credential-misrouting hazard

[zeroshade]

LGTM — fromProps now rejects malformed rest.authorization-url instead of silently skipping, propagated at both call sites. Reviewed + CI green. Thanks @fallintoplace!

[laskoviymishka]

Nice, focused fix — propagating the error through both newCatalogFromProps and fetchConfig is exactly right, and the error message format is clean. I'd hold it before merging though, because as written it doesn't quite close the hole it's aiming at.

url.Parse only errors on byte-level malformation, so the http://[::1 class is caught but the common typo — a missing scheme like localhost:8080 or example.com/auth, or an empty string — parses fine and sets authUri to a scheme-less URL. That sails past the authUri != nil fallback and points the OAuth exchange at a garbage endpoint, which is the exact silent misdirection this PR is trying to prevent. A u.Scheme != "" && u.Host != "" check right after the parse would make the fix actually cover the case people are likely to hit.

A couple of smaller things I left inline: the early return over a randomly-ordered map makes the failure non-deterministic, the server-config defaults path may be heavier than the threat warrants (worth scoping to overrides or to credential-configured loads), and the test only exercises defaults when overrides is the more sensitive precedence.

Quick recap of what I'd want before merge:

• validate scheme + host after url.Parse, not just the parse error
• add a test case with a scheme-less URL and one with the bad value in overrides
• clarify the threat model on the fetchConfig branch (it runs after setupOAuthManager)

One out-of-scope note for a follow-up: iceberg-go keys this on rest.authorization-url, while Java, PyIceberg, and Rust all use oauth2-server-uri — configs don't carry across clients. Not this PR's job, but worth a tracking issue.

Fix the scheme/host validation and add the scheme-less test and this is good to land — happy to take another pass.

---

Send to GH as a PENDING (draft) review? Reply "send to GH as draft" / "post as pending" to post the 5 inline comments. The top-level body stays here — paste it manually.

[laskoviymishka]

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:

if u.Scheme == "" || u.Host == "" {
    return fmt.Errorf("invalid %s %q: missing scheme or host", keyAuthUrl, v)
}

wdyt?

[laskoviymishka]

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.

[laskoviymishka]

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.

[laskoviymishka]

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.

[laskoviymishka]

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.