laskoviymishka commented on code in PR #1330:
URL: https://github.com/apache/iceberg-go/pull/1330#discussion_r3491552970
##########
catalog/rest/rest.go:
##########
@@ -451,6 +451,8 @@ func handleNon200(rsp *http.Response, override
map[int]error) error {
func fromProps(props iceberg.Properties, o *options) {
for k, v := range props {
switch k {
+ case keyOauthToken:
Review Comment:
Now that `fromProps` reads `keyOauthToken`, the server-override path becomes
reachable for the token. `fetchConfig` merges `defaults → toProps(opts) →
overrides` and then re-parses the merged map through `fromProps`, so a server
returning `{"token": "..."}` in overrides silently overwrites the caller's
static token in `o`. The live HTTP session was already built from the pre-fetch
token (line ~733), so we end up with `o`/`r.props` holding the server token
while the active session still uses the original — and a misconfigured or
untrusted server can redirect client auth.
I'd at minimum guard it so the caller's token wins:
```go
case keyOauthToken:
if o.oauthToken == "" {
o.oauthToken = v
}
```
If server-vended token override is actually intended here (the spec does
allow it), I'm fine leaving it unguarded — but then I'd want a one-line comment
saying so, because the silent overwrite reads like a bug otherwise. wdyt?
--
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]
