Skip to content

Conversation

@LukasReschke
Copy link
Member

This adds a new CSRF manager for unit testing purposes, it's interface is based upon https://github.com/symfony/security-csrf. Due to some of our required custom changes it is however not possible to use the Symfony component directly. Most of this are unit test adjustments.

Furthermore, this regenerates the CSRF token upon login because otherwise somebody else might be able to note down the CSRF token before login on a shared computer.

cc @rullzer @MorrisJobke @owncloud/security


Fixes #12159

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we expect to have multiple implementation here?

@DeepDiver1975
Copy link
Member

a lot of stuff foe just one little tiny token ....

This adds a new CSRF manager for unit testing purposes, it's interface is based upon https://github.com/symfony/security-csrf. Due to some of our required custom changes it is however not possible to use the Symfony component directly.
Otherwise somebody else might be able to note down the CSRF token before login on a shared computer.
@LukasReschke
Copy link
Member Author

a lot of stuff foe just one little tiny token ....

To be fair, most of it is simply boiler plate code (e.g. copyright header, class declaration, PHPDocs) and in the end it's simply moving static code out of some place and putting it in a unit-tested other place. As well as giving additional functionality such as invalidating a token (which was actually the reason for the change).

Actual logic:

  • csrftoken.php: 9 lines of code
  • csrftokengenerator.php: 2 lines of code
  • csrftokenmanager.php: 20 lines of code
  • sessionstorage.php: 10 lines of code
  • lib/private/server.php: 27 lines of code
  • … tons of boiler plate code plus tests …

The alternative would have been adding more static untested code. In fact, I'm tempted to say the actual logic has not really increased at all.

Other comments have been addressed.

@rullzer
Copy link
Contributor

rullzer commented Jan 25, 2016

Looks a lot cleaner and yay for more non-static code!

I did some tests and everything still works and the code did not burn down my house.

👍

@MorrisJobke
Copy link
Contributor

Code looks good and it works 👍

DeepDiver1975 added a commit that referenced this pull request Jan 26, 2016
Add new CSRF manager for unit testing purposes
@DeepDiver1975 DeepDiver1975 merged commit 2bafb1c into master Jan 26, 2016
@DeepDiver1975 DeepDiver1975 deleted the refactor-csrf branch January 26, 2016 10:36
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Perform CSRF check not within request class

4 participants