Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #11387: Reintroduce MultiPartCompliance.LEGACY in ee9/ee8 #11388

Merged
merged 29 commits into from
Feb 27, 2024
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
e125ada
Issue #11387: Reintroduce MultiPartCompliance.LEGACY in ee9/ee8
joakime Feb 7, 2024
e7a92ca
Correcting javadoc
joakime Feb 7, 2024
9120cfd
Updating MultiPartCaptureTest to ...
joakime Feb 7, 2024
9a7371f
Fixing checkstyle warning
joakime Feb 7, 2024
542f3ff
Re-enable Part-ContainsContents expectations
joakime Feb 8, 2024
744e382
Rename MultiPartCompliance.NO_CRLF_AFTER_PREAMBLE to WHITESPACE_AFTER…
joakime Feb 8, 2024
a2f2583
Make ee9/ee8 legacy parser use legacy tokenization
joakime Feb 12, 2024
f82683a
Testing ee9/ee8 legacy parser base64 auto-decoding behaviors
joakime Feb 12, 2024
c018cae
Cleanup jetty-test-multipart class naming
joakime Feb 12, 2024
56652f1
Adding ee10 tests against raw multipart examples
joakime Feb 12, 2024
ebd109d
Adding shorter whitespace multipart test
joakime Feb 12, 2024
eba54dd
Adding jetty-core version of failing ee10 tests
joakime Feb 12, 2024
367efd8
Fixing checkstyle
joakime Feb 13, 2024
9321e5e
Fixed missed notification for CR content in case of 1 chunk ending wi…
sbordet Feb 13, 2024
6e80cb7
Update test case, as there is now an extra notifyCRContent event
joakime Feb 13, 2024
18a0acd
Update test case, as there is now an extra notifyCRContent event
joakime Feb 13, 2024
edabdb5
Adding MultiPartCompliance.Violation events
joakime Feb 14, 2024
d56838e
Rename WHITESPACE_AFTER_PREAMBLE to WHITESPACE_BEFORE_BOUNDARY
joakime Feb 14, 2024
65bba39
Notifying WHITESPACE_BEFORE_BOUNDARY
joakime Feb 14, 2024
d464627
some simple cleanup of new ee9 code
joakime Feb 15, 2024
b9c1860
Fix comment
joakime Feb 15, 2024
5cdef49
Cleaning up @Disabled tests
joakime Feb 15, 2024
3f84156
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/12.0.x/mu…
joakime Feb 21, 2024
f21752f
Cleanup from merge
joakime Feb 21, 2024
30c7c35
Changes from review
joakime Feb 26, 2024
c09e017
Changes from review
joakime Feb 26, 2024
b2e8dee
Changes from review
joakime Feb 26, 2024
cc745c7
Cosmetics.
sbordet Feb 27, 2024
460b83e
More cosmetics.
sbordet Feb 27, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion jetty-core/jetty-http/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,17 @@
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>

<dependency>
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-slf4j-impl</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.eclipse.jetty.tests</groupId>
<artifactId>jetty-test-multipart</artifactId>
<version>${project.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.eclipse.jetty.toolchain</groupId>
<artifactId>jetty-test-helper</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ public enum HttpHeader
CONTENT_LOCATION("Content-Location"),
CONTENT_MD5("Content-MD5"),
CONTENT_RANGE("Content-Range"),
CONTENT_TRANSFER_ENCODING("Content-Transfer-Encoding"),
CONTENT_TYPE("Content-Type"),
EXPIRES("Expires"),
LAST_MODIFIED("Last-Modified"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.nio.file.StandardCopyOption;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
Expand Down Expand Up @@ -908,6 +909,7 @@ public static class Parser
private final Utf8StringBuilder text = new Utf8StringBuilder();
private final String boundary;
private final SearchPattern boundaryFinder;
private final MultiPartCompliance compliance;
private final Listener listener;
private int partHeadersLength;
private int partHeadersMaxLength = -1;
Expand All @@ -920,13 +922,33 @@ public static class Parser
private String fieldValue;
private long maxParts = 1000;
private int numParts;
private EnumSet<MultiPartCompliance.Violation> eols;

public Parser(String boundary, Listener listener)
{
this(boundary, MultiPartCompliance.RFC7578, listener);
}

public Parser(String boundary, MultiPartCompliance compliance, Listener listener)
{
this.boundary = boundary;
// While the spec mandates CRLF before the boundary, be more lenient and only require LF.
this.boundaryFinder = SearchPattern.compile("\n--" + boundary);
this.compliance = compliance;
this.listener = listener;

if (LOG.isDebugEnabled())
{
List.of(
MultiPartCompliance.Violation.CR_LINE_TERMINATION,
MultiPartCompliance.Violation.BASE64_TRANSFER_ENCODING,
MultiPartCompliance.Violation.WHITESPACE_BEFORE_BOUNDARY
).forEach(viol ->
{
if (compliance.allows(viol))
LOG.debug("{} ignoring compliance {}: unable to allow() it.", this.getClass().getName(), viol.name());
});
}
reset();
}

Expand Down Expand Up @@ -1050,6 +1072,7 @@ else if (type != HttpTokens.Type.SPACE && type != HttpTokens.Type.HTAB)
HttpTokens.Token token = next(buffer);
if (token.getByte() != '-')
throw new BadMessageException("bad last boundary");
notifyEolViolations();
state = State.EPILOGUE;
}
case HEADER_START ->
Expand Down Expand Up @@ -1099,6 +1122,7 @@ else if (type != HttpTokens.Type.SPACE && type != HttpTokens.Type.HTAB)
if (LOG.isDebugEnabled())
LOG.debug("parse failure {} {}", state, BufferUtil.toDetailString(buffer), x);
buffer.position(buffer.limit());
notifyEolViolations();
notifyFailure(x);
}
}
Expand All @@ -1123,18 +1147,35 @@ private HttpTokens.Token next(ByteBuffer buffer)
}
case LF ->
{
if (!crFlag)
{
sbordet marked this conversation as resolved.
Show resolved Hide resolved
MultiPartCompliance.Violation violation = MultiPartCompliance.Violation.LF_LINE_TERMINATION;
addEolViolation(violation);
if (!compliance.allows(violation))
throw new BadMessageException("invalid LF-only EOL");
}
crFlag = false;
}
case CR ->
{
if (crFlag)
throw new BadMessageException("invalid EOL");
{
MultiPartCompliance.Violation violation = MultiPartCompliance.Violation.CR_LINE_TERMINATION;
addEolViolation(violation);
if (!compliance.allows(violation))
throw new BadMessageException("invalid CR-only EOL");
}
crFlag = true;
}
default ->
{
if (crFlag)
throw new BadMessageException("invalid EOL");
{
MultiPartCompliance.Violation violation = MultiPartCompliance.Violation.CR_LINE_TERMINATION;
addEolViolation(violation);
if (!compliance.allows(violation))
throw new BadMessageException("invalid CR-only EOL");
}
}
}
return t;
Expand Down Expand Up @@ -1331,6 +1372,13 @@ private boolean parseContent(Content.Chunk chunk)
{
// The boundary was fully matched, so the part is complete.
buffer.position(buffer.position() + boundaryMatch - partialBoundaryMatch);
if (!crContent)
{
sbordet marked this conversation as resolved.
Show resolved Hide resolved
MultiPartCompliance.Violation violation = MultiPartCompliance.Violation.LF_LINE_TERMINATION;
addEolViolation(violation);
if (!compliance.allows(violation))
throw new BadMessageException("invalid LF-only EOL");
}
partialBoundaryMatch = 0;
crContent = false;
notifyPartContent(Content.Chunk.EOF);
Expand Down Expand Up @@ -1373,7 +1421,16 @@ private boolean parseContent(Content.Chunk chunk)
if (boundaryOffset >= 0)
{
if (boundaryOffset == 0)
{
if (!crContent)
{
MultiPartCompliance.Violation violation = MultiPartCompliance.Violation.LF_LINE_TERMINATION;
addEolViolation(violation);
if (!compliance.allows(violation))
throw new BadMessageException("invalid LF-only EOL");
}
crContent = false;
}

// Emit as content the last CR byte of the previous chunk, if any.
notifyCRContent();
Expand All @@ -1384,7 +1441,16 @@ private boolean parseContent(Content.Chunk chunk)
// BoundaryFinder is configured to search for '\n--Boundary';
// if '\r\n--Boundary' is found, then the '\r' is not content.
if (length > 0 && buffer.get(position + length - 1) == '\r')
{
--length;
}
else
{
MultiPartCompliance.Violation violation = MultiPartCompliance.Violation.LF_LINE_TERMINATION;
addEolViolation(violation);
if (!compliance.allows(violation))
throw new BadMessageException("invalid LF-only EOL");
}
Content.Chunk content = asSlice(chunk, position, length, true);
buffer.position(position + boundaryOffset + boundaryFinder.getLength());
notifyPartContent(content);
Expand Down Expand Up @@ -1536,6 +1602,39 @@ private void notifyFailure(Throwable failure)
}
}

private void notifyEolViolations()
{
if (eols != null)
{
for (MultiPartCompliance.Violation violation: eols)
{
notifyViolation(violation);
}
eols = null;
}
}

private void addEolViolation(MultiPartCompliance.Violation violation)
{
if (eols == null)
eols = EnumSet.of(violation);
else
eols.add(violation);
}

private void notifyViolation(MultiPartCompliance.Violation violation)
{
try
{
listener.onViolation(violation);
}
catch (Throwable x)
{
if (LOG.isDebugEnabled())
LOG.debug("failure while notifying listener {}", listener, x);
}
}

/**
* <p>A listener for events emitted by a {@link Parser}.</p>
*/
Expand Down Expand Up @@ -1598,6 +1697,16 @@ default void onComplete()
default void onFailure(Throwable failure)
{
}

/**
* <p>Callback method invoked when the low level parser encounters
* a fundamental multipart violation</p>>
*
* @param violation the violation detected
*/
default void onViolation(MultiPartCompliance.Violation violation)
{
}
}

private enum State
Expand Down
Loading
Loading