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.

Sign up for GitHub

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

Early Rejection of Large Requests Based on Content-Length Header #6032

Merged
merged 4 commits into from
Jan 9, 2025
Merged

Early Rejection of Large Requests Based on Content-Length Header #6032

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
990f4cb
feat: early reject large request
yzfeng2020 Dec 20, 2024
7167784
lint
yzfeng2020 Jan 2, 2025
3e9a3b5
Update core/src/main/java/com/linecorp/armeria/common/ContentTooLarge…
ikhoon Jan 8, 2025
3636dff
Update core/src/main/java/com/linecorp/armeria/common/ContentTooLarge…
ikhoon Jan 8, 2025
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
22 changes: 18 additions & 4 deletions core/src/main/java/com/linecorp/armeria/common/ContentTooLargeException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2016 LINE Corporation
* Copyright 2024 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
Expand Down Expand Up @@ -50,6 +50,7 @@ public static ContentTooLargeExceptionBuilder builder() {
private final long maxContentLength;
private final long contentLength;
private final long transferred;
private final boolean earlyRejection;

private ContentTooLargeException() {
this(false);
Expand All @@ -62,16 +63,18 @@ private ContentTooLargeException(boolean neverSample) {
maxContentLength = -1;
transferred = -1;
contentLength = -1;
earlyRejection = false;
}

ContentTooLargeException(long maxContentLength, long contentLength, long transferred,
@Nullable Throwable cause) {
super(toString(maxContentLength, contentLength, transferred), cause);
boolean earlyRejection, @Nullable Throwable cause) {
super(toString(maxContentLength, contentLength, transferred, earlyRejection), cause);

neverSample = false;
this.transferred = transferred;
this.contentLength = contentLength;
this.maxContentLength = maxContentLength;
this.earlyRejection = earlyRejection;
}

/**
Expand All @@ -96,6 +99,13 @@ public long maxContentLength() {
return maxContentLength;
}

/**
* Returns whether the exception is raised when reading content-length header.
*/
public boolean earlyRejection() {
return earlyRejection;
}

@Override
public Throwable fillInStackTrace() {
if (!neverSample && Flags.verboseExceptionSampler().isSampled(getClass())) {
Expand All @@ -105,7 +115,8 @@ public Throwable fillInStackTrace() {
}

@Nullable
private static String toString(long maxContentLength, long contentLength, long transferred) {
private static String toString(long maxContentLength, long contentLength, long transferred,
boolean earlyRejection) {
try (TemporaryThreadLocals ttl = TemporaryThreadLocals.acquire()) {
final StringBuilder buf = ttl.stringBuilder();
if (maxContentLength >= 0) {
Expand All @@ -117,6 +128,9 @@ private static String toString(long maxContentLength, long contentLength, long t
if (transferred >= 0) {
buf.append(", transferred: ").append(transferred);
}
if (earlyRejection) {
buf.append(", earlyRejection: ").append("true");
}
return buf.length() != 0 ? buf.substring(2) : null;
}
}
Expand Down
19 changes: 16 additions & 3 deletions core/src/main/java/com/linecorp/armeria/common/ContentTooLargeExceptionBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2021 LINE Corporation
* Copyright 2024 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
Expand Down Expand Up @@ -30,6 +30,8 @@ public final class ContentTooLargeExceptionBuilder {
private long maxContentLength = -1;
private long contentLength = -1;
private long transferred = -1;
private boolean earlyRejection;

@Nullable
private Throwable cause;

Expand Down Expand Up @@ -83,13 +85,24 @@ public ContentTooLargeExceptionBuilder cause(Throwable cause) {
return this;
}

/**
* Sets the exception as early rejection.
*/
@UnstableApi
public ContentTooLargeExceptionBuilder earlyRejection(boolean isEarlyRejection) {
yzfeng2020 marked this conversation as resolved.
Show resolved Hide resolved
this.earlyRejection = isEarlyRejection;
return this;
}

/**
* Returns a new instance of {@link ContentTooLargeException}.
*/
public ContentTooLargeException build() {
if (maxContentLength < 0 && contentLength < 0 && transferred < 0 && cause == null) {
if (maxContentLength < 0 && contentLength < 0 &&
transferred < 0 && !earlyRejection && cause == null) {
ikhoon marked this conversation as resolved.
Show resolved Hide resolved
return ContentTooLargeException.get();
}
return new ContentTooLargeException(maxContentLength, contentLength, transferred, cause);
return new ContentTooLargeException(maxContentLength, contentLength,
transferred, earlyRejection, cause);
ikhoon marked this conversation as resolved.
Show resolved Hide resolved
}
}
12 changes: 11 additions & 1 deletion core/src/main/java/com/linecorp/armeria/server/DecodedHttpRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2021 LINE Corporation
* Copyright 2024 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
Expand Down Expand Up @@ -143,6 +143,16 @@ default CompletableFuture<Void> whenAggregated() {
*/
long requestStartTimeMicros();

/**
* Returns the maximum allowed length of the content of the request.
*/
long maxRequestLength();

/**
* Returns the transferred bytes of the request.
*/
long transferredBytes();

/**
* Returns whether the request is an HTTP/1.1 webSocket request.
*/
Expand Down
7 changes: 1 addition & 6 deletions core/src/main/java/com/linecorp/armeria/server/DecodedHttpRequestWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2021 LINE Corporation
* Copyright 2024 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
Expand All @@ -19,10 +19,5 @@
import com.linecorp.armeria.common.HttpRequestWriter;

interface DecodedHttpRequestWriter extends DecodedHttpRequest, HttpRequestWriter {

long maxRequestLength();

long transferredBytes();

void increaseTransferredBytes(long delta);
}
12 changes: 11 additions & 1 deletion core/src/main/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2021 LINE Corporation
* Copyright 2024 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
Expand Down Expand Up @@ -260,4 +260,14 @@ public long requestStartTimeNanos() {
public long requestStartTimeMicros() {
return requestStartTimeMicros;
}

@Override
public long maxRequestLength() {
return 0;
}

@Override
public long transferredBytes() {
return 0;
}
}
69 changes: 40 additions & 29 deletions core/src/main/java/com/linecorp/armeria/server/Http1RequestDecoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2016 LINE Corporation
* Copyright 2024 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
Expand Down Expand Up @@ -208,8 +208,8 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
// Validate the 'content-length' header.
final String contentLengthStr = headers.get(HttpHeaderNames.CONTENT_LENGTH);
final boolean contentEmpty;
long contentLength = 0;
if (contentLengthStr != null) {
long contentLength;
try {
contentLength = Long.parseLong(contentLengthStr);
} catch (NumberFormatException ignored) {
Expand Down Expand Up @@ -280,6 +280,10 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
final boolean endOfStream = contentEmpty && !transferEncodingChunked;
this.req = req = DecodedHttpRequest.of(endOfStream, eventLoop, id, 1, headers,
keepAlive, inboundTrafficController, routingCtx);
final long maxRequestLength = req.maxRequestLength();
if (maxRequestLength > 0 && contentLength > maxRequestLength) {
abortLargeRequest(ctx, req, id, endOfStream, keepAliveHandler, true);
}
Comment on lines +284 to +286
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unsure if it is necessary to pass the content-length exceeding request to the service chains. Should we fail() the request instead? What do you think @minwoox?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, it was implemented this way to meet a demand for logging content-length-exceeding requests via LoggingService.

This feature could still be useful for identifying such requests on the server side, provided it doesn't cause a performance issue. 🤔

Copy link
Contributor

@ikhoon ikhoon Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't worried about performance, but I was considering the race condition between the response sent by the user and the aborted response.

That case would be rare, so I think it can be ignored.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let's revisit this if we encounter any issue with this. 😉

cfg.serverMetrics().increasePendingHttp1Requests();
yzfeng2020 marked this conversation as resolved.
Show resolved Hide resolved
ctx.fireChannelRead(req);
} else {
Expand Down Expand Up @@ -313,33 +317,7 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
final long maxContentLength = decodedReq.maxRequestLength();
final long transferredLength = decodedReq.transferredBytes();
if (maxContentLength > 0 && transferredLength > maxContentLength) {
final ContentTooLargeException cause =
ContentTooLargeException.builder()
.maxContentLength(maxContentLength)
.contentLength(req.headers())
.transferred(transferredLength)
.build();
discarding = true;
req = null;
final boolean shouldReset;
if (encoder instanceof ServerHttp1ObjectEncoder) {
if (encoder.isResponseHeadersSent(id, 1)) {
ctx.channel().close();
} else {
keepAliveHandler.disconnectWhenFinished();
}
shouldReset = false;
} else {
// Upgraded to HTTP/2. Reset only if the remote peer is still open.
shouldReset = !endOfStream;
}

// Wrap the cause with the returned status to let LoggingService correctly log the
// status.
final HttpStatusException httpStatusException =
HttpStatusException.of(HttpStatus.REQUEST_ENTITY_TOO_LARGE, cause);
decodedReq.setShouldResetOnlyIfRemoteIsOpen(shouldReset);
decodedReq.abortResponse(httpStatusException, true);
abortLargeRequest(ctx, decodedReq, id, endOfStream, keepAliveHandler, false);
return;
}

Expand Down Expand Up @@ -377,6 +355,39 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
}
}

private void abortLargeRequest(ChannelHandlerContext ctx, DecodedHttpRequest decodedReq, int id,
boolean endOfStream, KeepAliveHandler keepAliveHandler,
boolean isEarlyRejection) {
final ContentTooLargeException cause =
ContentTooLargeException.builder()
.maxContentLength(decodedReq.maxRequestLength())
.transferred(decodedReq.transferredBytes())
.contentLength(decodedReq.headers())
.earlyRejection(isEarlyRejection)
.build();
discarding = true;
req = null;
final boolean shouldReset;
if (encoder instanceof ServerHttp1ObjectEncoder) {
if (encoder.isResponseHeadersSent(id, 1)) {
ctx.channel().close();
} else {
keepAliveHandler.disconnectWhenFinished();
}
shouldReset = false;
} else {
// Upgraded to HTTP/2. Reset only if the remote peer is still open.
shouldReset = !endOfStream;
}

// Wrap the cause with the returned status to let LoggingService correctly log the
// status.
final HttpStatusException httpStatusException =
HttpStatusException.of(HttpStatus.REQUEST_ENTITY_TOO_LARGE, cause);
decodedReq.setShouldResetOnlyIfRemoteIsOpen(shouldReset);
decodedReq.abortResponse(httpStatusException, true);
}

private void removeFromPipelineIfUpgraded(ChannelHandlerContext ctx, boolean endOfStream) {
if (endOfStream && encoder instanceof ServerHttp2ObjectEncoder) {
// An HTTP/1 connection has been upgraded to HTTP/2.
Expand Down
42 changes: 26 additions & 16 deletions core/src/main/java/com/linecorp/armeria/server/Http2RequestDecoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2016 LINE Corporation
* Copyright 2024 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
Expand Down Expand Up @@ -165,8 +165,8 @@ public void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers

// Validate the 'content-length' header if exists.
final String contentLengthStr = headers.get(HttpHeaderNames.CONTENT_LENGTH);
long contentLength = 0;
if (contentLengthStr != null) {
long contentLength;
try {
contentLength = Long.parseLong(contentLengthStr);
} catch (NumberFormatException ignored) {
Expand Down Expand Up @@ -206,6 +206,10 @@ public void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers
final EventLoop eventLoop = ctx.channel().eventLoop();
req = DecodedHttpRequest.of(endOfStream, eventLoop, id, streamId, headers, true,
inboundTrafficController, routingCtx);
final long maxRequestLength = req.maxRequestLength();
if (maxRequestLength > 0 && contentLength > maxRequestLength) {
abortLargeRequest(req, endOfStream, true);
}
requests.put(streamId, req);
cfg.serverMetrics().increasePendingHttp2Requests();
ctx.fireChannelRead(req);
Expand Down Expand Up @@ -321,20 +325,7 @@ public int onDataRead(
final long maxContentLength = decodedReq.maxRequestLength();
final long transferredLength = decodedReq.transferredBytes();
if (maxContentLength > 0 && transferredLength > maxContentLength) {
assert encoder != null;
final ContentTooLargeException cause =
ContentTooLargeException.builder()
.maxContentLength(maxContentLength)
.contentLength(decodedReq.headers())
.transferred(transferredLength)
.build();

final boolean shouldReset = !endOfStream;

final HttpStatusException httpStatusException =
HttpStatusException.of(HttpStatus.REQUEST_ENTITY_TOO_LARGE, cause);
decodedReq.setShouldResetOnlyIfRemoteIsOpen(shouldReset);
decodedReq.abortResponse(httpStatusException, true);
abortLargeRequest(decodedReq, endOfStream, false);
} else if (decodedReq.isOpen()) {
try {
// The decodedReq will be automatically closed if endOfStream is true.
Expand All @@ -350,6 +341,25 @@ public int onDataRead(
return dataLength + padding;
}

private void abortLargeRequest(DecodedHttpRequest decodedReq, boolean endOfStream,
boolean isEarlyRejection) {
assert encoder != null;
final ContentTooLargeException cause =
ContentTooLargeException.builder()
.maxContentLength(decodedReq.maxRequestLength())
.contentLength(decodedReq.headers())
.transferred(decodedReq.transferredBytes())
.earlyRejection(isEarlyRejection)
.build();

final boolean shouldReset = !endOfStream;

final HttpStatusException httpStatusException =
HttpStatusException.of(HttpStatus.REQUEST_ENTITY_TOO_LARGE, cause);
decodedReq.setShouldResetOnlyIfRemoteIsOpen(shouldReset);
decodedReq.abortResponse(httpStatusException, true);
}

private void writeInvalidRequestPathResponse(int streamId, @Nullable RequestHeaders headers) {
writeErrorResponse(streamId, headers, HttpStatus.BAD_REQUEST,
"Invalid request path", null);
Expand Down
Loading
Loading