in Laravel there is a class VerifyCsrfToken, in which the handle function is defined, which catches the request and checks the correspondence of the client and server tokens, then adds a cookie in response, otherwise a throw error.

public function handle($request, Closure $next) { if ( $this->isReading($request) || //проверяет метод формы на соотв. из массива ['HEAD','GET',...] $this->runningUnitTests() || $this->shouldPassThrough($request) || $this->tokensMatch($request) ){ return $this->addCookieToResponse($request, $next($request)); } throw new TokenMismatchException; }

We look at the condition and see that 4 functions are checked, the priority at || starts on the left, that is (most likely I am mistaken here) the tokenMatch has the smallest priority and if the first 3 functions return true, then this function will not be taken into account in the condition that it is bad.

And actually the question is, for guru-laravel, are my arguments correct?

  • Incorrectly formulated the question - through xdebug it is impossible to break into the check function of the token, because of this, it feels like the token is not checked at all. - Yevgeny Ammosov

2 answers 2

The main thing - yes, your reasoning is correct. If any of the first 3 methods returns true , then tokensMatch will not even be called. This applies to any PHP code.

Now let's see if this is a mistake or so planned. If planned, why and if this is not a threat.

 $this->isReading($request) || //проверяет метод формы на соотв. из массива ['HEAD','GET',...]

Checks if the current query is a reader. Let me remind you that according to the HTTP specification, GET and HEAD requests should not change the state of the system. And since they do not change the state of the system, then they are trite and do not need to be protected from CSRF attacks.

 $this->runningUnitTests() ||

In fact, a crutch, usually the code should not distinguish whether it is running in the environment of unit tests or not. Apparently, it was easier. In any case, the point is that to run from unit tests, the test always passes.

 $this->shouldPassThrough($request) ||

Here, without reading the source, it is difficult for me to say what is being checked. Source Yeah, check the list of exceptions for URIs for which the CSRF check should be skipped. By default, the list is empty, but there is an opportunity to shoot yourself a leg.

 $this->tokensMatch($request)

Finally, if the request wants to change something, not in the environment of unit tests and not in the list of exceptions, then we check the validity of the token.

In my opinion, everything is logical, there is no security error here. tokensMatch really should be called at the very end.

Potential problem with isReading . Many people do not take into account the semantics of GET requests and make, for example, deleting an entity by reference because of the ease of writing a simple link, rather than an entire form. Don't do that.

    I can’t say about Lara, but ..

    if the first 3 functions return true

    why? there are no priorities at all, calculations go from left to right, as soon as the first true is met, nothing will be checked further