From 6d1eaf335b5d4db128a87087b128a96a30978956 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 14 Oct 2025 16:02:38 -0400 Subject: [PATCH 1/4] Improve array validator to also check for blank string in addition to null Signed-off-by: Craig Perkins --- .../dlic/rest/validation/RequestContentValidator.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java b/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java index 097b953690..f3056250df 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java +++ b/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java @@ -238,7 +238,7 @@ private ValidationResult nullValuesInArrayValidator(final JsonNode jso for (final Map.Entry allowedKey : validationContext.allowedKeys().entrySet()) { JsonNode value = jsonContent.get(allowedKey.getKey()); if (value != null) { - if (hasNullArrayElement(value)) { + if (hasNullOrBlankArrayElement(value)) { this.validationError = ValidationError.NULL_ARRAY_ELEMENT; return ValidationResult.error(RestStatus.BAD_REQUEST, this); } @@ -247,14 +247,14 @@ private ValidationResult nullValuesInArrayValidator(final JsonNode jso return ValidationResult.success(jsonContent); } - private boolean hasNullArrayElement(final JsonNode node) { + private boolean hasNullOrBlankArrayElement(final JsonNode node) { for (final JsonNode element : node) { - if (element.isNull()) { + if (element.isNull() || (element.isTextual() && Strings.isNullOrEmpty(element.asText().trim()))) { if (node.isArray()) { return true; } } else if (element.isContainerNode()) { - if (hasNullArrayElement(element)) { + if (hasNullOrBlankArrayElement(element)) { return true; } } @@ -380,7 +380,7 @@ public enum ValidationError { PAYLOAD_NOT_ALLOWED("Request body not allowed for this action."), PAYLOAD_MANDATORY("Request body required for this action."), SECURITY_NOT_INITIALIZED("Security index not initialized"), - NULL_ARRAY_ELEMENT("`null` is not allowed as json array element"); + NULL_ARRAY_ELEMENT("`null` or blank values are not allowed as json array elements"); private final String message; From ddd7a7b55c776b294fbfe839bcd661b7c3b9d122 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 14 Oct 2025 16:33:24 -0400 Subject: [PATCH 2/4] Write tests to verify blank string cannot be included in array Signed-off-by: Craig Perkins --- .../api/AbstractConfigEntityApiIntegrationTest.java | 6 +++++- .../security/api/InternalUsersRestApiIntegrationTest.java | 6 ++++++ .../security/api/RolesMappingRestApiIntegrationTest.java | 1 + 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/integrationTest/java/org/opensearch/security/api/AbstractConfigEntityApiIntegrationTest.java b/src/integrationTest/java/org/opensearch/security/api/AbstractConfigEntityApiIntegrationTest.java index d25ae508c8..a6d6902359 100644 --- a/src/integrationTest/java/org/opensearch/security/api/AbstractConfigEntityApiIntegrationTest.java +++ b/src/integrationTest/java/org/opensearch/security/api/AbstractConfigEntityApiIntegrationTest.java @@ -199,7 +199,11 @@ void creationOfReadOnlyEntityForbidden(final String entityName, final TestRestCl } void assertNullValuesInArray(final TestRestClient.HttpResponse response) throws Exception { - assertThat(response.getBody(), response.getTextFromJsonBody("/reason"), equalTo("`null` is not allowed as json array element")); + assertThat( + response.getBody(), + response.getTextFromJsonBody("/reason"), + equalTo("`null` or blank values are not allowed as json array elements") + ); } void assertInvalidKeys(final TestRestClient.HttpResponse response, final String expectedInvalidKeys) { diff --git a/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java b/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java index 18769949fe..a87121297f 100644 --- a/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java +++ b/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java @@ -254,6 +254,12 @@ void invalidJson(final TestRestClient client, final String predefinedUserName) t (builder, params) -> builder.startObject().field("backend_roles", configJsonArray(generateArrayValues(true))).endObject() ) ); + assertNullValuesInArray( + client.putJson( + apiPath(randomAsciiAlphanumOfLength(10)), + (builder, params) -> builder.startObject().field("backend_roles", configJsonArray("a", "")).endObject() + ) + ); // patch badRequest(() -> client.patch(apiPath(), patch(addOp(randomAsciiAlphanumOfLength(10), EMPTY_BODY)))); badRequest( diff --git a/src/integrationTest/java/org/opensearch/security/api/RolesMappingRestApiIntegrationTest.java b/src/integrationTest/java/org/opensearch/security/api/RolesMappingRestApiIntegrationTest.java index ac5e4b21d4..7255007271 100644 --- a/src/integrationTest/java/org/opensearch/security/api/RolesMappingRestApiIntegrationTest.java +++ b/src/integrationTest/java/org/opensearch/security/api/RolesMappingRestApiIntegrationTest.java @@ -270,6 +270,7 @@ void verifyCrudOperations(Boolean hidden, Boolean reserved, TestRestClient clien ok(() -> client.patch(apiPath(roleName), patch(addOp("users", configJsonArray("g", "h"))))); ok(() -> client.patch(apiPath(roleName), patch(addOp("and_backend_roles", configJsonArray("i", "j"))))); ok(() -> client.patch(apiPath(roleName), patch(addOp("and_backend_roles", configJsonArray("i", "j")))), "No updates required"); + badRequest(() -> client.patch(apiPath(roleName), patch(replaceOp("backend_roles", configJsonArray("c", ""))))); ok(() -> client.patch(apiPath(), patch(removeOp(roleName)))); notFound(() -> client.get(apiPath(roleName))); From c970ad8f82b09f9d4fc3885daac3d8f22474e0ec Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 14 Oct 2025 16:35:03 -0400 Subject: [PATCH 3/4] Add to CHANGELOG Signed-off-by: Craig Perkins --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a9b740bc8c..2db9c8eaac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Bug Fixes - Create a WildcardMatcher.NONE when creating a WildcardMatcher with an empty string ([#5694](https://github.com/opensearch-project/security/pull/5694)) +- Improve array validator to also check for blank string in addition to null ([#5714](https://github.com/opensearch-project/security/pull/5714)) ### Refactoring From 7808832ac8728e07faa98bb20e475a462e4a7631 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 14 Oct 2025 20:08:06 -0400 Subject: [PATCH 4/4] Fix audit api Signed-off-by: Craig Perkins --- .../dlic/rest/api/AuditApiActionTest.java | 46 +++++++++++++------ 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/AuditApiActionTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/AuditApiActionTest.java index ffc7b4a22d..39f1918f76 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/AuditApiActionTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/AuditApiActionTest.java @@ -686,18 +686,38 @@ public void testPatchRequest() throws Exception { } private String getTestPayload() { - return "{" - + "\"enabled\":true," - + "\"audit\":{" - + "\"enable_rest\":true,\"disabled_rest_categories\":[\"AUTHENTICATED\"]," - + "\"enable_transport\":true,\"disabled_transport_categories\":[\"SSL_EXCEPTION\"]," - + "\"resolve_bulk_requests\":true,\"log_request_body\":true,\"resolve_indices\":true,\"exclude_sensitive_headers\":true," - + "\"ignore_users\":[\"test-user-1\"],\"ignore_requests\":[\"test-request\"], \"ignore_headers\":[\"\"], \"ignore_url_params\":[]}," - + "\"compliance\":{" - + "\"enabled\":true," - + "\"internal_config\":true,\"external_config\":true," - + "\"read_metadata_only\":true,\"read_watched_fields\":{\"test-read-watch-field\":[]},\"read_ignore_users\":[\"test-user-2\"]," - + "\"write_metadata_only\":true,\"write_log_diffs\":true,\"write_watched_indices\":[\"test-write-watch-index\"],\"write_ignore_users\":[\"test-user-3\"]}" - + "}"; + return """ + { + "enabled": true, + "audit": { + "enable_rest": true, + "disabled_rest_categories": ["AUTHENTICATED"], + "enable_transport": true, + "disabled_transport_categories": ["SSL_EXCEPTION"], + "resolve_bulk_requests": true, + "log_request_body": true, + "resolve_indices": true, + "exclude_sensitive_headers": true, + "ignore_users": ["test-user-1"], + "ignore_requests": ["test-request"], + "ignore_headers": [], + "ignore_url_params": [] + }, + "compliance": { + "enabled": true, + "internal_config": true, + "external_config": true, + "read_metadata_only": true, + "read_watched_fields": { + "test-read-watch-field": [] + }, + "read_ignore_users": ["test-user-2"], + "write_metadata_only": true, + "write_log_diffs": true, + "write_watched_indices": ["test-write-watch-index"], + "write_ignore_users": ["test-user-3"] + } + } + """; } }