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

[chrispatmore]

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:

• store end handler ids and remove if more than one (not the easiest option)
• check the context for the token not just the session ctx.get(headerName) (I think this is better)
  • Use this value to gate generating a new token
  • Use this value to set the session in the end handler
  • Only need one of above, but might do both as I think they make logical sense to have anyway

[chrispatmore]

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

[chrispatmore]

Would be great to get this in soon if possible

[tsegismont]

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

vertx-web/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/SessionHandlerImpl.java

Lines 280 to 286 in ce78ea0

	// we need to keep state since we can be called again on reroute
	if (!((RoutingContextInternal) context).seenHandler(RoutingContextInternal.SESSION_HANDLER)) {
	((RoutingContextInternal) context).visitHandler(RoutingContextInternal.SESSION_HANDLER);
	} else {
	context.next();
	return;
	}

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

[chrispatmore]

Oh cool hadn't seen that pattern I did something else: #2558
Will update that PR when I get a moment thanks!

[tsegismont]

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

[tsegismont]

Fixed by #2560

[chrispatmore]

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]

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

Same fixes were applied in both branches

[chrispatmore]

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]

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