feat(settings): add packet authenticity policy#6178
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a shared (Android/Desktop) “Packet authenticity” security setting that writes directly to SecurityConfig.packet_signature_policy, including Strict-mode confirmation UX, capability gating via device metadata, and test/screenshot coverage.
Changes:
- Introduces
PacketAuthenticitySettingCompose UI with Strict confirmation dialog and “unsupported” state messaging. - Wires the setting into
SecurityConfigScreenCommonand adds previews + screenshot-test entries. - Adds common UI tests validating default mapping, selection behavior, and Strict confirmation dismissal on connection/support loss.
Reviewed changes
Copilot reviewed 7 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| screenshot-tests/src/screenshotTest/kotlin/org/meshtastic/screenshots/feature/SettingsScreenshotTests.kt | Adds screenshot-test composables for the new Packet authenticity setting states. |
| feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/component/PacketAuthenticitySettingTest.kt | Adds UI tests covering policy selection, Strict confirmation, and unsupported behavior. |
| feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/SecurityConfigScreen.kt | Integrates the Packet authenticity selector into the Security config screen. |
| feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/PacketAuthenticitySetting.kt | Implements the dropdown selector + Strict confirmation dialog logic. |
| feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/PacketAuthenticityPreviews.kt | Adds previews for supported/unsupported states and the Strict confirmation dialog. |
| core/resources/src/commonMain/composeResources/values/strings.xml | Adds localized strings for the Packet authenticity setting UI. |
| .skills/compose-ui/strings-index.txt | Updates the string index with the new packet_authenticity* keys. |
995d342 to
0937f65
Compare
📝 WalkthroughWalkthroughAdds packet authenticity policy resources and a shared Compose setting for compatible, balanced, and strict modes, including capability gating and strict confirmation. The setting is integrated into security configuration, with protobuf updates, behavioral tests, and light/dark screenshot coverage. ChangesPacket authenticity policy
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SecurityConfigScreen
participant PacketAuthenticitySetting
participant MeshtasticResourceDialog
SecurityConfigScreen->>PacketAuthenticitySetting: render policy and capability state
PacketAuthenticitySetting->>MeshtasticResourceDialog: request strict confirmation
MeshtasticResourceDialog-->>PacketAuthenticitySetting: confirm or dismiss
PacketAuthenticitySetting-->>SecurityConfigScreen: update packet signature policy
Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/component/PacketAuthenticitySettingTest.kt (2)
195-209: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the gating contract directly.
These tests do not directly verify that the selector is disabled. Additionally, the “unknown capability” case combines
connected = falsewithsupported = null, so it cannot distinguish connection gating from capability gating. AddassertIsNotEnabled()on the clickable selector and separate cases forconnected = false, supported = trueandconnected = true, supported = null, matchingcanConfigurePolicy = connected && supported == trueinPacketAuthenticitySetting.kt.Also applies to: 213-226
🤖 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 `@feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/component/PacketAuthenticitySettingTest.kt` around lines 195 - 209, The PacketAuthenticitySetting tests do not directly assert selector enablement and conflate connection and capability gating. In the tests around unsupported and unknown capability, target the clickable selector and assertIsNotEnabled(), then add separate cases for connected = false with supported = true and connected = true with supported = null, matching canConfigurePolicy = connected && supported == true in PacketAuthenticitySetting.
186-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid relying on popup node ordering.
Selecting
onAllNodesWithText(strictLabel)[1]assumes the menu item is always the second matching node. Use a stable test tag or semantic property for the strict option instead, so changes to the dropdown’s semantics do not break this test.🤖 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 `@feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/component/PacketAuthenticitySettingTest.kt` around lines 186 - 188, The test currently selects the strict packet authenticity option by assuming it is the second matching node. In the test around strictLabel selection, identify or add a stable test tag or semantic property for the strict menu item and use it to target that option directly instead of indexing onAllNodesWithText(strictLabel)[1].
🤖 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.
Nitpick comments:
In
`@feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/component/PacketAuthenticitySettingTest.kt`:
- Around line 195-209: The PacketAuthenticitySetting tests do not directly
assert selector enablement and conflate connection and capability gating. In the
tests around unsupported and unknown capability, target the clickable selector
and assertIsNotEnabled(), then add separate cases for connected = false with
supported = true and connected = true with supported = null, matching
canConfigurePolicy = connected && supported == true in
PacketAuthenticitySetting.
- Around line 186-188: The test currently selects the strict packet authenticity
option by assuming it is the second matching node. In the test around
strictLabel selection, identify or add a stable test tag or semantic property
for the strict menu item and use it to target that option directly instead of
indexing onAllNodesWithText(strictLabel)[1].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 22f4f19d-9dbc-4cf3-9912-a3d52e89a273
⛔ Files ignored due to path filters (10)
screenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotPacketAuthenticityBalanced_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotPacketAuthenticityBalanced_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotPacketAuthenticityCompatible_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotPacketAuthenticityCompatible_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotPacketAuthenticityStrictConfirmation_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotPacketAuthenticityStrictConfirmation_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotPacketAuthenticityStrict_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotPacketAuthenticityStrict_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotPacketAuthenticityUnsupported_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotPacketAuthenticityUnsupported_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (7)
.skills/compose-ui/strings-index.txtcore/resources/src/commonMain/composeResources/values/strings.xmlfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/PacketAuthenticityPreviews.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/PacketAuthenticitySetting.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/SecurityConfigScreen.ktfeature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/component/PacketAuthenticitySettingTest.ktscreenshot-tests/src/screenshotTest/kotlin/org/meshtastic/screenshots/feature/SettingsScreenshotTests.kt
0937f65 to
fa45449
Compare
|
Addressed the latest CodeRabbit test review in
I did not expand the shared
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
|
UI snapshot coverage is included in this draft. Balanced policy: Strict-mode confirmation: |
|
One thing to reconsider on the merits before this ships: the UI presents Balanced as the Recommended default, but per design#121 pre-2.8 firmware strips the Data field-10 signature on relayed packets during decode/re-encode, so Balanced silently drops legitimate relayed traffic from older nodes. Recommending the setting that causes the drops seems risky until 2.8 is widely deployed — worth reconsidering either the "Recommended" copy or the default until then. Noting for context that CI is red only because it pins the unpublished protobufs SNAPSHOT (depends on protobufs#983) — that's an external block, fine for a draft. The code and test coverage themselves look solid. |
00e6921 to
d5bdfb5
Compare
|
Holding this until it's re-aligned with the merged schema — the default policy decision changed upstream after this PR was written. I rebased this branch onto The blocker is that the default was reversed upstream during the protobufs#983 review. thebentern requested the change, citing user reports of pre-2.8 firmware eating signed packets on decode/re-encode (a signed NodeInfo getting blackholed by a legacy node in the rebroadcast chain). The merged
This PR still treats Balanced as the default and markets it as Recommended (
Once those are done and CI is green I'm happy to take another look. |
d5bdfb5 to
9a4936d
Compare
Summary
Adds the shared Android/Desktop Security setting for the device-enforced packet authenticity policy defined in design#121:
SecurityConfig.packet_signature_policy.DeviceMetadata.has_xeddsacapability handling: supported firmware can configure the policy, explicit lack of support is explained, and missing metadata remains an unknown or disconnected state.Closes #6176.
Visual validation
Rendered screenshot-test captures below cover each required state in both light and dark themes. These committed reference assets are intentional regression tests; the links are to the immutable PR-head commit, not local files. This isolated preference has no separate compact or desktop layout branch, so the light/dark component captures are the applicable coverage.
Validation
main, including protobuf snapshot2.7.26.119-g164b6e2-SNAPSHOTafter protobufs#983 merged.:feature:settings:jvmTest :feature:settings:spotlessCheck :feature:settings:detektpassed.:feature:settings:allTests :desktopApp:compileKotlinpassed.:core:ui:spotlessCheck :core:ui:detektpassed.:screenshot-tests:updateDebugScreenshotTest :screenshot-tests:validateDebugScreenshotTestpassed.Compatible = 0; focused settings tests cover mapping, selection, Strict confirmation, disconnect or capability-loss races, unknown capability, and unsupported firmware.protobufs#983 and firmware#10967 are merged. No radio hardware verification has been performed; keep this PR draft until a capable device completes a read/write/read-back round trip.