Skip to content

Commit bcbbc59

Browse files
Propagate correct HTTP status for security exceptions on SQL analytic… (opensearch-project#5473)
* 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 <carrofin@amazon.com> Signed-off-by: Finn Carroll <carrofin@amazon.com> * Spotless apply Signed-off-by: Finn Carroll <carrofin@amazon.com> * Re-use helper. Signed-off-by: Finn Carroll <carrofin@amazon.com> --------- Signed-off-by: carrofin <carrofin@amazon.com> Signed-off-by: Finn Carroll <carrofin@amazon.com>
1 parent 1183e73 commit bcbbc59

3 files changed

Lines changed: 6 additions & 24 deletions

File tree

integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java

Lines changed: 3 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -263,24 +263,14 @@ public void testSQLQueryAllowedForAuthorizedUser() throws IOException {
263263
}
264264
}
265265

266-
// TODO: The SQL endpoint (/_plugins/_sql) returns 500 instead of 403 for security exceptions.
267-
// The legacy RestSqlAction error handling wraps OpenSearchSecurityException as a generic 500
268-
// Internal Server Error rather than propagating the 403 Forbidden status. The authorization
269-
// IS denied (query does not execute), but the HTTP status is incorrect. These tests accept
270-
// either 403 or 500 until the SQL plugin's error propagation is fixed.
271-
272266
@Test
273267
public void testSQLQueryDeniedForUnauthorizedUser() throws IOException {
274268
ResponseException e =
275269
assertThrows(
276270
ResponseException.class,
277271
() ->
278272
executeSQLAsUser("SELECT name, age FROM " + TEST_INDEX + " LIMIT 3", DENIED_USER));
279-
assertTrue(
280-
"Expected 403 or 500 with security exception, got "
281-
+ e.getResponse().getStatusLine().getStatusCode(),
282-
e.getResponse().getStatusLine().getStatusCode() == 403
283-
|| e.getResponse().getStatusLine().getStatusCode() == 500);
273+
assertEquals(403, e.getResponse().getStatusLine().getStatusCode());
284274
}
285275

286276
@Test
@@ -291,11 +281,7 @@ public void testSQLQueryDeniedForForbiddenIndex() throws IOException {
291281
() ->
292282
executeSQLAsUser(
293283
"SELECT name, age FROM " + FORBIDDEN_INDEX + " LIMIT 3", ALLOWED_USER));
294-
assertTrue(
295-
"Expected 403 or 500 with security exception, got "
296-
+ e.getResponse().getStatusLine().getStatusCode(),
297-
e.getResponse().getStatusLine().getStatusCode() == 403
298-
|| e.getResponse().getStatusLine().getStatusCode() == 500);
284+
assertEquals(403, e.getResponse().getStatusLine().getStatusCode());
299285
}
300286

301287
@Test
@@ -306,11 +292,7 @@ public void testSQLQueryDeniedWithSearchPermissionOnly() throws IOException {
306292
() ->
307293
executeSQLAsUser(
308294
"SELECT name, age FROM " + TEST_INDEX + " LIMIT 3", SEARCH_ONLY_USER));
309-
assertTrue(
310-
"Expected 403 or 500 with security exception, got "
311-
+ e.getResponse().getStatusLine().getStatusCode(),
312-
e.getResponse().getStatusLine().getStatusCode() == 403
313-
|| e.getResponse().getStatusLine().getStatusCode() == 500);
295+
assertEquals(403, e.getResponse().getStatusLine().getStatusCode());
314296
}
315297

316298
/** Executes a PPL query via the production SQL plugin endpoint (/_plugins/_ppl). */

legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,7 @@ private void handleException(RestChannel restChannel, Exception exception) {
210210
reportError(restChannel, exception, status);
211211
}
212212

213-
private static RestStatus getRestStatus(Exception ex) {
213+
public static RestStatus getRestStatus(Exception ex) {
214214
int code = getRawErrorCode(ex);
215215
return RestStatus.fromCode(code);
216216
}

plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,7 @@ protected Object buildJsonObject(ExplainResponse resp) {
264264
@Override
265265
public void onFailure(Exception e) {
266266
channel.sendResponse(
267-
new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, e.getMessage()));
267+
new BytesRestResponse(RestSqlAction.getRestStatus(e), e.getMessage()));
268268
}
269269
});
270270
} else {
@@ -283,7 +283,7 @@ public void onResponse(TransportPPLQueryResponse response) {
283283
@Override
284284
public void onFailure(Exception e) {
285285
channel.sendResponse(
286-
new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, e.getMessage()));
286+
new BytesRestResponse(RestSqlAction.getRestStatus(e), e.getMessage()));
287287
}
288288
});
289289
}

0 commit comments

Comments
 (0)