Issue #3225 sendError does not set reason.

The message string passed to sendError should only be used for the
error page content and not for the reason string.

Setting the reason in a response via setStatus(int,String) is deprecated

Signed-off-by: Greg Wilkins <gregw@webtide.com>

jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/ResponsePatternRule.java

@@ -30,23 +30,23 @@ import org.eclipse.jetty.util.annotation.Name;
  */
 public class ResponsePatternRule extends PatternRule
 {
-    private String _code;
+    private int _code;
-    private String _reason;
+    private String _message;
 
     /* ------------------------------------------------------------ */
     public ResponsePatternRule()
     {
-        this(null,null,"");
+        this(null,null,null);
     }
 
     /* ------------------------------------------------------------ */
-    public ResponsePatternRule(@Name("pattern") String pattern, @Name("code") String code, @Name("reason") String reason)
+    public ResponsePatternRule(@Name("pattern") String pattern, @Name("code") String code, @Name("message") String message)
     {
         super(pattern);
         _handling = true;
         _terminating = true;
         setCode(code);
-        setReason(reason);
+        setMessage(message);
     }
 
     /* ------------------------------------------------------------ */
@@ -56,19 +56,34 @@ public class ResponsePatternRule extends PatternRule
      */
     public void setCode(String code)
     {
-        _code = code;
+        _code = code==null ? 0 : Integer.parseInt(code);
     }
 
     /* ------------------------------------------------------------ */
     /**
      * Sets the reason for the response status code. Reasons will only reflect
      * if the code value is greater or equal to 400.
-     * 
+     * @deprecated Reason has been replaced by message
      * @param reason the reason
+     * @see #setMessage(String)
      */
+    @Deprecated
     public void setReason(String reason)
     {
-        _reason = reason;
+        setMessage(reason);
+    }
+
+    /* ------------------------------------------------------------ */
+    /**
+     * Sets the message for the {@link org.eclipse.jetty.server.Response#sendError(int, String)} method.
+     * Reasons will only reflect
+     * if the code value is greater or equal to 400.
+     *
+     * @param message the reason
+     */
+    public void setMessage(String message)
+    {
+        _message = message;
     }
 
     /* ------------------------------------------------------------ */
@@ -79,16 +94,13 @@ public class ResponsePatternRule extends PatternRule
     @Override
     public String apply(String target, HttpServletRequest request, HttpServletResponse response) throws IOException
     {
-        int code = Integer.parseInt(_code);
-
         // status code 400 and up are error codes
-        if (code >= 400)
+        if (_code>0)
         {
-            response.sendError(code, _reason);
+            if (_message != null && !_message.isEmpty())
-        }
+                response.sendError(_code, _message);
             else
-        {
+                response.setStatus(_code);
-            response.setStatus(code);
         }
         return target;
     }
@@ -100,6 +112,6 @@ public class ResponsePatternRule extends PatternRule
     @Override
     public String toString()
     {
-        return super.toString()+"["+_code+","+_reason+"]";
+        return super.toString()+"["+_code+","+ _message +"]";
     }
 }

jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/ValidUrlRule.java

@@ -42,7 +42,7 @@ public class ValidUrlRule extends Rule
     private static final Logger LOG = Log.getLogger(ValidUrlRule.class);
 
     String _code = "400";
-    String _reason = "Illegal Url";
+    String _message = "Illegal Url";
     
     public ValidUrlRule()
     {
@@ -64,13 +64,26 @@ public class ValidUrlRule extends Rule
 
     /* ------------------------------------------------------------ */
     /**
-     * Sets the reason for the response status code. Reasons will only reflect if the code value is greater or equal to 400.
+     * Sets the reason for the response status code.
      * 
      * @param reason the reason
+     * @deprecated use {@link #setMessage(String)}
      */
+    @Deprecated
     public void setReason(String reason)
     {
-        _reason = reason;
+        _message = reason;
+    }
+
+    /* ------------------------------------------------------------ */
+    /**
+     * Sets the message for the {@link org.eclipse.jetty.server.Response#sendError(int, String)} method.
+     *
+     * @param message the message
+     */
+    public void setMessage(String message)
+    {
+        _message = message;
     }
 
     @Override
@@ -90,14 +103,10 @@ public class ValidUrlRule extends Rule
                 int code = Integer.parseInt(_code);
 
                 // status code 400 and up are error codes so include a reason
-                if (code >= 400)
+                if (_message!=null && !_message.isEmpty())
-                {
+                    response.sendError(code, _message);
-                    response.sendError(code,_reason);
-                }
                 else
-                {
                     response.setStatus(code);
-                }
 
                 // we have matched, return target and consider it is handled
                 return target;
@@ -121,6 +130,6 @@ public class ValidUrlRule extends Rule
     @Override
     public String toString()
     {
-        return super.toString() + "[" + _code + ":" + _reason + "]";
+        return super.toString() + "[" + _code + ":" + _message + "]";
     }
 }

jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ResponsePatternRuleTest.java

@@ -18,13 +18,15 @@
 
 package org.eclipse.jetty.rewrite.handler;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
-
 import java.io.IOException;
 
+import org.eclipse.jetty.server.Dispatcher;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
 public class ResponsePatternRuleTest extends AbstractRuleTestCase
 {
     private ResponsePatternRule _rule;
@@ -38,57 +40,27 @@ public class ResponsePatternRuleTest extends AbstractRuleTestCase
     }
 
     @Test
-    public void testStatusCodeNoReason() throws IOException
+    public void testStatusCodeNoMessage() throws IOException
     {
-        for (int i = 1; i < 400; i++)
+        for (int i = 1; i < 600; i++)
         {
             _rule.setCode("" + i);
+            _rule.setMessage(null);
             _rule.apply(null, _request, _response);
 
             assertEquals(i, _response.getStatus());
+            assertNull(_request.getAttribute(Dispatcher.ERROR_MESSAGE));
         }
     }
 
     @Test
-    public void testStatusCodeWithReason() throws IOException
+    public void testStatusCodeMessage() throws IOException
     {
-        for (int i = 1; i < 400; i++)
+        _rule.setCode("499");
-        {
+        _rule.setMessage("Message 499");
-            _rule.setCode("" + i);
-            _rule.setReason("reason" + i);
         _rule.apply(null, _request, _response);
 
-            assertEquals(i, _response.getStatus());
+        assertEquals(499, _response.getStatus());
-            assertEquals(null, _response.getReason());
+        assertEquals( "Message 499", _request.getAttribute(Dispatcher.ERROR_MESSAGE));
-        }
-    }
-
-    @Test
-    public void testErrorStatusNoReason() throws IOException
-    {
-        for (int i = 400; i < 600; i++)
-        {
-            _rule.setCode("" + i);
-            _rule.apply(null, _request, _response);
-
-            assertEquals(i, _response.getStatus());
-            assertEquals("", _response.getReason());
-            super.reset();
-        }
-    }
-
-    @Test
-    public void testErrorStatusWithReason() throws IOException
-    {
-        for (int i = 400; i < 600; i++)
-        {
-            _rule.setCode("" + i);
-            _rule.setReason("reason-" + i);
-            _rule.apply(null, _request, _response);
-
-            assertEquals(i, _response.getStatus());
-            assertEquals("reason-" + i, _response.getReason());
-            super.reset();
-        }
     }
 }

jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ValidUrlRuleTest.java

@@ -18,14 +18,15 @@
 
 package org.eclipse.jetty.rewrite.handler;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
+import org.eclipse.jetty.server.Dispatcher;
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
 @SuppressWarnings("unused")
 public class ValidUrlRuleTest extends AbstractRuleTestCase
 {
@@ -61,16 +62,16 @@ public class ValidUrlRuleTest extends AbstractRuleTestCase
     }
 
     @Test
-    public void testInvalidUrlWithReason() throws Exception
+    public void testInvalidUrlWithMessage() throws Exception
     {
         _rule.setCode("405");
-        _rule.setReason("foo");
+        _rule.setMessage("foo");
         _request.setURIPathQuery("/%00/");
         
         String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response);
 
         assertEquals(405,_response.getStatus());
-        assertEquals("foo",_response.getReason());
+        assertEquals( "foo", _request.getAttribute(Dispatcher.ERROR_MESSAGE));
     }
     
     @Test
@@ -83,7 +84,7 @@ public class ValidUrlRuleTest extends AbstractRuleTestCase
         String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response);
 
         assertEquals(405,_response.getStatus());
-        assertEquals("foo",_response.getReason());
+        assertEquals( "foo", _request.getAttribute(Dispatcher.ERROR_MESSAGE));
     }
     
     @Disabled("Not working in jetty-9")
@@ -97,7 +98,7 @@ public class ValidUrlRuleTest extends AbstractRuleTestCase
         String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response);
 
         assertEquals(405,_response.getStatus());
-        assertEquals("foo",_response.getReason());
+        assertEquals( "foo", _request.getAttribute(Dispatcher.ERROR_MESSAGE));
     }
 
     @Disabled("Not working in jetty-9")

jetty-security/src/test/java/org/eclipse/jetty/security/SpecExampleConstraintTest.java

@@ -18,11 +18,6 @@
 
 package org.eclipse.jetty.security;
 
-import static org.hamcrest.Matchers.startsWith;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.HashSet;
@@ -48,6 +43,11 @@ import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.startsWith;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
 /**
  * @version $Revision: 1441 $ $Date: 2010-04-02 12:28:17 +0200 (Fri, 02 Apr 2010) $
  */
@@ -323,7 +323,7 @@ public class SpecExampleConstraintTest
         response = _connector.getResponse("POST /ctx/acme/wholesale/index.html HTTP/1.0\r\n" +
                                            "Authorization: Basic " + B64Code.encode("chris:password") + "\r\n" +
                                            "\r\n");
-        assertThat(response,startsWith("HTTP/1.1 403 !"));
+        assertThat(response,startsWith("HTTP/1.1 403 "));
         
         //a user in role HOMEOWNER can do a GET
         response = _connector.getResponse("GET /ctx/acme/retail/index.html HTTP/1.0\r\n" +

jetty-server/src/main/java/org/eclipse/jetty/server/Response.java

@@ -633,13 +633,9 @@ public class Response implements HttpServletResponse
 
         Request request = _channel.getRequest();
         Throwable cause = (Throwable)request.getAttribute(Dispatcher.ERROR_EXCEPTION);
-        if (message==null)
-        {    
         _reason=HttpStatus.getMessage(code);
+        if (message==null)
             message=cause==null?_reason:cause.toString();
-        }    
-        else
-            _reason=message;
 
         // If we are allowed to have a body, then produce the error page.
         if (code != SC_NO_CONTENT && code != SC_NOT_MODIFIED &&
@@ -873,19 +869,19 @@ public class Response implements HttpServletResponse
 
     @Override
     @Deprecated
-    public void setStatus(int sc, String sm)
+    public void setStatus(int sc, String  message)
     {
-        setStatusWithReason(sc,sm);
+        setStatusWithReason(sc, null);
     }
 
-    public void setStatusWithReason(int sc, String sm)
+    public void setStatusWithReason(int sc, String message)
     {
         if (sc <= 0)
             throw new IllegalArgumentException();
         if (!isIncluding())
         {
             _status = sc;
-            _reason = sm;
+            _reason = message;
         }
     }
 

jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java

@@ -139,7 +139,14 @@ public class ErrorHandler extends AbstractHandler
 
         if (_cacheControl != null)
             response.setHeader(HttpHeader.CACHE_CONTROL.asString(), _cacheControl);
-        generateAcceptableResponse(baseRequest,request,response,response.getStatus(),baseRequest.getResponse().getReason());
+
+        String message = (String)request.getAttribute(RequestDispatcher.ERROR_MESSAGE);
+        if (message==null)
+            message = baseRequest.getResponse().getReason();
+        if (message==null)
+            message = HttpStatus.getMessage(response.getStatus());
+
+        generateAcceptableResponse(baseRequest,request,response,response.getStatus(),message);
     }
 
     /** 

jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java

@@ -18,21 +18,6 @@
 
 package org.eclipse.jetty.server;
 
-import static java.nio.charset.StandardCharsets.UTF_8;
-import static org.hamcrest.CoreMatchers.allOf;
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.containsInAnyOrder;
-import static org.hamcrest.Matchers.containsString;
-import static org.hamcrest.Matchers.is;
-import static org.hamcrest.Matchers.not;
-import static org.hamcrest.Matchers.nullValue;
-import static org.hamcrest.Matchers.startsWith;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertThrows;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
 import java.io.IOException;
 import java.io.InputStreamReader;
 import java.io.LineNumberReader;
@@ -62,6 +47,7 @@ import org.eclipse.jetty.http.CookieCompliance;
 import org.eclipse.jetty.http.HttpField;
 import org.eclipse.jetty.http.HttpFields;
 import org.eclipse.jetty.http.HttpHeader;
+import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.http.HttpURI;
 import org.eclipse.jetty.http.HttpVersion;
 import org.eclipse.jetty.http.MetaData;
@@ -86,6 +72,21 @@ import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.hamcrest.CoreMatchers.allOf;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.nullValue;
+import static org.hamcrest.Matchers.startsWith;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
 public class ResponseTest
 {
 
@@ -676,7 +677,9 @@ public class ResponseTest
 
         response.sendError(500, "Database Error");
         assertEquals(500, response.getStatus());
-        assertEquals("Database Error", response.getReason());
+        assertEquals("Server Error", response.getReason());
+        assertThat(BufferUtil.toString(_content), containsString("Database Error"));
+
         assertEquals("must-revalidate,no-cache,no-store", response.getHeader(HttpHeader.CACHE_CONTROL.asString()));
 
         response = getResponse();
@@ -689,7 +692,8 @@ public class ResponseTest
 
         response.sendError(406, "Super Nanny");
         assertEquals(406, response.getStatus());
-        assertEquals("Super Nanny", response.getReason());
+        assertEquals(HttpStatus.Code.NOT_ACCEPTABLE.getMessage(), response.getReason());
+        assertThat(BufferUtil.toString(_content), containsString("Super Nanny"));
         assertEquals("must-revalidate,no-cache,no-store", response.getHeader(HttpHeader.CACHE_CONTROL.asString()));
     }
     
@@ -707,7 +711,8 @@ public class ResponseTest
 
         response.sendError(500, "Database Error");
         assertEquals(500, response.getStatus());
-        assertEquals("Database Error", response.getReason());
+        assertEquals("Server Error", response.getReason());
+        assertThat(BufferUtil.toString(_content), is(""));
         assertThat(response.getHeader(HttpHeader.CACHE_CONTROL.asString()),Matchers.nullValue());
 
         response = getResponse();
@@ -720,7 +725,8 @@ public class ResponseTest
 
         response.sendError(406, "Super Nanny");
         assertEquals(406, response.getStatus());
-        assertEquals("Super Nanny", response.getReason());
+        assertEquals(HttpStatus.Code.NOT_ACCEPTABLE.getMessage(), response.getReason());
+        assertThat(BufferUtil.toString(_content), is(""));
         assertThat(response.getHeader(HttpHeader.CACHE_CONTROL.asString()),Matchers.nullValue());
     }
 
@@ -1384,6 +1390,7 @@ public class ResponseTest
     {
         _channel.recycle();
         _channel.getRequest().setMetaData(new MetaData.Request("GET",new HttpURI("/path/info"),HttpVersion.HTTP_1_0,new HttpFields()));
+        BufferUtil.clear(_content);
         return _channel.getResponse();
     }
 

jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SniSslConnectionFactoryTest.java

@@ -18,12 +18,6 @@
 
 package org.eclipse.jetty.server.ssl;
 
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.containsString;
-import static org.hamcrest.Matchers.startsWith;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.IOException;
@@ -70,6 +64,12 @@ import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.startsWith;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
 public class SniSslConnectionFactoryTest
 {
     private Server _server;
@@ -268,8 +268,9 @@ public class SniSslConnectionFactoryTest
             output.flush();
 
             response = response(input);
+            String body = IO.toString(input);
             assertThat(response,startsWith("HTTP/1.1 400 "));
-            assertThat(response, containsString("Host does not match SNI"));
+            assertThat(body, containsString("Host does not match SNI"));
         }
         finally
         {
@@ -326,7 +327,8 @@ public class SniSslConnectionFactoryTest
 
             response = response(input);
             assertTrue(response.startsWith("HTTP/1.1 400 "));
-            assertThat(response, Matchers.containsString("Host does not match SNI"));
+            String body = IO.toString(input);
+            assertThat(body, Matchers.containsString("Host does not match SNI"));
         }
         finally
         {

jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ErrorPageTest.java

@@ -18,10 +18,6 @@
 
 package org.eclipse.jetty.servlet;
 
-import static org.hamcrest.CoreMatchers.containsString;
-import static org.hamcrest.CoreMatchers.not;
-import static org.hamcrest.MatcherAssert.assertThat;
-
 import java.io.IOException;
 import java.io.PrintWriter;
 
@@ -42,6 +38,10 @@ import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.MatcherAssert.assertThat;
+
 public class ErrorPageTest
 {
     private Server _server;
@@ -168,7 +168,7 @@ public class ErrorPageTest
         try (StacklessLogging ignore = new StacklessLogging(Dispatcher.class))
         {
             String response = _connector.getResponse("GET /app?baa=%88%A4 HTTP/1.0\r\n\r\n");
-            assertThat(response, Matchers.containsString("HTTP/1.1 400 Bad query encoding"));
+            assertThat(response, Matchers.containsString("HTTP/1.1 400 "));
             assertThat(response, Matchers.containsString("ERROR_PAGE: /BadMessageException"));
             assertThat(response, Matchers.containsString("ERROR_MESSAGE: Bad query encoding"));
             assertThat(response, Matchers.containsString("ERROR_CODE: 400"));