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

Passwords is incompatible with PHP 7.4 due to PHP BC break #35

Closed
JanTvrdik opened this issue Jul 1, 2019 · 7 comments
Closed

Passwords is incompatible with PHP 7.4 due to PHP BC break #35

JanTvrdik opened this issue Jul 1, 2019 · 7 comments

Comments

@JanTvrdik
Copy link
Contributor

@JanTvrdik JanTvrdik commented Jul 1, 2019

Password hashing algorithm identifiers are now nullable strings rather than integers.

  • PASSWORD_DEFAULT was int 1; now is null
  • PASSWORD_BCRYPT was int 1; now is string '2y'
  • PASSWORD_ARGON2I was int 2; now is string 'argon2i'
  • PASSWORD_ARGON2ID was int 3; now is string 'argon2id'

https://github.com/php/php-src/blob/39a0854307cd63685b2fe158207727a2b8c06cd7/UPGRADING#L120-L130

@JanTvrdik JanTvrdik changed the title Password is incompatible with PHP 7.4 due to PHP BC break Passwords is incompatible with PHP 7.4 due to PHP BC break Jul 1, 2019
@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jul 1, 2019

I would wait if it does not change.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor Author

@JanTvrdik JanTvrdik commented Jul 1, 2019

It has passed through RFC process (https://wiki.php.net/rfc/password_registry) and the BC break was explicitly mentioned so I think that change is unlikely.

(Edit: but I don't need this right now, so feel free to wait)

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jul 1, 2019

I think that this type of BC break was not mentioned.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor Author

@JanTvrdik JanTvrdik commented Jul 2, 2019

It's explicitly mentioned in the Backward Incompatible Changes section of the RFC:

Algorithm identifiers are now (nullable) strings rather than numbers.

It's further discussed in Minimizing impact to BC section of the RFC.

In order to minimize the impact of the above BC. we could overload the password_hash() and password_needs_rehash() methods to accept integer values 0, 1, 2, and 3 to function as aliases for DEFAULT, BCRYPT, ARGIN2I, and ARGON2ID, respectively. Using an int would therefore work, but would produce a deprecation warning.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jul 2, 2019

But this is not related to this issue, or is it?

@JanTvrdik

This comment has been minimized.

Copy link
Contributor Author

@JanTvrdik JanTvrdik commented Jul 2, 2019

I don't understand the question, so instead I sent PR which shows the issue,

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jul 2, 2019

I'm trying to say that in Backward Incompatible Changes section they didn't mention the case when a BC break arises because of the typehint is used in the userland code.

Applications correctly using the constants such as PASSWORD_DEFAULT, PASSWORD_BCRYPT, etc… will continue to function correctly.

Nette\Security\Passwords correctly uses these constants but will not continue to work correctly.

ie. that the section is not exhaustive (Unlike for example are Nikita's RFC.)

So it is possible that over time it will appear that it breaks more code, and they change it (which is unlikely…).

@dg dg closed this in 6fd9f86 Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.