Skip to content

Commit 45b850b

Browse files
kimadelineKim-Adeline Miguel
kimadeline
authored and
Kim-Adeline Miguel
committed
Fix experiments hash parsing (microsoft#6492)
* Simplify & fix crypto.createHash * Add tests * Fix python test in news folder (microsoft#6398) * Fix broken python tests in news * Revert changes
1 parent eae03ed commit 45b850b

5 files changed

Lines changed: 37 additions & 13 deletions

File tree

‎src/client/common/crypto.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
// tslint:disable: no-any
66

7-
import { createHash, HexBase64Latin1Encoding } from 'crypto';
7+
import { createHash } from 'crypto';
88
import { injectable } from 'inversify';
99
import { ICryptoUtils, IHashFormat } from './types';
1010

@@ -13,8 +13,8 @@ import { ICryptoUtils, IHashFormat } from './types';
1313
*/
1414
@injectable()
1515
export class CryptoUtils implements ICryptoUtils {
16-
public createHash<E extends keyof IHashFormat>(data: string, encoding: HexBase64Latin1Encoding, hashFormat: E): IHashFormat[E] {
17-
const hash = createHash('sha512').update(data).digest(encoding);
18-
return hashFormat === 'number' ? parseInt(hash, undefined) : hash as any;
16+
public createHash<E extends keyof IHashFormat>(data: string, hashFormat: E): IHashFormat[E] {
17+
const hash = createHash('sha512').update(data).digest('hex');
18+
return hashFormat === 'number' ? parseInt(hash, 16) : hash as any;
1919
}
2020
}

‎src/client/common/experiments.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ export class ExperimentsManager implements IExperimentsManager {
148148
if (typeof (this.appEnvironment.machineId) !== 'string') {
149149
throw new Error('Machine ID should be a string');
150150
}
151-
const hash = this.crypto.createHash(`${this.appEnvironment.machineId}+${salt}`, 'hex', 'number');
151+
const hash = this.crypto.createHash(`${this.appEnvironment.machineId}+${salt}`, 'number');
152152
return hash % 100 >= min && hash % 100 < max;
153153
}
154154

‎src/client/common/types.ts‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
// Licensed under the MIT License.
33
'use strict';
44

5-
import { HexBase64Latin1Encoding } from 'crypto';
65
import { Socket } from 'net';
76
import { Request as RequestResult } from 'request';
87
import { ConfigurationTarget, DiagnosticSeverity, Disposable, DocumentSymbolProvider, Event, Extension, ExtensionContext, OutputChannel, Uri, WorkspaceEdit } from 'vscode';
@@ -485,10 +484,9 @@ export interface ICryptoUtils {
485484
* Creates hash using the data and encoding specified
486485
* @returns hash as number, or string
487486
* @param data The string to hash
488-
* @param encoding Data encoding to use
489487
* @param hashFormat Return format of the hash, number or string
490488
*/
491-
createHash<E extends keyof IHashFormat>(data: string, encoding: HexBase64Latin1Encoding, hashFormat: E): IHashFormat[E];
489+
createHash<E extends keyof IHashFormat>(data: string, hashFormat: E): IHashFormat[E];
492490
}
493491

494492
export const IAsyncDisposableRegistry = Symbol('IAsyncDisposableRegistry');

‎src/test/common/crypto.unit.test.ts‎

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,37 @@ suite('Crypto Utils', async () => {
1212
crypto = new CryptoUtils();
1313
});
1414
test('If hashFormat equals `number`, method createHash() returns a number', async () => {
15-
const hash = crypto.createHash('blabla', 'hex', 'number');
15+
const hash = crypto.createHash('blabla', 'number');
1616
assert.typeOf(hash, 'number', 'Type should be a number');
1717
});
1818
test('If hashFormat equals `string`, method createHash() returns a string', async () => {
19-
const hash = crypto.createHash('blabla', 'hex', 'string');
19+
const hash = crypto.createHash('blabla', 'string');
2020
assert.typeOf(hash, 'string', 'Type should be a string');
2121
});
22+
test('If hashFormat equals `number`, the hash should not be NaN', async () => {
23+
let hash = crypto.createHash('test', 'number');
24+
assert.isNotNaN(hash, 'Number hash should not be NaN');
25+
hash = crypto.createHash('hash', 'number');
26+
assert.isNotNaN(hash, 'Number hash should not be NaN');
27+
hash = crypto.createHash('HASH1', 'number');
28+
assert.isNotNaN(hash, 'Number hash should not be NaN');
29+
});
30+
test('If hashFormat equals `string`, the hash should not be undefined', async () => {
31+
let hash = crypto.createHash('test', 'string');
32+
assert.isDefined(hash, 'String hash should not be undefined');
33+
hash = crypto.createHash('hash', 'string');
34+
assert.isDefined(hash, 'String hash should not be undefined');
35+
hash = crypto.createHash('HASH1', 'string');
36+
assert.isDefined(hash, 'String hash should not be undefined');
37+
});
38+
test('If hashFormat equals `number`, hashes with different data should return different number hashes', async () => {
39+
const hash1 = crypto.createHash('hash1', 'number');
40+
const hash2 = crypto.createHash('hash2', 'number');
41+
assert.notEqual(hash1, hash2, 'Hashes should be different numbers');
42+
});
43+
test('If hashFormat equals `string`, hashes with different data should return different string hashes', async () => {
44+
const hash1 = crypto.createHash('hash1', 'string');
45+
const hash2 = crypto.createHash('hash2', 'string');
46+
assert.notEqual(hash1, hash2, 'Hashes should be different strings');
47+
});
2248
});

‎src/test/common/experiments.unit.test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -599,10 +599,10 @@ suite('A/B experiments', () => {
599599
expect(() => expManager.isUserInRange(79, 94, 'salt')).to.throw();
600600
} else if (testParams.error) {
601601
const error = new Error('Kaboom');
602-
when(crypto.createHash(anything(), 'hex', 'number')).thenThrow(error);
602+
when(crypto.createHash(anything(), 'number')).thenThrow(error);
603603
expect(() => expManager.isUserInRange(79, 94, 'salt')).to.throw(error);
604604
} else {
605-
when(crypto.createHash(anything(), 'hex', 'number')).thenReturn(testParams.hash);
605+
when(crypto.createHash(anything(), 'number')).thenReturn(testParams.hash);
606606
expect(expManager.isUserInRange(79, 94, 'salt')).to.equal(testParams.expectedResult, 'Incorrectly identified');
607607
}
608608
});
@@ -636,7 +636,7 @@ suite('A/B experiments', () => {
636636
.returns(() => testParams.experimentStorageValue);
637637
when(appEnvironment.machineId).thenReturn('101');
638638
if (testParams.hash) {
639-
when(crypto.createHash(anything(), 'hex', 'number')).thenReturn(testParams.hash);
639+
when(crypto.createHash(anything(), 'number')).thenReturn(testParams.hash);
640640
}
641641
expManager.populateUserExperiments();
642642
assert.deepEqual(expManager.userExperiments, testParams.expectedResult);

0 commit comments

Comments
 (0)