#2002 add csrf validation

[muhammad-abubakar]

Motivation:

relates to https://github.com/vert-x3/vertx-web/issues/2002

Conformance:

Your commits should be signed and you should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines

[muhammad-abubakar]

So in which direction should we go? define handler specific exceptions? If we want to go in this direction then it's better to discuss and name them explicitly. It will be a big change though, adjustment in actual implementation along with test cases. I am up for that though

[pmlopes]

@muhammad-abubakar I think it's all about the use case. (hence the need for a test, so we can see how this would be used). Say that we would like to allow users to use these exception types explicitly on their application code, say using a try catch or a instanceof operation. Then the exception should not live in the impl package.

In this case, we should define a couple of basic exceptions, can we agree on:

• ValidationException
• SecurityException

As a starting point?

If the use case if for internal usage, and or, logging purposes, then we can keep the exception in the impl package and we can have it as it is in the PR. The thing with impl package is that, if it is for internal use only, we can change it at any time, however if we are making it public, then we need to follow the rules of semantic versioning, otherwise we risk breaking everyones code if it changes.

[pmlopes]

The names I've used above are not normative. You are welcome to propose better ones, or a different hierarchy.

[muhammad-abubakar]

@pmlopes sure we can start with the two exception. I will write some test to see how can we use it. Thanks for the explanation related to impl

[muhammad-abubakar]

@pmlopes looking at illegalargumentexception of other handler I am inclined towards using individual validation exception inherited from the AccessDeniedException. For example JWTAuthenticationException. What do you think?

[muhammad-abubakar]

Hi @pmlopes , Any follow up to this?

[pmlopes]

@muhammad-abubakar, I'm not 100% sure about having individual exceptions for each handler:

Say we have:

• CorsValidationException
• JWTAuthorizationException
• Oauth2AuthorizationException
• HttpForbiddenException

What would be the real difference between all these exceptions? they all end up with a status code response of 403.

I don't see the benefit of such fine grained class tree, versus something smaller like:

1. UnauthorizedException (401)
2. BadRequestException (400)
3. NotFoundException (404)
4. ForbiddenException (403) <-- all above actually map to this one only
5. InternalServerErrorException (500)

What I think what we're missing is not really a large set of exceptions but a way to capture the context where the exceptions are throw. when we do something like:

ctx.fail(403, new IllegalStateException("validation is incorrect"));

The exception being propagated does not really capture the source where is happens, and this is what we probably should include. Also, it doesn't tell where it happened be it a stack trace or even which handler.

I am guessing that if we would say that we have this small subset of exceptions they could always force the fill of the stacktrace, so we know where they happen. With this information we could later at exception processing time, walk the trace and attempt to recover which handler throw?

Say if we would include the handler in the constructor we could get the class out of the caller and do logging as:

new ForbiddenException(this, "CORS validation failure");
// log as:
// [CORSHandler] Cors validation failure

And maybe we could have a callee() read only getter that would return the class type of the first argument. For example, it would return CORSHandlerImpl.class

[pmlopes]

@muhammad-abubakar what is your oppinion on this style:

https://github.com/vert-x3/vertx-web/blob/issues/2002/vertx-web/src/main/java/io/vertx/ext/web/handler/HttpException.java

If you import the static method, you could use it like this:

 switch (httpStatusCodeOf(exception)) {
      case 401:
        ((HttpException) exception)
          .catchFrom(CSRFHandler.class, err -> {
            // it is a 401 thrown by CSRFHandlerImpl
          })
          .catchFrom(JWTHandler.class, err -> {
            // it is a 401 thrown by JWTHandlerImpl
          });
        break;
      500:
      default:
        // either it is a 500 or the exception is not of type `HttpException`
    }

I am thinking this would address your requirement, right? To make things more interesting, we keep the exception types to the minimum and would populate the stacktrace if no cause was provided. Still not sure on this. Maybe we do want always a stacktrace as it could be hard to track back just from the cause.

[muhammad-abubakar]

@pmlopes Yes we can stick with the 5 you mentioned.

UnauthorizedException (401)
BadRequestException (400)
NotFoundException (404)
ForbiddenException (403) <-- all above actually map to this one only
InternalServerErrorException (500)

Anything apart from this, we can throw the stack or at least the class where this exception actually occurred.

> populate the stacktrace if no cause was provided I think we should always populate it even when the cause is provided. Because then it will be easy to debug or we could provide the class name when cause is known.

[lasteris]

@pmlopes @muhammad-abubakar I see that this issue stuck in a while. Maybe, i could continue to work on it as my first issue with new PR. But i've read the conversation and doubt that the end user should work with vertx interfaces. Instead, Exceptions with appropriate names would be more verbose and convenient. But on the other side...too many exception-classes then need to be created

[tsegismont]

Hi @lasteris , thank you for stepping up.

I don't believe we should move forward with this. As explained in https://github.com/vert-x3/vertx-web/issues/2002#issuecomment-882500614, it is possible add a failureHandler after the CORSHandler.

In this failure handler, users can create a custom response. And if they want to centralize error handling, instead of creating the response directly they can put an object with io.vertx.ext.web.RoutingContext#put and retrieve the data in the global failure handler.

[tsegismont]

@lasteris there are others "good first issue" or "help wanted" issues if you're willing to contribute