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

Testing

All 40 tests pass on a local cluster with analytics engine plugins built from current main.

[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 🔍

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 assertForbidden() fallback checks for "no permissions" when expectedAction is not found in the error body. If the security plugin returns a 403 with a message that contains neither the expected action string nor "no permissions", the assertion passes incorrectly. This occurs when expectedAction is a specific action like "indices:data/read/analytics/query" but the actual error message uses different wording (e.g., "access denied", "unauthorized"). The two-argument overload at line 759 always passes "no permissions" as expectedAction, making this scenario likely. 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 = 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) {

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

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