text-minimessage: Limit characters allowed in tag names

text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/internal/TagInternals.java

@@ -0,0 +1,68 @@
+/*
+ * This file is part of adventure, licensed under the MIT License.
+ *
+ * Copyright (c) 2017-2022 KyoriPowered
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+package net.kyori.adventure.text.minimessage.internal;
+
+import java.util.Locale;
+import java.util.Objects;
+import java.util.regex.Pattern;
+import org.jetbrains.annotations.ApiStatus;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Utility class for tag naming.
+ *
+ * @since 4.10.0
+ */
+@ApiStatus.Internal
+public final class TagInternals {
+  private static final Pattern TAG_NAME_PATTERN = Pattern.compile("[!?#]?[a-z0-9_-]*");
+
+  private TagInternals() {
+  }
+
+  /**
+   * Checks if a tag name matches the pattern for allowed tag names. If it does not, then
+   * this method will throw an {@link IllegalArgumentException}
+   *
+   * @param tagName the name of the tag
+   * @since 4.10.0
+   */
+  public static void checkTagName(final @NotNull String tagName) {
+    if (!TAG_NAME_PATTERN.matcher(Objects.requireNonNull(tagName)).matches()) {
+      throw new IllegalArgumentException("Tag name must match pattern " + TAG_NAME_PATTERN.pattern() + ", was " + tagName);
+    }
+  }
+
+  /**
+   * Checks if a tag name matches the pattern for allowed tag names, first sanitizing it
+   * by converting the tag name to lowercase. If it does not match the pattern, then this
+   * method will throw an {@link IllegalArgumentException}
+   *
+   * @param tagName the name of the tag
+   * @since 4.10.0
+   */
+  public static void sanitizeAndCheckTagName(final @NotNull String tagName) {
+    checkTagName(Objects.requireNonNull(tagName).toLowerCase(Locale.ROOT));
+  }
+}

text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/internal/parser/node/TagNode.java

@@ -26,6 +26,7 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Objects;
+import net.kyori.adventure.text.minimessage.internal.TagInternals;
 import net.kyori.adventure.text.minimessage.internal.parser.ParsingExceptionImpl;
 import net.kyori.adventure.text.minimessage.internal.parser.Token;
 import net.kyori.adventure.text.minimessage.internal.parser.TokenParser;
@@ -59,6 +60,18 @@ public TagNode(
   ) {
     super(parent, token, sourceMessage);
     this.parts = genParts(token, sourceMessage, tagProvider);
+
+    // Assert the tag node has parts.
+    if (this.parts.isEmpty()) {
+      throw new ParsingExceptionImpl("Tag has no parts? " + this, this.sourceMessage(), this.token());
+    }
+
+    // Then assert the tag node has a proper name.
+    try {
+      TagInternals.sanitizeAndCheckTagName(this.name());
+    } catch (final IllegalArgumentException | NullPointerException e) {
+      throw new ParsingExceptionImpl("Invalid tag name " + this.name(), this.sourceMessage(), e, this.token());
+    }
   }
 
   private static @NotNull List<TagPart> genParts(
@@ -94,9 +107,6 @@ public TagNode(
    * @since 4.10.0
    */
   public @NotNull String name() {
-    if (this.parts.isEmpty()) {
-      throw new ParsingExceptionImpl("Tag has no parts? " + this, this.sourceMessage(), this.token());
-    }
     return this.parts.get(0).value();
   }
 

text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/internal/serializer/SerializableResolver.java

@@ -25,14 +25,13 @@
 
 import java.util.Collections;
 import java.util.HashSet;
-import java.util.Locale;
-import java.util.Objects;
 import java.util.Set;
 import java.util.function.BiFunction;
 import java.util.function.Function;
 import net.kyori.adventure.text.Component;
 import net.kyori.adventure.text.minimessage.Context;
 import net.kyori.adventure.text.minimessage.ParsingException;
+import net.kyori.adventure.text.minimessage.internal.TagInternals;
 import net.kyori.adventure.text.minimessage.tag.Tag;
 import net.kyori.adventure.text.minimessage.tag.resolver.ArgumentQueue;
 import net.kyori.adventure.text.minimessage.tag.resolver.TagResolver;
@@ -72,7 +71,7 @@ public interface SerializableResolver {
   static @NotNull TagResolver claimingComponent(final @NotNull Set<String> names, final @NotNull BiFunction<ArgumentQueue, Context, Tag> handler, final @NotNull Function<Component, @Nullable Emitable> componentClaim) {
     final Set<String> ownNames = new HashSet<>(names);
     for (final String name : ownNames) {
-      checkKey(name);
+      TagInternals.checkTagName(name);
     }
     requireNonNull(handler, "handler");
     return new ComponentClaimingResolverImpl(ownNames, handler, componentClaim);
@@ -103,26 +102,12 @@ public interface SerializableResolver {
   static @NotNull TagResolver claimingStyle(final @NotNull Set<String> names, final @NotNull BiFunction<ArgumentQueue, Context, Tag> handler, final @NotNull StyleClaim<?> styleClaim) {
     final Set<String> ownNames = new HashSet<>(names);
     for (final String name : ownNames) {
-      checkKey(name);
+      TagInternals.checkTagName(name);
     }
     requireNonNull(handler, "handler");
     return new StyleClaimingResolverImpl(ownNames, handler, styleClaim);
   }
 
-  /**
-   * Check a key.
-   *
-   * <p>Temporary copy of {@code SingleResolver#checkKey(String)} until we have a better idea.</p>
-   *
-   * @param key the key
-   * @since 4.10.0
-   */
-  static void checkKey(final @NotNull String key) {
-    if (!Objects.requireNonNull(key, "key").equals(key.toLowerCase(Locale.ROOT))) {
-      throw new IllegalArgumentException("key must be lowercase, was " + key);
-    }
-  }
-
   /**
    * Attempt to process a component for serialization.
    *

text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/tag/resolver/SingleResolver.java

@@ -23,7 +23,6 @@
  */
 package net.kyori.adventure.text.minimessage.tag.resolver;
 
-import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
 import net.kyori.adventure.text.minimessage.tag.Tag;
@@ -33,12 +32,6 @@ final class SingleResolver implements TagResolver.Single, MappableResolver {
   private final String key;
   private final Tag tag;
 
-  static void checkKey(final @NotNull String key) {
-    if (!Objects.requireNonNull(key, "key").equals(key.toLowerCase(Locale.ROOT))) {
-      throw new IllegalArgumentException("key must be lowercase, was " + key);
-    }
-  }
-
   SingleResolver(final String key, final Tag tag) {
     this.key = key;
     this.tag = tag;

text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/tag/resolver/TagResolver.java

@@ -32,6 +32,7 @@
 import java.util.stream.Collector;
 import net.kyori.adventure.text.minimessage.Context;
 import net.kyori.adventure.text.minimessage.ParsingException;
+import net.kyori.adventure.text.minimessage.internal.TagInternals;
 import net.kyori.adventure.text.minimessage.tag.Tag;
 import net.kyori.adventure.text.minimessage.tag.standard.StandardTags;
 import org.jetbrains.annotations.ApiStatus;
@@ -91,7 +92,7 @@ public interface TagResolver {
    * @since 4.10.0
    */
   static TagResolver.@NotNull Single resolver(final @NotNull String name, final @NotNull Tag tag) {
-    SingleResolver.checkKey(name);
+    TagInternals.checkTagName(name);
     return new SingleResolver(
       name,
       requireNonNull(tag, "tag")
@@ -121,7 +122,7 @@ public interface TagResolver {
   static @NotNull TagResolver resolver(final @NotNull Set<String> names, final @NotNull BiFunction<ArgumentQueue, Context, Tag> handler) {
     final Set<String> ownNames = new HashSet<>(names);
     for (final String name : ownNames) {
-      SingleResolver.checkKey(name);
+      TagInternals.checkTagName(name);
     }
     requireNonNull(handler, "handler");
 

text-minimessage/src/test/java/net/kyori/adventure/text/minimessage/MiniMessageParserTest.java

@@ -26,14 +26,10 @@
 import net.kyori.adventure.text.Component;
 import net.kyori.adventure.text.TextComponent;
 import net.kyori.adventure.text.format.NamedTextColor;
-import net.kyori.adventure.text.minimessage.tag.Tag;
-import net.kyori.adventure.text.minimessage.tag.resolver.ArgumentQueue;
 import net.kyori.adventure.text.minimessage.tag.resolver.Placeholder;
 import net.kyori.adventure.text.minimessage.tag.resolver.TagResolver;
 import net.kyori.adventure.text.minimessage.tree.Node;
 import net.kyori.adventure.text.serializer.plain.PlainTextComponentSerializer;
-import org.jetbrains.annotations.NotNull;
-import org.jetbrains.annotations.Nullable;
 import org.junit.jupiter.api.Test;
 
 import static net.kyori.adventure.text.Component.empty;
@@ -53,9 +49,9 @@
 import static net.kyori.adventure.text.format.TextDecoration.UNDERLINED;
 import static net.kyori.adventure.text.minimessage.tag.resolver.Placeholder.component;
 import static net.kyori.adventure.text.minimessage.tag.resolver.Placeholder.parsed;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
-import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class MiniMessageParserTest extends AbstractTest {
 
@@ -345,40 +341,6 @@ void testEscapeAtEnd() {
     this.assertParsedEquals(expected, input);
   }
 
-  @Test
-  void testNoEscapesInTags() {
-    class TestResolver implements TagResolver {
-      private static final String EXPECTED_NAME = "hi\\\\there";
-      boolean has = false;
-      boolean resolved = false;
-
-      @Override
-      public @Nullable Tag resolve(@NotNull final String name, @NotNull final ArgumentQueue arguments, @NotNull final Context ctx) throws ParsingException {
-        if (name.equals(EXPECTED_NAME)) {
-          this.resolved = true;
-          return Tag.inserting(Component.text("hi"));
-        }
-        return null;
-      }
-
-      @Override
-      public boolean has(@NotNull final String name) {
-        if (name.equals(EXPECTED_NAME)) {
-          this.has = true;
-          return true;
-        }
-        return false;
-      }
-    }
-
-    final TestResolver instance = new TestResolver();
-
-    this.assertParsedEquals(Component.text("hi"), "<hi\\\\there>", instance);
-
-    assertTrue(instance.has, "has = false; escape was processed");
-    assertTrue(instance.resolved, "resolved = false; escape was processed");
-  }
-
   @Test
   void testCaseInsensitive() {
     final String input1 = "<red>this is <BOLD>an error</bold> message";
@@ -478,4 +440,32 @@ void testLegacySymbolForbidden() {
     // Non-strict
     System.out.println(assertThrows(ParsingException.class, () -> PARSER.deserialize(failingTest)).getMessage());
   }
+
+  @Test
+  void testInvalidTagNames() {
+    final String failingTest = "Hello <this_is_%not_allowed> but cool?";
+    final String failingTest1 = "Hello <this_is_not_allowed!> but cool?";
+    final String failingTest2 = "Hello <!?this_is_not_allowed> but cool?";
+    final String failingTest3 = "Hello <##this_is_%not_allowed> but cool?";
+
+    assertThrows(ParsingException.class, () -> PARSER.deserialize(failingTest));
+    assertThrows(ParsingException.class, () -> PARSER.deserialize(failingTest1));
+    assertThrows(ParsingException.class, () -> PARSER.deserialize(failingTest2));
+    assertThrows(ParsingException.class, () -> PARSER.deserialize(failingTest3));
+  }
+
+  @Test
+  void testValidTagNames() {
+    final String passingTest = "Hello <this_is_allowed> but cool?";
+    final String passingTest1 = "Hello <this-is-allowed> but cool?";
+    final String passingTest2 = "Hello <!allowed> but cool?";
+    final String passingTest3 = "Hello <?allowed> but cool?";
+    final String passingTest4 = "Hello <#allowed> but cool?";
+
+    assertDoesNotThrow(() -> PARSER.deserialize(passingTest));
+    assertDoesNotThrow(() -> PARSER.deserialize(passingTest1));
+    assertDoesNotThrow(() -> PARSER.deserialize(passingTest2));
+    assertDoesNotThrow(() -> PARSER.deserialize(passingTest3));
+    assertDoesNotThrow(() -> PARSER.deserialize(passingTest4));
+  }
 }

text-minimessage/src/test/java/net/kyori/adventure/text/minimessage/MiniMessageTest.java

@@ -200,7 +200,7 @@ void testOrderOfPlaceholders() {
     final Component expected = text("A")
       .append(text("B"))
       .append(text("C"));
-    final String input = "<a><b><_c>";
+    final String input = "<a><b><!c>";
     final MiniMessage miniMessage = MiniMessage.miniMessage();
 
     this.assertParsedEquals(
@@ -209,7 +209,7 @@ void testOrderOfPlaceholders() {
       input,
       component("a", text("A")),
       component("b", text("B")),
-      component("_c", text("C"))
+      component("!c", text("C"))
     );
   }
 

text-minimessage/src/test/java/net/kyori/adventure/text/minimessage/tag/TagResolverTest.java

@@ -38,8 +38,10 @@
 
 import static net.kyori.adventure.text.Component.text;
 import static net.kyori.adventure.text.format.TextColor.color;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.fail;
 
 class TagResolverTest {
@@ -118,6 +120,24 @@ void testContextParseVarargs() {
     assertEquals(expected, input);
   }
 
+  @Test
+  void testInvalidTagName() {
+    assertThrows(IllegalArgumentException.class, () -> TagResolver.resolver("INVALID_NAME", Tag.preProcessParsed("something")));
+    assertThrows(IllegalArgumentException.class, () -> TagResolver.resolver("!invalid!", Tag.preProcessParsed("something")));
+    assertThrows(IllegalArgumentException.class, () -> TagResolver.resolver("???", Tag.preProcessParsed("something")));
+    assertThrows(IllegalArgumentException.class, () -> TagResolver.resolver("#test#", Tag.preProcessParsed("something")));
+  }
+
+  @Test
+  void testValidTagName() {
+    assertDoesNotThrow(() -> TagResolver.resolver("valid_-name0909", Tag.preProcessParsed("something")));
+    assertDoesNotThrow(() -> TagResolver.resolver("!valid", Tag.preProcessParsed("something")));
+    assertDoesNotThrow(() -> TagResolver.resolver("?valid", Tag.preProcessParsed("something")));
+    assertDoesNotThrow(() -> TagResolver.resolver("#valid", Tag.preProcessParsed("something")));
+    assertDoesNotThrow(() -> TagResolver.resolver("valid99", Tag.preProcessParsed("something")));
+    assertDoesNotThrow(() -> TagResolver.resolver("v_9_v", Tag.preProcessParsed("something")));
+  }
+
   private static @NotNull Tag resolveForTest(final TagResolver resolver, final String tag) {
     try {
       final Context ctx = AbstractTest.dummyContext("help i shouldn't be seen");