yomorun / hashids-java

Hashids algorithm v1.0.0 implementation in Java
http://hashids.org
MIT License
1.02k stars 155 forks source link

different salt can give random number or crash #20

Closed chrisport closed 8 years ago

chrisport commented 9 years ago

Hi, When using different salt it should return empty array. But I recognized the behavior is quite unpredictable, there is a probability that it decodes to a wrong number instead or crashes with an ArrayIndexOutOfBoundsException.

Here and example that will decode to a wrong number:

    @Test
    public void encodeAndDecodeTokenWithWrongKey() {
        Hashids a = new Hashids("supersecret",4);
        Hashids b = new Hashids("differentsupersecret",4);

        long sampleId = 123l;
        String token = a.encode(sampleId);

        long[] ids = b.decode(token); // []long id = { 81143 }
        assertEquals(0, ids.length);
    }

assertion will fail, id[0] will be 81143

Same test with sampleId = 37l will cause "ArrayIndexOutOfBoundsException"

fanweixiao commented 9 years ago

We should not expect assertEquals(0, ids.length) will be success when using different salts.

fanweixiao commented 9 years ago

But crash should be avoid, should return empty array

chrisport commented 9 years ago

Okay good.

Also in case somebody needs: duplicating the id will decrease the probability of receiving an id from an invalid token:

String token = hashids.encode(id, id);

long[] ids = hashids.decode(token);
if (ids.length == 2 && ids[0] == ids[1]) {
   return ids[0];
}else{
   // not a valid token
} 
0x3333 commented 8 years ago

ArrayIndexOutOfBoundsException crash has been fixed in PR #27 .

The collision behavior is the same as the default javascript implementation, it is just a collision within different salts. Can be less possible increasing the minHashLength.

Collisions is not possible within the same salt, but perfectly possible in a small sample space as 4 lenght hash.

[A-Z,a-z,0-9] 62^4 = almost 14 million unique values, not that much [A-Z,a-z,0-9] 62^6 = almost 56 billion unique values, much better

As long as I can tell, this issue can be closed, as the crash has been fixed and the collision is not a bug.