Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReplaces instance/IHttpClientFactory-driven client creation with a static HTTP implementation that can optionally cache HttpClient via new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant HTTP as Static HTTP
participant Handler as HttpClientHandler
participant CertStore as CertificateStore
participant Server as Remote Server (httpbin)
Caller->>HTTP: Request(options)
HTTP->>Handler: Build/configure handler from options
Handler->>CertStore: GetCertificates(options, ref certificates)
CertStore-->>Handler: Certificate(s)
HTTP->>HTTP: Get or create HttpClient (cache if CacheHttpClient==true)
HTTP->>Server: Send HttpRequestMessage
Server-->>HTTP: HttpResponseMessage
HTTP-->>Caller: Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Frends.HTTP.Request/Frends.HTTP.Request/Request.cs (1)
62-66:⚠️ Potential issue | 🟠 MajorAdd
SuccessandErrorproperties to theResultclass.The
Resultclass inFrends.HTTP.Request/Frends.HTTP.Request/Definitions/Result.csis missing theSuccess(bool) andErrorobject properties required by coding guidelines. Currently, the class only containsBody,Headers, andStatusCode. Per guidelines, all task result classes must includeSuccess, a task-specific return value, and anErrorobject withMessageandAdditionalInfofields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs` around lines 62 - 66, Add two properties to the Result class in Frends.HTTP.Request/Frends.HTTP.Request/Definitions/Result.cs: a bool Success and an Error object (class or property) containing string Message and object or Dictionary<string, object> AdditionalInfo; then update the Request method (Request in Request.cs) to set Result.Success (e.g., true for 2xx status codes) and populate Result.Error when Success is false with a concise Message and any useful AdditionalInfo (status code, response body, exception details) so the task result conforms to the coding guidelines.
🧹 Nitpick comments (4)
Frends.HTTP.Request/Frends.HTTP.Request.Tests/UnitTests.cs (4)
22-22: Tests depend on external servicehttpbin.org—may cause flakiness.Using a live external endpoint introduces risks:
- Tests fail if
httpbin.orgis down or rate-limited- Tests fail in offline/air-gapped CI environments
- Network latency may cause intermittent timeouts
Consider using a local mock server (e.g., WireMock.Net) or Docker-based httpbin for reliable CI execution. As per coding guidelines: "Use mocking where real systems can't be simulated."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Frends.HTTP.Request/Frends.HTTP.Request.Tests/UnitTests.cs` at line 22, Tests in UnitTests.cs depend on the external BasePath constant pointing to https://httpbin.org which makes tests flaky and unsuitable for CI; replace this external dependency by introducing a local mock HTTP server (e.g., WireMock.Net or a Dockerized httpbin) and update tests that reference BasePath to use the mock server's URL (adjust BasePath and any setup/teardown in the UnitTests class or test fixture) so calls are intercepted locally and deterministic in offline/CI environments.
46-56: Test assertion relies on exact formatting from httpbin.The expected string
"\"args\": {\n \"id\": \"2\", \n \"userId\": \"1\"\n }"with specific whitespace/newlines is fragile. If httpbin changes its JSON formatting, this test breaks.Consider parsing the response as JSON and asserting on the parsed values instead.
♻️ Proposed fix using JSON parsing
- var expected = "\"args\": {\n \"id\": \"2\", \n \"userId\": \"1\"\n }"; var input = GetInputParams(url: $"{BasePath}/anything?id=2&userId=1"); ... var result = await HTTP.Request(input, options, CancellationToken.None); - ClassicAssert.IsTrue(result.Body.Contains(expected)); + var json = JObject.Parse(result.Body); + ClassicAssert.AreEqual("2", json["args"]["id"].ToString()); + ClassicAssert.AreEqual("1", json["args"]["userId"].ToString());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Frends.HTTP.Request/Frends.HTTP.Request.Tests/UnitTests.cs` around lines 46 - 56, The test in UnitTests.cs is asserting on a fragile exact-formatted substring of result.Body; instead parse result.Body as JSON (e.g., deserialize to a JObject or equivalent) and assert the values under the "args" object (check that args.id == "2" and args.userId == "1"). Update the test that calls HTTP.Request (the test using GetInputParams, Options and result.Body) to deserialize result.Body and use assertions against the parsed JSON properties rather than matching the whitespace-sensitive expected string.
8-9: Unusual namespace alias creates confusingMethod.Methodpattern.The alias
using Method = Frends.HTTP.Request.Definitions;causes usages likeMethod.Method.GETwhich is hard to read. Consider a clearer alias name.♻️ Proposed fix
-using Method = Frends.HTTP.Request.Definitions; +using Definitions = Frends.HTTP.Request.Definitions; ... - method: Method.Method.GET, + method: Definitions.Method.GET,Or simply use the enum directly with a
using static:using static Frends.HTTP.Request.Definitions.Method; // Then use: Method.GET becomes just GET🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Frends.HTTP.Request/Frends.HTTP.Request.Tests/UnitTests.cs` around lines 8 - 9, The current alias "using Method = Frends.HTTP.Request.Definitions;" creates awkward references like Method.Method.GET; change the import to a clearer name or use a static import of the enum to remove the double "Method" (for example replace the alias with a distinct identifier such as "using HTTPDefinitions = Frends.HTTP.Request.Definitions;" and reference HTTPDefinitions.Method.GET, or use "using static Frends.HTTP.Request.Definitions.Method" so tests can refer to GET directly); update all occurrences in Frends.HTTP.Request.Tests.UnitTests (references to Method.Method) to the chosen form.
288-289: Remove commented-out mock code.This leftover commented code serves no purpose and clutters the test file.
🧹 Proposed fix
- // _mockHttpMessageHandler.When(input.Url) - // .Respond("application/json", "<fail>failbar<fail>"); var ex = Assert.ThrowsAsync<JsonReaderException>(async () =>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Frends.HTTP.Request/Frends.HTTP.Request.Tests/UnitTests.cs` around lines 288 - 289, Remove the leftover commented-out mock lines that clutter the test file: delete the two commented lines referencing _mockHttpMessageHandler.When(input.Url).Respond(...) in Frends.HTTP.Request.Tests/UnitTests.cs so only active test setup remains (search for _mockHttpMessageHandler and input.Url in the UnitTests class to locate them).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Frends.HTTP.Request/Frends.HTTP.Request/Frends.HTTP.Request.csproj`:
- Line 5: The project file's <Version> element in Frends.HTTP.Request (the
Version tag in Frends.HTTP.Request.csproj) is set to 1.7.1 but the CHANGELOG.md
lists 1.8.0 as the latest; update the <Version> value to 1.8.0 to match the
changelog (or alternatively update CHANGELOG.md to reflect 1.7.1) so package
version and changelog are consistent.
In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs`:
- Around line 225-227: The HttpClientHandler, certificates and HttpClient are
currently static fields and must be converted to method-local variables to avoid
cross-request concurrency and disposal races; change the static fields
(httpClientHandler, certificates, httpClient) to locals inside the method that
creates the request, update the call to SetHandlerSettingsBasedOnOptions to
accept the local HttpClientHandler and certificates (e.g., pass the handler
instance and ref certificates into SetHandlerSettingsBasedOnOptions), and ensure
any other helper methods that previously relied on the static fields accept the
handler and certificates as parameters so each request uses its own handler and
cert collection.
- Line 70: Replace the incorrect ArgumentNullException usage for the URL check:
when validating input.Url in Request.cs (the check that currently throws new
ArgumentNullException("Url can not be empty.")) use the proper exception and
parameter name form—e.g., throw new ArgumentException("Url cannot be empty.",
nameof(input.Url))—so the exception receives the parameter name
(nameof(input.Url)) and a clear message.
- Around line 121-129: The finally block disposes the
HttpClient/httpClientHandler unconditionally which breaks cached clients
returned by GetHttpClientForOptions; change the client acquisition so
GetHttpClientForOptions (or its caller) indicates ownership (e.g., return or set
an ownsClient/ownsHandler flag or a tuple (HttpClient, HttpMessageHandler, bool
fromCache)), then in the finally block only call Dispose() on httpClient and
httpClientHandler when the flag shows they are newly created (not cached); keep
disposing request/response/httpContent and certificates as before but avoid
disposing cached instances.
- Around line 36-41: The static mutable fields httpContent, httpClient,
httpClientHandler, httpRequestMessage, httpResponseMessage and certificates
cause race conditions; remove these static declarations and declare them as
local variables inside the Request(...) method (e.g. HttpContent httpContent =
null; HttpClient httpClient = null; HttpClientHandler httpClientHandler = null;
HttpRequestMessage httpRequestMessage = null; HttpResponseMessage
httpResponseMessage = null; X509Certificate2[] certificates =
Array.Empty<X509Certificate2>()), ensure they are initialized where used, and
wrap usage in try/finally in Request to properly Dispose/cleanup each per-call
instance (alternatively use AsyncLocal<T> if per-async-flow state is required).
---
Outside diff comments:
In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs`:
- Around line 62-66: Add two properties to the Result class in
Frends.HTTP.Request/Frends.HTTP.Request/Definitions/Result.cs: a bool Success
and an Error object (class or property) containing string Message and object or
Dictionary<string, object> AdditionalInfo; then update the Request method
(Request in Request.cs) to set Result.Success (e.g., true for 2xx status codes)
and populate Result.Error when Success is false with a concise Message and any
useful AdditionalInfo (status code, response body, exception details) so the
task result conforms to the coding guidelines.
---
Nitpick comments:
In `@Frends.HTTP.Request/Frends.HTTP.Request.Tests/UnitTests.cs`:
- Line 22: Tests in UnitTests.cs depend on the external BasePath constant
pointing to https://httpbin.org which makes tests flaky and unsuitable for CI;
replace this external dependency by introducing a local mock HTTP server (e.g.,
WireMock.Net or a Dockerized httpbin) and update tests that reference BasePath
to use the mock server's URL (adjust BasePath and any setup/teardown in the
UnitTests class or test fixture) so calls are intercepted locally and
deterministic in offline/CI environments.
- Around line 46-56: The test in UnitTests.cs is asserting on a fragile
exact-formatted substring of result.Body; instead parse result.Body as JSON
(e.g., deserialize to a JObject or equivalent) and assert the values under the
"args" object (check that args.id == "2" and args.userId == "1"). Update the
test that calls HTTP.Request (the test using GetInputParams, Options and
result.Body) to deserialize result.Body and use assertions against the parsed
JSON properties rather than matching the whitespace-sensitive expected string.
- Around line 8-9: The current alias "using Method =
Frends.HTTP.Request.Definitions;" creates awkward references like
Method.Method.GET; change the import to a clearer name or use a static import of
the enum to remove the double "Method" (for example replace the alias with a
distinct identifier such as "using HTTPDefinitions =
Frends.HTTP.Request.Definitions;" and reference HTTPDefinitions.Method.GET, or
use "using static Frends.HTTP.Request.Definitions.Method" so tests can refer to
GET directly); update all occurrences in Frends.HTTP.Request.Tests.UnitTests
(references to Method.Method) to the chosen form.
- Around line 288-289: Remove the leftover commented-out mock lines that clutter
the test file: delete the two commented lines referencing
_mockHttpMessageHandler.When(input.Url).Respond(...) in
Frends.HTTP.Request.Tests/UnitTests.cs so only active test setup remains (search
for _mockHttpMessageHandler and input.Url in the UnitTests class to locate
them).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38efbd9f-f54f-4bb2-8de9-98fd76e16dea
📒 Files selected for processing (9)
Frends.HTTP.Request/CHANGELOG.mdFrends.HTTP.Request/Frends.HTTP.Request.Tests/Frends.HTTP.Request.Tests.csprojFrends.HTTP.Request/Frends.HTTP.Request.Tests/MockHttpClientFactory.csFrends.HTTP.Request/Frends.HTTP.Request.Tests/UnitTests.csFrends.HTTP.Request/Frends.HTTP.Request/Definitions/IHttpClientFactory.csFrends.HTTP.Request/Frends.HTTP.Request/Extensions.csFrends.HTTP.Request/Frends.HTTP.Request/Frends.HTTP.Request.csprojFrends.HTTP.Request/Frends.HTTP.Request/HttpClientFactory.csFrends.HTTP.Request/Frends.HTTP.Request/Request.cs
💤 Files with no reviewable changes (3)
- Frends.HTTP.Request/Frends.HTTP.Request.Tests/MockHttpClientFactory.cs
- Frends.HTTP.Request/Frends.HTTP.Request/HttpClientFactory.cs
- Frends.HTTP.Request/Frends.HTTP.Request/Definitions/IHttpClientFactory.cs
Frends.HTTP.Request/Frends.HTTP.Request/Frends.HTTP.Request.csproj
Outdated
Show resolved
Hide resolved
| private static HttpContent httpContent; | ||
| private static HttpClient httpClient; | ||
| private static HttpClientHandler httpClientHandler; | ||
| private static HttpRequestMessage httpRequestMessage; | ||
| private static HttpResponseMessage httpResponseMessage; | ||
| private static X509Certificate2[] certificates = Array.Empty<X509Certificate2>(); |
There was a problem hiding this comment.
Critical: Static mutable fields cause race conditions in concurrent scenarios.
These static fields (httpContent, httpClient, httpClientHandler, httpRequestMessage, httpResponseMessage, certificates) are shared across all concurrent calls to Request(). When multiple requests execute simultaneously, they will overwrite each other's state, leading to:
- Responses returned to wrong callers
- Premature disposal of another request's objects
- Corrupted request/response data
Either make these local variables within the Request method, or use proper thread-safe patterns (e.g., AsyncLocal<T>, or pass instances through the call chain).
🐛 Proposed fix: Use local variables instead of static fields
- private static HttpContent httpContent;
- private static HttpClient httpClient;
- private static HttpClientHandler httpClientHandler;
- private static HttpRequestMessage httpRequestMessage;
- private static HttpResponseMessage httpResponseMessage;
- private static X509Certificate2[] certificates = Array.Empty<X509Certificate2>();Then declare these as local variables inside the Request method:
public static async Task<Result> Request(...)
{
HttpContent httpContent = null;
HttpClient httpClient = null;
HttpClientHandler httpClientHandler = null;
HttpRequestMessage httpRequestMessage = null;
HttpResponseMessage httpResponseMessage = null;
X509Certificate2[] certificates = Array.Empty<X509Certificate2>();
try { ... }
finally { ... }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs` around lines 36 - 41, The
static mutable fields httpContent, httpClient, httpClientHandler,
httpRequestMessage, httpResponseMessage and certificates cause race conditions;
remove these static declarations and declare them as local variables inside the
Request(...) method (e.g. HttpContent httpContent = null; HttpClient httpClient
= null; HttpClientHandler httpClientHandler = null; HttpRequestMessage
httpRequestMessage = null; HttpResponseMessage httpResponseMessage = null;
X509Certificate2[] certificates = Array.Empty<X509Certificate2>()), ensure they
are initialized where used, and wrap usage in try/finally in Request to properly
Dispose/cleanup each per-call instance (alternatively use AsyncLocal<T> if
per-async-flow state is required).
| if (string.IsNullOrEmpty(input.Url)) throw new ArgumentNullException("Url can not be empty."); | ||
| try | ||
| { | ||
| if (string.IsNullOrEmpty(input.Url)) throw new ArgumentNullException("Url can not be empty."); |
There was a problem hiding this comment.
Fix ArgumentNullException usage as flagged by pipeline.
The pipeline warning CA2208 indicates incorrect usage. ArgumentNullException expects the parameter name as the first argument, not the message.
🔧 Proposed fix
- if (string.IsNullOrEmpty(input.Url)) throw new ArgumentNullException("Url can not be empty.");
+ if (string.IsNullOrEmpty(input.Url)) throw new ArgumentException("Url can not be empty.", nameof(input.Url));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (string.IsNullOrEmpty(input.Url)) throw new ArgumentNullException("Url can not be empty."); | |
| if (string.IsNullOrEmpty(input.Url)) throw new ArgumentException("Url can not be empty.", nameof(input.Url)); |
🧰 Tools
🪛 GitHub Actions: Request_push
[warning] 70-70: CA2208: Instantiate argument exceptions correctly. Method Request passes 'Url can not be empty.' as the paramName argument to ArgumentNullException; use a method parameter name with exact casing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs` at line 70, Replace the
incorrect ArgumentNullException usage for the URL check: when validating
input.Url in Request.cs (the check that currently throws new
ArgumentNullException("Url can not be empty.")) use the proper exception and
parameter name form—e.g., throw new ArgumentException("Url cannot be empty.",
nameof(input.Url))—so the exception receives the parameter name
(nameof(input.Url)) and a clear message.
| httpClientHandler = new HttpClientHandler(); | ||
| httpClientHandler.SetHandlerSettingsBasedOnOptions(options, ref certificates); | ||
| httpClient = new HttpClient(httpClientHandler); |
There was a problem hiding this comment.
Handler and certificates are also static fields—same concurrency issue.
httpClientHandler is assigned here but is a static field. In concurrent scenarios, one request could overwrite another's handler before it's added to the client, or dispose it while another request is using it.
This is related to the static fields issue flagged earlier. These should be local variables passed through the method chain.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs` around lines 225 - 227,
The HttpClientHandler, certificates and HttpClient are currently static fields
and must be converted to method-local variables to avoid cross-request
concurrency and disposal races; change the static fields (httpClientHandler,
certificates, httpClient) to locals inside the method that creates the request,
update the call to SetHandlerSettingsBasedOnOptions to accept the local
HttpClientHandler and certificates (e.g., pass the handler instance and ref
certificates into SetHandlerSettingsBasedOnOptions), and ensure any other helper
methods that previously relied on the static fields accept the handler and
certificates as parameters so each request uses its own handler and cert
collection.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
Frends.HTTP.Request/Frends.HTTP.Request/Request.cs (2)
72-72:⚠️ Potential issue | 🔴 CriticalCached
HttpClientinstances are still disposed unconditionally.When
CacheHttpClientis enabled,GetHttpClientForOptions()can return a shared client from cache, but thefinallyblock always disposeshttpClientandhttpClientHandler. The next request for the same cache key can therefore retrieve a disposed client.Also applies to: 121-125, 216-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs` at line 72, The finally block unconditionally disposes httpClient and httpClientHandler even when GetHttpClientForOptions(options) returned a cached/shared instance due to CacheHttpClient being true; update the cleanup logic in the method(s) using GetHttpClientForOptions (and the other affected blocks at the same pattern) to only dispose httpClient and httpClientHandler when they are not shared—i.e., detect whether the returned HttpClient/HttpClientHandler came from cache (use the cache-key logic or a flag from GetHttpClientForOptions) and skip disposal for cached instances, disposing only newly created/non-cached instances.
36-41:⚠️ Potential issue | 🔴 CriticalStill using mutable statics for per-request state.
These fields are assigned during every
Request()call and then disposed infinally, so concurrent executions can overwrite or dispose each other’s request, response, handler, and certificate state.Also applies to: 72-76, 121-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs` around lines 36 - 41, The static mutable fields (httpContent, httpClientHandler, httpRequestMessage, httpResponseMessage, certificates — and if httpClient is currently reassigned per call) cause race conditions; make these per-request locals instead of statics: move declarations of httpContent, httpClientHandler, httpRequestMessage, httpResponseMessage and certificates into the Request(...) method (or into a short-lived request-scoped object) so each invocation uses its own instances and dispose those locals in the existing finally block; if httpClient is intended to be shared, make it a true static singleton that is never disposed per request, otherwise also convert httpClient to a request-local variable. Ensure all references in code that currently use the static symbols (httpContent, httpClientHandler, httpRequestMessage, httpResponseMessage, certificates, httpClient) are updated to use the new local variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Frends.HTTP.Request/CHANGELOG.md`:
- Around line 9-11: Update the CHANGELOG entry for CacheHttpClient to a clear
user-facing sentence that follows "Keep a Changelog" style: state the change,
the default behavior, and how to opt-out — e.g. "Added CacheHttpClient option to
disable HttpClient caching; caching remains enabled by default." Replace the
ambiguous line about caching being "there were cached by default" with this
single concise sentence mentioning the option name (CacheHttpClient) and
HttpClient so users understand the visible behavior change.
In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs`:
- Around line 181-189: The early-return logic that creates and returns
StringContent(string.Empty) for methods not in the methodsWithBody array must be
removed because it discards input.Message for GET and other methods; update the
code in Request.cs so that you no longer return empty content for
non-POST/PUT/PATCH/DELETE methods (remove the
methodsWithBody.Contains(input.Method) check and the early return), and instead
construct StringContent from input.Message when any body is present (e.g., use
new StringContent(input.Message ?? string.Empty) or only when input.Message is
non-null) so GET and other methods preserve provided message content; keep the
methodsWithBody array only if needed elsewhere or remove it if unused.
- Around line 241-248: Replace the current plain-text concatenation in
GetHttpClientCacheKey with a deterministic, normalized representation of the
client-level options (same fields currently used except options.Token) and
return a secure hash (e.g. SHA-256 hex or base64) of that representation instead
of embedding raw secrets; ensure you include the same identifying fields
(Authentication, Username, Password, ClientCertificateSource,
ClientCertificateFilePath, ClientCertificateInBase64,
ClientCertificateKeyPhrase, CertificateThumbprint,
LoadEntireChainForCertificate, ConnectionTimeoutSeconds, FollowRedirects,
AllowInvalidCertificate, AllowInvalidResponseContentTypeCharSet,
ThrowExceptionOnErrorResponse, AutomaticCookieHandling) in the normalized input
but never store or return the plaintext sensitive values (Password,
ClientCertificateInBase64, ClientCertificateKeyPhrase) — serialize them
deterministically (e.g. JSON with fixed ordering) then compute and return the
hash string from GetHttpClientCacheKey.
- Around line 100-103: The JToken return branch (ReturnFormat.JToken) can NRE
when responseMessage.Content is null; update the branch around
TryParseRequestStringResultAsJToken to first guard with responseMessage.Content
!= null before calling ReadAsStringAsync (use a conditional to pass null or
empty string to TryParseRequestStringResultAsJToken when Content is null) and
replace any direct Content.Headers access with responseMessage.Content?.Headers
to mirror the defensive pattern used in the String branch.
---
Duplicate comments:
In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs`:
- Line 72: The finally block unconditionally disposes httpClient and
httpClientHandler even when GetHttpClientForOptions(options) returned a
cached/shared instance due to CacheHttpClient being true; update the cleanup
logic in the method(s) using GetHttpClientForOptions (and the other affected
blocks at the same pattern) to only dispose httpClient and httpClientHandler
when they are not shared—i.e., detect whether the returned
HttpClient/HttpClientHandler came from cache (use the cache-key logic or a flag
from GetHttpClientForOptions) and skip disposal for cached instances, disposing
only newly created/non-cached instances.
- Around line 36-41: The static mutable fields (httpContent, httpClientHandler,
httpRequestMessage, httpResponseMessage, certificates — and if httpClient is
currently reassigned per call) cause race conditions; make these per-request
locals instead of statics: move declarations of httpContent, httpClientHandler,
httpRequestMessage, httpResponseMessage and certificates into the Request(...)
method (or into a short-lived request-scoped object) so each invocation uses its
own instances and dispose those locals in the existing finally block; if
httpClient is intended to be shared, make it a true static singleton that is
never disposed per request, otherwise also convert httpClient to a request-local
variable. Ensure all references in code that currently use the static symbols
(httpContent, httpClientHandler, httpRequestMessage, httpResponseMessage,
certificates, httpClient) are updated to use the new local variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6350fc3a-7c0c-4820-9da0-ec93ad20d892
📒 Files selected for processing (4)
Frends.HTTP.Request/CHANGELOG.mdFrends.HTTP.Request/Frends.HTTP.Request/Definitions/Options.csFrends.HTTP.Request/Frends.HTTP.Request/Frends.HTTP.Request.csprojFrends.HTTP.Request/Frends.HTTP.Request/Request.cs
✅ Files skipped from review due to trivial changes (1)
- Frends.HTTP.Request/Frends.HTTP.Request/Frends.HTTP.Request.csproj
| case ReturnFormat.JToken: | ||
| var rbody = TryParseRequestStringResultAsJToken(await responseMessage.Content | ||
| .ReadAsStringAsync(cancellationToken) | ||
| .ConfigureAwait(false)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd Frends.HTTP.Request/Frends.HTTP.Request && wc -l Request.csRepository: FrendsPlatform/Frends.HTTP
Length of output: 84
🏁 Script executed:
cd Frends.HTTP.Request/Frends.HTTP.Request && sed -n '90,115p' Request.csRepository: FrendsPlatform/Frends.HTTP
Length of output: 1438
🏁 Script executed:
cd Frends.HTTP.Request/Frends.HTTP.Request && grep -n "ReturnFormat.String" Request.cs | head -5Repository: FrendsPlatform/Frends.HTTP
Length of output: 114
🏁 Script executed:
cd Frends.HTTP.Request/Frends.HTTP.Request && rg -n "TryParseRequestStringResultAsJToken" Request.cs -A 10Repository: FrendsPlatform/Frends.HTTP
Length of output: 1141
🏁 Script executed:
cd Frends.HTTP.Request/Frends.HTTP.Request && rg -n "204|NoContent|HEAD" Request.csRepository: FrendsPlatform/Frends.HTTP
Length of output: 52
🏁 Script executed:
cd Frends.HTTP.Request/Frends.HTTP.Request && grep -n "responseMessage.Content" Request.csRepository: FrendsPlatform/Frends.HTTP
Length of output: 586
🌐 Web query:
HttpResponseMessage.Content null 204 No Content HEAD request .NET
💡 Result:
For HTTP HEAD and status 204 No Content, the HTTP spec says the response must not include content (no message body), so you should treat it as “no payload” even if the request succeeded. In particular, 204 responses must not carry a Content-Length, and HEAD responses never include content. [1]
In .NET HttpClient, HttpResponseMessage.Content is usually not null (it’s commonly an “empty content” object), and reading it yields an empty string/stream. That’s why many people can’t reproduce Content == null even for HEAD and 204. [2]
If you’re seeing Content == null, it’s typically because the HttpResponseMessage was constructed by your code/test mock (or a custom handler) with Content = null, not because a real HttpClient response for 204/HEAD “has a body”. [3]
Practical guidance:
- If
response.StatusCode == HttpStatusCode.NoContentorrequest.Method == HttpMethod.Head, don’t attempt to read a body. - Otherwise, read normally; if your code/mocks might set
Content = null, guard withresponse.Content == null(or?.) before dereferencing headers/body.
Sources: [1] RFC 9110 (HTTP Semantics), [2] Stack Overflow: “Can HttpClient’s response Content be null?”, [3] Stack Overflow: “Unexpected null behavior with HttpResponseMessage.Content…”
🏁 Script executed:
cd Frends.HTTP.Request && find . -name "*test*" -o -name "*Test*" | head -10Repository: FrendsPlatform/Frends.HTTP
Length of output: 199
🏁 Script executed:
cd Frends.HTTP.Request && rg -l "204|NoContent|HEAD" --type csRepository: FrendsPlatform/Frends.HTTP
Length of output: 111
🏁 Script executed:
cd Frends.HTTP.Request.Tests && cat -n UnitTests.cs | head -100Repository: FrendsPlatform/Frends.HTTP
Length of output: 145
🏁 Script executed:
cd Frends.HTTP.Request.Tests && rg -n "204|NoContent|HEAD" UnitTests.csRepository: FrendsPlatform/Frends.HTTP
Length of output: 145
🏁 Script executed:
cd Frends.HTTP.Request.Tests && grep -n "JToken\|ReturnFormat" UnitTests.cs | head -20Repository: FrendsPlatform/Frends.HTTP
Length of output: 145
🏁 Script executed:
find . -name "UnitTests.cs" -o -name "*Test*.cs" | grep -i testRepository: FrendsPlatform/Frends.HTTP
Length of output: 422
🏁 Script executed:
git ls-files | grep -i test | head -20Repository: FrendsPlatform/Frends.HTTP
Length of output: 1421
🏁 Script executed:
git ls-files "*/Tests/*" | head -20Repository: FrendsPlatform/Frends.HTTP
Length of output: 52
🏁 Script executed:
cat -n "./Frends.HTTP.Request/Frends.HTTP.Request.Tests/UnitTests.cs" | head -150Repository: FrendsPlatform/Frends.HTTP
Length of output: 5432
🏁 Script executed:
rg -n "204|NoContent|HEAD" "./Frends.HTTP.Request/Frends.HTTP.Request.Tests/UnitTests.cs"Repository: FrendsPlatform/Frends.HTTP
Length of output: 52
🏁 Script executed:
rg -n "JToken|ReturnFormat" "./Frends.HTTP.Request/Frends.HTTP.Request.Tests/UnitTests.cs" | head -30Repository: FrendsPlatform/Frends.HTTP
Length of output: 546
🏁 Script executed:
cat -n "./Frends.HTTP.Request/Frends.HTTP.Request.Tests/UnitTests.cs" | sed -n '200,250p'Repository: FrendsPlatform/Frends.HTTP
Length of output: 2055
🏁 Script executed:
rg -n "empty|Empty|null|Null|Content" "./Frends.HTTP.Request/Frends.HTTP.Request.Tests/UnitTests.cs" | head -30Repository: FrendsPlatform/Frends.HTTP
Length of output: 864
🏁 Script executed:
cat -n "./Frends.HTTP.Request/Frends.HTTP.Request.Tests/Helper.cs"Repository: FrendsPlatform/Frends.HTTP
Length of output: 1834
🏁 Script executed:
cat -n "./Frends.HTTP.Request/Frends.HTTP.Request.Tests/UnitTests.cs" | sed -n '248,280p'Repository: FrendsPlatform/Frends.HTTP
Length of output: 1271
🏁 Script executed:
cat -n "./Frends.HTTP.Request/Frends.HTTP.Request/Request.cs" | sed -n '198,215p'Repository: FrendsPlatform/Frends.HTTP
Length of output: 749
🏁 Script executed:
cat -n "./Frends.HTTP.Request/Frends.HTTP.Request/Request.cs" | sed -n '80,120p'Repository: FrendsPlatform/Frends.HTTP
Length of output: 2174
Guard the JToken path against null Content.
The String branch defensively checks responseMessage.Content != null and uses the null-conditional operator ?. when accessing headers. The JToken branch lacks both safeguards, risking a NullReferenceException on lines 101–102 and 106 if Content is null (e.g., from a custom HttpMessageHandler or mock).
Make both branches consistent by applying the same defensive pattern:
- Check
Content != nullbefore callingReadAsStringAsync() - Use
Content?.Headersinstead ofContent.Headers
Suggested approach
case ReturnFormat.JToken:
- var rbody = TryParseRequestStringResultAsJToken(await responseMessage.Content
- .ReadAsStringAsync(cancellationToken)
- .ConfigureAwait(false));
+ var rbody = TryParseRequestStringResultAsJToken(await (responseMessage.Content != null
+ ? responseMessage.Content.ReadAsStringAsync(cancellationToken)
+ : Task.FromResult(string.Empty))
+ .ConfigureAwait(false));
- var rheaders =
- GetResponseHeaderDictionary(responseMessage.Headers, responseMessage.Content.Headers);
+ var rheaders =
+ GetResponseHeaderDictionary(responseMessage.Headers, responseMessage.Content?.Headers);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case ReturnFormat.JToken: | |
| var rbody = TryParseRequestStringResultAsJToken(await responseMessage.Content | |
| .ReadAsStringAsync(cancellationToken) | |
| .ConfigureAwait(false)); | |
| case ReturnFormat.JToken: | |
| var rbody = TryParseRequestStringResultAsJToken(await (responseMessage.Content != null | |
| ? responseMessage.Content.ReadAsStringAsync(cancellationToken) | |
| : Task.FromResult(string.Empty)) | |
| .ConfigureAwait(false)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs` around lines 100 - 103,
The JToken return branch (ReturnFormat.JToken) can NRE when
responseMessage.Content is null; update the branch around
TryParseRequestStringResultAsJToken to first guard with responseMessage.Content
!= null before calling ReadAsStringAsync (use a conditional to pass null or
empty string to TryParseRequestStringResultAsJToken when Content is null) and
replace any direct Content.Headers access with responseMessage.Content?.Headers
to mirror the defensive pattern used in the String branch.
| var methodsWithBody = new[] | ||
| { | ||
| Method.POST, Method.PUT, Method.PATCH, Method.DELETE | ||
| }; | ||
|
|
||
| if (!methodsWithBody.Contains(input.Method)) | ||
| { | ||
| return new StringContent(string.Empty); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the structure of the repository
git ls-files | grep -E "(Frends|README|CHANGELOG|changelog)" | head -20Repository: FrendsPlatform/Frends.HTTP
Length of output: 1417
🏁 Script executed:
# Find and read the relevant file to understand context
fd "Request.cs" -t f | grep -i "Frends.HTTP.Request"Repository: FrendsPlatform/Frends.HTTP
Length of output: 187
🏁 Script executed:
# Search for changelog files
fd -i "changelog\|release\|changes" -t fRepository: FrendsPlatform/Frends.HTTP
Length of output: 52
🏁 Script executed:
# Check the size of Request.cs
wc -l Frends.HTTP.Request/Frends.HTTP.Request/Request.csRepository: FrendsPlatform/Frends.HTTP
Length of output: 124
🏁 Script executed:
# Read the relevant section of Request.cs with context
sed -n '160,210p' Frends.HTTP.Request/Frends.HTTP.Request/Request.csRepository: FrendsPlatform/Frends.HTTP
Length of output: 1772
🏁 Script executed:
# Also read the beginning to understand the function signature
sed -n '1,50p' Frends.HTTP.Request/Frends.HTTP.Request/Request.csRepository: FrendsPlatform/Frends.HTTP
Length of output: 1523
🏁 Script executed:
# Check the changelog to see what behavior change was documented
cat Frends.HTTP.Request/CHANGELOG.mdRepository: FrendsPlatform/Frends.HTTP
Length of output: 2133
Remove the early return that silently discards request bodies for GET and other non-standard HTTP methods.
The code returns StringContent(string.Empty) for methods outside POST/PUT/PATCH/DELETE, which discards input.Message. This contradicts the changelog: v1.6.0 explicitly fixed "GET requests ignore message body content," and v1.2.0 removed restrictions on GET having content since HttpClient now tolerates it. The current code reintroduces this bug by silently discarding the message body for GET requests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs` around lines 181 - 189,
The early-return logic that creates and returns StringContent(string.Empty) for
methods not in the methodsWithBody array must be removed because it discards
input.Message for GET and other methods; update the code in Request.cs so that
you no longer return empty content for non-POST/PUT/PATCH/DELETE methods (remove
the methodsWithBody.Contains(input.Method) check and the early return), and
instead construct StringContent from input.Message when any body is present
(e.g., use new StringContent(input.Message ?? string.Empty) or only when
input.Message is non-null) so GET and other methods preserve provided message
content; keep the methodsWithBody array only if needed elsewhere or remove it if
unused.
| private static string GetHttpClientCacheKey(Options options) | ||
| { | ||
| // Includes everything except for options.Token, which is used on request level, not http client level | ||
| return $"{options.Authentication}:{options.Username}:{options.Password}:{options.ClientCertificateSource}" | ||
| + $":{options.ClientCertificateFilePath}:{options.ClientCertificateInBase64}:{options.ClientCertificateKeyPhrase}" | ||
| + $":{options.CertificateThumbprint}:{options.LoadEntireChainForCertificate}:{options.ConnectionTimeoutSeconds}" | ||
| + $":{options.FollowRedirects}:{options.AllowInvalidCertificate}:{options.AllowInvalidResponseContentTypeCharSet}" | ||
| + $":{options.ThrowExceptionOnErrorResponse}:{options.AutomaticCookieHandling}"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -i "request.cs"Repository: FrendsPlatform/Frends.HTTP
Length of output: 187
🏁 Script executed:
find . -type f -name "Request.cs" | head -20Repository: FrendsPlatform/Frends.HTTP
Length of output: 122
🏁 Script executed:
cat -n Frends.HTTP.Request/Frends.HTTP.Request/Request.cs | head -100Repository: FrendsPlatform/Frends.HTTP
Length of output: 4207
🏁 Script executed:
cat -n Frends.HTTP.Request/Frends.HTTP.Request/Request.cs | sed -n '230,260p'Repository: FrendsPlatform/Frends.HTTP
Length of output: 1951
🏁 Script executed:
find . -type f -name "*.cs" | xargs rg "class Options" | head -5Repository: FrendsPlatform/Frends.HTTP
Length of output: 547
🏁 Script executed:
find . -type f -name "*.cs" -path "*/Frends.HTTP.Request/*" | xargs rg -l "class Options"Repository: FrendsPlatform/Frends.HTTP
Length of output: 134
🏁 Script executed:
rg "class Options" Frends.HTTP.Request/Frends.HTTP.Request/ -A 50Repository: FrendsPlatform/Frends.HTTP
Length of output: 5412
🏁 Script executed:
cat -n Frends.HTTP.Request/Frends.HTTP.Request/Request.cs | sed -n '210,240p'Repository: FrendsPlatform/Frends.HTTP
Length of output: 1207
🏁 Script executed:
rg "GetHttpClientCacheKey|GetHttpClientForOptions" Frends.HTTP.Request/Frends.HTTP.Request/Request.cs -B 3 -A 8Repository: FrendsPlatform/Frends.HTTP
Length of output: 1835
🏁 Script executed:
cat -n Frends.HTTP.Request/Frends.HTTP.Request/Definitions/Options.cs | sed -n '1,100p'Repository: FrendsPlatform/Frends.HTTP
Length of output: 4988
🏁 Script executed:
rg "ClientCertificateInBase64|ClientCertificateKeyPhrase" Frends.HTTP.Request/Frends.HTTP.Request/Definitions/Options.cs -B 2 -A 3Repository: FrendsPlatform/Frends.HTTP
Length of output: 585
🏁 Script executed:
rg "GetHttpClientCacheKey|CacheHttpClient" Frends.HTTP.Request/Frends.HTTP.Request.Tests/ -B 2 -A 5Repository: FrendsPlatform/Frends.HTTP
Length of output: 52
🏁 Script executed:
find . -type f -name "*.cs" -path "*/Frends.HTTP.Request.Tests/*" | head -10Repository: FrendsPlatform/Frends.HTTP
Length of output: 188
Hash the cache key instead of embedding raw secrets.
The cache key concatenates sensitive values including Password, ClientCertificateInBase64, and ClientCertificateKeyPhrase directly. These are marked as credentials and can expose sensitive material in cache metadata. Additionally, inlining certificate bytes creates unnecessarily large keys. Use a hash of the normalized configuration instead of concatenating raw secret values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs` around lines 241 - 248,
Replace the current plain-text concatenation in GetHttpClientCacheKey with a
deterministic, normalized representation of the client-level options (same
fields currently used except options.Token) and return a secure hash (e.g.
SHA-256 hex or base64) of that representation instead of embedding raw secrets;
ensure you include the same identifying fields (Authentication, Username,
Password, ClientCertificateSource, ClientCertificateFilePath,
ClientCertificateInBase64, ClientCertificateKeyPhrase, CertificateThumbprint,
LoadEntireChainForCertificate, ConnectionTimeoutSeconds, FollowRedirects,
AllowInvalidCertificate, AllowInvalidResponseContentTypeCharSet,
ThrowExceptionOnErrorResponse, AutomaticCookieHandling) in the normalized input
but never store or return the plaintext sensitive values (Password,
ClientCertificateInBase64, ClientCertificateKeyPhrase) — serialize them
deterministically (e.g. JSON with fixed ordering) then compute and return the
hash string from GetHttpClientCacheKey.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
Frends.HTTP.Request/Frends.HTTP.Request/Request.cs (5)
36-41:⚠️ Potential issue | 🔴 CriticalStatic mutable request state is not thread-safe.
Lines 36–41 store per-request objects in static fields, so concurrent
Request(...)calls can overwrite/dispose each other’s state.Fix direction
- private static HttpContent httpContent; - private static HttpClient httpClient; - private static HttpClientHandler httpClientHandler; - private static HttpRequestMessage httpRequestMessage; - private static HttpResponseMessage httpResponseMessage; - private static X509Certificate2[] certificates = Array.Empty<X509Certificate2>(); + // Keep only cache-related static state here. + // Move request-scoped objects to locals inside Request(...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs` around lines 36 - 41, The static mutable fields (httpContent, httpClient, httpClientHandler, httpRequestMessage, httpResponseMessage, certificates) cause race conditions for concurrent Request(...) calls; make these per-request (non-static) by moving them into the Request method scope or into a request-specific instance so each invocation has its own HttpContent, HttpRequestMessage, HttpResponseMessage and handler/client state, and ensure certificates is created/copied per request rather than a shared static array; update any usages in methods like Request to reference the new local/instance variables and remove the static fields.
70-70:⚠️ Potential issue | 🟡 MinorUse correct argument exception types and guard null inputs.
Line 70 uses
ArgumentNullExceptionwith a message string asparamName, andinput/optionsare not null-checked before use.Proposed fix
+ if (input is null) throw new ArgumentNullException(nameof(input)); + if (options is null) throw new ArgumentNullException(nameof(options)); - if (string.IsNullOrEmpty(input.Url)) throw new ArgumentNullException("Url can not be empty."); + if (string.IsNullOrWhiteSpace(input.Url)) + throw new ArgumentException("Url cannot be empty.", nameof(input.Url));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs` at line 70, Guard against null inputs by first checking if input is null and throwing ArgumentNullException(nameof(input)); then validate the URL string by checking string.IsNullOrEmpty(input.Url) and throw ArgumentException("Url cannot be empty.", nameof(input.Url)); also null-check any nested properties referenced (e.g., input.Options) and throw ArgumentNullException(nameof(input.Options)) or ArgumentException as appropriate; use nameof(...) for parameter names rather than embedding messages as the paramName.
189-192:⚠️ Potential issue | 🟠 MajorNon-body methods currently discard provided payloads.
Lines 189–192 force
StringContent(string.Empty)for methods outside POST/PUT/PATCH/DELETE, which dropsinput.Messagefor GET and others.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs` around lines 189 - 192, The code currently returns new StringContent(string.Empty) for methods not in methodsWithBody, which discards any provided payload; update the logic in the method that builds request content (referencing methodsWithBody, input.Method and StringContent) to return the actual payload when present (e.g., new StringContent(input.Message ?? string.Empty), preserving content-type/encoding handling if present) instead of always returning an empty StringContent so input.Message is not lost for GET/other methods.
100-106:⚠️ Potential issue | 🟠 Major
JTokenbranch can null-derefContent.Lines 101–106 assume
responseMessage.Contentis non-null, unlike theStringbranch.Proposed fix
- var rbody = TryParseRequestStringResultAsJToken(await responseMessage.Content - .ReadAsStringAsync(cancellationToken) - .ConfigureAwait(false)); + var rbody = TryParseRequestStringResultAsJToken( + responseMessage.Content != null + ? await responseMessage.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false) + : string.Empty); var rstatusCode = (int)responseMessage.StatusCode; var rheaders = - GetResponseHeaderDictionary(responseMessage.Headers, responseMessage.Content.Headers); + GetResponseHeaderDictionary(responseMessage.Headers, responseMessage.Content?.Headers);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs` around lines 100 - 106, The JToken branch assumes responseMessage.Content is non-null and can NRE; update the ReturnFormat.JToken handling (the block that assigns rbody, rstatusCode and rheaders and calls TryParseRequestStringResultAsJToken and GetResponseHeaderDictionary) to mirror the String branch: check responseMessage.Content for null before calling ReadAsStringAsync/TryParseRequestStringResultAsJToken and when building rheaders, and if Content is null produce the same empty/null body and header values the String branch produces (or a JToken null equivalent) so no dereference occurs.
247-251:⚠️ Potential issue | 🟠 MajorCache key embeds plaintext secrets.
Lines 247–251 include
Password, certificate base64, and key phrase directly in the cache key. This leaks sensitive material and inflates key size.Proposed fix direction
- return $"{options.Authentication}:{options.Username}:{options.Password}:{options.ClientCertificateSource}" - + $":{options.ClientCertificateFilePath}:{options.ClientCertificateInBase64}:{options.ClientCertificateKeyPhrase}" - + $":{options.CertificateThumbprint}:{options.LoadEntireChainForCertificate}:{options.ConnectionTimeoutSeconds}" - + $":{options.FollowRedirects}:{options.AllowInvalidCertificate}:{options.AllowInvalidResponseContentTypeCharSet}" - + $":{options.ThrowExceptionOnErrorResponse}:{options.AutomaticCookieHandling}"; + // Build deterministic normalized payload, then SHA-256 hash it. + // Do not store plaintext secrets in key material.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs` around lines 247 - 251, The current cache key construction in Request.cs returns a string that includes plaintext secrets (options.Password, options.ClientCertificateInBase64, options.ClientCertificateKeyPhrase) which leaks sensitive data; change the cache key logic used by the method that returns this formatted string to exclude raw secret values and instead incorporate a stable non-reversible identifier (e.g. SHA-256 or HMAC of each secret or a single combined secret hash) or omit them entirely; update references to options.Password, options.ClientCertificateInBase64, and options.ClientCertificateKeyPhrase so the returned value contains only non-sensitive fields plus hashed/trimmed placeholders, preserving key uniqueness without exposing secrets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs`:
- Around line 121-131: The finally block currently guards disposal of
request-scoped objects behind options.CacheHttpClient, causing httpContent,
httpRequestMessage, httpResponseMessage (and certificates) to be leaked when
CacheHttpClient is true; change the logic so that httpContent,
httpRequestMessage, httpResponseMessage and foreach cert dispose always run,
while only httpClient and httpClientHandler disposal remain conditional on
options.CacheHttpClient being false; update the finally block around those
symbols (httpContent, httpRequestMessage, httpResponseMessage, certificates,
httpClient, httpClientHandler) to reflect this separation.
---
Duplicate comments:
In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs`:
- Around line 36-41: The static mutable fields (httpContent, httpClient,
httpClientHandler, httpRequestMessage, httpResponseMessage, certificates) cause
race conditions for concurrent Request(...) calls; make these per-request
(non-static) by moving them into the Request method scope or into a
request-specific instance so each invocation has its own HttpContent,
HttpRequestMessage, HttpResponseMessage and handler/client state, and ensure
certificates is created/copied per request rather than a shared static array;
update any usages in methods like Request to reference the new local/instance
variables and remove the static fields.
- Line 70: Guard against null inputs by first checking if input is null and
throwing ArgumentNullException(nameof(input)); then validate the URL string by
checking string.IsNullOrEmpty(input.Url) and throw ArgumentException("Url cannot
be empty.", nameof(input.Url)); also null-check any nested properties referenced
(e.g., input.Options) and throw ArgumentNullException(nameof(input.Options)) or
ArgumentException as appropriate; use nameof(...) for parameter names rather
than embedding messages as the paramName.
- Around line 189-192: The code currently returns new
StringContent(string.Empty) for methods not in methodsWithBody, which discards
any provided payload; update the logic in the method that builds request content
(referencing methodsWithBody, input.Method and StringContent) to return the
actual payload when present (e.g., new StringContent(input.Message ??
string.Empty), preserving content-type/encoding handling if present) instead of
always returning an empty StringContent so input.Message is not lost for
GET/other methods.
- Around line 100-106: The JToken branch assumes responseMessage.Content is
non-null and can NRE; update the ReturnFormat.JToken handling (the block that
assigns rbody, rstatusCode and rheaders and calls
TryParseRequestStringResultAsJToken and GetResponseHeaderDictionary) to mirror
the String branch: check responseMessage.Content for null before calling
ReadAsStringAsync/TryParseRequestStringResultAsJToken and when building
rheaders, and if Content is null produce the same empty/null body and header
values the String branch produces (or a JToken null equivalent) so no
dereference occurs.
- Around line 247-251: The current cache key construction in Request.cs returns
a string that includes plaintext secrets (options.Password,
options.ClientCertificateInBase64, options.ClientCertificateKeyPhrase) which
leaks sensitive data; change the cache key logic used by the method that returns
this formatted string to exclude raw secret values and instead incorporate a
stable non-reversible identifier (e.g. SHA-256 or HMAC of each secret or a
single combined secret hash) or omit them entirely; update references to
options.Password, options.ClientCertificateInBase64, and
options.ClientCertificateKeyPhrase so the returned value contains only
non-sensitive fields plus hashed/trimmed placeholders, preserving key uniqueness
without exposing secrets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a45eb60e-8f21-4d47-bc70-9d66efea6eba
📒 Files selected for processing (1)
Frends.HTTP.Request/Frends.HTTP.Request/Request.cs
| finally | ||
| { | ||
| if (!options.CacheHttpClient) | ||
| { | ||
| httpContent?.Dispose(); | ||
| httpClient?.Dispose(); | ||
| httpClientHandler?.Dispose(); | ||
| httpRequestMessage?.Dispose(); | ||
| httpResponseMessage?.Dispose(); | ||
| foreach (var cert in certificates) cert?.Dispose(); | ||
| } |
There was a problem hiding this comment.
Caching toggle currently suppresses disposal of request-scoped objects.
With CacheHttpClient = true, Lines 125/128/129 no longer dispose HttpContent/HttpRequestMessage/HttpResponseMessage. Only client/handler disposal should be conditional.
Proposed fix
finally
{
- if (!options.CacheHttpClient)
- {
- httpContent?.Dispose();
- httpClient?.Dispose();
- httpClientHandler?.Dispose();
- httpRequestMessage?.Dispose();
- httpResponseMessage?.Dispose();
- foreach (var cert in certificates) cert?.Dispose();
- }
+ httpContent?.Dispose();
+ httpRequestMessage?.Dispose();
+ httpResponseMessage?.Dispose();
+
+ if (!options.CacheHttpClient)
+ {
+ httpClient?.Dispose();
+ httpClientHandler?.Dispose();
+ foreach (var cert in certificates) cert?.Dispose();
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Frends.HTTP.Request/Frends.HTTP.Request/Request.cs` around lines 121 - 131,
The finally block currently guards disposal of request-scoped objects behind
options.CacheHttpClient, causing httpContent, httpRequestMessage,
httpResponseMessage (and certificates) to be leaked when CacheHttpClient is
true; change the logic so that httpContent, httpRequestMessage,
httpResponseMessage and foreach cert dispose always run, while only httpClient
and httpClientHandler disposal remain conditional on options.CacheHttpClient
being false; update the finally block around those symbols (httpContent,
httpRequestMessage, httpResponseMessage, certificates, httpClient,
httpClientHandler) to reflect this separation.
There was a problem hiding this comment.
@MatteoDelOmbra agree with coderabbit, only client and handler disposal should be conditional
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
agree, but also left certs conditional, as they are needed for httphandler
| finally | ||
| { | ||
| if (!options.CacheHttpClient) | ||
| { | ||
| httpContent?.Dispose(); | ||
| httpClient?.Dispose(); | ||
| httpClientHandler?.Dispose(); | ||
| httpRequestMessage?.Dispose(); | ||
| httpResponseMessage?.Dispose(); | ||
| foreach (var cert in certificates) cert?.Dispose(); | ||
| } |
There was a problem hiding this comment.
@MatteoDelOmbra agree with coderabbit, only client and handler disposal should be conditional
2 participants
Dear PR creator, please select one of the PR templates, then remove others and this text.
Default PR template
Please review my changes :)
Task Update PR template
Review Checklist
Task Harmonization PR template
Review Checklist
1. Frends Task Project File
Frends.*/Frends.*/*.csproj<TargetFramework>net8.0</TargetFramework><Version>x.0.0</Version><Authors>Frends</Authors><PackageLicenseExpression>MIT</PackageLicenseExpression><GenerateDocumentationFile>true</GenerateDocumentationFile><Description><RepositoryUrl>https://github.com/FrendsPlatform/Frends.SYSTEM/tree/main/Frends.SYSTEM.ACTION</RepositoryUrl><Nullable>disable</Nullable>StyleCop.Analyzers v1.2.0-beta.556FrendsTaskAnalyzers v1.*<Content Include="migration.json" PackagePath="/" Pack="true"/><Content Include="../CHANGELOG.md" PackagePath="/" Pack="true"/><AdditionalFiles Include="FrendsTaskMetadata.json" PackagePath="/" Pack="true"/>2. Frends Task Test Project File
Frends.*/Frends.*.Tests/*.Tests.csproj<TargetFramework>net8.0</TargetFramework><IsPackable>false</IsPackable><Nullable>disable</Nullable>StyleCop.Analyzers v1.2.0-beta.5563. Additional Files
LICENSEfile per repository.gitignorefile per repository.idea/foldersFrends.*/README.mdFrends.*/CHANGELOG.mdFrends.*/Frends.*/FrendsTaskMetadata.jsonFrends.System.Action.System.ActionFrends.*/Frends.*/migration.jsonFrends.*/Frends.*/GlobalSuppressions.csFrends.*/Frends.*.Tests/GlobalSuppressions.cs4. Source Code
5. GitHub Actions Workflows
.github/workflows/*.yml*_release.ymlfeed_api_key: ${{ secrets.TASKS_FEED_API_KEY }}*_test_on_main.ymlbadge_service_api_key: ${{ secrets.BADGE_SERVICE_API_KEY }}*_test_on_push.ymlbadge_service_api_key: ${{ secrets.BADGE_SERVICE_API_KEY }}test_feed_api_key: ${{ secrets.TASKS_TEST_FEED_API_KEY }}GITHUB_TOKENworkdir: Frends.SYSTEM.ACTIONstrict_analyzers: truedotnet_version: 8.0.xprebuild_command: docker-compose up -d)Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores