fix: support comma-separated values in -l/-list input#961
Open
flaggdavid-source wants to merge 2 commits intoprojectdiscovery:mainfrom
Open
fix: support comma-separated values in -l/-list input#961flaggdavid-source wants to merge 2 commits intoprojectdiscovery:mainfrom
flaggdavid-source wants to merge 2 commits intoprojectdiscovery:mainfrom
Conversation
The -u flag already handles comma-separated values (e.g., -u host1,host2), but -l/-list did not apply the same parsing to file or stdin input lines. This change splits each line on commas in both the file reader and stdin reader paths of normalizeAndQueueInputs, trimming whitespace and skipping empty entries, to match the -u flag behavior. Adds tests for comma-separated input files with domains and CIDR ranges. Fixes projectdiscovery#859
Neo - PR Security ReviewNo security issues found Highlights
Comment |
WalkthroughInput normalization now splits lines on commas and trims items: each non-empty comma-separated token is passed to the existing input-item processing path. This applies to both list-file ( 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)
📝 Coding Plan
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 Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/runner/runner.go (1)
443-450: Consider extracting the comma-splitting logic into a helper function to reduce duplication.The same splitting logic is duplicated for both file and stdin input paths. Extracting this into a helper would improve maintainability.
♻️ Suggested refactor
Add a helper method:
func (r *Runner) processCommaSeparatedLine(text string, inputs chan taskInput) { items := strings.Split(text, ",") for _, item := range items { item = strings.TrimSpace(item) if item != "" { r.processInputItem(item, inputs) } } }Then replace both occurrences:
if text != "" { - // Split comma-separated values to match -u flag behavior - items := strings.Split(text, ",") - for _, item := range items { - item = strings.TrimSpace(item) - if item != "" { - r.processInputItem(item, inputs) - } - } + r.processCommaSeparatedLine(text, inputs) }Also applies to: 459-466
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runner/runner.go` around lines 443 - 450, The comma-splitting and trimming logic duplicated in runner.go (used where text is split and each item passed to r.processInputItem) should be extracted into a helper on Runner, e.g. add func (r *Runner) processCommaSeparatedLine(text string, inputs chan taskInput) that runs strings.Split(text, ","), trims each item, skips empties, and calls r.processInputItem(item, inputs); then replace both duplicated blocks (the one around r.processInputItem in the file-reading branch and the one in the stdin branch) with calls to r.processCommaSeparatedLine to eliminate duplication and centralize behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/runner/runner_test.go`:
- Around line 324-376: Test_CommaSeparatedStdin is flaky because os.Pipe() error
is ignored, assertions (require.NoError) are called from background goroutines,
and os.Stdin is not restored with defer; fix by checking and failing the test
immediately if os.Pipe() returns an error, replace require.NoError calls inside
goroutines by sending errors over an error channel (e.g., make(chan error)) and
assert on those errors in the main test goroutine after the goroutines finish,
and call defer to restore os.Stdin right after replacing it; reference symbols:
Test_CommaSeparatedStdin, os.Pipe(), runner.normalizeAndQueueInputs,
runner.hasStdin/hasStdinSet, and the writer goroutine that writes to w.
---
Nitpick comments:
In `@internal/runner/runner.go`:
- Around line 443-450: The comma-splitting and trimming logic duplicated in
runner.go (used where text is split and each item passed to r.processInputItem)
should be extracted into a helper on Runner, e.g. add func (r *Runner)
processCommaSeparatedLine(text string, inputs chan taskInput) that runs
strings.Split(text, ","), trims each item, skips empties, and calls
r.processInputItem(item, inputs); then replace both duplicated blocks (the one
around r.processInputItem in the file-reading branch and the one in the stdin
branch) with calls to r.processCommaSeparatedLine to eliminate duplication and
centralize behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1388b45d-9b93-425c-99b7-decfddb804a9
📒 Files selected for processing (2)
internal/runner/runner.gointernal/runner/runner_test.go
Address CodeRabbit review feedback: - Extract duplicated comma-splitting logic into processCommaSeparatedLine method - Fix Test_CommaSeparatedStdin: check os.Pipe error, use error channels instead of require.NoError in goroutines, restore os.Stdin with defer
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/runner/runner_test.go (1)
211-322: Consider consolidating duplicate test logic into a table-driven test.
Test_CommaSeparatedInputFileandTest_CommaSeparatedInputFileWithWhitespaceshare nearly identical structure. A table-driven approach would reduce duplication and make adding new test cases easier.♻️ Example table-driven consolidation
func Test_CommaSeparatedInputFile(t *testing.T) { testCases := []struct { name string content string }{ { name: "basic comma-separated", content: "192.168.1.0/24,192.168.2.0/24,192.168.3.0/24\n", }, { name: "with whitespace", content: "192.168.1.0/24, 192.168.2.0/24 , 192.168.3.0/24\n", }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { tempFile, err := os.CreateTemp("", "tlsx-test-*.txt") require.NoError(t, err) defer os.Remove(tempFile.Name()) _, err = tempFile.WriteString(tc.content) require.NoError(t, err) tempFile.Close() options := &clients.Options{ Ports: []string{"443"}, InputList: tempFile.Name(), } runner := &Runner{options: options} inputs := make(chan taskInput, 100) errCh := make(chan error, 1) go func() { errCh <- runner.normalizeAndQueueInputs(inputs) close(inputs) }() var got []taskInput for task := range inputs { got = append(got, task) } require.NoError(t, <-errCh) // Verify all prefixes processed prefixes := []string{"192.168.1.", "192.168.2.", "192.168.3."} for _, prefix := range prefixes { found := false for _, task := range got { if strings.HasPrefix(task.host, prefix) { found = true break } } assert.True(t, found, "should have inputs from prefix %s", prefix) } }) } }🤖 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 211 - 322, Both tests Test_CommaSeparatedInputFile and Test_CommaSeparatedInputFileWithWhitespace duplicate setup and assertions; convert them into a single table-driven test that iterates test cases (e.g., names "basic comma-separated" and "with whitespace"), create temp file per case, construct options and Runner, call runner.normalizeAndQueueInputs(inputs) in a goroutine capturing its error, collect taskInput items from the inputs channel, assert no error from normalizeAndQueueInputs, and verify each expected prefix (e.g., "192.168.1.", "192.168.2.", "192.168.3.") appears in the collected tasks to replace the two separate tests with one concise loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/runner/runner_test.go`:
- Around line 232-236: The goroutine in the test calls require.NoError from a
non-test goroutine (inside the anonymous go func wrapping
runner.normalizeAndQueueInputs(inputs)), which can panic; change this to use an
error channel: create a chan error, run normalizeAndQueueInputs(inputs) in the
goroutine and send its returned error (or nil) on the channel, then in the main
test goroutine receive from that channel and call require.NoError on the
received error; ensure you still close(inputs) appropriately after calling
normalizeAndQueueInputs or from the goroutine before sending the error so the
test behavior remains the same.
- Around line 291-295: The test spawns a goroutine calling
runner.normalizeAndQueueInputs(inputs) but asserts directly in the goroutine;
change it to use the error-channel pattern: create a channel (e.g. errCh), run
the goroutine to call runner.normalizeAndQueueInputs(inputs) and send the
returned error on errCh, then close inputs, and in the main test goroutine
receive from errCh and assert require.NoError(t, err). This mirrors the fix used
in Test_CommaSeparatedInputFile and ensures the assertion is performed in the
test goroutine.
---
Nitpick comments:
In `@internal/runner/runner_test.go`:
- Around line 211-322: Both tests Test_CommaSeparatedInputFile and
Test_CommaSeparatedInputFileWithWhitespace duplicate setup and assertions;
convert them into a single table-driven test that iterates test cases (e.g.,
names "basic comma-separated" and "with whitespace"), create temp file per case,
construct options and Runner, call runner.normalizeAndQueueInputs(inputs) in a
goroutine capturing its error, collect taskInput items from the inputs channel,
assert no error from normalizeAndQueueInputs, and verify each expected prefix
(e.g., "192.168.1.", "192.168.2.", "192.168.3.") appears in the collected tasks
to replace the two separate tests with one concise loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92a7c096-8432-41a6-9451-1f7e72034e8e
📒 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
Comment on lines
+232
to
+236
| go func() { | ||
| err := runner.normalizeAndQueueInputs(inputs) | ||
| require.NoError(t, err) | ||
| close(inputs) | ||
| }() |
There was a problem hiding this comment.
Avoid require.NoError inside goroutines - test will panic on failure.
The same pattern that was fixed in Test_CommaSeparatedStdin should be applied here. Calling require.NoError from a non-test goroutine invokes t.FailNow(), which must only be called from the test goroutine per Go's testing package documentation.
🔧 Proposed fix using error channel pattern
inputs := make(chan taskInput, 100)
// Test normalizeAndQueueInputs with the file
+ errCh := make(chan error, 1)
go func() {
- err := runner.normalizeAndQueueInputs(inputs)
- require.NoError(t, err)
+ errCh <- runner.normalizeAndQueueInputs(inputs)
close(inputs)
}()
var got []taskInput
for task := range inputs {
got = append(got, task)
}
+
+ require.NoError(t, <-errCh, "normalizeAndQueueInputs failed")🤖 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 232 - 236, The goroutine in the
test calls require.NoError from a non-test goroutine (inside the anonymous go
func wrapping runner.normalizeAndQueueInputs(inputs)), which can panic; change
this to use an error channel: create a chan error, run
normalizeAndQueueInputs(inputs) in the goroutine and send its returned error (or
nil) on the channel, then in the main test goroutine receive from that channel
and call require.NoError on the received error; ensure you still close(inputs)
appropriately after calling normalizeAndQueueInputs or from the goroutine before
sending the error so the test behavior remains the same.
Comment on lines
+291
to
+295
| go func() { | ||
| err := runner.normalizeAndQueueInputs(inputs) | ||
| require.NoError(t, err) | ||
| close(inputs) | ||
| }() |
There was a problem hiding this comment.
Same goroutine assertion issue as Test_CommaSeparatedInputFile.
Apply the error channel pattern here as well for consistency and test robustness.
🔧 Proposed fix
inputs := make(chan taskInput, 100)
// Test normalizeAndQueueInputs with the file
+ errCh := make(chan error, 1)
go func() {
- err := runner.normalizeAndQueueInputs(inputs)
- require.NoError(t, err)
+ errCh <- runner.normalizeAndQueueInputs(inputs)
close(inputs)
}()
var got []taskInput
for task := range inputs {
got = append(got, task)
}
+
+ require.NoError(t, <-errCh, "normalizeAndQueueInputs failed")📝 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
| go func() { | |
| err := runner.normalizeAndQueueInputs(inputs) | |
| require.NoError(t, err) | |
| close(inputs) | |
| }() | |
| inputs := make(chan taskInput, 100) | |
| // Test normalizeAndQueueInputs with the file | |
| errCh := make(chan error, 1) | |
| go func() { | |
| errCh <- runner.normalizeAndQueueInputs(inputs) | |
| close(inputs) | |
| }() | |
| var got []taskInput | |
| for task := range inputs { | |
| got = append(got, task) | |
| } | |
| require.NoError(t, <-errCh, "normalizeAndQueueInputs failed") |
🤖 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 291 - 295, The test spawns a
goroutine calling runner.normalizeAndQueueInputs(inputs) but asserts directly in
the goroutine; change it to use the error-channel pattern: create a channel
(e.g. errCh), run the goroutine to call runner.normalizeAndQueueInputs(inputs)
and send the returned error on errCh, then close inputs, and in the main test
goroutine receive from errCh and assert require.NoError(t, err). This mirrors
the fix used in Test_CommaSeparatedInputFile and ensures the assertion is
performed in the test goroutine.
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 handles comma-separated values (e.g.,-u host1,host2), but-l/-listdid not apply the same parsing when reading from files or stdin. This PR aligns the behavior so that comma-separated entries in list files are split and processed individually.Changes
internal/runner/runner.go: ModifiednormalizeAndQueueInputsto split each input line on commas, trim whitespace, and skip empty entries — applied to both file reader and stdin reader paths.internal/runner/runner_test.go: Added test cases for comma-separated domains and CIDR ranges in input files, including whitespace handling.Testing
make test)Test_CommaSeparatedInputFileandTest_CommaSeparatedInputFileWithWhitespacepass./tlsx -l test-input.txt -p 443correctly processes comma-separated entriesExample
Before: A file containing
192.168.1.0/24,192.168.2.0/24was treated as a single (invalid) input.After: Each comma-separated value is processed independently, matching
-uflag behavior.Summary by CodeRabbit
New Features
Tests