Repository navigation
Conversation
Lua::setvar() returned -1 on its error paths (wrong number of arguments, no collection in the name). A lua_CFunction returns the number of values it pushed, so Lua returned whatever was on the stack: the script got the __transaction pointer or a stray string. Return 0 so the script gets nil. Also call luaL_checkstring() before the std::string locals exist, since it raises a Lua error with longjmp on a non-string argument, drop the NULL check it makes unreachable, and stop popping the arguments while var_value still points into them. Fixes owasp-modsecurity#3731 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RiUJuqXNvvpytiGRiDFyTy
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Argument checks and return handling src/engine/lua.cc |
setvar checks both arguments before constructing local strings. It returns zero values for an incorrect argument count or a name without a collection separator. It logs unknown collection names. |
Regression coverage test/test-cases/data/setvar-*.lua, test/test-cases/regression/collection-lua.json |
New Lua scripts and regression cases check missing and invalid arguments, unknown collections, successful TX assignment, and a non-string argument error. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Possibly related issues
- Issue 3655 lists the v3 Lua
setvarnegative return values as one scope item. This change addresses those return values; other issue items are not covered.
Suggested reviewers: airween
Merge Risk: 
3b81d
-
The changed Lua setvar behavior satisfies the stated requirements and no merge-blocking risk remains.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly identifies the main change: fixing Lua m.setvar() error paths that returned -1 and exposed invalid script-stack values. Linked Issues check Issue [ #3731] requires safeLua::setvar()error returns, earlyluaL_checkstring()calls, the corrected debug message, and regression coverage.src/engine/lua.ccreturns0for wrong argument co…Out of Scope Changes check The changes stay within [ #3731]. The stack handling change supports theLua::setvar()return-value fix. The unknown-collection log, TX control case, and non-string case test behavior required by the…Full details: Linked Issues check
-
Explanation
Issue [
#3731] requires safeLua::setvar()error returns, earlyluaL_checkstring()calls, the corrected debug message, and regression coverage.src/engine/lua.ccreturns0for wrong argument counts and missing collection separators, calls both checks before constructingstd::stringobjects, removes the obsolete stack pop and null check, and uses “function must have”. The regression cases verifynil, error logging, unknown collections, successful TX updates, and non-string argument errors.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within [
#3731]. The stack handling change supports theLua::setvar()return-value fix. The unknown-collection log, TX control case, and non-string case test behavior required by the issue discussion. No unrelated source or test changes are shown.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 6 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain, and the changes include targeted validation and regression coverage for the corrected invalid-call behavior.
No open issues 🎉
What changed in this PR
This pull request fixes Lua::setvar() so invalid calls return no Lua values instead of exposing stale stack data, while improving argument validation and adding regression coverage.
Changes:
- Return
0for incorrect argument counts and variable names without collections. - Validate Lua string arguments before constructing C++ strings.
- Remove unsafe stack popping and correct the debug-message typo.
- Add regression cases verifying invalid calls evaluate to
nil.
| File | Description |
|---|---|
test/test-cases/regression/collection-lua.json |
Adds regression cases for invalid m.setvar() calls. |
test/test-cases/data/setvar-error-onearg.lua |
Tests the one-argument error path. |
test/test-cases/data/setvar-error-nodot.lua |
Tests the missing-collection error path. |
src/engine/lua.cc |
Corrects setvar() error handling, argument validation order, stack handling, and debug text. |
Review effort: Lite
| t->m_rules->m_secWebAppId.m_value, | ||
| var_value); | ||
|
|
||
| } |
There was a problem hiding this comment.
An unknown collection, e.g. m.setvar("FOO.bar", "x"), still falls through all the branches below without a message. Could you add a final else after the USER branch (after line 404), with a level 8 message like the one above, and the name escaped as in #3730?
else {
ms_dbg_a(t, 8, "m.setvar: Unknown collection: "
+ utils::string::toHexIfNeeded(collection) + ".");
}There was a problem hiding this comment.
Done in 3b81dca: a final else logs m.setvar: Unknown collection: <name>. at level 8, with the name passed through toHexIfNeeded().
|
I also started to work on your issue #3731, and here are the test cases for that. They follow your two cases, so they can go at the end of
function main()
local r = m.setvar("FOO.bar", "x");
m.log(4, "m.setvar() with an unknown collection returned: " .. tostring(r));
return nil;
end
function main()
local r = m.setvar("tx.a", "x");
m.log(4, "m.setvar() with TX returned: " .. tostring(r));
m.log(4, "tx.a is now: " .. tostring(m.getvar("tx.a")));
return nil;
end
{
"enabled": 1,
"version_min": 300000,
"title": "Testing LUA :: m.setvar with an unknown collection returns nil",
"resource": "lua",
"client": {
"ip": "200.249.12.31",
"port": 123
},
"server": {
"ip": "200.249.12.31",
"port": 80
},
"request": {
"headers": {
"Host": "localhost",
"User-Agent": "My sweet little browser",
"Accept": "*/*",
"Content-Length": "0"
},
"uri": "/whee?res=1",
"method": "GET",
"body": [
""
]
},
"response": {
"headers": {
"Content-Length": "8"
},
"body": [
"no need."
]
},
"expected": {
"debug_log": "m.setvar: Unknown collection: FOO\\..*m.setvar\\(\\) with an unknown collection returned: nil",
"http_code": 200
},
"rules": [
"SecRuleEngine On",
"SecRuleScript test-cases/data/setvar-error-unknown-collection.lua \"id:1,pass\""
]
},
{
"enabled": 1,
"version_min": 300000,
"title": "Testing LUA :: m.setvar sets a TX variable and returns nil",
"resource": "lua",
"client": {
"ip": "200.249.12.31",
"port": 123
},
"server": {
"ip": "200.249.12.31",
"port": 80
},
"request": {
"headers": {
"Host": "localhost",
"User-Agent": "My sweet little browser",
"Accept": "*/*",
"Content-Length": "0"
},
"uri": "/whee?res=1",
"method": "GET",
"body": [
""
]
},
"response": {
"headers": {
"Content-Length": "8"
},
"body": [
"no need."
]
},
"expected": {
"debug_log": "m.setvar\\(\\) with TX returned: nil.*tx.a is now: x",
"http_code": 200
},
"rules": [
"SecRuleEngine On",
"SecRuleScript test-cases/data/setvar-tx.lua \"id:1,pass\""
]
}The first one needs the new Feel free to add them to this PR. |
m.setvar("FOO.bar", "x") fell through every collection branch and was
ignored without a message. Log it at level 8, like the other m.setvar()
errors, with the name escaped.
Tests: assert the debug message on each error path as well as the nil
return, since m.setvar() also returns nil on success. Add cases for an
unknown collection, a successful TX set (both from @airween), and a
non-string value, which must fail the script with a Lua error.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RiUJuqXNvvpytiGRiDFyTy
|
Thanks @airween. I added both of your cases as they are in 3b81dca, with the new
v2's |
|
what
Fixes three problems in
Lua::setvar()(src/engine/lua.cc):0instead of-1. This covers the wrong number of arguments and a name with no collection. The script now getsnil.luaL_checkstring()is called before thestd::stringlocals are constructed. It raises a Lua error withlongjmpon a non-string argument, which would skip their destructors. This makes theNULLcheck after it unreachable, so that check is removed.lua_pop(L, 2)is removed. It popped the value argument whilevar_valuestill pointed into it. Alua_CFunction's stack is cleared on return anyway.why
A
lua_CFunctionreturns the number of values it pushed. With-1, Lua returned whatever was on the stack. With the two new test scripts on v3/master,m.setvar("novar", "x")returned the__transactionpointer (userdata: 0x…), andm.setvar("tx.a")returned a stray string.tests
collection-lua.json, with the scriptssetvar-error-nodot.luaandsetvar-error-onearg.lua, check that the script getsnil. Both fail on v3/master and pass with this change.action-exec,directive-sec_rule_script,operator-inpectFileandissue-2427.make check(macOS, Lua 5.5.1, PCRE2): 5139 total, 0 fail.references
🤖 Generated with Claude Code
Summary by CodeRabbit