Strengthen scroll_id validation

[cwperks]

Description

Adds bounds checking on the varint-encoded count in TransportSearchHelper.parseScrollId(). Previously, a crafted scroll ID encoding a massive count (e.g., Integer.MAX_VALUE) would cause an OOM by allocating an oversized array before any data was read. The fix validates that the decoded count does not exceed the remaining payload size.

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 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 Loose Bound Check The check count > bytes.length is a very loose upper bound. Each SearchContextIdForNode entry requires multiple fields (strings + long + string), so the minimum bytes per entry is well above 1. An attacker can still craft a scroll id with count close to bytes.length (e.g., a few hundred thousand for a small payload) to allocate a large SearchContextIdForNode[] array before the stream is exhausted. Consider a tighter bound based on the minimum serialized entry size (e.g., bytes.length / MIN_ENTRY_SIZE), or read entries into a growable list rather than pre-allocating. int count = in.readVInt(); if (count < 0 || count > bytes.length) { throw new IllegalArgumentException("Invalid scroll id"); } SearchContextIdForNode[] context = new SearchContextIdForNode[count]; Test Assertion Mismatch The test asserts the exception message equals "Cannot parse scroll id", but the code throws new IllegalArgumentException("Invalid scroll id"). Unless the exception is wrapped/rethrown by an outer catch with that message elsewhere in parseScrollId, this assertion will fail. Verify the actual message produced at runtime and align the test. IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> TransportSearchHelper.parseScrollId(scrollId)); assertEquals("Cannot parse scroll id", e.getMessage());

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix mismatched expected exception message The test asserts the message equals "Cannot parse scroll id", but the new validation code throws with the message "Invalid scroll id". Unless there is an outer wrapper that rethrows with a different message, this assertion will fail; align the expected message with the one actually thrown. server/src/test/java/org/opensearch/action/search/TransportSearchHelperTests.java [114-115] IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> TransportSearchHelper.parseScrollId(scrollId)); -assertEquals("Cannot parse scroll id", e.getMessage()); +assertEquals("Invalid scroll id", e.getMessage()); Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that the test expects "Cannot parse scroll id" but the new code throws "Invalid scroll id". However, parseScrollId may wrap the exception (as suggested by the existing catch logic in the file), so verification is needed, but this is a likely test failure.	Medium
	Tighten upper bound on entry count The upper bound count > bytes.length is too loose because each SearchContextIdForNode entry requires multiple bytes to encode (at least a long id plus two strings), so a count close to bytes.length can still cause excessive allocation. Use a tighter bound based on the minimum per-entry byte size (e.g., at least 8 bytes for the long id), or better, compare against in.available() / remaining bytes rather than the total buffer length. server/src/main/java/org/opensearch/action/search/TransportSearchHelper.java [116-120] int count = in.readVInt(); -if (count < 0 || count > bytes.length) { +// Each entry requires at least a long (8 bytes) plus two strings, so cap generously. +if (count < 0 || count > bytes.length / 8) { throw new IllegalArgumentException("Invalid scroll id"); } SearchContextIdForNode[] context = new SearchContextIdForNode[count]; Suggestion importance[1-10]: 6 __ Why: Valid observation: since each SearchContextIdForNode requires multiple bytes to encode, a tighter bound based on minimum per-entry size would prevent excessive allocation more effectively. However, the current check already prevents catastrophic OOM.	Low

[github-actions]

❌ Gradle check result for 37a93f6: 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]

✅ Gradle check result for 37a93f6: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.52%. Comparing base (b99229e) to head (37a93f6).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...pensearch/action/search/TransportSearchHelper.java	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22396      +/-   ##
============================================
- Coverage     73.52%   73.52%   -0.01%     
+ Complexity    76254    76240      -14     
============================================
  Files          6076     6076              
  Lines        345790   345793       +3     
  Branches      49762    49763       +1     
============================================
+ Hits         254236   254238       +2     
+ Misses        71371    71343      -28     
- Partials      20183    20212      +29     

☔ 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.