From 815d3c5463345a5b1888ab995d586db04109717e Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Tue, 26 May 2026 18:00:42 -0700 Subject: [PATCH 1/3] Propagate correct HTTP status for security exceptions on SQL analytics path The SQL plugin's unified query handler (/_plugins/_sql) returned HTTP 500 for all exceptions from the analytics engine path, including security exceptions that should be 403 Forbidden. This caused authorization denials to appear as internal server errors to users. Fix: Check if the exception is an OpenSearchException and extract its proper HTTP status (e.g., 403 for OpenSearchSecurityException) instead of hardcoding 500. Both the explain and execute paths are fixed. Also updates AnalyticsEngineSecurityIT SQL deny tests to assert 403 directly, removing the previous workaround that accepted either 403 or 500. Signed-off-by: carrofin Signed-off-by: Finn Carroll --- .../security/AnalyticsEngineSecurityIT.java | 24 +++---------------- .../org/opensearch/sql/plugin/SQLPlugin.java | 12 ++++++---- 2 files changed, 11 insertions(+), 25 deletions(-) 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 4de5aa208b..a9e36e9b47 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 @@ -263,12 +263,6 @@ public void testSQLQueryAllowedForAuthorizedUser() throws IOException { } } - // TODO: The SQL endpoint (/_plugins/_sql) returns 500 instead of 403 for security exceptions. - // The legacy RestSqlAction error handling wraps OpenSearchSecurityException as a generic 500 - // Internal Server Error rather than propagating the 403 Forbidden status. The authorization - // IS denied (query does not execute), but the HTTP status is incorrect. These tests accept - // either 403 or 500 until the SQL plugin's error propagation is fixed. - @Test public void testSQLQueryDeniedForUnauthorizedUser() throws IOException { ResponseException e = @@ -276,11 +270,7 @@ public void testSQLQueryDeniedForUnauthorizedUser() throws IOException { ResponseException.class, () -> executeSQLAsUser("SELECT name, age FROM " + TEST_INDEX + " LIMIT 3", DENIED_USER)); - assertTrue( - "Expected 403 or 500 with security exception, got " - + e.getResponse().getStatusLine().getStatusCode(), - e.getResponse().getStatusLine().getStatusCode() == 403 - || e.getResponse().getStatusLine().getStatusCode() == 500); + assertEquals(403, e.getResponse().getStatusLine().getStatusCode()); } @Test @@ -291,11 +281,7 @@ public void testSQLQueryDeniedForForbiddenIndex() throws IOException { () -> executeSQLAsUser( "SELECT name, age FROM " + FORBIDDEN_INDEX + " LIMIT 3", ALLOWED_USER)); - assertTrue( - "Expected 403 or 500 with security exception, got " - + e.getResponse().getStatusLine().getStatusCode(), - e.getResponse().getStatusLine().getStatusCode() == 403 - || e.getResponse().getStatusLine().getStatusCode() == 500); + assertEquals(403, e.getResponse().getStatusLine().getStatusCode()); } @Test @@ -306,11 +292,7 @@ public void testSQLQueryDeniedWithSearchPermissionOnly() throws IOException { () -> executeSQLAsUser( "SELECT name, age FROM " + TEST_INDEX + " LIMIT 3", SEARCH_ONLY_USER)); - assertTrue( - "Expected 403 or 500 with security exception, got " - + e.getResponse().getStatusLine().getStatusCode(), - e.getResponse().getStatusLine().getStatusCode() == 403 - || e.getResponse().getStatusLine().getStatusCode() == 500); + assertEquals(403, e.getResponse().getStatusLine().getStatusCode()); } /** Executes a PPL query via the production SQL plugin endpoint (/_plugins/_ppl). */ diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java b/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java index 3692f49688..46bd98dcee 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java @@ -263,8 +263,10 @@ protected Object buildJsonObject(ExplainResponse resp) { @Override public void onFailure(Exception e) { - channel.sendResponse( - new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, e.getMessage())); + RestStatus status = (e instanceof org.opensearch.OpenSearchException osEx) + ? osEx.status() + : RestStatus.INTERNAL_SERVER_ERROR; + channel.sendResponse(new BytesRestResponse(status, e.getMessage())); } }); } else { @@ -282,8 +284,10 @@ public void onResponse(TransportPPLQueryResponse response) { @Override public void onFailure(Exception e) { - channel.sendResponse( - new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, e.getMessage())); + RestStatus status = (e instanceof org.opensearch.OpenSearchException osEx) + ? osEx.status() + : RestStatus.INTERNAL_SERVER_ERROR; + channel.sendResponse(new BytesRestResponse(status, e.getMessage())); } }); } From efa65e15fb4775dd7edd1b4afc141b02a7efafb4 Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Tue, 26 May 2026 18:29:31 -0700 Subject: [PATCH 2/3] Spotless apply Signed-off-by: Finn Carroll --- .../java/org/opensearch/sql/plugin/SQLPlugin.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java b/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java index 46bd98dcee..fa63f39f9e 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java @@ -263,9 +263,10 @@ protected Object buildJsonObject(ExplainResponse resp) { @Override public void onFailure(Exception e) { - RestStatus status = (e instanceof org.opensearch.OpenSearchException osEx) - ? osEx.status() - : RestStatus.INTERNAL_SERVER_ERROR; + RestStatus status = + (e instanceof org.opensearch.OpenSearchException osEx) + ? osEx.status() + : RestStatus.INTERNAL_SERVER_ERROR; channel.sendResponse(new BytesRestResponse(status, e.getMessage())); } }); @@ -284,9 +285,10 @@ public void onResponse(TransportPPLQueryResponse response) { @Override public void onFailure(Exception e) { - RestStatus status = (e instanceof org.opensearch.OpenSearchException osEx) - ? osEx.status() - : RestStatus.INTERNAL_SERVER_ERROR; + RestStatus status = + (e instanceof org.opensearch.OpenSearchException osEx) + ? osEx.status() + : RestStatus.INTERNAL_SERVER_ERROR; channel.sendResponse(new BytesRestResponse(status, e.getMessage())); } }); From 43d3bcb86291e08b3d1d410adb67e7dca1eb32b6 Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Wed, 27 May 2026 10:44:53 -0700 Subject: [PATCH 3/3] Re-use helper. Signed-off-by: Finn Carroll --- .../sql/legacy/plugin/RestSqlAction.java | 2 +- .../java/org/opensearch/sql/plugin/SQLPlugin.java | 14 ++++---------- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java index 2950f6c9b8..bf3ebad7d7 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java @@ -210,7 +210,7 @@ private void handleException(RestChannel restChannel, Exception exception) { reportError(restChannel, exception, status); } - private static RestStatus getRestStatus(Exception ex) { + public static RestStatus getRestStatus(Exception ex) { int code = getRawErrorCode(ex); return RestStatus.fromCode(code); } diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java b/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java index fa63f39f9e..57095c9679 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java @@ -263,11 +263,8 @@ protected Object buildJsonObject(ExplainResponse resp) { @Override public void onFailure(Exception e) { - RestStatus status = - (e instanceof org.opensearch.OpenSearchException osEx) - ? osEx.status() - : RestStatus.INTERNAL_SERVER_ERROR; - channel.sendResponse(new BytesRestResponse(status, e.getMessage())); + channel.sendResponse( + new BytesRestResponse(RestSqlAction.getRestStatus(e), e.getMessage())); } }); } else { @@ -285,11 +282,8 @@ public void onResponse(TransportPPLQueryResponse response) { @Override public void onFailure(Exception e) { - RestStatus status = - (e instanceof org.opensearch.OpenSearchException osEx) - ? osEx.status() - : RestStatus.INTERNAL_SERVER_ERROR; - channel.sendResponse(new BytesRestResponse(status, e.getMessage())); + channel.sendResponse( + new BytesRestResponse(RestSqlAction.getRestStatus(e), e.getMessage())); } }); }