Skip to content

Conversation

@xabbuh
Copy link
Member

@xabbuh xabbuh commented Jul 28, 2024

Q A
Branch? 7.2
Bug fix? no
New feature? no
Deprecations? no
Issues Fix #57588
License MIT

@xabbuh xabbuh requested a review from OskarStark as a code owner July 28, 2024 11:20
@carsonbot carsonbot added this to the 7.2 milestone Jul 28, 2024
@OskarStark OskarStark changed the title replace uniqid() with random_bytes() to create identifiers replace uniqid() with random_bytes() to create identifiers Jul 29, 2024
@fabpot
Copy link
Member

fabpot commented Jul 29, 2024

Thank you @xabbuh.

@fabpot fabpot merged commit 7368685 into symfony:7.2 Jul 29, 2024
@xabbuh xabbuh deleted the issue-57588 branch July 29, 2024 08:09
try {
if ($delayInMs > 0) { // the delay is <= 0 for queued messages
$id = uniqid('', true);
$id = bin2hex(random_bytes(4));
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that random_bytes(4) only has 32 bits of entropy, which is less than the previous call to uniqid() with $more_entropy set to true. This should use at least random_bytes(8) for 64 bits of entropy and ideally random_bytes(16) for 128 bits (the latter would result in a slightly longer output of 32 characters instead of 23 for uniqid).

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe we should use the initial suggestion from the issue and use a UUID instead (see #57872)

nicolas-grekas added a commit that referenced this pull request Aug 12, 2024
…() (xabbuh)

This PR was merged into the 7.2 branch.

Discussion
----------

[Messenger][Notifier] use more entropy with random_bytes()

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | #57856 (comment)
| License       | MIT

Commits
-------

8152a02 use more entropy with random_bytes()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ban uniqid() from codebase

5 participants