proposal: net/http: add Client.PrepareRequest

[neild]

The net/http package has three (possibly soon to be four) types representing an HTTP client, in the sense of something which can send an HTTP request and receive a response:

• RoundTripper is an interface representing the ability to execute a single HTTP transaction.
• Transport is an implementation of RoundTripper. It manages a pool of cached connections.
• Client is a concrete type which wraps a RoundTripper. It adds cookie management and redirect following handling.
• ClientConn is a proposed new addition (see net/http: client connection API #75772). This would be a RoundTripper implementation that represents a single connection.

The RoundTripper interface permits middleware to wrap a Transport or other RoundTripper and add additional functionality. For example, oauth2.Transport is a RoundTripper that adds "Authorization" headers to outbound requests.

While useful in some situations, the RoundTripper layer is the wrong place to add some types of functionality. For example, the oauth2.Transport I just mentioned can cause incorrect behavior when following a redirect: When an http.Client follows a cross-domain redirect, it strips sensitive headers (including "Authorization") from the redirected request. However, oauth2.Transport adds this header at a lower level than http.Client and has no way to identify redirect requests. Therefore, the "Authorization" header is still sent after a cross-domain redirect. (This is golang/oauth2#500, and I don't see any good way to fix it in the context of the current oauth2 package API.)

Middleware which acts on an entire request (including redirects) as opposed to an individual transaction needs to wrap the http.Client, not an RoundTripper.

Client is a concrete type. An API which wants to accept a user-provided client either accepts a concrete *http.Client (which precludes client-level middleware) or needs to define some additional interface. For example:

• https://pkg.go.dev/github.com/aws/aws-sdk-go/aws#Config.HTTPClient is a concrete *http.Client.
• https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/aws#Config.HTTPClient is an HTTPClient interface defined in the same package.
• Decouple client transports from *http.Client by accepting an interface modelcontextprotocol/go-sdk#522 contains some discussion on whether an API should accept an *http.Client or some locally-defined interface type.

We could add a new interface type to net/http that is implemented by *Client, and encourage APIs to accept this type rather than a concrete *Client. However, this approach doesn't do anything to help existing APIs which take a concrete *Client. In addition, *Client has six methods; while only one of them (Client.Do) is necessary for this use case, adding an interface type that is less useful than *Client itself seems unfortunate.

I instead propose that we add a new field to Client to provide a place to insert request-modifying middleware:

type Client struct {
  // PrepareRequest specifies a per-request hook.
  // If PrepareRequest is not nil, the client calls it before sending a request.
  // It is not called after redirects.
  //
  // PrepareRequest returns the request to send or an error to terminate the request.
  // If PrepareRequest needs to modify the request, it should clone the 
  PrepareRequest func(*Request) (*Request, error)

  // ...existing fields
}

The PrepareRequest hook gives us a way to inject middleware layers into any code which currently uses an *http.Client. For example, it would let us change oauth2.Config.Client to return an *http.Client that injects an Authorization header, but which still strips the header after cross-domain redirects.

[seankhliao]

This should hopefully resolve #59266 too?

[gabyhelp]

Related Issues

• proposal: net/http: add `RoundTripperFunc` and `Middleware` for server & client #38479
• proposal: net/http: add `Middleware` interface #60344 (closed)
• proposal: net/http: add HTTP transport interceptors #66844 (closed)
• net/http: clarify relationship of RoundTripper to Client #59266
• proposal: x/net/context: Context-aware HTTP helper functions. #11904 (closed)
• proposal: net/http: set default headers in http.Client #64309 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[earthboundkid]

As the opener of #59266 I would rather go the other way and try to unify RoundTripper and Client somehow. If Client were also a RoundTripper or had a method that returned a RoundTripper you could just accept that everywhere.

I don't think there's an appetite to bring back #23707, but thinking about it a little more we have:

• Individual connections (possibly exposed by net/http: client connection API #75772)
• A connection pool (Transport)
• The parsing that turns streams into Request and Response objects and is different for HTTP 1, 2, and 3
• Cookie and redirect handling (concrete http.Client or interface Do)
• Higher level concerns like auth and request preparation.

Ideally there would be some way for users to get into the tree of those and wrap any layer of it with a dummy for testing or an interceptor for auth (for example, requiring some certificate for HTTPS or injecting an Authorization header).

Maybe something like

type RoundTripStack interface {
  RoundTripper
  Parent() (RoundTripper, error)
  Role() string
}

And then there are fixed role names for different levels of the tree?

[neild]

If Client was a RoundTripper, you'd never be certain whether a RoundTrip call was going to follow redirects or not. The current collection of things-that-send-a-request is a mess, but I think it's a good thing that methods that do different things have different names.

The Client/RoundTripper distinction is confusing and if we were redesigning everything from scratch we could possibly find a better way to structure everything. Given the existing package design, however, I think the clearest approach is to say that most users should use a concrete Client and to make Client sufficiently configurable to permit inserting whatever behaviors are required.

[earthboundkid]

I really like how ResponseController solved this on the server side, and I'm wracking my brain to think about how a hypothetical RequestController would do it on the client side, but I take the point that even if multiple things are just turning a Request into a Response it might still be better to keep the names different to reflect that they have different responsibilities in the process.

[neild]

A related thought:

I wonder if we should add a Request field indicating that a request has been made after a cross-origin redirect. Then a RoundTripper could safely add an Authorization header:

if !req.IsCrossOriginRedirect {
  req.Header.Set("Authorization", token) // don't set this header after cross-origin redirects
}

Perhaps we could even have the *http.Transport strip sensitive headers when Request.IsCrossOriginRedirect is set, so existing RoundTrippers (like the one in oauth2) which inadvertently add sensitive headers after a redirect will have those headers removed before sending.

[seankhliao]

What about something more like the CheckRedirect function, but allows modifications?

[earthboundkid]

If Request had a method Via() iter.Seq[*Request] (a iterable because it's following a linked list of parent *Request fields, not a slice) then the transport could check that instead of needing a dedicated IsCrossOriginRedirect field. You might still want to add func (req *Request) IsCrossOrigin(origin *Request) bool though.

[earthboundkid]

I suppose IsCrossOrigin could be a method on url.URL instead.

[neild]

What's the use case for providing the history of prior requests?