feat(client): extra headers and optional http/1.1 default transport#2
feat(client): extra headers and optional http/1.1 default transport#2
Conversation
Integrators can set ExtraHeaders after mandatory MS-ASHTTP headers and opt into disabling HTTP/2 ALPN when using the library-built HTTP client, which helps Exchange deployments that mishandle HTTP/2.
There was a problem hiding this comment.
Pull request overview
Adds integrator-configurable outbound headers and an opt-in mode to disable HTTP/2 negotiation when the library constructs its own HTTP client, improving interoperability with Exchange deployments that mishandle HTTP/2.
Changes:
- Add
Config.ExtraHeaders(merged after mandatory MS-ASHTTP headers without overriding existing keys) andConfig.ForceHTTP11(disables HTTP/2 ALPN on the default transport whenHTTPClientis nil). - Implement header-merge helper and apply it to all outbound requests.
- Add spec coverage rows, tests (unit + integration), and README documentation for an “Outlook-like” profile.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/spec/coverage.csv | Adds new spec requirements for extra headers merge and ForceHTTP11 transport behavior |
| client/integration_test.go | Integration test validating extra headers are sent and mandatory headers aren’t overridden |
| client/headers_test.go | Unit tests for mergeExtraHeaders behavior (add, skip existing, canonicalize, preserve duplicates) |
| client/headers.go | Implements mergeExtraHeaders helper |
| client/client_profile_test.go | Adds tests for ForceHTTP11 transport configuration + extra headers during Provision |
| client/client_extra_test.go | Adds tests ensuring ForceHTTP11 affects only library-built clients and doesn’t mutate custom transports |
| client/client.go | Adds config/client fields, ForceHTTP11 transport logic, and applies extra headers to requests |
| README.md | Documents ExtraHeaders + ForceHTTP11 usage for an Outlook-like profile |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Config bundles the values required to construct a Client. | ||
| type Config struct { | ||
| BaseURL string | ||
| HTTPClient *http.Client | ||
| Auth Authenticator | ||
| DeviceID string | ||
| DeviceType string | ||
| UserAgent string | ||
| Locale uint16 | ||
| BaseURL string | ||
| HTTPClient *http.Client | ||
| Auth Authenticator | ||
|
|
||
| DeviceID string | ||
| // DeviceType is the device class in the MS-ASHTTP query (e.g. "SmartPhone"). | ||
| // For an Outlook-style profile many servers expect DeviceType "Outlook". | ||
| DeviceType string | ||
|
|
||
| // UserAgent is sent as the mandatory User-Agent header. | ||
| UserAgent string | ||
|
|
||
| // Locale is the LCID placed in the binary query (little-endian uint16), for | ||
| // example 0x0409 (en-US) or 0x0419 (ru-RU). | ||
| Locale uint16 | ||
|
|
||
| AcceptLanguage string | ||
|
|
||
| // ExtraHeaders optional integrator headers (device model, OS, or other | ||
| // vendor expectations). They are merged after mandatory headers and never | ||
| // replace keys the client already set; device model/OS are not separate | ||
| // Config fields because MS-ASHTTP only standardizes the query DeviceType. | ||
| ExtraHeaders http.Header | ||
|
|
||
| // ForceHTTP11, when true and HTTPClient is nil, builds an HTTP client whose | ||
| // transport clones http.DefaultTransport and disables HTTP/2 by setting | ||
| // TLSNextProto to a non-nil empty map. When HTTPClient is non-nil, | ||
| // ForceHTTP11 is ignored and the caller's transport is not modified. | ||
| ForceHTTP11 bool | ||
|
|
||
| PolicyStore PolicyStore | ||
| SyncStateStore SyncStateStore |
There was a problem hiding this comment.
New fields ExtraHeaders / ForceHTTP11 are inserted before PolicyStore/SyncStateStore in the exported Config (and similarly in exported Client). This is a breaking change for any downstream code using unkeyed struct literals (e.g., providing PolicyStore as a positional element will now fail to compile due to the shifted types). To preserve Go API compatibility, append new exported fields to the end of the struct (or avoid reordering existing fields).
| } | ||
| tr := dt.Clone() | ||
| tr.TLSNextProto = make(map[string]func(authority string, c *tls.Conn) http.RoundTripper) | ||
| c.HTTPClient = &http.Client{Transport: tr} |
There was a problem hiding this comment.
When ForceHTTP11 is true and HTTPClient is nil, New constructs a fresh http.Client{Transport: tr}. This drops any non-Transport settings that callers may have configured on http.DefaultClient (e.g., Timeout, CheckRedirect, Jar) and can behave differently from the non-ForceHTTP11 default path that reuses http.DefaultClient. Consider cloning *http.DefaultClient (copy the struct) and only swapping the Transport, so ForceHTTP11 preserves default client behavior aside from disabling HTTP/2.
| c.HTTPClient = &http.Client{Transport: tr} | |
| client := *http.DefaultClient | |
| client.Transport = tr | |
| c.HTTPClient = &client |
| // ExtraHeaders are merged into each request after mandatory headers without | ||
| // overwriting keys already set (see Config.ExtraHeaders). | ||
| ExtraHeaders http.Header | ||
|
|
||
| // ForceHTTP11 reflects the config flag; when HTTPClient was supplied to New | ||
| // the transport is never altered and this bit is informational only. | ||
| ForceHTTP11 bool |
There was a problem hiding this comment.
Client.ExtraHeaders is an exported http.Header (map) that is read on every request. If a caller mutates this map while requests are in-flight, it can cause data races or panics due to concurrent map iteration/writes. Consider keeping the headers unexported (store an internal clone) and exposing them via a method that returns a clone, or clearly documenting that the field must be treated as immutable after New (and not modified concurrently).
Move ExtraHeaders and ForceHTTP11 after Policy/Sync stores so positional Config literals stay valid. Copy http.DefaultClient when swapping Transport for ForceHTTP11 (preserve Timeout and other defaults). Document header map mutability; note append-only field order in AGENTS.md.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| saw = r.Header.Get("X-Integration-Probe") | ||
| body, err := io.ReadAll(r.Body) | ||
| if err != nil { | ||
| http.Error(w, err.Error(), 500) | ||
| return | ||
| } | ||
| var req eas.ProvisionRequest | ||
| if err := wbxml.Unmarshal(body, &req); err != nil { | ||
| http.Error(w, err.Error(), 400) | ||
| return | ||
| } | ||
| calls++ | ||
| var resp eas.ProvisionResponse |
There was a problem hiding this comment.
This test only asserts X-Integration-Probe after Provision completes, but Provision issues two HTTP requests; the current check could pass even if the header is missing on the first phase (or only present on one of the calls). Consider asserting the header value per-call inside the handler (e.g., check on both calls 1 and 2) so the requirement is covered for every outbound request.
Integrators can set ExtraHeaders after mandatory MS-ASHTTP headers and opt into disabling HTTP/2 ALPN when using the library-built HTTP client, which helps Exchange deployments that mishandle HTTP/2.