Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove FCVAR_SERVER_CAN_EXECUTE from disconnect #663

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Remove FCVAR_SERVER_CAN_EXECUTE from disconnect #663

wants to merge 4 commits into from

Conversation

ASpoonPlaysGames
Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames commented Feb 11, 2024 •
edited
Loading

Servers should use the NSDisconnectPlayer script function instead. (clients can just ignore the disconnect command, it's more a suggestion)

Closes #553

@GeckoEidechse
Copy link
Member

To be clear, do clients already ignore disconnect?

How is it handled in regards to vanilla? Like what happens if you run this PR in vanilla?

@ASpoonPlaysGames
Copy link
Contributor Author

ASpoonPlaysGames commented Feb 11, 2024 •
edited by GeckoEidechse
Loading

To be clear, do clients already ignore disconnect?

Normal clients do not ignore disconnect, but a malicious one very much could. Therefore using disconnect to disconnect a client is a bad idea and we shouldn't allow for it. The purpose of disconnect is for a client to disconnect themselves from a server.

How is it handled in regards to vanilla? Like what happens if you run this PR in vanilla?

Nothing, this is reverting disconnect back to vanilla behaviour. Vanilla doesn't expect disconnect to be called by servers because they could just choose to ignore it, vanilla uses other native functions to properly disconnect a client.

@GeckoEidechse
Copy link
Member

To be clear, do clients already ignore disconnect?

Normal clients do not ignore disconnect, but a malicious one very much could. Therefore using disconnect to disconnect a client is a bad idea and we shouldn't allow for it. The purpose of disconnect is for a client to disconnect themselves from a server.

Aight, thanks for the clarification <3

The only thing I need to keep in mind then in that regard is to give modders a heads-up in case they somehow made a server-mod that sends disconnect to client.

Man I wish I already had a system in place to crawl Thunderstore mods for this sorta stuff ^^"

@ASpoonPlaysGames
Copy link
Contributor Author

The only thing I need to keep in mind then in that regard is to give modders a heads-up in case they somehow made a server-mod that sends disconnect to client.

I think when we added NSDisconnectPlayer we did a similar notification already tbh

@GeckoEidechse
Copy link
Member

GeckoEidechse commented Feb 15, 2024 •
edited
Loading

So I got around writing a quick Thunderstore scraper and based on my local dataset, the following mods at their latest version will be affected

  • Zanieon-Matchmaking_Mixtape_Menu-1.2.0 doesn't matter cause it's local client
  • SoftSleeper-Attrition_Extended_Overhaul-1.2.11
  • VoyageDB_Modding_Home-Attrition_Extended-1.9.3
  • NanohmProtogen-VanillaPlus-2.4.1
  • VoyageDB_Modding_Home-Grunt_Mode-3.3.2
  • VoyageDB_Modding_Home-Nessie_Temp_Fix-1.3.16
  • The_Peepeepoopoo_man-Titanframework-2.4.2
  • Okudai-loeb-1.0.1
  • Capt_Diqhedd-GoMortyYourself-1.0.1
  • VoyageDB_Modding_Home-Modded_Bleedout_Game-1.0.3
  • VoyageDB_Modding_Home-The_Spawner_English-1.2.0
  • VoyageDB_Modding_Home-The_Spawner_English-1.2.0
  • taskinoz-EnhancedMenuMod-1.14.0
  • zxcPandora-DeveloperMode-1.0.1
  • EladNLG-RoguelikeBETA-0.3.102
  • EladNLG-Roguelike-0.3.102

checked by running:

$ find thunderstore -type f -exec grep -l '"disconnect[ "]' {} +
thunderstore/Zanieon-Matchmaking_Mixtape_Menu-1.2.0/mods/MixtapeMenu/mod/scripts/vscripts/ui/menu_lobby.nut
thunderstore/SoftSleeper-Attrition_Extended_Overhaul-1.2.11/mods/ATTExtendOverhaul/mod/scripts/vscripts/burnmeter/sh_burnmeter.gnut
thunderstore/VoyageDB_Modding_Home-Attrition_Extended-1.9.3/mods/AITdmExtended/mod/scripts/vscripts/burnmeter/sh_burnmeter.gnut
thunderstore/NanohmProtogen-VanillaPlus-2.4.1/mods/NP.VanillaPlus/mod/scripts/vscripts/ui/menu_lobby.nut
thunderstore/VoyageDB_Modding_Home-Grunt_Mode-3.3.2/mods/GruntModeNS/mod/scripts/vscripts/burnmeter/sh_burnmeter.gnut
thunderstore/VoyageDB_Modding_Home-Nessie_Temp_Fix-1.3.16/mods/NessieTempFix/mod/scripts/vscripts/burnmeter/sh_burnmeter.gnut
thunderstore/The_Peepeepoopoo_man-Titanframework-2.4.2/mods/peepee.Titanframework/mod/scripts/vscripts/sh_loadouts.nut
thunderstore/Okudai-loeb-1.0.1/mods/okudai.loeb/mod/scripts/vscripts/ui/menu_lobby.nut
thunderstore/Capt_Diqhedd-GoMortyYourself-1.0.1/mods/Diq.GoMortyYourself/mod/scripts/vscripts/cl_mortyyourself.gnut
thunderstore/VoyageDB_Modding_Home-Modded_Bleedout_Game-1.0.3/mods/Modded.Bleedout/mod/scripts/vscripts/burnmeter/sh_burnmeter.gnut
thunderstore/VoyageDB_Modding_Home-The_Spawner_English-1.2.0/mods/Super.Mixed.Game.English/mod/scripts/vscripts/burnmeter/sh_burnmeter.gnut
thunderstore/VoyageDB_Modding_Home-The_Spawner_English-1.2.0/mods/Super.Mixed.Game.English/mod/scripts/vscripts/sh_loadouts.nut
thunderstore/taskinoz-EnhancedMenuMod-1.14.0/mods/Enhanced.Menu.Mod.Northstar/mod/scripts/vscripts/sp/sp_training.nut
thunderstore/zxcPandora-DeveloperMode-1.0.1/mods/DeveloperMode/mod/scripts/vscripts/sh_loadouts.nut
thunderstore/EladNLG-RoguelikeBETA-0.3.102/mods/EladNLG.Roguelike/mod/scripts/vscripts/sp/_savegame.gnut
thunderstore/EladNLG-Roguelike-0.3.102/mods/EladNLG.Roguelike/mod/scripts/vscripts/sp/_savegame.gnut

@GeckoEidechse
Copy link
Member

What would you say is the best way to proceed? Ping mod authors and release PR as part of minor release?

@ASpoonPlaysGames
Copy link
Contributor Author

So I got around writing a quick Thunderstore scraper and based on my local dataset, the following mods at their latest version will be affected

  • Zanieon-Matchmaking_Mixtape_Menu-1.2.0
  • SoftSleeper-Attrition_Extended_Overhaul-1.2.11
  • VoyageDB_Modding_Home-Attrition_Extended-1.9.3
  • NanohmProtogen-VanillaPlus-2.4.1
  • VoyageDB_Modding_Home-Grunt_Mode-3.3.2
  • VoyageDB_Modding_Home-Nessie_Temp_Fix-1.3.16
  • The_Peepeepoopoo_man-Titanframework-2.4.2
  • Okudai-loeb-1.0.1
  • Capt_Diqhedd-GoMortyYourself-1.0.1
  • VoyageDB_Modding_Home-Modded_Bleedout_Game-1.0.3
  • VoyageDB_Modding_Home-The_Spawner_English-1.2.0
  • VoyageDB_Modding_Home-The_Spawner_English-1.2.0
  • taskinoz-EnhancedMenuMod-1.14.0
  • zxcPandora-DeveloperMode-1.0.1
  • EladNLG-RoguelikeBETA-0.3.102
  • EladNLG-Roguelike-0.3.102

Note that only mods that do this on server are affected by this, which reduces this count quite a lot.

@GeckoEidechse GeckoEidechse self-assigned this Feb 18, 2024
@GeckoEidechse
Copy link
Member

Note that only mods that do this on server are affected by this, which reduces this count quite a lot.

Good point. I manually looked through the mods a bit and this is what I ended up with:

Client
thunderstore/Zanieon-Matchmaking_Mixtape_Menu-1.2.0/mods/MixtapeMenu/mod/scripts/vscripts/ui/menu_lobby.nut
thunderstore/NanohmProtogen-VanillaPlus-2.4.1/mods/NP.VanillaPlus/mod/scripts/vscripts/ui/menu_lobby.nut
thunderstore/Okudai-loeb-1.0.1/mods/okudai.loeb/mod/scripts/vscripts/ui/menu_lobby.nut
thunderstore/Capt_Diqhedd-GoMortyYourself-1.0.1/mods/Diq.GoMortyYourself/mod/scripts/vscripts/cl_mortyyourself.gnut


Server
thunderstore/SoftSleeper-Attrition_Extended_Overhaul-1.2.11/mods/ATTExtendOverhaul/mod/scripts/vscripts/burnmeter/sh_burnmeter.gnut
thunderstore/VoyageDB_Modding_Home-Attrition_Extended-1.9.3/mods/AITdmExtended/mod/scripts/vscripts/burnmeter/sh_burnmeter.gnut
thunderstore/VoyageDB_Modding_Home-Grunt_Mode-3.3.2/mods/GruntModeNS/mod/scripts/vscripts/burnmeter/sh_burnmeter.gnut
thunderstore/VoyageDB_Modding_Home-Nessie_Temp_Fix-1.3.16/mods/NessieTempFix/mod/scripts/vscripts/burnmeter/sh_burnmeter.gnut
thunderstore/The_Peepeepoopoo_man-Titanframework-2.4.2/mods/peepee.Titanframework/mod/scripts/vscripts/sh_loadouts.nut	
thunderstore/VoyageDB_Modding_Home-Modded_Bleedout_Game-1.0.3/mods/Modded.Bleedout/mod/scripts/vscripts/burnmeter/sh_burnmeter.gnut
thunderstore/VoyageDB_Modding_Home-The_Spawner_English-1.2.0/mods/Super.Mixed.Game.English/mod/scripts/vscripts/burnmeter/sh_burnmeter.gnut
thunderstore/VoyageDB_Modding_Home-The_Spawner_English-1.2.0/mods/Super.Mixed.Game.English/mod/scripts/vscripts/sh_loadouts.nut
thunderstore/zxcPandora-DeveloperMode-1.0.1/mods/DeveloperMode/mod/scripts/vscripts/sh_loadouts.nut


No clue
thunderstore/taskinoz-EnhancedMenuMod-1.14.0/mods/Enhanced.Menu.Mod.Northstar/mod/scripts/vscripts/sp/sp_training.nut
thunderstore/EladNLG-RoguelikeBETA-0.3.102/mods/EladNLG.Roguelike/mod/scripts/vscripts/sp/_savegame.gnut
thunderstore/EladNLG-Roguelike-0.3.102/mods/EladNLG.Roguelike/mod/scripts/vscripts/sp/_savegame.gnut

Looking at it further the majority of the server sided mods that call disconnect do so cause they modify existing Northstar scripts

From the look of it, they are called on server but I might be mistaken.

If they are, I guess we'd have to update the corresponding mod code as well ^^"

@GeckoEidechse GeckoEidechse removed their assignment Feb 23, 2024
@ASpoonPlaysGames ASpoonPlaysGames mentioned this pull request Aug 22, 2024
Merged
@ASpoonPlaysGames
Copy link
Contributor Author

Not a lot that we can do about the mods that override these files. Lets be honest, they aren't going to get updated. But yeah R2Northstar/NorthstarMods#841 fixes the cases in Northstar.

@ASpoonPlaysGames ASpoonPlaysGames added needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code labels Aug 30, 2024
RoyalBlue1
RoyalBlue1 approved these changes Sep 7, 2024
View reviewed changes
Copy link
Contributor

@RoyalBlue1 RoyalBlue1 left a comment

Choose a reason for hiding this comment

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

in code it only removes the adding of the flag but as in the discussion above it will break some mods

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

@RoyalBlue1 RoyalBlue1 RoyalBlue1 approved these changes

Assignees
No one assigned
Labels
needs code review Changes from PR still need to be reviewed in code needs testing Changes from the PR still need to be tested
Projects
Northstar Roadmap
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

disconnect command shouldn't be FCVAR_SERVER_CAN_EXECUTE
4 participants
@ASpoonPlaysGames @GeckoEidechse @RoyalBlue1 @F1F7Y