zeroshade commented on code in PR #2098:
URL: https://github.com/apache/iceberg-go/pull/2098#discussion_r4168426529
##########
catalog/rest/rest.go:
##########
@@ -1122,6 +1147,7 @@ func (r *Catalog) createSession(ctx context.Context, opts
*options) (*http.Clien
session.defaultHeaders.Set("Content-Type", "application/json")
session.defaultHeaders.Set("User-Agent", "GoIceberg/"+iceberg.Version())
session.defaultHeaders.Set(headerIcebergAccessDelegation,
defaultAccessDelegation)
+ session.builtinHeaders = session.defaultHeaders.Clone()
Review Comment:
Minor: this snapshot is taken before the `WithHeaders` / `header.*` loops
below, so a cross-origin hop gets the built-in *default* for any key the
operator overrode. I checked this at `ab8b1b3` with `WithHeaders({"User-Agent":
"corp-agent/1"})` and `header.X-Iceberg-Access-Delegation=remote-signing`. The
same-origin request sends those values, but the hop sends `GoIceberg/…` and
`vended-credentials`. Before this PR the hop got the overrides. It also doesn't
match the field doc ("the subset of defaultHeaders"). Building it after the
override loops, from just the built-in keys, keeps the two consistent:
```go
session.builtinHeaders = http.Header{}
for _, k := range []string{"X-Client-Version", "Content-Type", "User-Agent",
headerIcebergAccessDelegation} {
if v := session.defaultHeaders.Values(k); len(v) > 0 {
session.builtinHeaders[k] = v
}
}
```
If reverting to the defaults is intended, the field doc should say "built-in
defaults" instead.
##########
catalog/rest/rest.go:
##########
@@ -282,12 +296,21 @@ func defaultedPort(u *url.URL) string {
const emptyStringHash =
"e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
func (s *sessionTransport) RoundTrip(r *http.Request) (*http.Response, error) {
+ // net/http strips Authorization from cross-origin redirect hops, but
this
Review Comment:
Nit: net/http's redirect stripping is narrower than this reads. It only
drops `Authorization` it copied from the first request, and only for a new
hostname. A port or scheme change, or a hop to a subdomain, keeps it
(`shouldCopyHeaderOnRedirect`). That's why this gate is needed even for the
port-only redirect the new test uses.
```suggestion
// net/http strips Authorization on redirect only for a new hostname (a
// port or scheme change, or a hop to a subdomain, keeps it), but this
```
--
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]
