From f6fe554d6e59b701b4864933d557c5e1ce4cc509 Mon Sep 17 00:00:00 2001 From: Thomas Cederholm Date: Wed, 1 Jul 2026 16:44:55 +0200 Subject: [PATCH 1/4] 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. --- src/main/java/com/retailsvc/http/Request.java | 25 +------ .../retailsvc/http/internal/QueryParams.java | 42 +++++++++++ .../internal/RequestPreparationFilter.java | 18 +---- .../http/validate/DefaultValidator.java | 69 ++++++++++++++++++- .../http/QueryParamSmugglingTest.java | 52 ++++++++++++++ .../validate/StringIntegerNumberTest.java | 34 +++++++++ 6 files changed, 199 insertions(+), 41 deletions(-) create mode 100644 src/main/java/com/retailsvc/http/internal/QueryParams.java create mode 100644 src/test/java/com/retailsvc/http/QueryParamSmugglingTest.java diff --git a/src/main/java/com/retailsvc/http/Request.java b/src/main/java/com/retailsvc/http/Request.java index 13d04043..545c878c 100644 --- a/src/main/java/com/retailsvc/http/Request.java +++ b/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 queryParams() { if (queryParamCache == null) { - queryParamCache = parseQuery(rawQuery); + queryParamCache = QueryParams.parse(rawQuery); } return queryParamCache; } @@ -345,23 +343,4 @@ public void afterResponse(Runnable runnable) { public List afterHooks() { return Collections.unmodifiableList(afterHooks); } - - private static Map parseQuery(String query) { - if (query == null || query.isBlank()) { - return Map.of(); - } - Map 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; - } } diff --git a/src/main/java/com/retailsvc/http/internal/QueryParams.java b/src/main/java/com/retailsvc/http/internal/QueryParams.java new file mode 100644 index 00000000..40f10afe --- /dev/null +++ b/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 before 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 parse(String rawQuery) { + if (rawQuery == null || rawQuery.isBlank()) { + return Map.of(); + } + Map 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; + } +} diff --git a/src/main/java/com/retailsvc/http/internal/RequestPreparationFilter.java b/src/main/java/com/retailsvc/http/internal/RequestPreparationFilter.java index f9283dc9..68014e0c 100644 --- a/src/main/java/com/retailsvc/http/internal/RequestPreparationFilter.java +++ b/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 parseQuery(String query) { - if (query == null || query.isBlank()) { - return Map.of(); - } - Map 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; - } } diff --git a/src/main/java/com/retailsvc/http/validate/DefaultValidator.java b/src/main/java/com/retailsvc/http/validate/DefaultValidator.java index 64902100..0d06b3b2 100644 --- a/src/main/java/com/retailsvc/http/validate/DefaultValidator.java +++ b/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; @@ -365,13 +366,40 @@ private static boolean isHextet(String hextet) { return true; } + /** Largest magnitude an IEEE-754 double represents as an exact integer (2^53). */ + private static final double MAX_EXACT_INTEGRAL_DOUBLE = 9007199254740992.0; + private static Optional 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); + } + // Integral doubles small enough to be represented exactly also take the long path. + double d = num.doubleValue(); + if (Double.isFinite(d) && d == Math.rint(d) && Math.abs(d) <= MAX_EXACT_INTEGRAL_DOUBLE) { + return checkIntegerBounds((long) d, s, pointer); + } + // Non-integral (e.g. 4.9), non-finite, or beyond a double's exact-integer range: verify the + // value is a whole number and compare the full magnitude so nothing wraps past a bound. + BigInteger n; + try { + n = new BigDecimal(num.toString()).toBigIntegerExact(); + } catch (ArithmeticException | NumberFormatException e) { + return err(pointer, "type", "expected integer", value); + } + return checkIntegerBounds(n, s, pointer); + } + private static Optional checkIntegerBounds( + long n, IntegerSchema s, String pointer) { if (s.minimum() != null && n < s.minimum()) { return err(pointer, "minimum", "integer below minimum " + s.minimum(), n); } @@ -394,6 +422,45 @@ private static Optional checkInteger( return OK; } + private static Optional checkIntegerBounds( + BigInteger n, IntegerSchema s, String pointer) { + long l; + try { + l = n.longValueExact(); + } catch (ArithmeticException outsideLong) { + // Magnitude exceeds long range: it is below any Long minimum (if negative) or above any Long + // maximum (if positive), and cannot satisfy an int32/int64 format. + if (n.signum() > 0) { + if (s.maximum() != null) { + return err(pointer, "maximum", "integer above maximum " + s.maximum(), n); + } + if (s.exclusiveMaximum() != null) { + return err( + pointer, "exclusiveMaximum", "integer not less than " + s.exclusiveMaximum(), n); + } + } else { + if (s.minimum() != null) { + return err(pointer, "minimum", "integer below minimum " + s.minimum(), n); + } + if (s.exclusiveMinimum() != null) { + return err( + pointer, "exclusiveMinimum", "integer not greater than " + s.exclusiveMinimum(), n); + } + } + if (s.multipleOf() != null && n.remainder(BigInteger.valueOf(s.multipleOf())).signum() != 0) { + return err(pointer, "multipleOf", "not a multiple of " + s.multipleOf(), n); + } + if (s.format() != null) { + IntegerFormatCheck check = INTEGER_FORMAT_CHECKS.get(s.format()); + if (check != null) { + return err(pointer, FORMAT_KEYWORD, check.message(), n); + } + } + return OK; + } + return checkIntegerBounds(l, s, pointer); + } + private static Optional checkNumber( Object value, NumberSchema s, String pointer) { if (!(value instanceof Number num)) { diff --git a/src/test/java/com/retailsvc/http/QueryParamSmugglingTest.java b/src/test/java/com/retailsvc/http/QueryParamSmugglingTest.java new file mode 100644 index 00000000..77a54732 --- /dev/null +++ b/src/test/java/com/retailsvc/http/QueryParamSmugglingTest.java @@ -0,0 +1,52 @@ +package com.retailsvc.http; + +import static java.net.http.HttpRequest.BodyPublishers.noBody; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.fail; + +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 response = httpClient().send(request, BodyHandlers.ofString()); + + assertThat(response.statusCode()).isEqualTo(400); + } + + @SuppressWarnings("unchecked") + private void constrainQ1ToLowercaseLetters() { + try (InputStream in = QueryParamSmugglingTest.class.getResourceAsStream("/openapi.json")) { + Map raw = + (Map) + gson.fromJson(new String(in.readAllBytes(), StandardCharsets.UTF_8), Map.class); + var paths = (Map) raw.get("paths"); + var op = (Map) ((Map) paths.get("/params/query")).get("get"); + for (Object p : (List) op.get("parameters")) { + var param = (Map) p; + if ("q1".equals(param.get("name"))) { + ((Map) param.get("schema")).put("pattern", "^[a-z]+$"); + } + } + spec = Spec.from(raw); + } catch (Exception e) { + fail(e); + } + } +} diff --git a/src/test/java/com/retailsvc/http/validate/StringIntegerNumberTest.java b/src/test/java/com/retailsvc/http/validate/StringIntegerNumberTest.java index ee8cc2e5..3bbffcf8 100644 --- a/src/test/java/com/retailsvc/http/validate/StringIntegerNumberTest.java +++ b/src/test/java/com/retailsvc/http/validate/StringIntegerNumberTest.java @@ -8,6 +8,7 @@ import com.retailsvc.http.spec.schema.NumberSchema; import com.retailsvc.http.spec.schema.StringSchema; import com.retailsvc.http.spec.schema.TypeName; +import java.math.BigInteger; import java.util.List; import java.util.Map; import java.util.Set; @@ -225,6 +226,39 @@ void integerMultipleOf() { .isEqualTo("multipleOf"); } + @Test + void integerRejectsFractionalDouble() { + // Exploit: a Double with a fractional part must not satisfy `type: integer`. Master narrowed + // via + // longValue() before the bound check, so 4.9 slipped through minimum/maximum as 4. + IntegerSchema s = + new IntegerSchema(Set.of(TypeName.INTEGER), 1L, 5L, null, null, null, null, Map.of()); + assertThatThrownBy(() -> v.validate(4.9, s, "/v")) + .isInstanceOf(ValidationException.class) + .extracting(t -> ((ValidationException) t).error().keyword()) + .isEqualTo("type"); + } + + @Test + void integerAcceptsIntegralDouble() { + IntegerSchema s = + new IntegerSchema(Set.of(TypeName.INTEGER), 1L, 5L, null, null, null, null, Map.of()); + assertThatCode(() -> v.validate(4.0, s, "/v")).doesNotThrowAnyException(); + } + + @Test + void integerRejectsValueOverflowingLongPastMaximum() { + // Exploit: 2^64 + 50 narrows to 50 via longValue(), sneaking past maximum:1000. The full + // magnitude must be compared so the bound cannot be wrapped. + IntegerSchema s = + new IntegerSchema(Set.of(TypeName.INTEGER), 0L, 1000L, null, null, null, null, Map.of()); + BigInteger huge = BigInteger.TWO.pow(64).add(BigInteger.valueOf(50)); + assertThatThrownBy(() -> v.validate(huge, s, "/v")) + .isInstanceOf(ValidationException.class) + .extracting(t -> ((ValidationException) t).error().keyword()) + .isEqualTo("maximum"); + } + @Test void numberAcceptsDoublesAndIntegers() { NumberSchema s = From f41402ed6e2b619495805db72b3bb9be0fcc3659 Mon Sep 17 00:00:00 2001 From: Thomas Cederholm Date: Wed, 1 Jul 2026 17:10:17 +0200 Subject: [PATCH 2/4] 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). --- .../http/validate/DefaultValidator.java | 48 ++++--------- .../http/internal/QueryParamsTest.java | 34 ++++++++++ .../validate/StringIntegerNumberTest.java | 68 +++++++++++++++++++ 3 files changed, 116 insertions(+), 34 deletions(-) create mode 100644 src/test/java/com/retailsvc/http/internal/QueryParamsTest.java diff --git a/src/main/java/com/retailsvc/http/validate/DefaultValidator.java b/src/main/java/com/retailsvc/http/validate/DefaultValidator.java index 0d06b3b2..12ba49be 100644 --- a/src/main/java/com/retailsvc/http/validate/DefaultValidator.java +++ b/src/main/java/com/retailsvc/http/validate/DefaultValidator.java @@ -424,41 +424,21 @@ private static Optional checkIntegerBounds( private static Optional checkIntegerBounds( BigInteger n, IntegerSchema s, String pointer) { - long l; - try { - l = n.longValueExact(); - } catch (ArithmeticException outsideLong) { - // Magnitude exceeds long range: it is below any Long minimum (if negative) or above any Long - // maximum (if positive), and cannot satisfy an int32/int64 format. - if (n.signum() > 0) { - if (s.maximum() != null) { - return err(pointer, "maximum", "integer above maximum " + s.maximum(), n); - } - if (s.exclusiveMaximum() != null) { - return err( - pointer, "exclusiveMaximum", "integer not less than " + s.exclusiveMaximum(), n); - } - } else { - if (s.minimum() != null) { - return err(pointer, "minimum", "integer below minimum " + s.minimum(), n); - } - if (s.exclusiveMinimum() != null) { - return err( - pointer, "exclusiveMinimum", "integer not greater than " + s.exclusiveMinimum(), n); - } - } - if (s.multipleOf() != null && n.remainder(BigInteger.valueOf(s.multipleOf())).signum() != 0) { - return err(pointer, "multipleOf", "not a multiple of " + s.multipleOf(), n); - } - if (s.format() != null) { - IntegerFormatCheck check = INTEGER_FORMAT_CHECKS.get(s.format()); - if (check != null) { - return err(pointer, FORMAT_KEYWORD, check.message(), n); - } - } - return OK; + 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", "integer out of range", n); } - return checkIntegerBounds(l, s, pointer); + if (n.signum() < 0 && (s.minimum() != null || s.exclusiveMinimum() != null)) { + return err(pointer, "minimum", "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 checkNumber( diff --git a/src/test/java/com/retailsvc/http/internal/QueryParamsTest.java b/src/test/java/com/retailsvc/http/internal/QueryParamsTest.java new file mode 100644 index 00000000..74700c4e --- /dev/null +++ b/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")); + } +} diff --git a/src/test/java/com/retailsvc/http/validate/StringIntegerNumberTest.java b/src/test/java/com/retailsvc/http/validate/StringIntegerNumberTest.java index 3bbffcf8..4a0ee02c 100644 --- a/src/test/java/com/retailsvc/http/validate/StringIntegerNumberTest.java +++ b/src/test/java/com/retailsvc/http/validate/StringIntegerNumberTest.java @@ -259,6 +259,74 @@ void integerRejectsValueOverflowingLongPastMaximum() { .isEqualTo("maximum"); } + @Test + void integerExclusiveBounds() { + IntegerSchema exclMin = + new IntegerSchema(Set.of(TypeName.INTEGER), null, null, 5L, null, null, null, Map.of()); + assertThatCode(() -> v.validate(6, exclMin, "/v")).doesNotThrowAnyException(); + assertThatThrownBy(() -> v.validate(5, exclMin, "/v")) + .extracting(t -> ((ValidationException) t).error().keyword()) + .isEqualTo("exclusiveMinimum"); + + IntegerSchema exclMax = + new IntegerSchema(Set.of(TypeName.INTEGER), null, null, null, 5L, null, null, Map.of()); + assertThatCode(() -> v.validate(4, exclMax, "/v")).doesNotThrowAnyException(); + assertThatThrownBy(() -> v.validate(5, exclMax, "/v")) + .extracting(t -> ((ValidationException) t).error().keyword()) + .isEqualTo("exclusiveMaximum"); + } + + @Test + void integerLargeButWithinLongRangeIsCheckedPrecisely() { + // > 2^53 skips the exact-double fast path, but 2^60 still fits in a long and is bound-checked. + IntegerSchema s = + new IntegerSchema(Set.of(TypeName.INTEGER), 0L, 1000L, null, null, null, null, Map.of()); + assertThatThrownBy(() -> v.validate(BigInteger.TWO.pow(60), s, "/v")) + .extracting(t -> ((ValidationException) t).error().keyword()) + .isEqualTo("maximum"); + } + + @Test + void integerOverflowingLongIsCheckedAgainstBoundsAndFormat() { + BigInteger huge = BigInteger.TWO.pow(64).add(BigInteger.valueOf(50)); // > Long.MAX + BigInteger hugeNegative = huge.negate(); // < Long.MIN + + IntegerSchema max = + new IntegerSchema(Set.of(TypeName.INTEGER), 0L, 1000L, null, null, null, null, Map.of()); + assertThatThrownBy(() -> v.validate(huge, max, "/v")) + .extracting(t -> ((ValidationException) t).error().keyword()) + .isEqualTo("maximum"); + + IntegerSchema exclMax = + new IntegerSchema(Set.of(TypeName.INTEGER), null, null, null, 1000L, null, null, Map.of()); + assertThatThrownBy(() -> v.validate(huge, exclMax, "/v")) + .extracting(t -> ((ValidationException) t).error().keyword()) + .isEqualTo("maximum"); + + IntegerSchema min = + new IntegerSchema(Set.of(TypeName.INTEGER), 0L, null, null, null, null, null, Map.of()); + assertThatThrownBy(() -> v.validate(hugeNegative, min, "/v")) + .extracting(t -> ((ValidationException) t).error().keyword()) + .isEqualTo("minimum"); + + IntegerSchema exclMin = + new IntegerSchema(Set.of(TypeName.INTEGER), null, null, 0L, null, null, null, Map.of()); + assertThatThrownBy(() -> v.validate(hugeNegative, exclMin, "/v")) + .extracting(t -> ((ValidationException) t).error().keyword()) + .isEqualTo("minimum"); + + IntegerSchema int64 = + new IntegerSchema( + Set.of(TypeName.INTEGER), null, null, null, null, null, "int64", Map.of()); + assertThatThrownBy(() -> v.validate(huge, int64, "/v")) + .extracting(t -> ((ValidationException) t).error().keyword()) + .isEqualTo("format"); + + IntegerSchema unbounded = + new IntegerSchema(Set.of(TypeName.INTEGER), null, null, null, null, null, null, Map.of()); + assertThatCode(() -> v.validate(huge, unbounded, "/v")).doesNotThrowAnyException(); + } + @Test void numberAcceptsDoublesAndIntegers() { NumberSchema s = From 1bbc2fef7302ba3cdfe15d580c6163d09f9a06ff Mon Sep 17 00:00:00 2001 From: Thomas Cederholm Date: Wed, 1 Jul 2026 17:35:37 +0200 Subject: [PATCH 3/4] 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. --- .../retailsvc/http/validate/DefaultValidator.java | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/main/java/com/retailsvc/http/validate/DefaultValidator.java b/src/main/java/com/retailsvc/http/validate/DefaultValidator.java index 12ba49be..16efe008 100644 --- a/src/main/java/com/retailsvc/http/validate/DefaultValidator.java +++ b/src/main/java/com/retailsvc/http/validate/DefaultValidator.java @@ -367,8 +367,6 @@ private static boolean isHextet(String hextet) { } /** Largest magnitude an IEEE-754 double represents as an exact integer (2^53). */ - private static final double MAX_EXACT_INTEGRAL_DOUBLE = 9007199254740992.0; - private static Optional checkInteger( Object value, IntegerSchema s, String pointer) { if (!(value instanceof Number num)) { @@ -382,17 +380,13 @@ private static Optional checkInteger( || num instanceof Byte) { return checkIntegerBounds(num.longValue(), s, pointer); } - // Integral doubles small enough to be represented exactly also take the long path. - double d = num.doubleValue(); - if (Double.isFinite(d) && d == Math.rint(d) && Math.abs(d) <= MAX_EXACT_INTEGRAL_DOUBLE) { - return checkIntegerBounds((long) d, s, pointer); - } - // Non-integral (e.g. 4.9), non-finite, or beyond a double's exact-integer range: verify the - // value is a whole number and compare the full magnitude so nothing wraps past a bound. + // 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 e) { + } catch (ArithmeticException | NumberFormatException notIntegral) { return err(pointer, "type", "expected integer", value); } return checkIntegerBounds(n, s, pointer); From 1ec3c422b9ac07d5b5fb327156696f3ff2fc0294 Mon Sep 17 00:00:00 2001 From: Thomas Cederholm Date: Thu, 2 Jul 2026 08:02:40 +0200 Subject: [PATCH 4/4] refactor: Resolve SonarCloud new-code issues - Extract MINIMUM_KEYWORD/MAXIMUM_KEYWORD constants (S1192: each literal now appears 3 times across the integer and number checks). - Use an unnamed catch variable for the discarded parse exception. - Replace try/catch + fail() with assertDoesNotThrow() in the query test. --- .../http/validate/DefaultValidator.java | 16 ++++---- .../http/QueryParamSmugglingTest.java | 40 +++++++++++-------- 2 files changed, 32 insertions(+), 24 deletions(-) diff --git a/src/main/java/com/retailsvc/http/validate/DefaultValidator.java b/src/main/java/com/retailsvc/http/validate/DefaultValidator.java index 16efe008..25de7d13 100644 --- a/src/main/java/com/retailsvc/http/validate/DefaultValidator.java +++ b/src/main/java/com/retailsvc/http/validate/DefaultValidator.java @@ -48,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 OK = Optional.empty(); private record FormatCheck(Predicate isValid, String message) {} @@ -386,7 +388,7 @@ private static Optional checkInteger( BigInteger n; try { n = new BigDecimal(num.toString()).toBigIntegerExact(); - } catch (ArithmeticException | NumberFormatException notIntegral) { + } catch (ArithmeticException | NumberFormatException _) { return err(pointer, "type", "expected integer", value); } return checkIntegerBounds(n, s, pointer); @@ -395,10 +397,10 @@ private static Optional checkInteger( private static Optional 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( @@ -424,10 +426,10 @@ private static Optional checkIntegerBounds( // 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", "integer out of range", n); + return err(pointer, MAXIMUM_KEYWORD, "integer out of range", n); } if (n.signum() < 0 && (s.minimum() != null || s.exclusiveMinimum() != null)) { - return err(pointer, "minimum", "integer out of range", n); + 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); @@ -443,10 +445,10 @@ private static Optional 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); diff --git a/src/test/java/com/retailsvc/http/QueryParamSmugglingTest.java b/src/test/java/com/retailsvc/http/QueryParamSmugglingTest.java index 77a54732..e8a85aa2 100644 --- a/src/test/java/com/retailsvc/http/QueryParamSmugglingTest.java +++ b/src/test/java/com/retailsvc/http/QueryParamSmugglingTest.java @@ -2,7 +2,7 @@ import static java.net.http.HttpRequest.BodyPublishers.noBody; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.fail; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import com.retailsvc.http.spec.Spec; import java.io.InputStream; @@ -32,21 +32,27 @@ void encodedAmpersandCannotSmuggleAValuePastQueryValidation() throws Exception { @SuppressWarnings("unchecked") private void constrainQ1ToLowercaseLetters() { - try (InputStream in = QueryParamSmugglingTest.class.getResourceAsStream("/openapi.json")) { - Map raw = - (Map) - gson.fromJson(new String(in.readAllBytes(), StandardCharsets.UTF_8), Map.class); - var paths = (Map) raw.get("paths"); - var op = (Map) ((Map) paths.get("/params/query")).get("get"); - for (Object p : (List) op.get("parameters")) { - var param = (Map) p; - if ("q1".equals(param.get("name"))) { - ((Map) param.get("schema")).put("pattern", "^[a-z]+$"); - } - } - spec = Spec.from(raw); - } catch (Exception e) { - fail(e); - } + spec = + assertDoesNotThrow( + () -> { + try (InputStream in = + QueryParamSmugglingTest.class.getResourceAsStream("/openapi.json")) { + Map raw = + (Map) + gson.fromJson( + new String(in.readAllBytes(), StandardCharsets.UTF_8), Map.class); + var paths = (Map) raw.get("paths"); + var op = + (Map) + ((Map) paths.get("/params/query")).get("get"); + for (Object p : (List) op.get("parameters")) { + var param = (Map) p; + if ("q1".equals(param.get("name"))) { + ((Map) param.get("schema")).put("pattern", "^[a-z]+$"); + } + } + return Spec.from(raw); + } + }); } }