From 5d9c55be8f281c6c2e49825822413c6d203b1487 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 6 Feb 2019 17:12:02 +1100 Subject: [PATCH] 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 --- .../rewrite/handler/ResponsePatternRule.java | 46 +++++++++------ .../jetty/rewrite/handler/ValidUrlRule.java | 29 ++++++---- .../handler/ResponsePatternRuleTest.java | 56 +++++-------------- .../rewrite/handler/ValidUrlRuleTest.java | 19 ++++--- .../security/SpecExampleConstraintTest.java | 12 ++-- .../org/eclipse/jetty/server/Response.java | 14 ++--- .../jetty/server/handler/ErrorHandler.java | 9 ++- .../eclipse/jetty/server/ResponseTest.java | 45 ++++++++------- .../ssl/SniSslConnectionFactoryTest.java | 18 +++--- .../eclipse/jetty/servlet/ErrorPageTest.java | 10 ++-- 10 files changed, 132 insertions(+), 126 deletions(-) diff --git a/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/ResponsePatternRule.java b/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/ResponsePatternRule.java index f0ab22576a6..62d4b4b221e 100644 --- a/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/ResponsePatternRule.java +++ b/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 String _reason; + private int _code; + 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); - } - else - { - response.setStatus(code); + if (_message != null && !_message.isEmpty()) + response.sendError(_code, _message); + else + 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 +"]"; } } diff --git a/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/ValidUrlRule.java b/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/ValidUrlRule.java index 650fe11012c..cff2080f697 100644 --- a/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/ValidUrlRule.java +++ b/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) - { - response.sendError(code,_reason); - } + if (_message!=null && !_message.isEmpty()) + response.sendError(code, _message); 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 + "]"; } } diff --git a/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ResponsePatternRuleTest.java b/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ResponsePatternRuleTest.java index 542a04d0434..29ad8c3d2cb 100644 --- a/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ResponsePatternRuleTest.java +++ b/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("" + i); - _rule.setReason("reason" + i); - _rule.apply(null, _request, _response); + _rule.setCode("499"); + _rule.setMessage("Message 499"); + _rule.apply(null, _request, _response); - assertEquals(i, _response.getStatus()); - assertEquals(null, _response.getReason()); - } - } - - @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(); - } + assertEquals(499, _response.getStatus()); + assertEquals( "Message 499", _request.getAttribute(Dispatcher.ERROR_MESSAGE)); } } diff --git a/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ValidUrlRuleTest.java b/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ValidUrlRuleTest.java index 9b1855faf13..846194b15ba 100644 --- a/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ValidUrlRuleTest.java +++ b/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 static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertTrue; - +import org.eclipse.jetty.server.Dispatcher; 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") diff --git a/jetty-security/src/test/java/org/eclipse/jetty/security/SpecExampleConstraintTest.java b/jetty-security/src/test/java/org/eclipse/jetty/security/SpecExampleConstraintTest.java index 74e241334d9..e057058a83a 100644 --- a/jetty-security/src/test/java/org/eclipse/jetty/security/SpecExampleConstraintTest.java +++ b/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" + diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java index 7cc8c604154..d4200db3ea4 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java +++ b/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); + _reason=HttpStatus.getMessage(code); if (message==null) - { - _reason=HttpStatus.getMessage(code); 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; } } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java index 84088146cd3..48d38feee41 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java +++ b/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); } /** diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java index 812519b8210..38e7ac1025d 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java +++ b/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(); } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SniSslConnectionFactoryTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SniSslConnectionFactoryTest.java index 7b39695b5a4..ec912540f29 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SniSslConnectionFactoryTest.java +++ b/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 { diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ErrorPageTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ErrorPageTest.java index e6414d7b561..8a881c339a2 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ErrorPageTest.java +++ b/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"));