Skip to content

Commit

Permalink
resourceloader: Replace SHA1 with 32-bit FNV-1 as hash function
Browse files Browse the repository at this point in the history
SHA-1 is not secure enough to be used as a cryptographic hash function, and its
implementation in JavaScript is too long and too slow for it to be a good
general-purpose hash function. And we currently throw away most of the work:
SHA-1 produces 160-bit hash values, of which we keep 48.

Although the JavaScript implementation is not exported, SHA-1 is a well-known
hash function, and I'm willing to bet that sooner or later someone will move to
make it accessible to other modules, at which point usage will start to spread.

For ResourceLoader, the qualities we're looking for in a hash function are:

* Already implemented in PHP
* Easy to implement in JavaScript
* Fast
* Collision-resistant

The requirement that hashes be cheap to compute in JavaScript narrows the field
to 32-bit hash functions, because in JavaScript bitwise operators treat their
operands as 32 bits, and arithmetic uses double-precision floats, which have a
total precision of 53 bits. It's possible to work around these limitations, but
it's a lot of extra work.

The best match I found is the 32-bit variant of FNV-1, which is available in
PHP as of version 5.4 (as 'fnv1a32'). The fnv132 JavaScript function is
around ten times faster and eight times shorter than sha1.

Change-Id: I1e4fb08d17948538d96f241b2464d594fdc14578
  • Loading branch information
atdt authored and Krinkle committed Jul 5, 2016
1 parent 78c1c6d commit dfd0464
Show file tree
Hide file tree
Showing 6 changed files with 35 additions and 159 deletions.
6 changes: 2 additions & 4 deletions includes/resourceloader/ResourceLoader.php
Expand Up @@ -601,10 +601,8 @@ public function getLoadScript( $source ) {
* @return string Hash
*/
public static function makeHash( $value ) {
// Use base64 to output more entropy in a more compact string (default hex is only base16).
// The first 8 chars of a base64 encoded digest represent the same binary as
// the first 12 chars of a hex encoded digest.
return substr( base64_encode( sha1( $value, true ) ), 0, 8 );
$hash = hash( 'fnv132', $value );
return Wikimedia\base_convert( $hash, 16, 36, 7 );
}

/**
Expand Down
1 change: 0 additions & 1 deletion resources/Resources.php
Expand Up @@ -841,7 +841,6 @@
'class' => 'ResourceLoaderRawFileModule',
// Keep in sync with maintenance/jsduck/eg-iframe.html
'scripts' => [
'resources/lib/phpjs-sha1/sha1.js',
'resources/src/mediawiki/mediawiki.js',
'resources/src/mediawiki/mediawiki.requestIdleCallback.js',
'resources/src/mediawiki/mediawiki.errorLogger.js',
Expand Down
147 changes: 0 additions & 147 deletions resources/lib/phpjs-sha1/sha1.js

This file was deleted.

36 changes: 31 additions & 5 deletions resources/src/mediawiki/mediawiki.js
Expand Up @@ -8,7 +8,6 @@
* @singleton
*/
/*jshint latedef:false */
/*global sha1 */
( function ( $ ) {
'use strict';

Expand All @@ -19,6 +18,36 @@
trackHandlers = [],
trackQueue = [];

/**
* FNV132 hash function
*
* This function implements the 32-bit version of FNV-1.
* It is equivalent to hash( 'fnv132', ... ) in PHP, except
* its output is base 36 rather than hex.
* See <https://en.wikipedia.org/wiki/FNV_hash_function>
*
* @private
* @param {string} str String to hash
* @return {string} hash as an seven-character base 36 string
*/
function fnv132( str ) {
/*jshint bitwise:false */
var hash = 0x811C9DC5,
i;

for ( i = 0; i < str.length; i++ ) {
hash += ( hash << 1 ) + ( hash << 4 ) + ( hash << 7 ) + ( hash << 8 ) + ( hash << 24 );
hash ^= str.charCodeAt( i );
}

hash = ( hash >>> 0 ).toString( 36 );
while ( hash.length < 7 ) {
hash = '0' + hash;
}

return hash;
}

/**
* Create an object that can be read from or written to from methods that allow
* interaction both with single and multiple properties at once.
Expand Down Expand Up @@ -931,10 +960,7 @@
var hashes = $.map( modules, function ( module ) {
return registry[ module ].version;
} );
// Trim for consistency with server-side ResourceLoader::makeHash. It also helps
// save precious space in the limited query string. Otherwise modules are more
// likely to require multiple HTTP requests.
return sha1( hashes.join( '' ) ).slice( 0, 12 );
return fnv132( hashes.join( '' ) );
}

/**
Expand Down
Expand Up @@ -5,7 +5,7 @@ class ResourceLoaderStartUpModuleTest extends ResourceLoaderTestCase {
// Version hash for a blank file module.
// Result of ResourceLoader::makeHash(), ResourceLoaderTestModule
// and ResourceLoaderFileModule::getDefinitionSummary().
protected static $blankVersion = 'GqV9IPpY';
protected static $blankVersion = '0a56zyi';

protected static function expandPlaceholders( $text ) {
return strtr( $text, [
Expand Down
2 changes: 1 addition & 1 deletion tests/phpunit/structure/ResourcesTest.php
Expand Up @@ -40,7 +40,7 @@ public function testVersionHash() {
$data = self::getAllModules();
foreach ( $data['modules'] as $moduleName => $module ) {
$version = $module->getVersionHash( $data['context'] );
$this->assertEquals( 8, strlen( $version ), "$moduleName must use ResourceLoader::makeHash" );
$this->assertEquals( 7, strlen( $version ), "$moduleName must use ResourceLoader::makeHash" );
}
}

Expand Down

0 comments on commit dfd0464

Please sign in to comment.