Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 2 additions & 23 deletions src/main/java/com/retailsvc/http/Request.java
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -265,7 +263,7 @@ public String rawQuery() {
*/
public Map<String, String> queryParams() {
if (queryParamCache == null) {
queryParamCache = parseQuery(rawQuery);
queryParamCache = QueryParams.parse(rawQuery);
}
return queryParamCache;
}
Expand Down Expand Up @@ -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;
}
}
42 changes: 42 additions & 0 deletions src/main/java/com/retailsvc/http/internal/QueryParams.java
Original file line number Diff line number Diff line change
@@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()) {
Expand Down Expand Up @@ -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;
}
}
53 changes: 48 additions & 5 deletions src/main/java/com/retailsvc/http/validate/DefaultValidator.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) {}
Expand Down Expand Up @@ -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(
Expand All @@ -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)) {
Expand All @@ -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);
Expand Down
58 changes: 58 additions & 0 deletions src/test/java/com/retailsvc/http/QueryParamSmugglingTest.java
Original file line number Diff line number Diff line change
@@ -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);
}
});
}
}
34 changes: 34 additions & 0 deletions src/test/java/com/retailsvc/http/internal/QueryParamsTest.java
Original file line number Diff line number Diff line change
@@ -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"));
}
}
Loading
Loading