Skip to content

Conversation

@manuelsblanco
Copy link
Contributor

@manuelsblanco manuelsblanco commented Oct 27, 2025

User description

Related Issues

Fixes a bug in PrintOptions#setPageRanges where the last page range argument was not being copied due to an off-by-one error in the System.arraycopy call.

What does this PR do?

This PR fixes a small but critical logic error in the setPageRanges(String firstRange, String... ranges) method inside org.openqa.selenium.print.PrintOptions.

Previously, only ranges.length - 1 elements were copied from the ranges array into this.pageRanges.
As a result, the last page range was silently omitted whenever multiple ranges were passed to the method.

Example of the previous behavior:

setPageRanges("1-3", "5", "8-10");
// Result before fix:
["1-3", "5", null]
setPageRanges("1-3", "5", "8-10");
// Correct result:
["1-3", "5", "8-10"]

@selenium-ci selenium-ci added the C-java Java Bindings label Oct 27, 2025
@qodo-merge-pro
Copy link
Contributor

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No auditing: The new code introduces or modifies behavior without adding any audit logging for critical
actions, but it is unclear whether this method constitutes a critical auditable action in
this context.

Referred Code
public void setPageRanges(String firstRange, String... ranges) {
  Require.nonNull("pageRanges", firstRange);
  this.pageRanges =
      new String[ranges.length + 1]; // Need to add all ranges and the initial range too.

  this.pageRanges[0] = firstRange;

  if (ranges.length > 0) System.arraycopy(ranges, 0, this.pageRanges, 1, ranges.length);
}
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Null/empty ranges: Method validates only the first range and silently does nothing when vararg
'ranges' is empty, lacking explicit handling or validation for null elements
within 'ranges'.

Referred Code
public void setPageRanges(String firstRange, String... ranges) {
  Require.nonNull("pageRanges", firstRange);
  this.pageRanges =
      new String[ranges.length + 1]; // Need to add all ranges and the initial range too.

  this.pageRanges[0] = firstRange;

  if (ranges.length > 0) System.arraycopy(ranges, 0, this.pageRanges, 1, ranges.length);
}
Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Input validation: The code only checks that 'firstRange' is non-null and does not validate the
content or format of ranges provided, which may be acceptable depending on upstream
validation not visible here.

Referred Code
public void setPageRanges(String firstRange, String... ranges) {
  Require.nonNull("pageRanges", firstRange);
  this.pageRanges =
      new String[ranges.length + 1]; // Need to add all ranges and the initial range too.

  this.pageRanges[0] = firstRange;

  if (ranges.length > 0) System.arraycopy(ranges, 0, this.pageRanges, 1, ranges.length);
}
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-merge-pro
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Use a List to build array

Refactor the setPageRanges method to use a List for assembling the page ranges
before converting to an array, avoiding manual System.arraycopy and potential
index-related bugs.

java/src/org/openqa/selenium/print/PrintOptions.java [67-73]

 Require.nonNull("pageRanges", firstRange);
-this.pageRanges =
-    new String[ranges.length + 1]; // Need to add all ranges and the initial range too.
+List<String> allRanges = new ArrayList<>(ranges.length + 1);
+allRanges.add(firstRange);
+Collections.addAll(allRanges, ranges);
+this.pageRanges = allRanges.toArray(new String[0]);
 
-this.pageRanges[0] = firstRange;
-
-if (ranges.length > 0) System.arraycopy(ranges, 0, this.pageRanges, 1, ranges.length);
-

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the manual array manipulation was the source of the original bug and proposes a more robust, readable, and idiomatic Java solution using a List, which improves maintainability.

Low
Learned
best practice
Add defensive input validation

Validate that no element in ranges is null and that firstRange and all ranges
are non-empty to avoid invalid page range states.

java/src/org/openqa/selenium/print/PrintOptions.java [66-74]

 public void setPageRanges(String firstRange, String... ranges) {
   Require.nonNull("pageRanges", firstRange);
-  this.pageRanges =
-      new String[ranges.length + 1]; // Need to add all ranges and the initial range too.
-
+  if (firstRange.isEmpty()) {
+    throw new IllegalArgumentException("pageRanges first value must be non-empty");
+  }
+  if (ranges != null) {
+    for (int i = 0; i < ranges.length; i++) {
+      String r = ranges[i];
+      if (r == null || r.isEmpty()) {
+        throw new IllegalArgumentException("pageRanges must not contain null or empty entries");
+      }
+    }
+  }
+  this.pageRanges = new String[ranges.length + 1];
   this.pageRanges[0] = firstRange;
-
-  if (ranges.length > 0) System.arraycopy(ranges, 0, this.pageRanges, 1, ranges.length);
+  if (ranges.length > 0) {
+    System.arraycopy(ranges, 0, this.pageRanges, 1, ranges.length);
+  }
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Validate inputs early to prevent nulls and invalid states (Pattern 2).

Low
  • More

@cgoldberg cgoldberg changed the title fix: Correct array copy length in setPageRanges method to include all… [Java] Fix copy length in PrintOptions.setPageRanges method Oct 27, 2025
@cgoldberg cgoldberg changed the title [Java] Fix copy length in PrintOptions.setPageRanges method [Java] Fix array copy length in PrintOptions.setPageRanges method Oct 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants