diff --git a/integ-test/build.gradle b/integ-test/build.gradle index 17b88a901de..1435d1d499d 100644 --- a/integ-test/build.gradle +++ b/integ-test/build.gradle @@ -502,11 +502,11 @@ testClusters.analyticsEngineSecurityIT { plugin(getJobSchedulerPlugin()) plugin(getArrowBasePlugin()) plugin(getArrowFlightRpcPlugin()) + plugin(getCompositeEnginePlugin()) plugin(getAnalyticsEnginePlugin()) plugin(getAnalyticsBackendLucenePlugin()) plugin(getAnalyticsBackendDatafusionPlugin()) plugin(getParquetDataFormatPlugin()) - plugin(getCompositeEnginePlugin()) plugin ":opensearch-sql-plugin" // Arrow Flight / streaming transport requirements jvmArgs '--add-opens=java.base/java.nio=ALL-UNNAMED' diff --git a/integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java b/integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java index 626a7658dab..2b4d346fa0d 100644 --- a/integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java @@ -393,6 +393,94 @@ public void testPPLQueryWithWildcardIndexPartialAccessDenied() throws IOExceptio assertEquals(403, e.getResponse().getStatusLine().getStatusCode()); } + // --- Multi-index comma-separated source tests (FGAC bypass regression) --- + + @Test + public void testPPLMultiIndexDeniedWhenSecondIndexUnauthorized() throws IOException { + ResponseException e = + assertThrows( + ResponseException.class, + () -> + executePPLAsUser( + "source = " + TEST_INDEX + ", " + FORBIDDEN_INDEX + " | fields name, age", + ALLOWED_USER)); + assertEquals(403, e.getResponse().getStatusLine().getStatusCode()); + } + + @Test + public void testPPLMultiIndexDeniedWithBackticksAuthorizedFirst() throws IOException { + ResponseException e = + assertThrows( + ResponseException.class, + () -> + executePPLAsUser( + "source = `" + TEST_INDEX + "`, `" + FORBIDDEN_INDEX + "` | fields name, age", + ALLOWED_USER)); + assertEquals(403, e.getResponse().getStatusLine().getStatusCode()); + } + + @Test + public void testPPLMultiIndexDeniedWithUnauthorizedFirst() throws IOException { + ResponseException e = + assertThrows( + ResponseException.class, + () -> + executePPLAsUser( + "source = " + FORBIDDEN_INDEX + ", " + TEST_INDEX + " | fields name, age", + ALLOWED_USER)); + assertEquals(403, e.getResponse().getStatusLine().getStatusCode()); + } + + @Test + public void testPPLMultiIndexAllowedWhenAllAuthorized() throws IOException { + try { + JSONObject result = + executePPLAsUser( + "source = " + TEST_INDEX + ", " + TEST_INDEX_2 + " | fields name, age", + WILDCARD_USER); + assertTrue("Expected datarows in response", result.has("datarows")); + } catch (ResponseException e) { + assertNotEquals( + "Expected auth to pass (not 403) when all indices are authorized", + 403, + e.getResponse().getStatusLine().getStatusCode()); + } + } + + // --- Edge cases: malformed comma-separated source patterns --- + + @Test + public void testPPLDoubleCommaRejected() throws IOException { + ResponseException e = + assertThrows( + ResponseException.class, + () -> + executePPLAsUser( + "source = " + TEST_INDEX + ",," + FORBIDDEN_INDEX + " | fields name, age", + ALLOWED_USER)); + assertEquals(400, e.getResponse().getStatusLine().getStatusCode()); + } + + @Test + public void testPPLLeadingCommaRejected() throws IOException { + ResponseException e = + assertThrows( + ResponseException.class, + () -> + executePPLAsUser("source = ," + TEST_INDEX + " | fields name, age", ALLOWED_USER)); + assertEquals(400, e.getResponse().getStatusLine().getStatusCode()); + } + + @Test + public void testPPLTrailingCommaRejected() throws IOException { + ResponseException e = + assertThrows( + ResponseException.class, + () -> + executePPLAsUser("source = " + TEST_INDEX + ", | fields name, age", ALLOWED_USER)); + assertEquals(400, e.getResponse().getStatusLine().getStatusCode()); + } + @Test public void testSQLQueryAllowedForAuthorizedUser() throws IOException { try { @@ -441,6 +529,51 @@ public void testSQLQueryDeniedWithSearchPermissionOnly() throws IOException { assertEquals(403, e.getResponse().getStatusLine().getStatusCode()); } + // --- SQL multi-index syntax validation --- + + @Test + public void testSQLMultiIndexCommaInFromRejected() throws IOException { + // SQL FROM "idx1,idx2" — comma inside identifier, should be rejected as syntax error + ResponseException e = + assertThrows( + ResponseException.class, + () -> + executeSQLAsUser( + "SELECT name, age FROM " + TEST_INDEX + "," + FORBIDDEN_INDEX + " LIMIT 3", + ALLOWED_USER)); + assertEquals(400, e.getResponse().getStatusLine().getStatusCode()); + } + + @Test + public void testSQLMultiIndexCrossJoinRejected() throws IOException { + // SQL cross join syntax — should not bypass FGAC + ResponseException e = + assertThrows( + ResponseException.class, + () -> + executeSQLAsUser( + "SELECT a.name FROM " + TEST_INDEX + " a, " + FORBIDDEN_INDEX + " b LIMIT 3", + ALLOWED_USER)); + assertEquals(400, e.getResponse().getStatusLine().getStatusCode()); + } + + @Test + public void testSQLMultiIndexJoinRejected() throws IOException { + // Explicit JOIN — should not bypass FGAC + ResponseException e = + assertThrows( + ResponseException.class, + () -> + executeSQLAsUser( + "SELECT a.name FROM " + + TEST_INDEX + + " a JOIN " + + FORBIDDEN_INDEX + + " b ON a.name = b.name LIMIT 3", + ALLOWED_USER)); + assertEquals(400, e.getResponse().getStatusLine().getStatusCode()); + } + /** Executes a PPL query via the production SQL plugin endpoint (/_plugins/_ppl). */ private JSONObject executePPLAsUser(String query, String username) throws IOException { Request request = new Request("POST", "/_plugins/_ppl"); diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java index 84ee147d34a..2e810033a9f 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java @@ -122,13 +122,35 @@ public boolean isAnalyticsIndex(String query, QueryType queryType) { try (UnifiedQueryContext context = buildParsingContext(queryType)) { return extractIndexName(query, queryType, context) .map(this::stripSchemaPrefix) - .map(this::isPluggableDataformatIndex) + .map(this::allIndicesArePluggableDataformat) .orElse(false); } catch (Exception e) { return false; } } + /** + * Checks if all indices in a (possibly comma-separated) index expression are pluggable + * dataformat. For multi-index queries (source=idx1,idx2), each index is checked independently. + * Returns true only if every index is composite — mixed or all-lucene returns false. + */ + private boolean allIndicesArePluggableDataformat(String indexExpression) { + String[] indices = indexExpression.split(","); + if (indices.length == 0) { + return false; + } + for (String idx : indices) { + String trimmed = idx.trim(); + if (trimmed.isEmpty()) { + continue; + } + if (!isPluggableDataformatIndex(trimmed)) { + return false; + } + } + return true; + } + private static boolean isSystemCatalog(String name) { return SystemIndexUtils.isSystemIndex(name) || SystemIndexUtils.DATASOURCES_TABLE_NAME.equals(name);