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

ContentCachingRequestWrapper can have maxContentLength to limit caching [SPR-14829] #19395

Closed
spring-issuemaster opened this issue Oct 20, 2016 · 3 comments

Comments

@spring-issuemaster
Copy link
Collaborator

commented Oct 20, 2016

Hamid Virani opened SPR-14829 and commented

we wanted to use ContentCachingRequestWrapper so that we can log the payload in one of the error handler of the @ControllerAdvice.

But, to protect from OutOfMemoryError, we want to restrict maximum content length that can be cached. If the content length is greater than this maximum length, the class can just throw an exception.

This maxContentLength can be be optional (so an overridden constructor which takes this parameter). If provided then it can throw a RunTimeException to indicate the same or else it can continue working without any restriction as it is working today.

This feature may be beneficial to anyone who wants to have the same capability.


Affects: 4.1.9

Referenced from: commits ad53867, 4beeeb8

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 20, 2016

Juergen Hoeller commented

Since our RequestLoggingFilter variants have the same issue, I'm keen on introducing such a content cache limit there as well. We already have a maxPayloadLength setting in AbstractRequestLoggingFilter, restricting the extent of the payload to log... so it would make a lot of sense to simply only cache the payload up until that length to begin with.

From that perspective, I'm leaning towards a lenient variant of your proposal: A setting that restricts the maximum length of ContentCachingRequestWrapper's cached content, simply not caching any further bytes read. This means that getContentAsByteArray may return a limited part of the content, but there won't ever be an exception from the caching request wrapper.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 20, 2016

Hamid Virani commented

Hi Juergen,

Thanks for acknowledging the problem.

I suggested throwing an Exception, so that we we can catch it somewhere (error handler in our case) and throw HTTP Status 413 - Payload Too Large. If we just limit the caching, will we ever know that the payload caching was indeed limited? Another idea to continue with the leniant approach can be setting some property (boolean maybe) to indicate that the caching was limited by maxPayloadLength.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 17, 2017

Juergen Hoeller commented

I've added a corresponding constructor with a contentCacheLimit argument which our request logging filter is using now.

Additionally, there's a handleContentOverflow method which gets triggered when the content cache limit is reached. The default implementation is empty but a custom subclass may raise an exception here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.