Skip to content

Commit 90ea185

Browse files
Fix ApiConnection authorization header thread safety (#9610)
* Fix ApiConnection authorization header thread safety SetAuthenticator(HttpRequestMessage) was writing the Authorization header to the shared HttpClient.DefaultRequestHeaders on every request. HttpHeaders is not thread safe: under concurrent requests one thread mutates the collection while another enumerates it during request serialization, causing NullReferenceException failures, and requests could also be serialized with a mismatched Authorization/Timestamp pair causing intermittent authentication failures. Set the Authorization header on the request message itself instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Include stack trace when logging unexpected api request exceptions Network failures are already described by their HttpRequestError and SocketErrorCode details and are frequent during outages, so they stay on a single line. Any other exception type is unexpected, include its stack trace so the source can be located directly from the logs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Harden ApiConnection.TryRequestAsync request path Guard against a null RequestUri, which is valid when the client has a BaseAddress set, and handle response deserialization failures with the http status code and raw response content instead of surfacing a parse exception, for example an html error page returned by a proxy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent c283231 commit 90ea185

1 file changed

Lines changed: 22 additions & 4 deletions

File tree

Api/ApiConnection.cs

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,7 @@ public async Task<Tuple<bool, T>> TryRequestAsync<T>(HttpRequestMessage request,
245245
var stopwatch = Stopwatch.StartNew();
246246
try
247247
{
248-
if (request.RequestUri.OriginalString.StartsWith('/'))
248+
if (request.RequestUri != null && request.RequestUri.OriginalString.StartsWith('/'))
249249
{
250250
request.RequestUri = new Uri(request.RequestUri.ToString().TrimStart('/'), UriKind.Relative);
251251
}
@@ -256,7 +256,18 @@ public async Task<Tuple<bool, T>> TryRequestAsync<T>(HttpRequestMessage request,
256256
response = await _httpClient.SendAsync(request, cancellationTokenSource.Token).ConfigureAwait(false);
257257
responseContentStream = await response.Content.ReadAsStreamAsync(cancellationTokenSource.Token).ConfigureAwait(false);
258258

259-
result = responseContentStream.DeserializeJson<T>(leaveOpen: true);
259+
try
260+
{
261+
result = responseContentStream.DeserializeJson<T>(leaveOpen: true);
262+
}
263+
catch (Exception err)
264+
{
265+
// a non json payload, for example an html error page from a proxy or load balancer,
266+
// the http status and raw content describe the failure better than the parse exception
267+
Log.Error($"ApiConnection.TryRequest({request.RequestUri}): failed to deserialize response: {err.GetType().Name}: {err.Message}." +
268+
$" HTTP {(int)response.StatusCode} {response.ReasonPhrase}. Content: {GetRawResponseContent(responseContentStream)}");
269+
return new Tuple<bool, T>(false, null);
270+
}
260271

261272
if (!response.IsSuccessStatusCode)
262273
{
@@ -323,6 +334,12 @@ private static string DescribeError(Exception exception, bool timeoutElapsed, lo
323334
}
324335
}
325336
builder.Append(Invariant($" after {elapsedMilliseconds}ms. ThreadPool: {ThreadPool.ThreadCount} threads, {ThreadPool.PendingWorkItemCount} pending work items"));
337+
if (exception is not HttpRequestException and not OperationCanceledException)
338+
{
339+
// network failures are described by their error codes above and are frequent during outages,
340+
// any other exception type is unexpected so include the stack trace to locate its source
341+
builder.Append(Environment.NewLine).Append(exception.StackTrace);
342+
}
326343
return builder.ToString();
327344
}
328345
catch (Exception)
@@ -360,11 +377,12 @@ private void SetAuthenticator(RestRequest request)
360377

361378
private void SetAuthenticator(HttpRequestMessage request)
362379
{
363-
request.Headers.Remove("Authorization");
364380
request.Headers.Remove("Timestamp");
365381

366382
var base64EncodedAuthenticationString = GetAuthenticatorHeader(out var timeStamp);
367-
_httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Basic", base64EncodedAuthenticationString);
383+
// set the authorization on the request itself, the client default headers are shared across
384+
// concurrent requests and mutating them while requests are in flight is not thread safe
385+
request.Headers.Authorization = new AuthenticationHeaderValue("Basic", base64EncodedAuthenticationString);
368386
request.Headers.Add("Timestamp", timeStamp);
369387
}
370388

0 commit comments

Comments
 (0)