SkriptLang
Possible crash exploit
Skript/Server Version
[18:45:22 INFO]: [Skript] Skript's aliases can be found here: https://github.com/SkriptLang/skript-aliases
[18:45:22 INFO]: [Skript] Skript's documentation can be found here: https://docs.skriptlang.org/
[18:45:22 INFO]: [Skript] Skript's tutorials can be found here: https://docs.skriptlang.org/tutorials
[18:45:22 INFO]: [Skript] Server Version: 1.21.1-2286-366af80 (MC: 1.21.1)
[18:45:22 INFO]: [Skript] Skript Version: 2.9.1 (skriptlang-github)
[18:45:22 INFO]: [Skript] Installed Skript Addons:
[18:45:22 INFO]: [Skript] - skript-reflect v2.5.1 (https://github.com/SkriptLang/skript-reflect)
[18:45:22 INFO]: [Skript] - Hippo v1.2
[18:45:22 INFO]: [Skript] - SkBee v3.6.0 (https://github.com/ShaneBeee/SkBee)
[18:45:22 INFO]: [Skript] Installed dependencies:
[18:45:22 INFO]: [Skript] - Vault v1.7.3-b131
[18:45:22 INFO]: [Skript] - WorldGuard v7.0.11-beta1+a801a9d
Bug Description
Basically, the offlineplayer function (which internally runs CraftServer#getOfflinePlayer(String)) has a crash possibility.
I have opened an issue on Paper about this, though they told me it's the plugin's responsibility to properly combat the issue. https://github.com/PaperMC/Paper/issues/11277
Basically, the getOfflinePlayer(String) method does a GET request to Mojang servers if a player isn't in the server's cache manager.
Expected Behavior
Obviously, the offlineplayer function shouldn't make the server lag. A solution to this problem is using Server#getOfflinePlayerIfCached, which only gets the player from the local cache.
Steps to Reproduce
- Spam offlineplayer("sdffsdfsd"). For production servers, the function isn't openly available, but any command which accepts an offlineplayer also works for this purpose. All you have to do is literally spam your keyboard (and make sure it is alphanumeric and max 16 chars), and spam a command with an offlineplayer arg.
Errors or Screenshots
https://paste.gg/p/anonymous/e562b603a8dd47b0874e6aa6beb8e61a I have tested this behavior, and in this paste, you can see that inside of this stacktrace, you can see 2 very interesting lines:
[18:25:46] [Watchdog Thread/ERROR]: com.mojang.authlib.yggdrasil.YggdrasilGameProfileRepository.findProfilesByNames(YggdrasilGameProfileRepository.java:100)
[18:25:46] [Watchdog Thread/ERROR]: com.destroystokyo.paper.profile.PaperGameProfileRepository.findProfilesByNames(PaperGameProfileRepository.java:43)
Other
No response
Agreement
- [X] I have read the guidelines above and affirm I am following them with this report.
Just checked https://github.com/SkriptLang/Skript/commit/1ac768153649956036914bdb4847640a53b8547d, apparently there is a parameter which can be provided to use the other variant but there is still the issue of command arguments. I am patching Skript myself to use getOfflinePlayerIfCached, but hope it will be an official fix.
We've discussed this and we'll probably move to making the command parser only do http lookups if a config option is enabled. That said, your error is not due to repeated http lookups. The first lookup (when it says there was a 3ish second lag spike) was an http request. All subsequent ones just request from the cache, which is what you're seeing in the stacktrace. This seems to be a server performance issue, not a skript issue.
Coming back to this, is the issue going to be patched in the near Skript release? Sure I can use the solution provided in the mentioned issue (making the parameter a text parameter and use offlineplayer function to prevent lookups), but it is too tedious for me and other people developing my server.
No one has made a PR for it yet, but it'd be a pretty simple change to the offline player parser. It'd have to wait for 2.10 since it's a breaking change, though.
Could we not just add a config option to prevent the breaking change? I don't see why we want to fully remove the functionality
We want to change the default, not remove it. That said, we could implement the config option in 2.9.x and change the default in 2.10