Conversation
There was a problem hiding this comment.
Pull request overview
Adds consistent single-quote wrapping for setvar action rendering to prevent SecLang formatting issues (notably values containing spaces), and introduces a new CI workflow to validate generated rules via Apache/ModSecurity.
Changes:
- Quote rendered
setvarassignments (and adjust delimiter formatting) inSetvarActionstring rendering. - Add unit tests covering
ToString()behavior forActionOnly,ActionWithParam, andSetvarAction. - Add a GitHub Actions workflow that generates rules and runs
httpd -tin a Rocky Linux container.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
types/actions.go |
Wraps setvar assignments in single quotes during rendering and updates join formatting. |
types/actions_test.go |
Adds new ToString() test coverage for actions, including setvar cases. |
.github/workflows/modsec_check.yml |
Introduces an Apache/ModSecurity-based syntax validation workflow. |
| var result []string | ||
| // Get all the variables | ||
| for _, asg := range a.Assignments { | ||
| res := SetVar.String() + ":" + a.Collection.String() + "." + asg.Variable + a.Operation.String() + asg.Value | ||
| res := SetVar.String() + ":'" + a.Collection.String() + "." + asg.Variable + a.Operation.String() + asg.Value + "'" | ||
| result = append(result, res) | ||
| } |
There was a problem hiding this comment.
Related to quoting: GetAllParams duplicates the quoting logic from ToString. To avoid inconsistent escaping/formatting over time, consider centralizing the setvar rendering (including escaping) in a single helper that both methods call.
| }, | ||
| expected: "setvar:'TX.test=critical',setvar:'TX.test2=payload with spaces'", | ||
| }, | ||
| { |
There was a problem hiding this comment.
The new setvar quoting behavior isn’t exercised for values containing embedded single quotes/backslashes. Adding a test case like Value: "O'Reilly" (and asserting proper escaping in the rendered output) would help ensure the generated SecLang stays valid for this edge case.
| { | |
| { | |
| name: "with embedded quotes and backslashes", | |
| action: SetvarAction{ | |
| Collection: TX, | |
| Operation: Assign, | |
| Assignments: []VarAssignment{ | |
| {Variable: "publisher", Value: "O'Reilly"}, | |
| {Variable: "path", Value: `C:\Temp\O'Reilly`}, | |
| }, | |
| }, | |
| expected: `setvar:'TX.publisher=O\'Reilly',setvar:'TX.path=C:\\Temp\\O\'Reilly'`, | |
| }, | |
| { |
| - name: Install dependencies | ||
| run: | | ||
| dnf -y install epel-release | ||
| dnf -y install httpd mod_security | ||
| dnf -y install golang |
There was a problem hiding this comment.
Installing Go via dnf install golang makes the workflow sensitive to whatever Go version the base image ships (which may be < the module’s go 1.22.2). To keep CI aligned with go.mod, use actions/setup-go with go-version-file: go.mod (and drop the OS package install).
| - name: Install dependencies | |
| run: | | |
| dnf -y install epel-release | |
| dnf -y install httpd mod_security | |
| dnf -y install golang | |
| - name: Set up Go | |
| uses: actions/setup-go@v5 | |
| with: | |
| go-version-file: go.mod | |
| - name: Install dependencies | |
| run: | | |
| dnf -y install epel-release | |
| dnf -y install httpd mod_security |
| cat <<EOF > /etc/httpd/conf.d/mod_security.conf | ||
| <IfModule security2_module> | ||
| SecRuleEngine On | ||
| SecDataDir /tmp | ||
| Include /etc/httpd/modsecurity.d/activated_rules/*.conf | ||
| </IfModule> | ||
| EOF |
There was a problem hiding this comment.
Because the ModSecurity directives are wrapped in <IfModule security2_module>, httpd -t can succeed even if the module isn’t actually loaded (in which case the rules won’t be parsed at all). To ensure this job truly validates ModSec syntax, either explicitly load/check the module (e.g., assert httpd -M contains security2_module) or avoid the conditional so missing module fails the run.
| for _, asg := range a.Assignments { | ||
| result = append(result, SetVar.String()+":"+a.Collection.String()+"."+asg.Variable+a.Operation.String()+asg.Value) | ||
| result = append(result, SetVar.String()+":'"+a.Collection.String()+"."+asg.Variable+a.Operation.String()+asg.Value+"'") | ||
| } | ||
| return strings.Join(result, ", ") | ||
| return strings.Join(result, ",") |
There was a problem hiding this comment.
SetvarAction.ToString now wraps the whole assignment in single quotes, but it does not escape any single quotes/backslashes that may appear in the collection/variable/value. If a YAML value contains an unescaped ' (e.g., "O'Reilly"), the generated SecLang will be syntactically invalid. Consider adding a small escaping helper (at least replacing ' with \', and being careful with existing backslashes) before concatenating into the quoted string.
What
setvaractions.Why
setvarassignments containing spaces break the Seclang format.