<cmd:> with $ symbol in it doesn't work

[ghost]

Description

cmd: with $ in it, does not work.

Steps to Reproduce

set {_test} to "/hello$"
send formatted "<cmd:/%{_test}%>click here"

Expected Behavior

to just work, now it sends absolutely nothing on that, I have a command with $ symbol in it.
(It used to work right before, I updated from 2.2 dev36 to 2.4)

Errors / Screenshots

None

Server Information

• Server version/platform: 1.12.2 Paper
• Skript version: 2.4

Additional Context

[Whimsyturtle]

For reference, this doesn't work because it throws an exception:

  [Server thread/ERROR]: #!#! Stack trace:
  [Server thread/ERROR]: #!#! java.lang.IllegalArgumentException: Illegal group reference
  [Server thread/ERROR]: #!#!     at java.util.regex.Matcher.appendReplacement(Unknown Source)
  [Server thread/ERROR]: #!#!     at ch.njol.util.StringUtils.replaceAll(StringUtils.java:90)
  [Server thread/ERROR]: #!#!     at ch.njol.skript.util.Utils.replaceChatStyles(Utils.java:536)
  [Server thread/ERROR]: #!#!     at ch.njol.skript.expressions.ExprColoured$1.convert(ExprColoured.java:86)
  [Server thread/ERROR]: #!#!     at ch.njol.skript.expressions.ExprColoured$1.convert(ExprColoured.java:1)
  [Server thread/ERROR]: #!#!     at ch.njol.skript.registrations.Converters.convert(Converters.java:391)
  [Server thread/ERROR]: #!#!     at ch.njol.skript.registrations.Converters.convertUnsafe(Converters.java:382)
  [Server thread/ERROR]: #!#!     at ch.njol.skript.expressions.base.PropertyExpression.get(PropertyExpression.java:104)
  [Server thread/ERROR]: #!#!     at ch.njol.skript.expressions.ExprColoured.get(ExprColoured.java:83)
  [Server thread/ERROR]: #!#!     at ch.njol.skript.expressions.ExprColoured.get(ExprColoured.java:1)
  [Server thread/ERROR]: #!#!     at ch.njol.skript.expressions.base.PropertyExpression.get(PropertyExpression.java:75)
  [Server thread/ERROR]: #!#!     at ch.njol.skript.lang.util.SimpleExpression.getArray(SimpleExpression.java:102)
  [Server thread/ERROR]: #!#!     at ch.njol.skript.effects.EffMessage.execute(EffMessage.java:91)
  [Server thread/ERROR]: #!#!     at ch.njol.skript.lang.Effect.run(Effect.java:52)
  [Server thread/ERROR]: #!#!     at ch.njol.skript.lang.TriggerItem.walk(TriggerItem.java:61)
  [Server thread/ERROR]: #!#!     at ch.njol.skript.lang.TriggerItem.walk(TriggerItem.java:89)
  [Server thread/ERROR]: #!#!     at ch.njol.skript.lang.Trigger.execute(Trigger.java:57)
  [Server thread/ERROR]: #!#!     at ch.njol.skript.command.ScriptCommand.execute2(ScriptCommand.java:292)
  [Server thread/ERROR]: #!#!     at ch.njol.skript.command.ScriptCommand.execute(ScriptCommand.java:251)
  [Server thread/ERROR]: #!#!     at ch.njol.skript.command.Commands.handleCommand(Commands.java:252)
  [Server thread/ERROR]: #!#!     at ch.njol.skript.command.Commands$1.onPlayerCommand(Commands.java:169)
  [Server thread/ERROR]: #!#!     at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
  [Server thread/ERROR]: #!#!     at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
  [Server thread/ERROR]: #!#!     at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
  [Server thread/ERROR]: #!#!     at java.lang.reflect.Method.invoke(Unknown Source)
  [Server thread/ERROR]: #!#!     at org.bukkit.plugin.java.JavaPluginLoader$1.execute(JavaPluginLoader.java:316)
  [Server thread/ERROR]: #!#!     at org.bukkit.plugin.RegisteredListener.callEvent(RegisteredListener.java:70)
  [Server thread/ERROR]: #!#!     at org.bukkit.plugin.SimplePluginManager.fireEvent(SimplePluginManager.java:529)
  [Server thread/ERROR]: #!#!     at org.bukkit.plugin.SimplePluginManager.callEvent(SimplePluginManager.java:514)
  [Server thread/ERROR]: #!#!     at net.minecraft.server.v1_14_R1.PlayerConnection.handleCommand(PlayerConnection.java:1633)
  [Server thread/ERROR]: #!#!     at net.minecraft.server.v1_14_R1.PlayerConnection.a(PlayerConnection.java:1481)
  [Server thread/ERROR]: #!#!     at net.minecraft.server.v1_14_R1.PacketPlayInChat.a(PacketPlayInChat.java:47)
  [Server thread/ERROR]: #!#!     at net.minecraft.server.v1_14_R1.PacketPlayInChat.a(PacketPlayInChat.java:1)
  [Server thread/ERROR]: #!#!     at net.minecraft.server.v1_14_R1.PlayerConnectionUtils.lambda$0(PlayerConnectionUtils.java:19)
  [Server thread/ERROR]: #!#!     at net.minecraft.server.v1_14_R1.TickTask.run(SourceFile:18)
  [Server thread/ERROR]: #!#!     at net.minecraft.server.v1_14_R1.IAsyncTaskHandler.executeTask(SourceFile:144)
  [Server thread/ERROR]: #!#!     at net.minecraft.server.v1_14_R1.IAsyncTaskHandlerReentrant.executeTask(SourceFile:23)
  [Server thread/ERROR]: #!#!     at net.minecraft.server.v1_14_R1.IAsyncTaskHandler.executeNext(SourceFile:118)
  [Server thread/ERROR]: #!#!     at net.minecraft.server.v1_14_R1.MinecraftServer.aX(MinecraftServer.java:910)
  [Server thread/ERROR]: #!#!     at net.minecraft.server.v1_14_R1.MinecraftServer.executeNext(MinecraftServer.java:903)
  [Server thread/ERROR]: #!#!     at net.minecraft.server.v1_14_R1.IAsyncTaskHandler.awaitTasks(SourceFile:127)
  [Server thread/ERROR]: #!#!     at net.minecraft.server.v1_14_R1.MinecraftServer.sleepForTick(MinecraftServer.java:887)
  [Server thread/ERROR]: #!#!     at net.minecraft.server.v1_14_R1.MinecraftServer.run(MinecraftServer.java:820)
  [Server thread/ERROR]: #!#!     at java.lang.Thread.run(Unknown Source)

Looks like Skript is trying to parse it as a regex, or something... Not sure how this should be handled exactly.

[bensku]

👀

Definitely a bug, probably one that will not be fun to debug.

[bluelhf]

Did someone forget to sanitize user input?

[Whimsyturtle]

The issue seems to be that for some reason it is trying to parse it as a regex in the first place

[bluelhf]

@Wealthyturtle The StringUtils replaceAll method uses regex - I think the colour conversion in Skript is trying to use StringUtils#replaceAll to replace colour codes, but the replaceAll method fails to parse the string as regex (because the string wasn't sanitized before passing it to replaceAll)

If this is the case, a solution would be to make sure the colour conversion sanitizes the string before passing it to replaceAll..

Then again, this suggests that there could be more problems with input sanitization hidden away in Njol's code...

Edit: Was able to trigger this with send formatted "$hello" but not send coloured "$hello"

Maybe Njol did sanitize input, but send formatted doesn't do so?

[Whimsyturtle]

@Wealthyturtle The StringUtils replaceAll method uses regex - I think the colour conversion in Skript is trying to use StringUtils#replaceAll to replace colour codes, but the replaceAll method fails to parse the string as regex (because the string wasn't sanitized before passing it to replaceAll)

If this is the case, a solution would be to make sure the colour conversion sanitizes the string before passing it to replaceAll..

Then again, this suggests that there could be more problems with input sanitization hidden away in Njol's code...

Edit: Was able to trigger this with send formatted "$hello" but not send coloured "$hello"

Maybe Njol did sanitize input, but send formatted doesn't do so?

Good investigation! Maybe we shouldn't be using regex in the formatted/coloured expression in the first place since it adds some overhead. I mean, we could just iterate through the String once and check if the next character is 0-9 or a-f or k-o to determine if they should be deformatted or whatever.

[Whimsyturtle]

From #3002: This issue also seems to occur in <tooltip:>