Conversation
WalkthroughRefactors output to accept io.Writer across request and certificate rendering, adds error handling for URL retrieval/parsing, enhances TLS test configuration, and introduces a comprehensive test suite for request/TLS/URL behaviors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
🧹 Nitpick comments (6)
internal/requests/requests_handlers.go (1)
279-331: Consider acceptingio.Writerfor consistency.
PrintResponseDatastill writes directly to stdout viafmt.Println, while other rendering functions (RenderTLSData,CertsToTables,HandleRequests) now accept anio.Writer. For consistent testability and output control, consider updating this method to also accept a writer parameter.internal/requests/main_test.go (1)
214-237: Consider addingMinVersionto the TLS configuration.The static analysis tool flags a missing
MinVersion. While this is test code and the default TLS 1.3 cipher suites effectively constrain the minimum version, explicitly settingMinVersionis a best practice and silences the warning.+ // Set default TLS MinVersion to 1.3 + var tlsMinVersion uint16 = tls.VersionTLS13 + ts.TLS = &tls.Config{ Certificates: []tls.Certificate{cert}, CipherSuites: tlsCipherSuites, + MinVersion: tlsMinVersion, MaxVersion: tlsMaxVersion, }If you need to test with older TLS versions, you could add a
tlsMinVersionfield todemoHttpServerDatasimilar totlsMaxVersion.internal/requests/requests_handlers_test.go (4)
186-201: Understand and document whyt.Parallel()breaksTestFilterResponseHeadersYou’ve commented that this test fails when
t.Parallel()is enabled, but the current assertions only check membership in the returned slice, which should be order-agnostic. That suggests either hidden shared state or some other concurrency-sensitive behavior infilterResponseHeadersor its dependencies.It would be good to either:
- Track down and fix any shared mutable state so the test can safely use
t.Parallel(), or- Expand the comment (or add a TODO) to briefly describe the concrete reason parallelization is unsafe, so future readers don’t accidentally re-enable it and reintroduce a flaky scenario.
293-310: Avoid strict equality onerr.Error()inTestTransportAddressFromURLStringThe
"Invalid"case asserts exact equality onerr.Error(), which couples this test to the precise error message formatting fromnet/url(including quoting and escape details). Minor changes in Go’s implementation could break the test even though behavior is still correct.You could make this more robust by, for example:
require.Error(t, err) assert.ErrorContains(t, err, "invalid URL escape")or asserting on a stable prefix/suffix you control, rather than the full string.
382-441:TestRenderTLSData: strong coverage; consider avoiding fixed ports if possibleThis test does a nice job exercising TLS version/cipher rendering, certificate output, and the
resp.TLS == nilbranch using an in-process HTTPS server and a buffer-backed writer.If
NewHTTPSTestServercan support binding to:0, you might consider switching from hard-coded ports (46101–46103) to ephemeral ports derived fromts.URL(and wiring that intoTransportOverrideURL). That would slightly reduce the risk of port-collision flakes when tests are run in parallel or in different environments.
493-537: TightenTestHandleRequestsstructure around the error pathThe success-path assertions on response map keys, request
Name, and effectiveUser-Agentpropagation look good and exercise the updatedHandleRequestsAPI well.For the error-path case, after
require.Error(t, err)you still:
- Assert on
out := buffer.String()and- Range over
respMap.This is safe today (ranging over a
nilmap is a no-op, andPrintCmdruns before the loop), but for clarity and future-proofing it might be cleaner to:if tt.expectErr { require.Error(t, err) return } require.NoError(t, err) // assertions on out and respMap...so future changes to
HandleRequests’s error behavior can’t accidentally make the test read half-populated state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
devenv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
cmd/requests.go(1 hunks)devenv.nix(2 hunks)devenv.yaml(1 hunks)internal/certinfo/certinfo_handlers.go(6 hunks)internal/requests/main_test.go(2 hunks)internal/requests/requests.go(1 hunks)internal/requests/requests_handlers.go(6 hunks)internal/requests/requests_handlers_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-15T17:16:55.075Z
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 5
File: devenv.nix:569-569
Timestamp: 2025-11-15T17:16:55.075Z
Learning: In the xenOs76/https-wrench repository, the devenv.nix test environment is local-only without GitHub CI integration, so automatic dependency updates in enterTest are acceptable for the maintainer's workflow.
Applied to files:
devenv.nixdevenv.yaml
🧬 Code graph analysis (3)
internal/requests/requests_handlers_test.go (2)
internal/requests/requests.go (5)
ResponseHeader(72-72)URI(71-71)Host(75-78)RequestConfig(85-103)RequestsMetaConfig(122-130)internal/requests/requests_handlers.go (3)
TLSVersionName(39-54)RenderTLSData(333-365)HandleRequests(206-225)
internal/requests/requests_handlers.go (4)
internal/requests/requests.go (2)
Host(75-78)ResponseData(112-120)internal/style/style.go (5)
CertKeyP4(61-63)CertValue(69-70)ItemKeyP3(46-48)Error(100-101)LGDefBorder(13-13)internal/style/style_handlers.go (1)
LgSprintf(21-26)internal/certinfo/certinfo_handlers.go (1)
CertsToTables(166-221)
cmd/requests.go (1)
internal/requests/requests_handlers.go (1)
HandleRequests(206-225)
🪛 ast-grep (0.40.0)
internal/requests/main_test.go
[warning] 232-236: 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{
Certificates: []tls.Certificate{cert},
CipherSuites: tlsCipherSuites,
MaxVersion: tlsMaxVersion,
}
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 (16)
devenv.nix (1)
54-59: LGTM: Good documentation of disabled feature.The expanded comment clearly documents why the Go language configuration is disabled (version alignment with vim) and provides a reference URL. This is helpful for future maintainers.
devenv.yaml (1)
6-7: Thenixos-25.05branch exists in the NixOS/nixpkgs repository and follows the correct YY.MM versioning scheme. No action required.internal/requests/requests.go (1)
571-574: LGTM! Proper error handling for URL retrieval.The addition of error handling for
getUrlsFromHostis a good improvement. The error is properly checked and propagated up the call stack, ensuring invalid URIs are caught early rather than causing issues later in the request processing.internal/certinfo/certinfo_handlers.go (3)
13-13: LGTM!Import added for
io.Writersupport.
66-66: LGTM! Consistent writer parameter usage.All call sites for
CertsToTablesare correctly updated to passos.Stdoutas the writer, maintaining existing behavior while enabling future flexibility for testing and alternative output destinations.Also applies to: 100-100, 120-120
166-218: LGTM! Writer-based output for certificate tables.The refactored
CertsToTablesfunction now accepts anio.Writer, making it testable and flexible. The output is correctly written usingfmt.Fprintln(w, ...).cmd/requests.go (1)
97-100: LGTM!The call site is correctly updated to pass
os.Stdoutas the writer parameter, aligning with the updatedHandleRequestssignature.internal/requests/requests_handlers.go (7)
29-37: LGTM!The comment clarifies that URIs must start with a slash, and the regex validation is appropriate for this requirement.
56-63: LGTM!Good check to handle unknown cipher suites by detecting the hex format indicator.
65-96: LGTM! Better function naming.Renaming from
parseResponseHeaderstofilterResponseHeadersmore accurately describes the function's purpose of filtering headers based on the provided filter list.
98-118: LGTM! Proper error propagation for invalid URIs.The function now returns an error when URI parsing fails, allowing callers to handle invalid configurations gracefully rather than silently proceeding with malformed URLs.
120-145: LGTM! Improved URL parsing robustness.The function now handles edge cases:
- Returns an error for empty input
- Auto-prefixes
https://when the scheme is missing- Appends default port
:443when not specifiedThis makes the function more forgiving of user input variations.
206-225: LGTM! Writer-based output for request handling.The
HandleRequestsfunction now accepts anio.Writer, enabling testability and flexible output destinations.
333-364: LGTM! Writer-based TLS data rendering.The
RenderTLSDatafunction correctly uses the provided writer for all output operations, enabling consistent output control and testability.internal/requests/main_test.go (1)
35-41: LGTM! Configurable TLS parameters for test server.The new fields
tlsCipherSuitesandtlsMaxVersionenable flexible TLS configuration in tests, allowing verification of behavior with different TLS settings.internal/requests/requests_handlers_test.go (1)
15-143: Solid table-driven unit tests for core helpersThe tests for
ResponseHeader.String,URI.Parse,TLSVersionName, andcipherSuiteNameare clean table-driven subtests, cover both normal and unknown values, and correctly usett := tcwitht.Parallel()to avoid closure capture issues. Nothing blocking here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/requests/requests_handlers_test.go (4)
15-34: BroadenTestResponseHeader_Printtable for future-proofingThe test is fine as-is, but it only exercises a single value. Consider adding a couple more table entries (e.g., empty header, header with special characters) so the behavior is documented a bit more robustly if
ResponseHeader.String()ever changes.
36-60: Minor simplification inTestURI_ParseWithin each subtest you can call
Parse()once and assert on the result instead of having two branches that each calltt.uri.Parse(). This slightly reduces duplication and makes the intent clearer:- if tt.expectValid { - assert.True(t, tt.uri.Parse()) - } - - if !tt.expectValid { - assert.False(t, tt.uri.Parse()) - } + got := tt.uri.Parse() + assert.Equal(t, tt.expectValid, got)
145-202: Investigate whyt.Parallel()breaksTestFilterResponseHeadersThe comment that the test fails when
t.Parallel()is enabled suggests some shared mutable state, either infilterResponseHeadersitself or in the header values passed in. Ideally this function should be side‑effect free on its inputs and safe to exercise in parallel.If the implementation mutates input headers or uses shared globals, consider:
- Making it pure (no mutation / shared state), or
- Cloning the headers inside the test cases so each subtest has isolated data,
and then re‑enable
t.Parallel()here.
253-314: Relax exact error-string match forInvalidURL caseFor the
"Invalid"case you assert full equality onerr.Error(), including the exactnet/urlparse message. This can be brittle across Go versions or subtle changes inurl.Parseformatting.To make the test more stable while still checking behavior, consider asserting on key substrings instead, e.g.:
- if tt.expectError { - require.Error(t, err) - assert.Equal(t, tt.expectOutput, err.Error()) - } + if tt.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid URL escape") + }and keep the exact string check only for errors you fully control (like the
"EmptyInput"case).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
devenv.nix(2 hunks)internal/requests/requests_handlers_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- devenv.nix
🔇 Additional comments (5)
internal/requests/requests_handlers_test.go (5)
62-104: Good coverage ofTLSVersionName, including unknown versionThe table-driven test nicely exercises all known versions plus the unknown-code path, and using
assert.Containsgives some resilience against formatting changes. No issues here.
106-143:TestCipherSuiteNamecleanly captures known/unknown pathsThis test clearly pins expected names for representative cipher suites and the unknown fallback; it’s straightforward and reads well. Looks good as written.
204-252:TestGetUrlsFromHosterror-path contract is now explicitDropping
expectedOutputfor the"OneNotParsing"case resolves the earlier ambiguity: the test now clearly documents that, on parse error, you only assert that an error is returned and leave the output unspecified. The success cases look correct and cover both “with URIs” and “no URI list” behaviors.
316-437: Solid end-to-end TLS rendering test; covers happy-path and no-TLS stateThis test does a good job of exercising
RenderTLSDataagainst real TLS connections, asserting on version, cipher suite, cert fields, and the “no TLS state” branch by zeroingResponse.TLS. The use of per-case ports andt.Parallel()is appropriate here. No changes needed from my side.
439-535:TestHandleRequestsnicely validates mapping, output, and User-Agent behaviorThe test covers both success and error meta-configs, asserts that some request summary is written to the provided writer, and checks that
respMapkeys line up withRequest.Nameand that the User-Agent is either the default or overridden value. This gives good coverage of the higher-level flow; I don’t see any correctness issues here.
1 participant
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.