[java] Enforce commas between JSON elements; accept trailing commas (#17738)

`hasNext()` previously consumed a leading comma if present, but never
required one. That accepted spec-invalid inputs like '[1 2 3]' (missing
separator) and '[,1]' (leading comma). It also rejected trailing commas
like '[1,2,3,]' because it consumed the comma and then tried to read
another element.

Track whether the current container has seen an element (parallel deque
kept in sync with the container stack, marked in expect()) and use it
in hasNext() to require a comma between elements while allowing a
single trailing comma before the container's closer.

Author: shs96c

java/src/org/openqa/selenium/json/JsonInput.java

@@ -49,6 +49,10 @@ public class JsonInput implements Closeable {
   // Used when reading maps and collections so that we handle de-nesting and
   // figuring out whether we're expecting a NAME properly.
   private final Deque<Container> stack = new ArrayDeque<>();
+  // Parallel stack tracking whether the current container has seen at least
+  // one element. Used by hasNext() to enforce comma separators between
+  // elements while remaining lenient about a single trailing comma.
+  private final Deque<Boolean> containerHasElement = new ArrayDeque<>();
 
   JsonInput(Reader source, JsonTypeCoercer coercer, PropertySetting setter) {
 
@@ -353,13 +357,30 @@ public boolean hasNext() {
     }
 
     skipWhitespace(input);
+    boolean seenElement = Boolean.TRUE.equals(containerHasElement.peekFirst());
+
     if (input.peek() == ',') {
+      if (!seenElement) {
+        throw new JsonException("Unexpected ',' before first element of container. " + input);
+      }
       input.read();
-      return true;
+      // We've moved past the separator, so we're once again expecting an element rather than
+      // another comma. Clear the flag so a repeat hasNext() before reading is a no-op.
+      clearSeenElement();
+      skipWhitespace(input);
+      JsonType afterComma = peek();
+      // Trailing comma leniency: '[1,]' and '{"a":1,}' are accepted.
+      return afterComma != JsonType.END_COLLECTION && afterComma != JsonType.END_MAP;
     }
 
     JsonType type = peek();
-    return type != JsonType.END_COLLECTION && type != JsonType.END_MAP;
+    if (type == JsonType.END_COLLECTION || type == JsonType.END_MAP) {
+      return false;
+    }
+    if (seenElement) {
+      throw new JsonException("Expected ',' or end of container but saw " + type + ". " + input);
+    }
+    return true;
   }
 
   /**
@@ -370,6 +391,7 @@ public boolean hasNext() {
   public void beginArray() {
     expect(JsonType.START_COLLECTION);
     stack.addFirst(Container.COLLECTION);
+    containerHasElement.addFirst(false);
     input.read();
   }
 
@@ -380,12 +402,13 @@ public void beginArray() {
    */
   public void endArray() {
     expect(JsonType.END_COLLECTION);
-    Container expectation = stack.removeFirst();
-    if (expectation != Container.COLLECTION) {
+    if (stack.peekFirst() != Container.COLLECTION) {
       // The only other thing we could be closing is a map
       throw new JsonException(
           "Attempt to close a JSON List, but a JSON Object was expected. " + input);
     }
+    stack.removeFirst();
+    containerHasElement.removeFirst();
     input.read();
   }
 
@@ -397,6 +420,7 @@ public void endArray() {
   public void beginObject() {
     expect(JsonType.START_MAP);
     stack.addFirst(Container.MAP_NAME);
+    containerHasElement.addFirst(false);
     input.read();
   }
 
@@ -407,11 +431,11 @@ public void beginObject() {
    */
   public void endObject() {
     expect(JsonType.END_MAP);
-    Container expectation = stack.removeFirst();
-    if (expectation != Container.MAP_NAME) {
-      // The only other thing we could be closing is a map
+    if (stack.peekFirst() != Container.MAP_NAME) {
       throw new JsonException("Attempt to close a JSON Map, but not ready to. " + input);
     }
+    stack.removeFirst();
+    containerHasElement.removeFirst();
     input.read();
   }
 
@@ -565,10 +589,31 @@ private void expect(JsonType type) {
       return; // End of Name handling
     }
 
-    // Handle the case where we're reading a value
+    // Handle the case where we're reading a value.
+    if (type == JsonType.END_COLLECTION || type == JsonType.END_MAP) {
+      // Closing the container - don't treat as a new element in it.
+      return;
+    }
     if (top == Container.MAP_VALUE) {
       stack.removeFirst();
       stack.addFirst(Container.MAP_NAME);
+      markElementRead();
+    } else if (top == Container.COLLECTION) {
+      markElementRead();
+    }
+  }
+
+  private void markElementRead() {
+    if (!containerHasElement.isEmpty()) {
+      containerHasElement.removeFirst();
+      containerHasElement.addFirst(true);
+    }
+  }
+
+  private void clearSeenElement() {
+    if (!containerHasElement.isEmpty()) {
+      containerHasElement.removeFirst();
+      containerHasElement.addFirst(false);
     }
   }
 

java/test/org/openqa/selenium/json/JsonInputTest.java

@@ -384,6 +384,119 @@ void shouldReportEndOfInputAsEofNotAsAUnicodeReplacement() {
     }
   }
 
+  @Test
+  void shouldAcceptTrailingCommaInArray() {
+    try (JsonInput input = newInput("[1,2,3,]")) {
+      input.beginArray();
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.nextNumber()).isEqualTo(1L);
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.nextNumber()).isEqualTo(2L);
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.nextNumber()).isEqualTo(3L);
+      assertThat(input.hasNext()).isFalse();
+      input.endArray();
+    }
+  }
+
+  @Test
+  void shouldAcceptTrailingCommaInObject() {
+    try (JsonInput input = newInput("{\"a\":1,}")) {
+      input.beginObject();
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.nextName()).isEqualTo("a");
+      assertThat(input.nextNumber()).isEqualTo(1L);
+      assertThat(input.hasNext()).isFalse();
+      input.endObject();
+    }
+  }
+
+  @Test
+  void shouldRejectMissingCommaBetweenArrayElements() {
+    try (JsonInput input = newInput("[1 2]")) {
+      input.beginArray();
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.nextNumber()).isEqualTo(1L);
+      assertThatExceptionOfType(JsonException.class).isThrownBy(input::hasNext);
+    }
+  }
+
+  @Test
+  void shouldRejectLeadingCommaInArray() {
+    try (JsonInput input = newInput("[,1]")) {
+      input.beginArray();
+      assertThatExceptionOfType(JsonException.class).isThrownBy(input::hasNext);
+    }
+  }
+
+  @Test
+  void shouldRejectMissingCommaBetweenObjectEntries() {
+    try (JsonInput input = newInput("{\"a\":1 \"b\":2}")) {
+      input.beginObject();
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.nextName()).isEqualTo("a");
+      assertThat(input.nextNumber()).isEqualTo(1L);
+      assertThatExceptionOfType(JsonException.class).isThrownBy(input::hasNext);
+    }
+  }
+
+  @Test
+  void shouldRejectLeadingCommaInObject() {
+    try (JsonInput input = newInput("{,\"a\":1}")) {
+      input.beginObject();
+      assertThatExceptionOfType(JsonException.class).isThrownBy(input::hasNext);
+    }
+  }
+
+  @Test
+  void mismatchedCloseLeavesParserStateIntact() {
+    // endObject() while inside an array must fail without popping the container stack,
+    // so the parser still knows it is inside the array afterwards.
+    try (JsonInput input = newInput("[1}")) {
+      input.beginArray();
+      assertThat(input.nextNumber()).isEqualTo(1L);
+      assertThatExceptionOfType(JsonException.class)
+          .isThrownBy(input::endObject)
+          .withMessageStartingWith("Attempt to close a JSON Map");
+      // Before the fix this threw "not in a container type" because the array's
+      // stack entry had already been popped.
+      assertThat(input.hasNext()).isFalse();
+    }
+  }
+
+  @Test
+  void hasNextIsIdempotentBetweenElementsInArray() {
+    // Iterator-style probing (peek/hasNext repeatedly before reading) must not falsely fail
+    // once hasNext() has already consumed the comma before the next element.
+    try (JsonInput input = newInput("[1,2]")) {
+      input.beginArray();
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.nextNumber()).isEqualTo(1L);
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.nextNumber()).isEqualTo(2L);
+      assertThat(input.hasNext()).isFalse();
+      assertThat(input.hasNext()).isFalse();
+      input.endArray();
+    }
+  }
+
+  @Test
+  void hasNextIsIdempotentBetweenEntriesInObject() {
+    try (JsonInput input = newInput("{\"a\":1,\"b\":2}")) {
+      input.beginObject();
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.nextName()).isEqualTo("a");
+      assertThat(input.nextNumber()).isEqualTo(1L);
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.hasNext()).isTrue();
+      assertThat(input.nextName()).isEqualTo("b");
+      assertThat(input.nextNumber()).isEqualTo(2L);
+      assertThat(input.hasNext()).isFalse();
+      input.endObject();
+    }
+  }
+
   @Test
   void nullInputsShouldCoerceAsNullValues() throws IOException {
     try (InputStream is = new ByteArrayInputStream(new byte[0]);