Fix double-eval in EffBroadcast by adding an unformatted hook to VString.getMessageComponents

Author: sovdeeth

src/main/java/ch/njol/skript/effects/EffBroadcast.java

@@ -1,12 +1,5 @@
 package ch.njol.skript.effects;
 
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Set;
-import java.util.regex.Pattern;
-
 import ch.njol.skript.Skript;
 import ch.njol.skript.doc.Description;
 import ch.njol.skript.doc.Examples;
@@ -24,6 +17,7 @@
 import ch.njol.skript.util.Utils;
 import ch.njol.skript.util.chat.BungeeConverter;
 import ch.njol.skript.util.chat.ChatMessages;
+import ch.njol.skript.util.chat.MessageComponent;
 import ch.njol.util.Kleenean;
 import ch.njol.util.StringUtils;
 import ch.njol.util.coll.CollectionUtils;
@@ -35,6 +29,9 @@
 import org.bukkit.event.server.BroadcastMessageEvent;
 import org.jetbrains.annotations.Nullable;
 
+import java.util.*;
+import java.util.regex.Pattern;
+
 @Name("Broadcast")
 @Description("Broadcasts a message to the server.")
 @Examples({
@@ -83,12 +80,15 @@ public void execute(Event event) {
 		}
 
 		for (Expression<?> message : getMessages()) {
-			if (message instanceof VariableString) {
-				if (!dispatchEvent(getRawString(event, (VariableString) message), receivers))
+			if (message instanceof VariableString variableString) {
+				// get both unformatted and components with single evaluation: https://github.com/SkriptLang/Skript/issues/7718
+				StringBuilder unformattedString = new StringBuilder();
+				List<MessageComponent> messageComponents = variableString.getMessageComponents(event, unformattedString);
+				if (!dispatchEvent(unformattedString.toString(), receivers))
 					continue;
-				BaseComponent[] components = BungeeConverter.convert(((VariableString) message).getMessageComponents(event));
+				BaseComponent[] components = BungeeConverter.convert(messageComponents);
 				receivers.forEach(receiver -> receiver.spigot().sendMessage(components));
-			} else if (message instanceof ExprColoured && ((ExprColoured) message).isUnsafeFormat()) { // Manually marked as trusted
+			} else if (message instanceof ExprColoured coloured && coloured.isUnsafeFormat()) { // Manually marked as trusted
 				for (Object realMessage : message.getArray(event)) {
 					if (!dispatchEvent(Utils.replaceChatStyles((String) realMessage), receivers))
 						continue;
@@ -97,7 +97,7 @@ public void execute(Event event) {
 				}
 			} else {
 				for (Object messageObject : message.getArray(event)) {
-					String realMessage = messageObject instanceof String ? (String) messageObject : Classes.toString(messageObject);
+					String realMessage = messageObject instanceof String string ? string : Classes.toString(messageObject);
 					if (!dispatchEvent(Utils.replaceChatStyles(realMessage), receivers))
 						continue;
 					receivers.forEach(receiver -> receiver.sendMessage(realMessage));
@@ -123,9 +123,9 @@ private Expression<?>[] getMessages() {
 	 * @param message the message
 	 * @return true if the dispatched event does not get cancelled
 	 */
-	@SuppressWarnings("deprecation")
+	@SuppressWarnings("removal")
 	private static boolean dispatchEvent(String message, List<CommandSender> receivers) {
-		Set<CommandSender> recipients = Collections.unmodifiableSet(new HashSet<>(receivers));
+		Set<CommandSender> recipients = Set.copyOf(receivers);
 		BroadcastMessageEvent broadcastEvent;
 		if (Skript.isRunningMinecraft(1, 14)) {
 			broadcastEvent = new BroadcastMessageEvent(!Bukkit.isPrimaryThread(), message, recipients);
@@ -136,11 +136,18 @@ private static boolean dispatchEvent(String message, List<CommandSender> receive
 		return !broadcastEvent.isCancelled();
 	}
 
-	@Nullable
-	private static String getRawString(Event event, Expression<? extends String> string) {
-		if (string instanceof VariableString)
-			return ((VariableString) string).toUnformattedString(event);
+	/**
+	 * Gets the raw string from the expression, replacing colour codes.
+	 * @param event the event
+	 * @param string the expression
+	 * @return the raw string
+	 */
+	private static @Nullable String getRawString(Event event, Expression<? extends String> string) {
+		if (string instanceof VariableString variableString)
+			return variableString.toUnformattedString(event);
 		String rawString = string.getSingle(event);
+		if (rawString == null)
+			return null;
 		rawString = SkriptColor.replaceColorChar(rawString);
 		if (rawString.toLowerCase().contains("&x")) {
 			rawString = StringUtils.replaceAll(rawString, HEX_PATTERN, matchResult ->

src/main/java/ch/njol/skript/lang/VariableString.java

@@ -24,8 +24,8 @@
 import com.google.common.collect.Lists;
 import org.bukkit.ChatColor;
 import org.bukkit.event.Event;
-import org.jetbrains.annotations.Nullable;
 import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
 import org.skriptlang.skript.lang.script.Script;
 
 import java.util.ArrayList;
@@ -385,6 +385,19 @@ public String toUnformattedString(Event event) {
 	 * @return Message components.
 	 */
 	public List<MessageComponent> getMessageComponents(Event event) {
+		return getMessageComponents(event, null);
+	}
+
+	/**
+	 * Gets message components from this string. Formatting is parsed only
+	 * in simple parts for security reasons. Providing a StringBuilder allows an unformatted output
+	 * identical to {@link #toUnformattedString(Event)} while only evaluating any expressions once.
+	 *
+	 * @param event Currently running event.
+	 * @param unformattedBuilder Unformatted string to append to.
+	 * @return Message components.
+	 */
+	public List<MessageComponent> getMessageComponents(Event event, @Nullable StringBuilder unformattedBuilder) {
 		if (isSimple) { // Trusted, constant string in a script
 			assert simpleUnformatted != null;
 			return ChatMessages.parse(simpleUnformatted);
@@ -408,14 +421,15 @@ public List<MessageComponent> getMessageComponents(Event event) {
 
 				// Convert it to plain text
 				String text = null;
-				if (string instanceof ExprColoured && ((ExprColoured) string).isUnsafeFormat()) { // Special case: user wants to process formatting
-					String unformatted = Classes.toString(((ExprColoured) string).getArray(event), true, mode);
-					if (unformatted != null) {
-						message.addAll(ChatMessages.parse(unformatted));
+				if (string instanceof Expression<?> expression) {
+					text = Classes.toString(expression.getArray(event), true, mode);
+					if (unformattedBuilder != null)
+						unformattedBuilder.append(text);
+					// Special case: user wants to process formatting
+					if (string instanceof ExprColoured exprColoured && exprColoured.isUnsafeFormat()) {
+						message.addAll(ChatMessages.parse(text));
+						continue;
 					}
-					continue;
-				} else if (string instanceof Expression<?>) {
-					text = Classes.toString(((Expression<?>) string).getArray(event), true, mode);
 				}
 
 				assert text != null;

src/test/skript/tests/regressions/7718-broadcast evaluates vstrings twice.sk

@@ -0,0 +1,8 @@
+test "broadcast evaluates vstrings twice":
+	broadcast "%func()%"
+	assert {7718::calls} is 1 with "Called func() more than once"
+	delete {7718::calls}
+
+function func() returns string:
+	add 1 to {7718::calls}
+	return "7718 broadcast regression test"