fix: URL parsing with lists & -scan-all-ips #5897
Conversation
WalkthroughThe changes made in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ListInputProvider
participant MetaInput
User->>ListInputProvider: Set(value)
ListInputProvider->>MetaInput: Set Input to trimmed URL
ListInputProvider->>User: Operation feedback (success or error)
User->>ListInputProvider: Del(value)
ListInputProvider->>MetaInput: Set Input to trimmed URL
ListInputProvider->>User: Operation feedback (success or error)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 0
🧹 Outside diff range and nitpick comments (1)
pkg/input/provider/list/hmap.go (1)
Line range hint
187-194: Consider enhancing error handling and loggingWhile the core functionality is correct, consider the following improvements:
- Make error messages more specific and structured
- Consider downgrading some Warning logs to Debug level
- Add structured logging fields for better filtering
Example improvement:
-gologger.Debug().Msgf("scanAllIps: dns resolution failed: %v", err) +gologger.Debug(). + Str("url", URL). + Str("error", err.Error()). + Msg("scan-all-ips: DNS resolution failed") -gologger.Warning().Msgf("target does not have ipv6 address falling back to ipv4 %v\n", err) +gologger.Debug(). + Str("url", URL). + Str("error", err.Error()). + Msg("IPv6 not available, falling back to IPv4")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
pkg/input/provider/list/hmap.go(1 hunks)
🔇 Additional comments (2)
pkg/input/provider/list/hmap.go (2)
Line range hint 32-38: Verify DNS resolution timeout handling
The DNS resolution in scan-all-ips mode could potentially lead to timeouts or hanging. Consider:
- Adding explicit timeouts for DNS resolution
- Implementing retry logic with backoff
- Adding metrics for DNS resolution failures
Let's check for existing timeout implementations:
Also applies to: 187-194
✅ Verification successful
Let me analyze the DNS resolution implementation further by checking the dialer configuration.
DNS resolution timeout is properly implemented
The DNS resolution is handled by fastdialer with appropriate timeout configurations:
- Default dial timeout is set to 10 seconds and can be customized
- The timeout is propagated through the dialer chain:
- Base dial timeout is configured in types.Timeouts
- Fastdialer uses this timeout for DNS operations
- Context cancellation is properly implemented for timeout handling
The implementation has proper timeout handling and does not require additional safeguards.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check DNS resolution timeout handling
# Look for timeout configurations and DNS resolution patterns
# Search for DNS timeout configurations
rg -B 5 -A 5 "GetDNSData"
# Search for context usage with DNS
rg -U "(?s)context.*?GetDNSData"
Length of output: 69592
Script:
#!/bin/bash
# Check DNS resolution timeout configuration in dialer
# Search for DNS timeout configurations in dialer
rg -B 5 -A 5 "fastdialer.*New"
# Search for DNS timeout related configurations
rg -B 2 -A 2 "DNSTimeout|DialTimeout|DialerTimeout"
Length of output: 3569
187-187: LGTM: Consistent usage of cleaned URL value
The change correctly uses the cleaned URL value (URL) instead of potentially uncleaned input, ensuring consistency with the URL cleaning done at the start of the method.
Let's verify the URL cleaning behavior across the codebase:
✅ Verification successful
Let me gather more information about the URL cleaning behavior by checking the code around this change.
Let me check the context around the specific line in question to understand the full scope of the change.
Based on the code context, I can now provide a final response:
LGTM: URL cleaning behavior is consistent
The change at line 187 using the cleaned URL value (URL) in metaInput.Input is correct and consistent with the codebase's URL handling pattern. The code shows that:
- URL cleaning via
strings.TrimSpace()is consistently applied in all input handling methods (Set, SetWithExclusions, Del) - The cleaned URL is used consistently for both regular inputs and when scan-all-ips is enabled
- The same cleaned URL value is used for all derived meta inputs (with different IPs)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent URL cleaning patterns
# Look for other instances where TrimSpace might be missing
# Search for URL assignments without TrimSpace
rg -U "(?s)(func.*?\{.*?URL\s*:=\s*[^T][^\n]*?\n)"
# Search for direct value usage without cleaning
rg "metaInput\.Input\s*=\s*[^U][^\n]*$"
Length of output: 106446
Script:
#!/bin/bash
# Look for URL cleaning patterns in the file
rg -A 5 -B 5 "URL\s*:=\s*strings\.TrimSpace" pkg/input/provider/list/hmap.go
# Look for URL variable assignments and usage
rg -A 5 -B 5 "URL\s*:=.*$" pkg/input/provider/list/hmap.go
Length of output: 1615
Script:
#!/bin/bash
# Look for the context around line 187 in the file
rg -A 10 -B 10 "metaInput\.Input\s*=\s*URL" pkg/input/provider/list/hmap.go
Length of output: 3871
|
Thanks so much for your contribution @p-l- ! :) |
|
Is there a reason why all the tests fail @GeorginaReeder? Is it related to this change? |
ehsandeep
requested review from
Mzack9999
and removed request for
dogancanbakir
Proposed changes
For some reason, the cleaned (with
strings.TrimSpace()) valueURLis not used when-scan-all-ipsis provided.Checklist
Summary by CodeRabbit
Bug Fixes
New Features