From 3890d273496fbf76ad466bfe0a464a5875ddeb5f Mon Sep 17 00:00:00 2001 From: Emilia Dreamer Date: Wed, 14 Sep 2022 02:50:43 +0300 Subject: [PATCH 1/3] fix(minimessage): Ignore quotes with no matching closing quote Don't enter string parsing mode during the first pass when encountering a quote, but no matching quote exists later on in the message. Note that this doesn't catch every case of invalid tags causing other valid tags to be ignored, in this case there could simply be an unrelated quote elsewhere in the message and the bug would still occur. The parser could: * Only allow quotes after specific characters, such as the colon `:`, which is currently the only place where quotes are really valid. However, even then, the buggy payload could just include a colon before the quote * Only allow quotes when inside of a valid tag, as in the test. (`<3` isn't a valid introducer for a tag). However, even then, the buggy payload could use a character valid for tag. Additionally, checking the validity of a tag isn't done by the first parse stage * Backtrack when a quote-delimited section doesn't end up being inside of a valid tag. That would (presumably) require some pretty complicated checks in the first phase, but checking for it in the second phase is probably too late, because it'd have to reparse something it has already passed by --- .../text/minimessage/internal/parser/TokenParser.java | 5 ++++- .../text/minimessage/MiniMessageParserTest.java | 10 ++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/internal/parser/TokenParser.java b/text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/internal/parser/TokenParser.java index 60fb9528c..900901b44 100644 --- a/text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/internal/parser/TokenParser.java +++ b/text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/internal/parser/TokenParser.java @@ -246,8 +246,11 @@ public static void parseString(final String message, final MatchedTokenConsumer< break; case '\'': case '"': - state = FirstPassState.STRING; currentStringChar = (char) codePoint; + // Look ahead if the quote being opened is ever closed + if (message.substring(i + 1).indexOf(codePoint) != -1) { + state = FirstPassState.STRING; + } break; } break; diff --git a/text-minimessage/src/test/java/net/kyori/adventure/text/minimessage/MiniMessageParserTest.java b/text-minimessage/src/test/java/net/kyori/adventure/text/minimessage/MiniMessageParserTest.java index 086f32e9b..2b29a3b1f 100644 --- a/text-minimessage/src/test/java/net/kyori/adventure/text/minimessage/MiniMessageParserTest.java +++ b/text-minimessage/src/test/java/net/kyori/adventure/text/minimessage/MiniMessageParserTest.java @@ -280,6 +280,16 @@ void testQuoteEscapingInArguments() { this.assertParsedEquals(expected3, input3); } + @Test + void testNonTerminatingQuote() { + final Component expected = empty().append(text("Remember the<3\"").color(RED)).append(text(" bug")); + final Component expected1 = empty().append(text("Remember the<3'").color(RED)).append(text(" bug")); + final String input = "Remember the<3\" bug"; + final String input1 = "Remember the<3' bug"; + this.assertParsedEquals(expected, input); + this.assertParsedEquals(expected1, input1); + } + // GH-68, GH-93 @Test void testAngleBracketsShit() { From 108dc343e4eaa73e937974be0a612c97c2a64020 Mon Sep 17 00:00:00 2001 From: Emilia Dreamer Date: Wed, 14 Sep 2022 20:24:55 +0300 Subject: [PATCH 2/3] fix(minimessage): Backtrack when the content of a tag never ends A tag is opened by `<` and closed by `>`. Upon encountering an opening angle bracket `<`, the parser enters the "TAG" state, during which it'll parse quote-delimited sequences. However, if by the end of the input, a closing angle bracket `>` is never encountered, the parser simply adds everything as a single text node, including the contents of the quote-delimited sequences. These quote delimited sequences could have contained other valid tags that are now directly added to the output without being parsed as tags. With this change, when the parser finds itself at the end of the input in the TAG state, it will backtrack back to the beginning of the `<` that caused a state change, and parse again from there on, but this time *not* in the tag state. This means that quotes are effectively ignored and their contents parsed as normal text This *still* doesn't account for all corner case, for example: `foo "> bar`. This is an example of a *correct* use of quotes, and the expected behaviour is for the whole output to be red, and the actual behaviour matches that, however: `foo <3:""> bar`. This is nearly identical to the above, but <3> isn't a valid tag, which means the red tag should be closed in the middle of the message. Instead, the message ends up wholly red. `foo "> bar`. The same applies here, but instead of an invalid tag, it is invalid syntax, since quotes should follow a colon. --- .../text/minimessage/internal/parser/TokenParser.java | 8 ++++++++ .../text/minimessage/MiniMessageParserTest.java | 10 ++++++++++ 2 files changed, 18 insertions(+) diff --git a/text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/internal/parser/TokenParser.java b/text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/internal/parser/TokenParser.java index 900901b44..3a86705d4 100644 --- a/text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/internal/parser/TokenParser.java +++ b/text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/internal/parser/TokenParser.java @@ -260,6 +260,14 @@ public static void parseString(final String message, final MatchedTokenConsumer< } break; } + + if (i == (length - 1) && state == FirstPassState.TAG) { + // We've reached the end of the input with an open `<`, but it was never matched to a closing `>`. + // Anything which was inside of quotes needs to be parsed again, as it may contain additional tags. + // Rewind back to directly after the `<`, but in the NORMAL state, instead of TAG. + i = marker; + state = FirstPassState.NORMAL; + } } // anything left over is plain text diff --git a/text-minimessage/src/test/java/net/kyori/adventure/text/minimessage/MiniMessageParserTest.java b/text-minimessage/src/test/java/net/kyori/adventure/text/minimessage/MiniMessageParserTest.java index 2b29a3b1f..bff3456d4 100644 --- a/text-minimessage/src/test/java/net/kyori/adventure/text/minimessage/MiniMessageParserTest.java +++ b/text-minimessage/src/test/java/net/kyori/adventure/text/minimessage/MiniMessageParserTest.java @@ -284,10 +284,20 @@ void testQuoteEscapingInArguments() { void testNonTerminatingQuote() { final Component expected = empty().append(text("Remember the<3\"").color(RED)).append(text(" bug")); final Component expected1 = empty().append(text("Remember the<3'").color(RED)).append(text(" bug")); + final Component expected2 = empty().append(text("Remember the \">bug").color(RED)); final String input = "Remember the<3\" bug"; final String input1 = "Remember the<3' bug"; + final String input2 = "Remember the bug"; + final String input3 = "Remember the \"bug"; + final String input4 = "Remember the \">bug"; this.assertParsedEquals(expected, input); this.assertParsedEquals(expected1, input1); + this.assertParsedEquals(expected2, input2); + this.assertParsedEquals(expected3, input3); + this.assertParsedEquals(expected4, input4); } // GH-68, GH-93 From 8b716909c30d0b0732f96e9d171e3c9211d76cd9 Mon Sep 17 00:00:00 2001 From: Emilia Dreamer Date: Mon, 7 Nov 2022 15:06:04 +0200 Subject: [PATCH 3/3] Apply suggestion from review Co-authored-by: zml --- .../adventure/text/minimessage/internal/parser/TokenParser.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/internal/parser/TokenParser.java b/text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/internal/parser/TokenParser.java index 3a86705d4..040741ca7 100644 --- a/text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/internal/parser/TokenParser.java +++ b/text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/internal/parser/TokenParser.java @@ -248,7 +248,7 @@ public static void parseString(final String message, final MatchedTokenConsumer< case '"': currentStringChar = (char) codePoint; // Look ahead if the quote being opened is ever closed - if (message.substring(i + 1).indexOf(codePoint) != -1) { + if (message.indexOf(codePoint, i + 1) != -1) { state = FirstPassState.STRING; } break;