Skip to content

Issue #13670 - Returning ISE in all getParameter use cases outlined by Servlet 6.1 spec #13678

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
wants to merge 4 commits into
base: jetty-12.1.x
Choose a base branch
from
Open

Issue #13670 - Returning ISE in all getParameter use cases outlined by Servlet 6.1 spec #13678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
981b522
Issue #13670 - Returning ISE in all getParameter use cases outlined b…
joakime Oct 8, 2025
65fe54a
Removing now redundant test
joakime Oct 8, 2025
8cbac3e
Excluding VirtualMachineError from ISE wrapping
joakime Oct 22, 2025
28ad2c8
Excluding LinkageError from ISE wrapping
joakime Oct 22, 2025
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
66 changes: 59 additions & 7 deletions ...11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ServletApiRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1233,28 +1233,80 @@ public ServletInputStream getInputStream() throws IOException
@Override
public String getParameter(String name)
{
return getParameters().getValue(name);
try
{
return getParameters().getValue(name);
}
catch (IllegalStateException | VirtualMachineError | LinkageError e)
{
throw e;
}
catch (Throwable t)
{
// Per Servlet https://github.com/jakartaee/servlet/issues/431 this should only
// ever throw an IllegalStateException
throw new IllegalStateException(t);
}
Comment on lines +1244 to +1249
Copy link
Contributor

@gregw gregw Oct 13, 2025

Choose a reason for hiding this comment

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

I'm not sure I agree with this as the whole point of uncheck throwables is that that they do not need to be declared. The contract for getParameter has declared ISE as a politeness not as a binding contract. Specifically, I would not expect throwables like OutOfMemoryError or ClassFormatError to be caught and wrapped as an ISE.

The javadoc of this method specifically allows for other handling of exceptional cases:

Containers may provide container specific options to handle some or all of these errors in an alternative manner that may include not throwing an exception.

So I read this as allowing other exception to be thrown .

The whole point of BadMessageException being a RuntimeException, is that it is unlikely to be caught by an application and flow back to Jetty, where it can be handled correctly.

Copy link
Contributor

@gregw gregw Oct 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

At best I could be convinced to catch(RuntimeException| Exception) and wrap as an ISE.... or better yet a custom list of exceptions that we know we throw if we detect the kinds of conditions the javadoc mentions that should be ISEs.
Even then, I think we should have a compliance mode to determine if we wrap unchecked exceptions with ISE or not.

Copy link
Contributor Author

@joakime joakime Oct 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

See the conversation ..

The purpose of "Containers may provide container specific options to handle some or all of these errors in an alternative manner that may include not throwing an exception." is to allow optional "container configuration" to change behavior away from the servlet spec.

We don't have such a configuration right now.
So we default to Servlet Spec behavior. (what this PR is doing)

If we want to honor the "container specific options" then we need a boolean like boolean containerSpecificGetParameterBehavior = false; which defaults to servlet spec behavior.
Allowing users to turn on the "container configuration" for our behavior.

Copy link
Contributor Author

@joakime joakime Oct 22, 2025

Choose a reason for hiding this comment

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

@gregw I can add a a configuration to ServletContextHandler to allow setting this behavior (as the spec states).

Copy link
Contributor Author

@joakime joakime Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

At best I could be convinced to catch(RuntimeException| Exception) and wrap as an ISE.... or better yet a custom list of exceptions that we know we throw if we detect the kinds of conditions the javadoc mentions that should be ISEs.

The javadoc uses the language "@throws IllegalStateException if parameter parsing is triggered and a problem is encountered parsing the parameters including, but not limited to: invalid percent (%nn) encoding; ..."

That's not a specific list of failures, but all failures, there's no specific list, no limiting to specific conditions, it's all conditions that would cause a failure in these getParameter*() methods.

Even then, I think we should have a compliance mode to determine if we wrap unchecked exceptions with ISE or not.

That would introduce a new ServletCompliance mode.
But I feel that this isn't needed, as the javadoc is unambiguous in this case.

Copy link
Contributor Author

@joakime joakime Oct 22, 2025

Choose a reason for hiding this comment

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

Added an exclusion for java.lang.VirtualMachineError and java.lang.LinkageError, but any larger exclusion list is just invalid IMO (this includes RuntimeException and Throwable which have many recoverable non-fatal exceptions already)

}

@Override
public Enumeration<String> getParameterNames()
{
return Collections.enumeration(getParameters().getNames());
try
{
return Collections.enumeration(getParameters().getNames());
}
catch (IllegalStateException e)
{
throw e;
}
catch (Throwable t)
{
// Per Servlet https://github.com/jakartaee/servlet/issues/431 this should only
// ever throw an IllegalStateException
throw new IllegalStateException(t);
}
}

@Override
public String[] getParameterValues(String name)
{
List<String> vals = getParameters().getValues(name);
if (vals == null)
return null;
return vals.toArray(new String[0]);
try
{
List<String> vals = getParameters().getValues(name);
if (vals == null)
return null;
return vals.toArray(new String[0]);
}
catch (IllegalStateException e)
{
throw e;
}
catch (Throwable t)
{
// Per Servlet https://github.com/jakartaee/servlet/issues/431 this should only
// ever throw an IllegalStateException
throw new IllegalStateException(t);
}
}

@Override
public Map<String, String[]> getParameterMap()
{
return Collections.unmodifiableMap(getParameters().toStringArrayMap());
try
{
return Collections.unmodifiableMap(getParameters().toStringArrayMap());
}
catch (IllegalStateException e)
{
throw e;
}
catch (Throwable t)
{
// Per Servlet https://github.com/jakartaee/servlet/issues/431 this should only
// ever throw an IllegalStateException
throw new IllegalStateException(t);
}
}

public Fields getParameters()
Expand Down
62 changes: 61 additions & 1 deletion jetty-ee11/jetty-ee11-servlet/src/test/java/org/eclipse/jetty/ee11/servlet/RequestTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@

import jakarta.servlet.MultipartConfigElement;
import jakarta.servlet.ServletException;
import jakarta.servlet.ServletRequest;
import jakarta.servlet.http.Cookie;
import jakarta.servlet.http.HttpServlet;
import jakarta.servlet.http.HttpServletRequest;
Expand Down Expand Up @@ -692,7 +693,6 @@ protected void service(HttpServletRequest request, HttpServletResponse resp) thr
out.println(Arrays.asList(request.getParameterValues("b")));
out.println(Arrays.asList(request.getParameterValues("c")));
out.println(Arrays.asList(request.getParameterValues("d")));

}
});

Expand All @@ -719,6 +719,66 @@ protected void service(HttpServletRequest request, HttpServletResponse resp) thr
"""));
}

public record ParameterCase(String purpose, Consumer<HttpServletRequest> requestAction)
{
@Override
public String toString()
{
return purpose;
}
}

public static Stream<Arguments> parameterISECases()
{
List<Arguments> cases = new ArrayList<>();

cases.add(Arguments.of(new ParameterCase("getParameter(String)",
(request) -> request.getParameter("x"))));
cases.add(Arguments.of(new ParameterCase("getParameterNames()",
ServletRequest::getParameterNames)));
cases.add(Arguments.of(new ParameterCase("getParameterValues(String)",
(request) -> request.getParameterValues("x"))));
cases.add(Arguments.of(new ParameterCase("getParameterMap()",
ServletRequest::getParameterMap)));

return cases.stream();
}

@ParameterizedTest
@MethodSource("parameterISECases")
public void testGetParameterISE(ParameterCase parameterCase) throws Exception
{
startServer(new HttpServlet()
{
@Override
protected void service(HttpServletRequest request, HttpServletResponse resp) throws IOException
{
try
{
parameterCase.requestAction.accept(request);
resp.setStatus(500);
resp.getWriter().print("BAD: ISE Should have Occurred");
}
catch (IllegalStateException e)
{
resp.setStatus(200);
resp.getWriter().print("GOOD: ISE Occurred");
}
}
});

String rawResponse = _connector.getResponse(
"""
POST /test/parameters?x=%80 HTTP/1.1\r
Host: localhost\r
Connection: close\r
\r
""");
HttpTester.Response response = HttpTester.parseResponse(rawResponse);
assertThat(response.getStatus(), is(HttpStatus.OK_200));
assertThat(response.getContent(), is("GOOD: ISE Occurred"));
}

static Stream<Arguments> suspiciousCharactersLegacy()
{
return Stream.of(
Expand Down