Conversation
WalkthroughConvert request/response printing to io.Writer-backed APIs, add a PROXY header setter, defer some proxy-protocol validation in HTTP client construction, validate non-negative timeouts, expand tests, and update CI workflows and Go toolchain patch version. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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: 3
🧹 Nitpick comments (4)
internal/requests/requests.go (1)
482-484: Consider clarifying behavior when timeout is 0.The validation rejects negative timeouts but allows 0. A timeout of 0 means no timeout (infinite wait), which may not be the intended behavior. Consider whether 0 should be rejected or handled specially.
internal/requests/requests_test.go (3)
824-825: Unchecked error return fromSetProxyProtocolHeader.The return values from
SetProxyProtocolHeaderare not checked, which could mask configuration errors in the test.c.SetTransportOverride(transportURL) - c.SetProxyProtocolHeader(header) + _, err = c.SetProxyProtocolHeader(header) + if err != nil { + t.Fatal(err) + }
919-920: Commented outt.Parallel()may cause test isolation issues.The
t.Parallel()is commented out for this test. If this is intentional due to shared state (the test server), consider adding a comment explaining why parallelism is disabled.
932-936: Consider adding TLS MinVersion for test clients (optional).The static analysis flags missing
MinVersionin TLS config. While this is test code and the risk is low, settingMinVersion: tls.VersionTLS12is a good practice.tr := &http.Transport{TLSClientConfig: &tls.Config{ RootCAs: caCertPool, + MinVersion: tls.VersionTLS12, // InsecureSkipVerify: true, }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/requests/requests.go(9 hunks)internal/requests/requests_handlers.go(2 hunks)internal/requests/requests_test.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/requests/requests.go (4)
internal/style/style_handlers.go (1)
LgSprintf(21-26)internal/style/style.go (1)
Cmd(32-33)internal/requests/requests_handlers.go (1)
TLSVersionName(38-53)internal/certinfo/common_handlers.go (1)
PrintCertInfo(19-35)
🪛 ast-grep (0.40.0)
internal/requests/requests_test.go
[warning] 932-935: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
RootCAs: caCertPool,
// InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 golangci-lint (2.5.0)
internal/requests/requests.go
[error] 597-597: Error return value of r.PrintRequestDebug is not checked
(errcheck)
🔇 Additional comments (6)
internal/requests/requests.go (2)
425-429:SetProxyProtocolV2now always returns nil error.The function signature returns an error, but the current implementation never returns one. This is fine for forward compatibility, but consider simplifying if an error return isn't needed.
290-292: The review comment's concern is incorrect—plain hostnames like"example.com"and"localhost"are successfully parsed byurl.Parseand pass validation.The tests explicitly confirm this:
TestNewRequestHTTPClient_SetServerNameincludes test cases for"localhost","127.0.0.1", and"example.com", all of which pass theurl.Parsecheck. Whileurl.Parsedoes treat these as relative paths (not full URLs), it does not reject them with an error. The only documented validation failure is IPv6 addresses with brackets (e.g.,"[::1]"), which fails due to Go's URL parsing rules about colons in path segments, not because hostnames are rejected.Additionally, the parsed URL result is discarded (using
_), and the function directly passes the originalserverNamestring toTLSClientConfig.ServerName, sourl.Parseis used purely for validation, not for extracting URL components.internal/requests/requests_handlers.go (1)
209-209: LGTM!The call correctly uses
os.Stdoutas the writer, aligning with the updatedPrintCmdsignature.internal/requests/requests_test.go (3)
161-196: LGTM!Good test coverage for
SetRequestswith multiple request configurations and hosts.
240-281: LGTM!Good error path coverage for
NewHTTPClientFromRequestConfig, testing proxy protocol requirements and server name parsing errors.
1082-1227: Good integration test forprocessHTTPRequestsByHost.Comprehensive test coverage including successful requests, certificate verification errors, and response body matching. The test properly sets up and tears down test servers.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/requests/requests_test.go (1)
897-1005: LGTM: Comprehensive response debug output tests.The test validates debug output for both TLS and non-TLS responses. The "non-TLS" subtest at lines 989-1004 is correctly placed outside the table-driven loop.
Optional: Consider adding TLS MinVersion for test TLS config.
At lines 933-936, the TLS config could explicitly set
MinVersion: tls.VersionTLS12(or TLS13) for clarity, though this is a minor point for test code.tr := &http.Transport{TLSClientConfig: &tls.Config{ RootCAs: caCertPool, + MinVersion: tls.VersionTLS12, // InsecureSkipVerify: true, }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/requests/requests_test.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/requests/requests_test.go (1)
internal/requests/requests.go (8)
Host(76-79)URI(72-72)RequestConfig(86-104)NewHTTPClientFromRequestConfig(492-548)NewRequestHTTPClient(264-282)RequestHTTPClient(106-111)RequestsMetaConfig(123-131)RequestHeader(81-84)
🪛 ast-grep (0.40.0)
internal/requests/requests_test.go
[warning] 932-935: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
RootCAs: caCertPool,
// InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 GitHub Check: SonarCloud Code Analysis
internal/requests/requests_test.go
[failure] 188-188: Handle this error explicitly or document why it can be safely ignored.
[warning] 161-161: Rename function "TestRequestsMetaConfig_SetRequests" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[failure] 164-164: Define a constant instead of duplicating this literal "example.com" 5 times.
[failure] 283-283: Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed.
[warning] 240-240: Rename function "TestNewHTTPClientFromRequestConfig_Error" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[failure] 257-257: Define a constant instead of duplicating this literal "https://localhost:8443" 3 times.
[warning] 451-451: Rename function "TestNewRequestHTTPClient_SetClientTimeout" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[warning] 480-480: Rename function "TestNewRequestHTTPClient_SetClientTimeout_Error" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[failure] 1082-1082: Refactor this method to reduce its Cognitive Complexity from 28 to the 15 allowed.
🔇 Additional comments (8)
internal/requests/requests_test.go (8)
4-4: LGTM: New imports support expanded test coverage.The added imports (
bytes,net/url,time) are appropriate for the new test functions covering buffer-based output, URL handling, and timeout behavior.Also applies to: 10-10, 12-12
161-196: LGTM: Comprehensive test for SetRequests.This test validates that
SetRequestscorrectly populates theRequestsfield with various configurations including hosts and URIs.
240-281: LGTM: Error path coverage for HTTP client construction.This test validates error handling for invalid proxy protocol configurations and unparseable server names.
384-449: LGTM: Expanded server name validation tests.The addition of error cases for invalid server names like
[::1]and nil client handling strengthens the test coverage. The move of[::1]from valid to invalid names aligns with the updated server name parsing logic.
451-507: LGTM: Thorough timeout validation tests.These tests cover valid timeout values, edge cases (zero timeout), and error conditions (negative timeout, nil client).
825-825: LGTM: API usage updated to SetProxyProtocolHeader.This reflects the refactoring of Proxy Protocol v2 handling into separate enable flag and header-writing mechanisms.
871-895: LGTM: Simple test for verbose command output.This test validates that
PrintCmdrespects theRequestVerboseflag.
1082-1227: LGTM: Comprehensive integration test for request processing.This test validates end-to-end request processing including TLS, error handling, custom headers, user agents, and response body matching. The complexity is appropriate for an integration test.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/requests/requests.go (1)
601-601: Handle the error returned byPrintRequestDebug.The error returned by
PrintRequestDebugshould be handled appropriately.Apply this diff:
- r.PrintRequestDebug(os.Stdout, req) + if err := r.PrintRequestDebug(os.Stdout, req); err != nil { + fmt.Fprintf(os.Stderr, "Warning: PrintRequestDebug failed: %v\n", err) + }
🧹 Nitpick comments (2)
internal/requests/requests.go (2)
241-241: Add defensive nil check forresp.Request.While the
httppackage typically setsresp.Request, adding a nil check would make this code more robust and consistent with the nil check forrespitself.Apply this diff:
- fmt.Fprintf(w, "Requested url: %s\n", resp.Request.URL) + if resp.Request != nil { + fmt.Fprintf(w, "Requested url: %s\n", resp.Request.URL) + }
249-261: Certificate printing bypasses the writer.The calls to
certinfo.PrintCertInfoat lines 251 and 259 usefmt.Printfdirectly instead of the provided writer, creating inconsistent output behavior. Debug output should consistently use the writer parameter.Consider updating
certinfo.PrintCertInfoto accept anio.Writerparameter in a future refactor, or add a note in the code about this known inconsistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/requests/requests.go(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/requests/requests.go (4)
internal/style/style_handlers.go (1)
LgSprintf(21-26)internal/style/style.go (1)
Cmd(32-33)internal/requests/requests_handlers.go (1)
TLSVersionName(38-53)internal/certinfo/common_handlers.go (1)
PrintCertInfo(19-35)
🪛 GitHub Check: SonarCloud Code Analysis
internal/requests/requests.go
[failure] 229-229: Refactor this method to reduce its Cognitive Complexity from 17 to the 15 allowed.
🪛 golangci-lint (2.5.0)
internal/requests/requests.go
[error] 601-601: Error return value of r.PrintRequestDebug is not checked
(errcheck)
🔇 Additional comments (6)
internal/requests/requests.go (6)
187-195: LGTM! Writer-based output improves testability.The refactor to accept an
io.Writerparameter makes this method more testable and flexible.
209-227: LGTM! Good defensive programming.The nil check and error propagation improve robustness. The writer-based output is consistent with the broader refactoring.
429-433: LGTM! Clean API separation.The refactor to separate the enable flag from header configuration is a good design improvement.
435-478: LGTM! Well-structured proxy protocol header setup.The implementation correctly validates preconditions and properly handles errors in the dial context.
486-488: LGTM! Good input validation.The negative timeout check prevents invalid configurations.
529-549: LGTM! Proper validation and error handling.The validation ensures proxy protocol configuration is complete before attempting to set up the header.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/requests/requests.go (1)
430-479: Fix error message in SetProxyProtocolHeader; simplification of SetProxyProtocolV2 is optional and would require call-site updates.
- The error message on line 438 incorrectly references
SetProxyProtocolV2when it should referenceSetProxyProtocolHeader:- if rc.transportAddress == emptyString { - return nil, errors.New("SetProxyProtocolV2 failed: transportOverrideURL not set") - } + if rc.transportAddress == emptyString { + return nil, errors.New("SetProxyProtocolHeader failed: transportOverrideURL not set") + }
- The proxy protocol dialer override and
header.WriteTo(conn)flow is sound, with proper connection closure on header write failure.- Simplifying
SetProxyProtocolV2to return only*RequestHTTPClient(removing theerrorreturn) is an optional refactor, but it would require updating the call site at line 530–533 since error handling is currently expected at that location.
♻️ Duplicate comments (3)
internal/requests/requests.go (3)
229-267:PrintResponseDebugis safer and writer-based; consider extra guards and refactor per TODO/Sonar.The new nil‑
respcheck resolves the earlier panic risk and routing all output throughwis consistent with the request side. For extra robustness and to satisfy the TODO / Sonar hint:
- Guard
resp.Requestbefore usingresp.Request.URLto avoid surprises if this is ever called with a manually constructed*http.Response.- Give this method an
errorreturn (likePrintRequestDebug) and return on dump/write failures; then the caller can log a warning similarly to the request path.- To reduce cognitive complexity, consider extracting TLS/certificate printing into a helper like
printTLSInfo(w io.Writer, cs *tls.ConnectionState).These changes would address the Sonar complexity warning and make the API more consistent.
295-307:SetServerNamevalidation still accepts full URLs, which can break SNI expectations.
url.Parse(serverName)will accept values such as"https://example.com/path", buttls.Config.ServerNameis intended to be a bare hostname (optionally host:port). Passing a full URL here can lead to TLS/SNI handshake failures or confusing errors.Consider tightening validation, e.g.:
- if _, err := url.Parse(serverName); err != nil { - return nil, fmt.Errorf("unable to parse serverName %s: %w", serverName, err) - } + if serverName == "" { + return nil, errors.New("serverName cannot be empty") + } + if strings.Contains(serverName, "://") || strings.Contains(serverName, "/") { + return nil, fmt.Errorf("serverName should be a bare hostname, not a URL: %s", serverName) + }(and optionally allow/validate
host:portvianet.SplitHostPortif you need that form).
602-604: Error handling forPrintRequestDebugcall resolves earlier concern.Wrapping the
PrintRequestDebugcall, checking the error, and logging a warning tostderrensures debug failures are visible instead of silently ignored, addressing the previous “ignored error return” issue.
🧹 Nitpick comments (3)
internal/requests/requests.go (3)
209-227:PrintRequestDebugerror handling and writer usage look solid; minor polish possible.The nil‑request guard prevents panics, dump/write errors are propagated, and callers now log failures. For symmetry with the dump error path, you might also emit a short warning to
wwhenreq == nil, so users see why no debug output was produced.
530-548: Proxy protocol wiring and validation inNewHTTPClientFromRequestConfiglook coherent.The explicit check that
EnableProxyProtocolV2requires a non‑emptyTransportOverrideURL, and the subsequent conditional call toSetProxyProtocolHeaderusing a derived header, make the configuration rules clear and avoid misconfigured PROXY‑protocol usage. If you want to simplify slightly, you could inline theenableProxyProtoV2assignment instead of callingSetProxyProtocolV2, or haveSetProxyProtocolHeaderinternally checkenableProxyProtoV2, but that’s purely optional.
621-621: Response debug output correctly uses the writer-based API; mirror error handling once available.Routing response debug info through
r.PrintResponseDebug(os.Stdout, resp)aligns with the new writer-based design. If you later changePrintResponseDebugto return anerror(as per the TODO), consider mirroring the same warning/log pattern used forPrintRequestDebugso response dump failures are also surfaced.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/requests/requests.go(9 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
internal/requests/requests.go
[warning] 230-230: Complete the task associated to this TODO comment.
[failure] 229-229: Refactor this method to reduce its Cognitive Complexity from 17 to the 15 allowed.
🔇 Additional comments (3)
internal/requests/requests.go (3)
10-15: Imports align with new writer-based and URL/OS usage.The added
io,net/url, andosimports are all used by the new writer-based debugging and server-name validation paths; no issues here.
187-195:PrintCmdwriter parameter is a good API improvement.Routing command output through an
io.Writerinstead of hard‑coding stdout improves testability and flexibility without changing behavior.
487-495: Negative-timeout validation is a good safety check.Rejecting negative
timeoutvalues before converting totime.Durationprevents accidental misconfiguration ofhttp.Client.Timeoutwith a negative duration while preserving existing behavior for zero/non‑negative values.
…l output strings and split non-TLS check
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
internal/requests/requests_test.go (8)
188-189: Handle the error fromNewRequestsMetaConfig.The error is discarded with
_. While unlikely to fail in tests, explicitly handling errors improves test reliability and follows static analysis recommendations.- rmc, _ := NewRequestsMetaConfig() + rmc, err := NewRequestsMetaConfig() + require.NoError(t, err)
257-258: Consider defining a constant for the repeated URL literal.
"https://localhost:8443"appears multiple times in this file. Extracting it to a test constant improves maintainability.const testProxyURL = "https://localhost:8443"Then use
testProxyURLat lines 257, 305, and 815.
464-466: Redundant error check.The
if err != nilguard is unnecessary beforerequire.Error. For successful cases, the error should be nil. If you expect no error here, userequire.NoError(t, err)instead.- _, err := c.SetClientTimeout(tt) - if err != nil { - require.Error(t, err) - } + _, err := c.SetClientTimeout(tt) + require.NoError(t, err)
492-492: Misleading test name."Nil Timeout" suggests testing a nil timeout value, but this test checks behavior when the client is nil. Rename for clarity.
- t.Run("Nil Timeout", func(t *testing.T) { + t.Run("Nil Client", func(t *testing.T) {
956-956: Remove or convert debug print statement.
fmt.Printfoutputs to stdout unconditionally. Uset.Logffor test-only output that respects-vflag.- fmt.Printf("got:\n%s\n", got) + t.Logf("got:\n%s\n", got)
1059-1062: Error handling doesn't validate expected behavior.This pattern silently passes when
err != nil. IfPrintRequestDebugshould return an error for certain inputs (like nil request), explicitly verify it. If errors are unexpected, userequire.NoError.For test cases where you expect an error:
if tt.expectError { require.Error(t, err, "expected error for %s", tt.desc) } else { require.NoError(t, err) }
1157-1158: Remove debug print statement.
fmt.Printfoutputs unconditionally and clutters test output. Uset.Logfif needed for debugging.for _, r := range respList { - fmt.Printf("resp type: %T\n", r) + // t.Logf("resp type: %T\n", r) // uncomment for debugging
1153-1155: Consider usingt.Fatalorrequire.NoErrorfor setup errors.Using
t.Error(err)allows the test to continue afterprocessHTTPRequestsByHostfails, which may cause confusing nil pointer panics in subsequent assertions. If this error indicates test infrastructure failure, fail fast.- if err != nil { - t.Error(err) - } + require.NoError(t, err, "processHTTPRequestsByHost setup failed")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/requests/requests_test.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/requests/requests_test.go (1)
internal/requests/requests.go (8)
Host(76-79)URI(72-72)RequestConfig(86-104)NewRequestsMetaConfig(133-144)NewHTTPClientFromRequestConfig(497-553)NewRequestHTTPClient(269-287)RequestHTTPClient(106-111)RequestsMetaConfig(123-131)
🪛 ast-grep (0.40.0)
internal/requests/requests_test.go
[warning] 939-941: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
RootCAs: caCertPool,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 GitHub Check: SonarCloud Code Analysis
internal/requests/requests_test.go
[failure] 164-164: Define a constant instead of duplicating this literal "example.com" 5 times.
[warning] 161-161: Rename function "TestRequestsMetaConfig_SetRequests" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[failure] 188-188: Handle this error explicitly or document why it can be safely ignored.
[failure] 257-257: Define a constant instead of duplicating this literal "https://localhost:8443" 3 times.
[warning] 240-240: Rename function "TestNewHTTPClientFromRequestConfig_Error" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[failure] 283-283: Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed.
[warning] 480-480: Rename function "TestNewRequestHTTPClient_SetClientTimeout_Error" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[warning] 451-451: Rename function "TestNewRequestHTTPClient_SetClientTimeout" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[failure] 897-897: Refactor this method to reduce its Cognitive Complexity from 18 to the 15 allowed.
[failure] 1072-1072: Refactor this method to reduce its Cognitive Complexity from 28 to the 15 allowed.
[warning] 978-978: Rename function "TestPrintResponseDebug_nonTLS" to match the regular expression ^(_|[a-zA-Z0-9]+)$
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
internal/requests/requests.go (1)
228-266: Still risk of nil deref onresp.RequestandPrintResponseDebugremains complex.
- Line 241:
fmt.Fprintf(w, "Requested url: %s\n", resp.Request.URL)will panic ifresp.Requestorresp.Request.URLis nil. Even with the newresp == nilguard, this remains a potential crash, especially for manually constructedhttp.Responsevalues (as in tests) or future callers. This overlaps the earlier review comment and is only partially addressed.- The TODO about returning an error plus multiple nested branches contributes to the Sonar cognitive-complexity warning.
Consider:
func (r *RequestConfig) PrintResponseDebug(w io.Writer, resp *http.Response) { // TODO: return an error if resp == nil { return } @@ - fmt.Fprintf(w, "Requested url: %s\n", resp.Request.URL) + if resp.Request != nil && resp.Request.URL != nil { + fmt.Fprintf(w, "Requested url: %s\n", resp.Request.URL) + } else { + fmt.Fprintln(w, "Requested url: <nil>") + } @@ - fmt.Fprintln(w, "TLS:") - fmt.Fprintf(w, "Version: %v\n", TLSVersionName(resp.TLS.Version)) - fmt.Fprintf(w, "CipherSuite: %v\n", cipherSuiteName(resp.TLS.CipherSuite)) + fmt.Fprintln(w, "TLS:") + fmt.Fprintf(w, "Version: %v\n", TLSVersionName(resp.TLS.Version)) + fmt.Fprintf(w, "CipherSuite: %v\n", cipherSuiteName(resp.TLS.CipherSuite)) // Consider extracting the TLS/cert printing below into a helper to // reduce this function's cognitive complexity.Optionally, to match the rest of the writer-based API, you might also:
- Extract TLS/cert printing into a small helper (e.g.
printTLSDebug(w io.Writer, cs *tls.ConnectionState)), which will address the Sonar Cognitive Complexity warning.- In a follow-up, add writer-aware variants in
certinfoso that certificate details respect the sameio.Writerinstead of always going to stdout.internal/requests/requests_test.go (2)
284-386: Fix typo in test struct field nametranportAddress.The table struct uses
tranportAddress(line 290), which is a typo and was already flagged in a prior review. Even though it’s only used as test data today, keeping the correct spelling avoids confusion:- desc string - reqConf RequestConfig - serverName string - pool *x509.CertPool - tranportAddress string + desc string + reqConf RequestConfig + serverName string + pool *x509.CertPool + transportAddress stringDon’t forget to update the field names in the table literals as well.
918-997: TightenTestPrintResponseDebugassertions and fix stale message.A few small issues here:
- The assertion message in the non-verbose branch still says
"check PrintRequestDebug with verbose False", which is misleading and was previously flagged.- The non-verbose branch only checks
len(got) == 0before asserting, instead of directly asserting emptiness unconditionally.- The
fmt.Printf("got:\n%s\n", got)debug line will spam test output.You can simplify and clarify:
- got := buffer.String() - fmt.Printf("got:\n%s\n", got) - - if !tt.verbose && len(got) == 0 { - assert.Equal(t, - []byte(nil), - buffer.Bytes(), - "check PrintRequestDebug with verbose False", - ) - } - - if tt.verbose { - for _, output := range tt.outputs { - assert.True(t, - bytes.Contains(buffer.Bytes(), []byte(output)), - "check PrintResponseDebug contains: %s", output, - ) - } - } + got := buffer.String() + + if !tt.verbose { + assert.Equal(t, + "", + got, + "check PrintResponseDebug with verbose false", + ) + return + } + + for _, output := range tt.outputs { + assert.True(t, + bytes.Contains(buffer.Bytes(), []byte(output)), + "check PrintResponseDebug contains: %s", output, + ) + }This removes the debug print, fixes the assertion message, and makes the expectations clearer.
🧹 Nitpick comments (7)
internal/requests/requests.go (2)
288-315: Server name validation is improved but still allows clearly invalid values.Empty strings and URL-like values are now rejected, which is good. However, values like
" a silly string "or"example.com/path"still pass and will be used asTLSClientConfig.ServerName, which is not a valid SNI hostname and may cause confusing TLS errors.Consider tightening validation here by:
- Trimming whitespace.
- Optionally rejecting values containing
/or spaces.- Optionally parsing
host:portvianet.SplitHostPortand using only the host part forServerName.That would make the behavior more predictable and aligned with what
ServerNameis meant to represent.
605-607: Good handling ofPrintRequestDebugerrors; mirror this oncePrintResponseDebugreturns errors.Handling the error from
PrintRequestDebugand logging a warning toos.Stderris exactly what was missing before. OncePrintResponseDebugis updated to return an error (per the TODO), it would be good to handle it in a similar way here for symmetry and observability.Also applies to: 624-624
internal/requests/requests_test.go (5)
161-196: HandleNewRequestsMetaConfigerrors explicitly inTestRequestsMetaConfig_SetRequests.Right now the test ignores the error from
NewRequestsMetaConfig:rmc, _ := NewRequestsMetaConfig()Given this function can fail (e.g. system cert pool issues), it’s better to assert success to keep the test honest:
- rmc, _ := NewRequestsMetaConfig() + rmc, err := NewRequestsMetaConfig() + require.NoError(t, err) rmc.SetRequests(tt)This also addresses the Sonar warning about ignoring the error.
472-499: SimplifyTestNewRequestHTTPClient_SetClientTimeoutand assert on errors directly.The current pattern:
_, err := c.SetClientTimeout(tt) if err != nil { require.Error(t, err) } var i any = c.client.Timeout duration, ok := i.(time.Duration)
- Only calls
require.Errorwhenerr != nil, which doesn’t assert the “no error expected” cases.- Casts
c.client.Timeoutthroughanyeven though it’s already atime.Duration.You can simplify and make the intent clearer:
- c := NewRequestHTTPClient() - - _, err := c.SetClientTimeout(tt) - if err != nil { - require.Error(t, err) - } - - var i any = c.client.Timeout - - duration, ok := i.(time.Duration) - if !ok { - t.Fatalf("expected time.Duration, got %T", c.client.Timeout) - } - - assert.Equal(t, time.Duration(tt)*time.Second, duration) + c := NewRequestHTTPClient() + + _, err := c.SetClientTimeout(tt) + require.NoError(t, err) + + assert.Equal(t, + time.Duration(tt)*time.Second, + c.client.Timeout, + )This makes the test clearer and removes unnecessary complexity.
791-890: Check the error returned bySetProxyProtocolHeaderin the server test.In
TestRequestHTTPClient_SetProxyProtocolV2_server, the call:c := NewRequestHTTPClient() c.SetTransportOverride(transportURL) c.SetProxyProtocolHeader(header)ignores the error from
SetProxyProtocolHeader. While this particular setup should succeed, handling the error will catch regressions in the proxy-protocol setup path:- c := NewRequestHTTPClient() - c.SetTransportOverride(transportURL) - c.SetProxyProtocolHeader(header) + c := NewRequestHTTPClient() + _, err = c.SetTransportOverride(transportURL) + require.NoError(t, err) + + _, err = c.SetProxyProtocolHeader(header) + require.NoError(t, err)That will also satisfy static analyzers complaining about unchecked errors.
1018-1091:TestPrintRequestDebugbroadly covers the new behaviors; minor enhancement opportunity.The table-driven cases validate the verbose/non-verbose paths, nil request handling, and error from
DumpRequestOut, including output content. If you want stricter checks, consider adding awantErr boolfield and assertingrequire.NoErrorvsrequire.Errorexplicitly per case, but the current coverage is already quite useful.
1093-1238:TestProcessHTTPRequestsByHostis comprehensive but quite complex.This test exercises many behaviors (UA/header propagation, TLS name mismatch, body regexp matching, error-path handling) in a single, fairly long function, which matches the Sonar “cognitive complexity” warning.
To improve maintainability, consider in a follow-up:
- Splitting it into smaller tests focused on one concern each (e.g., success path, TLS name mismatch, body-regexp behavior).
- Or extracting small helper functions for common setup and assertions.
Not a blocker, but it will make future changes easier to reason about.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/requests/requests.go(9 hunks)internal/requests/requests_test.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/requests/requests.go (4)
internal/style/style_handlers.go (1)
LgSprintf(21-26)internal/style/style.go (2)
Cmd(32-33)URL(79-80)internal/requests/requests_handlers.go (1)
TLSVersionName(38-53)internal/certinfo/common_handlers.go (1)
PrintCertInfo(19-35)
🪛 ast-grep (0.40.0)
internal/requests/requests_test.go
[warning] 960-962: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
RootCAs: caCertPool,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 GitHub Check: SonarCloud Code Analysis
internal/requests/requests_test.go
[failure] 164-164: Define a constant instead of duplicating this literal "example.com" 5 times.
[failure] 188-188: Handle this error explicitly or document why it can be safely ignored.
[warning] 161-161: Rename function "TestRequestsMetaConfig_SetRequests" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[failure] 258-258: Define a constant instead of duplicating this literal "https://localhost:8443" 3 times.
[warning] 240-240: Rename function "TestNewHTTPClientFromRequestConfig_Error" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[failure] 284-284: Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed.
[warning] 501-501: Rename function "TestNewRequestHTTPClient_SetClientTimeout_Error" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[warning] 472-472: Rename function "TestNewRequestHTTPClient_SetClientTimeout" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[warning] 429-429: Rename function "TestNewRequestHTTPClient_SetServerName_Error" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[failure] 1093-1093: Refactor this method to reduce its Cognitive Complexity from 28 to the 15 allowed.
[failure] 918-918: Refactor this method to reduce its Cognitive Complexity from 18 to the 15 allowed.
[warning] 999-999: Rename function "TestPrintResponseDebug_nonTLS" to match the regular expression ^(_|[a-zA-Z0-9]+)$
internal/requests/requests.go
[failure] 228-228: Refactor this method to reduce its Cognitive Complexity from 17 to the 15 allowed.
[warning] 229-229: Complete the task associated to this TODO comment.
🔇 Additional comments (9)
internal/requests/requests.go (4)
186-194: Writer-basedPrintCmdlooks consistent with style usage.Changing
PrintCmdto accept anio.Writerand usingfmt.Fprintfis a good step toward testability and output redirection; no behavioral issues spotted here.
208-226:PrintRequestDebugerror handling and writer injection look good.The nil guard on
req, propagation ofDumpRequestOut/Fprintferrors, and using the provided writer make this function safer and more testable. Callers now have a clear contract via the returned error.
484-498: Timeout validation is clear and safe.Rejecting negative timeouts with a descriptive error and using
time.Duration(timeout) * time.Secondis straightforward and correct; 0 seconds maps cleanly to Go's “no deadline” behavior.
533-542: Proxy-protocol configuration flow inNewHTTPClientFromRequestConfiglooks coherent.
- Enforcing
EnableProxyProtocolV2to require a non-emptyTransportOverrideURLgives a nice early and explicit error, and the tests cover these paths.- Conditioning
SetProxyProtocolHeaderon bothEnableProxyProtocolV2and a non-emptytransportAddressavoids misconfiguration when no transport override is present.No functional issues spotted here.
Also applies to: 549-552
internal/requests/requests_test.go (5)
240-282: Error-path tests forNewHTTPClientFromRequestConfigare well targeted.The two cases around missing
TransportOverrideURLwhen proxy protocol is enabled and empty server name give good coverage of the new validation behavior; the string comparisons on the wrapped errors look correct.
388-427: Server-name happy-path tests look good.These cases verify that
SetServerNamepopulatesTLSClientConfig.ServerNameand keepsTLSClientConfignon-nil. Once you tighten validation rules further (e.g., trimming whitespace or forbidding spaces), you may want to adjust or drop the" a silly string "case, but for now the coverage is appropriate.
429-470: Error-path coverage forSetServerNameis solid.The tests cover both invalid arguments (empty and URL-form values) and the “nil underlying HTTP client” case with precise message checks, which matches the new validation logic.
501-528: Negative-timeout and nil-client error cases are well covered.The error messages asserted here match the production behavior and nicely pin down the two main failure modes of
SetClientTimeout.
999-1016: Non-TLS response test nicely covers the “no TLS” branch.Constructing a plain HTTP
Responseand asserting the"TLS: Not available"marker exercises the non-TLS code path inPrintResponseDebugwell; no issues here.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/requests/requests_test.go (1)
892-916: Expected output may not match styled output.As noted in previous reviews,
PrintCmduses lipgloss styling which includes ANSI escape codes and padding. The expected string"\n Requests \n"may not match the actual styled output.Consider using content-based assertions instead:
got := buffer.String() - want := tt.output - - assert.Equal(t, want, got, "check PrintCmd") + if tt.verbose { + assert.Contains(t, got, "Requests", "check PrintCmd when verbose") + } else { + assert.Equal(t, "", got, "check PrintCmd when not verbose") + }
🧹 Nitpick comments (1)
internal/requests/requests_test.go (1)
961-963: Consider setting TLS MinVersion.The TLS configuration is missing
MinVersion. While this is test code, it's good practice to explicitly set the minimum TLS version to avoid using outdated protocols.Apply this diff to set a secure minimum TLS version:
tr := &http.Transport{TLSClientConfig: &tls.Config{ RootCAs: caCertPool, + MinVersion: tls.VersionTLS12, }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/requests/requests_test.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/requests/requests_test.go (2)
internal/requests/requests.go (9)
Host(75-78)URI(71-71)RequestConfig(85-103)NewRequestsMetaConfig(132-143)NewHTTPClientFromRequestConfig(500-556)NewRequestHTTPClient(268-286)RequestHTTPClient(105-110)RequestsMetaConfig(122-130)RequestHeader(80-83)internal/requests/main_test.go (1)
NewHTTPSTestServer(168-217)
🪛 ast-grep (0.40.0)
internal/requests/requests_test.go
[warning] 960-962: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
RootCAs: caCertPool,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 GitHub Check: SonarCloud Code Analysis
internal/requests/requests_test.go
[failure] 188-188: Handle this error explicitly or document why it can be safely ignored.
[warning] 161-161: Rename function "TestRequestsMetaConfig_SetRequests" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[failure] 164-164: Define a constant instead of duplicating this literal "example.com" 5 times.
[warning] 240-240: Rename function "TestNewHTTPClientFromRequestConfig_Error" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[failure] 258-258: Define a constant instead of duplicating this literal "https://localhost:8443" 3 times.
[failure] 284-284: Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed.
[warning] 472-472: Rename function "TestNewRequestHTTPClient_SetClientTimeout" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[warning] 501-501: Rename function "TestNewRequestHTTPClient_SetClientTimeout_Error" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[warning] 429-429: Rename function "TestNewRequestHTTPClient_SetServerName_Error" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[failure] 918-918: Refactor this method to reduce its Cognitive Complexity from 18 to the 15 allowed.
[warning] 999-999: Rename function "TestPrintResponseDebug_nonTLS" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[failure] 1093-1093: Refactor this method to reduce its Cognitive Complexity from 28 to the 15 allowed.
🔇 Additional comments (10)
internal/requests/requests_test.go (10)
240-282: LGTM!The error path testing for
NewHTTPClientFromRequestConfigis thorough and correctly validates both the error occurrence and the error messages.
284-386: LGTM!Comprehensive test coverage for
NewHTTPClientFromRequestConfigincluding timeout, method, proxy protocol, insecure mode, and CA pool configurations. The test structure is clear and validates all key properties.
388-427: LGTM!The test correctly handles the updated
SetServerNameAPI that now returns an error, and properly validates that the TLS configuration is set up with the correct server name.
429-470: LGTM!Excellent error path coverage for
SetServerName, including validation of empty inputs, URL vs hostname distinction, and nil client guards.
501-528: LGTM!Proper error path testing for
SetClientTimeout, covering negative timeout values and nil client scenarios with appropriate error message validation.
846-846: LGTM!Correctly updated to use the renamed
SetProxyProtocolHeadermethod, consistent with the API changes in this PR.
999-1016: LGTM!Good edge case coverage for non-TLS connections, verifying that the debug output correctly indicates when TLS is not available.
1018-1091: LGTM!Comprehensive test coverage for
PrintRequestDebugincluding normal operation, verbose mode toggle, nil request handling, and incomplete request error cases.
1093-1238: LGTM!Excellent integration test coverage for
processHTTPRequestsByHost. The test validates multiple scenarios including successful requests, certificate validation errors, custom headers/user-agent propagation, and response body regex matching. The cognitive complexity is justified given the comprehensive nature of the test.
188-189: The review comment is incorrect.SetRequestsdoes not return an error.The method signature at
internal/requests/requests.go:181shows thatSetRequestsreturns*RequestsMetaConfig, not an error. The implementation is a simple setter that assigns the requests and returns the receiver for method chaining. Line 188 in the test (rmc.SetRequests(tt)) is correct and requires no error handling.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/requests/requests_test.go (1)
484-487: Fix inverted error handling logic (still not addressed from previous review).The error handling is still incorrect. When
err != nil, callingrequire.Error(t, err)is redundant—it will always pass. The test should verify that no error occurs for valid timeout values.This is the same critical issue flagged in the previous review that remains unaddressed.
Apply this diff to fix the error handling:
_, err := c.SetClientTimeout(tt) - if err != nil { - require.Error(t, err) - } + require.NoError(t, err)
🧹 Nitpick comments (2)
internal/requests/requests_test.go (2)
333-342: Consider failing fast on initialization or type assertion errors.Lines 334 and 341 use
t.Error()andt.Errorf(), allowing the test to continue after critical failures. IfNewHTTPClientFromRequestConfigreturns an error or the client is the wrong type, subsequent assertions will fail with confusing messages.Apply this diff to fail immediately:
rcClient, err := NewHTTPClientFromRequestConfig( tt.reqConf, tt.serverName, tt.pool, ) - if err != nil { - t.Error(err) - } + require.NoError(t, err) var i any = rcClient.client client, ok := i.(*http.Client) if !ok { - t.Errorf("expecting *http.Client, got %T", client) + t.Fatalf("expecting *http.Client, got %T", client) }Similarly at line 365-367:
var ti any = rcClient.client.Transport transport, ok := ti.(*http.Transport) if !ok { - t.Errorf("expecting *http.Transport, got %T", transport) + t.Fatalf("expecting *http.Transport, got %T", transport) }
963-965: AddMinVersionto TLS configuration.The TLS config lacks an explicit
MinVersion, defaulting to TLS 1.2. For tests, it's best practice to explicitly setMinVersion: tls.VersionTLS12(or TLS13 if appropriate) to ensure consistent behavior and align with security best practices.Based on static analysis hints.
Apply this diff:
tr := &http.Transport{TLSClientConfig: &tls.Config{ - RootCAs: caCertPool, + RootCAs: caCertPool, + MinVersion: tls.VersionTLS12, }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/requests/requests_test.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/requests/requests_test.go (2)
internal/requests/requests.go (9)
Host(75-78)URI(71-71)RequestConfig(85-103)NewRequestsMetaConfig(132-143)NewHTTPClientFromRequestConfig(500-556)NewRequestHTTPClient(268-286)RequestHTTPClient(105-110)RequestsMetaConfig(122-130)RequestHeader(80-83)internal/requests/main_test.go (1)
NewHTTPSTestServer(168-217)
🪛 ast-grep (0.40.0)
internal/requests/requests_test.go
[warning] 962-964: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
RootCAs: caCertPool,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 GitHub Check: SonarCloud Code Analysis
internal/requests/requests_test.go
[warning] 161-161: Rename function "TestRequestsMetaConfig_SetRequests" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[failure] 188-188: Handle this error explicitly or document why it can be safely ignored.
[failure] 164-164: Define a constant instead of duplicating this literal "example.com" 5 times.
[failure] 258-258: Define a constant instead of duplicating this literal "https://localhost:8443" 3 times.
[failure] 284-284: Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed.
[warning] 240-240: Rename function "TestNewHTTPClientFromRequestConfig_Error" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[warning] 472-472: Rename function "TestNewRequestHTTPClient_SetClientTimeout" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[warning] 429-429: Rename function "TestNewRequestHTTPClient_SetServerName_Error" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[warning] 501-501: Rename function "TestNewRequestHTTPClient_SetClientTimeout_Error" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[failure] 920-920: Refactor this method to reduce its Cognitive Complexity from 18 to the 15 allowed.
[failure] 1095-1095: Refactor this method to reduce its Cognitive Complexity from 28 to the 15 allowed.
[warning] 1001-1001: Rename function "TestPrintResponseDebug_nonTLS" to match the regular expression ^(_|[a-zA-Z0-9]+)$
🔇 Additional comments (8)
internal/requests/requests_test.go (8)
240-282: LGTM!The error test cases properly validate that
NewHTTPClientFromRequestConfigreturns appropriate errors when proxy protocol is enabled without required fields or when serverName is missing.
388-427: LGTM!The test properly validates that
SetServerNamecorrectly configures the TLS client config with various hostname formats, including IPv6, localhost, IP addresses, and domain names.
429-470: LGTM!The error test cases properly validate that
SetServerNamereturns appropriate errors for empty strings, URLs instead of hostnames, and nil clients.
501-528: LGTM!The error test cases properly validate that
SetClientTimeoutreturns appropriate errors for negative timeout values and nil clients.
846-846: LGTM!The method call has been correctly updated to use
SetProxyProtocolHeader, aligning with the API changes in the request HTTP client.
1001-1018: LGTM!The test properly validates that
PrintResponseDebugcorrectly reports when TLS is not available for HTTP connections.
1020-1093: LGTM!The test provides comprehensive coverage of
PrintRequestDebug, including verbose/non-verbose modes, nil request handling, and incomplete request scenarios with proper error handling validation.
1095-1240: LGTM!This comprehensive test properly validates
processHTTPRequestsByHostfunctionality, including successful requests, TLS certificate validation failures, and response body regex matching. The test complexity is justified by the thorough coverage of different scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/requests/requests.go (2)
294-301: Server name validation still allows path-like or malformed hostnamesThe new checks (non-empty, no
"://") are an improvement overurl.Parse, but values like"example.com/path"or"example.com foo"would still be accepted and then used verbatim asTLSClientConfig.ServerName, which can cause confusing TLS failures.If you want to harden this further, consider additionally rejecting characters that imply a URL path or query, e.g.:
if strings.ContainsAny(serverName, "/?# ") { return nil, fmt.Errorf("serverName should be a bare hostname (no path/query): %q", serverName) }Optionally, you could also allow
host:portand strip the port before assigning toServerName.
228-266: TightenPrintResponseDebugsafety and consider a small refactorA few points here:
- You now guard
resp == nil, which is good. However,resp.Request(and itsURL) can still be nil, sofmt.Fprintf(w, "Requested url: %s\n", resp.Request.URL)is a potential nil dereference. A small defensive check would make this safer:- fmt.Fprintf(w, "Requested url: %s\n", resp.Request.URL) + if resp.Request != nil && resp.Request.URL != nil { + fmt.Fprintf(w, "Requested url: %s\n", resp.Request.URL) + } else { + fmt.Fprintln(w, "Requested url: <unknown>") + }
Most output now goes through
w, butcertinfo.PrintCertInfostill prints directly to stdout. That means TLS certificate details won’t respect the injected writer (e.g., tests using a buffer). If/when you touchcertinfo, it may be worth adding a writer-aware variant so all debug output is consistently routed throughw.The TODO about returning an error plus the Sonar cognitive-complexity warning are both centered on this method. You could address both by:
- Changing the signature to
PrintResponseDebug(w io.Writer, resp *http.Response) error(mirroringPrintRequestDebug), and- Extracting the TLS/certificate-printing block into a small helper (e.g.,
printTLSDebug(w io.Writer, state *tls.ConnectionState)), which should drop complexity below the threshold.
🧹 Nitpick comments (3)
internal/requests/requests.go (3)
208-226: Consider guarding against a nilio.WriterinPrintRequestDebugThe function already defends against a nil
*http.Request, but a nilwwould still cause a panic onfmt.Fprintf. If you expect this to be used with custom writers, a cheap early guard likeif w == nil { return errors.New("nil writer in PrintRequestDebug") }would make the API more robust.
490-492: Timeout error message could better match the checkThe guard correctly rejects negative values, but the message says “must be positive”, which usually implies
> 0while0is actually allowed:- return nil, fmt.Errorf("timeout value must be positive: %v provided", timeout) + return nil, fmt.Errorf("timeout value must be non-negative: %v provided", timeout)Minor, but a bit clearer for callers.
621-621: Plan for futurePrintResponseDebugerror handling at the call siteRight now the response debug call is fire‑and‑forget, which is fine given it returns no error. If you do implement the TODO and make
PrintResponseDebugreturn an error (likePrintRequestDebug), mirroring the same pattern here:if err := r.PrintResponseDebug(os.Stdout, resp); err != nil { fmt.Fprintf(os.Stderr, "Warning: PrintResponseDebug failed: %v\n", err) }will keep error handling consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/requests/requests.go(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/requests/requests.go (4)
internal/style/style_handlers.go (1)
LgSprintf(21-26)internal/style/style.go (1)
Cmd(32-33)internal/requests/requests_handlers.go (1)
TLSVersionName(38-53)internal/certinfo/common_handlers.go (1)
PrintCertInfo(19-35)
🪛 GitHub Check: SonarCloud Code Analysis
internal/requests/requests.go
[warning] 229-229: Complete the task associated to this TODO comment.
[failure] 228-228: Refactor this method to reduce its Cognitive Complexity from 17 to the 15 allowed.
🔇 Additional comments (5)
internal/requests/requests.go (5)
186-193: Writer-basedPrintCmdaligns well with new logging patternRouting the command banner through an
io.Writeris consistent with the rest of the debug API and keeps output easily testable; no issues from my side here.
433-437: SimplifiedSetProxyProtocolV2signature looks goodDropping the unused error return and just toggling the flag makes the method clearer and matches how it’s now used by
NewHTTPClientFromRequestConfig.
439-482:SetProxyProtocolHeaderdial override and header write look correctThe method cleanly enforces that
transportOverrideURLis set, clones the existing transport, and wrapsDialContextso it:
- Dials the override address, and
- Writes the PROXY header, closing the connection and surfacing an error if the write fails.
This is a solid pattern for injecting PROXY v2 without leaking half-open connections.
533-538: Proxy Protocol v2 wiring and validation inNewHTTPClientFromRequestConfiglook soundThe sequence here is coherent:
- You always call
SetProxyProtocolV2for configuration clarity.- You enforce that
EnableProxyProtocolV2implies a non-emptyTransportOverrideURL, failing fast otherwise.- When both the flag and
transportAddressare set, you build a header and callSetProxyProtocolHeader, propagating any error.This keeps misconfigurations from silently falling back to a non‑PROXY connection.
Also applies to: 546-549
602-604: Good: request debug errors are now surfaced but non-fatalHandling the error from
PrintRequestDebugand logging a warning tostderrwhile continuing the request flow is a nice balance between observability and robustness.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
internal/requests/requests_test.go (4)
161-197: SetRequests test looks good; Sonar warnings about literals/naming are optional.The table setup for
hostsandrequestConfigsplus thecmp.Diffassertion gives solid coverage thatSetRequestspreserves the configuration as-is. The use ofNewRequestsMetaConfigwithrequire.NoErroris also correct here.If you care about Sonar’s suggestions, you could:
- Extract
"example.com"(used in multiple tests) into a shared constant.- Optionally rename
TestRequestsMetaConfig_SetRequeststo avoid underscores (e.g.,TestRequestsMetaConfigSetRequests) to match its regex.Both are cosmetic and can be deferred.
389-471: ServerName tests are comprehensive; only minor style/naming nits.The positive-path test validates that
SetServerNamecorrectly wiresTLSClientConfig.ServerNamefor a variety of inputs (IPv4, IPv6, hostname, and a free-form string), and the error test nicely covers:
- Empty
serverName.- URL mistakenly passed as
serverName.- Nil underlying HTTP client.
Two optional polish points:
- In the success test (Lines 405–408), you could use
require.NoError(t, err)instead ofif err != nil { t.Fatal(err) }for consistency with the rest of the file.- Sonar is warning about underscores in
TestNewRequestHTTPClient_SetServerName_Error; if you want to silence that, renaming to something likeTestRequestHTTPClientSetServerNameErrorwould satisfy its regex without changing behavior.Functionally this looks solid.
790-889: Consider asserting onSetProxyProtocolHeadererrors inTestRequestHTTPClient_SetProxyProtocolV2_server.On Line 845,
SetProxyProtocolHeader(header)’s return value and error are ignored. If that method can fail (e.g., due to invalid header state), the test would continue and only fail indirectly via the subsequent HTTP call.To make the failure mode clearer and catch misconfigurations earlier, consider:
c := NewRequestHTTPClient() _, err := c.SetTransportOverride(transportURL) require.NoError(t, err) _, err = c.SetProxyProtocolHeader(header) require.NoError(t, err)This keeps the existing behavioral checks but gives you a precise assertion when header wiring breaks.
1094-1239:TestProcessHTTPRequestsByHostgives strong end-to-end coverage; consider minor cleanup only.This test nicely validates several important behaviors of
processHTTPRequestsByHost:
- Correct wiring of
TransportAddressback to the originating server (tt.srvAddr).- Differentiation between per-response
Errorvs. successful responses viarespStatusCode.- Propagation of default vs. overridden
User-Agent.- Response body regexp match flag.
- Propagation of custom request headers into the outgoing request.
A couple of optional refinements:
- The
fmt.Printfon Line 1180 is useful during development but can be noisy in normal test runs; you could remove or gate it ontesting.Verbose().- Sonar’s cognitive complexity warning for this test could be addressed by extracting shared setup (HTTP test server creation + call to
processHTTPRequestsByHost) into a small helper, but readability is already reasonable, so this is mostly a style call.Functionally, this test is in good shape.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/requests/requests_test.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/requests/requests_test.go (1)
internal/requests/requests.go (9)
Host(75-78)URI(71-71)RequestConfig(85-103)NewRequestsMetaConfig(132-143)NewHTTPClientFromRequestConfig(500-553)NewRequestHTTPClient(268-286)RequestHTTPClient(105-110)RequestsMetaConfig(122-130)RequestHeader(80-83)
🪛 ast-grep (0.40.0)
internal/requests/requests_test.go
[warning] 961-963: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
RootCAs: caCertPool,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 GitHub Check: SonarCloud Code Analysis
internal/requests/requests_test.go
[warning] 161-161: Rename function "TestRequestsMetaConfig_SetRequests" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[failure] 164-164: Define a constant instead of duplicating this literal "example.com" 5 times.
[failure] 285-285: Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed.
[failure] 259-259: Define a constant instead of duplicating this literal "https://localhost:8443" 3 times.
[warning] 241-241: Rename function "TestNewHTTPClientFromRequestConfig_Error" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[warning] 473-473: Rename function "TestNewRequestHTTPClient_SetClientTimeout" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[warning] 430-430: Rename function "TestNewRequestHTTPClient_SetServerName_Error" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[warning] 500-500: Rename function "TestNewRequestHTTPClient_SetClientTimeout_Error" to match the regular expression ^(_|[a-zA-Z0-9]+)$
[failure] 1094-1094: Refactor this method to reduce its Cognitive Complexity from 28 to the 15 allowed.
[failure] 919-919: Refactor this method to reduce its Cognitive Complexity from 18 to the 15 allowed.
[warning] 1000-1000: Rename function "TestPrintResponseDebug_nonTLS" to match the regular expression ^(_|[a-zA-Z0-9]+)$
🔇 Additional comments (4)
internal/requests/requests_test.go (4)
4-12: New imports are appropriate and used.
bytes,net/url, andtimeare all exercised in the new tests; import set looks clean with no unused entries.
241-283: Error-path coverage forNewHTTPClientFromRequestConfigmatches implementation.Both table entries correctly exercise the two early error branches:
- Proxy protocol enabled without a
TransportOverrideURL.- Empty
serverNamepropagating as a wrappedSetServerNameerror message.The exact message assertions ensure the
fmt.Errorf("...: %w", err)wrappers are behaving as expected. No functional issues here.
891-917:TestPrintCmdnow robustly validates verbose vs non-verbose output.The new test structure with:
- A bool table (
true,false),assert.Containsfor the verbose case, andassert.Emptyfor the non-verbose case,nicely decouples the test from exact styling while still checking semantics and satisfies the testifylint guidance about emptiness checks. No issues here.
1000-1017: Non-TLS response debug test is clear and targeted.
TestPrintResponseDebug_nonTLSsets up an HTTP (non-TLS) response and checks that the output contains"TLS: Not available". This neatly covers the non-TLS branch inPrintResponseDebug; the construction ofurl.URLandhttp.Responselooks correct.No changes needed here.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
internal/requests/requests_test.go (2)
1018-1091:TestPrintRequestDebugstill doesn’t assert expected error behavior per case.The table is nicely structured, but the error handling is effectively a no-op:
err := r.PrintRequestDebug(&buffer, tt.request) if err != nil { require.Error(t, err, "PrintRequestDebug error") }
- Success cases won’t fail if an unexpected error is returned; they just validate that “an error is an error”.
- Error cases (
nilRequestError,incompleteRequestError) don’t assert that an error occurred, only that the buffer output matches.To make the test precise, add an
expectErrflag and assert accordingly:- tests := []struct { - desc string - verbose bool - request *http.Request - output string - }{ + tests := []struct { + desc string + verbose bool + request *http.Request + output string + expectErr bool + }{ { desc: "verboseTrue", verbose: true, request: &requestTest, - output: expectedOutput, + output: expectedOutput, + expectErr: false, }, @@ { desc: "verboseFalse", verbose: false, request: &requestTest, - output: emptyString, + output: emptyString, + expectErr: false, }, { desc: "nilRequestError", verbose: true, request: requestTestNilPointer, - output: emptyString, + output: emptyString, + expectErr: true, }, { desc: "incompleteRequestError", verbose: true, request: &requestTestIncomplete, - output: expectedOutputIncomplete, + output: expectedOutputIncomplete, + expectErr: true, }, } @@ - err := r.PrintRequestDebug(&buffer, tt.request) - if err != nil { - require.Error(t, err, "PrintRequestDebug error") - } + err := r.PrintRequestDebug(&buffer, tt.request) + if tt.expectErr { + require.Error(t, err, "PrintRequestDebug should return an error") + } else { + require.NoError(t, err, "PrintRequestDebug should not return an error") + }You can keep the existing output equality check as-is afterwards. This aligns the test with the function’s intended behavior for each scenario.
473-497: Perfect! I now have all the information needed to verify the review comment. Let me confirm my findings:Verification Results:
✅ SetClientTimeout signature confirmed: The function returns
(*RequestHTTPClient, error)as the reviewer stated.✅ Success-path test missing error assertion: The test at lines 473-497 calls
c.SetClientTimeout(tt)without capturing the error return value. This is a real gap since if the function ever returns an error for valid inputs, the test won't catch it.✅ Redundant type assertion confirmed: The test unnecessarily converts
c.client.Timeout(which is alreadytime.Durationfrom Go'shttp.Clientstruct) toanyand back with a type assertion. This adds no safety.✅ Error-path tests are well-written: The
TestNewRequestHTTPClient_SetClientTimeout_Errorfunction (lines 499-526) correctly validates both error cases (negative timeout and nil client) with proper error message assertions.The reviewer's suggested refactoring is accurate and well-justified. The changes align perfectly with the actual implementation contract.
Strengthen
SetClientTimeoutsuccess-path test and remove redundant type assertion.The error-path tests (
Negative Timeout,Nil Timeout) validate both error presence and message content correctly. The success-path test needs two improvements:
- Capture and assert no error for valid timeouts.
Currently,c.SetClientTimeout(tt)ignores the return value. IfSetClientTimeoutever returns an error for valid inputs, the test won't detect it. Capture and validate:- c := NewRequestHTTPClient() - - c.SetClientTimeout(tt) - - var i any = c.client.Timeout - - duration, ok := i.(time.Duration) - if !ok { - t.Fatalf("expected time.Duration, got %T", c.client.Timeout) - } - - assert.Equal(t, time.Duration(tt)*time.Second, duration) + c := NewRequestHTTPClient() + + _, err := c.SetClientTimeout(tt) + require.NoError(t, err, "SetClientTimeout should succeed for %d", tt) + + assert.Equal(t, + time.Duration(tt)*time.Second, + c.client.Timeout, + )
- Drop the
any/type assertion.
c.client.Timeoutis already atime.Duration, so the type-assertion round-trip adds noise without safety.
🧹 Nitpick comments (1)
internal/requests/requests_test.go (1)
161-197:TestRequestsMetaConfig_SetRequestsis clear and exercises propagation correctly.The test covers a realistic mix of
RequestConfigfields (insecure, print body, hosts) and validates thatSetRequestsjust propagates what was set via acmp.Diffon the slice; error handling onNewRequestsMetaConfigis now correct viarequire.NoError.If you ever extend this table beyond a single entry, you could simplify by storing named test cases instead of wrapping the slice in a single-element
[][]RequestConfig, but that’s purely optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
-
.github/workflows/codeChecks.yml(1 hunks) -
.github/workflows/release.yml(2 hunks) -
.github/workflows/vulncheck.yml(0 hunks) -
internal/requests/requests_test.go(6 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/vulncheck.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-08T16:00:48.187Z
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 3
File: internal/requests/requests.go:164-182
Timestamp: 2025-11-08T16:00:48.187Z
Learning: In the https-wrench tool (xenOs76/https-wrench repository), do not enforce minimum TLS version constraints in the HTTP client configuration. The tool is designed to inspect and test TLS settings on various servers, including legacy ones, and needs the flexibility to connect to servers with older TLS versions.
Applied to files:
internal/requests/requests_test.go
🪛 ast-grep (0.40.0)
internal/requests/requests_test.go
[warning] 960-962: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
RootCAs: caCertPool,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🔇 Additional comments (9)
.github/workflows/codeChecks.yml (3)
1-42: Code checks workflow structure is sound.The new workflow follows standard Go CI practices with appropriate trigger paths, logical step sequencing (checkout → build → test → vulnerability scan), and matrix testing across multiple Go versions. The consolidation of vulnerability checks here (replacing the separate vulncheck workflow, per the summary) is a reasonable organizational improvement.
38-41: I'll help you verify thegolang/govulncheck-actionconfiguration. Let me start by examining the workflow file and researching the latest action parameters.
<function_calls>
golang/govulncheck-action latest version parameters documentation
</function_calls>Now let me also check the actual workflow file to see the full context:
<function_calls>
#!/bin/bashRead the codeChecks.yml file to see the full context
cat -n .github/workflows/codeChecks.yml | head -50
</function_calls>Please wait while I gather the necessary information to verify the action configuration.
17-17: Go versions 1.24 and 1.25 are valid, released, and actively maintained.Both versions are confirmed as released and receiving active maintenance as of November 2025, with recent patch updates (1.25.4 and 1.24.10 released on 2025-11-05). The workflow matrix correctly specifies appropriate versions for CI testing.
.github/workflows/release.yml (1)
16-16: The review comment is incorrect—actions/checkout@v5 is compatible and introduces no breaking changes for this workflow.The workflow uses
ubuntu-latest(GitHub-hosted runner), which is automatically maintained and updated by GitHub to support new action versions. The Node.js 24 runtime requirement for v4→v5 is automatically satisfied.The original concern about
fetch-depthand credential handling differences does not apply: the web search confirmed no behavioral breaking changes between v4 and v5 in these areas. The workflow already explicitly setsfetch-depth: 0(line 18) and passes credentials directly via secrets, both of which remain unchanged.internal/requests/requests_test.go (5)
3-17: Imports update looks consistent.The added imports (
bytes,net/url,time) are all used in the new tests and keep the import block clean; no issues here.
789-887: Proxy-protocol test correctly exercises header-based workflow.Switching to
SetProxyProtocolHeader(header)after constructing the header fromproxyProtoHeaderFromRequestkeeps the semantics clear: config → header → client. The test still validates end-to-end behavior (status, URL, User-Agent), so the new API is well covered.
890-916:TestPrintCmdnow uses content-based and emptiness assertions.The new version uses
assert.Containsfor the verbose path andassert.Emptyfor the non-verbose path, which is both clearer and in line withtestifylintguidance. No further changes needed here.
999-1016: Non-TLSPrintResponseDebugbehavior is well covered.This subtest cleanly exercises the HTTP (non-TLS) path and asserts that the output contains
"TLS: Not available", which is exactly the semantic you care about. Looks good.
389-427: I'll help you verify the review comment about theSetServerNametest. Let me start by examining the implementation to understand the expected behavior.
<function_calls>
#!/bin/bashSearch for SetServerName implementation
rg -n "SetServerName" internal/requests/requests.go -C5 2>/dev/null || echo "File or pattern not found"
</function_calls>Let me first locate the file and get a better understanding of the codebase structure:
<function_calls>
#!/bin/bashFind the requests.go file
find . -name "requests.go" -type f 2>/dev/null | head -10
</function_calls>I'll now search more broadly:
<function_calls>
#!/bin/bashSearch for SetServerName across the repository
rg "SetServerName" -n -C5 --type go
</function_calls>Let me also check the test file to see both test sections mentioned:
<function_calls>
#!/bin/bashRead the test file to see the full context
wc -l internal/requests/requests_test.go
</function_calls>Let me read the specific sections:
<function_calls>
#!/bin/bashRead lines 389-427 and 430-471 from the test file
sed -n '389,471p' internal/requests/requests_test.go
</function_calls>Now let me find the actual SetServerName implementation:
<function_calls>
#!/bin/bashSearch for SetServerName method definition
ast-grep --pattern 'func (
$_) SetServerName($ $$) { $$$ }'
</function_calls>Let me try a simpler search:
<function_calls>
#!/bin/bashFind all Go files in the requests directory
find . -path "/requests/.go" -type f 2>/dev/null
</function_calls>Let me search for the implementation more directly:
<function_calls>
#!/bin/bashSearch for SetServerName across all files
rg "func.*SetServerName" --type go -n -B2 -A10
</function_calls>Let me check what's in the internal/requests directory:
<function_calls>
#!/bin/bashList files in internal/requests
ls -la internal/requests/ 2>/dev/null || echo "Directory not found, checking structure"
find . -type d -name "requests" 2>/dev/null
</function_calls>Let me try reading the test file directly to understand the context:
<function_calls>
#!/bin/bashCheck if the test file exists and read it
if [ -f "internal/requests/requests_test.go" ]; then
echo "File found"
head -50 internal/requests/requests_test.go
elseSearch for the file
find . -name "requests_test.go" -type f 2>/dev/null
fi
</function_calls>
…ResponseDebug is false
… test case variable
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/requests/requests_test.go (2)
1112-1191: Remove debug print and strengthen non-verbose assertion.Two issues remain from previous reviews:
Line 1172: The
fmt.Printfdebug statement will clutter test output and should be removed.Lines 1174-1179: The conditional logic is weak. If
ResponseDebug=falseever starts producing output, the conditionlen(got) == 0becomes false and the assertion is silently skipped.Apply this diff:
rc := RequestConfig{ResponseDebug: tt.verbose} buffer := bytes.Buffer{} rc.PrintResponseDebug(&buffer, res) got := buffer.String() - fmt.Printf("got:\n%s\n", got) - if !tt.verbose && len(got) == 0 { + if !tt.verbose { assert.Empty(t, - buffer.Bytes(), - "check PrintResponseDebug with verbose False", + got, + "PrintResponseDebug should not write output when ResponseDebug is false", ) - } - - if tt.verbose { + } else { for _, output := range tt.outputs { assert.True(t, bytes.Contains(buffer.Bytes(), []byte(output)),
1340-1486: Fix error handling and remove debug print inTestProcessHTTPRequestsByHost.Three issues need attention:
Lines 1422-1424: Using
t.Error(err)is non-fatal. IfprocessHTTPRequestsByHostreturns an error,respListmay be nil or incomplete, causing panics when dereferenced at line 1426. Change torequire.NoError.Line 1427: Remove the debug
fmt.Printfto avoid cluttering test output.Lines 1435-1441: The code dereferences
r.Error.Error()without checking ifr.Erroris nil first. If the error field is unexpectedly nil, this will panic.Apply this diff:
respList, err := processHTTPRequestsByHost( tt.reqConf, tt.pool, tt.verbose, ) - if err != nil { - t.Error(err) - } + require.NoError(t, err, "processHTTPRequestsByHost should not return a top-level error") for _, r := range respList { - fmt.Printf("resp type: %T\n", r) - assert.Equal(t, tt.srvAddr, r.TransportAddress, "check TransportAddress", ) if tt.respStatusCode == 0 { + require.Error(t, + r.Error, + "expected ResponseData.Error to be set for %q", + tt.reqConf.Name, + ) assert.Equal(t, tt.errMsg, r.Error.Error(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/requests/requests_test.go(18 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:34.922Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
📚 Learning: 2025-11-29T14:05:34.922Z
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:34.922Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
Applied to files:
internal/requests/requests_test.go
📚 Learning: 2025-11-08T16:00:48.187Z
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 3
File: internal/requests/requests.go:164-182
Timestamp: 2025-11-08T16:00:48.187Z
Learning: In the https-wrench tool (xenOs76/https-wrench repository), do not enforce minimum TLS version constraints in the HTTP client configuration. The tool is designed to inspect and test TLS settings on various servers, including legacy ones, and needs the flexibility to connect to servers with older TLS versions.
Applied to files:
internal/requests/requests_test.go
🧬 Code graph analysis (1)
internal/requests/requests_test.go (1)
internal/requests/requests.go (8)
Host(75-78)URI(71-71)RequestConfig(85-103)NewRequestsMetaConfig(132-143)NewRequestHTTPClient(268-286)RequestHTTPClient(105-110)RequestsMetaConfig(122-130)RequestHeader(80-83)
🪛 ast-grep (0.40.0)
internal/requests/requests_test.go
[warning] 1155-1157: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
RootCAs: caCertPool,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🔇 Additional comments (2)
internal/requests/requests_test.go (2)
166-203: LGTM! Well-structured test forSetRequests.The new test properly validates the
SetRequestsmethod with comprehensive test data including hosts and URIs. Error handling is correct withrequire.NoError, and the loop variable capture is properly done.
1155-1157: Reminder: ConsiderMinVersionenforcement in TLS test configurations (deferred).The static analysis tool flags the missing
MinVersionin the TLS configuration. As noted in previous discussions, the https-wrench tool intentionally omits minimum TLS version constraints to maintain flexibility when testing legacy servers and older TLS implementations.You previously asked to be reminded about potentially adding
MinVersionenforcement to test code once legacy server compatibility becomes less critical.Based on learnings, this is intentionally deferred but tracked for future consideration.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
internal/requests/requests_test.go (5)
522-529: Simplify timeout assertion by avoiding unnecessary type assertion.You can read
c.client.Timeoutdirectly astime.Durationinstead of going throughany+ type assertion:- var i any = c.client.Timeout - - duration, ok := i.(time.Duration) - if !ok { - t.Fatalf("expected time.Duration, got %T", c.client.Timeout) - } - - assert.Equal(t, time.Duration(tt)*time.Second, duration) + assert.Equal(t, + time.Duration(tt)*time.Second, + c.client.Timeout, + )This keeps the test simpler and avoids dead code paths.
1259-1339:TestPrintRequestDebugnow cleanly asserts success vs error paths.The addition of
expectErrplus explicitrequire.Error/require.NoErrorbranches makes the behavior around nil/incomplete requests much clearer and prevents silent passes.
337-352: Make type assertions fatal to avoid potential nil derefs.If the type assertions on
rcClient.clientorrcClient.client.Transportever fail, the test will still proceed and may panic when accessing fields onclient/transport. Make these checks fatal so the test fails cleanly instead of panicking.- var i any = rcClient.client - - client, ok := i.(*http.Client) - if !ok { - t.Errorf("expecting *http.Client, got %T", client) - } + var i any = rcClient.client + + client, ok := i.(*http.Client) + require.Truef(t, ok, "expecting *http.Client, got %T", i) @@ - var ti any = rcClient.client.Transport - - transport, ok := ti.(*http.Transport) - if !ok { - t.Errorf("expecting *http.Transport, got %T", transport) - } + var ti any = rcClient.client.Transport + + transport, ok := ti.(*http.Transport) + require.Truef(t, ok, "expecting *http.Transport, got %T", ti)This keeps the test stable even if the implementation changes the underlying types.
Also applies to: 379-385
1113-1192: Strengthen non-verbose assertions and drop debug printing inTestPrintResponseDebug.Two small tweaks would make this test more robust:
- Always assert the non-verbose path is quiet.
Right now theassert.Emptyis guarded bylen(got) == 0, so ifResponseDebug=falseever starts producing output, the assertion is skipped instead of failing. You can make the behavior explicit:- got := buffer.String() - fmt.Printf("got:\n%s\n", got) - - if !tt.verbose && len(got) == 0 { - assert.Empty(t, - buffer.Bytes(), - "check PrintResponseDebug with verbose False", - ) - } - - if tt.verbose { + got := buffer.String() + + if !tt.verbose { + assert.Empty(t, + got, + "PrintResponseDebug should not write output when ResponseDebug is false", + ) + } else { for _, output := range tt.outputs { assert.True(t, bytes.Contains(buffer.Bytes(), []byte(output)), @@ - } - } + } + }
- Remove
fmt.Printffrom the test.
Once assertions cover the behavior, thefmt.Printf("got:\n%s\n", got)line becomes unnecessary noise in test output.As a future-hardening note only: when you eventually tighten TLS policies in tests, this is one of the
tls.Config{ RootCAs: caCertPool }spots where addingMinVersioncould reflect your intended baseline, once legacy compatibility is less critical. Based on learnings, this is intentionally deferred for now.
1418-1425: Fail fast onprocessHTTPRequestsByHosterrors and guardr.Errorbefore use.To make this test more robust and avoid panics on implementation changes:
- Treat top-level errors as fatal.
- respList, err := processHTTPRequestsByHost( + respList, err := processHTTPRequestsByHost( tt.reqConf, tt.pool, tt.verbose, ) - if err != nil { - t.Error(err) - } + require.NoError(t, err, "processHTTPRequestsByHost should not return a top-level error")
- Guard
r.Errorin the error-path case.- if tt.respStatusCode == 0 { - assert.Equal(t, - tt.errMsg, - r.Error.Error(), - "check Response Error", - ) - } + if tt.respStatusCode == 0 { + require.Error(t, + r.Error, + "expected ResponseData.Error to be set for %q", + tt.reqConf.Name, + ) + assert.Equal(t, + tt.errMsg, + r.Error.Error(), + "check Response Error", + ) + }
- Optional: drop the
fmt.Printf("resp type: %T\n", r)to keep test output clean once assertions cover behavior.These changes reduce the chance of panics and make failures easier to interpret.
Also applies to: 1427-1442
🧹 Nitpick comments (2)
internal/requests/requests_test.go (2)
972-987: Assert onSetTransportOverride/SetProxyProtocolHeadererrors in the server test.In this happy-path server test, failures from the setters should fail the test explicitly instead of being ignored:
- c := NewRequestHTTPClient() - c.SetTransportOverride(transportURL) - c.SetProxyProtocolHeader(header) + c := NewRequestHTTPClient() + + _, err = c.SetTransportOverride(transportURL) + require.NoError(t, err, "SetTransportOverride should succeed in proxy-proto server test") + + _, err = c.SetProxyProtocolHeader(header) + require.NoError(t, err, "SetProxyProtocolHeader should succeed in proxy-proto server test")This will surface regressions in the configuration helpers more clearly.
1194-1238:NilResponsebehavior covered; malformed-response TODO can wait.The
NilResponsesubtest correctly asserts that no output is produced whenResponseis nil. The commented-outMalformedResponsescenario is a good placeholder—happy to help flesh that out later if you decide to assert on the warning path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/requests/requests_test.go(18 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:34.922Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
📚 Learning: 2025-11-29T14:05:34.922Z
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:34.922Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
Applied to files:
internal/requests/requests_test.go
📚 Learning: 2025-11-08T16:00:48.187Z
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 3
File: internal/requests/requests.go:164-182
Timestamp: 2025-11-08T16:00:48.187Z
Learning: In the https-wrench tool (xenOs76/https-wrench repository), do not enforce minimum TLS version constraints in the HTTP client configuration. The tool is designed to inspect and test TLS settings on various servers, including legacy ones, and needs the flexibility to connect to servers with older TLS versions.
Applied to files:
internal/requests/requests_test.go
🪛 ast-grep (0.40.0)
internal/requests/requests_test.go
[warning] 1156-1158: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
RootCAs: caCertPool,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🔇 Additional comments (4)
internal/requests/requests_test.go (4)
56-57: Nice use of per-iteration copies witht.Parallel.Capturing
tcintottbeforet.Runfixes the loop-variable capture pitfall and makes these parallel table tests robust.Also applies to: 72-73, 98-99, 144-145, 188-191, 273-275, 332-335, 414-415, 480-483, 511-515, 580-582, 641-643, 671-673, 754-756, 781-783, 852-853, 949-952, 1087-1091, 1140-1143, 1317-1320, 1401-1405
166-203: Good targeted coverage forSetRequests.The test exercises a non-trivial
[]RequestConfig(including hosts and flags) and validates thatSetRequestsfully preserves structure viacmp.Diff; this nicely anchors the new API.
1033-1082: Good negative coverage forSetProxyProtocolHeader.The three subtests (nil client, malformed client, missing
transportAddress) nicely pin down the expected error messages and guard against subtle regressions in proxy-protocol setup.
1084-1111:TestPrintCmdassertions look solid.Asserting on semantic content in verbose mode and
assert.Emptywhen not verbose is a good fit for the writer-based API and keeps the test resilient to styling changes.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.