Fix DurationParser allowing invalid input by not using regex and manually parsing the input

Author: emilyy-dev

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<C> implements ArgumentParser<C, Duration>, Blo
         return CommandComponent.<C, Duration>builder().parser(durationParser());
     }
 
-    /**
-     * Matches durations in the format of: <code>2d15h7m12s</code>
-     */
-    private static final Pattern DURATION_PATTERN = Pattern.compile("(([1-9][0-9]+|[1-9])[dhms])");
-
     @Override
     public @NonNull ArgumentParseResult<Duration> parse(
             final @NonNull CommandContext<C> 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.
          *

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()
+        );
+    }
 }