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 
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 committed Jan 8, 2024
1 parent 581f9ae commit cd24aa9
Show file tree
Hide file tree
Showing 4 changed files with 155 additions and 67 deletions.
137 changes: 77 additions & 60 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 @@ -209,98 +209,115 @@ 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 ->
_percentCode = b;
_percent++;
continue;
}
case 2 ->
{
_percent = 0;
// If there is a "terminator" character, append the previous %x.
if (b == '&' || b == '=')
{
_builder.append(decodeHexByte((char)_percentCode, (char)b));
_percent = 0;
continue;
_builder.append('%');
_builder.append(_percentCode);
break;
}
_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();
if (_name.isEmpty())
throw new IllegalStateException("form with invalid parameter name");
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)
{
_builder.append('%');
_builder.append(_percentCode);
}
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);
}
}
58 changes: 51 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,44 @@
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("%A"), UTF_8, -1, -1, Map.of("%A", "")),
Arguments.of(List.of("name%A"), UTF_8, -1, -1, Map.of("name%A", "")),
Arguments.of(List.of("name%A="), UTF_8, -1, -1, Map.of("name%A", "")),
Arguments.of(List.of("name%A&"), UTF_8, -1, -1, Map.of("name%A", "")),
Arguments.of(List.of("name=%A"), UTF_8, -1, -1, Map.of("name", "%A")),
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=value%A"), UTF_8, -1, -1, Map.of("name", "value%A")),
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"))
);
}

@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 +81,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 +105,32 @@ public void testFormFields(List<String> chunks, Charset charset, int maxFields,
assertNull(expected);
}
}

public static Stream<Arguments> invalidData()
{
return Stream.of(
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("=v"), UTF_8, -1, -1, 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 cd24aa9

Please sign in to comment.