feat: add css filter #5
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds CSS filtering capabilities to the XSS filter library to prevent XSS attacks through CSS injection. The feature introduces a new cssfilter package that parses and filters CSS style attributes using a whitelist-based approach.
Key changes:
- New
cssfilterpackage with parser, filter, and configuration modules - Integration of CSS filtering into the main XSS processing pipeline via
EnableCssFilteroption - Comprehensive test coverage for CSS parsing and filtering logic
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| cssfilter/parser.go | Implements CSS parsing logic with support for comments, parentheses, and property extraction |
| cssfilter/filter.go | Core CSS filtering implementation with whitelist checking and attribute validation |
| cssfilter/css_option.go | Configuration options and default CSS property whitelist with JavaScript detection |
| cssfilter/parser_test.go | Tests for CSS parser covering various formatting scenarios |
| cssfilter/filter_test.go | Tests for CSS filtering including XSS attack prevention |
| cssfilter/css_option_test.go | Tests for CSS attribute value safety validation |
| xss.go | Integrates CSS filter into main XSS processor based on EnableCssFilter flag |
| xss_option.go | Adds EnableCssFilter and CssFilterOption configuration fields; modifies SafeAttrValue signature |
| xss_test.go | Adds integration test for CSS filtering and updates existing test for new SafeAttrValue signature |
| example_test.go | Provides usage example demonstrating CSS filter functionality |
| README.md | Documents CSS filter configuration in Chinese |
| README.en.md | Documents CSS filter configuration in English |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestFilterWithCssFilter(t *testing.T) { | ||
| source := `<div style="position: fixed; width:100px; height: 200px; background: url (xxx);">hello</div>` | ||
|
|
||
| xssOption := NewDefaultXssOption() | ||
| xssOption.EnableCssFilter = true | ||
| xssOption.WhiteList["div"] = []string{"style"} | ||
|
|
||
| html := FilterXSS(source, xssOption) | ||
|
|
||
| expect := `<div style="width:100px; height:200px; background:url (xxx);">hello</div>` | ||
| if html != expect { | ||
| t.Errorf("TestFilterWithCssFilter expect: %s but:%s", expect, html) | ||
|
|
||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Missing test coverage: There's no test case for when EnableCssFilter is true but CssFilterOption is nil. This scenario should be tested to ensure NewFilterCSS properly handles nil options and uses defaults correctly.
| options: options, | ||
| } | ||
|
|
||
| if options.EnableCssFilter { |
There was a problem hiding this comment.
Potential nil pointer issue: When EnableCssFilter is true but CssFilterOption is nil, this will pass nil to NewFilterCSS. While NewFilterCSS handles nil options, it would be clearer to initialize CssFilterOption with defaults when EnableCssFilter is true but CssFilterOption is not set. Consider adding: if options.EnableCssFilter && options.CssFilterOption == nil { options.CssFilterOption = NewCssFilterOption() }
| if options.EnableCssFilter { | |
| if options.EnableCssFilter { | |
| if options.CssFilterOption == nil { | |
| options.CssFilterOption = cssfilter.NewCssFilterOption() | |
| } |
|
|
||
| OnIgnoreTagAttr func(tag, name, value string, isWhiteAttr bool) *string | ||
|
|
||
| StripTagBody func(tags []string, next OnIgnoreTagFunc) StripTagBodyResult |
There was a problem hiding this comment.
Breaking API change: The SafeAttrValue function signature has been modified to include a cssFilter parameter. This will break existing custom implementations of this callback. Consider documenting this breaking change in a CHANGELOG or migration guide, and ensure the version number is bumped appropriately (following semantic versioning, this should be a major version bump).
| StripTagBody func(tags []string, next OnIgnoreTagFunc) StripTagBodyResult | |
| StripTagBody func(tags []string, next OnIgnoreTagFunc) StripTagBodyResult | |
| // BREAKING CHANGE: SafeAttrValue signature now includes cssFilter *cssfilter.FilterCSS as the fourth parameter. |
| addNewAttr() | ||
| } | ||
| } else if c == '\n' { | ||
| addNewAttr() |
There was a problem hiding this comment.
Inconsistent parenthesis handling: Newlines trigger attribute separation unconditionally (line 70), but semicolons only trigger separation when !isParenthesisOpen (lines 66-68). This inconsistency means that newlines will break CSS properties even inside parentheses, while semicolons are correctly preserved. For consistency, line 69-70 should be wrapped in an if !isParenthesisOpen check, or the security policy should be documented if this is intentional.
| addNewAttr() | |
| if !isParenthesisOpen { | |
| addNewAttr() | |
| } |
No description provided.