Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Jul 8, 2024

As reported on this http4s issue, in some conditions, if the request's EntityBody is consumed more than once by the caller, a BodyAlreadyConsumedError is raised. In our MAuthMiddleware, this can happen when performing the mAuth authentication on large or slow requests, whereby the request's Stream might be chunked, requiring multiple consumptions and causing intermittent failures.

This PR aims at solving this bug

@ghost ghost requested a review from amilne-mdsol July 8, 2024 17:41
* request body is now consumed in one go and stored in memory

* prevent occurrences of BodyAlreadyConsumedError
@ghost ghost force-pushed the NOJIRA-fix_body_consumption_in_http4s branch from e8c2b22 to bf42749 Compare July 8, 2024 17:42
tmilner
tmilner previously approved these changes Jul 9, 2024

authenticator.authenticate(req)(requestValidationTimeout).map(res => (res, authCtx))
fk(for {
strictBody <- request.toStrict(none)
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, toStrict enforces a request size limit doesn't it? Could pass that in as an option (potentially with a default to keep it backwards compatible?)

Copy link
Author

Choose a reason for hiding this comment

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

Nope, it doesn't enforce any limit - it would be recommended to enforce one, because toStrict only consumes the whole EntityBody and replaces any multipart header with a content-length (as a testament to its function), but there is no fixed size limit. In practice, this means that your application might blow up if the request is too large to be held in memory while it gets deserialised. I decided to not put any 'feature flag' on this, because I gauged that for our use cases there is no such risk - but if you know that it's not the case otherwise, I can add it.

@tmilner tmilner dismissed their stale review July 9, 2024 08:24

Question added

@tmilner tmilner merged commit 62a8187 into master Jul 9, 2024
@ghost ghost deleted the NOJIRA-fix_body_consumption_in_http4s branch July 9, 2024 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants