Skip to content

Commit

Permalink
Merge pull request #723 from KyoriPowered/fix/invalid-tag-parsing
Browse files Browse the repository at this point in the history
  • Loading branch information
@rymiel @zml2008
rymiel authored and zml2008 committed Mar 7, 2022
1 parent 7688fe5 commit ef0d837
Show file tree
Hide file tree
Showing 7 changed files with 72 additions and 28 deletions.
18 changes: 15 additions & 3 deletions ...minimessage/src/main/java/net/kyori/adventure/text/minimessage/internal/TagInternals.java
View file
Expand Up @@ -48,12 +48,24 @@ private TagInternals() {
* @param tagName the name of the tag
* @since 4.10.0
*/
public static void checkTagName(final @NotNull String tagName) {
public static void assertValidTagName(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. Returns a boolean representing the validity
*
* @param tagName the name of the tag
* @return validity of this tag when sanitized
* @since 4.10.1
*/
public static boolean sanitizeAndCheckValidTagName(final @NotNull String tagName) {
return TAG_NAME_PATTERN.matcher(Objects.requireNonNull(tagName).toLowerCase(Locale.ROOT)).matches();
}

/**
* 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
Expand All @@ -62,7 +74,7 @@ public static void checkTagName(final @NotNull String tagName) {
* @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));
public static void sanitizeAndAssertValidTagName(final @NotNull String tagName) {
assertValidTagName(Objects.requireNonNull(tagName).toLowerCase(Locale.ROOT));
}
}
10 changes: 10 additions & 0 deletions ...ssage/src/main/java/net/kyori/adventure/text/minimessage/internal/parser/TokenParser.java
View file
Expand Up @@ -31,6 +31,7 @@
import java.util.function.IntPredicate;
import java.util.function.Predicate;
import net.kyori.adventure.text.minimessage.ParsingException;
import net.kyori.adventure.text.minimessage.internal.TagInternals;
import net.kyori.adventure.text.minimessage.internal.parser.match.MatchedTokenConsumer;
import net.kyori.adventure.text.minimessage.internal.parser.match.StringResolvingMatchedTokenConsumer;
import net.kyori.adventure.text.minimessage.internal.parser.match.TokenListProducingMatchedTokenConsumer;
Expand Down Expand Up @@ -375,6 +376,15 @@ private static RootNode buildTree(

case OPEN_TAG:
case OPEN_CLOSE_TAG:
// Check if this even is a valid tag
final Token tagNamePart = token.childTokens().get(0);
final String tagName = message.substring(tagNamePart.startIndex(), tagNamePart.endIndex());
if (!TagInternals.sanitizeAndCheckValidTagName(tagName)) {
// This wouldn't be a valid tag, just parse it as text instead!
node.addChild(new TextNode(node, token, message));
break;
}

final TagNode tagNode = new TagNode(node, token, message, tagProvider);
if (tagNameChecker.test(tagNode.name())) {
final Tag tag = tagProvider.resolve(tagNode);
Expand Down
14 changes: 9 additions & 5 deletions ...adventure/text/minimessage/internal/parser/match/StringResolvingMatchedTokenConsumer.java
View file
Expand Up @@ -24,6 +24,7 @@
package net.kyori.adventure.text.minimessage.internal.parser.match;

import java.util.Objects;
import net.kyori.adventure.text.minimessage.internal.TagInternals;
import net.kyori.adventure.text.minimessage.internal.parser.TokenParser;
import net.kyori.adventure.text.minimessage.internal.parser.TokenParser.TagProvider;
import net.kyori.adventure.text.minimessage.internal.parser.TokenType;
Expand Down Expand Up @@ -69,12 +70,15 @@ public void accept(final int start, final int end, final @NotNull TokenType toke
final String match = this.input.substring(start, end);
final String tag = this.input.substring(start + 1, end - 1);

// we might care if it's a pre-process!
final @Nullable Tag replacement = this.tagProvider.resolve(TokenParser.TagProvider.sanitizePlaceholderName(tag));
// we might care if it's a valid tag!
if (TagInternals.sanitizeAndCheckValidTagName(tag)) {
// we might care if it's a pre-process!
final @Nullable Tag replacement = this.tagProvider.resolve(TokenParser.TagProvider.sanitizePlaceholderName(tag));

if (replacement instanceof PreProcess) {
this.builder.append(Objects.requireNonNull(((PreProcess) replacement).value(), "PreProcess replacements cannot return null"));
return;
if (replacement instanceof PreProcess) {
this.builder.append(Objects.requireNonNull(((PreProcess) replacement).value(), "PreProcess replacements cannot return null"));
return;
}
}

// if we get here, the placeholder wasn't found or was null
Expand Down
8 changes: 0 additions & 8 deletions ...sage/src/main/java/net/kyori/adventure/text/minimessage/internal/parser/node/TagNode.java
View file
Expand Up @@ -26,7 +26,6 @@
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;
Expand Down Expand Up @@ -65,13 +64,6 @@ public TagNode(
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(
Expand Down
4 changes: 2 additions & 2 deletions ...n/java/net/kyori/adventure/text/minimessage/internal/serializer/SerializableResolver.java
View file
Expand Up @@ -71,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) {
TagInternals.checkTagName(name);
TagInternals.assertValidTagName(name);
}
requireNonNull(handler, "handler");
return new ComponentClaimingResolverImpl(ownNames, handler, componentClaim);
Expand Down Expand Up @@ -102,7 +102,7 @@ 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) {
TagInternals.checkTagName(name);
TagInternals.assertValidTagName(name);
}
requireNonNull(handler, "handler");
return new StyleClaimingResolverImpl(ownNames, handler, styleClaim);
Expand Down
4 changes: 2 additions & 2 deletions ...imessage/src/main/java/net/kyori/adventure/text/minimessage/tag/resolver/TagResolver.java
View file
Expand Up @@ -92,7 +92,7 @@ public interface TagResolver {
* @since 4.10.0
*/
static TagResolver.@NotNull Single resolver(final @NotNull String name, final @NotNull Tag tag) {
TagInternals.checkTagName(name);
TagInternals.assertValidTagName(name);
return new SingleResolver(
name,
requireNonNull(tag, "tag")
Expand Down Expand Up @@ -122,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) {
TagInternals.checkTagName(name);
TagInternals.assertValidTagName(name);
}
requireNonNull(handler, "handler");

Expand Down
42 changes: 34 additions & 8 deletions ...minimessage/src/test/java/net/kyori/adventure/text/minimessage/MiniMessageParserTest.java
View file
Expand Up @@ -26,10 +26,13 @@
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.junit.jupiter.api.Test;

import static net.kyori.adventure.text.Component.empty;
Expand Down Expand Up @@ -443,15 +446,19 @@ void testLegacySymbolForbidden() {

@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?";
final String input1 = "Hello <this_is_%not_allowed> but ignored?";
final String input2 = "Hello <this_is_not_allowed!> but ignored?";
final String input3 = "Hello <!?this_is_not_allowed> but ignored?";
final String input4 = "Hello <##this_is_%not_allowed> but ignored?";
final String input5 = "<3 >Mini<3 />Message</3 >";
final String input6 = "this message <\"red\">isn't red";

assertThrows(ParsingException.class, () -> PARSER.deserialize(failingTest));
assertThrows(ParsingException.class, () -> PARSER.deserialize(failingTest1));
assertThrows(ParsingException.class, () -> PARSER.deserialize(failingTest2));
assertThrows(ParsingException.class, () -> PARSER.deserialize(failingTest3));
this.assertParsedEquals(Component.text(input1), input1);
this.assertParsedEquals(Component.text(input2), input2);
this.assertParsedEquals(Component.text(input3), input3);
this.assertParsedEquals(Component.text(input4), input4);
this.assertParsedEquals(Component.text(input5), input5);
this.assertParsedEquals(Component.text(input6), input6);
}

@Test
Expand All @@ -468,4 +475,23 @@ void testValidTagNames() {
assertDoesNotThrow(() -> PARSER.deserialize(passingTest3));
assertDoesNotThrow(() -> PARSER.deserialize(passingTest4));
}

@Test
void invalidPreprocessTagNames() {
final String input = "Some<##>of<>these<tag>are<3 >tags";
final Component expected = Component.text("Some<##>of<>these(meow)are<3 >tags");
final TagResolver alwaysMatchingResolver = new TagResolver() {
@Override
public Tag resolve(final @NotNull String name, final @NotNull ArgumentQueue arguments, final @NotNull Context ctx) throws ParsingException {
return Tag.preProcessParsed("(meow)");
}

@Override
public boolean has(final @NotNull String name) {
return true;
}
};

assertParsedEquals(expected, input, alwaysMatchingResolver);
}
}

0 comments on commit ef0d837

Please sign in to comment.