[labkey-jeckels]

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

• Add test cases for bad requests 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

• Claude Code Review
• Manual Testing - probably not needed
• Test Automation
• Code Review @labkey-adam

[labkey-adam]

Since the PR targets Tomcat behavior, I read Tomcat 11.0.26's Request.java to check how it wraps parse errors. Two facts drive the findings:

• When a multipart body fails to parse, any error from that parse comes back as an InvalidParameterException with code 400 on every later parameter read. That includes server-side I/O errors such as an invalid upload directory or a full disk.
• getParameterMap() rethrows the stored parse error, which is not always an InvalidParameterException.

Finding 1 - [Severity: Medium] — Error classification / information disclosure — api/src/org/labkey/api/action/SpringActionController.java, handleRequest() new MultipartException branch; also ExceptionUtil.getInvalidParameterException()

• Issue: Every MultipartException becomes a 400 that is only logged at debug, and the client gets the root cause's message. Tomcat's parseParts() also uses these errors for server-side failures:

  • "The temporary upload location [] is not valid" when the upload temp dir is missing or not a directory.
  • The I/O error from getCanonicalFile() on that directory.
  • Disk-full or permission errors while writing parts.

• Spring wraps all of these as MultipartException("Failed to parse multipart servlet request", ioe). The new branch answers 400 with, for example, the temp upload path. The new probe in ExceptionUtil.handleException has the same problem:

• Tomcat rethrows the same error from getParameterMap() as a 400 InvalidParameterException, and isIgnorable() then hides it from mothership everywhere. WebDAV's multipart path already answered 400, but with the new sendError threshold thoseerrors are no longer reported either.

• Why it matters: A misconfigured temp dir or a full disk breaks every upload on the server. After this PR that shows up only as client 400s and debug logs, with nothing in mothership or the error log. The 400 message also exposes an absolute server path.

• Suggestion:

  • Only treat the failure as a client error when the cause chain shows a parse problem: a fileupload FileUploadException/SizeException, Tomcat's BadRequestException, or MaxUploadSizeExceededException. Rethrow anything else, such as a bare IOException from the upload location or the disk.
  • Send a fixed message ("Unable to parse multipart request") instead of the root cause's message.
  • Make the same distinction in the ExceptionUtil probe: skip the short-circuit when the InvalidParameterException's cause is a server-side IOException rather than Tomcat's BadRequestException or a fileupload exception.

Finding 2 - [Severity: Low] — Robustness — api/src/org/labkey/api/util/ExceptionUtil.java, getInvalidParameterException()

• Issue: The probe catches only InvalidParameterException. Tomcat's getParameterMap() can also rethrow a plain IllegalStateException. It does this when the multipart parse error was itself a plain one, such as coyoteRequest.noMultipartConfig on a servlet without a multipart config, or an IllegalStateException from fileupload. The probe now runs for every exception that reaches handleException, so that error would escape the error handler itself. Before the PR it only escaped if rendering happened to read parameters.
• Why it matters: The error handler failing while handling an error produces a container-default 500 and loses the original exception. It's rare, because the LabKey servlets that do multipart have a multipart config.
• Suggestion: Catch IllegalStateException in the probe. Tomcat only throws it from getParameterMap() for parameter-parsing failures, so return a 400 for those too, or return null and let normal handling continue.

Finding 3 - [Severity: Low] — Test coverage — ExceptionUtil.TestCase.handleIt()

• Issue: handleIt decorates every exception with SkipMothershipLogging before calling handleException. So assertFalse(...containsKey("Logger.error")) in testInvalidParameters would pass whether or not the new branch suppressed reporting. The status assertions are real coverage, and isIgnorable is tested directly, so this is weaker than it looks rather than wrong.
• Suggestion: Drop the misleading assertions or add a comment saying so. If you take Finding 1, add a case where the InvalidParameterException wraps a bare IOException and still produces a 500.

[labkey-adam]

I think an integration test that sends malformed URLs through the Tomcat "front door" would be useful here to prove the fixes definitely. I can code something up next to WebDavTest.testBadUrlResponse.

[labkey-adam]

Claude thinks there's one finding that should be addressed. And I think an integration test would be useful. I can address both, if you concur.

[labkey-adam]

Corrections to my review above, after re-reading Tomcat 11.0.26's FileUploadBase and Request:

Finding 1's suggestion was wrong. Having a fileupload FileUploadException in the cause chain doesn't mean the client is at fault. FileUploadBase.parseRequest() wraps any IOException raised while copying a part in IOFileUploadException, which is a FileUploadException. That includes a full disk or a temp file that can't be created. Request.parseParameters() also wraps every multipart failure in a 400 InvalidParameterException, so that type can't serve as the marker either. The workable rule is an allowlist of client-side causes:

• org.apache.coyote.BadRequestException (read failure or disconnect)
• CharacterCodingException/CharConversionException (undecodable parameters)
• MalformedStreamException, IllegalBoundaryException
• InvalidContentTypeException, InvalidFileNameException
• SizeException, FileCountLimitExceededException
• Spring's MaxUploadSizeExceededException

Anything else, such as the bare IOException for an invalid upload location, stays a reported 500.

Finding 3 overstated the problem. The Logger.error assertions in testInvalidParameters do test something: dummyLog captures handleException's own log.error, and the SkipMothershipLogging decoration doesn't affect that. What handleIt can't verify is mothership suppression, and the direct isIgnorable assertions cover that.

Finding 2 stands as written. Fixes for all three are coming to this branch.

[labkey-adam]

@labkey-jeckels my changes are in and all looks good to me. Up to you to review my changes and merge.