Skip to content

fix(lockdown): re-lock serial console admin auth on USB link drop #11028

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
thebentern merged 1 commit into from
Jul 17, 2026
Merged

fix(lockdown): re-lock serial console admin auth on USB link drop #11028

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions src/SerialConsole.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,16 @@

SerialConsole *console;

#ifdef MESHTASTIC_PHONEAPI_ACCESS_CONTROL
// Last-seen USB-CDC host link (DTR/mount) state, sampled each runOnce() so a
// physical unplug/replug re-locks the per-connection admin auth (see runOnce()).
// Kept at file scope rather than as a member both because there is exactly one
// console singleton and because adding per-instance members to the PhoneAPI
// hierarchy has historically perturbed nRF52 USB-CDC enumeration (see PhoneAPI.h).
// Only compiled on lockdown (nRF52) builds.
static bool s_serialLinkUp = false;
Comment on lines +34 to +40

@coderabbitai coderabbitai Bot Jul 16, 2026

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.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Keep code comments minimal.

Both of these locations add multi-paragraph explanatory blocks, which violates the coding guidelines for C++ files requiring comments to be one or two lines maximum. As per coding guidelines, do not restate straightforward code or add multi-paragraph explanatory blocks.

  • src/SerialConsole.cpp#L34-L40: reduce this file-scope rationale to one or two lines (e.g., // Last-seen USB-CDC host link state; sampled each runOnce() to re-lock admin auth on unplug. Kept at file scope to avoid perturbing nRF52 USB-CDC enumeration with per-instance members.).
  • src/SerialConsole.cpp#L102-L118: reduce this detailed behavior explanation inside runOnce() to one or two lines (e.g., // Re-lock admin auth when the physical USB-CDC link drops by calling close(). This frees the auth slot and resets the session for the next client.).
📍 Affects 1 file
  • src/SerialConsole.cpp#L34-L40 (this comment)
  • src/SerialConsole.cpp#L102-L118
🤖 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 `@src/SerialConsole.cpp` around lines 34 - 40, The comments in
src/SerialConsole.cpp lines 34-40 and 102-118 are overly verbose; reduce each
explanatory block to no more than one or two lines. Keep the file-scope comment
focused on s_serialLinkUp and its purpose, and keep the runOnce() comment
focused on calling close() to re-lock admin authentication after the USB-CDC
link drops.

Source: Coding guidelines

#endif

/// Create the shared serial console once and register receive wakeups.
void consoleInit()
{
Expand Down Expand Up @@ -88,6 +98,30 @@ SerialConsole::SerialConsole() : StreamAPI(&Port), RedirectablePrint(&Port), con
/// Service one serial API iteration and select the next polling interval.
int32_t SerialConsole::runOnce()
{
#ifdef MESHTASTIC_PHONEAPI_ACCESS_CONTROL
// Lockdown (nRF52) builds only. The SerialConsole is a process-lifetime
// singleton, so its inherited PhoneAPI object - and therefore its entry in
// the per-connection admin-auth slot table (keyed by PhoneAPI*) - is reused
// for every USB/serial client for the whole boot. Nothing re-locks that slot
// when the operator unplugs and a different client plugs in before the
// 15-minute inactivity timeout fires, so a fresh client would inherit the
// prior operator's admin authorization. Re-lock when the physical USB-CDC link
// drops - the serial analog of the BLE onDisconnect() -> close() session reset.
//
// On the nRF52 TinyUSB (Adafruit) core, (bool)Port == tud_cdc_n_connected():
// it goes false on cable unplug or host port-close (DTR de-assert). close()
// frees the auth slot and resets PhoneAPI state, so whoever connects next
// re-locks via handleStartConfig()'s !isConnected() branch on their first
// want_config - the same physical-link boundary BLE enforces in onConnect().
// Console transports without a real DTR line (e.g. a UART USER_DEBUG_PORT) hold
// this constant, so no edge fires and we fall back to the existing inactivity
// timeout - no worse than the pre-fix behavior.
const bool linkUp = static_cast<bool>(Port);
if (s_serialLinkUp && !linkUp)
close();
s_serialLinkUp = linkUp;
#endif

#ifdef HELTEC_MESH_SOLAR
// After enabling the mesh solar serial port module configuration, command processing is handled by the serial port module.
if (moduleConfig.serial.enabled && moduleConfig.serial.override_console_serial_port &&
Expand Down
Loading