Disallow usage of md5(), sha1(), crc32() and hash() with weak algorithms

Do we have existing instances that need replaced? How often do they appear if it warrants a rule?

The pipeline has failed due to a PHP linting error.

If you read the comments it was being discussed not for code review

I think this is a good idea for md5 and sha1, we should point users to sha256 or xxhash instead I guess? Not sure what to do about uniqid().

#3307718: Implement xxHash for non-cryptographic use-cases suggests replacing md5/sha1 with xxHash alternatives

Awesome to see this one come together. Replacements make sense and probably could spin up follow ups to do the replacements.

NW for merge conflict in phpstan baseline.

Thanks for working on this. Added a comment to the MR, as I think it is not a good idea to link to the Drupal 7 documentation. Also think we should add a CR for this (a similar from the past https://www.drupal.org/node/2833433). Moving to NW for this.

Feedback and CR look good

crc32 shouldn't use Crypt::hashBase64(), it should be replaced with xxHash, the other exclusions in the MR say this.

The errorTip links to the change record, but then the change record links back to the errorTip.

I am not sure we should duplicate documentation on e.g. https://xxhash.com/ but I think we probably need to say at least something like:

'Use a cryptographic hashing algorithm when appropriate. Use a fast, low collision, non-cryptographic algorithm when approprate (usually one of the xxHash variants), don't use weak cryptographic algorithms.'

Also while this will stop further cases being added to core, I'm a little bit concerned about committing it with the core usages not converted yet, because when we (hopefully) set a good example in core that's an easy place to follow and refer to.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

@catch changes look good?

Going on a limb and believe feedback for this one has been addressed.

+$ignoreErrors[] = [
+	'message' => '#^Calling uniqid\\(\\) is forbidden, use bin2hex\\(random_bytes\\(\\)\\) instead\\.$#',
+	'identifier' => 'disallowed.function',
+	'count' => 1,
+	'path' => __DIR__ . '/tests/Drupal/FunctionalTests/Core/Recipe/RecipeCommandTest.php',
+];

This is in RecipeTestTrait, so any new recipe test will need this adding to the baseline.

Wondering if we should allow uniqid (or any of these functions) in tests? Or if we should just fix this case here?

https://git.drupalcode.org/project/drupal/-/merge_requests/15081/diffs?c... appears to have fixed #35 so going to mark it again, fingers crossed

Discussed this with @alexpott in slack.

The various hash algorithms switching to xxHash or an actual cryptographic hash when needed seems fine.

However:

uniqid\\(\\) is forbidden, use bin2hex\\(random_bytes\\(\\)\\)

This is not a straight replacement and requires thinking about what to send to random_bytes() etc., we also use uniqid in some places where the potential for collisions is really not an issue like creating a directory name for almost one-off operations.

Alex also pointed out that https://wiki.php.net/rfc/deprecate-uniqid is stalled. If it was unstalled, we wouldn't need this at all because it'd be a PHP deprecation, but with it stalled it's hard to tell what any eventual direction might be.

So I think we should split uniqid() out to its own issue and only cover hash algorithms here.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

@mondrake given #38 what are your thoughts on undoing the change to RecipeTestTrait? I don't know if it matters or not.

Understandable. To be honest, I felt kind of bad asking about that after all of the changes and updates this has been through.

The changes related to uniqid() have been removed. This can go back to RTBC.

I added this to the CR:

For cryptographic hashing, see the <a href="https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Utility%21Crypt.php/class/Crypt/main">Crypt</a> class for common use-cases.

Hopefully no-one is still using md5() or sha1() for cryptographic purposes but just in case.

Committed/pushed to main, thanks! This doesn't cherry-pick to 11.x and not sure we need to necessarily, it'll prevent new core usages in all branches.

Tagging for follow-ups to remove the baseline entries we're adding here.

Followup is available, see #3609780: Replace usage of md5(), sha1(), crc32() and hash() with weak algorithms