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

[ticket/15723] Rewrite gen_rand_string() and gen_rand_string_friendly() #5292

Merged
merged 2 commits into from Sep 17, 2018

Conversation

rubencm
Copy link
Member

@rubencm rubencm commented Jul 14, 2018

PHPBB3-15723

Checklist:

  • Correct branch: master for new features; 3.2.x for fixes
  • Tests pass
  • Code follows coding guidelines: master and 3.2.x
  • Commit follows commit message format

Tracker ticket (set the ticket ID to your ticket ID):

https://tracker.phpbb.com/browse/PHPBB3-15723

@rubencm
Copy link
Member Author

rubencm commented Aug 8, 2018

unique_id returned 16 characters before, i think it should return 32 better.
In some places like salt, if you update your installation you will get a 16 characters salt, but if you make a clean installation, you will get a 32 characters salt.
https://github.com/phpbb/phpbb/blob/master/phpBB/phpbb/install/module/install_database/task/add_config_settings.php#L233
https://github.com/phpbb/phpbb/blob/master/phpBB/phpbb/db/migration/data/v310/plupload.php#L33

@@ -88,21 +97,25 @@ function gen_rand_string($num_chars = 8)
*/
function gen_rand_string_friendly($num_chars = 8)
{
$rand_str = bin2hex(random_bytes($num_chars + 1));
$range = array_merge(range('A', 'N'), range('P', 'Z'), range(1, 9));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should add [a-z] too I think

@@ -73,8 +73,17 @@ function set_var(&$result, $var, $type, $multibyte = false)
*/
function gen_rand_string($num_chars = 8)
{
// [a, z] + [0, 9] = 36
return substr(strtoupper(base_convert(bin2hex(random_bytes($num_chars + 1)), 16, 36)), 0, $num_chars);
$range = array_merge(range('A', 'Z'), range(0, 9));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should add [a-z] too I think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, is the same characters, in the old version of the function, all characters were converted to uppercase

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@Nicofuma Nicofuma added this to the 3.2.4 milestone Sep 14, 2018
marc1706 added a commit to marc1706/phpbb that referenced this pull request Sep 17, 2018
[ticket/15723] Rewrite gen_rand_string() and gen_rand_string_friendly()
@marc1706 marc1706 merged commit ced8599 into phpbb:3.2.x Sep 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants