Skip to content

fix: add setvar quotes #68

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Agustindeleon wants to merge 3 commits into
base: main
Choose a base branch
from
+190 −3
Open

fix: add setvar quotes #68

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ce5bff0
fix: add single quotes to setvar actions
Agustindeleon Mar 21, 2026
773f830
test: add test cases
Agustindeleon Mar 21, 2026
15b233d
feat: add new pipeline to check syntax
Agustindeleon Mar 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions .github/workflows/modsec_check.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
name: Validate ModSecurity Rules with Apache

on:
push:
branches: ["main"]
pull_request:
branches: ["main"]

jobs:
modsecurity-syntax:
runs-on: ubuntu-latest
container:
image: rockylinux:9

steps:
- name: Checkout repo
uses: actions/checkout@v4

- name: Install dependencies
run: |
dnf -y install epel-release
dnf -y install httpd mod_security
dnf -y install golang
Comment on lines +19 to +23
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
- 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

Copilot uses AI. Check for mistakes.
dnf -y install git

- name: Clone OWASP CRS Project
run: |
git clone https://github.com/coreruleset/coreruleset.git crs
cd crs
git checkout 8edc58f3ef4d664577e14e5132072559ff30cb34

- name: Generate rules
run: |
go run . -o output crs/rules/
go run . -s -o /etc/httpd/modsecurity.d/activated_rules output.yaml
cp crs/rules/*.data /etc/httpd/modsecurity.d/activated_rules/

- name: Configure ModSecurity
run: |
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
Comment on lines +40 to +46
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

- name: Validate config with httpd -t
run: |
httpd -t
6 changes: 3 additions & 3 deletions types/actions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,9 @@ func (a SetvarAction) ToString() string {
var result []string
// Reconstruct the setvar actions
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, ",")
Comment on lines 173 to +176
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}

func (a *SetvarAction) AppendAssignment(variable, value string) error {
Expand All @@ -189,7 +189,7 @@ func (a SetvarAction) GetAllParams() []string {
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)
}
Comment on lines 189 to 194
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
return result
Expand Down
137 changes: 137 additions & 0 deletions types/actions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,3 +239,140 @@ func TestActionStringMethods(t *testing.T) {
assert.Equal(t, "unknown", NonDisruptiveUnknown.String())
})
}

func TestActionToStringMethods(t *testing.T) {
t.Run("ActionOnly ToString for all action keys", func(t *testing.T) {
tests := []struct {
name string
action ActionOnly
expected string
}{
{name: "disruptive allow", action: ActionOnly(Allow.String()), expected: Allow.String()},
{name: "disruptive block", action: ActionOnly(Block.String()), expected: Block.String()},
{name: "disruptive deny", action: ActionOnly(Deny.String()), expected: Deny.String()},
{name: "disruptive drop", action: ActionOnly(Drop.String()), expected: Drop.String()},
{name: "disruptive pass", action: ActionOnly(Pass.String()), expected: Pass.String()},
{name: "disruptive pause", action: ActionOnly(Pause.String()), expected: Pause.String()},
{name: "flow chain", action: ActionOnly(Chain.String()), expected: Chain.String()},
{name: "non disruptive auditlog", action: ActionOnly(AuditLog.String()), expected: AuditLog.String()},
{name: "non disruptive capture", action: ActionOnly(Capture.String()), expected: Capture.String()},
{name: "non disruptive log", action: ActionOnly(Log.String()), expected: Log.String()},
{name: "non disruptive multiMatch", action: ActionOnly(MultiMatch.String()), expected: MultiMatch.String()},
{name: "non disruptive noauditlog", action: ActionOnly(NoAuditLog.String()), expected: NoAuditLog.String()},
{name: "non disruptive nolog", action: ActionOnly(NoLog.String()), expected: NoLog.String()},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.Equal(t, tt.expected, tt.action.ToString())
})
}
})

t.Run("ActionWithParam ToString for all action keys", func(t *testing.T) {
tests := []struct {
name string
action ActionWithParam
expected string
}{
{name: "proxy", action: ActionWithParam{Proxy.String(): "value"}, expected: "proxy:'value'"},
{name: "redirect", action: ActionWithParam{Redirect.String(): "value"}, expected: "redirect:'value'"},
{name: "skip", action: ActionWithParam{Skip.String(): "value"}, expected: "skip:'value'"},
{name: "skipAfter", action: ActionWithParam{SkipAfter.String(): "value"}, expected: "skipAfter:'value'"},
{name: "status", action: ActionWithParam{Status.String(): "500"}, expected: "status:'500'"},
{name: "xmlns", action: ActionWithParam{XLMNS.String(): "ns"}, expected: "xmlns:'ns'"},
{name: "append", action: ActionWithParam{Append.String(): "value"}, expected: "append:'value'"},
{name: "ctl", action: ActionWithParam{Ctl.String(): "ruleEngine=Off"}, expected: "ctl:ruleEngine=Off"},
{name: "deprecatevar", action: ActionWithParam{DeprecateVar.String(): "value"}, expected: "deprecatevar:'value'"},
{name: "exec", action: ActionWithParam{Exec.String(): "value"}, expected: "exec:'value'"},
{name: "expirevar", action: ActionWithParam{ExpireVar.String(): "value"}, expected: "expirevar:'value'"},
{name: "initcol", action: ActionWithParam{InitCol.String(): "value"}, expected: "initcol:'value'"},
{name: "logdata", action: ActionWithParam{LogData.String(): "value"}, expected: "logdata:'value'"},
{name: "prepend", action: ActionWithParam{Prepend.String(): "value"}, expected: "prepend:'value'"},
{name: "sanitiseArg", action: ActionWithParam{SanitiseArg.String(): "value"}, expected: "sanitiseArg:'value'"},
{name: "sanitiseMatched", action: ActionWithParam{SanitiseMatched.String(): "value"}, expected: "sanitiseMatched:'value'"},
{name: "sanitiseMatchedBytes", action: ActionWithParam{SanitiseMatchedBytes.String(): "value"}, expected: "sanitiseMatchedBytes:'value'"},
{name: "sanitiseRequestHeader", action: ActionWithParam{SanitiseRequestHeader.String(): "value"}, expected: "sanitiseRequestHeader:'value'"},
{name: "sanitiseResponseHeader", action: ActionWithParam{SanitiseResponseHeader.String(): "value"}, expected: "sanitiseResponseHeader:'value'"},
{name: "setuid", action: ActionWithParam{SetUid.String(): "value"}, expected: "setuid:'value'"},
{name: "setrsc", action: ActionWithParam{SetRsc.String(): "value"}, expected: "setrsc:'value'"},
{name: "setsid", action: ActionWithParam{SetSid.String(): "value"}, expected: "setsid:'value'"},
{name: "setenv", action: ActionWithParam{SetEnv.String(): "value"}, expected: "setenv:'value'"},
{name: "setvar", action: ActionWithParam{SetVar.String(): "tx.flag=on"}, expected: "setvar:'tx.flag=on'"},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.Equal(t, tt.expected, tt.action.ToString())
})
}
})

t.Run("SetvarAction ToString cases", func(t *testing.T) {
tests := []struct {
name string
action SetvarAction
expected string
}{
{
name: "with string assignments",
action: SetvarAction{
Collection: TX,
Operation: Assign,
Assignments: []VarAssignment{
{Variable: "test", Value: "critical"},
{Variable: "test2", Value: "payload with spaces"},
},
},
expected: "setvar:'TX.test=critical',setvar:'TX.test2=payload with spaces'",
},
{
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
{
{
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'`,
},
{

Copilot uses AI. Check for mistakes.
name: "numeric assignments",
action: SetvarAction{
Collection: TX,
Operation: Assign,
Assignments: []VarAssignment{
{Variable: "counter", Value: "1"},
{Variable: "score", Value: "5"},
},
},
expected: "setvar:'TX.counter=1',setvar:'TX.score=5'",
},
{
name: "numeric assignments with increment operation",
action: SetvarAction{
Collection: TX,
Operation: Increment,
Assignments: []VarAssignment{
{Variable: "counter", Value: "1"},
{Variable: "score", Value: "5"},
},
},
expected: "setvar:'TX.counter=+1',setvar:'TX.score=+5'",
},
{
name: "numeric assignments with decrement operation",
action: SetvarAction{
Collection: TX,
Operation: Decrement,
Assignments: []VarAssignment{
{Variable: "counter", Value: "1"},
{Variable: "score", Value: "5"},
},
},
expected: "setvar:'TX.counter=-1',setvar:'TX.score=-5'",
},
{
name: "without assignments",
action: SetvarAction{Collection: TX, Operation: Assign},
expected: "",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.Equal(t, tt.expected, tt.action.ToString())
})
}
})
}
Loading