From f6ad36e9eeee2a77a820aa92cbdfcaac525f9a19 Mon Sep 17 00:00:00 2001 From: Emily Date: Wed, 11 Sep 2024 15:35:38 -0300 Subject: [PATCH 1/5] Fix `DurationParser` allowing invalid input by not using regex and manually parsing the input --- .../cloud/parser/standard/DurationParser.java | 74 ++++++++++++++----- .../parser/standard/DurationParserTest.java | 37 +++++++++- 2 files changed, 92 insertions(+), 19 deletions(-) diff --git a/cloud-core/src/main/java/org/incendo/cloud/parser/standard/DurationParser.java b/cloud-core/src/main/java/org/incendo/cloud/parser/standard/DurationParser.java index cfa955955..9799828f5 100644 --- a/cloud-core/src/main/java/org/incendo/cloud/parser/standard/DurationParser.java +++ b/cloud-core/src/main/java/org/incendo/cloud/parser/standard/DurationParser.java @@ -25,13 +25,12 @@ import java.time.Duration; import java.util.Collections; -import java.util.regex.Matcher; -import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.IntStream; import java.util.stream.Stream; import org.apiguardian.api.API; import org.checkerframework.checker.nullness.qual.NonNull; +import org.checkerframework.checker.nullness.qual.Nullable; import org.incendo.cloud.caption.CaptionVariable; import org.incendo.cloud.caption.StandardCaptionKeys; import org.incendo.cloud.component.CommandComponent; @@ -73,48 +72,65 @@ public final class DurationParser implements ArgumentParser, Blo return CommandComponent.builder().parser(durationParser()); } - /** - * Matches durations in the format of: 2d15h7m12s - */ - private static final Pattern DURATION_PATTERN = Pattern.compile("(([1-9][0-9]+|[1-9])[dhms])"); - @Override public @NonNull ArgumentParseResult parse( final @NonNull CommandContext commandContext, final @NonNull CommandInput commandInput ) { - final String input = commandInput.readString(); - - final Matcher matcher = DURATION_PATTERN.matcher(input); + final String input = commandInput.peekString(); Duration duration = Duration.ofNanos(0); - while (matcher.find()) { - String group = matcher.group(); - String timeUnit = String.valueOf(group.charAt(group.length() - 1)); - int timeValue = Integer.parseInt(group.substring(0, group.length() - 1)); + // substring range enclosing digits and unit (single char) + int rangeStart = 0; + int cursor = 0; + + while (cursor < input.length()) { + // advance cursor until time unit or we reach end of input (in which case it's invalid anyway) + while (cursor < input.length() && Character.isDigit(input.charAt(cursor))) { + cursor += 1; + } + + // reached end of input with no time unit + if (cursor == input.length()) { + return ArgumentParseResult.failure(new DurationParseException(input, commandContext)); + } + + final long timeValue; + try { + timeValue = Long.parseUnsignedLong(input.substring(rangeStart, cursor)); + } catch (final NumberFormatException ex) { + return ArgumentParseResult.failure(new DurationParseException(ex, input, commandContext)); + } + + final char timeUnit = input.charAt(cursor); switch (timeUnit) { - case "d": + case 'd': duration = duration.plusDays(timeValue); break; - case "h": + case 'h': duration = duration.plusHours(timeValue); break; - case "m": + case 'm': duration = duration.plusMinutes(timeValue); break; - case "s": + case 's': duration = duration.plusSeconds(timeValue); break; default: return ArgumentParseResult.failure(new DurationParseException(input, commandContext)); } + + // skip unit, reset rangeStart to start of next segment + cursor += 1; + rangeStart = cursor; } if (duration.isZero()) { return ArgumentParseResult.failure(new DurationParseException(input, commandContext)); } + commandInput.readString(); // pop read input on success return ArgumentParseResult.success(duration); } @@ -172,6 +188,28 @@ public DurationParseException( this.input = input; } + /** + * Construct a new {@link DurationParseException} with a causing exception. + * + * @param cause cause of exception + * @param input input string + * @param context command context + */ + public DurationParseException( + final @Nullable Throwable cause, + final @NonNull String input, + final @NonNull CommandContext context + ) { + super( + cause, + DurationParser.class, + context, + StandardCaptionKeys.ARGUMENT_PARSE_FAILURE_DURATION, + CaptionVariable.of("input", input) + ); + this.input = input; + } + /** * Returns the supplied input string. * diff --git a/cloud-core/src/test/java/org/incendo/cloud/parser/standard/DurationParserTest.java b/cloud-core/src/test/java/org/incendo/cloud/parser/standard/DurationParserTest.java index 6e6defbe4..610d7aeb3 100644 --- a/cloud-core/src/test/java/org/incendo/cloud/parser/standard/DurationParserTest.java +++ b/cloud-core/src/test/java/org/incendo/cloud/parser/standard/DurationParserTest.java @@ -75,15 +75,50 @@ void single_multiple_units() { } @Test - void invalid_format_failing() { + void invalid_format_no_time_value() { Assertions.assertThrows( CompletionException.class, () -> manager.commandExecutor().executeCommand(new TestCommandSender(), "duration d").join() ); + } + @Test + void invalid_format_no_time_unit() { + Assertions.assertThrows( + CompletionException.class, + () -> manager.commandExecutor().executeCommand(new TestCommandSender(), "duration 1").join() + ); + } + + @Test + void invalid_format_invalid_unit() { Assertions.assertThrows( CompletionException.class, () -> manager.commandExecutor().executeCommand(new TestCommandSender(), "duration 1x").join() ); } + + @Test + void invalid_format_leading_garbage() { + Assertions.assertThrows( + CompletionException.class, + () -> manager.commandExecutor().executeCommand(new TestCommandSender(), "duration foo1d").join() + ); + } + + @Test + void invalid_format_garbage() { + Assertions.assertThrows( + CompletionException.class, + () -> manager.commandExecutor().executeCommand(new TestCommandSender(), "duration 1dfoo2h").join() + ); + } + + @Test + void invalid_format_trailing_garbage() { + Assertions.assertThrows( + CompletionException.class, + () -> manager.commandExecutor().executeCommand(new TestCommandSender(), "duration 1dfoo").join() + ); + } } From 6bc7e4ee46b7f5fb8e3e6f83f708e5b08844dbf8 Mon Sep 17 00:00:00 2001 From: Emily Date: Wed, 11 Sep 2024 15:49:11 -0300 Subject: [PATCH 2/5] Do not parse time amount as unsigned long --- .../java/org/incendo/cloud/parser/standard/DurationParser.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cloud-core/src/main/java/org/incendo/cloud/parser/standard/DurationParser.java b/cloud-core/src/main/java/org/incendo/cloud/parser/standard/DurationParser.java index 9799828f5..be0a3ef72 100644 --- a/cloud-core/src/main/java/org/incendo/cloud/parser/standard/DurationParser.java +++ b/cloud-core/src/main/java/org/incendo/cloud/parser/standard/DurationParser.java @@ -98,7 +98,7 @@ public final class DurationParser implements ArgumentParser, Blo final long timeValue; try { - timeValue = Long.parseUnsignedLong(input.substring(rangeStart, cursor)); + timeValue = Long.parseLong(input.substring(rangeStart, cursor)); } catch (final NumberFormatException ex) { return ArgumentParseResult.failure(new DurationParseException(ex, input, commandContext)); } From 6b06491e20aa7e2353e89d2ce1ca2093491fe74c Mon Sep 17 00:00:00 2001 From: Emily Date: Wed, 11 Sep 2024 16:09:46 -0300 Subject: [PATCH 3/5] Address comment --- .../java/org/incendo/cloud/parser/standard/DurationParser.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cloud-core/src/main/java/org/incendo/cloud/parser/standard/DurationParser.java b/cloud-core/src/main/java/org/incendo/cloud/parser/standard/DurationParser.java index be0a3ef72..046a05058 100644 --- a/cloud-core/src/main/java/org/incendo/cloud/parser/standard/DurationParser.java +++ b/cloud-core/src/main/java/org/incendo/cloud/parser/standard/DurationParser.java @@ -77,7 +77,7 @@ public final class DurationParser implements ArgumentParser, Blo final @NonNull CommandContext commandContext, final @NonNull CommandInput commandInput ) { - final String input = commandInput.peekString(); + final String input = commandInput.readString(); Duration duration = Duration.ofNanos(0); @@ -130,7 +130,6 @@ public final class DurationParser implements ArgumentParser, Blo return ArgumentParseResult.failure(new DurationParseException(input, commandContext)); } - commandInput.readString(); // pop read input on success return ArgumentParseResult.success(duration); } From c210a5c2098a15bd9fb32be9f20287d53429f9e2 Mon Sep 17 00:00:00 2001 From: Emily Date: Wed, 11 Sep 2024 19:17:45 -0300 Subject: [PATCH 4/5] Readd parsed format to class javadoc --- .../java/org/incendo/cloud/parser/standard/DurationParser.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cloud-core/src/main/java/org/incendo/cloud/parser/standard/DurationParser.java b/cloud-core/src/main/java/org/incendo/cloud/parser/standard/DurationParser.java index 046a05058..0ce7164ef 100644 --- a/cloud-core/src/main/java/org/incendo/cloud/parser/standard/DurationParser.java +++ b/cloud-core/src/main/java/org/incendo/cloud/parser/standard/DurationParser.java @@ -45,6 +45,8 @@ /** * Parser for {@link Duration}. * + *

Matches durations in the format of: 2d15h7m12s.

+ * * @param command sender type */ @API(status = API.Status.STABLE) From d3911ac4529acebd3b8f08475aea198bf8c8b718 Mon Sep 17 00:00:00 2001 From: Emily Date: Sun, 15 Sep 2024 17:17:13 -0300 Subject: [PATCH 5/5] Handle case where input duration overflows --- .../cloud/parser/standard/DurationParser.java | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/cloud-core/src/main/java/org/incendo/cloud/parser/standard/DurationParser.java b/cloud-core/src/main/java/org/incendo/cloud/parser/standard/DurationParser.java index 0ce7164ef..727bb0bad 100644 --- a/cloud-core/src/main/java/org/incendo/cloud/parser/standard/DurationParser.java +++ b/cloud-core/src/main/java/org/incendo/cloud/parser/standard/DurationParser.java @@ -106,21 +106,25 @@ public final class DurationParser implements ArgumentParser, Blo } final char timeUnit = input.charAt(cursor); - switch (timeUnit) { - case 'd': - duration = duration.plusDays(timeValue); - break; - case 'h': - duration = duration.plusHours(timeValue); - break; - case 'm': - duration = duration.plusMinutes(timeValue); - break; - case 's': - duration = duration.plusSeconds(timeValue); - break; - default: - return ArgumentParseResult.failure(new DurationParseException(input, commandContext)); + try { + switch (timeUnit) { + case 'd': + duration = duration.plusDays(timeValue); + break; + case 'h': + duration = duration.plusHours(timeValue); + break; + case 'm': + duration = duration.plusMinutes(timeValue); + break; + case 's': + duration = duration.plusSeconds(timeValue); + break; + default: + return ArgumentParseResult.failure(new DurationParseException(input, commandContext)); + } + } catch (final ArithmeticException ex) { + return ArgumentParseResult.failure(new DurationParseException(ex, input, commandContext)); } // skip unit, reset rangeStart to start of next segment