Can't get CSRF protection to work on CXF

[mvcbot]

Issue by mthmulders
Thursday Sep 06, 2018 at 19:07 GMT
Originally opened as mvc-spec/ozark#202

---

Setup:

• OpenLiberty 18.0.0.2
• Java EE 8
• Ozark 1.0.0-m04 SNAPSHOT (20180903.132704-47)
• MVC 1.0 SNAPSHOT (20180902.064053-77)

I'm trying to get CSRF protection to work on my controller. It looks like this:

    @CsrfProtected
    @POST
    @Path("/configure")
    @View("redirect:/hello")
    public void configure(@FormParam("locale") final String locale) {
        // left out
    }

If I read the spec correctly, the default for javax.mvc.security.CsrfProtection is EXPLICIT, saying that any method annotated with @CsrfProtected is to be protected.

I have a page that submits an HTML form to this controller method using POST, and it lacks the CSRF token parameter. I would expect the form submission to be rejected, but my controller method is hit.

Browsing through Ozark source code, I expect it is the responsibility of the CsrfValidateInterceptor class to perform this check. Using the debugger, I can see it is being instantiated, but the breakpoint in aroundReadFrom(...) is never hit.

Again, reproduction repo is https://github.com/mthmulders/openliberty-mvc. Any clues?

[mvcbot]

Comment by chkal
Friday Sep 07, 2018 at 03:51 GMT

---

Yes, you are correct. The method should actually be processed by CsrfValidateInterceptor and the controller should not be invoked if the CSRF token is missing. That's really weird.

CsrfValidateInterceptor is registered via a name binding, which means that it should actually be invoked for all requests to controller methods (annotated with @Controller).

Could you please check if the interceptor registration works by confirming that this code gets executed:

https://github.com/mvc-spec/ozark/blob/master/core/src/main/java/org/mvcspec/ozark/bootstrap/DefaultConfigProvider.java#L45

There is also some special handling for CXF here:

https://github.com/mvc-spec/ozark/blob/master/core/src/main/java/org/mvcspec/ozark/bootstrap/DefaultConfigProvider.java#L56

But my guess is that this CXF workaround isn't the cause of your issues.

My only other guess is that the name binding isn't working as expected for some reason. I ran into issues regarding the name binding before, so this is just a wild guess, but it may be possible.

[mvcbot]

Comment by mthmulders
Friday Sep 07, 2018 at 08:11 GMT

---

I can confirm that DefaultConfigProvider.java#L45 is being hit. Also, inside register(FeatureContext, Class), isCxf equals true, and providerInstances is an ArrayList with one element in it, an instance of CsrfValidateInterceptor. This object is then registered with the feature context.

[mvcbot]

Comment by chkal
Saturday Sep 08, 2018 at 06:08 GMT

---

Thanks for confirming. Looks like we need to do some more debugging to solve this. Unfortunately there are a few weird issues with CXF like #168 for example.

I'll try to setup some test environment with plain Tomcat + CXF + Ozark to see if I can find the root cause for this.

Of course any kind of help is welcome!

[mvcbot]

Comment by mthmulders
Saturday Sep 08, 2018 at 07:05 GMT

---

Yesterday I've tried to debug OpenLiberty itself - to no avail. I was just about to resort to plan B, which is exactly what you've proposed. I'm far from an expert in CXF, but I'll see what I can find...

[mvcbot]

Comment by chkal
Saturday Sep 08, 2018 at 07:59 GMT

---

Ok, great! Any kind of help is welcome.

My current guess is that the name binding @Controller on the CsrfValidateInterceptor doesn't work as expected for some reason. Maybe because CDI is involved. But that's just a wild guess. Anyway, it is worth a try to just remove @Controller from CsrfValidateInterceptor which should execute the interceptor for all JAX-RS requests, not just for MVC ones.

[mvcbot]

Comment by mthmulders
Monday Sep 10, 2018 at 08:35 GMT

---

Just to make sure... does it work in other JAX-RS implementations? I've found RESTfu­­l Jav­a­ wit­h ­JAX­-­­RS 2.­0­ (Second Edition), that says:

These interceptors are only triggered when a MessageBodyReader or MessageBodyWriter is needed to unmarshal or marshal a Java object to and from the HTTP message body.

If that was true, I'd expect that other JAX-RS implementations also suffer from this. In this particular case, the controller method isn't reading Java objects directly from the HTTP message body.

[mvcbot]

Comment by chkal
Monday Sep 10, 2018 at 13:09 GMT

---

It is definitely working on Glassfish and AFAIK also on Wildfly. My interpretation of this spec requirement is that the reader is needed because you are actually using @FormParam in your code to access form parameters which got submitted in the body. No?

[mvcbot]

Comment by mthmulders
Monday Sep 10, 2018 at 13:42 GMT

---

Well, of course I hoped that it would work in other JAX-RS implementations :-). The document I referred to isn't a specification, so I don't know it's status. I interpreted it as "when a message body is read to a POJO" - like when a JSON structure is sent and the controller method has a method argument of a corresponding Java structure.

I'll try a bit more debugging when/how these interceptors are invoked in CXF in the coming days.

[mvcbot]

Comment by chkal
Monday Sep 10, 2018 at 15:23 GMT

---

Thanks for the clarification. Well, actually it may be possible that different JAX-RS implementations handle ReaderInterceptor invocation differently.

The exact wording in the JAX-RS spec is:

An entity interceptor implementing ReaderInterceptor wraps around calls to MessageBodyReader ’s
method readFrom . An entity interceptor implementing WriterInterceptor wraps around calls to
MessageBodyWriter ’s method writeTo . JAX-RS implementations are REQUIRED to call registered
interceptors when mapping representations to Java types and vice versa.

It would be interesting to know how CXF handles this requirement.

Also, it may be worth a try to add a parameter of type javax.ws.rs.core.Form to your controller method. If CXF doesn't execute the ReaderInterceptor because there is no Java type to map, this may force it to do so.

[mvcbot]

Comment by mthmulders
Monday Sep 10, 2018 at 21:14 GMT

---

Bingo! Adding an javax.ws.rs.core.Form parameter to the controller method fires the CsrfValidateInterceptor.

By the way, paragraph 3.3.2.1 of the JAX-RS 2.1 spec suggests that a ReaderInterceptor should be executed for @FormParam method arguments:

The value of a parameter not annotated with @FormParam or any of the annotations listed in in Section 3.2, called the entity parameter, is mapped from the request entity body. Conversion between an entity body and a Java type is the responsibility of an entity provider, see Section 4.2. Resource methods MUST have at most one entity parameter.

I did a quick search in the CXF issue tracker, but can't find anything related to this issue.

What would be the way to go here? File an issue with CXF and see if they agree upon our interpretation of the spec at this point?

[mvcbot]

Comment by chkal
Tuesday Sep 11, 2018 at 05:27 GMT

---

Ok, thanks a lot! That's interesting!

I just asked for clarification about this on the jaxrs-dev list. But I agree that CXF is most likely doing it wrong. However, as always, such statements in the spec are up to interpretation. 😁

[chkal]

Something similar seems to happen on Wildfly. See #38.

[gtudan]

We might have fixed this in #39, but there are still some issues with CSRF-Validation and CXF. I'll look into it.

[gtudan]

Things got better with the mentioned changes, but the example Maarten provided still doesn't work. The CSRF-Validation-Exception get's thrown and is handled by the ExceptionMapper, but the form renders nonetheless.

It looks to me like CXF doesn't bail out on the exception, but continues to pass the request to the ViewableWriter. This ends up in the strange situation where you get the view you requested, but with a 403 status code.

@chkal do we have any known issues with the exception mappers in OpenLiberty?

[chkal]

@gtudan To be honest, I'm focusing on Glassfish and Wildfly for now, as there were too many issues with CXF in the past. So TomEE and Liberty are second proprity for me at the moment.

But the behavior you are describing is really weird. IMO the ViewableWriter should not be executed in this case.

[gtudan]

So the good news is that the CSRF-Validation works on CXF on-par with the other implementations. Sadly we seem to have an issue with csrf-validation in combination with the @View-Annotation (see #91). You can work around this by replacing the annotation with a String return value:

    @CsrfProtected
    @POST
    @Path("/configure")
    // @View("redirect:/hello") - replace this
    public String configure(@FormParam("locale") final String locale) {
        return "redirect:/hello"; // with this
    }

@mthmulders: If its okay with you I would like to close the issue in favor of the more detailed #91.

[mthmulders]

I'm totally fine with that, thanks for verifying though.

[chkal]

@gtudan Thanks a lot for your help with this! We should continue to discuss this topic as part of #91.