fix: parse comma-separated entries in file (-l) and stdin input#936
Open
Deducer wants to merge 2 commits intoprojectdiscovery:mainfrom
Open
fix: parse comma-separated entries in file (-l) and stdin input#936Deducer wants to merge 2 commits intoprojectdiscovery:mainfrom
Deducer wants to merge 2 commits intoprojectdiscovery:mainfrom
Conversation
Neo - PR Security ReviewNo security issues found Highlights
Comment |
WalkthroughAdds Runner.processCommaSeparatedInput to split comma-separated input lines, trim and ignore empty entries, and queue each item. normalizeAndQueueInputs now calls this helper for lines from list files and stdin. New unit tests validate comma-separated parsing; some tests are duplicated in the diff. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (1)
internal/runner/runner_test.go (1)
234-237: Consider movingdefer close(inputs)before the function call.While functionally correct, placing
deferafter the main call is unconventional. The idiomatic pattern is to registerdeferstatements at the beginning of the function scope. This applies to all four new tests.♻️ Suggested refactor
go func() { + defer close(inputs) runner.processCommaSeparatedInput(line, inputs) - defer close(inputs) }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runner/runner_test.go` around lines 234 - 237, Move the defer close(inputs) to the start of each goroutine before calling runner.processCommaSeparatedInput to follow idiomatic Go: in the anonymous goroutine used in the tests (where inputs channel is closed), place defer close(inputs) as the first statement in the goroutine body so the channel is guaranteed to be closed at goroutine exit and the defer is registered before invoking runner.processCommaSeparatedInput; update this in all four new tests referencing the anonymous goroutine and runner.processCommaSeparatedInput.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/runner/runner_test.go`:
- Around line 234-237: Move the defer close(inputs) to the start of each
goroutine before calling runner.processCommaSeparatedInput to follow idiomatic
Go: in the anonymous goroutine used in the tests (where inputs channel is
closed), place defer close(inputs) as the first statement in the goroutine body
so the channel is guaranteed to be closed at goroutine exit and the defer is
registered before invoking runner.processCommaSeparatedInput; update this in all
four new tests referencing the anonymous goroutine and
runner.processCommaSeparatedInput.
The -u flag uses CommaSeparatedStringSliceOptions which auto-splits comma-separated values, but file (-l) and stdin input read lines as-is without splitting on commas. This adds processCommaSeparatedInput() to split comma-delimited entries before processing, making -l behavior consistent with -u. Fixes projectdiscovery#859
Deducer
force-pushed
the
fix/comma-separated-list-input
branch
from
d808bfa to
658b8f7
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/runner/runner.go (1)
439-455:⚠️ Potential issue | 🟠 MajorCheck
Scanner.Err()after both loops to prevent silent drops.If scanning fails (including oversized tokens), processing stops silently and targets are skipped without feedback.
🔧 Proposed fix
scanner := bufio.NewScanner(file) for scanner.Scan() { text := scanner.Text() if text != "" { r.processCommaSeparatedInput(text, inputs) } } + if err := scanner.Err(); err != nil { + return errkit.Wrap(err, "could not read input file") + } } if r.hasStdin { scanner := bufio.NewScanner(os.Stdin) for scanner.Scan() { text := scanner.Text() if text != "" { r.processCommaSeparatedInput(text, inputs) } } + if err := scanner.Err(); err != nil { + return errkit.Wrap(err, "could not read stdin") + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runner/runner.go` around lines 439 - 455, Both bufio.Scanner loops can fail silently; after each loop (the scanner created for file and the one for os.Stdin when r.hasStdin) check scanner.Err() and handle any error instead of ignoring it. Update the code surrounding the scanner usage that invokes r.processCommaSeparatedInput to call scanner.Err() after the for scanner.Scan() loop and propagate or log the error (e.g., return it from the enclosing function or call the runner's logger) so oversized tokens or IO errors don't cause silent drops; ensure you reference the same scanner variables and preserve existing behavior when err == nil.
🧹 Nitpick comments (1)
internal/runner/runner.go (1)
488-499: Normalize single-item input withTrimSpace()before passthrough.Right now only comma-split entries are trimmed. Trimming upfront makes single-line and comma-line behavior consistent.
♻️ Proposed refactor
func (r *Runner) processCommaSeparatedInput(input string, inputs chan taskInput) { + input = strings.TrimSpace(input) + if input == "" { + return + } if strings.Contains(input, ",") { for _, item := range strings.Split(input, ",") { item = strings.TrimSpace(item) if item != "" { r.processInputItem(item, inputs) } } return } r.processInputItem(input, inputs) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runner/runner.go` around lines 488 - 499, The function processCommaSeparatedInput trims only comma-split items, so a single-item input with surrounding whitespace bypasses trimming; update processCommaSeparatedInput to call strings.TrimSpace(input) at the start, return early if the trimmed input is empty, then proceed to split on commas and call r.processInputItem(trimmedItem, inputs) for each non-empty trimmed piece (and for the single-item path use the already-trimmed input) to make single-line and comma-separated behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/runner/runner.go`:
- Around line 439-455: Both bufio.Scanner loops can fail silently; after each
loop (the scanner created for file and the one for os.Stdin when r.hasStdin)
check scanner.Err() and handle any error instead of ignoring it. Update the code
surrounding the scanner usage that invokes r.processCommaSeparatedInput to call
scanner.Err() after the for scanner.Scan() loop and propagate or log the error
(e.g., return it from the enclosing function or call the runner's logger) so
oversized tokens or IO errors don't cause silent drops; ensure you reference the
same scanner variables and preserve existing behavior when err == nil.
---
Nitpick comments:
In `@internal/runner/runner.go`:
- Around line 488-499: The function processCommaSeparatedInput trims only
comma-split items, so a single-item input with surrounding whitespace bypasses
trimming; update processCommaSeparatedInput to call strings.TrimSpace(input) at
the start, return early if the trimmed input is empty, then proceed to split on
commas and call r.processInputItem(trimmedItem, inputs) for each non-empty
trimmed piece (and for the single-item path use the already-trimmed input) to
make single-line and comma-separated behavior consistent.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/runner/runner.gointernal/runner/runner_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/runner/runner_test.go
- Move defer close(inputs) before function call in tests (idiomatic Go) - Add TrimSpace on single-item input path for consistency - Add scanner.Err() checks after file and stdin scanner loops
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/runner/runner_test.go (2)
30-33: Optional: Consider aligning defer placement across all tests for consistency.The new tests correctly place
defer close(inputs)before the function call (idiomatic Go), but existing tests like this one place it after. Aligning the pattern across all tests would improve consistency.go func() { + defer close(inputs) runner.processInputItem(domain, inputs) - defer close(inputs) }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runner/runner_test.go` around lines 30 - 33, The test goroutine calls runner.processInputItem(domain, inputs) before deferring close(inputs), which is inconsistent with other tests; move the defer close(inputs) to immediately after starting the goroutine (i.e., place defer close(inputs) directly inside the goroutine before calling runner.processInputItem) so the input channel is guaranteed to be closed regardless of early returns in runner.processInputItem; update the goroutine in runner_test.go that references runner.processInputItem and inputs to follow the same defer placement pattern as other tests.
305-341: LGTM!Thorough test validating the cross-product behavior between comma-separated hosts and multiple ports.
Optional: Consider adding edge case tests for malformed input such as empty strings, consecutive commas (
"a,,b"), or trailing/leading commas (",a,b,") to ensure the implementation handles these gracefully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runner/runner_test.go` around lines 305 - 341, Add edge-case unit tests for Runner.processCommaSeparatedInput mirroring Test_CommaSeparatedInputMultiplePorts_processCommaSeparatedInput: create tests (e.g., Test_CommaSeparatedInput_EdgeCases) that feed lines with empty strings, consecutive commas ("a,,b"), and leading/trailing commas (",a,b,") into processCommaSeparatedInput and assert the produced taskInput channel behavior (decide and assert whether empty entries are skipped or produce empty-host tasks consistent with existing implementation). Locate the logic via the Runner.processCommaSeparatedInput function and the existing test Test_CommaSeparatedInputMultiplePorts_processCommaSeparatedInput to structure the new cases and expected taskInput slices accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/runner/runner_test.go`:
- Around line 30-33: The test goroutine calls runner.processInputItem(domain,
inputs) before deferring close(inputs), which is inconsistent with other tests;
move the defer close(inputs) to immediately after starting the goroutine (i.e.,
place defer close(inputs) directly inside the goroutine before calling
runner.processInputItem) so the input channel is guaranteed to be closed
regardless of early returns in runner.processInputItem; update the goroutine in
runner_test.go that references runner.processInputItem and inputs to follow the
same defer placement pattern as other tests.
- Around line 305-341: Add edge-case unit tests for
Runner.processCommaSeparatedInput mirroring
Test_CommaSeparatedInputMultiplePorts_processCommaSeparatedInput: create tests
(e.g., Test_CommaSeparatedInput_EdgeCases) that feed lines with empty strings,
consecutive commas ("a,,b"), and leading/trailing commas (",a,b,") into
processCommaSeparatedInput and assert the produced taskInput channel behavior
(decide and assert whether empty entries are skipped or produce empty-host tasks
consistent with existing implementation). Locate the logic via the
Runner.processCommaSeparatedInput function and the existing test
Test_CommaSeparatedInputMultiplePorts_processCommaSeparatedInput to structure
the new cases and expected taskInput slices accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/runner/runner.gointernal/runner/runner_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/runner/runner.go
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
1 participant
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.
Summary
Fixes #859
The
-uflag usesCommaSeparatedStringSliceOptionsfrom goflags which auto-splits comma-separated values, but file (-l) and stdin input paths read lines viabufio.Scannerand pass them directly toprocessInputItem()without splitting on commas. This means a file like:gets treated as a single host
scanme.sh,example.com,test.cominstead of three separate hosts.Changes
processCommaSeparatedInput()method that splits comma-delimited input before callingprocessInputItem()for each entry-l) and stdin input paths innormalizeAndQueueInputs()foo.com , bar.com)processInputItem()directly when no comma is present (zero overhead for non-comma input)Before
After
Same fix applies to
-lfile input.Test plan
Summary by CodeRabbit
New Features
Tests