[BugFix] Validate table name and reject duplicate or positional args in vectorSearch()

Three shape-level validation gaps on vectorSearch() previously crashed the
server or returned a misleading 200 with zero rows:

* Validate table= with the same SAFE_FIELD_NAME regex that already guards
  field=. Previously a hostile table name could reach the native layer
  unvalidated.
* Reject duplicate named arguments in both the Resolver and the
  Implementation. The same name appearing twice (e.g. table=a, table=b)
  previously produced a 500 ArrayIndexOutOfBoundsException.
* Shape-check arguments at the Resolver (not just arity) so positional or
  unknown-named arguments surface as a clean 400 before planning, instead
  of silently returning 200 with an empty DSL. Implementation keeps the
  original check as defense-in-depth.

Unit tests cover all three paths. Integration tests assert that the SQL
layer surfaces the three errors as user-facing 400s.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

Author: mengweieric

integ-test/src/test/java/org/opensearch/sql/sql/VectorSearchIT.java

@@ -300,4 +300,55 @@ public void testExplainWithoutKnnPluginStillWorks() throws IOException {
 
     assertThat(explain, containsString("wrapper"));
   }
+
+  // ── Argument shape validation (PR B) ──────────────────────────────────
+
+  @Test
+  public void testInvalidTableNameRejected() throws IOException {
+    // A slash is outside the SAFE_FIELD_NAME regex and is not a valid OpenSearch index character,
+    // so it should be rejected at the SQL layer before any cluster call is attempted.
+    ResponseException ex =
+        expectThrows(
+            ResponseException.class,
+            () ->
+                executeQuery(
+                    "SELECT v._id FROM vectorSearch(table='idx/evil', field='f', "
+                        + "vector='[1.0]', option='k=5') AS v"));
+
+    assertThat(ex.getMessage(), containsString("Invalid table name"));
+  }
+
+  @Test
+  public void testDuplicateNamedArgRejected() throws IOException {
+    // Previously this crashed the server with 500 ArrayIndexOutOfBoundsException. Must now
+    // surface as a clean 400 with a user-facing message.
+    ResponseException ex =
+        expectThrows(
+            ResponseException.class,
+            () ->
+                executeQuery(
+                    "SELECT v._id FROM vectorSearch(table='a', table='b', "
+                        + "vector='[1.0]', option='k=5') AS v"));
+
+    assertThat(ex.getMessage(), containsString("Duplicate argument name"));
+  }
+
+  @Test
+  public void testPositionalArgRejected() throws IOException {
+    // The SQL grammar already requires `ident=value` for each table function argument. That means
+    // the literal case `vectorSearch('idx', ...)` does not even reach this resolver: it fails to
+    // parse, then the whole statement falls back to the legacy engine, which previously returned
+    // 200 with zero rows. Guard that path at the first engine that understands vectorSearch() by
+    // sending a bogus named arg (grammar-legal but positional from the resolver's perspective, in
+    // the sense that it does not map to a known parameter name). The resolver now surfaces a 400.
+    ResponseException ex =
+        expectThrows(
+            ResponseException.class,
+            () ->
+                executeQuery(
+                    "SELECT v._id FROM vectorSearch(bogus='idx', field='f', "
+                        + "vector='[1.0]', option='k=5') AS v"));
+
+    assertThat(ex.getMessage(), containsString("Unknown argument name"));
+  }
 }

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/VectorSearchTableFunctionImplementation.java

@@ -10,6 +10,7 @@
 import static org.opensearch.sql.opensearch.storage.VectorSearchTableFunctionResolver.TABLE;
 import static org.opensearch.sql.opensearch.storage.VectorSearchTableFunctionResolver.VECTOR;
 
+import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -40,7 +41,8 @@ public class VectorSearchTableFunctionImplementation extends FunctionExpression
 
   /**
    * Field names must be safe for JSON interpolation: alphanumeric, dots (nested), underscores,
-   * hyphens. Rejects characters that could corrupt the WrapperQueryBuilder JSON.
+   * hyphens. Rejects characters that could corrupt the WrapperQueryBuilder JSON. The same regex is
+   * reused for table names so user-supplied identifiers cannot break out of the JSON context.
    */
   private static final Pattern SAFE_FIELD_NAME = Pattern.compile("^[a-zA-Z0-9._\\-]+$");
 
@@ -99,6 +101,7 @@ public Table applyArguments() {
     // clusters without k-NN.
     validateNamedArgs();
     String tableName = getArgumentValue(TABLE);
+    validateTableName(tableName);
     String fieldName = getArgumentValue(FIELD);
     validateFieldName(fieldName);
     String vectorLiteral = getArgumentValue(VECTOR);
@@ -162,8 +165,12 @@ static Map<String, String> parseOptions(String optionStr) {
     return options;
   }
 
-  /** Reject non-named arguments and null arg names early. */
+  /**
+   * Reject non-named arguments, null arg names, and duplicate named arguments early. Runs before
+   * any list-index-based lookup so a malformed argument list can never cause an AIOOBE downstream.
+   */
   private void validateNamedArgs() {
+    HashSet<String> seen = new HashSet<>();
     for (Expression arg : arguments) {
       if (!(arg instanceof NamedArgumentExpression)) {
         throw new ExpressionEvaluationException(
@@ -177,6 +184,27 @@ private void validateNamedArgs() {
             "vectorSearch() requires named arguments (e.g., table='index'), "
                 + "but received an argument with no name");
       }
+      if (!seen.add(name.toLowerCase())) {
+        throw new ExpressionEvaluationException(
+            "Duplicate argument name '"
+                + name
+                + "' in vectorSearch(); each named argument may appear at most once");
+      }
+    }
+  }
+
+  /**
+   * Reject table names with characters that could corrupt the WrapperQueryBuilder JSON or escape
+   * the target index name. Allows alphanumeric, dots, underscores, and hyphens (the characters
+   * OpenSearch index names already permit).
+   */
+  private void validateTableName(String tableName) {
+    if (!SAFE_FIELD_NAME.matcher(tableName).matches()) {
+      throw new ExpressionEvaluationException(
+          String.format(
+              "Invalid table name '%s': must contain only alphanumeric characters,"
+                  + " dots, underscores, or hyphens",
+              tableName));
     }
   }
 

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/VectorSearchTableFunctionResolver.java

@@ -7,10 +7,13 @@
 
 import static org.opensearch.sql.data.type.ExprCoreType.STRING;
 
+import java.util.HashSet;
 import java.util.List;
 import org.apache.commons.lang3.tuple.Pair;
 import org.opensearch.sql.common.setting.Settings;
+import org.opensearch.sql.exception.ExpressionEvaluationException;
 import org.opensearch.sql.expression.Expression;
+import org.opensearch.sql.expression.NamedArgumentExpression;
 import org.opensearch.sql.expression.function.FunctionBuilder;
 import org.opensearch.sql.expression.function.FunctionName;
 import org.opensearch.sql.expression.function.FunctionResolver;
@@ -68,5 +71,35 @@ private void validateArguments(List<Expression> arguments) {
               "vectorSearch requires %d arguments (%s), got %d",
               ARGUMENT_NAMES.size(), String.join(", ", ARGUMENT_NAMES), arguments.size()));
     }
+    // Shape check at the resolver so positional or unknown-named args produce a clean 400 before
+    // planning proceeds. The Implementation layer repeats these checks as defense-in-depth.
+    HashSet<String> seen = new HashSet<>();
+    for (Expression arg : arguments) {
+      if (!(arg instanceof NamedArgumentExpression)) {
+        throw new ExpressionEvaluationException(
+            "vectorSearch() requires named arguments (e.g., table='index'), "
+                + "but received: "
+                + arg.getClass().getSimpleName());
+      }
+      String name = ((NamedArgumentExpression) arg).getArgName();
+      if (name == null || name.isEmpty()) {
+        throw new ExpressionEvaluationException(
+            "vectorSearch() requires named arguments (e.g., table='index'), "
+                + "but received an argument with no name");
+      }
+      String lower = name.toLowerCase();
+      if (!ARGUMENT_NAMES.contains(lower)) {
+        throw new ExpressionEvaluationException(
+            String.format(
+                "Unknown argument name '%s' in vectorSearch(); allowed names are %s",
+                name, ARGUMENT_NAMES));
+      }
+      if (!seen.add(lower)) {
+        throw new ExpressionEvaluationException(
+            "Duplicate argument name '"
+                + name
+                + "' in vectorSearch(); each named argument may appear at most once");
+      }
+    }
   }
 }

opensearch/src/test/java/org/opensearch/sql/opensearch/storage/VectorSearchTableFunctionImplementationTest.java

@@ -458,6 +458,37 @@ void testParseOptionsPreservesFilterTypeValue() {
     assertEquals("post", options.get("filter_type"));
   }
 
+  @Test
+  void applyArguments_rejectsInvalidTableName() {
+    VectorSearchTableFunctionImplementation impl =
+        createImplWithArgs("idx\"; DROP", "embedding", "[1.0, 2.0]", "k=5");
+    ExpressionEvaluationException ex =
+        assertThrows(ExpressionEvaluationException.class, () -> impl.applyArguments());
+    assertTrue(ex.getMessage().contains("Invalid table name"));
+    assertTrue(
+        ex.getMessage()
+            .contains("must contain only alphanumeric characters, dots, underscores, or hyphens"));
+  }
+
+  @Test
+  void validateNamedArgs_rejectsDuplicateNames() {
+    // Two occurrences of "table" reach the Implementation layer directly (bypassing the resolver).
+    FunctionName functionName = FunctionName.of("vectorsearch");
+    List<Expression> args =
+        List.of(
+            DSL.namedArgument("table", DSL.literal("a")),
+            DSL.namedArgument("table", DSL.literal("b")),
+            DSL.namedArgument("vector", DSL.literal("[1.0]")),
+            DSL.namedArgument("option", DSL.literal("k=5")));
+    VectorSearchTableFunctionImplementation impl =
+        new VectorSearchTableFunctionImplementation(
+            functionName, args, client, settings, knnCapability);
+    ExpressionEvaluationException ex =
+        assertThrows(ExpressionEvaluationException.class, () -> impl.applyArguments());
+    assertTrue(ex.getMessage().contains("Duplicate argument name"));
+    assertTrue(ex.getMessage().contains("table"));
+  }
+
   private VectorSearchTableFunctionImplementation createImpl() {
     return createImplWithArgs("my-index", "embedding", "[1.0, 2.0, 3.0]", "k=5");
   }

opensearch/src/test/java/org/opensearch/sql/opensearch/storage/VectorSearchTableFunctionResolverTest.java

@@ -18,6 +18,7 @@
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
 import org.opensearch.sql.common.setting.Settings;
+import org.opensearch.sql.exception.ExpressionEvaluationException;
 import org.opensearch.sql.expression.DSL;
 import org.opensearch.sql.expression.Expression;
 import org.opensearch.sql.expression.function.FunctionBuilder;
@@ -127,4 +128,78 @@ void testZeroArguments() {
             IllegalArgumentException.class, () -> builder.apply(functionProperties, expressions));
     assertTrue(ex.getMessage().contains("requires 4 arguments"));
   }
+
+  @Test
+  void resolve_rejectsPositionalArgument() {
+    VectorSearchTableFunctionResolver resolver =
+        new VectorSearchTableFunctionResolver(client, settings);
+    FunctionName functionName = FunctionName.of("vectorsearch");
+    // One positional literal mixed with three named arguments. Arity passes, but the resolver
+    // must reject this before planning so the SQL layer returns a clean 400 rather than a 200
+    // with zero rows.
+    List<Expression> expressions =
+        List.of(
+            DSL.literal("my-index"),
+            DSL.namedArgument("field", DSL.literal("embedding")),
+            DSL.namedArgument("vector", DSL.literal("[1.0, 2.0]")),
+            DSL.namedArgument("option", DSL.literal("k=5")));
+    FunctionSignature functionSignature =
+        new FunctionSignature(
+            functionName, expressions.stream().map(Expression::type).collect(Collectors.toList()));
+    FunctionBuilder builder = resolver.resolve(functionSignature).getValue();
+
+    ExpressionEvaluationException ex =
+        assertThrows(
+            ExpressionEvaluationException.class,
+            () -> builder.apply(functionProperties, expressions));
+    assertTrue(ex.getMessage().contains("requires named arguments"));
+  }
+
+  @Test
+  void resolve_rejectsDuplicateNamedArgument() {
+    VectorSearchTableFunctionResolver resolver =
+        new VectorSearchTableFunctionResolver(client, settings);
+    FunctionName functionName = FunctionName.of("vectorsearch");
+    List<Expression> expressions =
+        List.of(
+            DSL.namedArgument("table", DSL.literal("a")),
+            DSL.namedArgument("table", DSL.literal("b")),
+            DSL.namedArgument("vector", DSL.literal("[1.0]")),
+            DSL.namedArgument("option", DSL.literal("k=5")));
+    FunctionSignature functionSignature =
+        new FunctionSignature(
+            functionName, expressions.stream().map(Expression::type).collect(Collectors.toList()));
+    FunctionBuilder builder = resolver.resolve(functionSignature).getValue();
+
+    ExpressionEvaluationException ex =
+        assertThrows(
+            ExpressionEvaluationException.class,
+            () -> builder.apply(functionProperties, expressions));
+    assertTrue(ex.getMessage().contains("Duplicate argument name"));
+    assertTrue(ex.getMessage().contains("table"));
+  }
+
+  @Test
+  void resolve_rejectsUnknownArgumentName() {
+    VectorSearchTableFunctionResolver resolver =
+        new VectorSearchTableFunctionResolver(client, settings);
+    FunctionName functionName = FunctionName.of("vectorsearch");
+    List<Expression> expressions =
+        List.of(
+            DSL.namedArgument("table", DSL.literal("my-index")),
+            DSL.namedArgument("field", DSL.literal("embedding")),
+            DSL.namedArgument("vector", DSL.literal("[1.0, 2.0]")),
+            DSL.namedArgument("bogus", DSL.literal("k=5")));
+    FunctionSignature functionSignature =
+        new FunctionSignature(
+            functionName, expressions.stream().map(Expression::type).collect(Collectors.toList()));
+    FunctionBuilder builder = resolver.resolve(functionSignature).getValue();
+
+    ExpressionEvaluationException ex =
+        assertThrows(
+            ExpressionEvaluationException.class,
+            () -> builder.apply(functionProperties, expressions));
+    assertTrue(ex.getMessage().contains("Unknown argument name"));
+    assertTrue(ex.getMessage().contains("bogus"));
+  }
 }