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

Prevent login warning on PHP 5.6+ #171

Merged
merged 2 commits into from Nov 12, 2015
Merged

Prevent login warning on PHP 5.6+ #171

merged 2 commits into from Nov 12, 2015

Conversation

ghost
Copy link

@ghost ghost commented Nov 12, 2015

Fix for ticket #1058

You might have to check my PR to make sure it's ok :P

@oldskool
Copy link
Contributor

According to the documentation, both arguments of hash_equals() should be strings, so both should be typecast to avoid the error from moving to the second argument. In other words, change it to:

return hash_equals((string)$a, (string)$b);

@ghost
Copy link
Author

ghost commented Nov 12, 2015

The second is the user password which will always be a string, even if nothing is entered, so that isn't a problem.

@oldskool
Copy link
Contributor

@Chris98 Well, it is currently. But it is an assumption to say it will always be that way. What if down the line we convert empty strings to null values somewhere? By typecasting the $b variable as well, you enforce it to be the proper type and avoid a similar bug to possibly appear in the future.

@ghost
Copy link
Author

ghost commented Nov 12, 2015

I don't really anticipate this changing anytime soon ;)

I'll leave this to the devs as to what they would like. Personally, I don't see any benefit from the additional cast.

Add additional cast
@ghost
Copy link
Author

ghost commented Nov 12, 2015

I've updated the PR and it's now casting both of them.

oldskool added a commit that referenced this pull request Nov 12, 2015
@oldskool oldskool merged commit 65a0199 into fluxbb:master Nov 12, 2015
@oldskool
Copy link
Contributor

@Chris98 Thanks, merged. This way we ensure that the input is always what the function expects it to be, so it gives more consistency.

@franzliedke
Copy link
Member

This has been merged into the wrong branch. ;)
master should be kept clean in case of urgent security releases needing to be made.

Before we merge any further PRs, I will clean up the branches.

@oldskool
Copy link
Contributor

@franzliedke Oh snap, sorry! 😭

@franzliedke
Copy link
Member

It's fine, though. If we're lucky, reverting is not necessary. ;)

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.

3 participants