fix: Close integer validation bypass and query parser differential (#109)

* fix: Close integer validation bypass and query parser differential

Two validation bypasses surfaced in a security review of the request
pipeline:

- checkInteger narrowed numbers with longValue() before bound-checking,
  so a fractional Double (4.9) satisfied `type: integer` and a value above
  2^64 wrapped past a declared maximum. It now rejects non-integral numbers
  and compares the full magnitude, keeping an allocation-free long path for
  the common case and falling back to BigDecimal/BigInteger otherwise.
- Query parameters were validated from the already-decoded URI.getQuery()
  but delivered to handlers from getRawQuery(), so an encoded separator
  (%26) could smuggle a value past a schema constraint. Both paths now
  share QueryParams.parse over the raw query, so the value validated is the
  value the handler receives.

Adds exploit tests asserting both are rejected.

* refactor: Simplify out-of-range integer check and cover new branches

The SonarCloud new-code coverage gate failed on PR #109: extracting the
bound checks into checkIntegerBounds turned the exclusive-bound and
beyond-long paths into "new" lines that no test exercised.

- Simplify the beyond-long branch to delegate to the long path when the
  value fits, and otherwise reject against whichever bound lies on its side
  (or an int32/int64 format), collapsing the per-bound arms.
- Add tests for exclusive bounds, values above 2^53 that still fit in a
  long, every beyond-long rejection path, and QueryParams edge cases
  (empty pairs, keys without '=', encoded separators).

* fix: Drop float-equality check that failed the reliability gate

SonarCloud flagged S1244 (floating-point equality) on the `d == Math.rint(d)`
integrality test in checkInteger's fast double path, dropping New Code
reliability to C.

Remove the double fast path entirely: doubles reaching an integer schema are
either fractional (rejected) or rare integral doubles, both handled correctly
by converting through BigDecimal.toBigIntegerExact(). Integral wrappers keep
the allocation-free long path, so the common case is unaffected.

Author: thced

src/main/java/com/retailsvc/http/Request.java

@@ -1,11 +1,9 @@
 package com.retailsvc.http;
 
+import com.retailsvc.http.internal.QueryParams;
 import com.retailsvc.http.spec.HttpMethod;
-import java.net.URLDecoder;
-import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -265,7 +263,7 @@ public String rawQuery() {
    */
   public Map<String, String> queryParams() {
     if (queryParamCache == null) {
-      queryParamCache = parseQuery(rawQuery);
+      queryParamCache = QueryParams.parse(rawQuery);
     }
     return queryParamCache;
   }
@@ -345,23 +343,4 @@ public void afterResponse(Runnable runnable) {
   public List<Runnable> afterHooks() {
     return Collections.unmodifiableList(afterHooks);
   }
-
-  private static Map<String, String> parseQuery(String query) {
-    if (query == null || query.isBlank()) {
-      return Map.of();
-    }
-    Map<String, String> out = new LinkedHashMap<>();
-    for (String pair : query.split("&")) {
-      if (pair.isEmpty()) {
-        continue;
-      }
-      int eq = pair.indexOf('=');
-      String rawKey = eq < 0 ? pair : pair.substring(0, eq);
-      String rawValue = eq < 0 ? "" : pair.substring(eq + 1);
-      out.putIfAbsent(
-          URLDecoder.decode(rawKey, StandardCharsets.UTF_8),
-          URLDecoder.decode(rawValue, StandardCharsets.UTF_8));
-    }
-    return out;
-  }
 }

src/main/java/com/retailsvc/http/internal/QueryParams.java

@@ -0,0 +1,42 @@
+package com.retailsvc.http.internal;
+
+import java.net.URLDecoder;
+import java.nio.charset.StandardCharsets;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+/**
+ * Parses a raw (percent-encoded) URI query string into decoded parameters. Splitting on {@code &}
+ * and {@code =} happens <em>before</em> per-component decoding, so a percent-encoded separator
+ * ({@code %26}, {@code %3D}) stays inside a value instead of becoming a delimiter. Request
+ * validation and handler consumption both parse through here, so the value validated is exactly the
+ * value the handler receives.
+ */
+public final class QueryParams {
+
+  private QueryParams() {}
+
+  /**
+   * Parses {@code rawQuery} (the undecoded query component, e.g. {@code URI.getRawQuery()}) into a
+   * name→value map. Values are URL-decoded with UTF-8. For repeated names the first occurrence
+   * wins.
+   */
+  public static Map<String, String> parse(String rawQuery) {
+    if (rawQuery == null || rawQuery.isBlank()) {
+      return Map.of();
+    }
+    Map<String, String> out = new LinkedHashMap<>();
+    for (String pair : rawQuery.split("&")) {
+      if (pair.isEmpty()) {
+        continue;
+      }
+      int eq = pair.indexOf('=');
+      String rawKey = eq < 0 ? pair : pair.substring(0, eq);
+      String rawValue = eq < 0 ? "" : pair.substring(eq + 1);
+      out.putIfAbsent(
+          URLDecoder.decode(rawKey, StandardCharsets.UTF_8),
+          URLDecoder.decode(rawValue, StandardCharsets.UTF_8));
+    }
+    return out;
+  }
+}

src/main/java/com/retailsvc/http/internal/RequestPreparationFilter.java

@@ -20,7 +20,6 @@
 import com.sun.net.httpserver.Headers;
 import com.sun.net.httpserver.HttpExchange;
 import java.io.IOException;
-import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Locale;
@@ -189,7 +188,7 @@ private void validateParameters(
     for (Parameter p : op.parameters()) {
       String pointer = p.pointer();
       if (p.in() == Parameter.Location.QUERY && query == null) {
-        query = parseQuery(exchange.getRequestURI().getQuery());
+        query = QueryParams.parse(exchange.getRequestURI().getRawQuery());
       }
       String value =
           switch (p.in()) {
@@ -253,19 +252,4 @@ private ParsedBody validateAndParseBody(HttpExchange exchange, Operation op, byt
     validator.validate(parsed, mt.schema(), "");
     return new ParsedBody(parsed, mapper);
   }
-
-  private static Map<String, String> parseQuery(String query) {
-    if (query == null || query.isBlank()) {
-      return Map.of();
-    }
-    Map<String, String> out = new HashMap<>();
-    for (String pair : query.split("&")) {
-      int eq = pair.indexOf('=');
-      if (eq <= 0) {
-        continue;
-      }
-      out.putIfAbsent(pair.substring(0, eq), pair.substring(eq + 1));
-    }
-    return out;
-  }
 }

src/main/java/com/retailsvc/http/validate/DefaultValidator.java

@@ -21,6 +21,7 @@
 import com.retailsvc.http.spec.schema.StringSchema;
 import com.retailsvc.http.spec.schema.TypeName;
 import java.math.BigDecimal;
+import java.math.BigInteger;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.time.LocalDate;
@@ -47,6 +48,8 @@
 public final class DefaultValidator implements Validator {
 
   private static final String FORMAT_KEYWORD = "format";
+  private static final String MINIMUM_KEYWORD = "minimum";
+  private static final String MAXIMUM_KEYWORD = "maximum";
   private static final Optional<ValidationError> OK = Optional.empty();
 
   private record FormatCheck(Predicate<String> isValid, String message) {}
@@ -365,18 +368,39 @@ private static boolean isHextet(String hextet) {
     return true;
   }
 
+  /** Largest magnitude an IEEE-754 double represents as an exact integer (2^53). */
   private static Optional<ValidationError> checkInteger(
       Object value, IntegerSchema s, String pointer) {
     if (!(value instanceof Number num)) {
       return err(pointer, "type", "expected integer", value);
     }
-    long n = num.longValue();
+    // Fast path: integral wrappers bound-check as a long with no allocation. This is the common
+    // case (e.g. Jackson int/long, Gson integral tokens parsed as Long).
+    if (num instanceof Long
+        || num instanceof Integer
+        || num instanceof Short
+        || num instanceof Byte) {
+      return checkIntegerBounds(num.longValue(), s, pointer);
+    }
+    // Any other numeric type (Double, BigDecimal, ...) must be a whole number: convert exactly and
+    // reject a fractional part (e.g. 4.9) or a non-finite value, comparing the full magnitude so
+    // nothing wraps past a bound.
+    BigInteger n;
+    try {
+      n = new BigDecimal(num.toString()).toBigIntegerExact();
+    } catch (ArithmeticException | NumberFormatException _) {
+      return err(pointer, "type", "expected integer", value);
+    }
+    return checkIntegerBounds(n, s, pointer);
+  }
 
+  private static Optional<ValidationError> checkIntegerBounds(
+      long n, IntegerSchema s, String pointer) {
     if (s.minimum() != null && n < s.minimum()) {
-      return err(pointer, "minimum", "integer below minimum " + s.minimum(), n);
+      return err(pointer, MINIMUM_KEYWORD, "integer below minimum " + s.minimum(), n);
     }
     if (s.maximum() != null && n > s.maximum()) {
-      return err(pointer, "maximum", "integer above maximum " + s.maximum(), n);
+      return err(pointer, MAXIMUM_KEYWORD, "integer above maximum " + s.maximum(), n);
     }
     if (s.exclusiveMinimum() != null && n <= s.exclusiveMinimum()) {
       return err(
@@ -394,6 +418,25 @@ private static Optional<ValidationError> checkInteger(
     return OK;
   }
 
+  private static Optional<ValidationError> checkIntegerBounds(
+      BigInteger n, IntegerSchema s, String pointer) {
+    if (n.bitLength() < Long.SIZE) {
+      return checkIntegerBounds(n.longValue(), s, pointer);
+    }
+    // Magnitude exceeds signed-long range: it breaches whichever bound lies on its side, and no
+    // int32/int64 format can represent it.
+    if (n.signum() > 0 && (s.maximum() != null || s.exclusiveMaximum() != null)) {
+      return err(pointer, MAXIMUM_KEYWORD, "integer out of range", n);
+    }
+    if (n.signum() < 0 && (s.minimum() != null || s.exclusiveMinimum() != null)) {
+      return err(pointer, MINIMUM_KEYWORD, "integer out of range", n);
+    }
+    if (s.format() != null && INTEGER_FORMAT_CHECKS.containsKey(s.format())) {
+      return err(pointer, FORMAT_KEYWORD, INTEGER_FORMAT_CHECKS.get(s.format()).message(), n);
+    }
+    return OK;
+  }
+
   private static Optional<ValidationError> checkNumber(
       Object value, NumberSchema s, String pointer) {
     if (!(value instanceof Number num)) {
@@ -402,10 +445,10 @@ private static Optional<ValidationError> checkNumber(
     double n = num.doubleValue();
 
     if (s.minimum() != null && n < s.minimum().doubleValue()) {
-      return err(pointer, "minimum", "number below minimum " + s.minimum(), n);
+      return err(pointer, MINIMUM_KEYWORD, "number below minimum " + s.minimum(), n);
     }
     if (s.maximum() != null && n > s.maximum().doubleValue()) {
-      return err(pointer, "maximum", "number above maximum " + s.maximum(), n);
+      return err(pointer, MAXIMUM_KEYWORD, "number above maximum " + s.maximum(), n);
     }
     if (s.exclusiveMinimum() != null && n <= s.exclusiveMinimum().doubleValue()) {
       return err(pointer, "exclusiveMinimum", "number not greater than " + s.exclusiveMinimum(), n);

src/test/java/com/retailsvc/http/QueryParamSmugglingTest.java

@@ -0,0 +1,58 @@
+package com.retailsvc.http;
+
+import static java.net.http.HttpRequest.BodyPublishers.noBody;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+
+import com.retailsvc.http.spec.Spec;
+import java.io.InputStream;
+import java.net.http.HttpResponse;
+import java.net.http.HttpResponse.BodyHandlers;
+import java.nio.charset.StandardCharsets;
+import java.util.List;
+import java.util.Map;
+import org.junit.jupiter.api.Test;
+
+class QueryParamSmugglingTest extends ServerBaseTest {
+
+  @Test
+  void encodedAmpersandCannotSmuggleAValuePastQueryValidation() throws Exception {
+    // Constrain q1 so any value containing '&' or '=' fails validation.
+    constrainQ1ToLowercaseLetters();
+    server = newServer(Map.of());
+
+    // %26 must stay inside q1: the handler would decode q1 to "abc&x=y", so validation must see the
+    // same value and reject it against ^[a-z]+$ (400). The exploit was that validation parsed the
+    // already-decoded query and saw q1="abc" (passing) while the handler received "abc&x=y".
+    var request = newRequest(server, "/params/query?q1=abc%26x=y&q2=ok", "GET", noBody());
+    HttpResponse<String> response = httpClient().send(request, BodyHandlers.ofString());
+
+    assertThat(response.statusCode()).isEqualTo(400);
+  }
+
+  @SuppressWarnings("unchecked")
+  private void constrainQ1ToLowercaseLetters() {
+    spec =
+        assertDoesNotThrow(
+            () -> {
+              try (InputStream in =
+                  QueryParamSmugglingTest.class.getResourceAsStream("/openapi.json")) {
+                Map<String, Object> raw =
+                    (Map<String, Object>)
+                        gson.fromJson(
+                            new String(in.readAllBytes(), StandardCharsets.UTF_8), Map.class);
+                var paths = (Map<String, Object>) raw.get("paths");
+                var op =
+                    (Map<String, Object>)
+                        ((Map<String, Object>) paths.get("/params/query")).get("get");
+                for (Object p : (List<Object>) op.get("parameters")) {
+                  var param = (Map<String, Object>) p;
+                  if ("q1".equals(param.get("name"))) {
+                    ((Map<String, Object>) param.get("schema")).put("pattern", "^[a-z]+$");
+                  }
+                }
+                return Spec.from(raw);
+              }
+            });
+  }
+}

src/test/java/com/retailsvc/http/internal/QueryParamsTest.java

@@ -0,0 +1,34 @@
+package com.retailsvc.http.internal;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.entry;
+
+import org.junit.jupiter.api.Test;
+
+class QueryParamsTest {
+
+  @Test
+  void nullOrBlankQueryYieldsEmptyMap() {
+    assertThat(QueryParams.parse(null)).isEmpty();
+    assertThat(QueryParams.parse("   ")).isEmpty();
+  }
+
+  @Test
+  void encodedSeparatorStaysInsideValue() {
+    // %26 is decoded after splitting, so it does not become a delimiter: one param, not two.
+    assertThat(QueryParams.parse("q=a%26b=c")).containsExactly(entry("q", "a&b=c"));
+  }
+
+  @Test
+  void splitsPairsDecodesAndKeepsFirstOccurrence() {
+    assertThat(QueryParams.parse("name=Alice%20Smith&name=Bob&city=x"))
+        .containsExactly(entry("name", "Alice Smith"), entry("city", "x"));
+  }
+
+  @Test
+  void skipsEmptyPairsAndHandlesKeysWithoutValue() {
+    // Empty pair (from "&&") is skipped; a key with no '=' maps to an empty value.
+    assertThat(QueryParams.parse("a=1&&flag&b=2"))
+        .containsExactly(entry("a", "1"), entry("flag", ""), entry("b", "2"));
+  }
+}