[Bugfix] Remove X-Request-Id format restrictions and make size configurable

[finnegancarroll]

Description

Introduces dynamic http.request_id.max_length setting for configuring maximum length for X-Request-Id headers. Removes the alpha-numeric validation of X-Request-Id.

Related Issues

Resolves #20688

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

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.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 16c1fae.

Path	Line	Severity	Description
server/src/main/java/org/opensearch/common/util/RequestUtils.java	30	medium	The refactored validateRequestId removes all character-level validation. The previous implementation enforced exactly 32 hex characters (0-9, a-f, A-F), preventing injection of control characters. The new implementation only checks for blank input and max length, allowing arbitrary characters including CRLF sequences, null bytes, and other control characters. If the X-Request-Id value is written to log files or echoed in HTTP responses without additional sanitization, this could enable log injection or HTTP response-splitting attacks. The change appears to be an intentional relaxation for supporting diverse request ID formats (UUID, AWS trace IDs, etc.), but no allowlist or control-character rejection was added as a replacement safety control.

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 🔍

(Review updated until commit a06d346)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Add configurable max length setting and update validation logic Relevant files: server/src/main/java/org/opensearch/http/HttpTransportSettings.java server/src/main/java/org/opensearch/common/settings/ClusterSettings.java server/src/main/java/org/opensearch/common/util/RequestUtils.java server/src/test/java/org/opensearch/common/util/RequestUtilsTests.java Sub-PR theme: Wire max length setting into RestController and add integration tests Relevant files: server/src/main/java/org/opensearch/rest/RestController.java server/src/main/java/org/opensearch/action/ActionModule.java modules/transport-netty4/src/javaRestTest/java/org/opensearch/rest/Netty4RequestIdIT.java

⚡ Recommended focus areas for review Thread Safety The requestIdMaxLength field is declared volatile, which ensures visibility but not atomicity for compound operations. However, since it is only read and written independently (no compound check-then-act), volatile should be sufficient. Still, the setter setRequestIdMaxLength is public with no validation — there is no guard against setting an invalid value (e.g., negative or zero), unlike the intSetting bounds enforced at the settings level. If called directly in tests or other contexts, it could bypass the min/max constraints defined in HttpTransportSettings. public void setRequestIdMaxLength(int maxLength) { this.requestIdMaxLength = maxLength; } Test Cleanup The integration test testRequestIdAfterSettingUpdate modifies a transient cluster setting (http.request_id.max_length) but does not restore it after the test. This could cause test pollution if other tests in the same suite rely on the default value. Consider resetting the setting to its default after the test or using a try/finally block. public void testRequestIdAfterSettingUpdate() throws IOException { int newMax = 20; // Expect request is valid under default assertThat(requestWithId("a".repeat(128)).getStatusLine().getStatusCode(), equalTo(200)); // Update setting Request updateSettings = new Request("PUT", "/_cluster/settings"); updateSettings.setJsonEntity("{\"transient\": {\"http.request_id.max_length\": " + newMax + "}}"); client().performRequest(updateSettings); // Was valid under default, now too long ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(129))); assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(400)); assertThat(e.getMessage(), containsString("exceeds maximum allowed length [" + newMax + "]")); // ID at new size passes assertThat(requestWithId("a".repeat(newMax)).getStatusLine().getStatusCode(), equalTo(200)); } Exception Handling The validateRequestId call inside tryAllHandlers can throw an IllegalArgumentException, but there is no explicit catch block shown for this exception in the visible diff. It should be verified that this exception is properly caught and translated into a 400 Bad Request response rather than propagating as an unhandled exception. RequestUtils.validateRequestId(distinctHeaderValues.getFirst(), requestIdMaxLength);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to a06d346

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Restore cluster setting after test The test testRequestIdAfterSettingUpdate changes a transient cluster setting but never restores it, which can cause test pollution for other tests running after it. Add a tearDown or finally block to reset http.request_id.max_length back to its default (or null) after the test completes. modules/transport-netty4/src/javaRestTest/java/org/opensearch/rest/Netty4RequestIdIT.java [42-60] public void testRequestIdAfterSettingUpdate() throws IOException { int newMax = 20; // Expect request is valid under default assertThat(requestWithId("a".repeat(128)).getStatusLine().getStatusCode(), equalTo(200)); // Update setting Request updateSettings = new Request("PUT", "/_cluster/settings"); updateSettings.setJsonEntity("{\"transient\": {\"http.request_id.max_length\": " + newMax + "}}"); client().performRequest(updateSettings); - // Was valid under default, now too long - ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(129))); - ... + try { + // Was valid under default, now too long + ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(129))); + assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(400)); + assertThat(e.getMessage(), containsString("exceeds maximum allowed length [" + newMax + "]")); + + // ID at new size passes + assertThat(requestWithId("a".repeat(newMax)).getStatusLine().getStatusCode(), equalTo(200)); + } finally { + // Restore default setting + Request restoreSettings = new Request("PUT", "/_cluster/settings"); + restoreSettings.setJsonEntity("{\"transient\": {\"http.request_id.max_length\": null}}"); + client().performRequest(restoreSettings); + } } Suggestion importance[1-10]: 7 __ Why: The test modifies a transient cluster setting (http.request_id.max_length) without restoring it, which can cause test pollution for other tests. Wrapping the test body in a try/finally block to reset the setting is a valid and important improvement for test isolation.	Medium
General	Validate input in setter method The setRequestIdMaxLength method does not validate the input value, so a caller could set a non-positive or otherwise invalid max length, bypassing the constraints defined in the setting. Add a guard to ensure the provided value is positive (or within the valid range defined by the setting). server/src/main/java/org/opensearch/rest/RestController.java [152-154] public void setRequestIdMaxLength(int maxLength) { + if (maxLength <= 0) { + throw new IllegalArgumentException("requestIdMaxLength must be positive, got: " + maxLength); + } this.requestIdMaxLength = maxLength; } Suggestion importance[1-10]: 3 __ Why: The setRequestIdMaxLength setter is only called from ActionModule with values sourced from SETTING_HTTP_REQUEST_ID_MAX_LENGTH, which already enforces a minimum of 16 and maximum of 1024. Adding validation here is redundant given the setting's built-in constraints, making this a low-impact suggestion.	Low

---

Previous suggestions

Suggestions up to commit c6abb99

Category	Suggestion	Impact
Possible issue	Restore cluster setting after test The integration test modifies a transient cluster setting but never restores it, which can cause test pollution for other tests running in the same cluster. Add a finally block or use a try/finally pattern to reset http.request_id.max_length back to its default (or null) after the test completes. modules/transport-netty4/src/javaRestTest/java/org/opensearch/rest/Netty4RequestIdIT.java [42-56] public void testRequestIdAfterSettingUpdate() throws IOException { int newMax = 20; Request updateSettings = new Request("PUT", "/_cluster/settings"); updateSettings.setJsonEntity("{\"transient\": {\"http.request_id.max_length\": " + newMax + "}}"); client().performRequest(updateSettings); - // Was valid under default, now too long - ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(newMax + 1))); - assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(400)); - assertThat(e.getMessage(), containsString("exceeds maximum allowed length [" + newMax + "]")); + try { + // Was valid under default, now too long + ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(newMax + 1))); + assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(400)); + assertThat(e.getMessage(), containsString("exceeds maximum allowed length [" + newMax + "]")); - // Exactly at new max — should pass - assertThat(requestWithId("a".repeat(newMax)).getStatusLine().getStatusCode(), equalTo(200)); + // Exactly at new max — should pass + assertThat(requestWithId("a".repeat(newMax)).getStatusLine().getStatusCode(), equalTo(200)); + } finally { + Request resetSettings = new Request("PUT", "/_cluster/settings"); + resetSettings.setJsonEntity("{\"transient\": {\"http.request_id.max_length\": null}}"); + client().performRequest(resetSettings); + } } Suggestion importance[1-10]: 7 __ Why: The test modifies a transient cluster setting http.request_id.max_length without restoring it, which can cause test pollution. Adding a finally block to reset the setting is a valid and important improvement for test isolation.	Medium
General	Initialize configured value via constructor The requestIdMaxLength field is initialized with the setting's default value, but the actual configured value from settings is only applied later via setRequestIdMaxLength in ActionModule. If RestController is used before setRequestIdMaxLength is called, it will use the default rather than the configured value. Consider accepting the initial value through the constructor to ensure the correct value is used from the start. server/src/main/java/org/opensearch/rest/RestController.java [103] -private volatile int requestIdMaxLength = HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.getDefault(Settings.EMPTY); +private volatile int requestIdMaxLength; Suggestion importance[1-10]: 4 __ Why: The concern about using the default value before setRequestIdMaxLength is called is valid, but the improved_code only removes the initialization without showing how to pass the value through the constructor, making it incomplete. The default value initialization is also a reasonable fallback since the setting default matches.	Low
	Add guard for invalid maxLength parameter When requestId is blank (e.g., " "), requestId.length() could exceed maxLength, but the blank check runs first and throws a different message. However, a blank string of length > maxLength would still be caught by the blank check, which is correct. The more subtle issue is that maxLength could be 0 or negative if misconfigured, bypassing the length check entirely. Since the setting already enforces a minimum of 1, this is low risk, but it's worth adding a guard or assertion for maxLength > 0 to make the contract explicit. server/src/main/java/org/opensearch/common/util/RequestUtils.java [34-41] if (requestId == null || requestId.isBlank()) { throw new IllegalArgumentException("X-Request-Id should not be null or empty"); } +assert maxLength > 0 : "maxLength must be positive"; if (requestId.length() > maxLength) { throw new IllegalArgumentException( "X-Request-Id length [" + requestId.length() + "] exceeds maximum allowed length [" + maxLength + "]" ); } Suggestion importance[1-10]: 2 __ Why: The setting already enforces a minimum of 1 via intSetting with a minimum bound, making this assertion redundant. The suggestion adds marginal defensive value but is low impact given the existing constraint.	Low

Suggestions up to commit 16c1fae

Category	Suggestion	Impact
Possible issue	Restore cluster settings after test The test testRequestIdAfterSettingUpdate changes a transient cluster setting but never restores it, which can cause test pollution and affect other tests running after it. Add a finally block or a teardown step to reset the setting back to its default value after the test. modules/transport-netty4/src/javaRestTest/java/org/opensearch/rest/Netty4RequestIdIT.java [71-85] public void testRequestIdAfterSettingUpdate() throws IOException { int newMax = 20; Request updateSettings = new Request("PUT", "/_cluster/settings"); updateSettings.setJsonEntity("{\"transient\": {\"http.request_id.max_length\": " + newMax + "}}"); client().performRequest(updateSettings); - // Was valid under default, now too long - ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(newMax + 1))); - assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(400)); - assertThat(e.getMessage(), containsString("exceeds maximum allowed length [" + newMax + "]")); + try { + // Was valid under default, now too long + ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(newMax + 1))); + assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(400)); + assertThat(e.getMessage(), containsString("exceeds maximum allowed length [" + newMax + "]")); - // Exactly at new max — should pass - assertThat(requestWithId("a".repeat(newMax)).getStatusLine().getStatusCode(), equalTo(200)); + // Exactly at new max — should pass + assertThat(requestWithId("a".repeat(newMax)).getStatusLine().getStatusCode(), equalTo(200)); + } finally { + Request resetSettings = new Request("PUT", "/_cluster/settings"); + resetSettings.setJsonEntity("{\"transient\": {\"http.request_id.max_length\": null}}"); + client().performRequest(resetSettings); + } } Suggestion importance[1-10]: 6 __ Why: The test modifies a transient cluster setting http.request_id.max_length without restoring it, which can cause test pollution if other tests run in the same cluster context. Adding a finally block to reset the setting is a valid and important improvement for test isolation.	Low
General	Validate input in the setter method The setRequestIdMaxLength method does not validate the input value, so it could be called with a non-positive or otherwise invalid value. Since the setting itself enforces a minimum of 1 and maximum of 1024, the setter should also guard against invalid values to prevent inconsistent state if called directly (e.g., in tests). server/src/main/java/org/opensearch/rest/RestController.java [152-154] public void setRequestIdMaxLength(int maxLength) { + if (maxLength < 1) { + throw new IllegalArgumentException("requestIdMaxLength must be at least 1, got: " + maxLength); + } this.requestIdMaxLength = maxLength; } Suggestion importance[1-10]: 3 __ Why: While adding validation to setRequestIdMaxLength is a defensive practice, the method is only called via the settings framework which already enforces the min/max bounds (1 to 1024). The risk of invalid values being passed is low in practice, making this a minor improvement.	Low

Suggestions up to commit 16c1fae

Category	Suggestion	Impact
General	Restore cluster setting after test The test modifies a transient cluster setting but never restores it, which can cause test pollution for other tests running after this one. Add a finally block (or use a @After teardown) to reset http.request_id.max_length back to its default value after the test completes. modules/transport-netty4/src/javaRestTest/java/org/opensearch/rest/Netty4RequestIdIT.java [71-85] public void testRequestIdAfterSettingUpdate() throws IOException { int newMax = 20; Request updateSettings = new Request("PUT", "/_cluster/settings"); updateSettings.setJsonEntity("{\"transient\": {\"http.request_id.max_length\": " + newMax + "}}"); client().performRequest(updateSettings); - // Was valid under default, now too long - ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(newMax + 1))); - assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(400)); - assertThat(e.getMessage(), containsString("exceeds maximum allowed length [" + newMax + "]")); + try { + // Was valid under default, now too long + ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(newMax + 1))); + assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(400)); + assertThat(e.getMessage(), containsString("exceeds maximum allowed length [" + newMax + "]")); - // Exactly at new max — should pass - assertThat(requestWithId("a".repeat(newMax)).getStatusLine().getStatusCode(), equalTo(200)); + // Exactly at new max — should pass + assertThat(requestWithId("a".repeat(newMax)).getStatusLine().getStatusCode(), equalTo(200)); + } finally { + Request resetSettings = new Request("PUT", "/_cluster/settings"); + resetSettings.setJsonEntity("{\"transient\": {\"http.request_id.max_length\": null}}"); + client().performRequest(resetSettings); + } } Suggestion importance[1-10]: 6 __ Why: The test modifies a transient cluster setting http.request_id.max_length without restoring it, which could cause test pollution. Adding a finally block to reset the setting is a valid improvement for test isolation.	Low
Possible issue	Validate input before updating max length The setRequestIdMaxLength method does not validate the input value, so it could be set to zero or a negative number, bypassing the minimum enforced by the setting definition. Add a guard to ensure the provided value is at least 1 before assigning it. server/src/main/java/org/opensearch/rest/RestController.java [152-154] public void setRequestIdMaxLength(int maxLength) { + if (maxLength < 1) { + throw new IllegalArgumentException("requestIdMaxLength must be at least 1, got: " + maxLength); + } this.requestIdMaxLength = maxLength; } Suggestion importance[1-10]: 3 __ Why: While the suggestion is logically sound, the setRequestIdMaxLength method is only called via clusterSettings.addSettingsUpdateConsumer and the setting definition already enforces a minimum of 1 via intSetting(..., 1, 1024, ...). Adding a redundant guard here provides minimal practical benefit.	Low

Suggestions up to commit 85e4753

Category	Suggestion	Impact
General	Restore cluster settings after test to prevent pollution The test modifies a transient cluster setting but never restores it after the test completes. This can cause test pollution and affect other tests that rely on the default http.request_id.max_length value. Add a finally block or a teardown step to reset the setting back to its default. modules/transport-netty4/src/javaRestTest/java/org/opensearch/rest/Netty4RequestIdIT.java [71-85] public void testRequestIdAfterSettingUpdate() throws IOException { int newMax = 20; Request updateSettings = new Request("PUT", "/_cluster/settings"); updateSettings.setJsonEntity("{\"transient\": {\"http.request_id.max_length\": " + newMax + "}}"); client().performRequest(updateSettings); - // Was valid under default, now too long - ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(newMax + 1))); - assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(400)); - assertThat(e.getMessage(), containsString("exceeds maximum allowed length [" + newMax + "]")); + try { + // Was valid under default, now too long + ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(newMax + 1))); + assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(400)); + assertThat(e.getMessage(), containsString("exceeds maximum allowed length [" + newMax + "]")); - // Exactly at new max — should pass - assertThat(requestWithId("a".repeat(newMax)).getStatusLine().getStatusCode(), equalTo(200)); + // Exactly at new max — should pass + assertThat(requestWithId("a".repeat(newMax)).getStatusLine().getStatusCode(), equalTo(200)); + } finally { + Request resetSettings = new Request("PUT", "/_cluster/settings"); + resetSettings.setJsonEntity("{\"transient\": {\"http.request_id.max_length\": null}}"); + client().performRequest(resetSettings); + } } Suggestion importance[1-10]: 6 __ Why: The test modifies a transient cluster setting http.request_id.max_length without restoring it, which could affect other tests in the same test run. Adding a finally block to reset the setting is a valid improvement for test isolation.	Low
	Initialize setting atomically in constructor to avoid race condition The requestIdMaxLength is initialized via setRequestIdMaxLength after construction, but RestController already initializes the field with the default value from Settings.EMPTY. If settings contains a non-default value, the explicit setRequestIdMaxLength call is needed — however, the constructor should ideally accept the settings directly to avoid a window where the wrong value is used. Alternatively, pass settings to the RestController constructor to ensure the correct value is set atomically. server/src/main/java/org/opensearch/action/ActionModule.java [616-621] -restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService); -restController.setRequestIdMaxLength(HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.get(settings)); +restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService, + HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.get(settings)); clusterSettings.addSettingsUpdateConsumer( HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH, restController::setRequestIdMaxLength ); Suggestion importance[1-10]: 2 __ Why: The suggested race condition window is negligible since RestController is constructed synchronously during node startup before any requests are processed. The improved_code also requires changing the RestController constructor signature which is a larger refactor not reflected in the PR diff.	Low
Possible issue	Ensure validation errors return proper HTTP 400 status The validation is called after distinctHeaderValues.getFirst() is retrieved, but the exception thrown is an IllegalArgumentException which may not be properly caught and converted to a 400 HTTP response. Ensure the exception is wrapped or handled so it results in a proper BAD_REQUEST response to the client rather than a 500 error. server/src/main/java/org/opensearch/rest/RestController.java [155-164] private void validateRequestId(String requestId) { if (requestId == null || requestId.isBlank()) { - throw new IllegalArgumentException("X-Request-Id should not be null or empty"); + throw new OpenSearchException("X-Request-Id should not be null or empty") { + @Override + public RestStatus status() { return RestStatus.BAD_REQUEST; } + }; } if (requestId.length() > requestIdMaxLength) { - throw new IllegalArgumentException( + throw new OpenSearchException( "X-Request-Id length [" + requestId.length() + "] exceeds maximum allowed length [" + requestIdMaxLength + "]" - ); + ) { + @Override + public RestStatus status() { return RestStatus.BAD_REQUEST; } + }; } } Suggestion importance[1-10]: 5 __ Why: The concern about IllegalArgumentException not being converted to a 400 response is valid in principle, but the integration tests in Netty4RequestIdIT.java already verify that 400 status codes are returned, suggesting the existing exception handling infrastructure already handles this correctly. The suggestion may be addressing a non-issue.	Low

Suggestions up to commit cdc7a69

Category	Suggestion	Impact
General	Pass initial setting value through constructor The RestController is constructed with a default value for requestIdMaxLength and then immediately overwritten via setRequestIdMaxLength. This two-step initialization creates a brief window where the controller has the wrong value and adds unnecessary complexity. Consider passing the setting value directly to the constructor to make initialization atomic and cleaner. server/src/main/java/org/opensearch/action/ActionModule.java [616-621] -restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService); -restController.setRequestIdMaxLength(HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.get(settings)); +int requestIdMaxLength = HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.get(settings); +restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService, requestIdMaxLength); clusterSettings.addSettingsUpdateConsumer( HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH, restController::setRequestIdMaxLength ); Suggestion importance[1-10]: 4 __ Why: The suggestion to pass the value via constructor is cleaner, but the "brief window" concern is negligible since RestController is not yet exposed to requests at construction time. It's a minor style/design improvement rather than a correctness fix.	Low
	Validate setter input to prevent invalid state The setRequestIdMaxLength method does not validate the input value, so a caller could set a non-positive or unreasonably small max length. Since the setting itself enforces a minimum of 1, the setter should mirror that constraint to prevent inconsistent state if called directly. server/src/main/java/org/opensearch/rest/RestController.java [152-154] public void setRequestIdMaxLength(int maxLength) { + if (maxLength < 1) { + throw new IllegalArgumentException("requestIdMaxLength must be at least 1, got: " + maxLength); + } this.requestIdMaxLength = maxLength; } Suggestion importance[1-10]: 3 __ Why: The setting already enforces a minimum of 1 via intSetting(..., 1, ...), so invalid values cannot reach setRequestIdMaxLength through the normal settings path. Adding a guard here is defensive but low-impact given the existing constraint.	Low
	Guard against non-positive max length parameter The blank check uses isBlank() which considers whitespace-only strings as blank, but the length check afterward uses the actual string length. A string like " " (3 spaces) would throw "null or empty" rather than a length error, which is fine, but a string of spaces longer than maxLength would also throw "null or empty" — this is acceptable. However, there is no validation that maxLength itself is a positive value, which could cause confusing behavior if 0 or negative is passed. server/src/main/java/org/opensearch/common/util/RequestUtils.java [34-43] if (requestId == null || requestId.isBlank()) { throw new IllegalArgumentException("X-Request-Id should not be null or empty"); +} + +if (maxLength <= 0) { + throw new IllegalArgumentException("maxLength must be positive, got: " + maxLength); } if (requestId.length() > maxLength) { throw new IllegalArgumentException( "X-Request-Id length [" + requestId.length() + "] exceeds maximum allowed length [" + maxLength + "]" ); } Suggestion importance[1-10]: 2 __ Why: The maxLength parameter is sourced from a setting with a minimum of 1, so a non-positive value cannot occur in practice. Adding this guard in validateRequestId is redundant and adds noise without meaningful safety benefit.	Low

[github-actions]

Persistent review updated to latest commit cdc7a69

[github-actions]

✅ Gradle check result for cdc7a69: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.23%. Comparing base (15fcc08) to head (a06d346).
⚠️ Report is 136 commits behind head on main.

Files with missing lines	Patch %	Lines
.../main/java/org/opensearch/rest/RestController.java	75.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21048      +/-   ##
============================================
- Coverage     73.26%   73.23%   -0.04%     
- Complexity    72743    72766      +23     
============================================
  Files          5862     5871       +9     
  Lines        332558   332670     +112     
  Branches      48010    48012       +2     
============================================
- Hits         243643   243621      -22     
- Misses        69343    69522     +179     
+ Partials      19572    19527      -45     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[finnegancarroll]

Code analyzer gives the following concern.

allows arbitrary characters (including newlines, control chars, or special sequences) in the request ID, which could enable log injection 

Gave this a quick test and it seems netty rejects these headers as invalid so we do not need to handle them ourselves.

curl -s --http1.1 -H $'X-Request-Id: foo\rbar' \
  -X GET "$BASE/test-index/_search" \
  -H "Content-Type: application/json" \
  -d '{"query": {"match_all": {}}}' | jq .

{
  "error": {
    "root_cause": [
      {
        "type": "illegal_argument_exception",
        "reason": "Validation failed for header 'X-Request-Id'"
      }
    ],
    "type": "illegal_argument_exception",
    "reason": "Validation failed for header 'X-Request-Id'",
    "caused_by": {
      "type": "illegal_argument_exception",
      "reason": "a header value contains prohibited character 0xd at index 3."
    }
  },
  "status": 400
}

[finnegancarroll]

@sgup432 can you take a look when you have a chance?

[github-actions]

Persistent review updated to latest commit 85e4753

[github-actions]

Persistent review updated to latest commit 16c1fae

[github-actions]

❌ Gradle check result for 16c1fae: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 16c1fae

[github-actions]

❌ Gradle check result for 16c1fae:

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit c6abb99

[github-actions]

Persistent review updated to latest commit a06d346

[bowenlan-amzn]

Looks good!
Regarding potential character injection concerns — I guess an attacker would need existing access and permissions to the cluster to exploit this, at which point they have far more impactful ways to cause damage.

[jainankitk]

Looks good to me!

[github-actions]

✅ Gradle check result for a06d346: SUCCESS