-
Notifications
You must be signed in to change notification settings - Fork 377
feat: hash PII (email/SMS) in SharedPreferences at rest #2614
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
a35c85e
d1b89d1
347fef8
938f14d
e780b74
a91c17e
6297d81
3dc4882
1e0a913
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| package com.onesignal.common | ||
|
|
||
| import java.security.MessageDigest | ||
|
|
||
| /** | ||
| * Deterministic SHA-256 hashing for PII fields (email, phone number) so that | ||
| * sensitive data stored in SharedPreferences is not readable in plain text on | ||
| * rooted devices or via ADB backup. | ||
| * | ||
| * The hash is hex-encoded and always 64 characters long. | ||
| */ | ||
| object PIIHasher { | ||
| private const val SHA256_HEX_LENGTH = 64 | ||
| private val SHA256_HEX_REGEX = Regex("^[a-f0-9]{$SHA256_HEX_LENGTH}$") | ||
|
|
||
| /** Returns the lowercase hex-encoded SHA-256 hash of [value]. */ | ||
| fun hash(value: String): String { | ||
| val digest = MessageDigest.getInstance("SHA-256") | ||
| val bytes = digest.digest(value.toByteArray(Charsets.UTF_8)) | ||
| return bytes.joinToString("") { "%02x".format(it) } | ||
|
sherwinski marked this conversation as resolved.
|
||
| } | ||
|
|
||
| /** Returns `true` if [value] looks like a 64-char lowercase hex SHA-256 digest. */ | ||
| fun isHashed(value: String): Boolean = SHA256_HEX_REGEX.matches(value) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,15 @@ | ||
| package com.onesignal.user.internal | ||
|
|
||
| import com.onesignal.common.PIIHasher | ||
|
sherwinski marked this conversation as resolved.
|
||
| import com.onesignal.user.internal.subscriptions.SubscriptionModel | ||
| import com.onesignal.user.subscriptions.IEmailSubscription | ||
|
|
||
| internal class EmailSubscription( | ||
| model: SubscriptionModel, | ||
| ) : Subscription(model), IEmailSubscription { | ||
| override val email: String | ||
| get() = model.address | ||
| get() { | ||
| val address = model.address | ||
| return if (PIIHasher.isHashed(address)) "" else address | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,15 @@ | ||
| package com.onesignal.user.internal | ||
|
|
||
| import com.onesignal.common.PIIHasher | ||
| import com.onesignal.user.internal.subscriptions.SubscriptionModel | ||
| import com.onesignal.user.subscriptions.ISmsSubscription | ||
|
|
||
| internal class SmsSubscription( | ||
| model: SubscriptionModel, | ||
| ) : Subscription(model), ISmsSubscription { | ||
| override val number: String | ||
| get() = model.address | ||
| get() { | ||
| val address = model.address | ||
| return if (PIIHasher.isHashed(address)) "" else address | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why return empty, would that affect getBySMS check?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead of this can we pass an Enum back. Hash("value") then its more clear and explicit
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with you, but I'd want to clarify one thing first.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah right, these Interface are exposed in the Can we update documentation that states -
After hydration
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,10 @@ | ||
| package com.onesignal.user.internal.subscriptions | ||
|
|
||
| import com.onesignal.common.PIIHasher | ||
| import com.onesignal.common.modeling.ModelChangeTags | ||
| import com.onesignal.common.modeling.SimpleModelStore | ||
| import com.onesignal.core.internal.preferences.IPreferencesService | ||
| import org.json.JSONObject | ||
|
|
||
| open class SubscriptionModelStore(prefs: IPreferencesService) : SimpleModelStore<SubscriptionModel>({ | ||
| SubscriptionModel() | ||
|
|
@@ -32,4 +34,18 @@ open class SubscriptionModelStore(prefs: IPreferencesService) : SimpleModelStore | |
| super.replaceAll(models, tag) | ||
| } | ||
| } | ||
|
|
||
| override fun transformJsonForPersistence( | ||
| model: SubscriptionModel, | ||
| json: JSONObject, | ||
| ): JSONObject { | ||
| val type = json.optString("type", "") | ||
| if (type.isEmpty() || type == SubscriptionType.PUSH.toString()) return json | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what if we hash the PUSH token as well? Would that cause us any issues?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think that would cause issues. On cold start (before server hydration), Take CreateSubscriptionOperation for example, which sends Is there a reason to hash the push token? It's not as personally-identifiable as email or phone number.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok makes sense. I was assuming that just like email and phone number these values are not really required and a one way hash would be applicable there as well. But we are using the token to Create the Subscription then yeah, we cant. Yeah I was trying to push more so on the consistency side. |
||
|
|
||
| val address = json.optString("address", "") | ||
| if (address.isNotEmpty() && !PIIHasher.isHashed(address)) { | ||
| json.put("address", PIIHasher.hash(address)) | ||
| } | ||
| return json | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| package com.onesignal.common | ||
|
|
||
| import io.kotest.core.spec.style.FunSpec | ||
| import io.kotest.matchers.shouldBe | ||
| import io.kotest.matchers.string.shouldHaveLength | ||
| import io.kotest.matchers.string.shouldMatch | ||
|
|
||
| class PIIHasherTests : FunSpec({ | ||
|
|
||
| test("hash produces 64-char lowercase hex string") { | ||
| val result = PIIHasher.hash("test@example.com") | ||
| result shouldHaveLength 64 | ||
| result shouldMatch Regex("^[a-f0-9]{64}$") | ||
| } | ||
|
|
||
| test("hash is deterministic") { | ||
| PIIHasher.hash("test@example.com") shouldBe PIIHasher.hash("test@example.com") | ||
| } | ||
|
|
||
| test("hash produces different output for different input") { | ||
| val hash1 = PIIHasher.hash("user1@example.com") | ||
| val hash2 = PIIHasher.hash("user2@example.com") | ||
| (hash1 != hash2) shouldBe true | ||
| } | ||
|
|
||
| test("hash matches known SHA-256 digest") { | ||
| // SHA-256 of "hello" is well-known | ||
| PIIHasher.hash("hello") shouldBe "2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824" | ||
| } | ||
|
|
||
| test("isHashed returns true for valid 64-char hex string") { | ||
| val hashed = PIIHasher.hash("test@example.com") | ||
| PIIHasher.isHashed(hashed) shouldBe true | ||
| } | ||
|
|
||
| test("isHashed returns false for plain email") { | ||
| PIIHasher.isHashed("test@example.com") shouldBe false | ||
| } | ||
|
|
||
| test("isHashed returns false for phone number") { | ||
| PIIHasher.isHashed("+15558675309") shouldBe false | ||
| } | ||
|
|
||
| test("isHashed returns false for empty string") { | ||
| PIIHasher.isHashed("") shouldBe false | ||
| } | ||
|
|
||
| test("isHashed returns false for uppercase hex") { | ||
| val upper = PIIHasher.hash("test").uppercase() | ||
| PIIHasher.isHashed(upper) shouldBe false | ||
| } | ||
|
|
||
| test("isHashed returns false for 63-char hex string") { | ||
| PIIHasher.isHashed("a".repeat(63)) shouldBe false | ||
| } | ||
|
|
||
| test("isHashed returns false for 65-char hex string") { | ||
| PIIHasher.isHashed("a".repeat(65)) shouldBe false | ||
| } | ||
| }) |
Uh oh!
There was an error while loading. Please reload this page.