Update RequestContentValidator to only validate fields from request payload and not pre-existing values stored in security index

[cwperks]

Description

This PR contains a fix for using PATCH API with the security plugin to update security configs like internal users.

This PR relates to the following 3 PRs:

• Create a WildcardMatcher.NONE when creating a WildcardMatcher with an empty string #5694
• Improve array validator to also check for blank string in addition to null #5714
• Filter blank backend role before creating internal user security-dashboards-plugin#2330

Prior to opensearch-project/security-dashboards-plugin#2330, when internal users would be created through OSD then they would automatically get empty string ("") pre-populated in the list of backend roles.

With the new API validations added in #5714, a user making a PATCH request can get a BAD_REQUEST error if the internaluser being edited has an empty string already stored in the security index regardless if the request payload is modifying this field.

For example in my internalusers.yml file I put:

anomalyadmin:
  hash: "$2y$12$TRwAAJgnNo67w3rVUz4FIeLx9Dy/llB79zf9I15CKJ9vkM4ZzAd3."
  reserved: false
  backend_roles:
    - ""

and then try to update the hash via API using PATCH. It fails:

> curl -XPATCH -k "https://admin:myStrongPassword123\!@localhost:9200/_opendistro/_security/api/internalusers" \
  -H "Content-Type: application/json" \
  --data '[
    {
      "op": "replace",
      "path": "/anomalyadmin/hash",
      "value": "myStrongPassword123!"
    }
  ]'
{"status":"error","reason":"`null` or blank values are not allowed as json array elements"}

With the change in this PR, the request would succeed bc the validation would only apply on the request payload itself.

• Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

Bugfix

Check List

• New functionality includes testing
• New functionality has been documented
• New Roles/Permissions have a corresponding security dashboards plugin PR
• API changes companion pull request created
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[codecov]

Codecov Report

❌ Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.04%. Comparing base (db9efb6) to head (52eb6ca).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
.../dlic/rest/validation/RequestContentValidator.java	90.47%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6061      +/-   ##
==========================================
+ Coverage   73.98%   74.04%   +0.05%     
==========================================
  Files         441      441              
  Lines       27413    27481      +68     
  Branches     4110     4123      +13     
==========================================
+ Hits        20281    20347      +66     
  Misses       5193     5193              
- Partials     1939     1941       +2     

Files with missing lines	Coverage Δ
.../dlic/rest/validation/RequestContentValidator.java	86.50% <90.47%> (+0.22%)	⬆️

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 3b4a597.

Path	Line	Severity	Description
src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java	392	medium	The null/blank array element validation is now skipped for array fields not modified by a PATCH operation. Since backend_roles controls access permissions, this means existing documents with empty-string backend_roles can be updated (e.g., hash rotated) without ever triggering re-validation of those roles. An attacker who could write a document with crafted backend_roles through a legacy path or race condition could then freely PATCH other fields without the invalid roles being caught. The intent is backward compatibility, and new document creation still validates fully, but the security model is weakened for existing documents.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 1 | Low: 0

---

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 Dual Diff Libraries The patch-aware nullValuesInArrayValidator overload (called from validate(request, patchedContent, originalContent)) uses Jackson3JsonDiff.asJson, while the single-argument nullValuesInArrayValidator (called when originalContent field is set via withOriginalContent) uses JsonDiff.asJson. These are two different diff libraries. Inconsistent diff results between the two code paths could cause subtle bugs where the same logical operation is validated differently depending on which path is taken. final JsonNode patch = JsonDiff.asJson(originalContent, jsonContent); return nullValuesInArrayValidator(jsonContent, patch); } Mutable State The originalContent field is set via withOriginalContent as mutable instance state on the validator. Since RequestContentValidator instances may be reused or shared, this could lead to unintended cross-request contamination if the same validator instance is used for multiple requests. The withOriginalContent method returns this, making it a fluent builder, but the validator is not clearly documented as single-use. private JsonNode originalContent; /** * Sets the original content for patch-aware validation. * When set, the null/blank array element check will only validate fields that changed. */ public RequestContentValidator withOriginalContent(final JsonNode originalContent) { this.originalContent = originalContent; return this; } Incomplete Validation The patch-aware nullValuesInArrayValidator(jsonContent, patch) only iterates over validationContext.allowedKeys() but not validationContext.allowedKeysWithConfig(). The single-argument version uses allowedKeysWithConfig() when available. This inconsistency means that fields defined only via allowedKeysWithConfig() will not be validated for blank array elements in the patch-aware path. private ValidationResult<JsonNode> nullValuesInArrayValidator(final JsonNode jsonContent, final JsonNode patch) { final Set<String> changedFields = new HashSet<>(); for (final JsonNode op : patch) { final String path = op.get("path").asText(); // path is like "/fieldName" or "/entityName/fieldName" — extract the top-level field final String[] parts = path.split("/"); if (parts.length >= 2) { changedFields.add(parts[1]); } } for (final Map.Entry<String, DataType> allowedKey : validationContext.allowedKeys().entrySet()) { if (!changedFields.contains(allowedKey.getKey())) { continue; } final DataType dataType = allowedKey.getValue(); if (dataType != DataType.ARRAY) { continue; } JsonNode value = jsonContent.get(allowedKey.getKey()); if (value != null) { if (hasNullOrBlankArrayElement(value)) { this.validationError = ValidationError.NULL_ARRAY_ELEMENT; return ValidationResult.error(RestStatus.BAD_REQUEST, this); } } } return ValidationResult.success(jsonContent); } Path Parsing The JSON Patch path parsing uses path.split("/") and takes parts[1] as the top-level field name. For a path like /fieldName, parts[0] is an empty string and parts[1] is the field name — this works. However, if a path is malformed or unexpectedly structured (e.g., empty path or just /), parts.length >= 2 may still hold but parts[1] could be an empty string, silently adding an empty key to changedFields. A null check on op.get("path") is also missing. for (final JsonNode op : patch) { final String path = op.get("path").asText(); // path is like "/fieldName" or "/entityName/fieldName" — extract the top-level field final String[] parts = path.split("/"); if (parts.length >= 2) { changedFields.add(parts[1]); } }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Guard against null path in patch operations The op.get("path") call can return null if a patch operation node doesn't have a "path" field, causing a NullPointerException when .asText() is called. Add a null check before calling .asText() on the path node. src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java [420-429] private ValidationResult<JsonNode> nullValuesInArrayValidator(final JsonNode jsonContent, final JsonNode patch) { final Set<String> changedFields = new HashSet<>(); for (final JsonNode op : patch) { - final String path = op.get("path").asText(); + final JsonNode pathNode = op.get("path"); + if (pathNode == null) { + continue; + } + final String path = pathNode.asText(); // path is like "/fieldName" or "/entityName/fieldName" — extract the top-level field final String[] parts = path.split("/"); if (parts.length >= 2) { changedFields.add(parts[1]); } } Suggestion importance[1-10]: 6 __ Why: The op.get("path") call could return null for malformed patch operations, causing a NullPointerException. Adding a null check is a valid defensive measure, though JSON patch operations from JsonDiff.asJson are well-formed and always include a path field in practice.	Low
General	Align patch-aware validation with full field config sources The patch-aware overload of nullValuesInArrayValidator only iterates over validationContext.allowedKeys(), but the original single-argument version also considers allowedKeysWithConfig(). This inconsistency means that fields defined only via allowedKeysWithConfig() (with DataType.ARRAY) will never be validated in patch mode, potentially allowing blank array elements to slip through. The patch-aware overload should mirror the same field-source logic as the original. src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java [420-447] private ValidationResult<JsonNode> nullValuesInArrayValidator(final JsonNode jsonContent, final JsonNode patch) { - ... - for (final Map.Entry<String, DataType> allowedKey : validationContext.allowedKeys().entrySet()) { + final Set<String> changedFields = new HashSet<>(); + for (final JsonNode op : patch) { + final JsonNode pathNode = op.get("path"); + if (pathNode == null) continue; + final String path = pathNode.asText(); + final String[] parts = path.split("/"); + if (parts.length >= 2) { + changedFields.add(parts[1]); + } + } + final Map<String, FieldConfiguration> fieldConfigs = validationContext.allowedKeysWithConfig(); + final Map<String, DataType> keysToCheck; + if (fieldConfigs != null) { + keysToCheck = fieldConfigs.entrySet().stream() + .collect(java.util.stream.Collectors.toMap(Map.Entry::getKey, e -> e.getValue().dataType())); + } else { + keysToCheck = validationContext.allowedKeys(); + } + for (final Map.Entry<String, DataType> allowedKey : keysToCheck.entrySet()) { if (!changedFields.contains(allowedKey.getKey())) { continue; } - final DataType dataType = allowedKey.getValue(); - if (dataType != DataType.ARRAY) { + if (allowedKey.getValue() != DataType.ARRAY) { continue; } JsonNode value = jsonContent.get(allowedKey.getKey()); - if (value != null) { - if (hasNullOrBlankArrayElement(value)) { - this.validationError = ValidationError.NULL_ARRAY_ELEMENT; - return ValidationResult.error(RestStatus.BAD_REQUEST, this); - } + if (value != null && hasNullOrBlankArrayElement(value)) { + this.validationError = ValidationError.NULL_ARRAY_ELEMENT; + return ValidationResult.error(RestStatus.BAD_REQUEST, this); } } return ValidationResult.success(jsonContent); } Suggestion importance[1-10]: 5 __ Why: The patch-aware nullValuesInArrayValidator overload only uses allowedKeys() while the original single-argument version also considers allowedKeysWithConfig(). This inconsistency could allow blank array elements in fields defined via allowedKeysWithConfig() to bypass validation in patch mode. However, the existing_code snippet contains ... which doesn't match the actual PR code exactly, slightly reducing confidence.	Low
	Handle empty patch case explicitly in validator When originalContent is set and JsonDiff.asJson produces an empty patch (no actual changes), the method delegates to the patch-aware overload which will find no changed fields and skip all validation. However, the caller in validate(RestRequest, JsonNode, JsonNode) already guards against no-op patches. In the withOriginalContent path (used from patchEntities), a no-op case could silently pass without any validation. Consider checking if the patch is empty and returning early or falling through to full validation to avoid skipping all array checks unintentionally. src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java [394-398] private ValidationResult<JsonNode> nullValuesInArrayValidator(final JsonNode jsonContent) { if (originalContent != null) { final JsonNode patch = JsonDiff.asJson(originalContent, jsonContent); + if (patch.isEmpty()) { + // No changes — nothing to validate + return ValidationResult.success(jsonContent); + } return nullValuesInArrayValidator(jsonContent, patch); } Suggestion importance[1-10]: 3 __ Why: When the patch is empty (no changes), the patch-aware overload finds no changed fields and skips all validation, which is the correct behavior. The suggestion adds an explicit early return for the empty patch case, but the behavior is already equivalent since no fields would be in changedFields and all validation would be skipped anyway. The improvement is minimal.	Low