PR: Sub Issue 1 - Security hardening (SSL bypass fix, HTTP auth, HttpClient pooling)#309
Closed
JusterZhu wants to merge 1 commit into
Closed
PR: Sub Issue 1 - Security hardening (SSL bypass fix, HTTP auth, HttpClient pooling)#309JusterZhu wants to merge 1 commit into
JusterZhu wants to merge 1 commit into
Conversation
…ling - Remove CheckValidationResult() returning always true; replace with StrictSslValidationPolicy - Add ISslValidationPolicy interface for pluggable cert validation - Add IHttpAuthProvider abstraction with 4 built-in implementations: BearerTokenAuthProvider, ApiKeyAuthProvider, HmacAuthProvider, NoOpAuthProvider - Add HttpAuthProviderFactory for auto-selection based on scheme/token/secretKey - Replace per-request new HttpClient() with static singleton (shared handler) - Add exponential-backoff retry logic for transient HTTP failures - Preserve backward-compatible static API surface Closes #308
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the update framework’s HTTP communication by removing permissive TLS certificate validation, introducing pluggable SSL/auth abstractions, and refactoring VersionService to use a shared HttpClient with retry logic.
Changes:
- Added
ISslValidationPolicy(+ strict default) to avoid accepting invalid/untrusted server certificates. - Added
IHttpAuthProvider(+ built-in Bearer/API key/HMAC/No-Op and factory) to make request auth pluggable. - Refactored
VersionServiceto use a sharedHttpClientand add retry/backoff behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/c#/GeneralUpdate.Common/Shared/Security/ISslValidationPolicy.cs | Introduces a certificate validation policy abstraction and strict default policy. |
| src/c#/GeneralUpdate.Common/Shared/Security/IHttpAuthProvider.cs | Adds auth provider interface, implementations, and provider selection factory. |
| src/c#/GeneralUpdate.Common/Shared/Service/VersionService.cs | Refactors HTTP request pipeline: shared client/handler, auth integration, retries. |
| src/c#/GeneralUpdate.Core/GeneralUpdate.Core.csproj | Links new shared Security sources into Core build. |
| src/c#/GeneralUpdate.ClientCore/GeneralUpdate.ClientCore.csproj | Links new shared Security sources into ClientCore build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+56
to
+60
| X509Chain? chain, | ||
| SslPolicyErrors sslPolicyErrors) | ||
| { | ||
| return _globalSslPolicy.ValidateCertificate(certificate, chain, sslPolicyErrors); | ||
| } |
Comment on lines
+47
to
+51
| public static void SetSslValidationPolicy(ISslValidationPolicy policy) | ||
| { | ||
| _globalSslPolicy = policy ?? throw new ArgumentNullException(nameof(policy)); | ||
| _handlerInitialized = true; | ||
| } |
Comment on lines
+188
to
+197
| // Apply auth | ||
| await _authProvider.ApplyAuthAsync(request, token).ConfigureAwait(false); | ||
|
|
||
| // Build body | ||
| var parametersJson = JsonSerializer.Serialize( | ||
| parameters, HttpParameterJsonContext.Default.DictionaryStringObject); | ||
| request.Content = new StringContent(parametersJson, Encoding.UTF8, "application/json"); | ||
| request.Headers.Accept.ParseAdd("application/json"); | ||
|
|
||
| // Re-apply auth after setting content (HMAC needs the body) |
Comment on lines
+204
to
+208
| var response = await _sharedClient.SendAsync(request, timeoutCts.Token) | ||
| .ConfigureAwait(false); | ||
|
|
||
| response.EnsureSuccessStatusCode(); | ||
|
|
Comment on lines
+224
to
+226
| if (ex is OperationCanceledException) | ||
| return false; | ||
|
|
Comment on lines
143
to
147
| { "Version", version }, | ||
| { "AppType", appType }, | ||
| { "AppKey", appKey }, | ||
| { "Platform", platform }, | ||
| { "ProductId", productId } | ||
| }; |
Comment on lines
+229
to
+237
| // Retry on timeout and server errors, not on client/SSL errors | ||
| var msg = hre.Message ?? string.Empty; | ||
| return msg.Contains("timeout", StringComparison.OrdinalIgnoreCase) | ||
| || msg.Contains("timed out", StringComparison.OrdinalIgnoreCase) | ||
| || msg.Contains("server", StringComparison.OrdinalIgnoreCase) | ||
| || msg.Contains("500") | ||
| || msg.Contains("502") | ||
| || msg.Contains("503") | ||
| || msg.Contains("504"); |
Comment on lines
+130
to
+150
| /// </summary> | ||
| /// <param name="scheme">Auth scheme: "Bearer", "ApiKey", or null.</param> | ||
| /// <param name="token">Auth token / API key.</param> | ||
| /// <param name="appSecretKey">HMAC secret key (takes priority over Token/Scheme).</param> | ||
| /// <returns>The appropriate IHttpAuthProvider.</returns> | ||
| public static IHttpAuthProvider Create(string? scheme, string? token, string? appSecretKey) | ||
| { | ||
| // HMAC takes priority (used for signed requests) | ||
| if (!string.IsNullOrEmpty(appSecretKey)) | ||
| return new HmacAuthProvider(appSecretKey); | ||
|
|
||
| // Bearer / API Key based on scheme | ||
| if (!string.IsNullOrEmpty(token)) | ||
| { | ||
| return (scheme ?? string.Empty).ToLowerInvariant() switch | ||
| { | ||
| "apikey" => new ApiKeyAuthProvider(token), | ||
| _ => new BearerTokenAuthProvider(token) // Bearer is the default | ||
| }; | ||
| } | ||
|
|
Comment on lines
+126
to
+130
| public static class HttpAuthProviderFactory | ||
| { | ||
| /// <summary> | ||
| /// Auto-select an auth provider based on the provided parameters. | ||
| /// </summary> |
Collaborator
Author
|
Closing: will re-implement in the new unified GeneralUpdate.Core project per Sub Issue 2 of the refactor plan. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #308
Changes
ew HttpClient()\ with a static singleton using shared handler — eliminates port exhaustion risk
Files changed
Build
✅ \GeneralUpdate.Common\ — 0 errors
✅ \GeneralUpdate.Core\ — 0 errors