feat: add burp xml input mode support (-im flag) #2372
Conversation
WalkthroughAdds a Format interface and registry, implements a BurpFormat parser for Burp Suite XML, registers it, and integrates format-based input loading into the runner with a new Changes
Sequence Diagram(s)sequenceDiagram
participant Runner
participant Formats as FormatRegistry
participant File as InputFile
participant Burp as BurpParser
participant Callback as URLProcessor
Runner->>Formats: GetFormat(InputMode)
Formats-->>Runner: BurpFormat
Runner->>File: open(filePath)
File-->>Runner: file handle
Runner->>Burp: Parse(file, callback)
Burp->>File: read XML
Burp->>Burp: unmarshal items
loop for each item with URL
Burp->>Callback: callback(url)
Callback->>Callback: trim/expand/dedupe
Callback-->>Burp: continue or stop
end
Burp-->>Runner: return nil or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @go.mod:
- Line 142: The go.mod entry for github.com/seh-msft/burpxml is incorrectly
marked as indirect even though it is directly imported from
common/inputformats/burp.go; fix this by running `go mod tidy` in the repository
root to regenerate go.mod/go.sum (or remove the `// indirect` annotation for the
github.com/seh-msft/burpxml line and then run `go mod tidy`), then verify the
import in common/inputformats/burp.go still builds and commit the updated
go.mod/go.sum.
In @runner/runner.go:
- Around line 659-665: streamInput currently discards the error from
format.Parse which hides parse failures; change the call in streamInput to
capture the returned error (err := format.Parse(...)), and then handle it the
same way prepareInput does — i.e., log or return the error instead of assigning
to `_`; keep the same parsing callback that uses r.options.SkipDedupe and
r.testAndSet to send items to out so only the Parse error handling changes.
🧹 Nitpick comments (5)
runner/options.go (1)
380-380: Add InputMode validation to ValidateOptions() for early error detection.Currently, invalid
InputModevalues cause agologger.Fatal()error during execution (runner.go lines 532, 651) rather than failing early during validation. Moving this check toValidateOptions()will provide cleaner error handling and fail faster with a proper validation error.Suggested addition in ValidateOptions()
if options.InputMode != "" && inputformats.GetFormat(options.InputMode) == nil { return fmt.Errorf("invalid input mode '%s', supported formats: %s", options.InputMode, inputformats.SupportedFormats()) }common/inputformats/burp_test.go (2)
68-73: Consider adding bounds check before index access.If
Parsereturns fewer URLs than expected (e.g., due to a bug), accessingurls[i]will panic with an index out of range error rather than providing a clear test failure message.💡 Suggested improvement
expectedURLs := []string{"http://example.com/path1", "https://example.com/path2"} + if len(urls) != len(expectedURLs) { + t.Fatalf("Expected %d URLs, got %d: %v", len(expectedURLs), len(urls), urls) + } for i, expected := range expectedURLs { - if urls[i] != expected { + if i < len(urls) && urls[i] != expected { t.Errorf("Expected URL %d to be '%s', got '%s'", i, expected, urls[i]) } }
1-148: Consider adding a test for malformed XML.The test suite covers valid XML scenarios well, but doesn't test error handling for malformed or invalid XML input. This would help ensure the parser returns appropriate errors rather than panicking.
💡 Suggested test case
func TestBurpFormat_ParseMalformed(t *testing.T) { malformedXML := `<?xml version="1.0"?> <items burpVersion="2023.10.1.2"> <item> <url><![CDATA[http://example.com/path1]]></url> <!-- missing closing tag --> </items>` b := NewBurpFormat() err := b.Parse(strings.NewReader(malformedXML), func(url string) bool { return true }) if err == nil { t.Error("Expected error for malformed XML, got nil") } }runner/runner.go (2)
647-676: Duplicate format validation logic should be extracted.The format lookup and validation logic (lines 649-652) is duplicated from
prepareInput(lines 530-533). Consider extracting this into a helper function to maintain DRY principles.♻️ Suggested helper function
// getInputFormat validates and returns the format for the given input mode. // Returns nil and logs fatal if the format is invalid. func (r *Runner) getInputFormat() inputformats.Format { if r.options.InputMode == "" { return nil } format := inputformats.GetFormat(r.options.InputMode) if format == nil { gologger.Fatal().Msgf("Invalid input mode '%s'. Supported: %s\n", r.options.InputMode, inputformats.SupportedFormats()) } return format }Then use it in both
prepareInputandstreamInput.
737-751: Shadow variableerrinside callback may cause confusion.The callback declares
erron line 739 which shadows the outererrfrom line 737. While this doesn't cause a bug (the outererris reassigned byformat.Parse), it could lead to maintenance issues. Consider renaming the inner variable.💡 Suggested improvement
err = format.Parse(finput, func(target string) bool { target = strings.TrimSpace(target) - expandedTarget, err := r.countTargetFromRawTarget(target) - if err == nil && expandedTarget > 0 { + expandedTarget, countErr := r.countTargetFromRawTarget(target) + if countErr == nil && expandedTarget > 0 { numTargets += expandedTarget r.hm.Set(target, []byte("1")) //nolint - } else if r.options.SkipDedupe && errors.Is(err, duplicateTargetErr) { + } else if r.options.SkipDedupe && errors.Is(countErr, duplicateTargetErr) { if v, ok := r.hm.Get(target); ok { cnt, _ := strconv.Atoi(string(v)) _ = r.hm.Set(target, []byte(strconv.Itoa(cnt+1))) numTargets += 1 } } return true })
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
common/inputformats/burp.gocommon/inputformats/burp_test.gocommon/inputformats/formats.gocommon/inputformats/formats_test.gogo.modrunner/options.gorunner/runner.go
🧰 Additional context used
🧬 Code graph analysis (4)
common/inputformats/burp.go (1)
common/inputformats/formats.go (1)
Format(11-17)
common/inputformats/formats.go (1)
common/inputformats/burp.go (1)
NewBurpFormat(14-16)
runner/options.go (1)
common/inputformats/formats.go (1)
SupportedFormats(35-41)
common/inputformats/burp_test.go (1)
common/inputformats/burp.go (1)
NewBurpFormat(14-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Test Builds (ubuntu-latest)
- GitHub Check: Test Builds (macOS-latest)
- GitHub Check: Test Builds (windows-latest)
- GitHub Check: Functional Test (ubuntu-latest)
- GitHub Check: Functional Test (windows-latest)
- GitHub Check: Functional Test (macOS-latest)
- GitHub Check: Analyze (go)
- GitHub Check: release-test
🔇 Additional comments (16)
common/inputformats/formats_test.go (2)
8-36: LGTM!Well-structured table-driven tests covering case-insensitivity, invalid inputs, and edge cases for the format registry lookup.
38-43: LGTM!Basic validation that "burp" format is registered. Consider adding a test that verifies the exact count of supported formats if the registry is expected to grow.
common/inputformats/formats.go (3)
10-17: LGTM!Clean interface design. The callback returning
boolfor early termination is a good pattern for streaming large input files.
19-22: LGTM!Format registry is properly initialized at package load time. The slice-based approach is appropriate for the expected small number of formats.
24-41: LGTM!Case-insensitive lookup is user-friendly. The implementation is straightforward and correct.
common/inputformats/burp.go (2)
18-18: LGTM!Good practice using compile-time interface assertion to ensure
BurpFormatimplementsFormat.
27-42: LGTM!The implementation correctly handles:
- Parse errors with contextual wrapping
- Empty URLs (skipped)
- Early termination via callback return value
Consider whether URL validation (e.g.,
url.Parse) would be beneficial before invoking the callback, though downstream consumers may already handle malformed URLs gracefully.runner/options.go (2)
26-26: LGTM!Import added correctly for the new inputformats package.
195-195: LGTM!Field follows existing naming conventions (
InputFile,InputMode).common/inputformats/burp_test.go (4)
8-13: LGTM!Simple and effective test for verifying the format name.
15-74: LGTM!Comprehensive test covering the happy path with realistic Burp XML data including CDATA sections, base64-encoded requests/responses, and multiple items.
76-96: LGTM!Good edge case coverage for empty XML input.
98-148: LGTM!Good test for early termination behavior via callback returning
false. This validates that the parser respects the callback's signal to stop processing.runner/runner.go (3)
38-38: LGTM!Import correctly added for the new inputformats package.
528-548: LGTM!Good implementation with proper validation of the input mode against supported formats, clear error messages, and appropriate fallback to line-based file reading when no format is specified.
730-753: LGTM!The
loadFromFormatmethod follows the same pattern asloadAndCloseFile, correctly handling target counting, deduplication, and theSkipDedupeoption. Good use ofdeferfor file cleanup.
- Handle error from format.Parse in streamInput - Add InputMode validation to ValidateOptions for early error detection - Extract duplicate format validation to getInputFormat helper - Fix shadow variable err in loadFromFormat callback - Add bounds check in test before index access - Add test for malformed XML input - Run go mod tidy to fix dependency marking
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
common/inputformats/burp_test.go (1)
64-76: Redundant length checks can be consolidated.The check at lines 64-66 (
len(urls) != 2) is redundant with the check at lines 69-71 (len(urls) != len(expectedURLs)), sinceexpectedURLsis always length 2. The first check usest.Errorf(continues), so if it fails, both messages will be logged beforet.Fatalfstops execution.Consider removing lines 64-66 to keep only the more informative check.
♻️ Suggested simplification
if err != nil { t.Fatalf("Parse returned error: %v", err) } - if len(urls) != 2 { - t.Errorf("Expected 2 URLs, got %d", len(urls)) - } - expectedURLs := []string{"http://example.com/path1", "https://example.com/path2"} if len(urls) != len(expectedURLs) { t.Fatalf("Expected %d URLs, got %d: %v", len(expectedURLs), len(urls), urls) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
common/inputformats/burp_test.gogo.modrunner/options.gorunner/runner.go
✅ Files skipped from review due to trivial changes (1)
- runner/options.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🧬 Code graph analysis (2)
runner/runner.go (1)
common/inputformats/formats.go (3)
Format(11-17)GetFormat(25-32)SupportedFormats(35-41)
common/inputformats/burp_test.go (1)
common/inputformats/burp.go (1)
NewBurpFormat(14-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Builds (macOS-latest)
- GitHub Check: Test Builds (windows-latest)
- GitHub Check: Functional Test (windows-latest)
🔇 Additional comments (9)
common/inputformats/burp_test.go (4)
8-13: LGTM!Simple and effective test for the
Name()method.
79-99: LGTM!Good coverage for the empty input edge case.
101-151: LGTM!Good test coverage for the early-stop callback behavior.
153-169: LGTM!Good negative test case for malformed XML handling.
runner/runner.go (5)
38-38: LGTM!Import added for the new inputformats package integration.
528-544: LGTM!Clean integration of format-based input handling with appropriate fallback to line-based reading.
637-648: LGTM!Well-designed helper that centralizes format validation and provides clear error messages for invalid input modes.
656-684: LGTM!Good implementation of format-based streaming with proper resource cleanup and error handling. The callback correctly applies
TrimSpaceand deduplication logic consistent with the line-based path.
738-761: LGTM!Clean implementation that correctly mirrors the line-based
loadAndCloseFilelogic while using format-based parsing. The use ofcountErrinstead of shadowingerris a good fix.
Mzack9999
added
the
Type: Enhancement
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
98-98: Consider clarifying the flag description.The description "mode of input file (burp)" could be more explicit about what "mode" means. Consider rephrasing to improve clarity, such as:
- "input file format (supported: burp)"
- "format of input file (e.g., burp for Burp Suite XML)"
This would help users immediately understand that the flag specifies the file format/type.
📝 Suggested description improvement
- -im, -input-mode string mode of input file (burp) + -im, -input-mode string input file format (supported: burp)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.mdcommon/inputformats/burp.gorunner/options.go
🚧 Files skipped from review as they are similar to previous changes (2)
- runner/options.go
- common/inputformats/burp.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test Builds (windows-latest)
- GitHub Check: Test Builds (ubuntu-latest)
- GitHub Check: Test Builds (macOS-latest)
- GitHub Check: Functional Test (windows-latest)
- GitHub Check: Functional Test (ubuntu-latest)
- GitHub Check: Functional Test (macOS-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
README.md (1)
283-283: LGTM!The note provides clear, actionable guidance with a concrete example showing how to use Burp Suite XML exports with the new
-imflag.
Summary
-im, --input-modeflag to specify input file format (closes Add Burp Suite import functionality (-im burp, --input-mode burp) #2359)Test Cases
1. Create a test Burp XML file
test.xml:2. Test commands:
3. Run unit tests:
go test ./common/inputformats/... -vSummary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.