Route background NIP-55 pubkey verification through shared checkPubkey helper

[wksantiago]

Routes the background NIP-55 pubkey verification (Nip55ContentProvider) through the same checkPubkey helper the foreground path uses, removing the last inline copy of the self-verification block.

Behavior-preserving dedup: checkPubkey still receives a groupPubkey read independently from keystore share metadata, so the independent-second-source property that makes the check meaningful is preserved. Same non-GET_PUBLIC_KEY skip, same empty-result rejection, same fail-closed on null/empty stored pubkey, same hex encoding, same constant-time MessageDigest.isEqual.

Relates to #414 — see that issue for the decision to keep the check in Kotlin rather than relocating into Rust.

[coderabbitai]

Warning

Review limit reached

@wksantiago, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 49 minutes

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9541b0f5-6419-4bf9-8d9b-f4ec7458fd6b

📥 Commits

Reviewing files that changed from the base of the PR and between 3b84f1c and 67732d1.

📒 Files selected for processing (1)

• app/src/main/kotlin/io/privkey/keep/nip55/Nip55ContentProvider.kt

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[kwsantiago]

Verified on a Pixel 9a running Android 17: full device suite 291 tests, 0 failures, 5 skipped, matching the main baseline exactly. Local gate green (unit tests, lintDebug, RNG hygiene guard) and PubkeyVerificationTest is 6/6. CI green.

The dedup is behavior-preserving, which is the thing worth checking carefully here since this is the one place the app catches a handler returning a pubkey that is not the user's. I compared the removed inline block against checkPubkey branch by branch: same skip for non-GET_PUBLIC_KEY, same empty-result rejection, same fail-closed on a null or empty stored pubkey, same hex encoding, same MessageDigest.isEqual constant-time compare. A refactor here could easily have turned "no stored pubkey available" into a pass, and it does not.

Both production paths now go through the one helper (Nip55Activity.kt:880 foreground, Nip55ContentProvider.kt:423 background) and a single MessageDigest.isEqual remains in the codebase, so there is no third copy left to drift. MessageDigest stays imported in the provider because line 80 still uses it for the caller-package digest, which is unrelated.

Collapsing the three distinct IllegalStateException messages into one is a small improvement rather than a loss: the specific reason is still logged under BuildConfig.DEBUG inside the helper, while the caller now learns only that verification failed.

One thing to settle before merging: Closes #414

This does not close #414. That issue's remaining item is specifically to move the self-verification into the Rust Nip55Handler, and it goes further, warning that folding both sides into Rust "removes the independent second source that makes the check meaningful" and suggesting that if the goal is deduplication rather than relocation, a shared Rust helper still taking the independently-read expected pubkey as a parameter would preserve the property.

What landed here is that shape in Kotlin. The security property is preserved, since checkPubkey still receives a groupPubkey read independently from keystore share metadata, so I think this is the better answer to the question the issue posed. But it answers it implicitly. Merging with Closes #414 retires the RMP-boundary question in a squash-merge message rather than recording a decision.

Suggest either leaving #414 open for the Rust relocation, or updating it to say the Kotlin dedup was chosen deliberately and why. "The independent second source is worth more than boundary purity" is a real architectural call, and the next person to look at the Kotlin/Rust split deserves to find it written down rather than inferred from a closed issue.

No blockers on the code.

[wksantiago]

Thanks @kwsantiago — agreed on all points, and thanks for the branch-by-branch check on the fail-closed path.

Settled the Closes #414 question your way, option 2: dropped Closes #414 from the PR body (now Relates to #414) so the boundary question isn't retired in a squash message, and recorded the decision on #414 before closing it.

The decision is written down as you suggested: the NIP-55 self-verification stays in Kotlin and we're declining the Rust relocation, because the independent second source (groupPubkey read from keystore share metadata) is what makes the check meaningful — folding both sides into Nip55Handler would let a single bug there pass both the response and its own self-check. #414 now carries that rationale plus a standing note for future work not to move the verification into the component that produces the pubkey. "The independent second source is worth more than boundary purity" — as you put it.

No code changes; ready to merge.