Skip to content

fix: prevent local node from self-flagging security warnings#2407

Merged
Yeraze merged 1 commit intomainfrom
fix/local-node-self-targeting
Mar 23, 2026
Merged

fix: prevent local node from self-flagging security warnings#2407
Yeraze merged 1 commit intomainfrom
fix/local-node-self-targeting

Conversation

@Yeraze
Copy link
Owner

@Yeraze Yeraze commented Mar 23, 2026

Summary

Three issues where the locally connected node could incorrectly target itself in automated operations:

1. Security flag self-targeting

NO_CHANNEL and PKI error handlers could flag the local node with keyMismatchDetected, showing a spurious "Possible key or channel mismatch" warning on the local node. Added toNodeId !== localNodeId / targetNodeId !== localNodeId guards to all four error paths.

2. Link quality bouncing

updateLinkQualityForMessage processed echoed packets from the local node, causing its link quality score to bounce between 8 and 10. Now skips the local node since our own echoed packets aren't meaningful for link quality.

3. Startup cleanup

processMyNodeInfo now clears any stale keyMismatchDetected / keySecurityIssueDetails flags on the local node when it connects, cleaning up any flags that were erroneously set before these guards were in place.

Full audit results

All 10 automated systems verified for local-node exclusion — see PR description for details.

Changes

File Change
src/server/meshtasticManager.ts Guards on 4 error paths, link quality skip, startup cleanup

Test plan

  • npx vitest run — 3070 tests pass
  • npm run build — no TypeScript errors
  • Deploy, verify local node no longer shows security warning
  • Verify local node link quality stabilizes

🤖 Generated with Claude Code

… quality bouncing

Three issues where the locally connected node could incorrectly target itself:

1. NO_CHANNEL and PKI error handlers could flag the local node with
   keyMismatchDetected — added localNodeId guards to all four error paths

2. Link quality updates processed echoed packets from the local node,
   causing the score to bounce between 8 and 10 — skip local node in
   updateLinkQualityForMessage

3. Startup cleanup: processMyNodeInfo now clears any stale security flags
   on the local node when it connects

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link

github-actions bot commented Mar 23, 2026

Claude finished @Yeraze's task —— View job


Code Review Complete

Todo List:

  • Read and analyze the changed file (src/server/meshtasticManager.ts)
  • Evaluate the three fixes implemented
  • Check for code quality and best practices
  • Identify potential bugs or security concerns
  • Assess performance implications
  • Review test coverage
  • Provide comprehensive feedback

Review Summary

This PR addresses three important issues where the local node could incorrectly target itself in automated operations. The implementation is well-designed and appears correct.

Code Quality & Best Practices ✅

Strengths:

  • Clear guard conditions: All checks use explicit !== localNodeId comparisons
  • Consistent pattern: Same guard approach applied across all 4 error handling paths
  • Descriptive logging: Clear log messages explain what's happening (🔐 Clearing erroneous security flags on local node)
  • Defensive programming: Proactive cleanup in processMyNodeInfo handles legacy state

Security Analysis ✅

No security concerns identified:

  • Guards prevent false security warnings that could confuse users
  • Local node self-targeting exclusions are logically sound (a node cannot have a key mismatch with itself)
  • No credentials or sensitive data exposure

Performance Considerations ✅

Minimal performance impact:

  • Link quality skip adds one simple comparison per message (fromNum !== this.localNodeInfo.nodeNum)
  • Security flag guards only execute during error conditions (rare)
  • Startup cleanup is one-time per connection

Implementation Analysis ✅

Fix 1: Security Flag Self-Targeting

// Before: Could flag local node
if (isPkiError(errorReason) && fromNodeId === localNodeId) {

// After: Excludes local node
if (isPkiError(errorReason) && fromNodeId === localNodeId && toNodeId !== localNodeId) {

✅ Correct: Guards added to all 4 error paths (src/server/meshtasticManager.ts:5364, 5383, 5413, 5439)

Fix 2: Link Quality Bouncing

// Before: Processed all messages including local echoes
this.updateLinkQualityForMessage(fromNum, messageHops);

// After: Skips local node
if (!this.localNodeInfo || fromNum !== this.localNodeInfo.nodeNum) {
  this.updateLinkQualityForMessage(fromNum, messageHops);
}

✅ Logical: Local node echoes aren't meaningful for link quality measurement

Fix 3: Startup Cleanup

// Clear any erroneous security flags on the local node
if (existingNode?.keyMismatchDetected || existingNode?.keySecurityIssueDetails) {
  // Reset flags and emit update
}

✅ Defensive: Cleans up any stale flags that may exist from before these guards

Potential Issues & Suggestions

Minor: Error Message Consistency

The error descriptions could be more consistent:

  • Line 5367: "your radio's stored key for this node may be outdated"
  • Line 5387: "NO_CHANNEL error on request - target node rejected the message"

Suggestion: Consider standardizing the tone and technical level across error messages.

Test Coverage Gap ⚠️

Missing test coverage for the core scenarios:

  • Local node self-flagging prevention
  • Link quality bouncing with local echo packets
  • Startup cleanup of stale flags

Recommendation: Add unit tests covering:

describe('Local node self-targeting prevention', () => {
  it('should not flag local node with PKI errors')
  it('should not flag local node with NO_CHANNEL errors') 
  it('should skip link quality updates for local node packets')
  it('should clear stale security flags on startup')
})

Edge Case Analysis ✅

Verified protection against:

  • localNodeInfo being undefined (proper null checks)
  • Multiple error conditions in the same session
  • Race conditions during startup (cleanup runs in processMyNodeInfo)

Final Assessment

Overall: High quality fix

  • Addresses real user-facing issues
  • Implementation is correct and consistent
  • No security or performance concerns
  • Could benefit from additional test coverage

The PR effectively solves the stated problems with a clean, defensive approach that should eliminate spurious security warnings and link quality bouncing for local nodes.


@Yeraze Yeraze merged commit 804d942 into main Mar 23, 2026
17 checks passed
@Yeraze Yeraze deleted the fix/local-node-self-targeting branch March 23, 2026 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant