feat(diff): add configuration diff tool for OPNsense XML comparison - #227
Conversation
Implements GitHub Issue #200: content-aware, security-focused comparison of OPNsense configuration files with semantic change detection. Key features: - Compare firewall rules by UUID with field-level change tracking - Compare interfaces by name with detailed property changes - Compare static DHCP reservations by MAC address - Detect NAT rules, VLANs, users, routes, and system settings - Security impact flagging (high/medium/low) for sensitive changes - Multiple output formats: terminal (color-coded), markdown, JSON - Section filtering (--section firewall,nat,interfaces) - Security-only mode (--security flag) Package structure: - internal/diff/engine.go: Orchestrates comparison workflow - internal/diff/analyzer.go: Content-aware structural analysis - internal/diff/types.go: Core data structures (Change, Result, Section) - internal/diff/formatters/: Terminal, Markdown, and JSON formatters - cmd/diff.go: CLI command with Cobra framework Closes #200 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
dosubot
Bot
added
the
size:XXL
|
Caution Review failedFailed to post review comments 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a full diff feature: CLI command to compare two OPNsense XML configs, a diff engine and analyzer producing structured Change results, three output formatters (terminal, markdown, JSON), options for section filtering and security-only output, plus unit and integration tests. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI (cmd/diff)
participant Parser as Parser
participant Engine as Diff Engine
participant Analyzer as Analyzer
participant Formatter as Formatter
participant Output
User->>CLI: opndossier diff old.xml new.xml --format markdown
CLI->>Parser: parse(old.xml), parse(new.xml)
Parser-->>CLI: oldCfg, newCfg
CLI->>Engine: NewEngine(oldCfg, newCfg, opts)
CLI->>Engine: Compare(ctx)
Engine->>Analyzer: CompareSection(System/Firewall/NAT/Interfaces/...)
loop per section
Analyzer-->>Engine: []Change
end
Engine->>Engine: apply section filter / security-only
Engine-->>CLI: Result (Summary, Metadata, Changes)
CLI->>Formatter: Format(result)
Formatter->>Output: write to stdout/file
Output-->>User: render result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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)
Comment |
dosubot
Bot
added
enhancement
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR implements GitHub Issue #200, adding a content-aware configuration diff tool for comparing OPNsense XML files. The implementation provides semantic comparison of configuration elements (matching firewall rules by UUID, interfaces by name, DHCP reservations by MAC), security impact flagging, and multiple output formats (terminal, markdown, JSON).
Changes:
- Adds a complete diff engine with semantic analysis for all major OPNsense configuration sections (system, firewall, NAT, interfaces, VLANs, DHCP, users, routing)
- Implements three output formatters (terminal with color support, markdown for documentation, JSON for automation) with proper TERM=dumb support for CI environments
- Provides CLI command with section filtering (
--section) and security-only mode (--security) flags - Includes comprehensive test coverage with unit tests for all components and integration tests with real config files
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
internal/diff/types.go |
Core data structures (Change, Result, Section, ChangeType) with JSON serialization |
internal/diff/types_test.go |
Unit tests for types with comprehensive coverage of all methods |
internal/diff/engine.go |
Comparison orchestration with context cancellation and section filtering |
internal/diff/engine_test.go |
Engine tests covering identical configs, changes, and context cancellation |
internal/diff/analyzer.go |
Content-aware structural analysis for all OPNsense sections (745 lines) |
internal/diff/analyzer_test.go |
Comprehensive analyzer tests with multiple scenarios per section |
internal/diff/integration_test.go |
Integration tests with real config files and build tag isolation |
internal/diff/formatters/terminal.go |
Color-coded terminal output with lipgloss styling and TERM=dumb support |
internal/diff/formatters/terminal_test.go |
Terminal formatter tests with style toggling for verification |
internal/diff/formatters/markdown.go |
Markdown output with tables, collapsible details, and special character escaping |
internal/diff/formatters/markdown_test.go |
Markdown formatter tests including pipe character escaping |
internal/diff/formatters/json.go |
JSON formatter with pretty and compact modes |
internal/diff/formatters/json_test.go |
JSON formatter tests validating output structure |
cmd/diff.go |
CLI command with comprehensive help, flag validation, and shell completion |
| changes = append(changes, Change{ | ||
| Type: ChangeModified, | ||
| Section: SectionFirewall, | ||
| Path: fmt.Sprintf("filter.rule[uuid=%s]", uuid), | ||
| Description: "Modified rule: " + ruleDescription(newRule), | ||
| OldValue: formatRule(oldRule), | ||
| NewValue: formatRule(newRule), |
There was a problem hiding this comment.
Missing security impact assessment for modified firewall rules. When a firewall rule is modified (line 132-141), no SecurityImpact is assigned, unlike added rules (which check for permissive rules on line 121-122) and removed rules (which always get "medium" on line 111). A modified rule could have security implications if it becomes more permissive (e.g., changing from specific port to any port, or from specific source to any source). Consider adding security impact analysis for modifications similar to the logic used for added rules.
| changes = append(changes, Change{ | |
| Type: ChangeModified, | |
| Section: SectionFirewall, | |
| Path: fmt.Sprintf("filter.rule[uuid=%s]", uuid), | |
| Description: "Modified rule: " + ruleDescription(newRule), | |
| OldValue: formatRule(oldRule), | |
| NewValue: formatRule(newRule), | |
| impact := "" | |
| // Flag cases where the modified rule becomes permissive while the old rule was not. | |
| if isPermissiveRule(newRule) && !isPermissiveRule(oldRule) { | |
| impact = "high" | |
| } | |
| changes = append(changes, Change{ | |
| Type: ChangeModified, | |
| Section: SectionFirewall, | |
| Path: fmt.Sprintf("filter.rule[uuid=%s]", uuid), | |
| Description: "Modified rule: " + ruleDescription(newRule), | |
| OldValue: formatRule(oldRule), | |
| NewValue: formatRule(newRule), | |
| SecurityImpact: impact, |
| "slices" | ||
| "strings" | ||
|
|
||
| "github.com/EvilBit-Labs/opnDossier/internal/schema" |
There was a problem hiding this comment.
Package import inconsistency within the diff module. The implementation files use different packages: internal/diff/engine.go and internal/diff/integration_test.go use internal/model, while internal/diff/analyzer.go and test files use internal/schema. While both work (model re-exports schema types), this inconsistency reduces code maintainability. Consider standardizing on one package throughout the diff module. Since internal/schema is the canonical package (see internal/model/system.go:1 which states "Package model re-exports types from internal/schema for backward compatibility"), using internal/schema consistently would be preferred.
| // Get all sample config files | ||
| configs := []string{} | ||
| for i := 1; i <= 7; i++ { | ||
| path := filepath.Join(testdataDir, "sample.config."+string(rune('0'+i))+".xml") |
There was a problem hiding this comment.
String concatenation bug in filename generation. The expression string(rune('0'+i)) converts an integer to a rune character incorrectly. When i=1, this gives '0'+1 = '1' (ASCII 49), but when i=7, it gives '0'+7 = '7' (ASCII 55). This happens to work for single digits 1-7, but the logic is fragile. Use fmt.Sprintf("sample.config.%d.xml", i) instead for clarity and correctness.
| // Version is the tool version included in metadata. | ||
| const Version = "1.0.0" |
There was a problem hiding this comment.
The Version constant is duplicated. A canonical version constant already exists in internal/constants/constants.go (see line 7). According to project standards, version information should be centralized. Replace this local constant with constants.Version by importing "github.com/EvilBit-Labs/opnDossier/internal/constants" and using constants.Version on line 42 instead of the local Version constant.
| "firewall", | ||
| "nat", | ||
| "interfaces", | ||
| "vlans", | ||
| "dhcp", | ||
| "users", | ||
| "routing", | ||
| "dns", | ||
| "vpn", | ||
| "certificates", | ||
| } | ||
| for _, s := range diffSections { | ||
| if !slices.Contains(validSections, strings.ToLower(s)) { | ||
| return fmt.Errorf("invalid section %q, must be one of: %s", | ||
| s, strings.Join(validSections, ", ")) | ||
| } | ||
| } |
There was a problem hiding this comment.
Mismatch between section validation and shell completion. The validSections list here includes: system, firewall, nat, interfaces, vlans, dhcp, users, routing, dns, vpn, certificates. However, the ValidSections completion function in cmd/shared_flags.go:188-196 only provides completions for: system, network, firewall, services, security. This inconsistency will confuse users as the shell completion won't suggest valid sections like "nat", "interfaces", "vlans", etc. Either update the ValidSections function to match these sections, or create a new diff-specific completion function that's registered on line 67.
| "firewall", | |
| "nat", | |
| "interfaces", | |
| "vlans", | |
| "dhcp", | |
| "users", | |
| "routing", | |
| "dns", | |
| "vpn", | |
| "certificates", | |
| } | |
| for _, s := range diffSections { | |
| if !slices.Contains(validSections, strings.ToLower(s)) { | |
| return fmt.Errorf("invalid section %q, must be one of: %s", | |
| s, strings.Join(validSections, ", ")) | |
| } | |
| } | |
| "network", | |
| "firewall", | |
| "services", | |
| "security", | |
| } | |
| for _, s := range diffSections { | |
| if !slices.Contains(validSections, strings.ToLower(s)) { | |
| return fmt.Errorf("invalid section %q, must be one of: %s", | |
| s, strings.Join(validSections, ", ")) | |
| } | |
| } |
coderabbitai
Bot
added
documentation
- Add security impact assessment for modified firewall rules that become more permissive (high impact when rule changes from restrictive to any-any) - Use constants.Version instead of duplicate local Version constant - Fix string concatenation bug in integration test (use fmt.Sprintf) - Add ValidDiffSections completion function with all diff-specific sections to fix mismatch with shell completion Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Critical fixes: - Add file.Sync() before close in outputDiffResult to prevent data loss - Add nil pointer checks to all analyzer comparison functions Important improvements: - Validate and reject unimplemented sections (dns, vpn, certificates) - Fix Interface.Get() return handling (bool, not error) - Fix rulesEqual to include Interface field comparison - Add explicit switch cases for unimplemented sections in engine Type safety enhancements: - Add IsValid() method for ChangeType validation - Add ImplementedSections(), IsValid(), IsImplemented() for Section - Add SecurityImpact enum type with High/Medium/Low constants Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
Implements GitHub Issue #200: a content-aware, security-focused configuration diff tool for comparing OPNsense XML configuration files.
--section firewall,nat,interfaces--securityflagChanges
New Files (14 files, 3424 lines)
cmd/diff.go- CLI command with Cobra frameworkinternal/diff/types.go- Core data structures (Change, Result, Section, ChangeType)internal/diff/engine.go- Comparison orchestrationinternal/diff/analyzer.go- Content-aware structural analysis for all OPNsense sectionsinternal/diff/formatters/terminal.go- Color-coded terminal output with Lipglossinternal/diff/formatters/markdown.go- Markdown formatted outputinternal/diff/formatters/json.go- JSON structured outputinternal/diff/integration_test.go- Integration tests with real config filesSections Supported
Usage Examples
Test plan
just ci-checkpasses (0 linting issues, all tests pass)Closes #200
🤖 Generated with Claude Code