eclipse-ee4j
Jersey not compatible with HttpServletRequests not always returning the same input stream from version 2.44
After this pr jersey is no longer handling correctly any override of HttpServletRequest which does not return always the same input stream when invoking getInputStream.
Context about different implementation of ServletRequest#getInputStream
The servlet api does not seem to specify wether or not it is required for an implementation to always return the same input stream across multiple invocations and it therefore seems valid in some cases to override this method in an HttpServletRequestWrapper returning a new copy of the input stream for any invocation (to be able to consume the input stream multiple times). This is showcased for example in this stack overflow answer about how a servlet input stream could be consumed multiple times.
Current issue
With the aforementioned change the InputStreamWrapper will invoke the underlying ServletRequest#getInputStream whenever any operation is performed to the request input stream. This can cause the input stream to be read forever as invocations to InputStreamWrapper#available will create a new input stream every time that this has not yet been consumed.
How to reproduce this issue
When overriding the http servlet request in a servlet filter with a request such as the one showed in the previous stack overflow answer no jersey invocation to an endpoint consuming json seems to work.
Possible solutions
This use case seems quiet valid also considering that HttpServletRequest does not expose anywhere in its api a way to override the input stream and therefore allow for more easy input replayability. I therefore wondered if this could be fixed/supported by having InputStreamWrapper caching the result of InputStreamWrapper#getWrapped after it is invoked the first time?
@jansupol do you have any update on this issue, perhaps? This is preventing us from updating Jersey to the latest
@senivam I do believe that #5806 as you mentioned in #5780 should indeed solve the error handling issue but the problem of repayable input streams which is described here should still be persisting right? Is there any plan to address this in the future or is this use case not going to be supported by jersey going forward (considering that this was working before 2.44)?
In case this issue needs further explanation I would gladly help with that.
@Lorenzo-Bracci, in the description of the issue you are referring to the commit which introduces InputStreamWrapper in Jersey. This wrapper had made the issue with the first byte corruption in the SerlvetInputStream more visible because Jersey attempts to read the first byte to determine whether the stream is empty or not. The fix for that issue is indeed the #5806 PR which attempts to use ServletInputStream functionality to discover if the underlying stream is ready. However the purpose of the InputStreamWrapper is to prevent Jersey from getting the ServletInputStream too early (or at all) if it is not required by Jersey. If the Servlet is not designed to be processed by Jersey, the ServletInputStream must not be touched by Jersey that is why wrapper uses lazy call to the getInputStream method. Caching however is applied only in the case if the first byte had been read thus having the original input stream been corrupted but preserving the first byte in the byte buffer. This use case shall not occur any more in the case of the ServletInputStream after the #5806 is merged. So, the original InputStream won't be cached (because it won't be read for checking of the readiness) and this issue should be resolved.
@senivam I don t believe this issue is solved, I will attempt to explain this better also with a reproducer.
Assume that similarly to what is proposed in the stack overflow answer the servlet request is overridden with one which has a replay-able input stream (which seems valid for use cases such as logging or really any servlet filter preprocessing of the request payload before a servlet such as jersey processes the request). To achieve this you would need a wrapper for the servlet request such as the following:
public class MultiReadHttpServletRequest extends HttpServletRequestWrapper {
private ByteArrayOutputStream cachedBytes;
public MultiReadHttpServletRequest(HttpServletRequest request) {
super(request);
}
@Override
public ServletInputStream getInputStream() throws IOException {
if (cachedBytes == null)
cacheInputStream();
return new CachedServletInputStream(cachedBytes.toByteArray());
}
@Override
public BufferedReader getReader() throws IOException{
return new BufferedReader(new InputStreamReader(getInputStream()));
}
private void cacheInputStream() throws IOException {
// Cache the inputstream in order to read it multiple times.
cachedBytes = new ByteArrayOutputStream();
IOUtils.copy(super.getInputStream(), cachedBytes);
}
/* An input stream which reads the cached request body */
private class CachedServletInputStream extends ServletInputStream {
private final ByteArrayInputStream buffer;
public CachedServletInputStream(byte[] contents) {
this.buffer = new ByteArrayInputStream(contents);
}
@Override
public int read() {
return buffer.read();
}
@Override
public boolean isFinished() {
return buffer.available() == 0;
}
@Override
public boolean isReady() {
return true;
}
@Override
public void setReadListener(ReadListener listener) {
throw new RuntimeException("Not implemented");
}
}
}
And then a filter which overrides the original servlet request with the wrapped one, that would look something like this:
public class FilterSettingMultiReadRequest implements Filter {
@Override
public void doFilter(ServletRequest request, ServletResponse response,
FilterChain chain) throws IOException, ServletException {
/* wrap the request in order to read the inputstream multiple times */
MultiReadHttpServletRequest multiReadRequest = new MultiReadHttpServletRequest((HttpServletRequest) request);
chain.doFilter(multiReadRequest, response);
}
}
Once any such a filter is registered the underlying jersey servlet in the current state will not be able to process any json requests using the jackson provider (and I suspect the same also happens for other providers although I did not yet verify that). That is because the jackson parser will invoke InputStream#read several times while parsing and ever since InputStreamWrapper was introduced this will always invoke InputStreamWrapper#getWrapped which in case you have a replay-able input stream as showcased here will lead to a creation of a new copy of the input stream (see MultiReadHttpServletRequest#getInputStream). The consequence of this is that jackson will never be able to finish to read the request (requests fails with 400).
I wrote a test similar to ResponseReadEntityStreamTest in order to showcase this issue:
public class ResponseReadJsonEntityStreamUsingJacksonTest extends JerseyTest {
@Override
protected TestContainerFactory getTestContainerFactory() throws TestContainerException {
return (baseUri, deploymentContext) -> {
final Server server = JettyHttpContainerFactory.createServer(baseUri, false);
final ServerConnector connector = new ServerConnector(server);
connector.setPort(9001);
server.addConnector(connector);
final ServletContainer jerseyServletContainer = new ServletContainer(deploymentContext.getResourceConfig());
final ServletHolder jettyServletHolder = new ServletHolder(jerseyServletContainer);
final ServletContextHandler context = new ServletContextHandler(ServletContextHandler.NO_SESSIONS);
context.setContextPath("/");
// filter which will change the http servlet request to have a reply-able input stream
context.addFilter(new FilterSettingMultiReadRequest(), "/*", EnumSet.allOf(DispatcherType.class));
context.addServlet(jettyServletHolder, "/api/*");
server.setHandler(context);
return new TestContainer() {
@Override
public ClientConfig getClientConfig() {
return new ClientConfig();
}
@Override
public URI getBaseUri() {
return baseUri;
}
@Override
public void start() {
try {
server.start();
} catch (Exception e) {
throw new RuntimeException(e);
}
}
@Override
public void stop() {
try {
server.stop();
} catch (Exception e) {
throw new RuntimeException(e);
}
}
};
};
}
@Override
protected Application configure() {
ResourceConfig resourceConfig = new ResourceConfig(TestResource.class);
// force jersey to use jackson for deserialization
resourceConfig.addProperties(Map.of(InternalProperties.JSON_FEATURE, JacksonFeature.class.getSimpleName()));
return resourceConfig;
}
@Test
public void readJsonWithReplayableInputStreamFailsTest() {
final Invocation.Builder requestBuilder = target("/api/v1/echo").request();
MyDto myDto = new MyDto();
myDto.setMyField("Something");
try (Response response = requestBuilder.post(Entity.entity(myDto, MediaType.APPLICATION_JSON))) {
// will fail with a 400 as jackson can never finish reading the input stream
assertEquals(400, response.getStatus());
}
}
@Path("/v1")
public static class TestResource {
@POST
@Path("/echo")
@Produces(MediaType.APPLICATION_JSON)
@Consumes(MediaType.APPLICATION_JSON)
public MyDto echo(MyDto input) {
return input;
}
}
public final static class MyDto {
private String myField;
public String getMyField() {
return myField;
}
public void setMyField(String myField) {
this.myField = myField;
}
}
}
Note that before #5669 this was not an issue (i.e the test showed here would have failed as the request would have resulted in a 200). Furthermore if FilterSettingMultiReadRequest is not registered the tests also fails (request results in 200). The test should have the same outcome both before and after #5806 is applied
@senivam do you have any estimate for when you may have some time to look into the reproducer? It's preventing a jersey update so it would be useful to know wether this new behaviour is something which will be fixed upstream
Hi @Lorenzo-Bracci, yes, I've already implemented your reproducer, and you are right—the described case is still not fixed in Jersey. The next logical step would be to fix the issue; however, due to other tasks in the queue, I do not have time to totally focus on this issue. I estimate I will return to it in the 2nd half of May. If someone here can fix it faster, a PR would be appreciated. But I still have it in the queue.
@Lorenzo-Bracci, I've accepted my own challenge to fix it faster, so I have a PR for it. However, the Eclipse infra is not healthy for a moment, and after it gets fixed I hope all checks will pass OK.