vert-x3 / vertx-web

HTTP web applications for Vert.x
Apache License 2.0
1.11k stars 535 forks source link

CSRF Handler update doesn't work with `context.reroute()` #2557

Closed chrispatmore closed 9 months ago

chrispatmore commented 10 months ago

Version

4.5.0, 4.5.1

Context

When upgrading to 4.5.1 in our application the CSRF handler check started failing saying the token was used or expired. This is because the app uses context.reroute() which sends the context back though the CSRFHandler and as (after my last set of changes) the token is not added to the session until after the request completes, it thinks there is no token so generates a new one updates the request and adds a new end handler. then when the request does end it unwinds the end handlers in the reverse order, which means the wrong token (first one) is set into the session, and the second token is sent back on the response, so they now don't match and the client is broken.

Do you have a reproducer?

No

Steps to reproduce

  1. Set up a route with CSRF handling
  2. when hitting that route call ctx.reroute() to another route with CSRF handling
  3. try to use the returned token to do a post call on another route with CSRF handling

Extra

Lots of fix options for this:

chrispatmore commented 10 months ago

Found an issue with my change, sorry... fyi @pmlopes @tsegismont

chrispatmore commented 9 months ago

Would be great to get this in soon if possible

tsegismont commented 9 months ago

I think the CSRF handler should be protected from invocations of reroute, like the SessionHandler (and some others):

https://github.com/vert-x3/vertx-web/blob/ce78ea0446a9e0fc8c309f5103a5f5a6b9012534/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/SessionHandlerImpl.java#L280-L286

That should solve the problem. Can you contribute the fix?

chrispatmore commented 9 months ago

Oh cool hadn't seen that pattern I did something else: https://github.com/vert-x3/vertx-web/pull/2558 Will update that PR when I get a moment thanks!

tsegismont commented 9 months ago

I'll go ahead and update it so we can get the fix in 4.5.2

tsegismont commented 9 months ago

Fixed by #2560

chrispatmore commented 9 months ago

Thanks @tsegismont ! we've got different fixes in master vs. 4.x now. Do we want that?

Should we also include your fix in the master branch?

tsegismont commented 9 months ago

we've got different fixes in master vs. 4.x now. Do we want that?

Same fixes were applied in both branches

chrispatmore commented 9 months ago

Ah I see you merged my branch to master, merged your branch to 4.x and then yours into master, so the diffs on both are the same, but my original changes were undone, fine. Thanks!

tsegismont commented 9 months ago

I did my best in order to keep the trace of your commit. Thank you for your contributions!