chore/code-cleanup #173
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
| "bytes" | ||
| "context" | ||
| "encoding/json" | ||
| "errors" | ||
| "fmt" | ||
| "io" | ||
| "mime" | ||
|
|
@@ -248,7 +249,10 @@ | |
|
|
||
| setFetchHeaders(request) | ||
|
|
||
| client, closeIdleConnections := fetchTool.httpClientWithRedirectValidation(requestCtx) | ||
| defer closeIdleConnections() | ||
|
|
||
| response, err := client.Do(request) | ||
| if err != nil { | ||
| return nil, fetchResponseInfo{}, oops.In("tool").Code("fetch_request").Wrapf(err, "fetch url") | ||
| } | ||
|
|
@@ -298,11 +302,17 @@ | |
| return http.DefaultClient | ||
| } | ||
|
|
||
| func (fetchTool *FetchTool) httpClientWithRedirectValidation( | ||
| ctx context.Context, | ||
| ) (client *http.Client, closeIdleConnections func()) { | ||
| baseClient := fetchTool.httpClient() | ||
| clonedClient := *baseClient | ||
|
|
||
| transport, closeIdleConnections := fetchTool.transportWithNetworkValidation(baseClient.Transport) | ||
| clonedClient.Transport = transport | ||
|
|
||
| baseCheckRedirect := baseClient.CheckRedirect | ||
| clonedClient.CheckRedirect = func(request *http.Request, via []*http.Request) error { | ||
| if baseCheckRedirect != nil { | ||
| if err := baseCheckRedirect(request, via); err != nil { | ||
| return err | ||
|
|
@@ -314,7 +324,86 @@ | |
| return fetchTool.validatePublicFetchURL(ctx, request.URL) | ||
| } | ||
|
|
||
| return &clonedClient, closeIdleConnections | ||
| } | ||
|
|
||
| func (fetchTool *FetchTool) transportWithNetworkValidation( | ||
| baseTransport http.RoundTripper, | ||
| ) (roundTripper http.RoundTripper, closeIdleConnections func()) { | ||
| if fetchTool.allowPrivateNetworks { | ||
| return baseTransport, func() {} | ||
|
Check failure on line 334 in internal/tool/fetch.go
|
||
| } | ||
|
|
||
| transport, ok := cloneFetchHTTPTransport(baseTransport) | ||
| if !ok { | ||
| return baseTransport, func() {} | ||
|
Check failure on line 339 in internal/tool/fetch.go
|
||
| } | ||
|
|
||
| transport.Proxy = nil | ||
| transport.DialContext = validatingFetchDialContext(fetchDialContext(transport)) | ||
|
|
||
| if transport.DialTLSContext != nil { | ||
| transport.DialTLSContext = validatingFetchDialContext(transport.DialTLSContext) | ||
| } | ||
|
Comment on lines
+342
to
+347
There was a problem hiding this comment. 🔒 Security & Privacy | 🟠 Major 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Description: Find custom http.Transport dial hooks that need preservation/validation.
# Expectation: Any Dial/DialTLS usage should remain behaviorally equivalent after wrapping.
rg -nP --type=go -C3 '\bDial(TLS|TLSContext|Context)?\s*:' .Repository: omarluq/librecode Length of output: 748 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Inspect the implementation around the cited lines and related helpers/tests.
git ls-files 'internal/tool/fetch.go' 'internal/tool/fetch_internal_test.go'
wc -l internal/tool/fetch.go internal/tool/fetch_internal_test.go
sed -n '320,390p' internal/tool/fetch.go
printf '\n--- tests around transport setup ---\n'
sed -n '480,560p' internal/tool/fetch_internal_test.go
printf '\n--- search for legacy dial hooks in repo ---\n'
rg -n --type=go '\bDialTLSContext\b|\bDialTLS\b|\bDial\b' internal/toolRepository: omarluq/librecode Length of output: 4752 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Look for other cloning/sanitizing logic for http.Transport in the codebase.
rg -n --type=go 'Clone\(\)|\bProxy\s*=\s*nil|\bDialContext\s*=\s*validatingFetchDialContext|\bvalidateFetchDialedConnection\b' internalRepository: omarluq/librecode Length of output: 1141 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Read the implementation and helper definitions that matter for the transport wrapping logic.
sed -n '300,420p' internal/tool/fetch.go
printf '\n---\n'
sed -n '1,140p' internal/tool/fetch.goRepository: omarluq/librecode Length of output: 7693 🌐 Web query:
💡 Result: The net/http.Transport.Clone method performs a shallow copy of the Transport's exported fields, which includes the legacy Dial, DialTLS, and modern DialTLSContext hooks [1][2]. Specifically regarding these hooks: - The fields Dial, DialTLS, and DialTLSContext are copied directly from the original Transport to the new one [2]. - Because they are function types, the cloned Transport will point to the exact same dialer functions as the original Transport [2]. - There is no special validation logic for these hooks during the cloning process [2]. Regarding DialTLSContext behavior specifically: - The Transport prioritizes DialTLSContext over the deprecated DialTLS if both are configured [3][2]. - When you clone a Transport, this priority logic remains preserved in the new instance because the fields themselves are copied as-is [2]. It is important to note that while Clone is intended to create a usable copy, it has historically been subject to various edge-case issues—particularly concerning internal state like HTTP/2 configuration (TLSNextProto and ForceAttemptHTTP2) [4][5]. Users should be aware that if the original Transport has not yet been initialized (via a request), the cloned Transport might behave differently than expected due to lazy initialization of default values [5]. Always ensure the Transport state is fully configured if you intend to rely on specific protocol or dialer behaviors after cloning [4][5]. Citations:
🌐 Web query:
💡 Result: In the Go net/http package, the DialTLSContext and DialTLS fields on the http.Transport struct manage TLS connection dialing for HTTPS requests, with specific precedence rules [1][2]. Precedence and Behavior: 1. If DialTLSContext is provided, it takes precedence [1][2]. 2. If DialTLSContext is nil, the transport falls back to the deprecated DialTLS [1][2]. 3. If both DialTLSContext and DialTLS are nil, the transport uses the configured DialContext (or the deprecated Dial) in combination with the Transport's TLSClientConfig to establish the connection [1][2]. 4. When DialTLSContext is set, the Dial and DialContext hooks are ignored for HTTPS requests, as are the TLSClientConfig and TLSHandshakeTimeout fields [1][2]. The DialTLS and Dial hooks are deprecated in favor of DialTLSContext and DialContext, respectively, because the context-aware versions allow the transport to cancel dials immediately when they are no longer needed [1][2]. Clone Documentation: The Clone method on http.Transport returns a deep copy of the Transport's exported fields [3]. This is useful for creating a modified version of an existing transport without affecting the original [4][3]. The clone process performs a shallow copy of most configuration fields (such as dialers and timeout durations) while creating new, independent instances for fields that require deep copying to prevent shared-state side effects, such as the TLSClientConfig [3]. Note that the internal state—such as connection pools or synchronization primitives—is managed appropriately to ensure the cloned transport is safe for use independently [3]. Citations:
🌐 Web query:
💡 Result: In Go's net/http package, the Transport.Clone method creates a shallow copy of the Transport structure, specifically copying exported fields, including the dial hooks [1][2][3]. When Clone is called, both the deprecated DialTLS and the modern DialTLSContext hooks are copied from the original Transport to the new instance [1][2]. Regarding their behavior and validation: 1. Prioritization: When both DialTLSContext and the deprecated DialTLS are set on a Transport (whether original or cloned), DialTLSContext takes precedence [1][4][2]. 2. Legacy Hook Handling: The Transport continues to support the legacy DialTLS hook for backward compatibility [1][2]. However, the documentation explicitly recommends using DialTLSContext instead, as it allows for better control, such as canceling dials when they are no longer needed [1][4]. 3. Validation and Fallback: If DialTLSContext is nil and DialTLS is also nil, the Transport falls back to using the DialContext (or Dial) hook combined with the TLSClientConfig to initiate TLS connections [1][4]. 4. Connection Behavior: If DialTLSContext is provided, the Transport ignores the standard Dial/DialContext hooks for HTTPS requests, as well as the TLSClientConfig and TLSHandshakeTimeout [1][4][2]. The returned net.Conn is expected to be already established past the TLS handshake [1][4]. Users should be aware that while Clone copies these hooks, it does not perform validation to ensure they are mutually compatible or optimal; it simply replicates the configuration [1][3]. Furthermore, while Clone is useful for creating variations of a Transport, users have historically reported issues regarding the cloning of internal state (such as HTTP/2 configuration) when the original transport was auto-configured [5][6], so it is generally best practice to ensure the cloned Transport is explicitly configured if specific behavior is required. Citations:
Wrap legacy transport dial hooks too. 🤖 Prompt for AI Agents |
||
|
|
||
| return transport, transport.CloseIdleConnections | ||
| } | ||
|
|
||
| func cloneFetchHTTPTransport(baseTransport http.RoundTripper) (*http.Transport, bool) { | ||
| if baseTransport == nil { | ||
| baseTransport = http.DefaultTransport | ||
| } | ||
|
|
||
| transport, ok := baseTransport.(*http.Transport) | ||
| if !ok { | ||
| return nil, false | ||
| } | ||
|
|
||
| return transport.Clone(), true | ||
| } | ||
|
|
||
| func fetchDialContext(transport *http.Transport) func(context.Context, string, string) (net.Conn, error) { | ||
| if transport.DialContext != nil { | ||
| return transport.DialContext | ||
| } | ||
|
|
||
| dialer := &net.Dialer{} | ||
|
|
||
| return dialer.DialContext | ||
| } | ||
|
|
||
| func validatingFetchDialContext( | ||
| dialContext func(context.Context, string, string) (net.Conn, error), | ||
| ) func(context.Context, string, string) (net.Conn, error) { | ||
| return func(ctx context.Context, network, address string) (net.Conn, error) { | ||
| return validateFetchDialedConnection(dialContext(ctx, network, address)) | ||
| } | ||
| } | ||
|
|
||
| func validateFetchDialedConnection(conn net.Conn, dialErr error) (net.Conn, error) { | ||
| if dialErr != nil { | ||
| return nil, dialErr | ||
| } | ||
|
|
||
| if conn == nil { | ||
| return nil, oops.In("tool").Code("fetch_nil_connection").Errorf("fetch dial returned nil connection") | ||
| } | ||
|
|
||
| if err := validatePublicFetchRemoteAddr(conn.RemoteAddr()); err != nil { | ||
| if closeErr := conn.Close(); closeErr != nil { | ||
| return nil, errors.Join( | ||
| err, | ||
| oops.In("tool").Code("fetch_close_rejected_connection").Wrapf( | ||
| closeErr, | ||
| "close rejected fetch connection", | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| return nil, err | ||
| } | ||
|
|
||
| return conn, nil | ||
| } | ||
|
|
||
| func (fetchTool *FetchTool) validatePublicFetchURL(ctx context.Context, requestURL *url.URL) error { | ||
|
|
@@ -390,6 +479,37 @@ | |
| return nil | ||
| } | ||
|
|
||
| func validatePublicFetchRemoteAddr(remoteAddr net.Addr) error { | ||
| ipAddress := fetchRemoteAddrIP(remoteAddr) | ||
| if ipAddress == nil { | ||
| return oops.In("tool").Code("fetch_invalid_remote_address").Errorf("fetch remote address is not an IP") | ||
| } | ||
|
|
||
| return validatePublicFetchIP(ipAddress) | ||
| } | ||
|
|
||
| func fetchRemoteAddrIP(remoteAddr net.Addr) net.IP { | ||
| if remoteAddr == nil { | ||
| return nil | ||
| } | ||
|
|
||
| switch addr := remoteAddr.(type) { | ||
| case *net.TCPAddr: | ||
| return addr.IP | ||
| case *net.UDPAddr: | ||
| return addr.IP | ||
| case *net.IPAddr: | ||
| return addr.IP | ||
| } | ||
|
|
||
| host, _, err := net.SplitHostPort(remoteAddr.String()) | ||
| if err != nil { | ||
| host = remoteAddr.String() | ||
| } | ||
|
|
||
| return parseFetchHostIP(normalizedFetchHost(host)) | ||
| } | ||
|
|
||
| func isPrivateFetchIP(ipAddress net.IP) bool { | ||
| return ipAddress.IsLoopback() || | ||
| ipAddress.IsPrivate() || | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the intentional no-op closers.
CI is flagging both empty closures. Add a short comment inside each no-op so the fallback is explicit and Sonar passes.
Proposed fix
if fetchTool.allowPrivateNetworks { - return baseTransport, func() {} + return baseTransport, func() { + // No cloned transport is owned here, so there are no idle connections to close. + } } transport, ok := cloneFetchHTTPTransport(baseTransport) if !ok { - return baseTransport, func() {} + return baseTransport, func() { + // Non-http transports are not cloned here, so cleanup remains with the owner. + } }📝 Committable suggestion
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 334-334: Add a nested comment explaining why this function is empty or complete the implementation.
See more on https://sonarcloud.io/project/issues?id=omarluq_librecode&issues=AZ78qYofaN4rNUSQH_vU&open=AZ78qYofaN4rNUSQH_vU&pullRequest=173
[failure] 339-339: Add a nested comment explaining why this function is empty or complete the implementation.
See more on https://sonarcloud.io/project/issues?id=omarluq_librecode&issues=AZ78qYofaN4rNUSQH_vV&open=AZ78qYofaN4rNUSQH_vV&pullRequest=173
🤖 Prompt for AI Agents
Source: Linters/SAST tools