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

SizeLimitHandler does not enforce 0 responseLimit #10337

Closed
gonphsn opened this issue Aug 17, 2023 · 3 comments
Closed

SizeLimitHandler does not enforce 0 responseLimit #10337

gonphsn opened this issue Aug 17, 2023 · 3 comments
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@gonphsn
Copy link

gonphsn commented Aug 17, 2023

Jetty version(s)

jetty-all-compact3-9.4.51.v20230217.jar

Jetty Environment

N/A

Java version/vendor (use: java -version)

java version "1.8.0_381"
Java(TM) SE Runtime Environment (build 1.8.0_381-b09)
Java HotSpot(TM) 64-Bit Server VM (build 25.381-b09, mixed mode)

OS type/version
Windows (Behavior is OS agnostic)

Description

We've deployed the SizeLimitHandler added in #8774 and observed that a 0 responseLimit is not being enforced.

This behavior is at odds with SizeLimitHandler documentation:

public SizeLimitHandler​(long requestLimit,
                        long responseLimit)

Parameters:
    requestLimit - The request body size limit in bytes or -1 for no limit
    responseLimit - The response body size limit in bytes or -1 for no limit 

and has code smell in the implementation:

        if (_requestLimit >= 0 || _responseLimit >= 0)   <-- Check for >= here...
        {
            HttpOutput httpOutput = baseRequest.getResponse().getHttpOutput();
            HttpOutput.Interceptor interceptor = httpOutput.getInterceptor();
            LimitInterceptor limit = new LimitInterceptor(interceptor);

           ...

            if (_responseLimit > 0)                    <-- But check for > here
            {
                httpOutput.setInterceptor(limit);
                response = new LimitResponse(response);
            }
        }

The test coverage in SizeLimitHandlerTest does not appear to cover a 0 case so I am unsure if this behavior is intentional - meaning the doc should be updated - or unintentional, and the implementation should be updated.

We are definitely aware of the EOCS status of Jetty 9.4.XX - but you seem to be building a project right now for 9.4.52 that includes other tweaks to SizeLimitHandler - ec197a2

Can you confirm behavior and sneak this in?

How to reproduce?
Use a SizeLimitHandler with a _responseLimit of 0, observe the limit is not enforced.
Use a SizeLimitHandler with a _responseLimit of 1, observe the limit is enforced.

@gonphsn gonphsn added the Bug For general bugs on Jetty side label Aug 17, 2023
@joakime
Copy link
Contributor

joakime commented Aug 17, 2023

The release 9.4.52 is a paid sponsored release on a version that is at End of Community Support.

@gonphsn
Copy link
Author

gonphsn commented Aug 17, 2023

I understand, just thought to ask. It certainly is an edge condition with a reasonable workaround of just using 1.

Bear in mind, this observation would be true for 10.0 through 12.0 from what I can see in the source tree, but it would be lovely if it came back to 9.4.X the next time the opportunity presents itself.

@lachlan-roberts lachlan-roberts self-assigned this Aug 18, 2023
lachlan-roberts added a commit that referenced this issue Aug 18, 2023
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Aug 18, 2023
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Aug 18, 2023
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Aug 18, 2023
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Aug 18, 2023
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Aug 22, 2023
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Aug 23, 2023
lachlan-roberts added a commit that referenced this issue Aug 24, 2023
lachlan-roberts added a commit that referenced this issue Aug 25, 2023
@lachlan-roberts
Copy link
Contributor

Completed in PR #10342.
Will be in 9.4.52 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

No branches or pull requests

3 participants