Skip to content

Commit

Permalink
Fixes #11230 Problem with parsing of form parameters without values. (#…
Browse files Browse the repository at this point in the history 
…11255)

Fixed parsing of form parameters in FormFields.parse().
Added more tests for valid edge cases, and invalid cases.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
  • Loading branch information
@sbordet
sbordet authored Jan 12, 2024
1 parent a9e564a commit c8cd6f4
Show file tree
Hide file tree
Showing 4 changed files with 158 additions and 71 deletions.
134 changes: 70 additions & 64 deletions jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/FormFields.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,9 @@
import static org.eclipse.jetty.util.UrlEncoded.decodeHexByte;

/**
* A {@link CompletableFuture} that is completed once a {@code application/x-www-form-urlencoded}
* content has been parsed asynchronously from the {@link Content.Source}.
* <p>A {@link CompletableFuture} that is completed once a {@code application/x-www-form-urlencoded}
* content has been parsed asynchronously from the {@link Content.Source}.</p>
* <p><a href="https://url.spec.whatwg.org/#application/x-www-form-urlencoded">Specification</a>.</p>
*/
public class FormFields extends ContentSourceCompletableFuture<Fields>
{
Expand Down Expand Up @@ -209,98 +210,103 @@ private FormFields(Content.Source source, Charset charset, int maxFields, int ma
@Override
protected Fields parse(Content.Chunk chunk) throws CharacterCodingException
{
String value = null;
ByteBuffer buffer = chunk.getByteBuffer();

do
while (BufferUtil.hasContent(buffer))
{
loop:
while (BufferUtil.hasContent(buffer))
byte b = buffer.get();
switch (_percent)
{
byte b = buffer.get();
switch (_percent)
case 1 ->
{
case 1 ->
{
_percentCode = b;
_percent++;
continue;
}
case 2 ->
{
_builder.append(decodeHexByte((char)_percentCode, (char)b));
_percent = 0;
continue;
}
_percentCode = b;
_percent++;
continue;
}
case 2 ->
{
_percent = 0;
_builder.append(decodeHexByte((char)_percentCode, (char)b));
continue;
}
}

if (_name == null)
if (_name == null)
{
switch (b)
{
switch (b)
case '&' ->
{
case '=' ->
{
_name = _builder.build();
checkLength(_name);
}
case '+' -> _builder.append((byte)' ');
case '%' -> _percent++;
default -> _builder.append(b);
String name = _builder.build();
checkMaxLength(name);
onNewField(name, "");
}
}
else
{
switch (b)
case '=' ->
{
case '&' ->
{
value = _builder.build();
checkLength(value);
break loop;
}
case '+' -> _builder.append((byte)' ');
case '%' -> _percent++;
default -> _builder.append(b);
_name = _builder.build();
checkMaxLength(_name);
}
case '+' -> _builder.append(' ');
case '%' -> _percent++;
default -> _builder.append(b);
}
}

if (_name != null)
else
{
if (value == null && chunk.isLast())
switch (b)
{
if (_percent > 0)
case '&' ->
{
_builder.append((byte)'%');
_builder.append(_percentCode);
String value = _builder.build();
checkMaxLength(value);
onNewField(_name, value);
_name = null;
}
value = _builder.build();
checkLength(value);
case '+' -> _builder.append(' ');
case '%' -> _percent++;
default -> _builder.append(b);
}
}
}

if (value != null)
{
Fields.Field field = new Fields.Field(_name, value);
_name = null;
value = null;
if (_maxFields >= 0 && _fields.getSize() >= _maxFields)
throw new IllegalStateException("form with too many fields > " + _maxFields);
_fields.add(field);
}
if (!chunk.isLast())
return null;

// Append any remaining %x.
if (_percent > 0)
throw new IllegalStateException("invalid percent encoding");
String value = _builder.build();

if (_name == null)
{
if (!value.isEmpty())
{
checkMaxLength(value);
onNewField(value, "");
}
return _fields;
}
while (BufferUtil.hasContent(buffer));

return chunk.isLast() ? _fields : null;
checkMaxLength(value);
onNewField(_name, value);
return _fields;
}

private void checkLength(String nameOrValue)
private void checkMaxLength(String nameOrValue)
{
if (_maxLength >= 0)
{
_length += nameOrValue.length();
if (_length > _maxLength)
throw new IllegalStateException("form too large");
throw new IllegalStateException("form too large > " + _maxLength);
}
}

private void onNewField(String name, String value)
{
Fields.Field field = new Fields.Field(name, value);
_fields.add(field);
if (_maxFields >= 0 && _fields.getSize() > _maxFields)
throw new IllegalStateException("form with too many fields > " + _maxFields);
}
}
68 changes: 61 additions & 7 deletions jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/FormFieldsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,13 @@

package org.eclipse.jetty.server;

import java.nio.charset.CharacterCodingException;
import java.nio.charset.Charset;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.stream.Stream;

Expand All @@ -32,31 +34,41 @@
import org.junit.jupiter.params.provider.MethodSource;

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.instanceOf;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;

public class FormFieldsTest
{
public static Stream<Arguments> tests()
public static Stream<Arguments> validData()
{
return Stream.of(
Arguments.of(List.of(""), UTF_8, -1, -1, Map.of()),
Arguments.of(List.of("name"), UTF_8, -1, -1, Map.of("name", "")),
Arguments.of(List.of("name&"), UTF_8, -1, -1, Map.of("name", "")),
Arguments.of(List.of("name="), UTF_8, -1, -1, Map.of("name", "")),
Arguments.of(List.of("name%00="), UTF_8, -1, -1, Map.of("name\u0000", "")),
Arguments.of(List.of("n1=v1&n2"), UTF_8, -1, -1, Map.of("n1", "v1", "n2", "")),
Arguments.of(List.of("n1=v1&n2&n3=v3&n4"), UTF_8, -1, -1, Map.of("n1", "v1", "n2", "", "n3", "v3", "n4", "")),
Arguments.of(List.of("name=value"), UTF_8, -1, -1, Map.of("name", "value")),
Arguments.of(List.of("name=%0A"), UTF_8, -1, -1, Map.of("name", "\n")),
Arguments.of(List.of("name=value", ""), UTF_8, -1, -1, Map.of("name", "value")),
Arguments.of(List.of("name", "=value", ""), UTF_8, -1, -1, Map.of("name", "value")),
Arguments.of(List.of("n", "ame", "=", "value"), UTF_8, -1, -1, Map.of("name", "value")),
Arguments.of(List.of("n=v&X=Y"), UTF_8, 2, 4, Map.of("n", "v", "X", "Y")),
Arguments.of(List.of("name=f¤¤&X=Y"), UTF_8, -1, -1, Map.of("name", "f¤¤", "X", "Y")),
Arguments.of(List.of("n=v&X=Y"), UTF_8, 1, -1, null),
Arguments.of(List.of("n=v&X=Y"), UTF_8, -1, 3, null)
Arguments.of(List.of("na+me=", "va", "+", "lue"), UTF_8, -1, -1, Map.of("na me", "va lue")),
Arguments.of(List.of("=v"), UTF_8, -1, -1, Map.of("", "v"))
);
}

@ParameterizedTest
@MethodSource("tests")
public void testFormFields(List<String> chunks, Charset charset, int maxFields, int maxLength, Map<String, String> expected)
throws Exception
@MethodSource("validData")
public void testValidFormFields(List<String> chunks, Charset charset, int maxFields, int maxLength, Map<String, String> expected)
{
AsyncContent source = new AsyncContent();
Attributes attributes = new Attributes.Mapped();
Expand All @@ -66,8 +78,9 @@ public void testFormFields(List<String> chunks, Charset charset, int maxFields,
int last = chunks.size() - 1;
FutureCallback eof = new FutureCallback();
for (int i = 0; i <= last; i++)
{
source.write(i == last, BufferUtil.toBuffer(chunks.get(i), charset), i == last ? eof : Callback.NOOP);

}

try
{
Expand All @@ -89,4 +102,45 @@ public void testFormFields(List<String> chunks, Charset charset, int maxFields,
assertNull(expected);
}
}

public static Stream<Arguments> invalidData()
{
return Stream.of(
Arguments.of(List.of("%A"), UTF_8, -1, -1, IllegalStateException.class),
Arguments.of(List.of("name%"), UTF_8, -1, -1, IllegalStateException.class),
Arguments.of(List.of("name%A"), UTF_8, -1, -1, IllegalStateException.class),

// TODO: these 2 should throw the same exception.
Arguments.of(List.of("name%A="), UTF_8, -1, -1, CharacterCodingException.class),
Arguments.of(List.of("name%A&"), UTF_8, -1, -1, IllegalArgumentException.class),

Arguments.of(List.of("name=%"), UTF_8, -1, -1, IllegalStateException.class),
Arguments.of(List.of("name=A%%A"), UTF_8, -1, -1, IllegalArgumentException.class),
Arguments.of(List.of("name=A%%3D"), UTF_8, -1, -1, IllegalArgumentException.class),
Arguments.of(List.of("%="), UTF_8, -1, -1, IllegalStateException.class),
Arguments.of(List.of("name=%A"), UTF_8, -1, -1, IllegalStateException.class),
Arguments.of(List.of("name=value%A"), UTF_8, -1, -1, IllegalStateException.class),
Arguments.of(List.of("n=v&X=Y"), UTF_8, 1, -1, IllegalStateException.class),
Arguments.of(List.of("n=v&X=Y"), UTF_8, -1, 3, IllegalStateException.class),
Arguments.of(List.of("n%AH=v"), UTF_8, -1, -1, IllegalArgumentException.class),
Arguments.of(List.of("n=v%AH"), UTF_8, -1, -1, IllegalArgumentException.class),
Arguments.of(List.of("n=v%FF"), UTF_8, -1, -1, CharacterCodingException.class)
);
}

@ParameterizedTest
@MethodSource("invalidData")
public void testInvalidFormFields(List<String> chunks, Charset charset, int maxFields, int maxLength, Class<? extends Exception> expectedException)
{
AsyncContent source = new AsyncContent();
CompletableFuture<Fields> futureFields = FormFields.from(source, new Attributes.Mapped(), charset, maxFields, maxLength);
assertFalse(futureFields.isDone());
int last = chunks.size() - 1;
for (int i = 0; i <= last; i++)
{
source.write(i == last, BufferUtil.toBuffer(chunks.get(i)), Callback.NOOP);
}
Throwable cause = assertThrows(ExecutionException.class, futureFields::get).getCause();
assertThat(cause, instanceOf(expectedException));
}
}
26 changes: 26 additions & 0 deletions jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/CharsetStringBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,14 @@ default void append(ByteBuffer buf)
*/
String build() throws CharacterCodingException;

/**
* @return the length in characters
*/
int length();

/**
* <p>Resets this sequence to be empty.</p>
*/
void reset();

/**
Expand Down Expand Up @@ -168,6 +176,12 @@ public String build()
return s;
}

@Override
public int length()
{
return _builder.length();
}

@Override
public void reset()
{
Expand Down Expand Up @@ -207,6 +221,12 @@ public String build()
return s;
}

@Override
public int length()
{
return _builder.length();
}

@Override
public void reset()
{
Expand Down Expand Up @@ -319,6 +339,12 @@ public String build() throws CharacterCodingException
}
}

@Override
public int length()
{
return _stringBuilder.length();
}

@Override
public void reset()
{
Expand Down
1 change: 1 addition & 0 deletions jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Utf8StringBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ protected Utf8StringBuilder(StringBuilder buffer)
_buffer = buffer;
}

@Override
public int length()
{
return _buffer.length();
Expand Down

0 comments on commit c8cd6f4

Please sign in to comment.