From dff8fb6b90952a92e308d7687bfaf597393876f4 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Wed, 29 Mar 2017 23:00:39 -0400 Subject: [PATCH] Issue parameter decoding (#1330) * Issue #1327 - Removing non-standard (Microsoft only) %uXXXX support * Issue #1322 - Removing attempts at "solving" bad behavior in UrlEncoded + No longer captures NumberFormatException and Utf8Exception and NotUtf8Exception for purposes of "recovering" from a bad encoding. + Introduces UrlEncode.decodeHexChar() and .decodeHexByte() to make reporting of bad encoding more clear. * Issue #1316 - throw a BadMessageException on bad parameter parsing + If BadMessageException is uncaught by the webapp, this will result in an error 400 response message. + If an application decides to catch the BadMessageException, they can choose to ignore the exception and do their own error reporting. + This piggybacks on Issue #1327 and Issue #1322 --- .../jetty/http/BadMessageException.java | 5 + .../org/eclipse/jetty/http/HttpURITest.java | 36 +- .../org/eclipse/jetty/server/Request.java | 22 +- .../org/eclipse/jetty/server/RequestTest.java | 150 ++++--- .../org/eclipse/jetty/util/UrlEncoded.java | 403 ++++++------------ .../eclipse/jetty/util/URLEncodedTest.java | 125 ++---- .../util/UrlEncodedInvalidEncodingTest.java | 87 ++++ .../jetty/util/UrlEncodedUtf8Test.java | 39 -- 8 files changed, 352 insertions(+), 515 deletions(-) create mode 100644 jetty-util/src/test/java/org/eclipse/jetty/util/UrlEncodedInvalidEncodingTest.java diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/BadMessageException.java b/jetty-http/src/main/java/org/eclipse/jetty/http/BadMessageException.java index 3b89ab63dec..daaea17b08d 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/BadMessageException.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/BadMessageException.java @@ -45,6 +45,11 @@ public class BadMessageException extends RuntimeException this(400,reason); } + public BadMessageException(String reason, Throwable cause) + { + this(400, reason, cause); + } + public BadMessageException(int code, String reason) { super(code+": "+reason); diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java index 43aad78ba77..0853abfa65a 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java @@ -21,16 +21,15 @@ package org.eclipse.jetty.http; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; -import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; -import java.io.UnsupportedEncodingException; -import java.net.URLDecoder; import java.net.URLEncoder; import java.nio.charset.StandardCharsets; import org.eclipse.jetty.util.MultiMap; -import org.eclipse.jetty.util.Utf8Appendable; -import org.junit.Assert; import org.junit.Test; public class HttpURITest @@ -100,33 +99,6 @@ public class HttpURITest assertThat(uri.getHost(),is("foo")); assertThat(uri.getPath(),is("/bar")); } - - - - @Test - public void testUnicodeErrors() throws UnsupportedEncodingException - { - String uri="http://server/path?invalid=data%uXXXXhere%u000"; - try - { - URLDecoder.decode(uri,"UTF-8"); - Assert.assertTrue(false); - } - catch (IllegalArgumentException e) - { - } - - HttpURI huri=new HttpURI(uri); - MultiMap params = new MultiMap<>(); - huri.decodeQueryTo(params); - assertEquals("data"+Utf8Appendable.REPLACEMENT+"here"+Utf8Appendable.REPLACEMENT,params.getValue("invalid",0)); - - huri=new HttpURI(uri); - params = new MultiMap<>(); - huri.decodeQueryTo(params,StandardCharsets.UTF_8); - assertEquals("data"+Utf8Appendable.REPLACEMENT+"here"+Utf8Appendable.REPLACEMENT,params.getValue("invalid",0)); - - } @Test public void testExtB() throws Exception diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 493f470fd94..417f5502c6e 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -362,13 +362,31 @@ public class Request implements HttpServletRequest // once extracted and may have already been extracted by getParts() or // by a processing happening after a form-based authentication. if (_contentParameters == null) - extractContentParameters(); + { + try + { + extractContentParameters(); + } + catch(IllegalStateException | IllegalArgumentException e) + { + throw new BadMessageException("Unable to parse form content", e); + } + } } // Extract query string parameters; these may be replaced by a forward() // and may have already been extracted by mergeQueryParameters(). if (_queryParameters == null) - extractQueryParameters(); + { + try + { + extractQueryParameters(); + } + catch(IllegalStateException | IllegalArgumentException e) + { + throw new BadMessageException("Unable to parse URI query", e); + } + } // Do parameters need to be combined? if (_queryParameters==NO_PARAMS || _queryParameters.size()==0) diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java index a132e991b7a..95ab4110069 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java @@ -18,6 +18,7 @@ package org.eclipse.jetty.server; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.startsWith; import static org.junit.Assert.assertEquals; @@ -57,15 +58,16 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.servlet.http.Part; +import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.http.MimeTypes; import org.eclipse.jetty.server.handler.AbstractHandler; import org.eclipse.jetty.server.handler.ContextHandler; +import org.eclipse.jetty.server.handler.ErrorHandler; import org.eclipse.jetty.toolchain.test.FS; import org.eclipse.jetty.toolchain.test.MavenTestingUtils; import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.MultiPartInputStreamParser; -import org.eclipse.jetty.util.Utf8Appendable; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; import org.eclipse.jetty.util.log.StacklessLogging; @@ -96,6 +98,10 @@ public class RequestTest _server.addConnector(_connector); _handler = new RequestHandler(); _server.setHandler(_handler); + + ErrorHandler errors = new ErrorHandler(); + errors.setShowStacks(true); + _server.addBean(errors); _server.start(); } @@ -114,13 +120,19 @@ public class RequestTest @Override public boolean check(HttpServletRequest request,HttpServletResponse response) { - Map map = null; - //do the parse - map = request.getParameterMap(); - assertEquals("aaa"+Utf8Appendable.REPLACEMENT+"bbb",map.get("param")[0]); - assertEquals("value",map.get("other")[0]); - - return true; + try + { + Map map = null; + // do the parse + map = request.getParameterMap(); + return false; + } + catch(BadMessageException e) + { + // Should be able to retrieve the raw query + String rawQuery = request.getQueryString(); + return rawQuery.equals("param=aaa%ZZbbb&other=value"); + } } }; @@ -134,7 +146,6 @@ public class RequestTest String responses=_connector.getResponses(request); assertTrue(responses.startsWith("HTTP/1.1 200")); - } @Test @@ -264,7 +275,7 @@ public class RequestTest "Accept-Language: XX;q=0, en-au;q=0.9\r\n"+ "\r\n"; String response = _connector.getResponses(request); - assertThat(response,Matchers.containsString(" 200 OK")); + assertThat(response, containsString(" 200 OK")); } @@ -401,8 +412,18 @@ public class RequestTest @Override public boolean check(HttpServletRequest request,HttpServletResponse response) { - String value=request.getParameter("param"); - return value.startsWith("aaa") && value.endsWith("bb"); + try + { + // This throws an exception if attempted + request.getParameter("param"); + return false; + } + catch(BadMessageException e) + { + // Should still be able to get the raw query. + String rawQuery = request.getQueryString(); + return rawQuery.equals("param=aaa%E7bbb"); + } } }; @@ -517,7 +538,7 @@ public class RequestTest "Connection: close\n"+ "\n"); int i=0; - assertThat(response, Matchers.containsString("200 OK")); + assertThat(response, containsString("200 OK")); assertEquals("http://myhost/",results.get(i++)); assertEquals("0.0.0.0",results.get(i++)); assertEquals("myhost",results.get(i++)); @@ -531,7 +552,7 @@ public class RequestTest "Connection: close\n"+ "\n"); i=0; - assertThat(response, Matchers.containsString("200 OK")); + assertThat(response, containsString("200 OK")); assertEquals("http://myhost:8888/",results.get(i++)); assertEquals("0.0.0.0",results.get(i++)); assertEquals("myhost",results.get(i++)); @@ -543,7 +564,7 @@ public class RequestTest "GET http://myhost:8888/ HTTP/1.0\n"+ "\n"); i=0; - assertThat(response, Matchers.containsString("200 OK")); + assertThat(response, containsString("200 OK")); assertEquals("http://myhost:8888/",results.get(i++)); assertEquals("0.0.0.0",results.get(i++)); assertEquals("myhost",results.get(i++)); @@ -556,7 +577,7 @@ public class RequestTest "Connection: close\n"+ "\n"); i=0; - assertThat(response, Matchers.containsString("200 OK")); + assertThat(response, containsString("200 OK")); assertEquals("http://myhost:8888/",results.get(i++)); assertEquals("0.0.0.0",results.get(i++)); assertEquals("myhost",results.get(i++)); @@ -571,7 +592,7 @@ public class RequestTest "\n"); i=0; - assertThat(response, Matchers.containsString("200 OK")); + assertThat(response, containsString("200 OK")); assertEquals("http://1.2.3.4/",results.get(i++)); assertEquals("0.0.0.0",results.get(i++)); assertEquals("1.2.3.4",results.get(i++)); @@ -585,7 +606,7 @@ public class RequestTest "Connection: close\n"+ "\n"); i=0; - assertThat(response, Matchers.containsString("200 OK")); + assertThat(response, containsString("200 OK")); assertEquals("http://1.2.3.4:8888/",results.get(i++)); assertEquals("0.0.0.0",results.get(i++)); assertEquals("1.2.3.4",results.get(i++)); @@ -599,7 +620,7 @@ public class RequestTest "Connection: close\n"+ "\n"); i=0; - assertThat(response, Matchers.containsString("200 OK")); + assertThat(response, containsString("200 OK")); assertEquals("http://[::1]/",results.get(i++)); assertEquals("0.0.0.0",results.get(i++)); assertEquals("[::1]",results.get(i++)); @@ -613,7 +634,7 @@ public class RequestTest "Connection: close\n"+ "\n"); i=0; - assertThat(response, Matchers.containsString("200 OK")); + assertThat(response, containsString("200 OK")); assertEquals("http://[::1]:8888/",results.get(i++)); assertEquals("0.0.0.0",results.get(i++)); assertEquals("[::1]",results.get(i++)); @@ -629,7 +650,7 @@ public class RequestTest "Connection: close\n"+ "\n"); i=0; - assertThat(response, Matchers.containsString("200 OK")); + assertThat(response, containsString("200 OK")); assertEquals("https://[::1]/",results.get(i++)); assertEquals("remote",results.get(i++)); assertEquals("[::1]",results.get(i++)); @@ -645,7 +666,7 @@ public class RequestTest "x-forwarded-proto: https\n"+ "\n"); i=0; - assertThat(response, Matchers.containsString("200 OK")); + assertThat(response, containsString("200 OK")); assertEquals("https://[::1]:8888/",results.get(i++)); assertEquals("remote",results.get(i++)); assertEquals("[::1]",results.get(i++)); @@ -693,7 +714,7 @@ public class RequestTest Log.getRootLogger().debug("test l={}",l); String response = _connector.getResponses(request); Log.getRootLogger().debug(response); - assertThat(response, Matchers.containsString(" 200 OK")); + assertThat(response, containsString(" 200 OK")); assertEquals(l,length.get()); content+="x"; } @@ -722,7 +743,7 @@ public class RequestTest "\r\n"+ content; String response = _connector.getResponses(request); - assertThat(response,Matchers.containsString(" 200 OK")); + assertThat(response, containsString(" 200 OK")); } @Test @@ -746,7 +767,7 @@ public class RequestTest "\r\n"+ content; String response = _connector.getResponses(request); - assertThat(response,Matchers.containsString(" 200 OK")); + assertThat(response, containsString(" 200 OK")); } @Test @@ -772,7 +793,7 @@ public class RequestTest "\r\n"+ content; String response = _connector.getResponses(request); - assertThat(response,Matchers.containsString(" 200 OK")); + assertThat(response, containsString(" 200 OK")); } @Test @@ -800,7 +821,7 @@ public class RequestTest "\r\n"+ content; String response = _connector.getResponses(request); - assertThat(response,Matchers.containsString(" 200 OK")); + assertThat(response, containsString(" 200 OK")); } @Test @@ -828,7 +849,7 @@ public class RequestTest "\r\n"+ content; String response = _connector.getResponses(request); - assertThat(response,Matchers.containsString(" 200 OK")); + assertThat(response, containsString(" 200 OK")); } @@ -1021,8 +1042,8 @@ public class RequestTest "Host: myhost\n"+ "Connection: close\n"+ "\n"); - assertThat(response,Matchers.containsString(" 302 Found")); - assertThat(response,Matchers.containsString("Location: http://myhost/foo")); + assertThat(response, containsString(" 302 Found")); + assertThat(response, containsString("Location: http://myhost/foo")); } @Test @@ -1091,9 +1112,9 @@ public class RequestTest "\n", 200, TimeUnit.MILLISECONDS ); - assertThat(response, Matchers.containsString("200")); - assertThat(response, Matchers.not(Matchers.containsString("Connection: close"))); - assertThat(response, Matchers.containsString("Hello World")); + assertThat(response, containsString("200")); + assertThat(response, Matchers.not(containsString("Connection: close"))); + assertThat(response, containsString("Hello World")); response=_connector.getResponses( "GET / HTTP/1.1\n"+ @@ -1101,9 +1122,9 @@ public class RequestTest "Connection: close\n"+ "\n" ); - assertThat(response, Matchers.containsString("200")); - assertThat(response, Matchers.containsString("Connection: close")); - assertThat(response, Matchers.containsString("Hello World")); + assertThat(response, containsString("200")); + assertThat(response, containsString("Connection: close")); + assertThat(response, containsString("Hello World")); response=_connector.getResponses( "GET / HTTP/1.1\n"+ @@ -1112,18 +1133,18 @@ public class RequestTest "\n" ); - assertThat(response, Matchers.containsString("200")); - assertThat(response, Matchers.containsString("Connection: close")); - assertThat(response, Matchers.containsString("Hello World")); + assertThat(response, containsString("200")); + assertThat(response, containsString("Connection: close")); + assertThat(response, containsString("Hello World")); response=_connector.getResponses( "GET / HTTP/1.0\n"+ "Host: whatever\n"+ "\n" ); - assertThat(response, Matchers.containsString("200")); - assertThat(response, Matchers.not(Matchers.containsString("Connection: close"))); - assertThat(response, Matchers.containsString("Hello World")); + assertThat(response, containsString("200")); + assertThat(response, Matchers.not(containsString("Connection: close"))); + assertThat(response, containsString("Hello World")); response=_connector.getResponses( "GET / HTTP/1.0\n"+ @@ -1131,8 +1152,8 @@ public class RequestTest "Connection: Other, close\n"+ "\n" ); - assertThat(response, Matchers.containsString("200")); - assertThat(response, Matchers.containsString("Hello World")); + assertThat(response, containsString("200")); + assertThat(response, containsString("Hello World")); response=_connector.getResponses( "GET / HTTP/1.0\n"+ @@ -1141,9 +1162,9 @@ public class RequestTest "\n", 200, TimeUnit.MILLISECONDS ); - assertThat(response, Matchers.containsString("200")); - assertThat(response, Matchers.containsString("Connection: keep-alive")); - assertThat(response, Matchers.containsString("Hello World")); + assertThat(response, containsString("200")); + assertThat(response, containsString("Connection: keep-alive")); + assertThat(response, containsString("Hello World")); _handler._checker = new RequestTester() { @@ -1163,9 +1184,9 @@ public class RequestTest "\n", 200, TimeUnit.MILLISECONDS ); - assertThat(response, Matchers.containsString("200")); - assertThat(response, Matchers.containsString("Connection: TE,Other")); - assertThat(response, Matchers.containsString("Hello World")); + assertThat(response, containsString("200")); + assertThat(response, containsString("Connection: TE,Other")); + assertThat(response, containsString("Hello World")); response=_connector.getResponses( "GET / HTTP/1.1\n"+ @@ -1173,9 +1194,9 @@ public class RequestTest "Connection: close\n"+ "\n" ); - assertThat(response, Matchers.containsString("200 OK")); - assertThat(response, Matchers.containsString("Connection: close")); - assertThat(response, Matchers.containsString("Hello World")); + assertThat(response, containsString("200 OK")); + assertThat(response, containsString("Connection: close")); + assertThat(response, containsString("Hello World")); } @Test @@ -1252,7 +1273,7 @@ public class RequestTest "\n" ); assertThat(response, Matchers.startsWith("HTTP/1.1 200 OK")); - assertThat(response.substring(15), Matchers.containsString("HTTP/1.1 200 OK")); + assertThat(response.substring(15), containsString("HTTP/1.1 200 OK")); assertEquals(4,cookies.size()); assertEquals("name", cookies.get(0).getName()); assertEquals("value", cookies.get(0).getValue()); @@ -1277,7 +1298,7 @@ public class RequestTest "\n" ); assertThat(response, Matchers.startsWith("HTTP/1.1 200 OK")); - assertThat(response.substring(15), Matchers.containsString("HTTP/1.1 200 OK")); + assertThat(response.substring(15), containsString("HTTP/1.1 200 OK")); assertEquals(4,cookies.size()); assertEquals("name", cookies.get(0).getName()); assertEquals("value", cookies.get(0).getValue()); @@ -1415,11 +1436,10 @@ public class RequestTest { try (StacklessLogging stackless = new StacklessLogging(HttpChannel.class)) { - LOG.info("Expecting maxFormKeys limit and Closing HttpParser exceptions..."); + // Expecting maxFormKeys limit and Closing HttpParser exceptions... _server.setAttribute("org.eclipse.jetty.server.Request.maxFormContentSize",-1); _server.setAttribute("org.eclipse.jetty.server.Request.maxFormKeys",1000); - StringBuilder buf = new StringBuilder(4000000); buf.append("a=b"); @@ -1427,7 +1447,7 @@ public class RequestTest File evil_keys = new File("/tmp/keys_mapping_to_zero_2m"); if (evil_keys.exists()) { - LOG.info("Using real evil keys!"); + // Using real evil keys! try (BufferedReader in = new BufferedReader(new FileReader(evil_keys))) { String key=null; @@ -1462,8 +1482,11 @@ public class RequestTest buf; long start=System.currentTimeMillis(); - String response = _connector.getResponses(request); - assertThat(response,Matchers.containsString("IllegalStateException")); + String rawResponse = _connector.getResponses(request); + HttpTester.Response response = HttpTester.parseResponse(rawResponse); + assertThat("Response.status", response.getStatus(), is(400)); + assertThat("Response body content", response.getContent(),containsString(BadMessageException.class.getName())); + assertThat("Response body content", response.getContent(),containsString(IllegalStateException.class.getName())); long now=System.currentTimeMillis(); assertTrue((now-start)<5000); } @@ -1503,8 +1526,11 @@ public class RequestTest buf; long start=System.currentTimeMillis(); - String response = _connector.getResponses(request); - assertTrue(response.contains("IllegalStateException")); + String rawResponse = _connector.getResponses(request); + HttpTester.Response response = HttpTester.parseResponse(rawResponse); + assertThat("Response.status", response.getStatus(), is(400)); + assertThat("Response body content", response.getContent(),containsString(BadMessageException.class.getName())); + assertThat("Response body content", response.getContent(),containsString(IllegalStateException.class.getName())); long now=System.currentTimeMillis(); assertTrue((now-start)<5000); } diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/UrlEncoded.java b/jetty-util/src/main/java/org/eclipse/jetty/util/UrlEncoded.java index fb7c4349320..3a9986e5bd1 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/UrlEncoded.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/UrlEncoded.java @@ -29,7 +29,6 @@ import java.nio.charset.StandardCharsets; import java.util.List; import java.util.Map; -import org.eclipse.jetty.util.Utf8Appendable.NotUtf8Exception; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; @@ -316,88 +315,53 @@ public class UrlEncoded extends MultiMap implements Cloneable for (int i=offset;i0) - { - map.add(value,""); - } - key = null; - value=null; - break; + case '&': + value = buffer.toReplacedString(); + buffer.reset(); + if (key != null) + { + map.add(key,value); + } + else if (value!=null&&value.length()>0) + { + map.add(value,""); + } + key = null; + value=null; + break; - case '=': - if (key!=null) - { - buffer.append(c); - break; - } - key = buffer.toReplacedString(); - buffer.reset(); - break; - - case '+': - buffer.append((byte)' '); - break; - - case '%': - if (i+2 implements Cloneable case '%': int code0=in.read(); - if ('u'==code0) - { - int code1=in.read(); - if (code1>=0) - { - int code2=in.read(); - if (code2>=0) - { - int code3=in.read(); - if (code3>=0) - buffer.append(Character.toChars((convertHexDigit(code0)<<12)+(convertHexDigit(code1)<<8)+(convertHexDigit(code2)<<4)+convertHexDigit(code3))); - } - } - } - else if (code0>=0) - { - int code1=in.read(); - if (code1>=0) - buffer.append((char)((convertHexDigit(code0)<<4)+convertHexDigit(code1))); - } + int code1=in.read(); + buffer.append(decodeHexChar(code0,code1)); break; default: @@ -537,99 +483,51 @@ public class UrlEncoded extends MultiMap implements Cloneable int totalLength=0; while ((b=in.read())>=0) { - try + switch ((char) b) { - switch ((char) b) - { - case '&': - value = buffer.toReplacedString(); - buffer.reset(); - if (key != null) - { - map.add(key,value); - } - else if (value!=null&&value.length()>0) - { - map.add(value,""); - } - key = null; - value=null; - if (maxKeys>0 && map.size()>maxKeys) - throw new IllegalStateException("Form too many keys"); - break; + case '&': + value = buffer.toReplacedString(); + buffer.reset(); + if (key != null) + { + map.add(key,value); + } + else if (value!=null&&value.length()>0) + { + map.add(value,""); + } + key = null; + value=null; + if (maxKeys>0 && map.size()>maxKeys) + throw new IllegalStateException("Form has too many keys"); + break; - case '=': - if (key!=null) - { - buffer.append((byte)b); - break; - } - key = buffer.toReplacedString(); - buffer.reset(); - break; - - case '+': - buffer.append((byte)' '); - break; - - case '%': - int code0=in.read(); - boolean decoded=false; - if ('u'==code0) - { - code0=in.read(); // XXX: we have to read the next byte, otherwise code0 is always 'u' - if (code0>=0) - { - int code1=in.read(); - if (code1>=0) - { - int code2=in.read(); - if (code2>=0) - { - int code3=in.read(); - if (code3>=0) - { - buffer.getStringBuilder().append(Character.toChars - ((convertHexDigit(code0)<<12)+(convertHexDigit(code1)<<8)+(convertHexDigit(code2)<<4)+convertHexDigit(code3))); - decoded=true; - } - } - } - } - } - else if (code0>=0) - { - int code1=in.read(); - if (code1>=0) - { - buffer.append((byte)((convertHexDigit(code0)<<4)+convertHexDigit(code1))); - decoded=true; - } - } - - if (!decoded) - buffer.getStringBuilder().append(Utf8Appendable.REPLACEMENT); - - break; - - default: + case '=': + if (key!=null) + { buffer.append((byte)b); break; - } - } - catch(NotUtf8Exception e) - { - LOG.warn(e.toString()); - LOG.debug(e); - } - catch(NumberFormatException e) - { - buffer.append(Utf8Appendable.REPLACEMENT_UTF8,0,3); - LOG.warn(e.toString()); - LOG.debug(e); + } + key = buffer.toReplacedString(); + buffer.reset(); + break; + + case '+': + buffer.append((byte)' '); + break; + + case '%': + char code0= (char) in.read(); + char code1= (char) in.read(); + buffer.append(decodeHexByte(code0,code1)); + break; + + default: + buffer.append((byte)b); + break; } if (maxLength>=0 && (++totalLength > maxLength)) - throw new IllegalStateException("Form too large"); + throw new IllegalStateException("Form is too large"); } if (key != null) @@ -751,7 +649,7 @@ public class UrlEncoded extends MultiMap implements Cloneable key = null; value=null; if (maxKeys>0 && map.size()>maxKeys) - throw new IllegalStateException("Form too many keys"); + throw new IllegalStateException("Form has too many keys"); break; case '=': if (key!=null) @@ -768,27 +666,8 @@ public class UrlEncoded extends MultiMap implements Cloneable break; case '%': int code0=in.read(); - if ('u'==code0) - { - int code1=in.read(); - if (code1>=0) - { - int code2=in.read(); - if (code2>=0) - { - int code3=in.read(); - if (code3>=0) - output.write(new String(Character.toChars((convertHexDigit(code0)<<12)+(convertHexDigit(code1)<<8)+(convertHexDigit(code2)<<4)+convertHexDigit(code3))).getBytes(charset)); - } - } - - } - else if (code0>=0) - { - int code1=in.read(); - if (code1>=0) - output.write((convertHexDigit(code0)<<4)+convertHexDigit(code1)); - } + int code1=in.read(); + output.write(decodeHexChar(code0,code1)); break; default: output.write(c); @@ -797,7 +676,7 @@ public class UrlEncoded extends MultiMap implements Cloneable totalLength++; if (maxLength>=0 && totalLength > maxLength) - throw new IllegalStateException("Form too large"); + throw new IllegalStateException("Form is too large"); } size=output.size(); @@ -874,42 +753,10 @@ public class UrlEncoded extends MultiMap implements Cloneable if ((i+2) implements Cloneable { if(i+2 implements Cloneable return buffer.toString(); } - + } + + private static char decodeHexChar(int hi, int lo) + { + try + { + return (char) ((convertHexDigit(hi) << 4) + convertHexDigit(lo)); + } + catch(NumberFormatException e) + { + throw new IllegalArgumentException("Not valid encoding '%" + (char) hi + (char) lo + "'"); + } + } + + private static byte decodeHexByte(char hi, char lo) + { + try + { + return (byte) ((convertHexDigit(hi) << 4) + convertHexDigit(lo)); + } + catch(NumberFormatException e) + { + throw new IllegalArgumentException("Not valid encoding '%" + hi + lo + "'"); + } } /* ------------------------------------------------------------ */ diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/URLEncodedTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/URLEncodedTest.java index 43a5c5b7cc9..6cdaffe4ae7 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/URLEncodedTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/URLEncodedTest.java @@ -18,7 +18,9 @@ package org.eclipse.jetty.util; +import static org.hamcrest.CoreMatchers.is; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import java.io.ByteArrayInputStream; @@ -26,17 +28,16 @@ import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; -/* ------------------------------------------------------------ */ -/** Util meta Tests. - * +/** + * URL Encoding / Decoding Tests */ public class URLEncodedTest { - - /* -------------------------------------------------------------- */ static { /* @@ -46,9 +47,10 @@ public class URLEncodedTest System.setProperty("org.eclipse.jetty.util.UrlEncoding.charset", StringUtil.__ISO_8859_1); */ } + + @Rule + public ExpectedException expectedException = ExpectedException.none(); - - /* -------------------------------------------------------------- */ @Test public void testUrlEncoded() { @@ -120,82 +122,8 @@ public class URLEncodedTest assertEquals("encoded param size",1, url_encoded.size()); assertEquals("encoded encode","Name8=xx%2C++yy++%2Czz", url_encoded.encode()); assertEquals("encoded get", url_encoded.getString("Name8"),"xx, yy ,zz"); - - url_encoded.clear(); - url_encoded.decode("Name11=%u30EDxxVerdi+%C6+og+2zz", StandardCharsets.ISO_8859_1); - assertEquals("encoded param size",1, url_encoded.size()); - assertEquals("encoded get", "?xxVerdi \u00c6 og 2zz",url_encoded.getString("Name11")); - - url_encoded.clear(); - url_encoded.decode("Name12=%u30EDxxVerdi+%2F+og+2zz", StandardCharsets.UTF_8); - assertEquals("encoded param size",1, url_encoded.size()); - assertEquals("encoded get", url_encoded.getString("Name12"),"\u30edxxVerdi / og 2zz"); - - url_encoded.clear(); - url_encoded.decode("Name14=%uXXXXa%GGb%+%c%+%d", StandardCharsets.ISO_8859_1); - assertEquals("encoded param size",1, url_encoded.size()); - assertEquals("encoded get","?a?b?c?d", url_encoded.getString("Name14")); - - url_encoded.clear(); - url_encoded.decode("Name14=%uXXXX%GG%+%%+%", StandardCharsets.UTF_8); - assertEquals("encoded param size",1, url_encoded.size()); - assertEquals("encoded get", "\ufffd\ufffd\ufffd\ufffd",url_encoded.getString("Name14")); - - - /* Not every jvm supports this encoding */ - - if (java.nio.charset.Charset.isSupported("SJIS")) - { - url_encoded.clear(); - url_encoded.decode("Name9=%u30ED%83e%83X%83g", Charset.forName("SJIS")); // "Test" in Japanese Katakana - assertEquals("encoded param size",1, url_encoded.size()); - assertEquals("encoded get", "\u30ed\u30c6\u30b9\u30c8", url_encoded.getString("Name9")); - } - else - assertTrue("Charset SJIS not supported by jvm", true); } - - /* -------------------------------------------------------------- */ - @Test - public void testBadEncoding() - { - UrlEncoded url_encoded = new UrlEncoded(); - url_encoded.decode("Name15=xx%zzyy", StandardCharsets.UTF_8); - assertEquals("encoded param size",1, url_encoded.size()); - assertEquals("encoded get", "xx\ufffdyy", url_encoded.getString("Name15")); - - String bad="Name=%FF%FF%FF"; - MultiMap map = new MultiMap(); - UrlEncoded.decodeUtf8To(bad,map); - assertEquals("encoded param size",1, map.size()); - assertEquals("encoded get", "\ufffd\ufffd\ufffd", map.getString("Name")); - - url_encoded.clear(); - url_encoded.decode("Name=%FF%FF%FF", StandardCharsets.UTF_8); - assertEquals("encoded param size",1, url_encoded.size()); - assertEquals("encoded get", "\ufffd\ufffd\ufffd", url_encoded.getString("Name")); - - url_encoded.clear(); - url_encoded.decode("Name=%EF%EF%EF", StandardCharsets.UTF_8); - assertEquals("encoded param size",1, url_encoded.size()); - assertEquals("encoded get", "\ufffd\ufffd", url_encoded.getString("Name")); - - assertEquals("x",UrlEncoded.decodeString("x",0,1,StandardCharsets.UTF_8)); - assertEquals("x\ufffd",UrlEncoded.decodeString("x%",0,2,StandardCharsets.UTF_8)); - assertEquals("x\ufffd",UrlEncoded.decodeString("x%2",0,3,StandardCharsets.UTF_8)); - assertEquals("x ",UrlEncoded.decodeString("x%20",0,4,StandardCharsets.UTF_8)); - - assertEquals("xxx",UrlEncoded.decodeString("xxx",0,3,StandardCharsets.UTF_8)); - assertEquals("xxx\ufffd",UrlEncoded.decodeString("xxx%",0,4,StandardCharsets.UTF_8)); - assertEquals("xxx\ufffd",UrlEncoded.decodeString("xxx%u",0,5,StandardCharsets.UTF_8)); - assertEquals("xxx\ufffd",UrlEncoded.decodeString("xxx%u1",0,6,StandardCharsets.UTF_8)); - assertEquals("xxx\ufffd",UrlEncoded.decodeString("xxx%u12",0,7,StandardCharsets.UTF_8)); - assertEquals("xxx\ufffd",UrlEncoded.decodeString("xxx%u123",0,8,StandardCharsets.UTF_8)); - assertEquals("xxx\u1234",UrlEncoded.decodeString("xxx%u1234",0,9,StandardCharsets.UTF_8)); - } - - /* -------------------------------------------------------------- */ @Test public void testUrlEncodedStream() @@ -209,6 +137,7 @@ public class URLEncodedTest {StringUtil.__UTF16,StringUtil.__UTF16,"%00%30"}, }; + // Note: "%30" -> decode -> "0" for (int i=0;i m = new MultiMap<>(); UrlEncoded.decodeTo(in, m, charsets[i][1]==null?null:Charset.forName(charsets[i][1]),-1,-1); assertEquals(charsets[i][1]+" stream length",4,m.size()); - assertEquals(charsets[i][1]+" stream name\\n","value 0",m.getString("name\n")); - assertEquals(charsets[i][1]+" stream name1","",m.getString("name1")); - assertEquals(charsets[i][1]+" stream name2","",m.getString("name2")); - assertEquals(charsets[i][1]+" stream n\u00e3me3","value 3",m.getString("n\u00e3me3")); + assertThat(charsets[i][1]+" stream name\\n",m.getString("name\n"),is("value 0")); + assertThat(charsets[i][1]+" stream name1",m.getString("name1"),is("")); + assertThat(charsets[i][1]+" stream name2",m.getString("name2"),is("")); + assertThat(charsets[i][1]+" stream n\u00e3me3",m.getString("n\u00e3me3"),is("value 3")); } @@ -270,21 +199,19 @@ public class URLEncodedTest String expected = new String(TypeUtil.fromHexString(hex),"utf-8"); Assert.assertEquals(expected,url_encoded.getString("text")); } - - /* -------------------------------------------------------------- */ + @Test - public void testNotUtf8() throws Exception + public void testUtf8_MultiByteCodePoint() { - String query="name=X%c0%afZ"; - - MultiMap map = new MultiMap<>(); - UrlEncoded.LOG.info("EXPECT 4 Not Valid UTF8 warnings..."); - UrlEncoded.decodeUtf8To(query,0,query.length(),map); - assertEquals("X"+Utf8Appendable.REPLACEMENT+Utf8Appendable.REPLACEMENT+"Z",map.getValue("name",0)); - - map.clear(); - - UrlEncoded.decodeUtf8To(new ByteArrayInputStream(query.getBytes(StandardCharsets.ISO_8859_1)),map,100,-1); - assertEquals("X"+Utf8Appendable.REPLACEMENT+Utf8Appendable.REPLACEMENT+"Z",map.getValue("name",0)); + String input = "text=test%C3%A4"; + UrlEncoded url_encoded = new UrlEncoded(); + url_encoded.decode(input); + + // http://www.ltg.ed.ac.uk/~richard/utf-8.cgi?input=00e4&mode=hex + // Should be "testä" + // "test" followed by a LATIN SMALL LETTER A WITH DIAERESIS + + String expected = "test\u00e4"; + assertThat(url_encoded.getString("text"),is(expected)); } } diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/UrlEncodedInvalidEncodingTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/UrlEncodedInvalidEncodingTest.java new file mode 100644 index 00000000000..2c9080fbfe4 --- /dev/null +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/UrlEncodedInvalidEncodingTest.java @@ -0,0 +1,87 @@ +// +// ======================================================================== +// Copyright (c) 1995-2017 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.util; + +import static java.nio.charset.StandardCharsets.UTF_8; + +import java.nio.charset.Charset; +import java.util.ArrayList; +import java.util.List; + +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +@RunWith(Parameterized.class) +public class UrlEncodedInvalidEncodingTest +{ + @Rule + public ExpectedException expectedException = ExpectedException.none(); + + @Parameterized.Parameters(name = "{1} | {0}") + public static List data() + { + ArrayList data = new ArrayList<>(); + + data.add(new Object[]{ "Name=xx%zzyy", UTF_8, IllegalArgumentException.class }); + data.add(new Object[]{ "Name=%FF%FF%FF", UTF_8, Utf8Appendable.NotUtf8Exception.class }); + data.add(new Object[]{ "Name=%EF%EF%EF", UTF_8, Utf8Appendable.NotUtf8Exception.class }); + data.add(new Object[]{ "Name=%E%F%F", UTF_8, IllegalArgumentException.class }); + data.add(new Object[]{ "Name=x%", UTF_8, Utf8Appendable.NotUtf8Exception.class }); + data.add(new Object[]{ "Name=x%2", UTF_8, Utf8Appendable.NotUtf8Exception.class }); + data.add(new Object[]{ "Name=xxx%", UTF_8, Utf8Appendable.NotUtf8Exception.class }); + data.add(new Object[]{ "name=X%c0%afZ", UTF_8, Utf8Appendable.NotUtf8Exception.class }); + return data; + } + + @Parameterized.Parameter(0) + public String inputString; + + @Parameterized.Parameter(1) + public Charset charset; + + @Parameterized.Parameter(2) + public Class expectedThrowable; + + @Test + public void testDecode() + { + UrlEncoded url_encoded = new UrlEncoded(); + expectedException.expect(expectedThrowable); + url_encoded.decode(inputString, charset); + } + + @Test + public void testDecodeUtf8ToMap() + { + MultiMap map = new MultiMap(); + expectedException.expect(expectedThrowable); + UrlEncoded.decodeUtf8To(inputString,map); + } + + @Test + public void testDecodeTo() + { + MultiMap map = new MultiMap(); + expectedException.expect(expectedThrowable); + UrlEncoded.decodeTo(inputString,map,charset); + } +} diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/UrlEncodedUtf8Test.java b/jetty-util/src/test/java/org/eclipse/jetty/util/UrlEncodedUtf8Test.java index 992fe0487e2..b60de683dd5 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/UrlEncodedUtf8Test.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/UrlEncodedUtf8Test.java @@ -80,45 +80,6 @@ public class UrlEncodedUtf8Test fromInputStream(test,bytes,name,value,false); } - @Test - public void testCorrectUnicode() throws Exception - { - String chars="a=%u0061"; - byte[] bytes= chars.getBytes(StandardCharsets.UTF_8); - String test=new String(bytes,StandardCharsets.UTF_8); - String name = "a"; - String value = "a"; - - fromString(test,test,name,value,false); - fromInputStream(test,bytes,name,value,false); - } - - @Test - public void testIncompleteUnicode() throws Exception - { - String chars="a=%u0"; - byte[] bytes= chars.getBytes(StandardCharsets.UTF_8); - String test=new String(bytes,StandardCharsets.UTF_8); - String name = "a"; - String value = ""+Utf8Appendable.REPLACEMENT; - - fromString(test,test,name,value,false); - fromInputStream(test,bytes,name,value,false); - } - - @Test - public void testIncompletePercent() throws Exception - { - String chars="a=%A"; - byte[] bytes= chars.getBytes(StandardCharsets.UTF_8); - String test=new String(bytes,StandardCharsets.UTF_8); - String name = "a"; - String value = ""+Utf8Appendable.REPLACEMENT; - - fromString(test,test,name,value,false); - fromInputStream(test,bytes,name,value,false); - } - static void fromString(String test,String s,String field,String expected,boolean thrown) throws Exception { MultiMap values=new MultiMap<>();