CSRF token generation incompatible with optional route parameters

Reading #14 seems like this will be a much needed and welcomes test. Maybe #testing channel could help point where it should go. Or could CsrfTokenGeneratorTest be extended to cover this?

I've made some manual tests, and noticed that some cases were not working, even with the proposed patch in #13.

These have been my tests:

Without the patch from #13:

$url = Url::fromRoute('test.example', ['param1' => 'value']);// --> /test/example/value <-- Works.
$url = Url::fromRoute('test.example', ['param1' => '']);// --> /test/example <-- Works.
$url = Url::fromRoute('test.example', ['param1' => NULL]); // --> /test/example <-- Works. 
$url = Url::fromRoute('test.example', []); --> /test/example <-- Does NOT work.

With the patch from #13:

$url = Url::fromRoute('test.example', ['param1' => 'value']); // --> /test/example/value <-- Works.
$url = Url::fromRoute('test.example', ['param1' => '']); // /test/example <-- Does NOT work.
$url = Url::fromRoute('test.example', ['param1' => NULL]); // --> /test/example <-- Does NOT work.
$url = Url::fromRoute('test.example', []); // --> /test/example <-- Works.

The patch in #13 has been a step towards the solution, but an additional step was needed: cleaning the optional params from the path before validating. After doing that, all four examples are working now.

This is all what has been additionally done:

• Remove the unreplaced params from the route path and the trailing slash.
• Unify with a trait the code which generates a route path replacing its param placeholders, so both classes share the same logic.
• Add a test. It's a unit test, but involving two classes: RouteProcessorCsrf and CsrfAccessCheck

vidorado → changed the visibility of the branch 11.x to hidden.

vidorado → changed the visibility of the branch 2987987-csrf-token-optional-route-params to hidden.

Whoops, I just realized that the MR target branch was 10.x, so I created a new one targeting 11.x.

Now, $request->query is typed as InputBag, and I haven’t been able to mock it because it is a final class—not even with Prophecy. I ended up using a real InputBag, but it feels a bit 'dirty' since the unit test is relying on a class that isn’t the subject of the test.

While PHPUnit can be configured to allow mocking final classes, changing core's phpunit.xml.dist seems out of scope.

Does anyone have any ideas? I will also post a question in #testing Slack channel.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Fixed the PHPCS issue reported on #25 :)

Have not reviewed yet

But issue summary appears to be incomplete. Please use standard issue template

Thanks!

Oops! sorry, I missed that! I've now updated the Issue Summary.

While working on it, I realized that previously generated CSRF-protected links will no longer work after this change, making it a breaking change.

Leaving the status as 'Needs work' until I find a solution to address this issue. Most likely, a fallback backward compatibility check can be added if the initial check fails. I think this is the least disruptive approach, although it will not be entirely clean.

Added backwards compatibility with previously valid CSRF tokens.

Left some comments on MR

Apologies for the mistakes, @smustgrave! I’ve gotten used to avoiding modifications to type hints in existing code, as Berdir mentioned it was outside the scope of a bugfix issue. That said, I’m more than happy to embrace the 'boy scout' principle by improving types and docs! 😊

On another note, I’m adding an additional PHPCS sniff to my pre-commit script to ensure I don’t miss any more {@inheritdoc} annotations in the future. 😅

There’s a few issues floating around to address those so the rule can be enabled.

Believe feedback for this one has been addressed.

Updated comments to meet the coding standards. Most are for wrapping, so should be fine, but at least the summary lines need to be checked. They had to be reduced to 80 so there could be mistakes.

Re 35, checked 'summary lines' made edits. I am ok with RTBTC.

Summary lines look good to me :)

@vidorado I am reading through the comment in the last commit. I see what you mean with the 'it'. I think it is quite a complex message you are putting across. I must ahve read it 20 times and tried to understand it in relation to your comments before i understood. Not sure why but i was initially thinking 'method' meant 'a way of doing things' rather than 'a function'. But at least by throwing in the 'it' you were able to see that I was misunderstanding the comment and to identify what it was that i was misunderstanding.

It is nitty but grammatically

This allows checking for legacy CSRF tokens and treat them as valid, too.

should be

This allows checking for legacy CSRF tokens and treating them as valid, too.

I'm responding to you in the MR on git.drupalcode.org instead of here in the issue queue, to avoid bothering other followers with detailed comments :)

I had to adjust my notification preferences on git.drupalcode.org because an internal email was selected for notifications instead of my actual email. Now, I’ll finally receive notifications properly.

#41 If they are bothered im sure they will let us know : ).

The approach looks great, just added some commenting nits to the MR.

Thanks for your suggestions @longwave!

It's not clear to me why we'd need a bc layer for stale CSRF links - CSRF tokens are ephemeral based on the session, and would be regenerated when a user refreshes a page anyway.

@catch #45 I'm not sure this involves 'stale CSRF links'? Is it not about the limitations on how routes can be structured? Certain types of routes that contain placeholders used to allow CSRF generation but now they will no longer support CSRF generation? Something like that..

@catch I realize now that I completely misunderstood the use of CSRF tokens. I thought they were also meant to be used for public links, ensuring they were generated by Drupal, similar to how the itok parameter works for image styles.

After reviewing the code more closely, I see your point. A session-stored random seed is used to generate the CSRF token, so it doesn’t actually make sense to include it in a 'public' link.

class CsrfTokenGenerator {
  ...
  public function get($value = '') {
    $seed = $this->sessionMetadata->getCsrfTokenSeed();
    if (empty($seed)) {
      $seed = Crypt::randomBytesBase64();
      $this->sessionMetadata->setCsrfTokenSeed($seed);
    }
    return $this->computeToken($seed, $value);
  }
  ...
}

So I’ll revert that part. Thanks for pointing it out! :)

Done!

I guess I can safely restore to RTBC...

Good work! But i think NR is correct.

All threads are now resolved. Thanks.

@vidorado After welcome visits from @longwave and @catch in the same day we'll probably have to wait a month now : ).

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Fixed a deprecation and restoring to RTBC

Committed/pushed to 11.x, thanks!

catch → closed merge request !10575

Automatically closed - issue fixed for 2 weeks with no activity.