Skip to content

Commit 7a9c4f5

Browse files
Return 400 instead of reporting malformed requests as server errors (#8160)
## Rationale Malformed requests, mostly from vulnerability scanners, are logged as 500s and reported to mothership as server errors. This PR returns a 4xx for them instead, and stops reporting them. Tomcat 11 throws on every parameter read once a query string or form body fails to parse. Rendering the standard error page reads parameters too, so even a 404 for such a request became a 500. Triage notes for these mothership reports: https://claude.ai/artifact/CtxKZANKRqfhZxs7PHEqBr ## Related Pull Requests - LabKey/testAutomation#3247 ## Changes - Requests whose parameters can't be parsed get a plain 4xx response using Tomcat's status code, without rendering an error page. They are never reported to mothership. - Multipart bodies that can't be parsed get a 400 on standard actions, instead of being rethrown as a server error. - WebDAV reports to mothership only for 5xx responses, and returns 400 for unparseable query strings. - Added a unit test for the invalid-parameter handling. Tested against a local server with probe-style requests: invalid UTF-8 and empty-name parameters, a forwarded image request, and broken multipart POSTs to actions and WebDAV. All returned 400 with no errors logged, and normal pages were unaffected. ## Tasks - [x] Claude Code Review - [x] Manual Testing - probably not needed - [x] Test Automation - [x] Code Review @labkey-adam --------- Co-authored-by: Adam Rauch <adam@labkey.com>
1 parent 6c45a37 commit 7a9c4f5

5 files changed

Lines changed: 2042 additions & 1789 deletions

File tree

‎api/src/org/labkey/api/action/SpringActionController.java‎

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import jakarta.servlet.http.HttpServletRequest;
2222
import jakarta.servlet.http.HttpServletResponse;
2323
import org.apache.commons.lang3.StringUtils;
24+
import org.apache.commons.lang3.exception.ExceptionUtils;
2425
import org.apache.logging.log4j.Logger;
2526
import org.jetbrains.annotations.NotNull;
2627
import org.jetbrains.annotations.Nullable;
@@ -73,6 +74,8 @@
7374
import org.springframework.context.ApplicationContext;
7475
import org.springframework.context.ApplicationContextAware;
7576
import org.springframework.web.HttpRequestMethodNotSupportedException;
77+
import org.springframework.web.multipart.MaxUploadSizeExceededException;
78+
import org.springframework.web.multipart.MultipartException;
7679
import org.springframework.web.multipart.support.StandardServletMultipartResolver;
7780
import org.springframework.web.servlet.DispatcherServlet;
7881
import org.springframework.web.servlet.ModelAndView;
@@ -367,6 +370,11 @@ private ModelAndView handleBadRequestException(HttpServletRequest request, HttpS
367370
}
368371

369372
private ModelAndView handleBadRequestException(HttpServletRequest request, HttpServletResponse response, Controller controller, BadRequestException x) throws Exception
373+
{
374+
return handleBadRequestException(request, response, controller, x, HttpServletResponse.SC_BAD_REQUEST);
375+
}
376+
377+
private ModelAndView handleBadRequestException(HttpServletRequest request, HttpServletResponse response, Controller controller, BadRequestException x, int status) throws Exception
370378
{
371379
// TODO: we are assuming that all ApiActions respond with JSON and all !ApiAction respond HTML. Would be better to be explicit.
372380
boolean jsonResponse = controller instanceof BaseApiAction;
@@ -379,13 +387,13 @@ private ModelAndView handleBadRequestException(HttpServletRequest request, HttpS
379387
{
380388
// Issue 53120: Set content-type, status when responding with JSON content
381389
response.setContentType(ApiJsonWriter.CONTENT_TYPE_JSON);
382-
response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
390+
response.setStatus(status);
383391
}
384392

385393
Writer w = response.getWriter();
386394
JSONObject json = new JSONObject();
387395
json.put("success", false);
388-
json.put("status", HttpServletResponse.SC_BAD_REQUEST);
396+
json.put("status", status);
389397
json.put("exception", x.getMessage());
390398
if (htmlWrappedJson)
391399
w.write("<html><body><textarea>");
@@ -397,7 +405,7 @@ private ModelAndView handleBadRequestException(HttpServletRequest request, HttpS
397405

398406
if (textResponse)
399407
{
400-
response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
408+
response.setStatus(status);
401409
response.setContentType("text/plain");
402410
Writer w = response.getWriter();
403411
w.write(x.getMessage());
@@ -407,7 +415,7 @@ private ModelAndView handleBadRequestException(HttpServletRequest request, HttpS
407415
// TODO design/text for custom view
408416
if (x instanceof AntiVirusException)
409417
{
410-
response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
418+
response.setStatus(status);
411419
HttpView<?> view = SimpleErrorView.fromMessage(x.getMessage());
412420

413421
PageConfig page = new PageConfig(request);
@@ -416,7 +424,7 @@ private ModelAndView handleBadRequestException(HttpServletRequest request, HttpS
416424
return null;
417425
}
418426

419-
response.sendError(HttpServletResponse.SC_BAD_REQUEST, x.getMessage());
427+
response.sendError(status, x.getMessage());
420428
return null;
421429
}
422430

@@ -538,6 +546,23 @@ public ModelAndView handleRequest(HttpServletRequest request, @NotNull HttpServl
538546
// TODO Better/custom error report page/response
539547
if (x.getCause() instanceof BadRequestException)
540548
return handleBadRequestException(request, response, controller, (BadRequestException)x.getCause());
549+
if (x instanceof MultipartException && ExceptionUtil.isClientRequestFailure(x))
550+
{
551+
_log.debug("Unable to parse multipart request", x);
552+
int status;
553+
String message;
554+
if (x instanceof MaxUploadSizeExceededException)
555+
{
556+
status = HttpServletResponse.SC_REQUEST_ENTITY_TOO_LARGE;
557+
message = StringUtils.defaultIfBlank(ExceptionUtils.getRootCause(x).getMessage(), x.getMessage());
558+
}
559+
else
560+
{
561+
status = HttpServletResponse.SC_BAD_REQUEST;
562+
message = "Unable to parse multipart request";
563+
}
564+
return handleBadRequestException(request, response, controller, new BadRequestException(message, x), status);
565+
}
541566
throw x;
542567
}
543568
}

‎api/src/org/labkey/api/module/DefaultModule.java‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1061,7 +1061,17 @@ public void dispatch(HttpServletRequest request, HttpServletResponse response, A
10611061
return;
10621062
}
10631063

1064-
ViewContext rootContext = new ViewContext(request, response, url);
1064+
ViewContext rootContext;
1065+
try
1066+
{
1067+
rootContext = new ViewContext(request, response, url);
1068+
}
1069+
// First parameter read; unparseable parameters would otherwise escape to Tomcat's own error handling
1070+
catch (IllegalStateException x)
1071+
{
1072+
ExceptionUtil.handleException(request, response, x, null, false);
1073+
return;
1074+
}
10651075

10661076
Container container = rootContext.getContainer();
10671077
if (container != null && !isAvailable(container))

0 commit comments

Comments
 (0)