Add moderation exception to self destruct#7132
Conversation
📝 WalkthroughWalkthroughAdds an EmmyLua type for the self-destruct data and replaces the direct callback alias with a validating wrapper that enforces owner authorization, rejects empty selections, and in FullShare mode restricts the action to COMMAND (ACU) units when present before delegating to the core self-destruct function. ChangesSelf-Destruct Callback Validation
Sequence Diagram(s)sequenceDiagram
participant Caller as Callbacks.ToggleSelfDestruct
participant Scenario as ScenarioInfo
participant Filter as CategoryFilter
participant SD as selfdestruct.ToggleSelfDestruct
Caller->>Scenario: read Options.Share
Caller->>Filter: filter units by categories.COMMAND
Note over Filter,Scenario: if FullShare and filtered non-empty -> use filtered\nelse -> use original units
Caller->>SD: ToggleSelfDestruct(data, selectedUnits)
SD-->>Caller: done
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lua/SimCallbacks.lua`:
- Around line 185-207: The callback dereferences units and data.owner before
validation; normalize and validate them up front by calling SecureUnits(units)
into a local variable (e.g., local units = SecureUnits(units)) before any
table.getn or EntityCategoryFilterDown calls, and check that data is non-nil and
data.owner exists (if not, return) rather than accessing data.owner earlier;
then use the normalized units variable in the rest of the function (including
the FullShare branch and the final
import("/lua/selfdestruct.lua").ToggleSelfDestruct calls) to avoid crashes on
malformed payloads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37d00683-45e8-4e3f-998e-b074075b3cc6
📒 Files selected for processing (2)
lua/SimCallbacks.lualua/selfdestruct.lua
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lua/SimCallbacks.lua (1)
185-207:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
unitsbefore anytable.getn/forwarding.Line 186 still dereferences raw callback input, so a singleton unit/id or any other non-table payload will throw before this wrapper can return safely.
SecureUnitsalready canonicalizes and filters the selection; normalize once up front and reuse that sanitized table in both the FullShare branch and the fallback call.Suggested fix
Callbacks.ToggleSelfDestruct = function(data, units) - -- prevent malformed input - if (not units) or (table.getn(units) == 0) then + -- prevent malformed input + units = SecureUnits(units) + if TableEmpty(units) then return end -- prevent abuse if (not data) or (not data.owner) or (not OkayToMessWithArmy(data.owner)) then @@ -- moderation rule: if you self destruct with one or more ACUs in the selection when playing full -- share, then you only self destruct the ACUs. This does not make it impossible to abuse, but it -- does introduce a simple guardrail. if ScenarioInfo.Options.Share == "FullShare" then - local commandUnits = EntityCategoryFilterDown(categories.COMMAND, SecureUnits(units)) + local commandUnits = EntityCategoryFilterDown(categories.COMMAND, units) if table.getn(commandUnits) > 0 then import("/lua/selfdestruct.lua").ToggleSelfDestruct(data, commandUnits) return end end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lua/SimCallbacks.lua` around lines 185 - 207, Normalize the incoming selection by calling SecureUnits(units) once at the top (e.g., replace raw uses of units with a local sanitized = SecureUnits(units)), then use that sanitized table for the table.getn check and for all calls to import("/lua/selfdestruct.lua").ToggleSelfDestruct; ensure you perform the early return when the sanitized table is empty and still validate data.owner/OkayToMessWithArmy(data.owner) before invoking ToggleSelfDestruct, and use the same sanitized variable in the ScenarioInfo.Options.Share branch (commandUnits = EntityCategoryFilterDown(categories.COMMAND, sanitized)).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@lua/SimCallbacks.lua`:
- Around line 185-207: Normalize the incoming selection by calling
SecureUnits(units) once at the top (e.g., replace raw uses of units with a local
sanitized = SecureUnits(units)), then use that sanitized table for the
table.getn check and for all calls to
import("/lua/selfdestruct.lua").ToggleSelfDestruct; ensure you perform the early
return when the sanitized table is empty and still validate
data.owner/OkayToMessWithArmy(data.owner) before invoking ToggleSelfDestruct,
and use the same sanitized variable in the ScenarioInfo.Options.Share branch
(commandUnits = EntityCategoryFilterDown(categories.COMMAND, sanitized)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6657cad3-a08f-4d2d-a15e-429c81770c8d
📒 Files selected for processing (3)
changelog/snippets/features.7132.mdchangelog/snippets/graphics.7095.mdlua/SimCallbacks.lua
✅ Files skipped from review due to trivial changes (1)
- changelog/snippets/features.7132.md
| end | ||
|
|
||
| -- prevent abuse | ||
| if (not data) or (not data.owner) or (not OkayToMessWithArmy(data.owner)) then |
There was a problem hiding this comment.
Using user-defined Args in the data table cannot be relied upon for security.
When I look into it further, the verification for this function is ending up a bit all over the place:
data.owneris verified withOkayToMessWithArmyin this callback and the import function.data.owneris confirmed to not be -1 (observer) in the import function. Which I guess prevents observers from self-destructing units in cheat games? Butdata.ownerisGetFocusArmyfrom UI side, and observers can't select units so they can't trigger the callback with any selection to act upon while also sending an owner of -1.unitsis checked withSecureUnits, which loops over units and checksOkayToMessWithArmy(unit.Army)(it also has a part converting entity ids into entities but that isn't relevant here). This is all sim-side info and the units list is from the engine UI code, so it is secure. The issue is the duplication where thisOkayToMessWithArmy(unit.Army)is used in your category filter, and the imported function (twice at that).
Overall, the import function has the final results and always checks OkayToMessWithArmy(unit.Army), so it works, but it seems like the rest of the checks can be replaced. data.owner can instead be an early exit using GetCommandSourceArmy or GetFocusArmy sim-side. Checks for OkayToMessWithArmy(unit.Army) can be replaced with SecureUnits in the main callback function replacing the units variable, and then sending it to the imported function.
There was a problem hiding this comment.
This is just me pulling the logic from the SelfDestruct function to the boundary, see also lines 35: if data.owner ~= -1 and OkayToMessWithArmy(data.owner) then.
I personally do not think that the units list needs to be checked, since it's the engine that appends it. Unless we also want to guard against hacked clients, where you can append arbitrary units.
Note that I intentionally did not mess with the existing functionality, since that's not what was requested.
| -- moderation rule: if you self destruct with one or more ACUs in the selection when playing full | ||
| -- share, then you only self destruct the ACUs. This does not make it impossible to abuse, but it | ||
| -- does introduce a simple guardrail. | ||
| if ScenarioInfo.Options.Share == "FullShare" then | ||
| local commandUnits = EntityCategoryFilterDown(categories.COMMAND, SecureUnits(units)) | ||
| if table.getn(commandUnits) > 0 then | ||
| import("/lua/selfdestruct.lua").ToggleSelfDestruct(data, commandUnits) | ||
| return | ||
| end | ||
| end |
There was a problem hiding this comment.
This is trivial to bypass with UI mods splitting the user's order into two callback calls, so why not implement it in the UI layer? Then it's easy to have a handler with proper user feedback.
There was a problem hiding this comment.
It's also trivial to identify that this happened when parsing the replay. Malicious intent is then confirmed, easier to ban in my point of view.
You're free to also implement something in the UI layer, by all means.
2 participants
Description of the proposed changes
Adds the exception that when you self destruct a selection with one or more ACUs it will only self destruct the ACUs.
Testing done on the proposed changes
Launched the game and verified that the ACU is isolated when I try to self destruct all my units.
Additional context
For this fortunate enough to view it:
Checklist
Summary by CodeRabbit