Skip to content

Commit

Permalink
Fix to send 400 from gRPC HTTP/JSON transcoding service when the requ…
Browse files Browse the repository at this point in the history 
…est is invalid (#5307)

Motivation:

Malformed json requests continued with an empty request instead of returning an error.

Modifications:

Throw an illegal argument exception when json processing fails instead of ignoring it. The surrounding service will translate it into a BAD_REQUEST response.

Result:

- Closes #5306 
- Requests with a malformed json body will now return a BAD_REQUEST status
  • Loading branch information
@mraasvel
mraasvel authored Dec 8, 2023
1 parent 6477e89 commit efca3c5
Show file tree
Hide file tree
Showing 2 changed files with 92 additions and 21 deletions.
12 changes: 9 additions & 3 deletions grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -610,8 +610,11 @@ private HttpData convertToJson(ServiceRequestContext ctx,
final ObjectNode root;
if (body instanceof ObjectNode) {
root = (ObjectNode) body;
} else {
} else if (body == null) {
root = mapper.createObjectNode();
} else {
throw new IllegalArgumentException("Unexpected JSON: " +
body + ", (expected: ObjectNode or null).");
}
return setParametersAndWriteJson(root, ctx, spec);
}
Expand Down Expand Up @@ -646,12 +649,15 @@ private static JsonNode getBodyContent(AggregatedHttpRequest request) {
@Nullable
final MediaType contentType = request.contentType();
if (contentType == null || !contentType.isJson()) {
return null;
if (request.content().isEmpty()) {
return null;
}
throw new IllegalArgumentException("Missing or invalid content-type in JSON request.");
}
try {
return mapper.readTree(request.contentUtf8());
} catch (JsonProcessingException e) {
return null;
throw new IllegalArgumentException("Failed to parse JSON request.", e);
}
}

Expand Down
101 changes: 83 additions & 18 deletions grpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -264,10 +264,10 @@ public void echoRecursive2(Recursive request,

static EchoResponseBodyResponse getResponseBodyResponse(EchoResponseBodyRequest request) {
return EchoResponseBodyResponse.newBuilder()
.setValue(request.getValue())
.addAllArrayField(request.getArrayFieldList())
.setStructBody(request.getStructBody())
.build();
.setValue(request.getValue())
.addAllArrayField(request.getArrayFieldList())
.setStructBody(request.getStructBody())
.build();
}

@Override
Expand Down Expand Up @@ -734,17 +734,17 @@ void shouldAcceptResponseBodyValue() {
final QueryParamsBuilder query = QueryParams.builder();
query.add("value", "value");
final AggregatedHttpResponse response = webClient.get("/v1/echo/response_body/value?" +
query.toQueryString())
.aggregate().join();
query.toQueryString())
.aggregate().join();
assertThat(response.contentType()).isEqualTo(MediaType.JSON_UTF_8);
assertThat(response.contentUtf8()).isEqualTo("\"value\"");
}

@Test
void shouldAcceptResponseBodyRepeated() throws JsonProcessingException {
final AggregatedHttpResponse response = webClient.get(
"/v1/echo/response_body/repeated?array_field=value1&array_field=value2")
.aggregate().join();
final AggregatedHttpResponse response =
webClient.get("/v1/echo/response_body/repeated?array_field=value1&array_field=value2")
.aggregate().join();
final JsonNode root = mapper.readTree(response.contentUtf8());
assertThat(response.contentType()).isEqualTo(MediaType.JSON_UTF_8);
assertThat(root.isArray()).isTrue();
Expand All @@ -754,9 +754,9 @@ void shouldAcceptResponseBodyRepeated() throws JsonProcessingException {
@Test
void shouldAcceptResponseBodyValueStruct() throws JsonProcessingException {
final String jsonContent = "{\"value\":\"value\",\"structBody\":{\"structBody\":\"struct_value\"}," +
"\"arrayField\":[\"value1\",\"value2\"]}";
"\"arrayField\":[\"value1\",\"value2\"]}";
final AggregatedHttpResponse response = jsonPostRequest(webClient,
"/v1/echo/response_body/struct", jsonContent);
"/v1/echo/response_body/struct", jsonContent);
final JsonNode root = mapper.readTree(response.contentUtf8());
assertThat(response.contentType()).isEqualTo(MediaType.JSON_UTF_8);
assertThat(root.has("structBody")).isTrue();
Expand All @@ -766,9 +766,9 @@ void shouldAcceptResponseBodyValueStruct() throws JsonProcessingException {
@Test
void shouldAcceptResponseBodyValueNullValue() throws JsonProcessingException {
final String jsonContent = "{\"value\":\"value\"," +
"\"arrayField\":[\"value1\",\"value2\"]}";
"\"arrayField\":[\"value1\",\"value2\"]}";
final AggregatedHttpResponse response = jsonPostRequest(webClient,
"/v1/echo/response_body/struct", jsonContent);
"/v1/echo/response_body/struct", jsonContent);
final JsonNode root = mapper.readTree(response.contentUtf8());
assertThat(response.contentType()).isEqualTo(MediaType.JSON_UTF_8);
assertThat(root.isEmpty()).isTrue();
Expand All @@ -777,9 +777,9 @@ void shouldAcceptResponseBodyValueNullValue() throws JsonProcessingException {
@Test
void shouldAcceptResponseBodyValueAnonymusField() throws JsonProcessingException {
final String jsonContent = "{\"value\":\"value\",\"structBody\":{\"structBody\":\"struct_value\"}" +
",\"arrayField\":[\"value1\",\"value2\"]}";
",\"arrayField\":[\"value1\",\"value2\"]}";
final AggregatedHttpResponse response = jsonPostRequest(webClient,
"/v1/echo/response_body/nomatch", jsonContent);
"/v1/echo/response_body/nomatch", jsonContent);
final JsonNode root = mapper.readTree(response.contentUtf8());
assertThat(response.contentType()).isEqualTo(MediaType.JSON_UTF_8);
assertThatJson(root).isEqualTo("{\"value\":\"value\"," +
Expand All @@ -789,13 +789,78 @@ void shouldAcceptResponseBodyValueAnonymusField() throws JsonProcessingException

@Test
void shouldAcceptResponseBodyValueNoMatchInside() throws JsonProcessingException {
final String jsonContent = "{\"value\":\"value\",\"structBody\":{\"structBody\":\"struct_value\"}";
final AggregatedHttpResponse response = jsonPostRequest(webClient,
"/v1/echo/response_body/repeated", jsonContent);
final String jsonContent = "{\"value\":\"value\",\"structBody\":{\"structBody\":\"struct_value\"} }";
final AggregatedHttpResponse response = jsonPostRequest(
webClient, "/v1/echo/response_body/repeated", jsonContent);
assertThat(response.contentType()).isEqualTo(MediaType.JSON_UTF_8);
assertThat(response.contentUtf8()).isEqualTo("null");
}

@Test
void shouldDenyMalformedJson() throws JsonProcessingException {
final String jsonContent = "{\"value\":\"value\",\"structBody\":{\"structBody\":\"struct_value\"}";
final AggregatedHttpResponse response =
jsonPostRequest(webClient, "/v1/echo/response_body/repeated", jsonContent);
assertThat(response.status()).isEqualTo(HttpStatus.BAD_REQUEST);
}

@Test
void shouldDenyMissingContentType() {
final String validJson = "{\"value\":\"value\",\"structBody\":{\"structBody\":\"struct_value\"} }";
final RequestHeaders headers = RequestHeaders.builder()
.method(HttpMethod.POST)
.path("/v1/echo/response_body/repeated")
.build();
final AggregatedHttpResponse response = webClient.execute(headers, validJson).aggregate().join();
assertThat(response.status()).isEqualTo(HttpStatus.BAD_REQUEST);
}

@Test
void shouldDenyNonJsonContentType() {
final String validJson = "{\"value\":\"value\",\"structBody\":{\"structBody\":\"struct_value\"} }";
final RequestHeaders headers = RequestHeaders.builder()
.method(HttpMethod.POST)
.path("/v1/echo/response_body/repeated")
.contentType(MediaType.CSV_UTF_8)
.build();
final AggregatedHttpResponse response = webClient.execute(headers, validJson).aggregate().join();
assertThat(response.status()).isEqualTo(HttpStatus.BAD_REQUEST);
}

@Test
void shouldDenyEmptyJson() {
final String emptyJson = "";
final RequestHeaders headers = RequestHeaders.builder()
.method(HttpMethod.POST)
.path("/v1/echo/response_body/repeated")
.contentType(MediaType.JSON)
.build();
final AggregatedHttpResponse response = webClient.execute(headers, emptyJson).aggregate().join();
assertThat(response.status()).isEqualTo(HttpStatus.BAD_REQUEST);
}

@Test
void shouldAcceptEmptyNonJson() {
final String body = "";
final RequestHeaders headers = RequestHeaders.builder()
.method(HttpMethod.POST)
.path("/v1/echo/response_body/repeated")
.build();
final AggregatedHttpResponse response = webClient.execute(headers, body).aggregate().join();
assertThat(response.status()).isEqualTo(HttpStatus.OK);
}

@Test
void shouldDenyNonObjectJson() {
final String body = "[ 42, null ]";
final RequestHeaders headers = RequestHeaders.builder()
.method(HttpMethod.POST)
.path("/v1/echo/response_body/repeated")
.build();
final AggregatedHttpResponse response = webClient.execute(headers, body).aggregate().join();
assertThat(response.status()).isEqualTo(HttpStatus.BAD_REQUEST);
}

@Test
void shouldAcceptResponseBodyValueStructPreservingProtoFieldNames() throws JsonProcessingException {
final String jsonContent = "{\"value\":\"value\",\"structBody\":{\"structBody\":\"struct_value\"}," +
Expand Down

0 comments on commit efca3c5

Please sign in to comment.