From af693d3e45f3630227261cddfd8435f2fa7db94b Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Fri, 16 Feb 2024 10:16:39 -0500 Subject: [PATCH 1/6] validate request payload: ensure id is not specified --- src/main/java/io/cryostat/discovery/CustomDiscovery.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/main/java/io/cryostat/discovery/CustomDiscovery.java b/src/main/java/io/cryostat/discovery/CustomDiscovery.java index 0d94ada2a..1fb59a1a3 100644 --- a/src/main/java/io/cryostat/discovery/CustomDiscovery.java +++ b/src/main/java/io/cryostat/discovery/CustomDiscovery.java @@ -89,6 +89,11 @@ void onStart(@Observes StartupEvent evt) { @RolesAllowed("write") public Response create( Target target, @RestQuery boolean dryrun, @RestQuery boolean storeCredentials) { + if (target.id != null) { + return Response.status(Response.Status.CONFLICT.getStatusCode()) + .entity("ID field cannot be specified in the request body") + .build(); + } // TODO handle credentials embedded in JSON body return doV2Create(target, Optional.empty(), dryrun, storeCredentials); } From 187abedea6f1dd13364e28af9782099636ed4320 Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Thu, 22 Feb 2024 13:23:09 -0500 Subject: [PATCH 2/6] used JsonRequestFIlter --- .../java/io/cryostat/JsonRequestFilter.java | 51 +++++++++++++++++++ .../cryostat/discovery/CustomDiscovery.java | 5 -- 2 files changed, 51 insertions(+), 5 deletions(-) create mode 100644 src/main/java/io/cryostat/JsonRequestFilter.java diff --git a/src/main/java/io/cryostat/JsonRequestFilter.java b/src/main/java/io/cryostat/JsonRequestFilter.java new file mode 100644 index 000000000..857365349 --- /dev/null +++ b/src/main/java/io/cryostat/JsonRequestFilter.java @@ -0,0 +1,51 @@ +/* + * Copyright The Cryostat Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.cryostat; + +import java.io.BufferedReader; +import java.io.IOException; +import java.io.InputStreamReader; + +import jakarta.ws.rs.container.ContainerRequestContext; +import jakarta.ws.rs.container.ContainerRequestFilter; +import jakarta.ws.rs.core.Response; +import jakarta.ws.rs.ext.Provider; + +@Provider +public class JsonRequestFilter implements ContainerRequestFilter { + + @Override + public void filter(ContainerRequestContext requestContext) throws IOException { + if (requestContext.getMediaType() != null + && requestContext + .getMediaType() + .isCompatible(jakarta.ws.rs.core.MediaType.APPLICATION_JSON_TYPE)) { + BufferedReader reader = + new BufferedReader(new InputStreamReader(requestContext.getEntityStream())); + try { + String line = reader.readLine(); + if (line.contains("\"id\"")) { + requestContext.abortWith( + Response.status(Response.Status.BAD_REQUEST) + .entity("ID field cannot be specified in the request body") + .build()); + } + } finally { + reader.close(); + } + } + } +} diff --git a/src/main/java/io/cryostat/discovery/CustomDiscovery.java b/src/main/java/io/cryostat/discovery/CustomDiscovery.java index 1fb59a1a3..0d94ada2a 100644 --- a/src/main/java/io/cryostat/discovery/CustomDiscovery.java +++ b/src/main/java/io/cryostat/discovery/CustomDiscovery.java @@ -89,11 +89,6 @@ void onStart(@Observes StartupEvent evt) { @RolesAllowed("write") public Response create( Target target, @RestQuery boolean dryrun, @RestQuery boolean storeCredentials) { - if (target.id != null) { - return Response.status(Response.Status.CONFLICT.getStatusCode()) - .entity("ID field cannot be specified in the request body") - .build(); - } // TODO handle credentials embedded in JSON body return doV2Create(target, Optional.empty(), dryrun, storeCredentials); } From 0e8ab3666cef71aafcd98bc105359e2460601595 Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Wed, 6 Mar 2024 12:37:54 -0500 Subject: [PATCH 3/6] adjusted JsonRequestFilter class; added unit test --- .../java/io/cryostat/JsonRequestFilter.java | 49 +++++++---- .../io/cryostat/JsonRequestFilterTest.java | 82 +++++++++++++++++++ 2 files changed, 115 insertions(+), 16 deletions(-) create mode 100644 src/test/java/io/cryostat/JsonRequestFilterTest.java diff --git a/src/main/java/io/cryostat/JsonRequestFilter.java b/src/main/java/io/cryostat/JsonRequestFilter.java index 857365349..a5772c51f 100644 --- a/src/main/java/io/cryostat/JsonRequestFilter.java +++ b/src/main/java/io/cryostat/JsonRequestFilter.java @@ -15,37 +15,54 @@ */ package io.cryostat; -import java.io.BufferedReader; +import java.io.ByteArrayInputStream; import java.io.IOException; -import java.io.InputStreamReader; +import java.nio.charset.StandardCharsets; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; import jakarta.ws.rs.container.ContainerRequestContext; import jakarta.ws.rs.container.ContainerRequestFilter; +import jakarta.ws.rs.core.MediaType; import jakarta.ws.rs.core.Response; import jakarta.ws.rs.ext.Provider; @Provider public class JsonRequestFilter implements ContainerRequestFilter { + private final ObjectMapper objectMapper = new ObjectMapper(); + @Override public void filter(ContainerRequestContext requestContext) throws IOException { if (requestContext.getMediaType() != null - && requestContext - .getMediaType() - .isCompatible(jakarta.ws.rs.core.MediaType.APPLICATION_JSON_TYPE)) { - BufferedReader reader = - new BufferedReader(new InputStreamReader(requestContext.getEntityStream())); - try { - String line = reader.readLine(); - if (line.contains("\"id\"")) { - requestContext.abortWith( - Response.status(Response.Status.BAD_REQUEST) - .entity("ID field cannot be specified in the request body") - .build()); + && requestContext.getMediaType().isCompatible(MediaType.APPLICATION_JSON_TYPE)) { + byte[] jsonData = requestContext.getEntityStream().readAllBytes(); + String json = new String(jsonData, StandardCharsets.UTF_8); + JsonNode rootNode = objectMapper.readTree(json); + + if (containsIdField(rootNode)) { + requestContext.abortWith( + Response.status(Response.Status.BAD_REQUEST) + .entity("ID field cannot be specified in the request body.") + .build()); + return; + } + + requestContext.setEntityStream(new ByteArrayInputStream(json.getBytes())); + } + } + + private boolean containsIdField(JsonNode node) { + if (node.has("id")) { + return true; + } + if (node.isContainerNode()) { + for (JsonNode child : node) { + if (containsIdField(child)) { + return true; } - } finally { - reader.close(); } } + return false; } } diff --git a/src/test/java/io/cryostat/JsonRequestFilterTest.java b/src/test/java/io/cryostat/JsonRequestFilterTest.java new file mode 100644 index 000000000..b0c03350b --- /dev/null +++ b/src/test/java/io/cryostat/JsonRequestFilterTest.java @@ -0,0 +1,82 @@ +/* + * Copyright The Cryostat Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.cryostat; + +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; + +import java.io.ByteArrayInputStream; +import java.nio.charset.StandardCharsets; + +import jakarta.ws.rs.container.ContainerRequestContext; +import jakarta.ws.rs.core.MediaType; +import jakarta.ws.rs.core.Response; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class JsonRequestFilterTest { + private JsonRequestFilter filter; + private ContainerRequestContext requestContext; + + @BeforeEach + void setUp() { + filter = new JsonRequestFilter(); + requestContext = mock(ContainerRequestContext.class); + } + + @Test + void testRejectsPayloadWithId() throws Exception { + + String[] testPayloads = { + "{\"id\": 1}", + "{\n \"id\": 1\n}", + "{\n \"foo\": \"bar\",\n \"id\": 1\n}", + "[\n {\n \"id\": 1\n }\n]" + }; + + for (String payload : testPayloads) { + simulateRequest(payload); + verify(requestContext, times(1)).abortWith(any(Response.class)); + reset(requestContext); + } + } + + @Test + void testAllowsPayloadWithoutId() throws Exception { + + String[] testPayloads = { + "{ \"message\": \"this text includes the string literal \\\"id\\\"\" }", + "{}", + "[]", + "{ \"foo\": \"bar\" }" + }; + + for (String payload : testPayloads) { + simulateRequest(payload); + verify(requestContext, never()).abortWith(any(Response.class)); + reset(requestContext); + } + } + + private void simulateRequest(String jsonPayload) throws Exception { + + ByteArrayInputStream payloadStream = + new ByteArrayInputStream(jsonPayload.getBytes(StandardCharsets.UTF_8)); + when(requestContext.getEntityStream()).thenReturn(payloadStream); + when(requestContext.getMediaType()).thenReturn(MediaType.APPLICATION_JSON_TYPE); + filter.filter(requestContext); + } +} From 7be94a10e7e203dafa452c2318635d2e0888c8cd Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Wed, 6 Mar 2024 13:00:39 -0500 Subject: [PATCH 4/6] explicitly use UTF-8 --- src/main/java/io/cryostat/JsonRequestFilter.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/io/cryostat/JsonRequestFilter.java b/src/main/java/io/cryostat/JsonRequestFilter.java index a5772c51f..798624d93 100644 --- a/src/main/java/io/cryostat/JsonRequestFilter.java +++ b/src/main/java/io/cryostat/JsonRequestFilter.java @@ -48,7 +48,8 @@ public void filter(ContainerRequestContext requestContext) throws IOException { return; } - requestContext.setEntityStream(new ByteArrayInputStream(json.getBytes())); + requestContext.setEntityStream( + new ByteArrayInputStream(json.getBytes(StandardCharsets.UTF_8))); } } From d1aac74a9727db730decc77a181b645dd29f4f49 Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Wed, 6 Mar 2024 20:49:46 -0500 Subject: [PATCH 5/6] refactor JsonRequestFilterTest: use @ParametarizedTest --- .../io/cryostat/JsonRequestFilterTest.java | 59 +++++++++---------- 1 file changed, 29 insertions(+), 30 deletions(-) diff --git a/src/test/java/io/cryostat/JsonRequestFilterTest.java b/src/test/java/io/cryostat/JsonRequestFilterTest.java index b0c03350b..d2668cde0 100644 --- a/src/test/java/io/cryostat/JsonRequestFilterTest.java +++ b/src/test/java/io/cryostat/JsonRequestFilterTest.java @@ -20,12 +20,14 @@ import java.io.ByteArrayInputStream; import java.nio.charset.StandardCharsets; +import java.util.stream.Stream; import jakarta.ws.rs.container.ContainerRequestContext; import jakarta.ws.rs.core.MediaType; import jakarta.ws.rs.core.Response; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; public class JsonRequestFilterTest { private JsonRequestFilter filter; @@ -37,42 +39,39 @@ void setUp() { requestContext = mock(ContainerRequestContext.class); } - @Test - void testRejectsPayloadWithId() throws Exception { - - String[] testPayloads = { - "{\"id\": 1}", - "{\n \"id\": 1\n}", - "{\n \"foo\": \"bar\",\n \"id\": 1\n}", - "[\n {\n \"id\": 1\n }\n]" - }; - - for (String payload : testPayloads) { - simulateRequest(payload); - verify(requestContext, times(1)).abortWith(any(Response.class)); - reset(requestContext); - } + static Stream payloadsReject() { + return Stream.of( + "{\"id\": 1}", + "{\n \"id\": 1\n}", + "{\n \"foo\": \"bar\",\n \"id\": 1\n}", + "[\n {\n \"id\": 1\n }\n]"); } - @Test - void testAllowsPayloadWithoutId() throws Exception { + @ParameterizedTest + @MethodSource("payloadsReject") + void testRejectsPayloadWithId(String payload) throws Exception { + simulateRequest(payload); + verify(requestContext, times(1)).abortWith(any(Response.class)); + reset(requestContext); + } - String[] testPayloads = { - "{ \"message\": \"this text includes the string literal \\\"id\\\"\" }", - "{}", - "[]", - "{ \"foo\": \"bar\" }" - }; + static Stream payloadsAllow() { + return Stream.of( + "{ \"message\": \"this text includes the string literal \\\"id\\\"\" }", + "{}", + "[]", + "{ \"foo\": \"bar\" }"); + } - for (String payload : testPayloads) { - simulateRequest(payload); - verify(requestContext, never()).abortWith(any(Response.class)); - reset(requestContext); - } + @ParameterizedTest + @MethodSource("payloadsAllow") + void testAllowsPayloadWithoutId(String payload) throws Exception { + simulateRequest(payload); + verify(requestContext, never()).abortWith(any(Response.class)); + reset(requestContext); } private void simulateRequest(String jsonPayload) throws Exception { - ByteArrayInputStream payloadStream = new ByteArrayInputStream(jsonPayload.getBytes(StandardCharsets.UTF_8)); when(requestContext.getEntityStream()).thenReturn(payloadStream); From 4ed834f3b5b0e53ccc3597a0b15600512f02b5c2 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Thu, 7 Mar 2024 11:45:10 -0500 Subject: [PATCH 6/6] read JSON using stream, ensure stream closed --- .../java/io/cryostat/JsonRequestFilter.java | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/main/java/io/cryostat/JsonRequestFilter.java b/src/main/java/io/cryostat/JsonRequestFilter.java index 798624d93..c8334fab2 100644 --- a/src/main/java/io/cryostat/JsonRequestFilter.java +++ b/src/main/java/io/cryostat/JsonRequestFilter.java @@ -17,6 +17,7 @@ import java.io.ByteArrayInputStream; import java.io.IOException; +import java.io.InputStream; import java.nio.charset.StandardCharsets; import com.fasterxml.jackson.databind.JsonNode; @@ -36,20 +37,21 @@ public class JsonRequestFilter implements ContainerRequestFilter { public void filter(ContainerRequestContext requestContext) throws IOException { if (requestContext.getMediaType() != null && requestContext.getMediaType().isCompatible(MediaType.APPLICATION_JSON_TYPE)) { - byte[] jsonData = requestContext.getEntityStream().readAllBytes(); - String json = new String(jsonData, StandardCharsets.UTF_8); - JsonNode rootNode = objectMapper.readTree(json); + try (InputStream stream = requestContext.getEntityStream()) { + JsonNode rootNode = objectMapper.readTree(stream); - if (containsIdField(rootNode)) { - requestContext.abortWith( - Response.status(Response.Status.BAD_REQUEST) - .entity("ID field cannot be specified in the request body.") - .build()); - return; - } + if (containsIdField(rootNode)) { + requestContext.abortWith( + Response.status(Response.Status.BAD_REQUEST) + .entity("ID field cannot be specified in the request body.") + .build()); + return; + } - requestContext.setEntityStream( - new ByteArrayInputStream(json.getBytes(StandardCharsets.UTF_8))); + requestContext.setEntityStream( + new ByteArrayInputStream( + rootNode.toString().getBytes(StandardCharsets.UTF_8))); + } } }