Skip to content

fix(core): validate fxserver command input and simplify dispatch - #1144

Closed
mattibat wants to merge 2 commits into
citizenfx:developfrom
mattibat:fix/fxserver-commands-validation
Closed

mattibat wants to merge 2 commits into
citizenfx:developfrom
mattibat:fix/fxserver-commands-validation

Conversation

@mattibat

Copy link
Copy Markdown

Problem

  • action/parameter were only checked for "defined", not type
  • Command dispatch was a long if/else chain with permission check duplicated per branch, easy to forget on a new action

Fix

  • action/parameter now require typeof === 'string'
  • Dispatch replaced with a lookup table: action -> { permission, handler }
  • Permission check centralized in one place
  • No behavior change beyond the stricter input validation

@mattibat
mattibat requested a review from tabarra as a code owner September 17, 2026 18:43
…route

Signed-off-by: matti.bat <138713607+mattibat@users.noreply.github.com>
…able

Signed-off-by: matti.bat <138713607+mattibat@users.noreply.github.com>
@mattibat
mattibat force-pushed the fix/fxserver-commands-validation branch from dc00822 to e729e3e Compare September 17, 2026 18:53
@tabarra

tabarra commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

Please don't spam AI-generated PRs, this one is also just noise that shuffles code around for little to no gain.
This is one of the oldest files in txAdmin, sure it can be better, but:

  1. Any change brings new risks
  2. It makes harder to do a git blame drill down
  3. Makes this file different than its sibling controls.ts

The "problem" you described has literally zero impact, an intentionally crafted invalid authenticated request would cause a zero-impact reflected 500 error if the params exist but not string, and your code solves this bug, but it also introduces the exact same bug again for strings like toString, and constructor... so what was gained here?
What could be a two-lined patch became a full file rewrite.

And again, I do recognize that this file could be done better, but so can basically every file in this codebase.
I could also ask Codex or Claude to go around the code doing drive-by refactors... but I just don't want to.

@tabarra tabarra closed this Sep 17, 2026
@mattibat

Copy link
Copy Markdown
Author

fair point. agreed the refactor wasn't worth the diff for what it fixed. Happy to send just the 2 line validation fix on its own if thats useful, otherwise no worries either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants