Strengthen AnalyticsEngineSecurityIT coverage and fix routing

[finnegancarroll]

Strengthen AnalyticsEngineSecurityIT coverage and fix routing

Routing fix for analytics engine security plugin tests

The SQL plugin routes queries to the analytics engine based on isAnalyticsIndex(), which checks whether the target index uses the composite data format. Previously, this check only worked for concrete index names — it performs a direct Metadata.index(name) lookup which does not resolve wildcards, aliases, or multi-index expressions. Queries using source = analytics_* or source = my_alias would silently fall back to the legacy PPL backend because the lookup returned null for non-concrete names.

This meant wildcard, alias, and some multi-index security tests were passing but testing the legacy PPL path's security behavior, not the analytics engine's. The concrete-index tests (e.g., testPPLQueryDeniedWithSearchPermissionOnly) correctly validated analytics engine routing, but the wildcard/alias ALLOW tests masked the routing gap with try/catch fallbacks.

The fix adds cluster.pluggable.dataformat=composite (plus cluster.composite.secondary_data_formats=["lucene"]) to the testClusters.analyticsEngineSecurityIT configuration. This activates the cluster-level fast path in isAnalyticsIndex(), ensuring ALL queries route through the analytics engine regardless of source format.

To prevent future silent fallback regressions, we add three routing guard tests that use profile=true. The analytics engine returns a "profile" key containing engine-specific execution details (stage timings, physical plans, DataFusion metrics) that are distinct from the legacy PPL profile output. If a query silently falls back to the legacy path, the response structure will differ and these tests will fail:

Test	What it guards
testRoutingGuardConcreteIndex	Concrete index name routing
testRoutingGuardWildcardSource	Wildcard source routing
testRoutingGuardAlias	Alias source routing

Test assertion enhancements

Removed try/catch(ResponseException) fallbacks that previously masked non-403 errors (including 500s) as passing tests. ALLOW tests now assert HTTP 200, validate non-empty datarows, and check actual row content (e.g., assertContainsName("alice")). DENY tests assert 403 status AND validate that the error body references the denied action or contains "no permissions". Also validates that 403 responses do not leak internal stack traces or node IDs.

New test coverage

Test	Description
testRoutingGuardConcreteIndex	Proves analytics engine handles concrete index queries (profile key present)
testRoutingGuardWildcardSource	Proves analytics engine handles wildcard source queries
testRoutingGuardAlias	Proves analytics engine handles alias queries
testPPLCommaSourceAllAuthorized	Comma-separated source, user authorized on all indices
testPPLCommaSourcePartiallyAuthorized	Comma-separated source with one forbidden index → deny
testPPLMultiIndexDeniedWithBackticksAuthorizedFirst	Backtick-quoted index names must not bypass FGAC
testPPLMultiIndexDeniedWithUnauthorizedFirst	Unauthorized index first in comma list must still deny
testPPLQueryAllowedViaConcreteIndexForAliasUser	Alias grant implies concrete index access
testPPLQueryDeniedWithoutClusterPermission	Missing cluster:admin/opensearch/ppl → 403
testPPLQueryMultiBackingAliasAllowed	Multi-backing alias, user covers all backing indices
testPPLQueryMultiBackingAliasDenied	Multi-backing alias, user has no access
testPPLQueryMultiBackingAliasPartialAccessDenied	Multi-backing alias, user covers only some backing indices
testPPLQueryWithWildcardSourceAllowed	Wildcard source expanding to authorized indices only
testPPLQueryWithWildcardSourceDenied	Wildcard source, user unauthorized on all matching indices
testPPLQueryWithWildcardSourcePartialAccessDenied	Wildcard expands to include forbidden index → deny
testSQLQueryAllowedWithWildcardPermission	SQL parity: wildcard role permission matches concrete index
testSQLQueryDeniedWithWildcardPermissionOnNonMatchingIndex	SQL parity: wildcard role does not match forbidden index
testSQLQueryAllowedViaAlias	SQL parity: alias as table name
testSQLQueryWithExactAnalyticsPermission	SQL parity: exact analytics/query permission is sufficient
testDeniedResponseContainsActionName	403 body references the denied action for user actionability
testDeniedResponseDoesNotLeakInternalDetails	403 body contains no stack traces or internal node info

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 6ea9a6a.

Path	Line	Severity	Description
integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java	295	medium	Removed `testPPLMultiIndexDeniedWithBackticksAuthorizedFirst`, which was an explicit FGAC bypass regression test for backtick-quoted index names. The old comment called it out as a 'FGAC bypass regression' vector. No replacement test covers this specific syntax variant, leaving a known bypass path untested.
integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java	295	medium	Removed `testPPLMultiIndexDeniedWithUnauthorizedFirst`, which tested that placing the unauthorized index first in a comma-separated source does not bypass FGAC. Ordering-dependent authorization bypasses are a known attack surface; the replacement test only covers the case where the authorized index comes first.
integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java	268	medium	Removed `testPPLQueryAllowedViaConcreteIndexForAliasUser`, which verified that alias-based grants also permit access to the underlying concrete index. This specific test documented the security model's alias-to-concrete-index grant semantics; its removal leaves that authorization path without dedicated test coverage.
integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java	340	low	Removed parser-validation tests (`testPPLDoubleCommaRejected`, `testPPLLeadingCommaRejected`, `testPPLTrailingCommaRejected`, `testSQLMultiIndexCommaInFromRejected`, `testSQLMultiIndexCrossJoinRejected`, `testSQLMultiIndexJoinRejected`) that verified malformed multi-index syntax returns HTTP 400. These guarded against parser edge cases that could be abused to craft inputs the authorization layer did not anticipate.

The table above displays the top 10 most important findings.

Total: 4 | Critical: 0 | High: 0 | Medium: 3 | Low: 1

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 4bceb37)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue In executePPLAsUser and executeSQLAsUser, the response body is read into a variable body that is not declared. The code calls TestUtils.getResponseBody(response, true) and assigns it to body, but body is never defined in scope. This will cause a compilation error. The same issue occurs in executePPLWithProfileAsUser. return new JSONObject(body); Possible Issue In assertForbidden, the code attempts to read the response body with TestUtils.getResponseBody(e.getResponse(), true) inside a try-catch. However, if the response body cannot be read (e.g., already consumed), the catch block calls fail() which will cause the test to fail even if the 403 status was correct. This makes tests fragile to response body reading issues unrelated to the actual authorization behavior being tested. String body = TestUtils.getResponseBody(e.getResponse(), true); assertTrue( "Expected error body to reference '" + expectedAction + "' for " + context + ", got: " + body, body.contains(expectedAction) || body.contains("no permissions")); } catch (IOException ioe) { fail("Could not read response body for " + context + ": " + ioe.getMessage()); } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 4bceb37

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Preserve exception context in error handling The assertForbidden method silently catches IOException and calls fail(), which may hide the root cause if response body reading fails. Consider rethrowing the exception or logging it before failing to preserve the full error context for debugging. integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java [891-909] private void assertForbidden(ResponseException e, String context, String expectedAction) { assertEquals( "Expected 403 for " + context + ", got " + e.getResponse().getStatusLine().getStatusCode(), 403, e.getResponse().getStatusLine().getStatusCode()); + String body; try { - String body = TestUtils.getResponseBody(e.getResponse(), true); - assertTrue( - "Expected error body to reference '" - + expectedAction - + "' for " - + context - + ", got: " - + body, - body.contains(expectedAction) || body.contains("no permissions")); + body = TestUtils.getResponseBody(e.getResponse(), true); } catch (IOException ioe) { - fail("Could not read response body for " + context + ": " + ioe.getMessage()); + throw new AssertionError("Could not read response body for " + context, ioe); } + assertTrue( + "Expected error body to reference '" + + expectedAction + + "' for " + + context + + ", got: " + + body, + body.contains(expectedAction) || body.contains("no permissions")); } Suggestion importance[1-10]: 6 __ Why: The suggestion improves error handling by preserving the full exception context when IOException occurs during response body reading. Using AssertionError with the original exception as cause is better than fail() with just a message, as it maintains the stack trace for debugging. However, this is a test code improvement with moderate impact.	Low

---

Previous suggestions

Suggestions up to commit 4bceb37

Category	Suggestion	Impact
General	Add bounds check before array access The method assumes datarows contains arrays and directly calls getString(nameIdx) without bounds checking. If a row has fewer columns than expected, this will throw an IndexOutOfBoundsException. Add bounds validation before accessing the column index. integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java [867-888] private void assertContainsName(JSONObject result, String expectedName) { JSONArray schema = result.getJSONArray("schema"); int nameIdx = -1; for (int i = 0; i < schema.length(); i++) { if ("name".equals(schema.getJSONObject(i).getString("name"))) { nameIdx = i; break; } } assertTrue("Expected 'name' column in schema", nameIdx >= 0); JSONArray rows = result.getJSONArray("datarows"); boolean found = false; for (int i = 0; i < rows.length(); i++) { JSONArray row = rows.getJSONArray(i); - if (expectedName.equals(row.getString(nameIdx))) { + if (row.length() > nameIdx && expectedName.equals(row.getString(nameIdx))) { found = true; break; } } assertTrue("Expected to find name='" + expectedName + "' in datarows", found); } Suggestion importance[1-10]: 7 __ Why: Valid suggestion that prevents potential IndexOutOfBoundsException if a row has fewer columns than expected. The bounds check row.length() > nameIdx is a defensive programming practice that improves test robustness, though the likelihood of malformed data in this controlled test environment is low.	Medium
	Avoid swallowing IOException in assertion The catch block swallows the IOException and converts it to a test failure, which may hide the actual cause. Consider rethrowing the IOException or using assertThrows to make the test failure more transparent and debuggable. integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java [891-909] -private void assertForbidden(ResponseException e, String context, String expectedAction) { +private void assertForbidden(ResponseException e, String context, String expectedAction) throws IOException { assertEquals( "Expected 403 for " + context + ", got " + e.getResponse().getStatusLine().getStatusCode(), 403, e.getResponse().getStatusLine().getStatusCode()); - try { - String body = TestUtils.getResponseBody(e.getResponse(), true); - assertTrue( - "Expected error body to reference '" - + expectedAction - + "' for " - + context - + ", got: " - + body, - body.contains(expectedAction) || body.contains("no permissions")); - } catch (IOException ioe) { - fail("Could not read response body for " + context + ": " + ioe.getMessage()); - } + String body = TestUtils.getResponseBody(e.getResponse(), true); + assertTrue( + "Expected error body to reference '" + + expectedAction + + "' for " + + context + + ", got: " + + body, + body.contains(expectedAction) || body.contains("no permissions")); } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that swallowing IOException in the catch block reduces debuggability. However, the impact is moderate since this is test code and the fail() call does provide context about the failure. Rethrowing would make debugging slightly easier.	Low

Suggestions up to commit e3ddd7e

Category	Suggestion	Impact
General	Preserve exception context in assertion helper The assertForbidden method swallows the IOException and converts it to a test failure, which loses the original exception context. Consider rethrowing the IOException or wrapping it in an AssertionError to preserve the full stack trace for debugging. integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java [872-890] -private void assertForbidden(ResponseException e, String context, String expectedAction) { +private void assertForbidden(ResponseException e, String context, String expectedAction) throws IOException { assertEquals( "Expected 403 for " + context + ", got " + e.getResponse().getStatusLine().getStatusCode(), 403, e.getResponse().getStatusLine().getStatusCode()); - try { - String body = org.opensearch.sql.legacy.TestUtils.getResponseBody(e.getResponse(), true); - assertTrue( - "Expected error body to reference '" - + expectedAction - + "' for " - + context - + ", got: " - + body, - body.contains(expectedAction) || body.contains("no permissions")); - } catch (IOException ioe) { - fail("Could not read response body for " + context + ": " + ioe.getMessage()); - } + String body = org.opensearch.sql.legacy.TestUtils.getResponseBody(e.getResponse(), true); + assertTrue( + "Expected error body to reference '" + + expectedAction + + "' for " + + context + + ", got: " + + body, + body.contains(expectedAction) || body.contains("no permissions")); } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that catching IOException and converting it to a test failure loses stack trace context. However, the impact is moderate since test failures will still be reported with the context message, and the IOException scenario (failure to read response body) is relatively rare in practice.	Low
	Validate schema structure before access The method assumes the schema contains a name field in each object but doesn't handle potential JSONException if the structure is unexpected. Add validation or handle the exception to prevent test crashes from malformed responses. integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java [848-857] private void assertContainsName(JSONObject result, String expectedName) { + assertTrue("Expected 'schema' field in response", result.has("schema")); JSONArray schema = result.getJSONArray("schema"); int nameIdx = -1; for (int i = 0; i < schema.length(); i++) { - if ("name".equals(schema.getJSONObject(i).getString("name"))) { + JSONObject col = schema.getJSONObject(i); + if (col.has("name") && "name".equals(col.getString("name"))) { nameIdx = i; break; } } assertTrue("Expected 'name' column in schema", nameIdx >= 0); Suggestion importance[1-10]: 5 __ Why: The suggestion adds defensive validation to prevent JSONException from malformed responses. While this improves robustness, the existing code already has implicit validation through assertTrue on nameIdx >= 0, and the test environment is controlled. The added result.has("schema") check and col.has("name") check provide marginal improvement in error clarity.	Low

Suggestions up to commit 6ea9a6a

Category	Suggestion	Impact
General	Propagate IOException instead of catching The assertForbidden method swallows IOException and converts it to a test failure, which may hide the root cause. Consider letting the IOException propagate by declaring it in the method signature, allowing the test framework to handle it properly. integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java [737-755] -private void assertForbidden(ResponseException e, String context, String expectedAction) { +private void assertForbidden(ResponseException e, String context, String expectedAction) throws IOException { assertEquals( "Expected 403 for " + context + ", got " + e.getResponse().getStatusLine().getStatusCode(), 403, e.getResponse().getStatusLine().getStatusCode()); - try { - String body = org.opensearch.sql.legacy.TestUtils.getResponseBody(e.getResponse(), true); - assertTrue( - "Expected error body to reference '" - + expectedAction - + "' for " - + context - + ", got: " - + body, - body.contains(expectedAction) || body.contains("no permissions")); - } catch (IOException ioe) { - fail("Could not read response body for " + context + ": " + ioe.getMessage()); - } + String body = org.opensearch.sql.legacy.TestUtils.getResponseBody(e.getResponse(), true); + assertTrue( + "Expected error body to reference '" + + expectedAction + + "' for " + + context + + ", got: " + + body, + body.contains(expectedAction) || body.contains("no permissions")); } Suggestion importance[1-10]: 6 __ Why: Propagating IOException instead of catching it improves error visibility and aligns with test framework conventions. However, the current approach of converting to test failure is also acceptable in test code, making this a moderate improvement rather than a critical fix.	Low
	Add guard after schema assertion The assertion nameIdx >= 0 will fail silently if the schema doesn't contain a name column, but the subsequent array access row.getString(nameIdx) could throw an exception with -1. Add an early return or throw after the assertion to prevent potential index errors. integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java [713-723] private void assertContainsName(JSONObject result, String expectedName) { JSONArray schema = result.getJSONArray("schema"); int nameIdx = -1; for (int i = 0; i < schema.length(); i++) { if ("name".equals(schema.getJSONObject(i).getString("name"))) { nameIdx = i; break; } } assertTrue("Expected 'name' column in schema", nameIdx >= 0); + if (nameIdx < 0) { + return; + } Suggestion importance[1-10]: 2 __ Why: The suggestion is technically incorrect. The assertTrue assertion will fail the test immediately if nameIdx < 0, preventing any subsequent code execution. Adding an early return after a failed assertion is redundant since the test framework stops execution at assertion failures.	Low

[github-actions]

Persistent review updated to latest commit e3ddd7e

[github-actions]

Persistent review updated to latest commit 4bceb37

[github-actions]

Persistent review updated to latest commit 4bceb37