Merge pull request #968

Fixes #848

package.json

@@ -47,6 +47,7 @@
   },
   "dependencies": {
     "@braintree/sanitize-url": "^3.1.0",
+    "crypto-random-string": "^3.0.1",
     "d3": "^5.7.0",
     "dagre-d3-renderer": "^0.5.8",
     "dagre-layout": "^0.8.8",

src/diagrams/git/gitGraphAst.js

@@ -1,4 +1,5 @@
 import _ from 'lodash';
+import randomString from 'crypto-random-string';
 
 import { logger } from '../../logger';
 
@@ -9,17 +10,11 @@ let curBranch = 'master';
 let direction = 'LR';
 let seq = 0;
 
-function getRandomInt(min, max) {
-  return Math.floor(Math.random() * (max - min)) + min;
-}
-
 function getId() {
-  const pool = '0123456789abcdef';
-  let id = '';
-  for (let i = 0; i < 7; i++) {
-    id += pool[getRandomInt(0, 16)];
-  }
-  return id;
+  return randomString({
+    length: 7,
+    characters: '0123456789abcdef'
+  });
 }
 
 function isfastforwardable(currentCommit, otherCommit) {

src/diagrams/git/gitGraphParser.spec.js

@@ -1,11 +1,24 @@
 /* eslint-env jasmine */
 import gitGraphAst from './gitGraphAst';
 import { parser } from './parser/gitGraph';
+import randomString from 'crypto-random-string';
+import cryptoRandomString from 'crypto-random-string';
+
+jest.mock('crypto-random-string');
 
 describe('when parsing a gitGraph', function() {
+  let randomNumber;
   beforeEach(function() {
     parser.yy = gitGraphAst;
     parser.yy.clear();
+    randomNumber = 0;
+    cryptoRandomString.mockImplementation(() => {
+      randomNumber = randomNumber + 1;
+      return String(randomNumber);
+    });
+  });
+  afterEach(function() {
+    cryptoRandomString.mockReset();
   });
   it('should handle a gitGraph defintion', function() {
     const str = 'gitGraph:\n' + 'commit\n';
@@ -224,4 +237,51 @@ describe('when parsing a gitGraph', function() {
 
     parser.yy.prettyPrint();
   });
+
+  it('it should generate a secure random ID for commits', function() {
+    const str = 'gitGraph:\n' + 'commit\n' + 'commit\n';
+    const EXPECTED_LENGTH = 7;
+    const EXPECTED_CHARACTERS = '0123456789abcdef';
+
+    let idCount = 0;
+    randomString.mockImplementation(options => {
+      if (
+        options.length === EXPECTED_LENGTH &&
+        options.characters === EXPECTED_CHARACTERS &&
+        Object.keys(options).length === 2
+      ) {
+        const id = `abcdef${idCount}`;
+        idCount += 1;
+        return id;
+      }
+      return 'unexpected-ID';
+    });
+
+    parser.parse(str);
+    const commits = parser.yy.getCommits();
+
+    expect(Object.keys(commits)).toEqual(['abcdef0', 'abcdef1']);
+    Object.keys(commits).forEach(key => {
+      expect(commits[key].id).toEqual(key);
+    });
+  });
+
+  it('it should generate an array of known branches', function() {
+    const str =
+      'gitGraph:\n' +
+      'commit\n' +
+      'branch b1\n' +
+      'checkout b1\n' +
+      'commit\n' +
+      'commit\n' +
+      'branch b2\n';
+
+    parser.parse(str);
+    const branches = gitGraphAst.getBranchesAsObjArray();
+
+    expect(branches).toHaveLength(3);
+    expect(branches[0]).toHaveProperty('name', 'master');
+    expect(branches[1]).toHaveProperty('name', 'b1');
+    expect(branches[2]).toHaveProperty('name', 'b2');
+  });
 });