Skip to content

Handle an unknown HTTP method instead of raising IllegalArgumentException #2043

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
M0nd0R wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Handle an unknown HTTP method instead of raising IllegalArgumentException #2043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethod.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
*/
package org.apache.struts2.interceptor.httpmethod;

import java.util.Locale;

/**
* Enum represents possible http request types
*
Expand All @@ -36,8 +38,28 @@ public enum HttpMethod {
CONNECT,
PATCH;

/**

@lukaszlenart lukaszlenart Oct 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* Returns the {@link HttpMethod} for the given request method, or {@code null} if it is not
* one of the methods this enum knows about.
*
* <p>Use this for the method of an incoming request. The client chooses that token freely,
* so an unrecognised value is ordinary input rather than a programming error, and must not
* raise.</p>
*/
public static HttpMethod parseOrNull(String httpRequestMethod) {
if (httpRequestMethod == null) {
return null;
}

try {

@lukaszlenart lukaszlenart Oct 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

return valueOf(httpRequestMethod.toUpperCase(Locale.ROOT));
} catch (IllegalArgumentException e) {
return null;
}
}

public static HttpMethod parse(String httpRequestMethod) {

@lukaszlenart lukaszlenart Oct 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return valueOf(httpRequestMethod.toUpperCase());
return valueOf(httpRequestMethod.toUpperCase(Locale.ROOT));
}

}
5 changes: 4 additions & 1 deletion core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,10 @@ public String intercept(ActionInvocation invocation) throws Exception {
if (action instanceof HttpMethodAware) {
LOG.debug("Action: {} implements: {}, setting request method: {}",
action, HttpMethodAware.class.getSimpleName(), request.getMethod());
((HttpMethodAware) (action)).setMethod(HttpMethod.parse(request.getMethod()));
HttpMethod requestedHttpMethod = HttpMethod.parseOrNull(request.getMethod());
if (requestedHttpMethod != null) {

@lukaszlenart lukaszlenart Oct 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

((HttpMethodAware) (action)).setMethod(requestedHttpMethod);

@lukaszlenart lukaszlenart Oct 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());
}

}
}

if (invocation.getProxy().isMethodSpecified()) {
Expand Down
55 changes: 55 additions & 0 deletions core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;

public class HttpMethodTest {

Expand Down Expand Up @@ -55,4 +56,58 @@ public void shouldValueOfThrowsException() {
assertEquals(expected.getClass(), IllegalArgumentException.class);
}


@Test
public void shouldParseUnknownRequestMethod_asNull_notRaise() {
// given : a request method this enum does not know about, e.g. a WebDAV verb
String httpRequestMethod = "PROPFIND";

// when
HttpMethod httpMethod = HttpMethod.parseOrNull(httpRequestMethod);

// then
assertNull(httpMethod);
}

@Test
public void shouldParseKnownRequestMethod_regardlessOfCase() {
// given
String httpRequestMethod = "get";

// when
HttpMethod httpMethod = HttpMethod.parseOrNull(httpRequestMethod);

// then
assertEquals(HttpMethod.GET, httpMethod);
}

@Test
public void shouldParseNullRequestMethod_asNull() {
// given
String httpRequestMethod = null;

// when
HttpMethod httpMethod = HttpMethod.parseOrNull(httpRequestMethod);

// then
assertNull(httpMethod);
}

@Test

@lukaszlenart lukaszlenart Oct 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

public void shouldStillRejectUnknownMethodInConfiguration() {
// given : configuration is trusted input, an unknown name there is a real error
String configuredMethod = "PROPFIND";

// when
Throwable expected = null;
try {
HttpMethod.parse(configuredMethod);
} catch (IllegalArgumentException e) {
expected = e;
}

// then
assertNotNull(expected);
}

}
Loading