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

Handle X-CSRF-TOKEN - CSRF #2272

Merged
merged 4 commits into from
Sep 28, 2019
Merged

Conversation

nowackipawel
Copy link
Contributor

@nowackipawel nowackipawel commented Sep 27, 2019

According to your comments, I have to agree: it could be useful and should be characterized by better performance than parsing php://input jsons everytime when $_POST token is not set.

@michalsn @jim-parry @lonnieezell

Order:

  1. $_POST
  2. HTTP HEADER
  3. php://input - trying to parse posted JSON (last because of performance)

Order:

1. $_POST
2. HTTP HEADER
3. php://input - trying to parse posted JSON  (last because of performance)
@michalsn
Copy link
Member

@nowackipawel you should add:

public $CSRFHeaderName  = 'X-CSRF-TOKEN';

to the tests/_support/Config/MockAppConfig.php file to make tests pass again.

@michalsn
Copy link
Member

I think that we should add a few tests to this (to cover handling CSRF by header and json) - but I like it.

@nowackipawel
Copy link
Contributor Author

nowackipawel commented Sep 27, 2019

@michalsn : yes, I agree... and as long as I tested php://input and I am using this in my APP with _POST simultaneusly ... and as long as I tested HEADER version with some test requests ... I am so sorry but I won't be able to write tests for this PR :(.

The same is with:
#2067

@michalsn
Copy link
Member

@nowackipawel it's fine. I can probably work on it later today - after work.

@michalsn michalsn mentioned this pull request Sep 27, 2019
5 tasks
Good idea to use built-in functions  ;-)

Co-Authored-By: Michal Sniatala <michal@sniatala.pl>
@MGatner
Copy link
Member

MGatner commented Sep 28, 2019

This is out of my knowledge but you two seem to know what you’re talking about. @michalsn is this ready to go?

@michalsn
Copy link
Member

@MGatner Yes, it is.

@MGatner MGatner merged commit 3aa4a59 into codeigniter4:develop Sep 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants