fix(map): allow editing/deleting your own locked waypoint#6344
Conversation
Toggling a waypoint to "locked" wrote a placeholder locked_to = 1 instead of the creator's node number. Every edit/delete gate checks locked_to == myNodeNum, so 1 never matched and the creator was locked out of their own waypoint — could not edit, delete, or remove it. Most visible on the google/Play Store flavor, which (unlike fdroid) never re-stamped the value on send. The "locked" toggle now stamps the creator's real node number, and the lock permission check is consolidated into a single source of truth. - core/model: add Waypoint.isLocked and Waypoint.isModifiableBy(nodeNum), replacing the duplicated, drifted locked_to expressions across both map flavors and the inbound-packet validator (the validator change is an exact behavioral equivalent of the previous check). - EditWaypointDialog: stamp locked_to = myNodeNum when locked, and drop the per-flavor re-stamp on send (the divergence that let the google path ship the placeholder unchanged). - google onDelete: broadcast the removal for any waypoint we may modify (was locked_to == 0 only, which blocked mesh-wide removal of even a correctly owned locked waypoint). - google marker: show a lock indicator in the info-window title, for parity with the fdroid marker. - add WaypointLockTest, including a regression case for #6343. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughWaypoint lock state and ownership checks are centralized in model helpers. Map editing, deletion, marker rendering, dialog lock assignment, and incoming waypoint handling now use those helpers, with tests covering owner and placeholder-node behavior. ChangesWaypoint lock permissions
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
actor Operator
participant EditWaypointDialog
participant MapView
participant WaypointLock
participant MeshDataHandlerImpl
Operator->>EditWaypointDialog: Toggle waypoint lock
EditWaypointDialog->>WaypointLock: Resolve lock state
EditWaypointDialog->>MapView: Send waypoint with lock owner
MapView->>WaypointLock: Check modification permission
MapView->>MeshDataHandlerImpl: Process waypoint packet
MeshDataHandlerImpl->>WaypointLock: Check sender permission
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerImpl.kt`:
- Around line 292-293: Update the waypoint handling around the existing
u.isModifiableBy(packet.from) check to first load the persisted waypoint by ID
and enforce its stored owner before applying updates, including when the
incoming locked_to value is 0. Reject updates from any sender other than the
persisted owner, preserve normal creation and owner-authorized update behavior,
and add a regression test covering replay of an existing locked waypoint with
locked_to = 0 by a non-owner.
In
`@feature/map/src/androidMain/kotlin/org/meshtastic/feature/map/component/EditWaypointDialog.kt`:
- Around line 229-235: Update the locking switch around onCheckedChange so it is
disabled whenever myNodeNum is null, preventing users from enabling locking
without a valid owner node number. Preserve the existing locked_to assignment
for known node identities and the unlock behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 02cad309-0593-49b3-af4d-2e8e870e7fdd
📒 Files selected for processing (7)
androidApp/src/fdroid/kotlin/org/meshtastic/app/map/MapView.ktandroidApp/src/google/kotlin/org/meshtastic/app/map/MapView.ktandroidApp/src/google/kotlin/org/meshtastic/app/map/component/WaypointMarkers.ktcore/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerImpl.ktcore/model/src/commonMain/kotlin/org/meshtastic/core/model/WaypointLock.ktcore/model/src/commonTest/kotlin/org/meshtastic/core/model/WaypointLockTest.ktfeature/map/src/androidMain/kotlin/org/meshtastic/feature/map/component/EditWaypointDialog.kt
| // A locked waypoint may only be created/updated by its owner; drop it if the sender isn't allowed to modify it. | ||
| if (!u.isModifiableBy(packet.from)) return |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the surrounding implementation.
ast-grep outline core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerImpl.kt --view expanded
printf '\n--- snippet around lines 250-340 ---\n'
sed -n '250,340p' core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerImpl.kt
printf '\n--- search for lock/owner related waypoint logic ---\n'
rg -n "locked_to|isModifiableBy|waypoint|owner" core/data/src/commonMain/kotlin/org/meshtastic/core/data -S
printf '\n--- search for related tests ---\n'
rg -n "locked_to|isModifiableBy|waypoint" . -g '*Test*' -g '*test*' -SRepository: meshtastic/Meshtastic-Android
Length of output: 24351
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Waypoint model and lock helpers ---'
rg -n "fun isModifiableBy|val isLocked|locked_to" core/model/src/commonMain/kotlin core/model/src/commonTest/kotlin -S
printf '\n%s\n' '--- Waypoint model file ---'
waypoint_file=$(rg -l "fun isModifiableBy" core/model/src/commonMain/kotlin core/model/src/commonTest/kotlin -S | head -n 1)
sed -n '1,220p' "$waypoint_file"
printf '\n%s\n' '--- lock tests ---'
sed -n '1,220p' core/model/src/commonTest/kotlin/org/meshtastic/core/model/WaypointLockTest.kt
printf '\n%s\n' '--- MeshDataHandlerTest around waypoint handling ---'
sed -n '1,260p' core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerTest.ktRepository: meshtastic/Meshtastic-Android
Length of output: 1970
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- WaypointLock.kt ---'
sed -n '1,160p' core/model/src/commonMain/kotlin/org/meshtastic/core/model/WaypointLock.kt
printf '\n%s\n' '--- MeshDataHandlerImpl rememberDataPacket path ---'
sed -n '390,470p' core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerImpl.kt
printf '\n%s\n' '--- packet/waypoint persistence search ---'
rg -n "waypoint.*upsert|upsert.*waypoint|locked_to|getWaypoints|rememberDataPacket|waypointId|waypoint =" core/database core/data core/model -SRepository: meshtastic/Meshtastic-Android
Length of output: 13569
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- ActiveWaypoints.kt ---'
sed -n '1,220p' core/model/src/commonMain/kotlin/org/meshtastic/core/model/geofence/ActiveWaypoints.kt
printf '\n%s\n' '--- MeshDataHandlerTest waypoint-related cases ---'
rg -n "waypoint|handleWaypoint|locked_to|isModifiableBy" core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerTest.kt -A 6 -B 6
printf '\n%s\n' '--- PacketRepositoryImpl / data persistence ---'
sed -n '1,180p' core/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/PacketRepositoryImpl.ktRepository: meshtastic/Meshtastic-Android
Length of output: 11599
Persisted waypoint owner must gate updates. This check only looks at the incoming locked_to value. A sender can replay the same waypoint ID with locked_to = 0 and overwrite an existing locked waypoint unless the stored row is loaded first and compared against packet.from. Add a regression test for the owner-mismatch case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerImpl.kt`
around lines 292 - 293, Update the waypoint handling around the existing
u.isModifiableBy(packet.from) check to first load the persisted waypoint by ID
and enforce its stored owner before applying updates, including when the
incoming locked_to value is 0. Reject updates from any sender other than the
persisted owner, preserve normal creation and owner-authorized update behavior,
and add a regression test covering replay of an existing locked waypoint with
locked_to = 0 by a non-owner.
There was a problem hiding this comment.
Deferring in this PR. This behavior is pre-existing — the change here was an exact-equivalent refactor of the prior locked_to != 0 && locked_to != packet.from check, so it introduces no regression. Enforcing the persisted owner (rejecting non-owner updates, including locked_to = 0) is a separate, protocol-facing hardening: it needs a repository read in the inbound packet path, firmware-parity consideration, and on-mesh/device testing to avoid dropping legitimate cross-client updates. Tracking it as a standalone follow-up rather than bundling it untested into this bug fix.
🤖 Addressed by Claude Code
There was a problem hiding this comment.
@jamesarich, agreed—this refactor preserves the prior inbound-payload validation semantics, so the persisted-owner check is a pre-existing hardening opportunity rather than a regression from this PR. Deferring it pending protocol/firmware parity and on-mesh validation is reasonable.
Would you like me to open a GitHub follow-up issue for the persisted-owner enforcement work?
🐇💭
✏️ Learnings added
Learnt from: jamesarich
Repo: meshtastic/Meshtastic-Android PR: 6344
File: core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerImpl.kt:292-293
Timestamp: 2026-07-21T20:37:19.272Z
Learning: In `core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerImpl.kt`, inbound waypoint validation intentionally preserves the existing rule that checks the incoming `Waypoint.locked_to` against `MeshPacket.from`. Enforcing ownership against a persisted waypoint record is a separate protocol-facing hardening effort that requires firmware-parity analysis and on-mesh/device validation before changing packet acceptance behavior.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
No need to open one right now, thanks. We'll file a dedicated issue if/when the persisted-owner enforcement is scoped (with the firmware-parity + on-mesh validation it needs). Leaving this thread unresolved as a visible marker of the known gap.
🤖 Addressed by Claude Code
There was a problem hiding this comment.
@jamesarich, understood. No follow-up issue will be opened now, and I’ll leave this thread unresolved as the visible marker for the persisted-owner enforcement gap.
Addresses review feedback on #6344: when myNodeNum is null the toggle would write locked_to = 0 and silently stay unlocked. Disable the switch until an owner node number is available. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Inbound waypoint handling only validated the incoming packet's locked_to against its sender, never the already-stored waypoint's owner. A non-owner could hijack a locked waypoint by replaying its id with locked_to = 0 (unlock) or their own num (takeover) — both slip past the inbound-only check. handleWaypoint now reads the currently-stored waypoint (getWaypoints() collapsed via activeWaypointPackets, the same normalisation the map UI and geofence engine use) and drops updates to a locked waypoint from anyone but the node it is locked to — including inbound locked_to = 0 unlock attempts. The stored-owner read and the persist share one session lease (persistDataPacket extracted from rememberDataPacket) so the check sees a committed snapshot instead of nesting a second lease across two coroutines. Preserves new-waypoint creation, owner unlock/edit, updates to unlocked waypoints, and the pre-existing "can't lock to a third party" rule. Adds 7 regression tests in core/data MeshDataHandlerTest. Follow-up to #6344 (deferred CodeRabbit review item); enforcement is client-side and self-heals across re-broadcasts. Un-draft gate is on-mesh / replay validation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Why
Creating a waypoint and toggling it to Locked made it impossible to edit, delete, or remove — even for the person who created it (reported on 2.8 / Play Store build, Pixel 9a).
Root cause: the Locked toggle wrote a placeholder
locked_to = 1instead of the creator's node number. Every edit/delete permission gate checkslocked_to == myNodeNum, and1never equals a real node number, so the creator failed their own gates — the marker just flashed a "locked" toast and the editor/delete never opened. The google (Play Store) flavor never re-stamped the value on send the way fdroid did, which is why it reproduced there.Fixes #6343.
What
🐛 Fix — the Locked toggle now stamps the creator's real node number, so the owner passes their own edit/delete gates. Also fixed the google
onDeletebroadcast gate, which usedlocked_to == 0only and would otherwise have blocked mesh-wide removal of even a correctly-owned locked waypoint.🧹 DRY — introduced a single source of truth in
core/model(Waypoint.isLocked,Waypoint.isModifiableBy(nodeNum)) and replaced the duplicated, driftedlocked_toexpressions across both map flavors and the inbound-packet validator (MeshDataHandlerImpl, an exact behavioral equivalent). Removed the per-flavor lock re-stamp on send — the divergence that let the google path ship the placeholder unchanged.🛠️ UI parity — the google map marker now shows a 🔒 in its info-window title for locked waypoints, matching the fdroid marker.
Testing Performed
WaypointLockTest(core/model), including a regression case for the [Bug]: Cannot delete/edit locked waypoint #6343 placeholder bug.spotlessCheck,detekt,assembleDebug(both flavors),test,allTests,kmpSmokeCompile.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests