Skip to content

Commit

Permalink
Issue parameter decoding (#1330)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
joakime authored and gregw committed Mar 30, 2017
1 parent 9496cef commit dff8fb6
Show file tree
Hide file tree
Showing 8 changed files with 350 additions and 513 deletions.
Expand Up @@ -45,6 +45,11 @@ public BadMessageException(String reason)
this(400,reason);
}

public BadMessageException(String reason, Throwable cause)
{
this(400, reason, cause);
}

public BadMessageException(int code, String reason)
{
super(code+": "+reason);
Expand Down
36 changes: 4 additions & 32 deletions jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java
Expand Up @@ -21,16 +21,15 @@

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
Expand Down Expand Up @@ -100,33 +99,6 @@ public void testParseRequestTarget()
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<String> 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
Expand Down
22 changes: 20 additions & 2 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
Expand Up @@ -362,13 +362,31 @@ private MultiMap<String> getParameters()
// 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)
Expand Down

0 comments on commit dff8fb6

Please sign in to comment.