Skip to content

fix: address 2.8.0 release-audit findings#6189

Merged
jamesarich merged 3 commits into
mainfrom
claude/review-v2-7-14-head-cf3269
Jul 10, 2026
Merged

fix: address 2.8.0 release-audit findings#6189
jamesarich merged 3 commits into
mainfrom
claude/review-v2-7-14-head-cf3269

Conversation

@jamesarich

@jamesarich jamesarich commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Why

A full audit of the v2.7.14..HEAD range (387 commits) ahead of the 2.8.0 public release surfaced a short punch list: one web-reachable security gap, one fatal crash new to the 2.8.0 line, two correctness bugs in systems introduced this cycle, and localization/permission polish. This PR lands the review-sized items in one pass so they're in before 2.8.0 promotes out of closed testing.

What

🐛 Bug Fixes

  • Confirm deep-link connection changes. connections?address=… is a verified meshtastic.org app link, so any web page could silently re-point the app at an arbitrary (including attacker-hosted TCP) address or force a disconnect. Deep-link-originated changes now show a confirm dialog, and MOCK/REPLAY addresses are gated on debug builds in BaseRadioTransportFactory (the base class short-circuited them to valid before the platform check ran).
  • Fix new-in-2.8.0 fatal crash by swapping the ktor engine to OkHttp (Crashlytics 97ae5ea1, builds 29321237–29321401). The Android engine rides HttpURLConnection, whose AOSP-bundled okhttp 2.x fork throws IllegalStateException: Unbalanced enter/exit when a request is cancelled mid-body — amplified by the retry-on-timeout policy from fix(network): retry transient connection/IO failures to api.meshtastic.org #5870. This stays entirely within ktor: only the engine artifact changes; client config, plugins, and ktor-network are untouched.
  • Merge quick_chat during cross-transport DB unification (feat(database): unify device database across transports #6096 follow-up). The one-time merge folded every per-device table except quick-chat actions, then deleted the source DB — multi-transport users lost their buttons. Dedupes on name+message+mode and appends with fresh ids.
  • Dedupe discovered TCP devices by address. mDNS can resolve two services to one host:port (re-announce before expiry), producing duplicate fullAddress entries and a hard LazyColumn duplicate-key crash (tcp-discovered: variant of Crashlytics 159e2535).
  • Reject digit-led sibling variants in firmware filename validation. firmware-tlora-v2-1-1_6-2.7.14.bin validated for target tlora-v2, so the feat(firmware): Confirm local firmware files before flashing #6083 confirm gate could pass a wrong-variant image. The post-target token must now be a dotted version or the literal firmware.
  • Pre-Android 12: surface fine location as the Bluetooth scan permission. Reporting "granted" pre-S left users who declined location with an empty device list and no re-prompt path (the platform gates BLE scanning on fine location there; coarse-only returns zero scan results on API 29/30).
  • Show 0 °C temperature readings in the node list. Temperature is nullable (null = no sensor); guarding against 0 hid an ordinary freezing-point reading the chart already plots. Humidity keeps its zero-guard intentionally (0 % RH is not physically reachable, matching the chart's filter).

🧹 Chores

  • Localize hardcoded PM1.0/PM2.5/PM10/CO₂ and Current/Voltage labels in the air-quality and power metrics cards (Crowdin can't see string literals).

Testing Performed

  • New regression tests in IsValidFirmwareFileTest: rejects digit-led sibling variants (verified to fail against the old regex) and accepts a v-prefixed version after the target.
  • Full baseline green on this branch: spotlessApply spotlessCheck detekt assembleDebug test allTests (both flavors).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added confirmation prompts when deep-linking to connect or disconnect a device.
  • Bug Fixes
    • Merge now includes quick-chat actions from per-node databases without duplicates.
    • Improved Bluetooth permission behavior on pre-Android 12 devices.
    • Prevented duplicate TCP discovery entries and tightened address validation for virtual transports.
    • Refined temperature sensor selection and firmware filename matching.
    • Improved localized unit/label rendering for air quality and power charts.
  • Tests
    • Added coverage for firmware filename validation edge cases and TCP discovery deduplication.

- Confirm deep-link connect/disconnect requests before re-pointing the
  radio (the connections path is a verified meshtastic.org app link, so
  any web page can fire one); gate MOCK/REPLAY addresses on debug builds
- Swap the ktor Android engine for OkHttp: the platform's bundled
  okhttp 2.x fork behind HttpURLConnection crashes with "Unbalanced
  enter/exit" when a request cancels mid-body (Crashlytics 97ae5ea1,
  new in 2.8.0)
- Merge quick_chat actions during cross-transport DB unification before
  the source database is retired
- Dedupe discovered TCP devices by address (LazyColumn duplicate-key
  crash on re-announced mDNS services)
- Reject digit-led sibling variants in firmware filename validation
  (tlora-v2 no longer matches tlora-v2-1-1_6 images)
- Pre-S: surface ACCESS_FINE_LOCATION as the Bluetooth scan permission
  so a declined grant is re-promptable instead of an empty device list
- Localize hardcoded PM/CO2/current/voltage labels; show 0 degree
  temperature readings (null already means absent)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions github-actions Bot added the bugfix PR tag label Jul 10, 2026
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 34d72b70-38d8-400d-89af-c01e6fd6fb33

📥 Commits

Reviewing files that changed from the base of the PR and between ee7e877 and cf19986.

📒 Files selected for processing (1)
  • feature/connections/src/commonTest/kotlin/org/meshtastic/feature/connections/domain/usecase/TcpDiscoveryHelpersTest.kt

📝 Walkthrough

Walkthrough

The PR switches the Android HTTP client to OkHttp, updates connectivity handling, merges quick-chat actions between databases, tightens firmware filename matching, adds deep-link confirmation dialogs, and adjusts sensor and metric presentation.

Changes

Network and runtime connectivity

Layer / File(s) Summary
Ktor OkHttp engine wiring
gradle/libs.versions.toml, androidApp/build.gradle.kts, androidApp/src/main/kotlin/org/meshtastic/app/di/NetworkModule.kt
The Android Ktor dependency and HttpClient engine change from Android to OkHttp.
Virtual transport address validation
core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BaseRadioTransportFactory.kt
Mock and replay transports are separately gated by isMockTransport(), while the ! address prefix remains valid.
Legacy Bluetooth permission state
core/ui/src/androidMain/kotlin/org/meshtastic/core/ui/util/PlatformUtils.kt
Pre-Android-12 Bluetooth permission state now reflects runtime fine-location permission.

Connection discovery and deep links

Layer / File(s) Summary
Deep-link confirmation flow
.skills/compose-ui/strings-index.txt, core/resources/src/commonMain/composeResources/values/strings.xml, feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/navigation/ConnectionsNavigation.kt
Deep links use saveable pending state and localized confirmation dialogs before connecting or disconnecting.
TCP discovery deduplication
feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/domain/usecase/TcpDiscoveryHelpers.kt, feature/connections/src/commonTest/kotlin/org/meshtastic/feature/connections/domain/usecase/TcpDiscoveryHelpersTest.kt
TCP services are deduplicated by fullAddress before sorting and rendering, with a test for duplicate records.

Quick-chat database merging

Layer / File(s) Summary
Quick-chat merge checkpoint
core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/QuickChatActionDao.kt, core/database/src/commonMain/kotlin/org/meshtastic/core/database/DatabaseMerger.kt
Database folding reads ordered quick-chat snapshots, deduplicates by name/message/mode, and appends new actions with fresh UUIDs and positions.

Firmware filename validation

Layer / File(s) Summary
Firmware target pattern validation
feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/FirmwareFileHandler.kt, feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/IsValidFirmwareFileTest.kt
Firmware matching accepts dotted or v-prefixed versions and rejects digit-led sibling variants, with tests for both cases.

Sensor and metrics presentation

Layer / File(s) Summary
Temperature sensor selection
core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItem.kt
Temperature selection uses null presence, preferring environment temperature and falling back to CO₂ sensor temperature.
Localized air-quality and power metrics
feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/AirQualityMetrics.kt, feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/PowerMetrics.kt
Metric labels and units use localized resources and dedicated formatter functions.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant ConnectionsNavigation
  participant MeshtasticDialog
  participant scanModel
  User->>ConnectionsNavigation: open connection deep link
  ConnectionsNavigation->>MeshtasticDialog: show connect or disconnect confirmation
  User->>MeshtasticDialog: confirm or dismiss
  MeshtasticDialog->>ConnectionsNavigation: return action
  ConnectionsNavigation->>scanModel: changeDeviceAddress or disconnect
Loading
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects that this PR addresses release-audit findings, which matches the broad set of fixes in the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/PowerMetrics.kt (1)

324-332: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Apply the same MetricFormatter.voltage fix to the end-axis label.

The marker formatter above was just fixed to use MetricFormatter.voltage(value.toFloat(), 1) instead of manual " V" concatenation, but the end-axis valueFormatter a few lines below still uses the old pattern. Same value, same screen, inconsistent formatting/unit-localization.

♻️ Proposed fix
                 endAxis =
                 if (voltageData.isNotEmpty()) {
                     VerticalAxis.rememberEnd(
                         label = ChartStyling.rememberAxisLabel(color = voltageColor),
-                        valueFormatter = { _, value, _ -> "${NumberFormatter.format(value.toFloat(), 1)} V" },
+                        valueFormatter = { _, value, _ -> MetricFormatter.voltage(value.toFloat(), 1) },
                     )
                 } else {
                     null
                 },

As per coding guidelines, "Use MetricFormatter from core:common for display strings (temperature, voltage, percent, signal). Avoid scattered formatString("%.1f°C", val) calls."

🤖 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/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/PowerMetrics.kt`
around lines 324 - 332, Update the end-axis valueFormatter in the voltageData
branch of PowerMetrics to call MetricFormatter.voltage(value.toFloat(), 1)
instead of NumberFormatter.format(...)+manual " V" concatenation, matching the
marker formatter and localized voltage formatting.

Source: Coding guidelines

🤖 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/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/PowerMetrics.kt`:
- Around line 324-332: Update the end-axis valueFormatter in the voltageData
branch of PowerMetrics to call MetricFormatter.voltage(value.toFloat(), 1)
instead of NumberFormatter.format(...)+manual " V" concatenation, matching the
marker formatter and localized voltage formatting.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7de8e8e7-6de2-4039-8a70-ebd31d9ff9d8

📥 Commits

Reviewing files that changed from the base of the PR and between de37a8c and b8810c0.

📒 Files selected for processing (16)
  • .skills/compose-ui/strings-index.txt
  • androidApp/build.gradle.kts
  • androidApp/src/main/kotlin/org/meshtastic/app/di/NetworkModule.kt
  • core/database/src/commonMain/kotlin/org/meshtastic/core/database/DatabaseMerger.kt
  • core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/QuickChatActionDao.kt
  • core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BaseRadioTransportFactory.kt
  • core/resources/src/commonMain/composeResources/values/strings.xml
  • core/ui/src/androidMain/kotlin/org/meshtastic/core/ui/util/PlatformUtils.kt
  • core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItem.kt
  • feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/domain/usecase/TcpDiscoveryHelpers.kt
  • feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/navigation/ConnectionsNavigation.kt
  • feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/FirmwareFileHandler.kt
  • feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/IsValidFirmwareFileTest.kt
  • feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/AirQualityMetrics.kt
  • feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/PowerMetrics.kt
  • gradle/libs.versions.toml

@jamesarich
jamesarich enabled auto-merge July 10, 2026 12:08
@jamesarich
jamesarich disabled auto-merge July 10, 2026 12:08
jamesarich and others added 2 commits July 10, 2026 07:17
Matches the marker formatter fixed in the previous commit; the axis
label was still hand-concatenating " V".

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Regression test for the distinctBy fix: duplicate mDNS announcements
for the same host/port must collapse to one device entry.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@jamesarich
jamesarich added this pull request to the merge queue Jul 10, 2026
Merged via the queue into main with commit 0a3e000 Jul 10, 2026
17 checks passed
@jamesarich
jamesarich deleted the claude/review-v2-7-14-head-cf3269 branch July 10, 2026 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix PR tag

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant