fix: improve URL parsing error handling in ListInputProvider#6448
Closed
oleveloper wants to merge 3 commits intoprojectdiscovery:devfrom
Closed
fix: improve URL parsing error handling in ListInputProvider#6448oleveloper wants to merge 3 commits intoprojectdiscovery:devfrom
oleveloper wants to merge 3 commits intoprojectdiscovery:devfrom
Conversation
- Add *types.Options field to ListInputProvider struct - Initialize options field in New() constructor - Enhance URL parsing error handling: - Change from debug messages to error messages - Distinguish single-target (-u) and batch (-l) modes - Single-target mode: exit immediately with os.Exit(1) - Batch mode: log error and skip invalid target - Apply consistent logic to both Set and Del functions Fixes projectdiscovery#6416
Contributor
WalkthroughAdds options to ListInputProvider, refactors error handling in Set/Del to branch on single-target vs batch mode, switches fmt errors to structured logging, adjusts deletion to use original input for IPs, and exits on invalid single-target inputs while skipping in batch mode. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant ListInputProvider as ListInputProvider
participant Options as Options
participant Logger as Logger
participant OS as OS
User->>ListInputProvider: Set(value)
ListInputProvider->>ListInputProvider: parse URL / validate hostname
alt invalid target
ListInputProvider->>Options: check TargetsFilePath / Targets
alt Single-target mode
ListInputProvider->>Logger: info("invalid target ...")
ListInputProvider->>OS: Exit(1)
else Batch mode
ListInputProvider->>Logger: warn("skipping invalid target ...")
ListInputProvider-->>User: return (skip)
end
else valid target
ListInputProvider-->>User: proceed (add)
end
sequenceDiagram
autonumber
participant User
participant ListInputProvider as ListInputProvider
participant Options as Options
participant Logger as Logger
participant OS as OS
User->>ListInputProvider: Del(value)
ListInputProvider->>ListInputProvider: parse (URL/IP)
alt invalid target
ListInputProvider->>Options: check TargetsFilePath / Targets
alt Single-target mode
ListInputProvider->>Logger: info("invalid target ...")
ListInputProvider->>OS: Exit(1)
else Batch mode
ListInputProvider->>Logger: warn("skipping invalid target ...")
ListInputProvider-->>User: return (skip)
end
else IP deletion
ListInputProvider->>ListInputProvider: metaInput.Input = value
ListInputProvider-->>User: proceed (delete)
else URL deletion
ListInputProvider-->>User: proceed (delete)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/input/provider/list/hmap.go (1)
448-451: Use the trimmed URL when setting metaInput.Input (consistency bug).Using value (untrimmed) can change the delItem key/parse behavior vs Set which uses URL (trimmed). This can make exclusions fail to match.
- metaInput := contextargs.NewMetaInput() - metaInput.Input = value + metaInput := contextargs.NewMetaInput() + metaInput.Input = URL metaInput.CustomIP = ip i.delItem(metaInput)
🧹 Nitpick comments (3)
pkg/input/provider/list/hmap.go (3)
41-41: Storing global options on the provider — add a clear contract (and guard nil).Keeping a pointer to types.Options here is fine, but please document immutability expectations and add a nil-guard in New to avoid panics if someone constructs Options with a nil .Options.
Example guard near New(...):
if opts == nil || opts.Options == nil { return nil, errors.New("list input provider: options must be non-nil") }
74-74: Initialize options with an early nil-check.New dereferences multiple fields on options later; protect against nil to fail fast with a helpful error.
if options == nil { return nil, errors.New("list input provider: options must be non-nil") }
538-539: Regex errors ignored; untrusted patterns can backfire.regexp.MatchString(metaInput.Input, tmpUrl.Host) ignores compile errors and may ReDoS. Compile once, handle error, and pre-validate exclude patterns.
- matched, _ := regexp.MatchString(metaInput.Input, tmpUrl.Host) + re, reErr := regexp.Compile(metaInput.Input) + if reErr != nil { + gologger.Warning().Msgf("invalid exclude regex %q: %v", metaInput.Input, reErr) + return nil + } + matched := re.MatchString(tmpUrl.Host)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/input/provider/list/hmap.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/input/provider/list/hmap.go (1)
pkg/types/types.go (1)
Options(32-464)
🔇 Additional comments (1)
pkg/input/provider/list/hmap.go (1)
62-109: Clarify timeout error suppression vs parse-time exits
- This change only suppresses parse-time invalid-target logs (os.Exit in pkg/input/provider/list/hmap.go at lines 159–161 and 409–413).
- It does not address the repeated network-layer timeout errors for unresponsive hosts (issue #6416); I found no gologger logs for connection timeouts in pkg—please verify where timeouts are handled/retried and adjust logging accordingly.
Comment on lines
+151
to
+166
| if err != nil { | ||
| gologger.Error().Msgf("Failed to parse URL %v: %v", URL, err) | ||
| } else { | ||
| gologger.Error().Msgf("Invalid hostname for URL %v", URL) | ||
| } | ||
|
|
||
| // Check if this is single target mode (-u) or batch mode (-l) | ||
| if i.options.TargetsFilePath == "" && len(i.options.Targets) <= 1 { | ||
| // Single target mode: exit immediately | ||
| gologger.Info().Msg("Stopping scan due to invalid target") | ||
| os.Exit(1) | ||
| } else { | ||
| // Batch mode: log error and skip this target | ||
| gologger.Warning().Msgf("Skipping invalid target: %v", URL) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Invalid-target handling: wrong single-target detection, duplicate logs, and os.Exit in library code.
- Misclassification: stdin/uncover-only runs (Targets==0, TargetsFilePath=="") are treated as “single-target”, causing premature exit on any bad line.
- Duplicate logs: you emit an Error then an Info/Warning for the same invalid input.
- os.Exit here makes the package kill the process; better keep exit decisions at the CLI/runner layer.
Minimum fix: correct detection and emit a single message; keep exit only if you must. Suggested patch:
- if err != nil {
- gologger.Error().Msgf("Failed to parse URL %v: %v", URL, err)
- } else {
- gologger.Error().Msgf("Invalid hostname for URL %v", URL)
- }
-
- // Check if this is single target mode (-u) or batch mode (-l)
- if i.options.TargetsFilePath == "" && len(i.options.Targets) <= 1 {
- // Single target mode: exit immediately
- gologger.Info().Msg("Stopping scan due to invalid target")
- os.Exit(1)
- } else {
- // Batch mode: log error and skip this target
- gologger.Warning().Msgf("Skipping invalid target: %v", URL)
- return
- }
+ // Single clear message + correct single/batch detection
+ reason := "empty hostname"
+ if err != nil {
+ reason = err.Error()
+ }
+ if i.isSingleTargetMode() {
+ gologger.Error().Msgf("Invalid target %v: %v", URL, reason)
+ // Ideally propagate to caller instead of exiting here.
+ os.Exit(1)
+ } else {
+ gologger.Warning().Msgf("Skipping invalid target %v: %v", URL, reason)
+ return
+ }Add this helper once in the file:
func (i *ListInputProvider) isSingleTargetMode() bool {
if i == nil || i.options == nil {
return false
}
// Consider only explicit -u usage; stdin/-l/uncover are batch-like.
return i.options.TargetsFilePath == "" &&
!i.options.Stdin &&
!i.options.Uncover &&
len(i.options.Targets) == 1
}If feasible, prefer bubbling an error to the caller instead of os.Exit to keep package-level code side-effect free. I can draft that refactor if you want.
🤖 Prompt for AI Agents
In pkg/input/provider/list/hmap.go around lines 151 to 166: the current logic
misdetects single-target mode (treats stdin/uncover runs as single-target),
emits duplicate logs for the same invalid URL, and calls os.Exit from library
code; fix by adding a helper method isSingleTargetMode on ListInputProvider that
returns true only when TargetsFilePath=="" AND Targets length == 1 AND explicit
flags Stdin and Uncover are false (also guard i/options nil), replace the
current detection with that helper, remove the redundant Error+Info/Warning pair
so only one log entry is emitted for an invalid target, and do NOT call os.Exit
here — instead return an error or a sentinel so the CLI/runner layer can decide
to exit; if you prefer, change the function signature to return an error so
callers can handle exit behavior.
Comment on lines
+401
to
+416
| if err != nil { | ||
| gologger.Error().Msgf("Failed to parse URL %v: %v", URL, err) | ||
| } else { | ||
| gologger.Error().Msgf("Invalid hostname for URL %v", URL) | ||
| } | ||
|
|
||
| // Check if this is single target mode (-u) or batch mode (-l) | ||
| if i.options.TargetsFilePath == "" && len(i.options.Targets) <= 1 { | ||
| // Single target mode: exit immediately | ||
| gologger.Info().Msg("Stopping scan due to invalid target") | ||
| os.Exit(1) | ||
| } else { | ||
| // Batch mode: log error and skip this target | ||
| gologger.Warning().Msgf("Skipping invalid target: %v", URL) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t os.Exit from Del; treat invalid excludes as non-fatal and skip.
Exiting from Del can kill a scan due to an invalid exclude pattern even in -l/stdin runs. Keep it a single warning and continue.
- if err != nil {
- gologger.Error().Msgf("Failed to parse URL %v: %v", URL, err)
- } else {
- gologger.Error().Msgf("Invalid hostname for URL %v", URL)
- }
-
- // Check if this is single target mode (-u) or batch mode (-l)
- if i.options.TargetsFilePath == "" && len(i.options.Targets) <= 1 {
- // Single target mode: exit immediately
- gologger.Info().Msg("Stopping scan due to invalid target")
- os.Exit(1)
- } else {
- // Batch mode: log error and skip this target
- gologger.Warning().Msgf("Skipping invalid target: %v", URL)
- return
- }
+ reason := "empty hostname"
+ if err != nil {
+ reason = err.Error()
+ }
+ gologger.Warning().Msgf("Skipping invalid exclude target %v: %v", URL, reason)
+ return📝 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.
Suggested change
| if err != nil { | |
| gologger.Error().Msgf("Failed to parse URL %v: %v", URL, err) | |
| } else { | |
| gologger.Error().Msgf("Invalid hostname for URL %v", URL) | |
| } | |
| // Check if this is single target mode (-u) or batch mode (-l) | |
| if i.options.TargetsFilePath == "" && len(i.options.Targets) <= 1 { | |
| // Single target mode: exit immediately | |
| gologger.Info().Msg("Stopping scan due to invalid target") | |
| os.Exit(1) | |
| } else { | |
| // Batch mode: log error and skip this target | |
| gologger.Warning().Msgf("Skipping invalid target: %v", URL) | |
| return | |
| } | |
| // Treat invalid exclude patterns as non-fatal—just warn and skip | |
| reason := "empty hostname" | |
| if err != nil { | |
| reason = err.Error() | |
| } | |
| gologger.Warning().Msgf("Skipping invalid exclude target %v: %v", URL, reason) | |
| return |
🤖 Prompt for AI Agents
In pkg/input/provider/list/hmap.go around lines 401 to 416, the code currently
calls os.Exit(1) when encountering an invalid target, which terminates the
entire scan; instead, treat invalid excludes/non-fatal parse failures as
non-fatal: remove the os.Exit call, change the logic so that both single-target
and batch runs log an appropriate warning or info and return to skip processing
this target (for single-target mode, return an error to the caller or set a
non-fatal status rather than exiting), ensuring the function does not terminate
the process and batch/stdin runs continue.
1 task
dwisiswant0
reviewed
Sep 3, 2025
Member
There was a problem hiding this comment.
Just a quick note: this case is kind of similar on the surface, but the behavior we’re chasing here is different from the linked issue.
Unresolved questions/TODOs:
- How's the behavior if the target is successfully resolved (valid hostname/IP address)? -- this is the primary case for #6416.
- Can you verify that this also works with the SDK? -- integration test may be needed.
|
This pull request has been automatically marked as stale due to inactivity. It will be closed in 7 days if no further activity occurs. Please update if you wish to keep it open. |
github-actions
Bot
added
the
Status: Stale
|
This pull request has been automatically closed due to inactivity. If you think this is a mistake or would like to continue working on it, please comment or feel free to reopen it. |
github-actions
Bot
added
the
Status: Abandoned
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Fixes #6416
Fixes issue where:
Screenshot
Batch mode (-l option)
Single target mode (-u option)
Checklist
Summary by CodeRabbit