Skip to content

Conversation

@miwarnec
Copy link
Collaborator

@miwarnec miwarnec commented Nov 25, 2025

preview PR open for discussion.
adds life safer in case of exploits in production games.

basically:

  • studio forgot validating sender in a [Command(requiresAuthority=false)]
  • adding NetworkConnectionToClient parameter is the solution, however:
  • this would require a full server + client release, which is very costly and takes validation time
  • adding this flag in some form can be a life safer: server sided change without needing a client rebuild

@miwarnec miwarnec changed the title cmd thing NetworkBehaviour.lastCommandSender Nov 25, 2025
Copy link
Contributor

@tigran-sargsyan-w tigran-sargsyan-w left a comment

Choose a reason for hiding this comment

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

Hey, thanks for putting this together — the idea of exposing lastCommandSender on NetworkBehaviour makes a lot of sense for handling urgent exploit scenarios without forcing a client rollout.

A few small thoughts while reading through the diff:

  • Right now lastCommandSender is only set, never cleared. Do you see any risk of consumers accidentally using a stale value if they read it outside the current RPC handler? Might be worth either resetting it after Invoke or documenting that it's only valid during the command call.
  • Since this becomes part of the surface area of NetworkBehaviour, a short XML doc comment on the field / setter explaining the security use-case and lifetime would be super helpful for folks who just discover it via IDE tooltips.
  • Not sure if this is in scope for this PR, but having a tiny example in the docs that shows how to use lastCommandSender to validate a [Command(requiresAuthority = false)] would make the intent a lot clearer.

Overall the change itself is small and easy to reason about, and the trade‑off vs. forcing a client update is clear. Feels like it just needs a bit of extra guidance in docs/comments so people don’t accidentally misuse it.

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.

4 participants