Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

3.0 - adding error callback to CsrfComponent #6546

Closed
PGBI opened this issue May 12, 2015 · 11 comments
Closed

3.0 - adding error callback to CsrfComponent #6546

PGBI opened this issue May 12, 2015 · 11 comments
Milestone

Comments

@PGBI
Copy link
Contributor

PGBI commented May 12, 2015

In cakephp 2, it was possible to handle Csrf errors with a blackhole method. In cakephp 3, Csrf handling was moved out from SecurityComponent to CsrfComponent, but the CsrfComponent does not offer the option to choose how to handle csrf errors (no blackhole callback).

I'm asking this cause most of the time, csrf errors are caused by session expiration rather than hack attempt. I think it would be nice to be able to tell the user "sorry, your session has expired" instead of throwing a 403 error.

What do you think?

@dereuromark dereuromark added this to the 3.0.5 milestone May 12, 2015
@lorenzo
Copy link
Member

lorenzo commented May 12, 2015

The csrf check does not rely on the session

@PGBI
Copy link
Contributor Author

PGBI commented May 12, 2015

My bad. But it does rely on a cookie right? A cookie that can also expire.

@lorenzo
Copy link
Member

lorenzo commented May 12, 2015

The cookie will live for as long as you have the browser open

@PGBI
Copy link
Contributor Author

PGBI commented May 12, 2015

Unless you switch to privacy mode. Or if you delete them. Ok that's tricky situations, but I trust my users to find many more of them :D

In cakephp 2 it was possible to capture csrf errors with the blackhole method. Not in cakephp 3. Don't you think that's a regression?

Is there a reason why SecurityComponent would have such a feature and not the Csrf one?

@lorenzo
Copy link
Member

lorenzo commented May 12, 2015

I just think adding the callback would be misleading.
On tir. 12. maj 2015 at 19.38 Pierre notifications@github.com wrote:

Unless you switch to privacy mode. Or if you delete them. Ok that's tricky
situations, but I trust my users to find many more of them :D

In cakephp 2 it was possible to capture csrf errors with the blackhole
method. Not in cakephp 3. Don't you think that's a regression?

Is there a reason why SecurityComponent would have such a feature and not
the Csrf one?


Reply to this email directly or view it on GitHub
#6546 (comment).

@markstory
Copy link
Member

I don't see the lack of a callback as a regression as the current CSRF implemenatation is totally separate and distinct from the previous one. Would having a specific exception class help? That way you could render a different error template for CSRF errors which sounds like what you need if I'm understanding things correctly.

@markstory markstory modified the milestones: 3.1.0, 3.0.5 May 12, 2015
@PGBI
Copy link
Contributor Author

PGBI commented May 13, 2015

In cakephp 2, we could define a blackhole method as this:

public function beforeFilter() {
    $this->Security->blackHoleCallback = 'blackhole';
}

public function blackhole($type) {
    // handle errors.
}

$type being one of these strings: 'auth', 'csrf', 'secure', ...

So in case of csrf error, it was possible to tell the user something like "please make sure you have cookies enabled or reload the page to refresh session/cookie and try again" instead of just throwing a 400 error.

In cakephp 3, I don't see how to do the same thing. Here is why I think this is a regression.

Maybe adding a "blackhole-like" callback is not a good option. If you guys prefer using a specific exception class, it'll be good too, as long as I can render a different error template for CSRF errors as you said @markstory .

@PGBI
Copy link
Contributor Author

PGBI commented May 13, 2015

btw, I can work on this if you agree with this feature.

@markstory
Copy link
Member

I think specific exceptions is a better way to handle these kinds of situations going forward. Exceptions can be easily converted into error pages, or responses, or caught by controller hook methods like startupProcess(). I don't like callback methods as they are easy to accidentally not stop the request.

@lorenzo
Copy link
Member

lorenzo commented May 13, 2015

I think a custom exception is the way to go

tigrang added a commit to tigrang/cakephp that referenced this issue May 23, 2015
tigrang added a commit to tigrang/cakephp that referenced this issue May 23, 2015
@markstory
Copy link
Member

Fixed with #6620

lorenzo added a commit that referenced this issue May 23, 2015
Added InvalidCsrfTokenException. Implements #6546
jadb added a commit that referenced this issue May 28, 2015
* origin/3.1: (305 commits)
  Update version number to 3.0.6
  New mock objects version causes our test suites to fail.
  Update FloatType.php
  Update DateTimeType.php
  Updated docBlock
  Fix empty query expressions for generating invalid SQL.
  Add tests for empty expression objects in association conditions.
  Add option to disable local XML file parsing.
  Port the Inflector fixes from #6635 to 3.0
  Fix typo in error template file name.
  Add test for ProgressHelper output with options.
  Add missing doc blcoks.
  Fix getOriginal() not preserving nulls.
  Fix incorrect doc blocks and PHPCS.
  Fixed @SInCE in QueryExpressionTest
  Added __call() on QueryExpression to allow for and() and or() to be called transparently. Implements #6477
  Added default message for InvalidCsrfTokenException Updated thrown exception messages to be more descriptive of the cause
  replace DS with DIRECTORY_SEPARATOR in filesystem sub-package
  Added InvalidCsrfTokenException. Implements #6546
  Use ProgressHelper in i18n task.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants