Fix SSC login deadlock when players are crowd controlled before auth#3218
Fix SSC login deadlock when players are crowd controlled before auth#3218Xekep wants to merge 2 commits intoPryaxis:general-develfrom
Conversation
|
@greptile review? |
Greptile SummaryThis PR fixes a deadlock where players cannot authenticate when server-applied crowd control (Webbed buff) from SSC/RequireLogin auth gates blocks the Key Changes:
How It Works: Confidence Score: 5/5
Important Files Changed
Last reviewed commit: caea4e7 |
|
@greptile i have my doubts as to whether or not this is reachable in practice? |
There was a problem hiding this comment.
Pull request overview
This PR addresses an SSC authentication deadlock where the server’s pre-login “disabled” state can apply crowd control (e.g., Webbed), preventing /login and leaving players unable to authenticate.
Changes:
- Adds a pre-login “auth gate” condition to allow
/loginwhile CCed in certain disabled states. - Updates the CCed login guard to exempt players disabled by SSC (and currently also
RequireLogin).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var disabledForAuthGate = args.Player.IsDisabledForSSC | ||
| || (!args.Player.IsLoggedIn && TShock.Config.Settings.RequireLogin); | ||
| if (args.TPlayer.CCed && Main.ServerSideCharacter && !disabledForAuthGate) |
There was a problem hiding this comment.
PR description says /login should be allowed while CCed only for the SSC pre-login disabled state (IsDisabledForSSC), but disabledForAuthGate also treats RequireLogin as an auth-gate condition. If the intent is strictly SSC-only, drop the RequireLogin clause; if RequireLogin is also intended here, please update the PR description to match (and clarify why this remains gated behind Main.ServerSideCharacter).
| // Pre-login auth gates can disable movement (SSC/RequireLogin), which may mark the player as CCed. | ||
| // Allow login in that temporary state to avoid an auth deadlock. | ||
| var disabledForAuthGate = args.Player.IsDisabledForSSC | ||
| || (!args.Player.IsLoggedIn && TShock.Config.Settings.RequireLogin); |
There was a problem hiding this comment.
disabledForAuthGate includes !args.Player.IsLoggedIn, but this method already returns early when IsLoggedIn is true. Removing the redundant check would make the intent clearer and avoid implying that disabledForAuthGate could ever be evaluated in a logged-in state.
| || (!args.Player.IsLoggedIn && TShock.Config.Settings.RequireLogin); | |
| || TShock.Config.Settings.RequireLogin; |
| // Pre-login auth gates can disable movement (SSC/RequireLogin), which may mark the player as CCed. | ||
| // Allow login in that temporary state to avoid an auth deadlock. | ||
| var disabledForAuthGate = args.Player.IsDisabledForSSC | ||
| || (!args.Player.IsLoggedIn && TShock.Config.Settings.RequireLogin); |
There was a problem hiding this comment.
This change allows /login to proceed while args.TPlayer.CCed when disabledForAuthGate is true, but the login flow still calls PlayerData.RestoreCharacter(...) (when SSC is enabled) which sends a SyncLoadout packet. Earlier in this method you note CC prevents loadout switching and can cause catastrophic desync; if the player is CCed due to the server-applied Webbed disable buff, the client may still reject the loadout sync even though login is now permitted. Consider explicitly clearing the server-applied disable CC (e.g., Webbed) before restoring/syncing character data, or otherwise ensuring the client is not CCed at the point the loadout index is synced.
| || (!args.Player.IsLoggedIn && TShock.Config.Settings.RequireLogin); | |
| || (!args.Player.IsLoggedIn && TShock.Config.Settings.RequireLogin); | |
| // If the player is only CCed because of a server-side auth gate (e.g., Webbed to disable movement | |
| // before login), clear that CC before we proceed into SSC restore/loadout sync. Otherwise the client | |
| // may reject the SyncLoadout packet while still CCed, causing desync. | |
| if (disabledForAuthGate && args.TPlayer.CCed) | |
| { | |
| for (int i = 0; i < args.TPlayer.buffType.Length; i++) | |
| { | |
| if (args.TPlayer.buffType[i] == Terraria.ID.BuffID.Webbed) | |
| { | |
| args.TPlayer.DelBuff(i); | |
| i--; | |
| } | |
| } | |
| // Ensure the auth-gate CC flag is cleared on the server-side Player instance. | |
| args.TPlayer.webbed = false; | |
| } |
This is reachable in practice; I reproduced it on a survival server with SSC enabled. Steps:
Before this fix, login is rejected with “You cannot login whilst crowd controlled.”, causing an auth lock loop. |
|
Thanks for the clear reproduction steps. The scenario makes sense - SSC disable applies Webbed status, which then blocks The fix is appropriately scoped since it only bypasses the CC check when
Good catch and clean solution. |
|
I think we've effectively fixed the hole causing this, can you check if this patch is still necessary? |
Summary
Scope