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

Now accepting Headers #143

Closed
wants to merge 5 commits into from
Closed

Conversation

deployHuman
Copy link
Contributor

@deployHuman deployHuman commented Nov 4, 2021

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,

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()])) {
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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 😄

Copy link
Contributor Author

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]

Copy link
Contributor

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';
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@t0mmy742
Copy link
Contributor

t0mmy742 commented Nov 4, 2021

Getting token from custom header can work to prevent CSRF.
But we need to fix/add tests first 😄

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.

None yet

2 participants