Skip to content

Commit 175de37

Browse files
tim48-robottim48-robot
committed
fix(api): address Copilot review feedback and add test coverage
- Use ObjectMapper for proper JSON escaping (prevents injection) - Reuse ErrorResponse class for consistency with GlobalExceptionHandler - Check response.isCommitted() before writing error response - Catch IOException specifically instead of broad Exception - Fix log message typo (clinicalDataBinCountFilter -> clinicalDataCountFilter) - Add comprehensive unit tests for error handling scenarios
1 parent 6c767bf commit 175de37

File tree

2 files changed

+167
-12
lines changed

2 files changed

+167
-12
lines changed

‎src/main/java/org/cbioportal/legacy/web/util/InvolvedCancerStudyExtractorInterceptor.java‎

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import com.fasterxml.jackson.databind.ObjectMapper;
3636
import jakarta.servlet.http.HttpServletRequest;
3737
import jakarta.servlet.http.HttpServletResponse;
38+
import java.io.IOException;
3839
import java.util.ArrayList;
3940
import java.util.Arrays;
4041
import java.util.Collection;
@@ -43,6 +44,7 @@
4344
import java.util.List;
4445
import java.util.Set;
4546
import java.util.stream.Collectors;
47+
import org.cbioportal.application.rest.error.ErrorResponse;
4648
import org.cbioportal.legacy.model.AlterationFilter;
4749
import org.cbioportal.legacy.model.MolecularProfile;
4850
import org.cbioportal.legacy.model.MolecularProfileCaseIdentifier;
@@ -165,24 +167,29 @@ public class InvolvedCancerStudyExtractorInterceptor implements HandlerIntercept
165167
* @param message the error message to include in the response body
166168
*/
167169
private void sendBadRequestResponse(HttpServletResponse response, String message) {
168-
try {
169-
response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
170-
response.setContentType("application/json");
171-
response.setCharacterEncoding("UTF-8");
170+
if (response.isCommitted()) {
171+
LOG.warn("Cannot send error response - response already committed");
172+
return;
173+
}
172174

175+
try {
173176
// Truncate message before Jackson source location info if present
174-
String truncatedMessage = message;
177+
String cleanMessage = message;
175178
int sourceIndex = message.indexOf(" at [Source:");
176179
if (sourceIndex != -1) {
177-
truncatedMessage = message.substring(0, sourceIndex);
180+
cleanMessage = message.substring(0, sourceIndex);
178181
}
179182

180-
String jsonError =
181-
String.format("{\"message\":\"%s\"}", truncatedMessage.replace("\"", "\\\""));
182-
response.getWriter().write(jsonError);
183+
response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
184+
response.setContentType("application/json");
185+
response.setCharacterEncoding("UTF-8");
186+
187+
// Use ObjectMapper for proper JSON escaping (matches GlobalExceptionHandler pattern)
188+
ErrorResponse errorResponse = new ErrorResponse("Invalid request body: " + cleanMessage);
189+
response.getWriter().write(objectMapper.writeValueAsString(errorResponse));
183190
response.getWriter().flush();
184-
} catch (Exception ex) {
185-
LOG.error("Failed to send error response: {}", ex.getMessage());
191+
} catch (IOException ex) {
192+
LOG.error("Failed to write error response: {}", ex.getMessage());
186193
}
187194
}
188195

@@ -819,7 +826,7 @@ private boolean extractAttributesFromClinicalDataCountFilter(
819826
}
820827
} catch (Exception e) {
821828
LOG.error(
822-
"exception thrown during extraction of clinicalDataBinCountFilter: {}", e.getMessage());
829+
"exception thrown during extraction of clinicalDataCountFilter: {}", e.getMessage());
823830
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
824831
return false;
825832
}

‎src/test/java/org/cbioportal/legacy/web/util/InvolvedCancerStudyExtractorInterceptorTest.java‎

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
package org.cbioportal.legacy.web.util;
2+
3+
import static org.junit.Assert.assertFalse;
4+
import static org.junit.Assert.assertTrue;
5+
import static org.mockito.Mockito.*;
6+
7+
import com.fasterxml.jackson.databind.ObjectMapper;
8+
import jakarta.servlet.ServletInputStream;
9+
import jakarta.servlet.http.HttpServletRequest;
10+
import jakarta.servlet.http.HttpServletResponse;
11+
import java.io.ByteArrayInputStream;
12+
import java.io.IOException;
13+
import java.io.PrintWriter;
14+
import java.io.StringWriter;
15+
import java.nio.charset.StandardCharsets;
16+
import org.cbioportal.legacy.persistence.cachemaputil.CacheMapUtil;
17+
import org.junit.Before;
18+
import org.junit.Test;
19+
import org.junit.runner.RunWith;
20+
import org.mockito.InjectMocks;
21+
import org.mockito.Mock;
22+
import org.mockito.Spy;
23+
import org.mockito.junit.MockitoJUnitRunner;
24+
import org.springframework.mock.web.DelegatingServletInputStream;
25+
26+
@RunWith(MockitoJUnitRunner.class)
27+
public class InvolvedCancerStudyExtractorInterceptorTest {
28+
29+
@InjectMocks private InvolvedCancerStudyExtractorInterceptor interceptor;
30+
31+
@Mock private CacheMapUtil cacheMapUtil;
32+
33+
@Mock private HttpServletRequest request;
34+
35+
@Mock private HttpServletResponse response;
36+
37+
@Spy private ObjectMapper objectMapper = new ObjectMapper();
38+
39+
private StringWriter responseWriter;
40+
41+
@Before
42+
public void setUp() throws IOException {
43+
responseWriter = new StringWriter();
44+
when(response.getWriter()).thenReturn(new PrintWriter(responseWriter));
45+
when(response.isCommitted()).thenReturn(false);
46+
when(cacheMapUtil.hasCacheEnabled()).thenReturn(false);
47+
}
48+
49+
private ServletInputStream createInputStream(String content) {
50+
ByteArrayInputStream byteStream =
51+
new ByteArrayInputStream(content.getBytes(StandardCharsets.UTF_8));
52+
return new DelegatingServletInputStream(byteStream);
53+
}
54+
55+
@Test
56+
public void testMalformedJsonReturnsBadRequest() throws Exception {
57+
when(request.getMethod()).thenReturn("POST");
58+
when(request.getPathInfo()).thenReturn("/api/patients/fetch");
59+
when(request.getInputStream()).thenReturn(createInputStream("{'invalid json'}"));
60+
61+
boolean result = interceptor.preHandle(request, response, null);
62+
63+
assertFalse("preHandle should return false for malformed JSON", result);
64+
verify(response).setStatus(HttpServletResponse.SC_BAD_REQUEST);
65+
verify(response).setContentType("application/json");
66+
assertTrue(
67+
"Response should contain error message",
68+
responseWriter.toString().contains("Invalid request body"));
69+
}
70+
71+
@Test
72+
public void testEmptyBodyReturnsBadRequest() throws Exception {
73+
when(request.getMethod()).thenReturn("POST");
74+
when(request.getPathInfo()).thenReturn("/api/samples/fetch");
75+
when(request.getInputStream()).thenReturn(createInputStream(""));
76+
77+
boolean result = interceptor.preHandle(request, response, null);
78+
79+
assertFalse("preHandle should return false for empty body", result);
80+
verify(response).setStatus(HttpServletResponse.SC_BAD_REQUEST);
81+
}
82+
83+
@Test
84+
public void testValidJsonPassesThrough() throws Exception {
85+
String validJson =
86+
"{\"sampleIdentifiers\":[{\"sampleId\":\"sample1\",\"studyId\":\"study1\"}]}";
87+
when(request.getMethod()).thenReturn("POST");
88+
when(request.getPathInfo()).thenReturn("/api/samples/fetch");
89+
when(request.getInputStream()).thenReturn(createInputStream(validJson));
90+
91+
boolean result = interceptor.preHandle(request, response, null);
92+
93+
assertTrue("preHandle should return true for valid JSON", result);
94+
verify(response, never()).setStatus(HttpServletResponse.SC_BAD_REQUEST);
95+
}
96+
97+
@Test
98+
public void testNonPostRequestPassesThrough() throws Exception {
99+
when(request.getMethod()).thenReturn("GET");
100+
101+
boolean result = interceptor.preHandle(request, response, null);
102+
103+
assertTrue("preHandle should return true for GET requests", result);
104+
verify(response, never()).setStatus(anyInt());
105+
}
106+
107+
@Test
108+
public void testMessageTruncationRemovesJacksonDetails() throws Exception {
109+
when(request.getMethod()).thenReturn("POST");
110+
when(request.getPathInfo()).thenReturn("/api/patients/fetch");
111+
when(request.getInputStream()).thenReturn(createInputStream("{bad}"));
112+
113+
interceptor.preHandle(request, response, null);
114+
115+
String responseBody = responseWriter.toString();
116+
assertFalse(
117+
"Response should not contain Jackson source details", responseBody.contains("at [Source:"));
118+
}
119+
120+
@Test
121+
public void testJsonEscapingForSpecialCharacters() throws Exception {
122+
// This tests that ObjectMapper properly escapes special characters
123+
when(request.getMethod()).thenReturn("POST");
124+
when(request.getPathInfo()).thenReturn("/api/patients/fetch");
125+
// JSON with unmatched quote that would cause parsing error with special chars in message
126+
when(request.getInputStream()).thenReturn(createInputStream("{\"field\": \"value with \\n}"));
127+
128+
interceptor.preHandle(request, response, null);
129+
130+
String responseBody = responseWriter.toString();
131+
// Verify it's valid JSON (ObjectMapper would produce valid JSON)
132+
assertTrue("Response should be valid JSON", responseBody.startsWith("{\"message\":"));
133+
assertTrue("Response should end properly", responseBody.endsWith("}"));
134+
}
135+
136+
@Test
137+
public void testCommittedResponseDoesNotWrite() throws Exception {
138+
when(request.getMethod()).thenReturn("POST");
139+
when(request.getPathInfo()).thenReturn("/api/patients/fetch");
140+
when(request.getInputStream()).thenReturn(createInputStream("invalid"));
141+
when(response.isCommitted()).thenReturn(true);
142+
143+
interceptor.preHandle(request, response, null);
144+
145+
verify(response, never()).setStatus(anyInt());
146+
verify(response, never()).getWriter();
147+
}
148+
}

0 commit comments

Comments
 (0)