Skip to content

Commit 713f6de

Browse files
thcedthced
authored
fix: Reject string-to-number coercion in type validation (#48)
* fix: Reject string-to-number coercion in type validation JSON Schema 'type' refers to the JSON value's intrinsic kind, not whether its lexical form is parseable to another type. DefaultValidator was parsing numeric-looking strings as integers/numbers, which broke oneOf branches mixing string and number (both matched) and silently accepted strings for plain {"type": "number"} fields. Validator is now strict; parameter values (always strings on the wire) are coerced to the schema's primitive type in RequestPreparationFilter before validation runs. * test: Static-import Mockito.mock and assert chain invocation Reduces visual noise at mock call sites and addresses three Sonar 'add at least one assertion' findings in the new coercion happy-path tests by verifying the downstream chain was invoked.
1 parent 1a223af commit 713f6de

7 files changed

Lines changed: 218 additions & 46 deletions

File tree

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

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,10 @@
1111
import com.retailsvc.http.spec.Parameter;
1212
import com.retailsvc.http.spec.RequestBody;
1313
import com.retailsvc.http.spec.Spec;
14+
import com.retailsvc.http.spec.schema.BooleanSchema;
15+
import com.retailsvc.http.spec.schema.IntegerSchema;
16+
import com.retailsvc.http.spec.schema.NumberSchema;
17+
import com.retailsvc.http.spec.schema.Schema;
1418
import com.retailsvc.http.validate.ValidationError;
1519
import com.retailsvc.http.validate.Validator;
1620
import com.sun.net.httpserver.Filter;
@@ -124,10 +128,48 @@ private void validateParameters(
124128
}
125129
continue;
126130
}
127-
validator.validate(value, p.schema(), pointer);
131+
validator.validate(coerceParameterValue(value, p.schema(), pointer), p.schema(), pointer);
128132
}
129133
}
130134

135+
/**
136+
* Converts a raw parameter string into a typed value matching the schema's primitive kind, so the
137+
* validator (which is faithful to JSON Schema {@code type} semantics) sees the value the spec
138+
* describes rather than its string serialization. Strings that fail to parse are passed through
139+
* unchanged so the validator surfaces a {@code type} error with the original input.
140+
*/
141+
private static Object coerceParameterValue(String raw, Schema schema, String pointer) {
142+
return switch (schema) {
143+
case IntegerSchema _ -> {
144+
try {
145+
yield Long.parseLong(raw);
146+
} catch (NumberFormatException _) {
147+
throw new ValidationException(
148+
new ValidationError(pointer, "type", "expected integer", raw));
149+
}
150+
}
151+
case NumberSchema _ -> {
152+
try {
153+
yield Double.parseDouble(raw);
154+
} catch (NumberFormatException _) {
155+
throw new ValidationException(
156+
new ValidationError(pointer, "type", "expected number", raw));
157+
}
158+
}
159+
case BooleanSchema _ -> {
160+
if ("true".equals(raw)) {
161+
yield Boolean.TRUE;
162+
}
163+
if ("false".equals(raw)) {
164+
yield Boolean.FALSE;
165+
}
166+
throw new ValidationException(
167+
new ValidationError(pointer, "type", "expected boolean", raw));
168+
}
169+
default -> raw;
170+
};
171+
}
172+
131173
private Object validateAndParseBody(HttpExchange exchange, Operation op, byte[] body) {
132174
Optional<RequestBody> rb = op.requestBody();
133175
if (rb.isEmpty()) {

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

Lines changed: 8 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -347,22 +347,11 @@ private static boolean isHextet(String hextet) {
347347
}
348348

349349
private void validateInteger(Object value, IntegerSchema s, String pointer) {
350-
long n;
351-
switch (value) {
352-
case Number num -> n = num.longValue();
353-
case String str -> {
354-
try {
355-
n = Long.parseLong(str);
356-
} catch (NumberFormatException _) {
357-
fail(pointer, "type", "expected integer", value);
358-
return;
359-
}
360-
}
361-
case null, default -> {
362-
fail(pointer, "type", "expected integer", value);
363-
return;
364-
}
350+
if (!(value instanceof Number num)) {
351+
fail(pointer, "type", "expected integer", value);
352+
return;
365353
}
354+
long n = num.longValue();
366355

367356
if (s.minimum() != null && n < s.minimum()) {
368357
fail(pointer, "minimum", "integer below minimum " + s.minimum(), n);
@@ -385,22 +374,11 @@ private void validateInteger(Object value, IntegerSchema s, String pointer) {
385374
}
386375

387376
private void validateNumber(Object value, NumberSchema s, String pointer) {
388-
double n;
389-
switch (value) {
390-
case Number num -> n = num.doubleValue();
391-
case String str -> {
392-
try {
393-
n = Double.parseDouble(str);
394-
} catch (NumberFormatException _) {
395-
fail(pointer, "type", "expected number", value);
396-
return;
397-
}
398-
}
399-
case null, default -> {
400-
fail(pointer, "type", "expected number", value);
401-
return;
402-
}
377+
if (!(value instanceof Number num)) {
378+
fail(pointer, "type", "expected number", value);
379+
return;
403380
}
381+
double n = num.doubleValue();
404382

405383
if (s.minimum() != null && n < s.minimum().doubleValue()) {
406384
fail(pointer, "minimum", "number below minimum " + s.minimum(), n);

‎src/test/java/com/retailsvc/http/HandlersDefaultExceptionTest.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package com.retailsvc.http;
22

33
import static org.assertj.core.api.Assertions.assertThat;
4+
import static org.mockito.Mockito.mock;
45

56
import com.retailsvc.http.spec.HttpMethod;
67
import com.retailsvc.http.validate.ValidationError;
@@ -13,7 +14,7 @@
1314

1415
class HandlersDefaultExceptionTest {
1516
private HttpExchange newExchange(ByteArrayOutputStream sink) {
16-
HttpExchange ex = Mockito.mock(HttpExchange.class);
17+
HttpExchange ex = mock(HttpExchange.class);
1718
Mockito.when(ex.getResponseHeaders()).thenReturn(new Headers());
1819
Mockito.when(ex.getResponseBody()).thenReturn(sink);
1920
return ex;

‎src/test/java/com/retailsvc/http/internal/DispatchHandlerTest.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package com.retailsvc.http.internal;
22

33
import static org.assertj.core.api.Assertions.assertThatThrownBy;
4+
import static org.mockito.Mockito.mock;
45

56
import com.retailsvc.http.MissingOperationHandlerException;
67
import com.retailsvc.http.Request;
@@ -20,8 +21,8 @@ private static void withOperationId(
2021

2122
@Test
2223
void invokesRegisteredHandler() throws Exception {
23-
HttpHandler handler = Mockito.mock(HttpHandler.class);
24-
HttpExchange ex = Mockito.mock(HttpExchange.class);
24+
HttpHandler handler = mock(HttpHandler.class);
25+
HttpExchange ex = mock(HttpExchange.class);
2526

2627
withOperationId(
2728
"get-x",
@@ -37,7 +38,7 @@ void invokesRegisteredHandler() throws Exception {
3738
@Test
3839
void throwsWhenHandlerMissing() {
3940
DispatchHandler d = new DispatchHandler(Map.of());
40-
HttpExchange ex = Mockito.mock(HttpExchange.class);
41+
HttpExchange ex = mock(HttpExchange.class);
4142

4243
assertThatThrownBy(
4344
() ->

‎src/test/java/com/retailsvc/http/internal/ExceptionFilterTest.java‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package com.retailsvc.http.internal;
22

3+
import static org.mockito.Mockito.mock;
4+
35
import com.retailsvc.http.ExceptionHandler;
46
import com.retailsvc.http.NotFoundException;
57
import com.sun.net.httpserver.Filter;
@@ -10,21 +12,21 @@
1012
class ExceptionFilterTest {
1113
@Test
1214
void delegatesToExceptionHandler() throws Exception {
13-
HttpExchange ex = Mockito.mock(HttpExchange.class);
14-
ExceptionHandler handler = Mockito.mock(ExceptionHandler.class);
15+
HttpExchange ex = mock(HttpExchange.class);
16+
ExceptionHandler handler = mock(ExceptionHandler.class);
1517
Filter f = new ExceptionFilter(handler);
16-
Filter.Chain chain = Mockito.mock(Filter.Chain.class);
18+
Filter.Chain chain = mock(Filter.Chain.class);
1719
Mockito.doThrow(new NotFoundException("x")).when(chain).doFilter(ex);
1820
f.doFilter(ex, chain);
1921
Mockito.verify(handler).handle(Mockito.eq(ex), Mockito.any(NotFoundException.class));
2022
}
2123

2224
@Test
2325
void passThroughOnSuccess() throws Exception {
24-
HttpExchange ex = Mockito.mock(HttpExchange.class);
25-
ExceptionHandler handler = Mockito.mock(ExceptionHandler.class);
26+
HttpExchange ex = mock(HttpExchange.class);
27+
ExceptionHandler handler = mock(ExceptionHandler.class);
2628
Filter f = new ExceptionFilter(handler);
27-
Filter.Chain chain = Mockito.mock(Filter.Chain.class);
29+
Filter.Chain chain = mock(Filter.Chain.class);
2830
f.doFilter(ex, chain);
2931
Mockito.verify(chain).doFilter(ex);
3032
Mockito.verifyNoInteractions(handler);

‎src/test/java/com/retailsvc/http/internal/RequestPreparationFilterTest.java‎

Lines changed: 136 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import static org.assertj.core.api.Assertions.assertThat;
44
import static org.assertj.core.api.Assertions.assertThatThrownBy;
5+
import static org.mockito.Mockito.mock;
56

67
import com.retailsvc.http.JsonMapper;
78
import com.retailsvc.http.MethodNotAllowedException;
@@ -15,6 +16,9 @@
1516
import com.retailsvc.http.spec.PathTemplate;
1617
import com.retailsvc.http.spec.Server;
1718
import com.retailsvc.http.spec.Spec;
19+
import com.retailsvc.http.spec.schema.BooleanSchema;
20+
import com.retailsvc.http.spec.schema.IntegerSchema;
21+
import com.retailsvc.http.spec.schema.NumberSchema;
1822
import com.retailsvc.http.spec.schema.StringSchema;
1923
import com.retailsvc.http.spec.schema.TypeName;
2024
import com.retailsvc.http.validate.DefaultValidator;
@@ -34,7 +38,7 @@
3438
class RequestPreparationFilterTest {
3539

3640
private HttpExchange exchange(String method, String path, byte[] body) {
37-
HttpExchange ex = Mockito.mock(HttpExchange.class);
41+
HttpExchange ex = mock(HttpExchange.class);
3842
Mockito.when(ex.getRequestMethod()).thenReturn(method);
3943
Mockito.when(ex.getRequestURI()).thenReturn(URI.create(path));
4044
Mockito.when(ex.getRequestHeaders()).thenReturn(new Headers());
@@ -80,7 +84,7 @@ void successPathBindsRequestContextDuringChain() throws Exception {
8084
AtomicReference<String> seenOpId = new AtomicReference<>();
8185
AtomicReference<Map<String, String>> seenPathParams = new AtomicReference<>();
8286

83-
Filter.Chain chain = Mockito.mock(Filter.Chain.class);
87+
Filter.Chain chain = mock(Filter.Chain.class);
8488
Mockito.doAnswer(
8589
inv -> {
8690
seenOpId.set(Request.operationId());
@@ -112,7 +116,7 @@ void unknownPathThrowsNotFound() {
112116
Filter f = newFilter(spec);
113117

114118
HttpExchange ex = exchange("GET", "/missing", new byte[0]);
115-
assertThatThrownBy(() -> f.doFilter(ex, Mockito.mock(Filter.Chain.class)))
119+
assertThatThrownBy(() -> f.doFilter(ex, mock(Filter.Chain.class)))
116120
.isInstanceOf(NotFoundException.class);
117121
}
118122

@@ -131,7 +135,7 @@ void wrongMethodThrowsMethodNotAllowed() {
131135
Filter f = newFilter(spec);
132136

133137
HttpExchange ex = exchange("POST", "/x", new byte[0]);
134-
assertThatThrownBy(() -> f.doFilter(ex, Mockito.mock(Filter.Chain.class)))
138+
assertThatThrownBy(() -> f.doFilter(ex, mock(Filter.Chain.class)))
135139
.isInstanceOf(MethodNotAllowedException.class);
136140
}
137141

@@ -152,9 +156,136 @@ void invalidQueryParamThrowsValidation() {
152156
Filter f = newFilter(spec);
153157

154158
HttpExchange ex = exchange("GET", "/x?q=ab", new byte[0]);
155-
assertThatThrownBy(() -> f.doFilter(ex, Mockito.mock(Filter.Chain.class)))
159+
assertThatThrownBy(() -> f.doFilter(ex, mock(Filter.Chain.class)))
156160
.isInstanceOf(ValidationException.class)
157161
.extracting(t -> ((ValidationException) t).error().pointer())
158162
.isEqualTo("/query/q");
159163
}
164+
165+
@Test
166+
void integerQueryParamIsCoercedFromStringBeforeValidation() throws Exception {
167+
var intSchema = new IntegerSchema(Set.of(TypeName.INTEGER), 1L, 100L, null, null, null, null);
168+
var op =
169+
new Operation(
170+
"a",
171+
HttpMethod.GET,
172+
PathTemplate.compile("/x"),
173+
Optional.empty(),
174+
List.of(new Parameter("n", Parameter.Location.QUERY, true, intSchema)),
175+
Map.of());
176+
Spec spec = specWith(op);
177+
Filter f = newFilter(spec);
178+
179+
HttpExchange ex = exchange("GET", "/x?n=42", new byte[0]);
180+
Filter.Chain chain = mock(Filter.Chain.class);
181+
f.doFilter(ex, chain);
182+
Mockito.verify(chain).doFilter(ex);
183+
}
184+
185+
@Test
186+
void integerQueryParamRejectsNonNumericString() {
187+
var intSchema = new IntegerSchema(Set.of(TypeName.INTEGER), null, null, null, null, null, null);
188+
var op =
189+
new Operation(
190+
"a",
191+
HttpMethod.GET,
192+
PathTemplate.compile("/x"),
193+
Optional.empty(),
194+
List.of(new Parameter("n", Parameter.Location.QUERY, true, intSchema)),
195+
Map.of());
196+
Spec spec = specWith(op);
197+
Filter f = newFilter(spec);
198+
199+
HttpExchange ex = exchange("GET", "/x?n=abc", new byte[0]);
200+
assertThatThrownBy(() -> f.doFilter(ex, mock(Filter.Chain.class)))
201+
.isInstanceOf(ValidationException.class)
202+
.extracting(t -> ((ValidationException) t).error().keyword())
203+
.isEqualTo("type");
204+
}
205+
206+
@Test
207+
void numberQueryParamIsCoercedFromStringBeforeValidation() throws Exception {
208+
var numSchema = new NumberSchema(Set.of(TypeName.NUMBER), null, null, null, null, null, null);
209+
var op =
210+
new Operation(
211+
"a",
212+
HttpMethod.GET,
213+
PathTemplate.compile("/x"),
214+
Optional.empty(),
215+
List.of(new Parameter("n", Parameter.Location.QUERY, true, numSchema)),
216+
Map.of());
217+
Spec spec = specWith(op);
218+
Filter f = newFilter(spec);
219+
220+
HttpExchange ex = exchange("GET", "/x?n=1.5", new byte[0]);
221+
Filter.Chain chain = mock(Filter.Chain.class);
222+
f.doFilter(ex, chain);
223+
Mockito.verify(chain).doFilter(ex);
224+
}
225+
226+
@Test
227+
void numberQueryParamRejectsNonNumericString() {
228+
var numSchema = new NumberSchema(Set.of(TypeName.NUMBER), null, null, null, null, null, null);
229+
var op =
230+
new Operation(
231+
"a",
232+
HttpMethod.GET,
233+
PathTemplate.compile("/x"),
234+
Optional.empty(),
235+
List.of(new Parameter("n", Parameter.Location.QUERY, true, numSchema)),
236+
Map.of());
237+
Spec spec = specWith(op);
238+
Filter f = newFilter(spec);
239+
240+
HttpExchange ex = exchange("GET", "/x?n=abc", new byte[0]);
241+
assertThatThrownBy(() -> f.doFilter(ex, mock(Filter.Chain.class)))
242+
.isInstanceOf(ValidationException.class)
243+
.extracting(t -> ((ValidationException) t).error().keyword())
244+
.isEqualTo("type");
245+
}
246+
247+
@Test
248+
void booleanQueryParamCoercesTrueAndFalse() throws Exception {
249+
var boolSchema = new BooleanSchema(Set.of(TypeName.BOOLEAN));
250+
var op =
251+
new Operation(
252+
"a",
253+
HttpMethod.GET,
254+
PathTemplate.compile("/x"),
255+
Optional.empty(),
256+
List.of(new Parameter("b", Parameter.Location.QUERY, true, boolSchema)),
257+
Map.of());
258+
Spec spec = specWith(op);
259+
Filter f = newFilter(spec);
260+
261+
Filter.Chain trueChain = mock(Filter.Chain.class);
262+
Filter.Chain falseChain = mock(Filter.Chain.class);
263+
HttpExchange trueEx = exchange("GET", "/x?b=true", new byte[0]);
264+
HttpExchange falseEx = exchange("GET", "/x?b=false", new byte[0]);
265+
f.doFilter(trueEx, trueChain);
266+
f.doFilter(falseEx, falseChain);
267+
Mockito.verify(trueChain).doFilter(trueEx);
268+
Mockito.verify(falseChain).doFilter(falseEx);
269+
}
270+
271+
@Test
272+
void booleanQueryParamRejectsNonBooleanString() {
273+
var boolSchema = new BooleanSchema(Set.of(TypeName.BOOLEAN));
274+
var op =
275+
new Operation(
276+
"a",
277+
HttpMethod.GET,
278+
PathTemplate.compile("/x"),
279+
Optional.empty(),
280+
List.of(new Parameter("b", Parameter.Location.QUERY, true, boolSchema)),
281+
Map.of());
282+
Spec spec = specWith(op);
283+
Filter f = newFilter(spec);
284+
285+
HttpExchange ex = exchange("GET", "/x?b=yes", new byte[0]);
286+
assertThatThrownBy(() -> f.doFilter(ex, mock(Filter.Chain.class)))
287+
.isInstanceOf(ValidationException.class)
288+
.extracting(t -> ((ValidationException) t).error().keyword())
289+
.isEqualTo("type");
290+
}
160291
}

0 commit comments

Comments
 (0)