Skip to content

[Backport 2.19] Strengthen scroll_id validation - #22401

Merged
cwperks merged 1 commit into
opensearch-project:2.19from
opensearch-ci-bot:backport/backport-22396-to-2.19
Jul 7, 2026
Merged

[Backport 2.19] Strengthen scroll_id validation#22401
cwperks merged 1 commit into
opensearch-project:2.19from
opensearch-ci-bot:backport/backport-22396-to-2.19

Conversation

@opensearch-ci-bot

Copy link
Copy Markdown
Contributor

Backport bcf120a from #22396.

Signed-off-by: Craig Perkins <cwperx@amazon.com>
(cherry picked from commit bcf120a)
Signed-off-by: opensearch-ci-bot <opensearch-infra@amazon.com>
@opensearch-ci-bot
opensearch-ci-bot requested a review from a team as a code owner July 7, 2026 00:52
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

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
⚡ No major issues detected

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Tighten upper bound for entry count

The upper bound count > bytes.length is still too permissive because each
SearchContextIdForNode entry requires multiple bytes to encode (contextUUID string,
long id, target string, optional cluster alias). A more accurate bound would divide
bytes.length by the minimum bytes-per-entry (at least ~10 bytes) to prevent large
allocations that pass the current check but still cannot be filled from the
remaining payload.

server/src/main/java/org/opensearch/action/search/TransportSearchHelper.java [108-112]

 int count = in.readVInt();
-if (count < 0 || count > bytes.length) {
+// Each entry requires at least ~10 bytes (long id + two vint-prefixed strings); guard accordingly.
+if (count < 0 || count > bytes.length / 10) {
     throw new IllegalArgumentException("Invalid scroll id");
 }
 SearchContextIdForNode[] context = new SearchContextIdForNode[count];
Suggestion importance[1-10]: 6

__

Why: The suggestion is reasonable since each entry requires multiple bytes, so count > bytes.length is a loose bound. Tightening it would provide better protection, though the current check already prevents extreme OOM scenarios. The divisor of 10 is somewhat arbitrary but represents a meaningful improvement.

Low

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 01ada51: SUCCESS

@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 72.00%. Comparing base (aff3489) to head (01ada51).
⚠️ Report is 15 commits behind head on 2.19.

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              @@
##               2.19   #22401      +/-   ##
============================================
+ Coverage     71.92%   72.00%   +0.07%     
+ Complexity    66009    64462    -1547     
============================================
  Files          5342     5121     -221     
  Lines        307392   300250    -7142     
  Branches      44862    44096     -766     
============================================
- Hits         221105   216180    -4925     
+ Misses        67823    65949    -1874     
+ Partials      18464    18121     -343     

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

@cwperks
cwperks merged commit fc2d6c8 into opensearch-project:2.19 Jul 7, 2026
45 of 46 checks passed
@opensearch-ci-bot
opensearch-ci-bot deleted the backport/backport-22396-to-2.19 branch July 7, 2026 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants