-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
fix: ensure csrf token is string #9365
base: develop
Are you sure you want to change the base?
Conversation
The same check should be made for |
Look at this: private function getPostedToken(RequestInterface $request): ?string
{
assert($request instanceof IncomingRequest);
// Does the token exist in POST, HEADER or optionally php:://input - json data or PUT, DELETE, PATCH - raw data.
if ($tokenValue = $request->getPost($this->config->tokenName)) {
return is_string($tokenValue) ? $tokenValue : null;
}
if ($request->hasHeader($this->config->headerName)
&& $request->header($this->config->headerName)->getValue() !== ''
&& $request->header($this->config->headerName)->getValue() !== []) {
return $request->header($this->config->headerName)->getValue();
}
$body = (string) $request->getBody();
if ($body !== '') {
$json = json_decode($body);
if ($json !== null && json_last_error() === JSON_ERROR_NONE) {
return (isset($json->{$this->config->tokenName}) && is_string($json->{$this->config->tokenName}))
? $json->{$this->config->tokenName}
: null;
}
parse_str($body, $parsed);
return (isset($parsed[$this->config->tokenName]) && is_string($parsed[$this->config->tokenName]))
? $parsed[$this->config->tokenName]
: null;
}
return null;
} |
246c279
to
504b62a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Please also handle the case where the CSRF token comes from the php://input
.
Your commits have to be signed:
- https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#gpg-signing-old-commits
- https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/signing.md
You can fix the code style with the command:
composer cs-fix
504b62a
to
4087e5b
Compare
4087e5b
to
3284730
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there - we also have to check the result of parse_str()
function.
And we also need tests for these two scenarios: php://input
and parse_str()
.
@datlechin Please run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a chengelog entry here: https://github.com/codeigniter4/CodeIgniter4/blob/develop/user_guide_src/source/changelogs/v4.5.8.rst under "Bugs Fixed".
You can see the previous changelogs as a reference.
You need to check here too, the array is unnecessary: if ($request->hasHeader($this->config->headerName)
&& $request->header($this->config->headerName)->getValue() !== ''
&& $request->header($this->config->headerName)->getValue() !== []) { if ($request->hasHeader($this->config->headerName)
&& is_string($request->header($this->config->headerName)->getValue())
&& $request->header($this->config->headerName)->getValue() !== '') {
|
@datlechin Please run |
Description
Make sure the CSRF token must be a string, because hackers can fake the request and pass csrf_main as an array, the system will break 500.
Checklist: