Add Alive Proxy into Options #5903
Conversation
- Provides ability to pass diff proxy in single nuclei instance using sdk
WalkthroughThe changes in this pull request involve a refactoring of proxy handling within the application. A new constant Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)pkg/reporting/trackers/github/github.go (1)
Complete the type assertion safety checks. While the safety check for Apply this diff to add complete safety checks: if options.HttpClient != nil && options.HttpClient.HTTPClient != nil {
if tcTransport, ok := tc.Transport.(*http.Transport); ok {
- tcTransport.Proxy = options.HttpClient.HTTPClient.Transport.(*http.Transport).Proxy
+ if clientTransport, ok := options.HttpClient.HTTPClient.Transport.(*http.Transport); ok {
+ tcTransport.Proxy = clientTransport.Proxy
+ }
}
}🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
pkg/protocols/headless/engine/http_client.go (1)
Line range hint
64-68: Fix error variable shadowing in SOCKS proxy error handlingThere's a potential bug where the wrong error is being returned in the SOCKS proxy error handling.
Apply this fix:
socksURL, proxyErr := url.Parse(options.AliveSocksProxy) if proxyErr != nil { - return nil, err + return nil, proxyErr }pkg/types/types.go (2)
97-98: Consider renaming field to follow Go naming convention for HTTPThe field name
AliveHttpProxyshould beAliveHTTPProxyto follow Go's naming convention where HTTP should be capitalized.- AliveHttpProxy string + AliveHTTPProxy string
97-100: Improve field documentationThe current comments could be more descriptive about the purpose and usage of these fields.
- // AliveProxy is the alive proxy to use - AliveHttpProxy string - // AliveSocksProxy is the alive socks proxy to use - AliveSocksProxy string + // AliveHTTPProxy specifies the HTTP proxy URL to be used for individual requests + // This allows for isolated proxy configurations in concurrent scenarios + AliveHTTPProxy string + // AliveSocksProxy specifies the SOCKS proxy URL to be used for individual requests + // This allows for isolated proxy configurations in concurrent scenarios + AliveSocksProxy stringinternal/runner/runner.go (1)
185-185: Improve condition readabilityThe condition structure could be clearer by grouping the proxy checks.
- if options.ProxyInternal && options.AliveHttpProxy != "" || options.AliveSocksProxy != "" { + if options.ProxyInternal && (options.AliveHttpProxy != "" || options.AliveSocksProxy != "") {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
internal/runner/proxy.go(2 hunks)internal/runner/runner.go(1 hunks)lib/sdk_private.go(1 hunks)pkg/protocols/common/protocolstate/state.go(1 hunks)pkg/protocols/headless/engine/engine.go(1 hunks)pkg/protocols/headless/engine/http_client.go(1 hunks)pkg/protocols/http/httpclientpool/clientpool.go(2 hunks)pkg/reporting/trackers/github/github.go(1 hunks)pkg/reporting/trackers/gitlab/gitlab.go(1 hunks)pkg/reporting/trackers/linear/linear.go(1 hunks)pkg/types/proxy.go(0 hunks)pkg/types/types.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/types/proxy.go
🔇 Additional comments (9)
internal/runner/proxy.go (2)
17-19: LGTM: Good package encapsulation
Moving the HTTP_PROXY_ENV constant into the runner package improves encapsulation and reduces dependencies on the types package.
59-62: LGTM: Improved proxy configuration handling
The change from global variables to options-based configuration improves thread safety for concurrent requests. This aligns well with the PR's objective of fixing concurrent request issues.
pkg/protocols/headless/engine/http_client.go (1)
59-62: LGTM: Thread-safe proxy configuration
The change to use options-scoped proxy configuration improves thread safety and aligns with the PR's objective.
pkg/protocols/headless/engine/engine.go (1)
76-77: LGTM: Proper integration with Chrome launcher
The change correctly integrates the new proxy configuration with the Chrome launcher while maintaining thread safety.
pkg/reporting/trackers/gitlab/gitlab.go (1)
104-109: LGTM! Improved error handling.
The error handling has been simplified by removing the unused response variable while maintaining the same functionality.
pkg/protocols/common/protocolstate/state.go (1)
112-113: LGTM! Verify proxy initialization.
The change from global ProxySocksURL to options.AliveSocksProxy improves concurrent request handling by isolating proxy configurations. This aligns with the PR objective of preventing shared proxy issues.
✅ Verification successful
Let me verify the initialization by checking where the AliveSocksProxy is set in the codebase.
Let me check how the proxy is being initialized by looking at the proxy setup code.
Let me try a different approach to check the proxy initialization code.
Based on the code analysis, I can now provide the final response:
LGTM! Proxy initialization is properly handled.
The AliveSocksProxy is correctly initialized in the loadProxyServers function, which:
- Validates the proxy URL before assignment
- Only sets
AliveSocksProxyfor SOCKS5 scheme - Verifies proxy availability using
GetAnyAliveProxy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that AliveSocksProxy is properly initialized before use
rg -A 2 "AliveSocksProxy.*=.*"
Length of output: 1833
Script:
#!/bin/bash
# Check for AliveSocksProxy struct field definition
ast-grep --pattern 'type $_ struct {
$$$
AliveSocksProxy $_
$$$
}'
# Check for any direct assignments to AliveSocksProxy
rg "\.AliveSocksProxy\s*=" -A 2
Length of output: 28038
Script:
#!/bin/bash
# Check the proxy initialization code
rg -A 10 "func.*proxy" internal/runner/proxy.go
Length of output: 49
Script:
#!/bin/bash
# Check the proxy setup code in the runner package
rg -A 10 "parseProxyURL|configureProxy" internal/runner/
# Also check for any proxy-related function
rg -A 10 "proxy" internal/runner/proxy.go
Length of output: 1562
pkg/reporting/trackers/linear/linear.go (1)
63-65: LGTM! Custom HTTP client transport is correctly handled.
The implementation properly reuses the transport from the provided HttpClient, maintaining connection settings and proxy configurations.
pkg/protocols/http/httpclientpool/clientpool.go (1)
160-163: LGTM! Proxy configuration priority is correctly implemented.
The implementation properly prioritizes HTTP proxy over SOCKS proxy and maintains the FastDialer fallback.
internal/runner/runner.go (1)
Line range hint 97-100: Verify consistent proxy field usage across codebase
Let's verify that all references to the new proxy fields are consistent across the codebase.
✅ Verification successful
Based on the search results, I can now generate the final response:
Proxy configuration usage is consistent across codebase
The proxy field references are consistently used throughout the codebase:
- Defined in
pkg/types/types.go - Properly set in
internal/runner/proxy.go - Correctly utilized in HTTP client configurations in
pkg/protocols/http/httpclientpool/clientpool.go - Properly implemented in headless engine in
pkg/protocols/headless/engine/ - Correctly handled in protocol state management in
pkg/protocols/common/protocolstate/state.go
The remaining ProxyURL references are standard Go HTTP client function calls, not related to the configuration fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all occurrences of AliveHttpProxy and AliveSocksProxy
echo "Searching for proxy field references..."
rg "AliveHttpProxy|AliveSocksProxy" --type go
# Search for any remaining global proxy URL references that should be migrated
echo "Searching for potential remaining global proxy references..."
rg "ProxyURL|ProxySocksURL" --type go
Length of output: 2561
| @@ -120,7 +120,7 @@ func (e *NucleiEngine) init(ctx context.Context) error { | |||
| _ = protocolinit.Init(e.opts) | |||
| }) | |||
|
|
|||
| if e.opts.ProxyInternal && types.ProxyURL != "" || types.ProxySocksURL != "" { | |||
| if e.opts.ProxyInternal && e.opts.AliveHttpProxy != "" || e.opts.AliveSocksProxy != "" { | |||
There was a problem hiding this comment.
Fix the logical operator in proxy condition.
The condition combines multiple checks with incorrect operator precedence which could lead to unintended behavior.
Apply this diff to fix the condition:
-if e.opts.ProxyInternal && e.opts.AliveHttpProxy != "" || e.opts.AliveSocksProxy != "" {
+if e.opts.ProxyInternal && (e.opts.AliveHttpProxy != "" || e.opts.AliveSocksProxy != "") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if e.opts.ProxyInternal && e.opts.AliveHttpProxy != "" || e.opts.AliveSocksProxy != "" { | |
| if e.opts.ProxyInternal && (e.opts.AliveHttpProxy != "" || e.opts.AliveSocksProxy != "") { |
| @@ -182,7 +182,7 @@ func New(options *types.Options) (*Runner, error) { | |||
| runner.catalog = disk.NewCatalog(config.DefaultConfig.TemplatesDirectory) | |||
|
|
|||
| var httpclient *retryablehttp.Client | |||
| if options.ProxyInternal && types.ProxyURL != "" || types.ProxySocksURL != "" { | |||
| if options.ProxyInternal && options.AliveHttpProxy != "" || options.AliveSocksProxy != "" { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update field name for consistency
Once the field name in types.go is updated to AliveHTTPProxy, this reference should be updated as well.
- if options.ProxyInternal && options.AliveHttpProxy != "" || options.AliveSocksProxy != "" {
+ if options.ProxyInternal && options.AliveHTTPProxy != "" || options.AliveSocksProxy != "" {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if options.ProxyInternal && options.AliveHttpProxy != "" || options.AliveSocksProxy != "" { | |
| if options.ProxyInternal && options.AliveHTTPProxy != "" || options.AliveSocksProxy != "" { |
This PR addresses an issue where concurrent requests in a single Nuclei instance shared the same proxy due to global variable usage. By moving proxy configurations to individual request options, we enable isolated and parallel request handling, improving flexibility and reliability in SDK mode operations.
Proposed changes
Checklist
Summary by CodeRabbit
New Features
AliveHttpProxyandAliveSocksProxy.Bug Fixes
Refactor
Chores