feat(diff): add HTML formatter, side-by-side mode, analyzers, and security scoring - #245
Conversation
- Implemented SideBySideFormatter to display diff results in a two-column layout. - Added fallback to unified format for narrow terminals. - Included detailed formatting for changes, including security impact badges. - Created tests for the new formatter to ensure correct functionality. feat(security): introduce security impact scoring system - Added Pattern struct to define security impact matching rules. - Implemented Scorer to evaluate security impacts based on configuration changes. - Introduced RiskSummary and RiskItem to aggregate and report security risks. - Added default patterns for common security risks in configurations. - Created tests for scoring logic and pattern matching. fix(diff): enhance section filtering and change type handling - Added ChangeReordered type to track reordered elements. - Improved section inclusion checks to be case-insensitive. - Updated Result struct to include RiskSummary for comprehensive reporting. refactor(report): streamline markdown report generation - Refactored report generation to use a fluent interface for markdown building. - Consolidated statistics and findings sections for better readability. - Improved overall structure and formatting of the report output. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdds HTML output and a side‑by‑side terminal formatter, a formatter factory, normalization and reorder-detection analyzers, pattern-based security scoring with RiskSummary, engine wiring to apply normalization/reorder detection/scoring, and extensive formatter/analyzer/golden tests and CLI flags. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Diff CLI
participant Engine as Diff Engine
participant Normal as Normalizer
participant OrderDet as OrderDetector
participant Scorer as Security Scorer
participant Factory as Formatter Factory
participant Formatter as Concrete Formatter
participant Writer as Output Writer
CLI->>Engine: Compare(options, oldCfg, newCfg)
alt DetectOrder enabled
Engine->>OrderDet: DetectReorders(oldRuleIDs, newRuleIDs)
OrderDet-->>Engine: []OrderChange
end
loop per change
alt Normalize enabled
Engine->>Normal: NormalizeValue(old/new)
Normal-->>Engine: normalized value
end
Engine->>Scorer: Score(change) // preserves explicit SecurityImpact or matches patterns
Scorer-->>Engine: SecurityImpact
end
Engine->>Scorer: ScoreAll(changes)
Scorer-->>Engine: RiskSummary
Engine->>Factory: NewWithMode(format, mode, Writer)
Factory-->>Formatter: ConcreteFormatter
Engine->>Formatter: Format(result)
Formatter->>Writer: Write(output)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.20.0)AGENTS.mdmarkdownlint-cli2 v0.20.0 (markdownlint v0.40.0) Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Adds new diff output capabilities (HTML + side-by-side terminal) and introduces a security scoring subsystem intended to enrich diff results with risk summaries, fitting into the cmd/diff → internal/diff pipeline.
Changes:
- Introduces a formatter factory + new HTML and side-by-side terminal formatters (with embedded static assets).
- Adds new analyzer utilities (normalization + reorder detection) and a pattern-based security scoring engine with aggregated
RiskSummary. - Extends diff CLI/options/types to support new flags, output mode, and risk summary metadata.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/processor/report.go | Refactors markdown generation to use chained builder calls; adjusts Summary() rendering. |
| internal/diff/types.go | Adds ChangeReordered, risk summary types, and new diff options/behavior knobs. |
| internal/diff/types_test.go | Extends section filtering tests for case-insensitive matches. |
| internal/diff/security/scorer.go | Adds pattern-based scoring and aggregate risk computation. |
| internal/diff/security/scorer_test.go | Adds unit tests for scorer behavior and aggregation. |
| internal/diff/security/risk.go | Defines risk summary model and scoring weights for the security package. |
| internal/diff/security/patterns.go | Defines built-in risk scoring patterns. |
| internal/diff/security/patterns_test.go | Tests for default pattern integrity/uniqueness. |
| internal/diff/formatters/formatter.go | Adds Formatter interface and factory (New, NewWithMode). |
| internal/diff/formatters/formatter_test.go | Tests factory behavior and interface compliance. |
| internal/diff/formatters/html.go | Adds self-contained HTML report formatter (template + embedded assets). |
| internal/diff/formatters/html_test.go | Tests HTML output structure, embedding, and risk/security badge rendering. |
| internal/diff/formatters/terminal_sidebyside.go | Adds side-by-side terminal formatter with width detection + unified fallback. |
| internal/diff/formatters/terminal_sidebyside_test.go | Tests side-by-side formatting behavior and fallback paths. |
| internal/diff/formatters/static/styles.css | Adds embedded CSS for HTML report (dark/light + print). |
| internal/diff/formatters/static/scripts.js | Adds embedded JS for HTML interactivity (toggle/filter). |
| internal/diff/formatters/static/report.html.tmpl | Adds embedded HTML template for diff reports and risk summary section. |
| internal/diff/formatters/markdown.go | Updates security badge mapping to use diff.SecurityImpact* constants. |
| internal/diff/engine.go | Wires in security scoring + aggregate risk summary population. |
| internal/diff/analyzers/normalizer.go | Adds normalization helpers (IP/whitespace/port/protocol/path). |
| internal/diff/analyzers/normalizer_test.go | Unit tests for normalizer behaviors. |
| internal/diff/analyzers/order_detector.go | Adds reorder detection helper for ordered ID lists. |
| internal/diff/analyzers/order_detector_test.go | Unit tests for reorder detection behavior. |
| cmd/diff.go | Adds new CLI flags/completions/help text and uses formatter factory. |
| internal/audit/plugin.go | Collapses package comment formatting. |
| go.mod | Adds golang.org/x/term dependency for terminal sizing. |
Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
- Fix help text: side-by-side mode is terminal-only, not HTML - Wire --normalize and --detect-order flags into diff engine - Fix non-deterministic HTML template iteration by using ordered slice - Add Reordered counter to Summary struct for ChangeReordered tracking - Include error context in report Summary() failure message
- Add Reordered count to side-by-side terminal summary line - Apply security scoring and SecurityOnly filtering to reorder changes - Validate --mode side-by-side only works with --format terminal - Document detectTerminalWidth stdout behavior
- Replace O(n*m) slices.Contains loops with O(1) map lookups in
CompareInterfaces and CompareDHCP
- Add capacity hints to DHCP name slice allocations
- Cache parsed HTML template at package level instead of per-Format call
- Add early exit in scorer when highest impact ("high") is reached
- Consolidate port normalization to single-pass separator detection
- Deduplicate badge text formatting in side-by-side formatter
Replace the standalone HTML template/CSS/JS approach with a pipeline that generates markdown via MarkdownFormatter, converts to HTML via goldmark (already a dependency), and wraps in a minimal HTML shell. This eliminates 3 static asset files (template, CSS, JS), removes the go:embed dependency, and ensures HTML output stays consistent with markdown output since it's derived from the same source.
…rmatters Adds goldie-based snapshot tests covering all diff formatters with three scenarios: no changes, single section, and multi-section with security badges. Includes consistency and structural validation tests for HTML.
Replace duplicate RiskSummary/RiskItem structs with type aliases to security package, eliminating field-by-field copy in computeRiskSummary. Remove redundant golden tests (trivial determinism, structure checks already covered by snapshots) and consolidate interface compliance tests.
- Add nil receiver guard to RiskSummary.HasRisks() - Wrap HTML formatter write error with context - Add ChangeReordered to side-by-side style switch - Use rune-based truncation to prevent UTF-8 garbling - Fix global state leakage in diffSections normalization - Enable goldmark table extension for proper HTML table rendering - Pre-compile regex patterns to package-level variables - Document TopRisks tier-based prioritization strategy - Add IPv6 and invalid CIDR test cases for normalizer - Use require.False/ErrorContains for stronger test assertions - Add t.Parallel() to stateless table-driven tests - Regenerate golden files with proper HTML table elements
- Sort DetectReorders output by ID for deterministic results (partial #243) - Make NormalizePort safe for non-port strings (validate input is numeric) - Fix ANSI escape sequence column misalignment in side-by-side formatter - Enable goldmark unsafe HTML to preserve <details> sections in HTML output - Filter duplicate reorder/modified changes for same firewall rule - Fix misleading --normalize flag description (display normalization, not comparison) - Update ChangeInput.Type comment to include "reordered" - Add 5 engine integration tests for DetectOrder, Normalize, and RiskSummary - Regenerate golden files for HTML formatter
|
Addressed all unresolved Copilot review findings in commit 1787694: Code fixes:
Low priority/deferred:
|
When --normalize is enabled, skip modified changes where normalized old and new values are equal (e.g., IP with different leading zeros). This makes normalize actually reduce noise rather than just reformatting display values. Also extract repeated test path to constant.
Summary
Formatterinterface withNew()andNewWithMode()factory functions, replacing the switch statement inoutputDiffResult()--format html) using markdown-to-HTML conversion via goldmark, with embedded CSS, dark/light theme, collapsible sections, security badges, and print-friendly styles — fully offline, no CDN--mode side-by-side) usinggolang.org/x/termfor width detection, with automatic fallback to unified format on narrow terminals (<80 cols)internal/diff/analyzers/package with IP normalization (handles leading-zero octets), whitespace/port/protocol/path normalization (--normalize), and rule reorder detection (--detect-order)internal/diff/security/package with pattern-based scoring (10 built-in patterns covering firewall, system, NAT, users, interfaces), aggregate risk summaries, and automatic augmentation of changes without existing security impact--format html,--mode unified|side-by-side,--normalize,--detect-orderwith validation, shell completions, and updated help textNew packages
internal/diff/formatters/—Formatterinterface, factory, HTML/side-by-side formattersinternal/diff/analyzers/— Normalizer, OrderDetectorinternal/diff/security/— Scorer, Patterns, RiskSummaryStats
Test Plan
just ci-checkpasses (lint + all tests)opndossier diff old.xml new.xml -f html -o report.htmlproduces valid self-contained HTMLopndossier diff old.xml new.xml -m side-by-siderenders two-column layoutopndossier diff old.xml new.xml --normalize --detect-orderreduces noise🤖 Generated with Claude Code