growthbook / growthbook-swift

GrowthBook iOS (Swift) SDK
MIT License
13 stars 19 forks source link

Hashing algorithm changed without mentioning and caused Multiple Exposures #75

Closed levochkaa closed 2 months ago

levochkaa commented 2 months ago

We have updated GrowthBook SDK from 1.0.48 to 1.0.61, because of this fix #74 But got another problem: qWstmCxY

After some investigation, we've found, that:

I've made a quick test to check if the hashing really changed:

import Foundation

enum Utils {
    static func oldHash(seed: String, value: String, version: Float) -> Float? {
        switch version {
        case 2:
            // New unbiased hashing algorithm
            let combinedValue = seed + value
            let hashedValue = digest(combinedValue + "")
            return Float(hashedValue % 10000) / 10000

        case 1:
            let combinedValue = value + seed
            let hashedValue = digest(combinedValue + "")
            return Float(hashedValue % 1000) / 1000

        default:
            // Unknown hash version
            return nil
        }
    }

    static func newHash(seed: String, value: String, version: Float) -> Float? {
        switch version {
        case 2:
            // New unbiased hashing algorithm
            let combinedValue = seed + value
            let hashedCombinedValue = digest(combinedValue).description + ""
            let hashedValue = digest(hashedCombinedValue) % 10000
            return Float(hashedValue) / 10000

        case 1:
            let combinedValue = value + seed
            let hashedValue = digest(combinedValue)
            return Float(hashedValue % 1000) / 1000

        default:
            // Unknown hash version
            return nil
        }
    }
}

enum Common {
    static let offsetBasis32: UInt32 = 2_166_136_261
    static let prime32: UInt32 = 16_777_619

    static func fnv1a<T: FixedWidthInteger>(_ array: [UInt8], offsetBasis: T, prime: T) -> T {
        var hash: T = offsetBasis

        for elm in array {
            hash = hash ^ T(elm)
            hash = hash &* prime
        }

        return hash
    }
}

func digest(_ string: String) -> UInt32 {
    Common.fnv1a(Array(string.utf8), offsetBasis: Common.offsetBasis32, prime: Common.prime32)
}

let seed = "seed"
let value = "value"

print(
    "version 1 -",
    "old: \(Utils.oldHash(seed: seed, value: value, version: 1) ?? 0)",
    "new: \(Utils.newHash(seed: seed, value: value, version: 1) ?? 0)"
)

print(
    "version 2 -",
    "old: \(Utils.oldHash(seed: seed, value: value, version: 2) ?? 0)",
    "new: \(Utils.newHash(seed: seed, value: value, version: 2) ?? 0)"
)

The output is:

version 1 - old: 0.579 new: 0.579
version 2 - old: 0.1733 new: 0.9213

Yes, hashing changed.

vazarkevych commented 2 months ago

Hi, @levochkaa. Thank you for providing the detailed information. We will review it and get back to you.

vazarkevych commented 2 months ago

We just checked the hash in different versions of the latest Growthbook SDKs and received the same result as in Swift SDK. So it seems to be working as expected. Did you try another latest SDK, and did you get a different result?

levochkaa commented 2 months ago

Did you try another latest SDK

The hashing didn't change after 1.0.49 release, as I see in git commits history, so I am comparing 1.0.48 (oldHash) vs 1.0.49 (newHash)

We just checked the hash in different versions of the latest Growthbook SDKs and received the same result as in Swift SDK.

Can you share what versions you were comparing, how, and what are the results?

vazarkevych commented 2 months ago

Hi, @levochkaa. Sure, here is example of running that using the Java SDK. JavaSDK

levochkaa commented 2 months ago

We are going to test some features through JS SDK, so I wanted to make sure, that the changed hash is correct now. https://github.com/growthbook/growthbook/blob/main/packages/sdk-js/src/util.ts

function hashFnv32a(str: string): number {
  let hval = 0x811c9dc5;
  const l = str.length;

  for (let i = 0; i < l; i++) {
    hval ^= str.charCodeAt(i);
    hval +=
      (hval << 1) + (hval << 4) + (hval << 7) + (hval << 8) + (hval << 24);
  }
  return hval >>> 0;
}

export function hash(
  seed: string,
  value: string,
  version: number
): number | null {
  // New unbiased hashing algorithm
  if (version === 2) {
    return (hashFnv32a(hashFnv32a(seed + value) + "") % 10000) / 10000;
  }
  // Original biased hashing algorithm (keep for backwards compatibility)
  if (version === 1) {
    return (hashFnv32a(value + seed) % 1000) / 1000;
  }

  // Unknown hash version
  return null;
}

const seed = "seed"
const value = "value"

console.log(`version 1 - ${hash(seed, value, 1)}`)
console.log(`version 2 - ${hash(seed, value, 2)}`)

The output is:

version 1 - 0.579
version 2 - 0.9213

Which matches the new hash in 1.0.49 in iOS SDK, so, thanks for the fix, but this really should be mentioned somewhere (in readme, release notes). And now it is mentioned on Issues tab, for folks that going to google for this error.

levochkaa commented 2 months ago

Hi, @vazarkevych, didn't see your reply about Java SDK.

I think the issue is closed, consider mentioning the change in release notes, thank you.