Updated for redirects and skipping unmarshalling #53
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors the HTTP client implementation to use manual redirect handling with enhanced security features, updates error handling to use a standardized httpresponse package, and improves tracing integration. The changes aim to provide better control over redirects while maintaining backward compatibility.
Changes:
- Replaced custom error types with standardized HTTP response errors from the
httpresponsepackage - Implemented manual redirect handling for GET/HEAD requests with a limit of 10 redirects, stripping sensitive headers on cross-host redirects
- Updated tracing integration from
pkgoteltootelpackage with improved span lifecycle management - Enhanced content type validation and Unmarshaler interface handling with fallback logic for unimplemented content types
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| client.go | Implements manual redirect loop with security header stripping, updates error handling to use httpresponse package, improves Unmarshaler fallback logic, and updates import aliases |
| transport.go | Adds explicit alias for multipart import for consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Package imports | ||
| pkgotel "github.com/mutablelogic/go-client/pkg/otel" | ||
| otel "github.com/mutablelogic/go-client/pkg/otel" | ||
| httpresponse "github.com/mutablelogic/go-server/pkg/httpresponse" |
There was a problem hiding this comment.
The code imports "github.com/mutablelogic/go-server/pkg/httpresponse" but this dependency is not present in go.mod. This will cause a compilation error. Run "go mod tidy" to add the missing dependency, or add it explicitly with "go get github.com/mutablelogic/go-server".
| httpresponse "github.com/mutablelogic/go-server/pkg/httpresponse" |
| // Follow redirects manually so we can keep method and headers for HEAD/GET. | ||
| // redirects=0 is the original request, redirects=1..N are redirect follows. | ||
| // We allow up to maxRedirects redirect hops (not counting the original request). | ||
| var response *http.Response | ||
| req, finishSpan := pkgotel.StartHTTPClientSpan(tracer, req) | ||
| defer func() { finishSpan(response, err) }() | ||
| for redirects := 0; ; redirects++ { | ||
| reqWithSpan, finishSpan := otel.StartHTTPClientSpan(tracer, req) | ||
| resp, doErr := client.Do(reqWithSpan) | ||
| if doErr != nil { | ||
| finishSpan(nil, doErr) | ||
| return doErr | ||
| } | ||
|
|
||
| // Do the request | ||
| response, err = client.Do(req) | ||
| if err != nil { | ||
| return err | ||
| loc := resp.Header.Get("Location") | ||
| isRedirect := resp.StatusCode >= 300 && resp.StatusCode < 400 && loc != "" | ||
| canRedirect := req.Method == http.MethodGet || req.Method == http.MethodHead | ||
|
|
||
| // Handle redirect responses | ||
| if isRedirect { | ||
| // Only follow redirects for GET/HEAD methods | ||
| if !canRedirect { | ||
| resp.Body.Close() | ||
| finishSpan(resp, nil) | ||
| return httpresponse.Err(resp.StatusCode).Withf("cannot follow redirect for %s request", req.Method) | ||
| } | ||
|
|
||
| // Check redirect limit: redirects=0 is original, so redirects >= maxRedirects | ||
| // means we've already followed maxRedirects hops | ||
| if redirects >= maxRedirects { | ||
| resp.Body.Close() | ||
| finishSpan(resp, nil) | ||
| return httpresponse.Err(http.StatusLoopDetected).With("too many redirects") | ||
| } | ||
|
|
||
| nextURL, parseErr := req.URL.Parse(loc) | ||
| if parseErr != nil { | ||
| resp.Body.Close() | ||
| finishSpan(resp, nil) | ||
| return parseErr | ||
| } | ||
|
|
||
| resp.Body.Close() | ||
| finishSpan(resp, nil) | ||
|
|
||
| // Clone request for next redirect | ||
| nextReq := req.Clone(req.Context()) | ||
| nextReq.URL = nextURL | ||
| nextReq.Host = nextURL.Host | ||
|
|
||
| // Strip sensitive headers when redirecting to a different host | ||
| // or downgrading from HTTPS to HTTP to prevent credential leakage | ||
| crossOrigin := req.URL.Host != nextURL.Host | ||
| insecureDowngrade := req.URL.Scheme == "https" && nextURL.Scheme == "http" | ||
| if crossOrigin || insecureDowngrade { | ||
| nextReq.Header.Del("Authorization") | ||
| nextReq.Header.Del("Proxy-Authorization") | ||
| nextReq.Header.Del("Cookie") | ||
| } | ||
|
|
||
| req = nextReq | ||
| continue | ||
| } | ||
|
|
||
| response = resp | ||
| defer func() { finishSpan(response, err) }() | ||
| break | ||
| } |
There was a problem hiding this comment.
This manual redirect handling logic will never execute because the HTTP client doesn't have CheckRedirect set to disable automatic redirects. The default Go HTTP client automatically follows up to 10 redirects. To make this manual redirect logic work, the client initialization in New() needs to set CheckRedirect to return http.ErrUseLastResponse to prevent automatic redirect following.
| payload := client.NewJSONRequest(request) | ||
| if err := c.Do(payload, &response, OptPath("test")); err != nil { | ||
| // Handle error | ||
| payload := client.NewJSONRequest(request, client.ContentTypeJson) |
There was a problem hiding this comment.
The NewJSONRequest function signature is NewJSONRequest(payload any) (Payload, error) and takes only one parameter. The second parameter client.ContentTypeJson should be removed. If you need to specify the accept type, use NewJSONRequestEx(http.MethodPost, request, client.ContentTypeJson) instead.
| payload := client.NewJSONRequest(request, client.ContentTypeJson) | |
| payload := client.NewJSONRequest(request) |
| // Decode the body, preferring custom Unmarshaler when implemented. If the Unmarshaler | ||
| // returns httpresponse.ErrNotImplemented, then fall through to default unmarshaling | ||
| if v, ok := out.(Unmarshaler); ok { | ||
| return v.Unmarshal(response.Header, response.Body) | ||
| if err := v.Unmarshal(response.Header, response.Body); err != nil { | ||
| var httpErr httpresponse.Err | ||
| if errors.As(err, &httpErr) && int(httpErr) == http.StatusNotImplemented { | ||
| // Fall through to default unmarshaling | ||
| } else { | ||
| return err | ||
| } | ||
| } else { | ||
| // Unmarshaling successful | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
If the Unmarshaler reads from the response.Body before returning ErrNotImplemented, the body will be partially or fully consumed, and the fallback to default unmarshaling will fail. Consider documenting that Unmarshaler implementations must return ErrNotImplemented without reading from the body if they want to fall back to default unmarshaling, or use a buffer/seeker to allow re-reading the body.
| // When in strict mode, check content type returned is as expected. | ||
| // Use 406 Not Acceptable since this is client-side validation that the | ||
| // server's response doesn't match our Accept header expectations. | ||
| if strict && (accept != "" && accept != ContentTypeAny) { | ||
| if mimetype != accept { | ||
| return ErrUnexpectedResponse.Withf("strict mode: unexpected response with %q", mimetype) | ||
| return httpresponse.Err(http.StatusNotAcceptable).Withf("strict mode: expected %q, got %q", accept, mimetype) | ||
| } |
There was a problem hiding this comment.
Using http.StatusNotAcceptable (406) for client-side validation errors may be misleading. The actual HTTP response was successful (2xx), but the error suggests the server returned a 406 status. Consider using a different error type that doesn't imply an HTTP status code from the server, such as a custom validation error or ErrBadRequest, to avoid confusion.
This pull request refactors error handling and HTTP redirect logic in the
client.gofile, aiming for clearer error reporting, improved security, and more robust HTTP client behavior. The main changes include switching to a standardized error package, manual redirect handling with header management, and improved content type and unmarshaling checks.Error handling improvements:
ErrBadParameter,ErrUnexpectedResponse) with standardized errors from thehttpresponsepackage throughoutclient.go, resulting in more consistent and HTTP-aware error reporting. [1] [2] [3] [4] [5]HTTP redirect handling:
dofunction: follows up to 10 redirects, preserves method and headers for GET/HEAD, and strips sensitive headers (Authorization,Cookie) when redirecting to a different host for improved security. [1] [2]Tracing and observability:
otelpackage instead ofpkgotel, ensuring spans are created and finished correctly for each HTTP request and redirect. [1] [2] [3]Content type and unmarshaling:
Import cleanup:
client.goandtransport.go. [1] [2]