Normalize control characters in X-Request-Id and X-Opaque-Id before logging

[DarshitChanpura]

Description

The X-Request-Id and X-Opaque-Id request headers are client-provided and are written into the search slow logs (SearchSlowLog) and echoed back into HTTP responses. Today their values are used as-is. This change strips ASCII control characters from these values before they are used in log output, so that log records stay single-line and well-formed regardless of client input.

What changed

• Added a shared RequestUtils.sanitizeHeaderValue() helper that removes ASCII control characters (\p{Cntrl}), leaving null and well-formed values (UUIDs, hex/alphanumeric identifiers) unchanged.
• Applied it centrally in RestController where correlation headers are copied into the ThreadContext, so every downstream consumer that reads them via Task.getHeader(...) (including SearchSlowLog) receives a normalized value.
• Applied it in DefaultRestChannel, where X-Request-Id/X-Opaque-Id are read directly off the request to echo into the response (this path bypasses the thread context).
• The JSON slow-log layout already escapes these values via %enc{...}{JSON}; this closes the gap for the text (PatternLayout) path and response headers.

Why

Header values come directly from clients. Normalizing them keeps log files clean and reliably parseable for downstream tooling (log shippers, SIEM ingestion) and keeps a single logical request represented on a single log line.

Related component

Search/Logging

Check List

• Functionality includes testing.
• 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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit caa8998)

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 Validation Behavior Change validateRequestId is now called with the sanitized (control-characters-stripped) value instead of the raw first distinct header value. If a client supplies a request-id that exceeds requestIdMaxLength only after including control characters (e.g., a 33-char value with a control char that becomes 32 after stripping), validation will now pass whereas it would previously have failed. Additionally, previously distinctHeaderValues.getFirst() was validated, but now the joined comma-separated string is validated — although multi-valued headers already returned earlier, so this change is effectively equivalent for the single-value case. Confirm the intent is to validate the sanitized value. String headerValue = String.join(",", distinctHeaderValues); // Normalize client-provided correlation headers so downstream log output stays single-line if (Task.X_REQUEST_ID.equals(name) || Task.X_OPAQUE_ID.equals(name)) { headerValue = RequestUtils.sanitizeHeaderValue(headerValue); } threadContext.putHeader(name, headerValue); // Validate request-id header if present if (Task.X_REQUEST_ID.equals(name)) { RequestUtils.validateRequestId(headerValue, requestIdMaxLength); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to caa8998
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Preserve prior single-value validation semantics The previous code validated distinctHeaderValues.getFirst() (a single value), but the new code validates the joined value which may include a comma. If multiple X-Request-Id values were sent, the joined string length could exceed the configured max even when each individual value is valid, causing a semantics change in validation behavior. Consider validating each distinct value, or validating before joining, to preserve prior behavior. server/src/main/java/org/opensearch/rest/RestController.java [443-452] String headerValue = String.join(",", distinctHeaderValues); // Normalize client-provided correlation headers so downstream log output stays single-line if (Task.X_REQUEST_ID.equals(name) || Task.X_OPAQUE_ID.equals(name)) { headerValue = RequestUtils.sanitizeHeaderValue(headerValue); } threadContext.putHeader(name, headerValue); // Validate request-id header if present if (Task.X_REQUEST_ID.equals(name)) { - RequestUtils.validateRequestId(headerValue, requestIdMaxLength); + RequestUtils.validateRequestId(RequestUtils.sanitizeHeaderValue(distinctHeaderValues.getFirst()), requestIdMaxLength); } Suggestion importance[1-10]: 6 __ Why: Correctly identifies a behavioral change: the joined value with commas could exceed the max length even when individual values are valid, altering validation semantics compared to the previous distinctHeaderValues.getFirst() check.	Low
General	Broaden control character stripping pattern Java's \p{Cntrl} regex class by default matches only ASCII control characters, but the comment claims 0x00-0x1F and 0x7F. This is only accurate when the default (non-Unicode) mode is used; if Pattern.UNICODE_CHARACTER_CLASS is ever enabled elsewhere or if input contains non-ASCII control characters (e.g., Unicode line separators U+2028/U+2029 which also break single-line logs), they will not be stripped. Consider using an explicit character class to guarantee coverage of all log-injection-relevant characters. server/src/main/java/org/opensearch/common/util/RequestUtils.java [24-25] -/** Matches ASCII control characters (0x00-0x1F and 0x7F). */ -private static final Pattern CONTROL_CHARS = Pattern.compile("\\p{Cntrl}"); +/** Matches control characters that could break single-line log output. */ +private static final Pattern CONTROL_CHARS = Pattern.compile("[\\x00-\\x1F\\x7F\\u2028\\u2029]"); Suggestion importance[1-10]: 5 __ Why: Valid point that \p{Cntrl} only covers ASCII control characters and Unicode line separators (U+2028/U+2029) could also break single-line logs. The suggestion improves defense in depth, though the current implementation is functionally correct for the common log-injection case.	Low

---

Previous suggestions

Suggestions up to commit 3ac878a

Category	Suggestion	Impact
Possible issue	Validate request-id before sanitizing value validateRequestId is now called with the sanitized, comma-joined headerValue rather than the original first distinct value. This changes validation semantics: a request with only control characters would validate as an empty string, and the length check is now performed on the joined string instead of a single value. Validate the original raw value first (as before) and then store the sanitized version, to preserve the previous rejection behavior for malformed IDs. server/src/main/java/org/opensearch/rest/RestController.java [443-452] +// Validate request-id header (against the raw client value) if present +if (Task.X_REQUEST_ID.equals(name)) { + RequestUtils.validateRequestId(distinctHeaderValues.getFirst(), requestIdMaxLength); +} String headerValue = String.join(",", distinctHeaderValues); // Normalize client-provided correlation headers so downstream log output stays single-line if (Task.X_REQUEST_ID.equals(name) || Task.X_OPAQUE_ID.equals(name)) { headerValue = RequestUtils.sanitizeHeaderValue(headerValue); } threadContext.putHeader(name, headerValue); -// Validate request-id header if present -if (Task.X_REQUEST_ID.equals(name)) { - RequestUtils.validateRequestId(headerValue, requestIdMaxLength); -} Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a semantic change: validation now runs on the sanitized/joined value rather than the original first distinct value, which could allow malformed IDs (e.g., control chars only) to pass length validation as an empty string. This is a valid concern about preserving prior rejection behavior, though the practical impact is moderate.	Low

[DarshitChanpura]

this previously passed distinctHeaderValues.getFirst(); it now passes the joined headerValue. These are equivalent for X-Request-Id: it's registered as single-valued (new RestHeaderDefinition(Task.X_REQUEST_ID, false)), so the guard above (L433) returns 400 for more than one value before this branch is reached — meaning distinctHeaderValues always has exactly one element here, and String.join(",", …) equals getFirst(). Validating headerValue directly is intentional: it validates the exact value stored in the ThreadContext (no chance of the validated and stored values drifting) and stays correct if the header were ever made multi-valued.

[github-actions]

✅ Gradle check result for 3ac878a: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.49%. Comparing base (c8e2303) to head (caa8998).
⚠️ Report is 36 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main   #22682   +/-   ##
=========================================
  Coverage     71.48%   71.49%           
- Complexity    76960    76981   +21     
=========================================
  Files          6156     6156           
  Lines        358444   358452    +8     
  Branches      52246    52247    +1     
=========================================
+ Hits         256240   256273   +33     
- Misses        81792    81808   +16     
+ Partials      20412    20371   -41     

☔ View full report in Codecov by Harness.
📢 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.

[github-actions]

Persistent review updated to latest commit caa8998

[github-actions]

❕ Gradle check result for caa8998: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[cwperks]

@DarshitChanpura FYI there used to be some formatting imposed, but that was removed in #21048. I think there is definitely room for sanitization, but can there also be something on the security plugin side to escape characters like newline?

[DarshitChanpura]

@cwperks We added this recently and can be expanded to cover more:
https://github.com/opensearch-project/security/blob/92c9a20a3ed327f938ddc9b98ebae4644c25e270/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java#L421