feat(roadflare): Kind 3189 driver ping receiver (Issue #4)#60
feat(roadflare): Kind 3189 driver ping receiver (Issue #4)#60variablefate merged 10 commits intomainfrom
Conversation
…tion body locally Codex review flagged that the rider-supplied \`message\` field in the Kind 3189 payload was displayed verbatim by the driver's system notification. Any approved follower with the RoadFlare key could use a custom client to set \`message\` to arbitrary text. Receiver now ignores the \`message\` field entirely, requires \`action == \"ping\"\`, and builds the notification body locally from \`riderName\` with length/control-char sanitization. Added test for malicious \`message\` field being suppressed.
variablefate
left a comment
There was a problem hiding this comment.
Code review (hostile cold-read, head e38271b)
Found 5 issues. Verified items listed at the bottom.
HIGH
1. Suppression ordering inverted — seenRequests dedup fires before HMAC validation
The plan specifies: HMAC validation → seenRequests dedup → mute → presence gate → rate-limit. The implementation reverses the first two steps: seenRequests.add(event.id) runs synchronously in the subscription callback before processPingEvent is launched, and parseAndDecrypt (HMAC) runs inside processPingEvent.
Consequence: any approved RoadFlare follower (or a misbehaving relay) can submit a Kind 3189 event with a valid event ID but invalid HMAC. The event ID gets permanently committed to seenRequests; the legitimate copy (relay retransmit, or the real event arriving after the forgery) is silently dropped as already-seen, without ever reaching HMAC validation. Rate-limit slots are not consumed, so this is a cheap suppression attack with no per-attempt cost to the attacker.
Fix: move seenRequests.add(event.id) to the top of processPingEvent, after parseAndDecrypt returns non-null.
MEDIUM
2. T_TAG is dead code — subscription filter omits the t-tag
T_TAG = "roadflare-ping" is defined but the subscribeToDriverPings filter only constrains kinds and p-tag. The t-tag documented in the spec (["t", "roadflare-ping"]) is not applied as a relay-side filter.
If any other Kind 3189 traffic addresses this driver (now or in future protocol versions that reuse the kind), all of it will pass through to processPingEvent. HMAC auth provides defense-in-depth, but the subscription is broader than intended. Either add "t" to listOf(T_TAG) to the filter, or remove the unused constant.
3. CHANNEL_DRIVER_PING uses IMPORTANCE_DEFAULT but caller passes isHighPriority = true
On Android O+ (API 26+), channel importance is the sole gating factor for heads-up display eligibility. IMPORTANCE_DEFAULT suppresses heads-up banners; NotificationCompat.PRIORITY_HIGH set at the notification level is silently ignored by the system. The channel also configures setSound(null, null) + enableVibration(false), and buildDriverStatusNotification further sets setSilent(true). isHighPriority = true has zero effect here.
If the intent is a silent tray notification (a "friendly nudge"), use isHighPriority = false to match the channel semantics and avoid confusion. If heads-up is actually desired, upgrade the channel to IMPORTANCE_HIGH and remove setSilent(true).
LOW
4. Empty riderName produces malformed notification body
parseAndDecrypt falls back to "" when riderName is absent (optString("riderName", "")). The notification body becomes " is hoping you come online" — a leading space with no subject. The Kind 3173 handler in the same file uses riderName ?: "Someone" as a guard; the same pattern is missing here.
Fix: val displayName = pingData.riderName.ifEmpty { "Someone" } before the string interpolation.
5. NOTIFICATION_ID_DRIVER_PING declared in RoadflareListenerService.companion, not NotificationHelper
All other NOTIFICATION_ID_* constants (ONLINE_STATUS, RIDE_REQUEST, RIDE_UPDATE, RIDE_CANCELLED, CHAT_MESSAGE, RIDER_ACTIVE, FOLLOW_REQUEST) live in NotificationHelper. The new 14001 constant breaks this convention, leaving the notification ID namespace map in NotificationHelper incomplete for future reference.
INFO (pre-existing, not introduced here)
NOTIFICATION_ID_FOLLOW_REQUEST (NotificationHelper:36) == NOTIFICATION_ID_ROADFLARE_LISTENER (RoadflareListenerService:58) == 3001. Follow-request dynamic IDs occupy [3001, 13000]; if abs(riderPubKey.hashCode() % 10000) == 0, a follow-request notification overwrites the foreground service sticky. Unrelated to this PR — track separately.
Verified clean
- Codex fix landed correctly:
RoadflareDriverPingDatahas nomessageproperty;action != "ping"returns null;riderNamesanitized with.take(64).filter { it >= ' ' }; notification body built locally. Tests 15+16 (action-mismatch drop, malicious-message ignore) present. - HMAC: raw 32-byte key (no SHA256 wrap),
driverPubkey + riderPubkey + window.toString()(driver-first, no delimiters), 3-window tolerance,.lowercase()before comparison. Sign-extension:"%02x".format(it.toInt() and 0xFF)correct. - Rate limiter: per-rider 30s + global 2/10min rolling window (proper eviction, not fixed bucket).
@Synchronized, injectable clock, 7 tests. - Test counts: 16 common (Robolectric) + 7 drivestr (JUnit4). No tautologies found.
NotificationHelper:CHANNEL_DRIVER_PINGadded as 6th channel; 5 existing channels untouched.NOSTR_EVENTS.md: Kind 3189 section present after Kind 3188; both3186-3188range expressions updated to3186-3189.seenRequestsshared with Kind 3173, cleared instopListening()— correct.pingSubscriptionIdlifecycle symmetric withsubscriptionId— no leak.- Unit tests:
./gradlew :common:testDebugUnitTest :drivestr:testDebugUnitTest→ BUILD SUCCESSFUL. Note:assembleDebugfails in this worktree with a pre-existing duplicate-classes DEX error onhilt_aggregated_deps._com_DrivestrApplication_GeneratedInjector— stale build artifacts, not introduced by this PR.
🤖 Generated with Claude Code
If this review was useful, react with 👍. Otherwise 👎.
|
Retraction: HIGH finding from earlier review is invalid In my previous review I flagged the suppression ordering as HIGH — claiming Plan explicitly specifies dedup-before-HMAC. Lines 94–95 of
Lines 974–975 of the same plan show the intended callback layout: // Event-id dedup: seenRequests is shared with Kind 3173 — event IDs are globally unique
if (!seenRequests.add(event.id)) return@subscribeThe code matches the plan exactly. The "MUST be" suppression ordering in the plan ( The slot-poisoning attack is impossible. Nostr event IDs are No fix needed. The ordering is correct as implemented. Apologies for the noise. 🤖 Generated with Claude Code |
…erName
Finding 1: subscribeToDriverPings was missing the "t"="roadflare-ping"
filter from the relay subscription, allowing any Kind 3189 event (from
any source) to reach the HMAC pipeline. Added T_TAG filter to match spec.
Finding 2: empty riderName produced " is hoping you come online" with a
leading space. Mirror the Kind 3173 pattern: fall back to "Someone" via
ifEmpty { "Someone" }.
…ion on re-subscribe Finding 1: the NOTIFICATION_ID_DRIVER_PING range comment falsely implied [3001, 13000] was a clean follow-request range. It isn't — 3001 and 3002 are occupied by ROADFLARE_LISTENER and ROADFLARE_REQUEST in the same companion object. Replaced with an accurate occupancy accounting. Finding 2: subscribeToDriverPings overwrote pingSubscriptionId without closing the previous relay subscription first, leaking the old subscription on re-subscribe. Added a close-first guard matching the closeSubscription() call pattern already used in stopListening().
…e 30-min TTL constant RoadflareListenerService.showPingNotification was passing isHighPriority = true to buildDriverStatusNotification while also overriding the channel to CHANNEL_DRIVER_PING (IMPORTANCE_DEFAULT + silent-tray pattern, SoundManager already played the alert tone). On API 26+ the channel importance is the sole heads-up gate — PRIORITY_HIGH at the notification level is silently ignored, so the true flag was misreading caller intent. Flip to false to match actual behaviour. Add ROADFLARE_DRIVER_PING_MINUTES = 30 to RideshareExpiration so the Kind 3189 TTL lives alongside the other named TTLs instead of as a magic 1800 in the KDoc, matching the pattern for every other TTL-bearing rideshare event. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code reviewReviewed at
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
|
Retraction: subscription-prune finding is invalid The previous review claimed Kind 3189 subscriptions are silently pruned after 30 minutes. That is wrong. private val RIDESHARE_KINDS = setOf(
30173, 3173, 3174, 3175, 3176, 3177, 3178, 3179, 3180, 30174
)Kind 3189 is not in this set, so 🤖 Generated with Claude Code |
Catches docs up to recent merges that added two protocol features: 1. Fiat fare fields on Kind 3173 ride offer events (PRs #61 + #62, ADR-0008) - fare_fiat_amount: decimal string (e.g., "12.50") - fare_fiat_currency: ISO 4217 code (e.g., "USD") - Both-or-neither rule - Encoded only for fiat rails (not cashu/lightning) - Authoritative display value — no BTC conversion drift - Compatible with roadflare-ios per ADR-0008 2. Kind 3189 RoadFlare Driver Ping (PR #60, Issue #4) - Already had in-depth section in NOSTR_EVENTS.md - Missing from project CLAUDE.md RoadFlare events table - Missing from CONNECTIONS.md activity summary - Added RoadflareListenerService + DriverPingRateLimiter to Key Files Files updated: - docs/protocol/NOSTR_EVENTS.md: bumped 1.8 → 1.9; added fare_fiat_amount + fare_fiat_currency to Direct + Broadcast Kind 3173 JSON schemas; added Authoritative Fiat Fare Fields explanation - docs/CONNECTIONS.md: bumped Last Updated; added April 2026 changes block - .claude/CLAUDE.md: added Kind 3189 to RoadFlare events table; updated Kind 3173 row + Multi-Mint table to mention fiat fare; added RoadflareListenerService + DriverPingRateLimiter to Key Files; added Driver Ping section under RoadFlare Architecture; added Authoritative Fiat Fare section under Payment System No code changes — documentation only. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…63) Catches docs up to recent merges that added two protocol features: 1. Fiat fare fields on Kind 3173 ride offer events (PRs #61 + #62, ADR-0008) - fare_fiat_amount: decimal string (e.g., "12.50") - fare_fiat_currency: ISO 4217 code (e.g., "USD") - Both-or-neither rule - Encoded only for fiat rails (not cashu/lightning) - Authoritative display value — no BTC conversion drift - Compatible with roadflare-ios per ADR-0008 2. Kind 3189 RoadFlare Driver Ping (PR #60, Issue #4) - Already had in-depth section in NOSTR_EVENTS.md - Missing from project CLAUDE.md RoadFlare events table - Missing from CONNECTIONS.md activity summary - Added RoadflareListenerService + DriverPingRateLimiter to Key Files Files updated: - docs/protocol/NOSTR_EVENTS.md: bumped 1.8 → 1.9; added fare_fiat_amount + fare_fiat_currency to Direct + Broadcast Kind 3173 JSON schemas; added Authoritative Fiat Fare Fields explanation - docs/CONNECTIONS.md: bumped Last Updated; added April 2026 changes block - .claude/CLAUDE.md: added Kind 3189 to RoadFlare events table; updated Kind 3173 row + Multi-Mint table to mention fiat fare; added RoadflareListenerService + DriverPingRateLimiter to Key Files; added Driver Ping section under RoadFlare Architecture; added Authoritative Fiat Fare section under Payment System No code changes — documentation only. Co-authored-by: variablefate <variablefate@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
driverPingRequest: new Nostr event that lets a rider nudge an offline trusted driver to come online via their RoadFlare networkRoadflareDriverPingEvent(common): stateless validation pipeline — kind check, NIP-40 expiry, 3-window HMAC-SHA256 auth, NIP-44 decrypt (14 Robolectric tests)DriverPingRateLimiter(drivestr): per-rider 30 s spam throttle + 2-per-10-min global rolling cap, injectable clock for determinism (7 JUnit4 tests)RoadflareListenerService: new Kind 3189 subscription alongside existing 3173; suppression ordering mute → presence gate →tryAccept();NOTIFICATION_ID_DRIVER_PINGrange [14001, 24000]CHANNEL_DRIVER_PING("driver_ping"): new notification channel with operator-controllable settingsNOSTR_EVENTS.md: Kind 3189 section + range expressions updated from3186-3188→3186-3189Test Plan
./gradlew :common:testDebugUnitTest—RoadflareDriverPingEventTest./gradlew :drivestr:testDebugUnitTest—DriverPingRateLimiterTest./gradlew :common:assembleDebug :drivestr:assembleDebugAVAILABLEorIN_RIDE→ ping notification suppressedNotes
roadflare-iosbranchclaude/issue-4-driver-ping; both PRs should merge in lockstep (either order is safe — graceful degradation if one side lags)roadflare-iosrepo for architectural rationale🤖 Generated with Claude Code