Strengthen the hashing to prevent hash collisions.

As a follow-up to #2844852: Minify the cache tags sent in the header, hash collisions happen.

#2844852-11: Minify the cache tags sent in the header shows this example:
node_list and node:287766
Both have a hash of ef74.

Proposal is to convert the base from 16 to 64, and use base64url encoding.
Currently 4 characters base 16 is used = 65536 unique hashes.
If we switch to 4 characters base 64 = 16777216 unique hashes.

I have grabbed some base conversion code from the internet and drafted the code here to validate my assumptions.

Here's the test: http://sandbox.onlinephpfunctions.com/code/ad1431ad58d0e740fa7e10e1259d1...

Comments

MiroslavBanov created an issue. See original summary.

miroslavbanov’s picture

Issue summary: View changes

Removed redundant description.

Some materials on base conversion:
http://elenzil.com/esoterica/baseConversion.html
https://stackoverflow.com/a/4964352

miroslavbanov’s picture

StatusFileSize
new854 bytes

Adding a patch with only base32 encoding.

Unfortunately I believe that Acquia VCL lowercases the string so base64 probably isn't going to collide less until that is changed.

miroslavbanov’s picture

Status: Active » Needs review
StatusFileSize
new1.05 KB
new1.14 KB

Adding the base64 version.

Like I said above, unfortunately I believe that Acquia VCL lowercases the string so base64 probably isn't going to collide less than base32 until that is changed.

nielsvm’s picture

Status: Needs review » Fixed

Thank you for your amazingly helpful contribution MiroslavBanov, I've tested it extensively. You're correct on assuming that we lowercase the strings here at Acquia so we're indeed going for the B32 version as we're reluctant about making our server-side implementation case sensitive (and we're not sure that's possible/safe).

This got included in the recent release, thanks everyone!

https://www.drupal.org/project/acquia_purge/releases/8.x-1.0

josh waihi’s picture

This breaks invalidation of existing cached pages right? So Varnish will need to be purged with the release of 8.x-1.0?

nielsvm’s picture

This breaks invalidation of existing cached pages right? So Varnish will need to be purged with the release of 8.x-1.0

Hey Josh - Yes, and this is in the release notes :-).

This was one of the changes that I only wanted to make between beta-releases, so going forward the idea is that new releases don't break existing installations in the way the 1.0 does. Worst case we could write update-hooks in the future, although those aren't perfect either.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.