Sending Request Entity Too Large status in preference to TooLongFrameException #1019

Closed
coltnz opened this Issue Feb 7, 2013 · 5 comments

Comments

Projects
None yet
3 participants
@coltnz
Contributor

coltnz commented Feb 7, 2013

Hi, I would like to implements the ToDos for oversized messages in HttpObjectAggregator.java and HttpObjectDecoder since I can't currently give my users proper error statuses. I need some guidance though:

Basic implementation would be:

    public void handleOversizedRequest(ChannelHandlerContext ctx, HttpMessage currentMessage) {
        if (currentMessage instanceof HttpRequest) {
            HttpResponse resp =
                    new DefaultFullHttpResponse(currentMessage.getProtocolVersion(),
                            HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE);
            // Write the response.
            ChannelFuture future = ctx.write(resp);
            // Close the non-keep-alive connection after the write operation is done.
            future.addListener(ChannelFutureListener.CLOSE);
        }
        else {
            ctx.close();
            throw new TooLongFrameException(
                    "HTTP content length exceeded " + maxContentLength +
                            " bytes.");
        }
    }

But this would need to be use presumably in HttpObjectAggregator and in HttpObjectDecoder how to share ? Is it enough to be public and overridable or is their a more pluggable strategy?

Then theres the issue of invoking it. Three sites & tests seem obvious, but not so this snippet from H.O.A line 118

 protected Object decode(ChannelHandlerContext ctx, HttpObject msg) throws Exception {
        FullHttpMessage currentMessage = this.currentMessage;

        if (msg instanceof HttpMessage) {
            assert currentMessage == null;

            HttpMessage m = (HttpMessage) msg;

            // Handle the 'Expect: 100-continue' header if necessary.
            // TODO: Respond with 413 Request Entity Too Large

How do I test content size here?

@trustin

This comment has been minimized.

Show comment
Hide comment
@trustin

trustin Feb 7, 2013

Member

How about generating a FullHttpMessage with failed DecoderResult? Then a user handler could decide what to do with too large message. HttpObjectAggregator could keep discarding the subsequent HttpContents decoded by HttpObjectDecoder until LastHttpContent is received. To avoid unnecessary inbound traffic a user handler could often send the 'entity too large' response immediately and close a connection.

Member

trustin commented Feb 7, 2013

How about generating a FullHttpMessage with failed DecoderResult? Then a user handler could decide what to do with too large message. HttpObjectAggregator could keep discarding the subsequent HttpContents decoded by HttpObjectDecoder until LastHttpContent is received. To avoid unnecessary inbound traffic a user handler could often send the 'entity too large' response immediately and close a connection.

@trustin

This comment has been minimized.

Show comment
Hide comment
@trustin

trustin Feb 7, 2013

Member

A user handler could check if the message decoded by HttpObjectAggregator is broken using the decoderResult property:

if (msg.getDecoderResult().isFailure()) {
    if (msg.getDecoderResult().cause() instanceof TooLongFrameException) {
        // send 413 Request Entity Too Large here and close the connection.
    } else {
        // send 403 Bad Request and close the connection
    }
}
Member

trustin commented Feb 7, 2013

A user handler could check if the message decoded by HttpObjectAggregator is broken using the decoderResult property:

if (msg.getDecoderResult().isFailure()) {
    if (msg.getDecoderResult().cause() instanceof TooLongFrameException) {
        // send 413 Request Entity Too Large here and close the connection.
    } else {
        // send 403 Bad Request and close the connection
    }
}

@trustin trustin modified the milestones: 4.1.0.Final, 5.0.0.Alpha2 Feb 7, 2014

@trustin

This comment has been minimized.

Show comment
Hide comment
@trustin

trustin Feb 7, 2014

Member

Maybe worthwhile to fix this in 4.1.

Member

trustin commented Feb 7, 2014

Maybe worthwhile to fix this in 4.1.

@trustin trustin added the feature label Feb 14, 2014

@m0wfo

This comment has been minimized.

Show comment
Hide comment
@m0wfo

m0wfo Feb 22, 2014

Contributor

@trustin can this be closed now since #2211 is closed?

Contributor

m0wfo commented Feb 22, 2014

@trustin can this be closed now since #2211 is closed?

@trustin

This comment has been minimized.

Show comment
Hide comment
@trustin

trustin Mar 20, 2014

Member

@m0wfo You are right. Closing.

Member

trustin commented Mar 20, 2014

@m0wfo You are right. Closing.

@trustin trustin closed this Mar 20, 2014

@trustin trustin modified the milestones: 4.1.0.Beta1, 4.1.0.Final Jul 3, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment