-
Notifications
You must be signed in to change notification settings - Fork 59
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
Now accepting Headers #143
Conversation
src/Guard.php
Outdated
$name = null; | ||
$value = null; | ||
|
||
if (is_array($body)) { | ||
$name = $body[$this->getTokenNameKey()] ?? null; | ||
$value = $body[$this->getTokenValueKey()] ?? null; | ||
} | ||
|
||
if (($name == null || $value == null) && isset($headers[$this->getTokenNameKey()], $headers[$this->getTokenValueKey()])) { |
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.
You do not need isset
here. Next lines already checking token existence.
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.
agree, that was redundant, how about the new commit i did?
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.
You do NOT need to check if $header is an array since ServerRequestInterface::getHeaders()
already return string[][]
. If no header, it will be an empty string
.
Here, you just need to check $name == null || $value == null
😄
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.
somehow this needs to be checked i think, cuz its a nested array, both headers and headers[tokennamekey][0]
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.
If $headers
is not an array, it means you are using a bad PSR-7 provider. This package needs a compliant PSR-7 provider.
Following are equivalent:
$name = $headers[$this->getTokenNameKey()][0] ?? null;
and
$name = null;
if (isset($headers[$this->getTokenNameKey()]) && isset($headers[$this->getTokenNameKey()][0])) {
$name = $headers[$this->getTokenNameKey()][0];
}
@@ -258,15 +258,15 @@ public function getTokenValue(): ?string | |||
*/ | |||
public function getTokenNameKey(): string | |||
{ | |||
return $this->prefix . '_name'; | |||
return $this->prefix . '-name'; |
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.
I understand why, but I'm no sure if it is a breaking change
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.
was thinking about it too, but should not be for the framework itself, only if someone have hardcoded reference checks or something similar.
As i did first -.- haha.
As tokens to expire and on failure it generates a new one, so in theory it should work still with update mid-production.
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.
I just checked README file. We do not talk about hardcoded value, only getting these values and names from public methods Guard::getTokenNameKey()
and Guard::getTokenValueKey()
.
It should not be a breaking change, if users are not using hardcoded values.
Getting token from custom header can work to prevent CSRF. |
So, How about a PR just for now acception the key pair in the form of Headers as a option to postdata ?
the suffix cant contain underscore when in headers apparently, so had to change that too.
Hope this is more correct.
And i know, no phpunit testing,