Skip to content
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.

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

ErrorPageErrorHandler does not use the proper attributes for error handling #12505

Open
jebeaudet opened this issue Nov 8, 2024 · 6 comments · May be fixed by #12586
Open

ErrorPageErrorHandler does not use the proper attributes for error handling #12505

jebeaudet opened this issue Nov 8, 2024 · 6 comments · May be fixed by #12586
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@jebeaudet
Copy link
Contributor

Jetty version(s)
12.0.14

Jetty Environment
ee10

Java version/vendor
openjdk 21.0.3 2024-04-16 LTS

OS type/version
MacOS

Description
I'm upgrading an app from Jetty 10 to 12 and I'm running into some unexpected errors being mapped by jetty. I'm using the latest Spring Boot 3.3 version with no special custom configuration.

Basically I have a request that is clearly a poor hacking attempt on the path //WEB-INF/classes/META-INF/microprofile-config.properties. This is correctly mapped to a 404 by Jetty, however during error handling, I end up with a blank 500.

Tracing into the code, I see that I end up in the ErrorPageErrorHandler class here : https://github.com/jetty/jetty.project/blob/jetty-12.0.x/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ErrorPageErrorHandler.java#L63. This fails because it can't find any Throwable(which is normal AFAICT) but it cannot find a errorStatusCode either so it falls back on the generic error page.

Walking up the stack trace, I see that it's called from the Response interface https://github.com/jetty/jetty.project/blob/jetty-12.0.x/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java#L605 with an ErrorHandler.ErrorRequest instance. The ErrorRequest instance has the proper information (the status code + message) but it's under the keys in the ErrorHandler class :
image

Now, I understand that the servlet + server part has been decoupled completely and that the ErrorPageErrorHandler is correctly looking at the servlet spec keys to find the exception or the status code. However, shouldn't this class also look at the server specific keys as a fallback?

Thanks in advance!

@jebeaudet jebeaudet added the Bug For general bugs on Jetty side label Nov 8, 2024
@jebeaudet
Copy link
Contributor Author

jebeaudet commented Nov 8, 2024

I've made a small repo to reproduce but it's really simple : https://github.com/jebeaudet/jetty-gh-12505

Checkout the code, build and run and just go to http://localhost:8080/WEB-INF/classes/META-INF/microprofile-config.properties and you'll get a 500 instead of what I would expect a 404.

Digging into it some more, I see it's because the WEB-INF and META-INF are protected path! So it really seems like a corner case since a request to /potato will properly end up in 404 with the same ErrorPageErrorHandler, though the statusCode is properly set in those cases.

@janbartel
Copy link
Contributor

@jebeaudet thanks for the repro, will look into it. I've recently fixed the ErrorHandler/ErrorPageErrorHandler for ee11 in jetty-12.1.x to ensure only the core attributes are used, see #11982. I'll look at porting that to ee10 and maybe backporting to jetty-12.0.x ee10.

@janbartel
Copy link
Contributor

@jebeaudet do you have an ErrorPageErrorHandler mapped with a code or with an exception or both? I can't tell from the repro.

@jebeaudet
Copy link
Contributor Author

@janbartel By default (as it's the case in my repro sample), Spring will inject a global error handler here. This mapper uses the special org.eclipse.jetty.server.error_page.global key to map a /error handler. No other mappings will be present.

@janbartel
Copy link
Contributor

@jebeaudet not being familiar with spring implementation, I'm getting lost down in spring calling stack in the debugger. So I tried creating a repro case as a jetty unit test. The code is here:

    @Test
    public void testErrorPageErrorHandler() throws Exception
    {
        ServletContextHandler contextHandler = new ServletContextHandler(ServletContextHandler.NO_SECURITY | ServletContextHandler.NO_SESSIONS);
        contextHandler.setContextPath("/foo");
        contextHandler.setBaseResourceAsPath(Path.of("/tmp"));
        ServletHolder defaultHolder = new ServletHolder(new DefaultServlet());
        defaultHolder.setDisplayName("default");

        contextHandler.addServlet(defaultHolder, "/");
        contextHandler.addServlet(ErrorDumpServlet.class, "/error/*");
        ErrorPageErrorHandler errorPageErrorHandler = new ErrorPageErrorHandler();
        //errorPageErrorHandler.addErrorPage(404, "/error/TestException");
        errorPageErrorHandler.addErrorPage(ErrorPageErrorHandler.GLOBAL_ERROR_PAGE, "/error/TestException");
        contextHandler.setErrorHandler(errorPageErrorHandler);
        startServer(contextHandler);

        try (StacklessLogging stackless = new StacklessLogging(ServletChannel.class))
        {
            StringBuilder rawRequest = new StringBuilder();
            rawRequest.append("GET /foo/this/does/not/exist").append(" HTTP/1.1\r\n");
            rawRequest.append("Host: test\r\n");
            rawRequest.append("Connection: close\r\n");
            rawRequest.append("\r\n");

            String rawResponse = _connector.getResponse(rawRequest.toString());

            HttpTester.Response response = HttpTester.parseResponse(rawResponse);
            System.err.println(response);
            System.err.println(response.getContent());
        }
    }

    private void startServer(ServletContextHandler servletContextHandler) throws Exception
    {
        _server = new Server();
        _connector = new LocalConnector(_server);
        _server.addConnector(_connector);

        // Add regression test for double dispatch
        servletContextHandler.addFilter(EnsureSingleDispatchFilter.class, "/*", EnumSet.allOf(DispatcherType.class));
        _server.setHandler(servletContextHandler);
        //_server.setDumpAfterStart(true);
        _server.start();
    }

And I can't seem to reproduce the error 500, I always get error 404 handled correctly. So it seems this might only happen with the spring integration? Could you maybe mutate my test code to more closely match what spring is doing so I can reproduce the error so I can debug it easily?

@janbartel
Copy link
Contributor

@jebeaudet never mind, I've found it! Converted it to a WebAppContext, which has the protections on WEB-INF and I was able to generate the problem. I think I've got enough to work with now:

@Test
    public void testErrorPage() throws Exception
    {
        WebAppContext contextHandler = new WebAppContext();
        contextHandler.setContextPath("/foo");
        contextHandler.setBaseResourceAsPath(Path.of("/tmp"));
        ServletHolder defaultHolder = new ServletHolder(new DefaultServlet());
        defaultHolder.setDisplayName("default");

        contextHandler.addServlet(defaultHolder, "/");
        contextHandler.addServlet(ErrorDumpServlet.class, "/error/*");
        contextHandler.addServlet(GlobalErrorDumpServlet.class, "/global/*");
        ErrorPageErrorHandler errorPageErrorHandler = new ErrorPageErrorHandler();
        errorPageErrorHandler.addErrorPage(404, "/error/TestException");
        errorPageErrorHandler.addErrorPage(ErrorPageErrorHandler.GLOBAL_ERROR_PAGE, "/global/TestException");
        contextHandler.setErrorHandler(errorPageErrorHandler);
        Server server = new Server();
        server.setHandler(contextHandler);

        LocalConnector connector = new LocalConnector(server);
        server.addConnector(connector);
        server.start();

        try (StacklessLogging stackless = new StacklessLogging(ServletChannel.class))
        {
            StringBuilder rawRequest = new StringBuilder();
            rawRequest.append("GET /foo/WEB-INF/classes/this/does/not/exist").append(" HTTP/1.1\r\n");
            rawRequest.append("Host: test\r\n");
            rawRequest.append("Connection: close\r\n");
            rawRequest.append("\r\n");

            String rawResponse = connector.getResponse(rawRequest.toString());

            HttpTester.Response response = HttpTester.parseResponse(rawResponse);
            System.err.println(response);
            System.err.println(response.getContent());
        }
    }

@sbordet sbordet moved this to 🏗 In progress in Jetty 12.0.16 FROZEN Nov 20, 2024
@janbartel janbartel linked a pull request Nov 27, 2024 that will close this issue
@joakime joakime moved this to 🏗 In progress in Jetty 12.1.0 Dec 2, 2024
@joakime joakime linked a pull request Dec 2, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
Status: 🏗 In progress
Development

Successfully merging a pull request may close this issue.

2 participants