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 407cf1a)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 407cf1a

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add bounds check before array access The method accesses row.getString(nameIdx) without verifying that the row array has sufficient elements. If a row has fewer columns than expected, this will throw an IndexOutOfBoundsException. Add a bounds check before accessing the array element to prevent unexpected test failures. integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java [922-943] 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]: 8 __ Why: The suggestion identifies a potential IndexOutOfBoundsException when accessing row.getString(nameIdx) without verifying the row has sufficient elements. Adding a bounds check (row.length() > nameIdx) prevents unexpected test failures and makes the code more robust against malformed responses.	Medium
	Preserve exception context in assertion The assertForbidden method swallows the IOException and converts it to a test failure with fail(). This loses the original exception context and stack trace. Consider rethrowing the exception or using assertThrows to preserve the full error information for debugging. integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java [946-964] -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]: 7 __ Why: The suggestion correctly identifies that catching IOException and converting it to a test failure with fail() loses stack trace information. Propagating the exception improves debugging by preserving the full error context. However, this requires updating all call sites to handle the checked exception.	Medium

---

Previous suggestions

Suggestions up to commit 407cf1a

Category	Suggestion	Impact
Possible issue	Fix overly permissive assertion logic The assertion logic body.contains(expectedAction) || body.contains("no permissions") is too permissive. When expectedAction is "no permissions" (via the overloaded method), the condition always passes regardless of actual body content. This could mask authorization bypass bugs where the error message doesn't contain the expected action. integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java [946-964] private void assertForbidden(ResponseException e, String context, String expectedAction) { 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")); + body.contains(expectedAction)); } catch (IOException ioe) { fail("Could not read response body for " + context + ": " + ioe.getMessage()); } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the || condition makes the assertion always pass when expectedAction is "no permissions", potentially masking bugs. However, the fix may be too strict since some error messages might legitimately use "no permissions" instead of the specific action name. The issue is valid but the solution needs refinement to handle both cases appropriately.	Medium

Suggestions up to commit 6307be5

Category	Suggestion	Impact
General	Preserve exception stack trace The assertForbidden method silently catches IOException and calls fail(), which may hide the root cause. Consider rethrowing the exception or logging it before failing to preserve the stack trace for debugging. integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java [946-964] private void assertForbidden(ResponseException e, String context, String expectedAction) { 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()); + throw new AssertionError("Could not read response body for " + context, ioe); } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that catching IOException and calling fail() loses the stack trace. Using AssertionError with the cause preserves debugging information, which is valuable for test failures. However, this is a test utility method improvement rather than a critical bug fix.	Medium

Suggestions up to commit 6307be5

Category	Suggestion	Impact
General	Preserve exception stack trace The assertForbidden method silently catches IOException and calls fail(), which may hide the root cause of response body read failures. Consider rethrowing the exception or logging it before failing to preserve the full stack trace for debugging. integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java [946-964] private void assertForbidden(ResponseException e, String context, String expectedAction) { 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()); + throw new AssertionError("Could not read response body for " + context, ioe); } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that catching IOException and calling fail() loses the stack trace. Using AssertionError with the cause preserves debugging information, which is valuable for test diagnostics.	Medium
	Add array bounds validation The method assumes datarows contains arrays but doesn't validate array bounds before accessing row.getString(nameIdx). If a row has fewer columns than expected, this will throw an IndexOutOfBoundsException. Add bounds checking to prevent test failures from malformed responses. integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java [922-943] 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]: 6 __ Why: Valid defensive programming suggestion. Adding bounds checking (row.length() > nameIdx) prevents potential IndexOutOfBoundsException if the response contains malformed rows with fewer columns than expected, improving test robustness.	Low

Suggestions up to commit 4bceb37

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

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

[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

[dai-chen]

Minor comment. Thanks for the changes!

[github-actions]

Persistent review updated to latest commit 6307be5

[github-actions]

Persistent review updated to latest commit 6307be5

[dai-chen]

Retrying failed AE CI which seems failed transiently.

[dai-chen]

@finnegancarroll The AE CI is failing on error below consistently. It seems unrelated to this PR but do we have plan to fix?

> A failure occurred while executing de.undercouch.gradle.tasks.download.internal.DefaultWorkerExecutorHelper$DefaultWorkAction
   > de.undercouch.gradle.tasks.download.org.apache.hc.client5.http.ClientProtocolException: Forbidden (HTTP status code: 403, URL: https://ci.opensearch.org/ci/dbc/feature-build-opensearch/feature-datafusion/latest/linux/x64/tar/builds/opensearch/plugins/1-analytics-engine-3.9.0.zip)

[github-actions]

Persistent review updated to latest commit 407cf1a

[finnegancarroll]

@finnegancarroll The AE CI is failing on error below consistently. It seems unrelated to this PR but do we have plan to fix?

It looks like it's failing to pull the 3.9 AE artifacts published by core. I see core is on 3.9 so maybe bad timing and just needs another retry.

[github-actions]

Persistent review updated to latest commit 407cf1a