Problem/Motivation

#3573259: Prevent new expects($this->any()) in tests introduced spaze/phpstan-disallowed-calls as a dev dependency.

We can now leverage on the extension to disallow usage of uniqid(), md5(), sha1().

Proposed resolution

do it

Remaining tasks

decide if we want to do this

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3574204

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Related issues: +#2972100: Remove usage of uniqid
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Postponed » Active

Parent was committed, this is now actionable.

mondrake changed the visibility of the branch 3574204-disallow-usage-of to hidden.

mondrake opened merge request !15081

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review

Is this worth doing?

smustgrave’s picture

smustgrave commented

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

mondrake’s picture

mondrake
Location 🇮🇹
commented
Component: phpunit » base system

#7 the raw output of the PHPStan job tells where the functions are used. The error could be baselined in the MR, and this would then prevent more usage once committed.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Reference for md5 and sha1 ban: https://www.drupal.org/docs/7/security/writing-secure-code-0/use-of-hash...

sivaji_ganesh_jojodae’s picture

sivaji_ganesh_jojodae commented
Status: Needs review » Needs work

The pipeline has failed due to a PHP linting error.

smustgrave’s picture

smustgrave commented

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

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
Status: Needs work » Needs review

Yes please, let’s first discuss if this is ok to do.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

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().

mondrake’s picture

mondrake
Location 🇮🇹
commented

#13 one of these maybe? #2972100-24: Remove usage of uniqid

There was some consensus on crypt::randomBytesBase64(16) afaics

idebr’s picture

idebr commented

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

mondrake’s picture

mondrake
Location 🇮🇹
commented

Reflected latest comments in the MR - it now redirects sha1() and md5() to use sha256() or hash() with xxHash algo, and uniqid() to use Crypt:: randomBytesBase64().

Rabased and now added errors to the baseline.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

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

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs work

NW for merge conflict in phpstan baseline.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Reviewed & tested by the community

Merged with main and resolved a conflict.

poker10’s picture

poker10 commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

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.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Added draft CR https://www.drupal.org/node/3581605

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Disallow usage of uniqid(), md5(), sha1() » Disallow usage of uniqid(), md5(), sha1(), crc32() and hash() with weak algorithms
Issue tags: -Needs change record
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

Addressed @poker10 review, added a draft CR, added more disallows based on what I read. Thanks!

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Feedback and CR look good

catch’s picture

catch
he/him
Primary language English
commented

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.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Postponed
Related issues: +#3581442: Replace usage of uniqid() in the Database system

Maybe we can do #3581442: Replace usage of uniqid() in the Database system first, then.