RESTEasy Reactive: make body-reading filters force reading the body

[FroMage]

Description

In #22209 I asked to be able to run request filters after the body has been read, but it turns out that if the endpoint doesn't have any @FormParam we skip reading the body, which means I can't use these filters for some endpoints.

I'd like to make these filters force the body to be read to avoid this issue.

Implementation ideas

No response

[quarkus-bot]

/cc @geoand, @stuartwdouglas

[geoand]

Thanks for reporting. I'll have a look.

[geoand]

Under what circumstances would we read the body? For each and every resource method (that is POST or PUT I guess)?

[FroMage]

Yeah, I guess. If such filters are present.

[geoand]

Just to be sure we are on the same page, what do you expect to be able to do in a such a filter when there is no body?

[FroMage]

In my case there is a body :) It's just that the endpoint isn't reading it. My filter is verifying CRSF tokens set in the form body, without the endpoint having to worry about them.

[geoand]

And you expect your filter to read the input stream?

[FroMage]

Oh no. The content-type is formish (urlencoded or multipart) so I expect RR to read it and populate FormData.

[geoand]

Hm... That's a little tough TBH as we engage the form handling code based on the @Consumes content-type, not the client content-type.

[FroMage]

Mmmm… I never set that @Consumes though… I think the presence of @FormParam must play a role too, no?

[geoand]

Ah, right - absolutely correct.

The point is that up until now, we have relied on the declarative way to figure out if we should be reading a form or not.

[FroMage]

Yeah. Not sure what's best to do here. Probably let's think about this some more…

[FroMage]

Yeah. Not sure what's best to do here. Probably let's think about this some more…

[geoand]

Yup, I agree

[FroMage]

Note that the new security-crsf extension does this by using the Vert.x request to force-read the body.

Also note that another user requested this for regular endpoints: #25103 (reply in thread)

[FroMage]

So, both #25103 (reply in thread) and #29763 rely on forcing reading form parameters. In one case for a filter, in the other, for an endpoint.

We could make a @ForceReadFormBody (name doesn't matter) annotation that can be placed on an endpoint or filter and would force the form body handlers being added to the handler chains.

I thought about making it more generic even, and do something like @ForceReadFormBody(forContentTypes = { urlencode, multipart }) but that's probably ridiculous, since we're forcing the reading of form parameters in any case, it's not like we can do anything useful by filtering out or adding some form body types since there are only two that the form body handler can handle.

@geoand WDYT?

[geoand]

A generic @ForceReadFormBody sounds like a fine compromise

[FroMage]

Any opinion on naming?

• @ForceReadFormBody
• @ForceFormRead
• @RequiresFormRead
• @RequiresForm
• @WithForm
• @WithFormRead

We could also repurpose @RestForm to allow it on methods/classes (for endpoints and filters) but that's stretching its definition a bit.

[geoand]

I like @WithFormRead. Repusposing @RestForm sounds a bit too much if you ask me :)

[FroMage]

Yes, I thought so too :)

[FroMage]

OK, let's see if I can do this.

[geoand]

💪🏼

[sberyozkin]

@FroMage May be worth avoiding Form and have something more generic like @ForceBodyRead. If we decide to handle some JSON requests carrying the tokens then having a Form in the name might be confusing, and also having an option to force a body read may be useful in other non-CSRF related cases

[FroMage]

I'm not sure how we can force the body being read if we don't have any idea what type to deserialise it to by having a declared body parameter. I'd rather wait for a requirement for this feature than make it more complex than it needs now with the two use-cases we do have and know.

[FroMage]

I'm not sure exactly how @WithFormRead interacts with @ServerRequestFilter(readBody = true).

[geoand]

Oh darn, I forgot about that completely...

[geoand]

Maybe we should deprecate it?

[FroMage]

Well, it seems sublty different, because it forces the filter to be run after the body has been read (by what?) but before deserialisation happens (which is not the case for form parameters, they're deserialised).

So, they sort of seem related but orthogonal too :(

[FroMage]

Interesting that ReadBodyRequestFilterTest tests the reading of form parameters :)

[FroMage]

The other thing that's interesting is that the FormBodyHandler doesn't appear to check the content-type, because it's only added if there are any form parameters required. On the other hand, it never had to be invoked if there were no form parameters, so there are no checks that would dynamically invoke it if the body content type matches.

[geoand]

LOL

[FroMage]

Every time I touch this issue, I'm reminded that "it's complicated" 😂

[FroMage]

Apparently, the FormBodyHandler does check the content types because the parser factory checks the content type before returning a parser, which makes sense. So it won't be invoked if it doesn't match. Good.

Now, the semantics of @ServerRequestFilter(readBody = true) are currently: move this filter after the body is read, IFF it's read. And it's only read if there are endpoint parameters that require the body to be read.

If there are no @RestForm or @FormParam we don't read the body. If there are no entity parameter, we don't even know what MessageBodyReader to invoke, so again it won't be read.

It'd be more correct to call it @ServerRequestFilter(afterBodyIsRead = true).

I could make @ServerRequestFilter(readBody = true) imply @WithFormRead, but not sure if I should.

I could also make @WithFormRead imply readBody = true because if you want a form to be read in a filter, I can't see why you wouldn't want your filter to run after it's read. That one seems pretty obvious to me.

Or we keep both concerns separate. Apparently, we added readBody = true to solve #22209 which is the same use-case as here. So, in light of this, we could indeed make @WithFormRead imply readBody = true and deprecate readBody unless it happens that somebody else is using this for another use-case?

[geoand]

Thanks for looking into it!

I agree, let's deprecate the field and use the new annotation to imply it

[FroMage]

OK, done in #29825