Handle an unknown HTTP method instead of raising IllegalArgumentException

[M0nd0R]

An unknown HTTP method reaches HttpMethodInterceptor and produces a 500.

HttpMethod.parse() is valueOf(method.toUpperCase()) with no fallback, and the interceptor calls it with the raw request method for any HttpMethodAware action — before the allow-checks, and with no try/catch:

HttpMethod method = HttpMethod.parse(request.getMethod());

The enum has nine constants, so PROPFIND, MKCOL, REPORT or any invented token raises IllegalArgumentException instead of being treated as "not allowed". When http-method is configured the code already knows how to answer that case — 405 with an Allow header — it just never gets there.

HttpMethod.parseOrNull() returns null for an unrecognised method; the interceptor leaves the method unset and falls through to the existing isAllowed() check. Both parse() and parseOrNull() use toUpperCase(Locale.ROOT) — the bare call is locale-sensitive, and XmlDocConfigurationProvider already uses Locale.ROOT for the same lookup.

Repro: any action with http-method set, request PROPFIND /that-action → 500 before, 405 + Allow after.

Tests: four added to HttpMethodTest. mvn -pl core -Dtest=HttpMethodTest test → BUILD SUCCESS, 6 tests, 0 failures (JDK 21).

Two process notes, so you can redirect me:

• There is no WW-XXXX ticket for this yet, and the contributing guide requires one. Tell me the key to use and I will update the title and commit message to match.
• This is an error-handling bug rather than a vulnerability in the sense of SECURITY.md. If you would rather it went through security@struts.apache.org, close this and I will send it there instead.

[lukaszlenart]

Thanks for the fix and the clear write-up. I reproduced the bug with an HttpMethodAware action and a PROPFIND request: IllegalArgumentException on main, 405 with Allow: POST on this branch. This is an ordinary bug and the public PR is the right place for it.

Before we go further, please create a Jira ticket at https://issues.apache.org/jira/projects/WW (type Bug, component Core Interceptors) and retitle the PR and the commit as WW-XXXX Handle an unknown HTTP method instead of raising IllegalArgumentException. If you would rather not create an account, say so and I will file it and give you the key.

The inline comments cover the rest. The one I would not merge without is the interceptor-level regression test.

[lukaszlenart]

The four new tests exercise the enum directly, so none of them reach this line. I reverted this hunk locally and HttpMethodTest + HttpMethodInterceptorTest still pass. The existing testConfigRejectsUnknownRequestMethod uses TestAction, which is not HttpMethodAware, so it never hit the old parse() call.

Please add a test to HttpMethodInterceptorTest that drives the fixed path, along the lines of:

public void testHttpMethodAwareActionWithUnknownRequestMethod() throws Exception {
    HttpMethodsTestAction action = new HttpMethodsTestAction();
    prepareActionInvocation(action, actionConfig());
    MockHttpServletResponse response = prepareRequest("PROPFIND");

    String resultName = interceptor.intercept(invocation);

    assertEquals(Action.NONE, resultName);
    assertEquals(HttpServletResponse.SC_METHOD_NOT_ALLOWED, response.getStatus());
    assertEquals("POST", response.getHeader("Allow"));
    assertNull(action.getHttpMethod());
}

[lukaszlenart]

Skipping the call changes the HttpMethodAware contract: its Javadoc says the action is notified about the request method, and an action instance that is reused (a Spring singleton, a custom ObjectFactory) would keep the previous request's method. I would call setMethod(requestedHttpMethod) unconditionally and add a line to HttpMethodAware#setMethod saying null means the request method is not one of the enum constants.

The debug line just above also still says "setting request method: PROPFIND" when nothing is set; worth adjusting once the null is passed through.

[lukaszlenart]

After this change parse() has no caller in src/main: XmlDocConfigurationProvider.buildHttpMethods calls valueOf(value.toUpperCase(Locale.ROOT)) directly. The new shouldStillRejectUnknownMethodInConfiguration test describes parse() as the configuration path, which it currently is not.

Simplest fix: have buildHttpMethods call HttpMethod.parse(value) so there is one lookup and one locale rule, and the test's claim becomes true.

[lukaszlenart]

Minor: catching IllegalArgumentException on a client-driven path uses the exception as control flow. The interceptor already matches by name with equalsIgnoreCase in isAllowed, so a lookup in the same style keeps the two consistent and drops the locale question:

return Arrays.stream(values())
    .filter(method -> method.name().equalsIgnoreCase(httpRequestMethod))
    .findFirst()
    .orElse(null);

[lukaszlenart]

Minor: the second paragraph is the rationale from the PR description, which is where it is best kept. A one-liner is enough here: @return the matching method, or null when the request method is not one of the constants.

[lukaszlenart]

Minor: JUnit 4.13 has assertThrows, which reads as the intent and fails with a clear message:

assertThrows(IllegalArgumentException.class, () -> HttpMethod.parse("PROPFIND"));