Repository navigation
Conversation
Lua::applyTransformations() had four problems: - It pushed each entry of a transformation table with lua_rawgeti() and never popped it, so a table longer than LUA_MINSTACK wrote past the Lua stack and crashed the process. - It applied each transformation as soon as it read the name, so a "none" couldn't undo the ones before it, as it does in v2 and as the comment says. - It called luaL_checkstring() on the entries while getvar()'s std::string was alive, and the Lua error it raises on a non-string entry longjmps past the destructor. - m.getvars() didn't call it at all, so its transformation argument was ignored. Split it in two. getTransformationNames() reads the names first, pops each entry, resets the list on "none", and skips a non-string entry with a debug message instead of raising a Lua error. applyTransformations() then applies the names to a value. getvar() and getvars() both use them, getvars() on each value. Also fix the type name in the debug message for a parameter that is neither a string nor a table: lua_typename() takes a type, not a stack index. Add regression tests for a 100-entry table, "none" after and before a transformation, a non-string entry, and m.getvars() with a transformation name and with a table. Fixes owasp-modsecurity#3737, v3
Findings
 |
| Layer / File(s) | Summary |
|---|---|
Parse and apply transformation names src/engine/lua.h, src/engine/lua.cc |
The Lua interface now uses a vector of transformation names. Argument parsing collects names, skips and logs non-string table entries, and clears previously collected names when it encounters "none". A separate helper applies the names and logs invalid transformations. |
Integrate transformations and add regression coverage src/engine/lua.cc, test/test-cases/data/lua-tfn-*.lua, test/test-cases/regression/misc-lua-getvar-transformations.json, test/test-suite.in |
getvar and getvars parse optional transformation arguments and apply transformations to resolved values. Regression cases cover long tables, "none" ordering, non-string entries, invalid names, and string and table arguments to getvars. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix · Severity of issue fixed: Medium
Possibly related PRs
- owasp-modsecurity/ModSecurity#3732: Both changes avoid Lua argument checks that can raise errors while C++ objects are alive.
- owasp-modsecurity/ModSecurity#3647: This PR tests
htmlEntityDecodethroughgetvars; that PR changes the transformation’s behavior.
Suggested reviewers: eduar-hte
Merge Risk: 
feac5
-
Misspelled transformations can leave values unchanged without a diagnostic.
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 "fix: Lua m.getvar()/m.getvars() transformation arguments" names the main change. The PR changes how Lua transformation arguments are parsed and applied for m.getvar() and m.getvars(), inclu… Linked Issues check Directly linked issue #3737requires a v3 fix for long transformation tables, the "none" reset, and unsafe non-string table entries.getTransformationNames()pops every table entry, including skippe…Out of Scope Changes check The changed production code remains within the transformation arguments of Lua m.getvar()andm.getvars(). ThegetTransformationNames()andapplyTransformations()split supports issue#3737. A…Full details: Title check
-
Explanation
The title "fix: Lua m.getvar()/m.getvars() transformation arguments" names the main change. The PR changes how Lua transformation arguments are parsed and applied for m.getvar() and m.getvars(), including clearing names on "none", skipping non-string entries, and applying transformations to m.getvars() results. The title is specific and short. It does not mention the stack overrun fix or the new regression tests, which is acceptable because the title covers the primary change.
Full details: Linked Issues check
-
Explanation
Directly linked issue
#3737requires a v3 fix for long transformation tables, the "none" reset, and unsafe non-string table entries.getTransformationNames()pops every table entry, including skipped entries, so a 100-entry table does not grow the Lua stack. It clears collected names on "none" and applies only the remaining names. It reads names beforegetvar()creates itsstd::stringand skips non-string entries with a level 8 message. The implementation also rejects numeric entries and colon-containing names without applying them. Regression cases cover the long table, both "none" positions, non-string and numeric entries,getvars()transformations, and colon handling. The separate v2 fix is not required by the issue scope.
Full details: Out of Scope Changes check
-
Explanation
The changed production code remains within the transformation arguments of Lua
m.getvar()andm.getvars(). ThegetTransformationNames()andapplyTransformations()split supports issue#3737. Applying names to eachm.getvars()value fixes the same transformation argument behavior. Numeric-entry rejection, colon-name rejection, corrected type reporting, and the added regression cases support safe parsing and validation in this code path. No unrelated production change is 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 8 functions across 10 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 were identified for the Lua variable-lookup transformation changes.
No open issues 🎉
What changed in this PR
Fixes transformation handling in ModSecurity’s Lua variable lookups, addressing #3737 and making m.getvar() and m.getvars() consistent.
Changes:
- Separates transformation parsing from application, balances Lua stack usage, and makes
"none"reset the transformation list. - Applies transformations to
m.getvars()results and skips invalid entries with diagnostic logging. - Adds six regression cases covering long lists, reset behavior, invalid entries, and transformed multi-variable lookups.
| File | Description |
|---|---|
test/test-suite.in |
Registers the new regression suite. |
test/test-cases/regression/misc-lua-getvar-transformations.json |
Defines six transformation regression cases. |
test/test-cases/data/lua-tfn-not-a-string.lua |
Checks that invalid entries are skipped. |
test/test-cases/data/lua-tfn-none-last.lua |
Checks that "none" clears earlier transformations. |
test/test-cases/data/lua-tfn-none-first.lua |
Checks that transformations after "none" apply. |
test/test-cases/data/lua-tfn-long-table.lua |
Exercises a 100-entry transformation list. |
test/test-cases/data/lua-tfn-getvars-table.lua |
Checks ordered transformations in m.getvars(). |
test/test-cases/data/lua-tfn-getvars-string.lua |
Checks a single transformation in m.getvars(). |
src/engine/lua.h |
Declares separate parsing and application helpers. |
src/engine/lua.cc |
Implements stack-safe parsing, reset behavior, and transformations for both lookup APIs. |
Review effort: Balanced
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Two cases the first commit missed (found by CodeRabbit): - lua_isstring() is true for a number too, so a table entry like 12 was converted to the name "12" instead of being skipped with the debug message. Check for LUA_TSTRING instead, for a table entry and for a single transformation name. - A name ending in ':', such as "lowercase:", still matched the transformation by its prefix, and the Action constructor then threw std::out_of_range on the empty payload. Nothing catches it on the way out of the Lua C function. Transformations take no argument, so applyTransformations() now rejects any name with a ':' with the invalid transformation message. This was already reachable from m.getvar() before this PR. Also declare the single name in the if statement (SonarCloud S6004). Add regression cases for both.
|
resolve_tfns() read each table entry with luaL_checkstring(), so an entry that isn't a string or a number, such as a table, raised a Lua error and failed the whole script with "bad argument", and a number was converted to a name that never resolves. Check the type with lua_type() instead, and skip an entry that isn't a string with a debug message, as v3 does since owasp-modsecurity#3738. Check a single transformation name the same way; a number there now gets the "Transformation parameter must be ..." message. Add a regression test with a table and a number entry. Refs owasp-modsecurity#3737
resolve_tfns() passed the stack index to lua_typename(), which takes a type, so the message for a transformation parameter that is neither a string nor a table named the wrong type: "userdata" for a boolean, for example. v3 had the same bug, fixed in owasp-modsecurity#3738. Add a regression test with a boolean parameter. Refs owasp-modsecurity#3737
what
Lua::applyTransformations()(src/engine/lua.cc) is split in two:getTransformationNames()reads the transformation argument ofm.getvar()/m.getvars()into a list of names. It pops each table entry after reading it, empties the list on"none", and skips an entry that isn't a string with a level 8 debug message.applyTransformations()applies a list of names to a value.Lua::getvar()reads the names before it creates itsstd::string, and applies them to the value.Lua::getvars()now applies the transformations too, to each value it returns.The debug message for a transformation parameter that is neither a string nor a table now names the right type:
lua_typename()takes a type, not a stack index.Add
test/test-cases/regression/misc-lua-getvar-transformations.json, registered intest/test-suite.in, with 6 cases and 6 Lua scripts undertest/test-cases/data/(lua-tfn-*.lua), all with?a=ABC&b=%26LT%3BSCRIPT:m.getvar("ARGS:a", t)with 100"lowercase"entries:abc;{ "lowercase", "none" }:ABC;{ "none", "lowercase" }:abc, as a control;{ "lowercase", {} }: the debug message, thenabc;m.getvars("ARGS:a", "lowercase"):abc;m.getvars("ARGS:b", { "lowercase", "htmlEntityDecode" }):<script.On the current
v3/master, without the fix, case 1 crashes the test runner with a segmentation fault, cases 2, 4, 5 and 6 fail, and case 3 (the control) passes. With the fix, all 6 pass, and so does the wholemake checksuite.why
LUA_MINSTACK(20) free stack slots unless it callslua_checkstack(), so a longer table wrote past the Lua stack. @fzipi got a segmentation fault with 100 entries (Lua m.getvar()/m.getvars(): long transformation tables crash, and "none" doesn't reset in v3 #3737)."none"didn't start over. Each transformation was applied as soon as its name was read, so thecontinueon"none"couldn't undo the ones already applied. v2'sresolve_tfns()empties its list, and the comment says the same. Collecting the names first makes v3 behave the same way.luaL_checkstring()could skip C++ destructors. On a non-string entry it raises a Lua error, which with the usual C build of Lua is alongjmp, whilegetvar()'sstd::string varwas alive. The names are now read withlua_isstring()/lua_tostring(), which don't raise an error, and beforevarexists. A non-string entry is skipped with a debug message instead of failing the script. This is a behavior change: such a script used to fail with "bad argument".m.getvars()ignored its transformation argument. It never calledapplyTransformations(), while v2'sl_getvars()callsresolve_tfns(). Som.getvars("ARGS", { "lowercase", "htmlEntityDecode" }), the example in the SecRuleScript documentation, returned the raw values in v3, and a script looking for<scriptthat way missed<SCRIPT. This is a behavior change too: a v3 script that passed transformations tom.getvars()now gets transformed values, as it would in v2.resolve_tfns()inapache2/msc_lua.c); that's fixed in a separate PR.references
luaL_checkstring()andlongjmppattern inm.setvar()AI disclosure:
Made with help of Claude Code, Opus 5.5
Summary by CodeRabbit
Summary by CodeRabbit
noneoption clears transformations collected earlier in a list. Non-string entries are skipped, and unsupported option types do not apply transformations.