Skip to content

fix(mqtt): stamp node.channel on ingest so the map honors VC permissions#3110

Merged
Yeraze merged 2 commits into
mainfrom
fix/mqtt-node-channel
May 20, 2026
Merged

fix(mqtt): stamp node.channel on ingest so the map honors VC permissions#3110
Yeraze merged 2 commits into
mainfrom
fix/mqtt-node-channel

Conversation

@Yeraze

@Yeraze Yeraze commented May 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Summary

#3108 routed MQTT message ingest through channel_database and stamped message.channel = CHANNEL_DB_OFFSET + dbId. The matching changes on the node and neighbor-info / traceroute endpoints were missed, so non-admin / anonymous viewers either saw nothing on the map or — worse — saw line segments rendered between coordinates of nodes they had no permission to see ("floating lines").

What this PR fixes

  1. MQTT NODEINFO/POSITION ingest didn't stamp node.channel. mqttIngestion.ts left the column NULL, so the map node filter fell back to permissions[channel_0] — a slot grant the feat(mqtt): route MQTT channel permissions through channel_database #3108 UI hides for MQTT scopes. Result: anonymous and non-admin viewers couldn't see any MQTT nodes regardless of what they granted.
  2. /api/sources/:id/traceroutes returned rows unfiltered. Each row carries a routePositions JSON snapshot of hop coordinates. The frontend's useTraceroutePaths hook prefers that snapshot over the live nodes list, so it drew lines for traceroutes the user had no channel access to — coordinates leaked, lines floated.
  3. /api/sources/:id/neighbor-info enriched records with positions from getNodesByNums with no channel check. Same leak shape: lines drawn between coordinates of nodes the user shouldn't see.

Fix

  • mqttIngestion.ts: pass channel: effectiveChannel into the NODEINFO_APP and POSITION_APP upsertNode calls. Existing rows with channel=NULL recover as each node re-broadcasts NODEINFO.
  • sourceRoutes.ts /traceroutes: apply maskTraceroutesByChannel (already existed in nodeEnhancer.ts) to the response.
  • sourceRoutes.ts /neighbor-info: build a visible-node set via filterNodesByChannelPermission and drop neighbor records whose endpoints aren't in it.

Tests

  • 3 new tests in mqttIngestion.test.ts pinning the node.channel stamping (NODEINFO, POSITION, raw-slot fallback).
  • 1 new regression test in sourceRoutes.neighbor-info.test.ts for the channel-gate.
  • New sourceRoutes.traceroutes.test.ts covering admin-pass, no-grant-drop, and partial-grant scenarios.
  • Full Vitest suite: 10158 pass / 0 fail.

Test plan

  • Pull and deploy on a system with an MQTT source.
  • Confirm new NODEINFO arrivals land with nodes.channel = CHANNEL_DB_OFFSET + dbId (not NULL or 0).
  • Grant Anonymous viewOnMap on the corresponding Virtual Channel Permissions entry.
  • Reload the map as an anonymous viewer → nodes now show.
  • Without VC grants, confirm zero MQTT nodes appear AND no traceroute/neighbor lines float in empty space.

🤖 Generated with Claude Code

@Yeraze @claude
fix(mqtt): stamp node.channel on ingest so the map filter honors VC p…
41fcc24 
…erms

#3108 routed MQTT message ingest through `channel_database` and stamped
`message.channel = CHANNEL_DB_OFFSET + dbId`. The matching change to
the node-upsert paths was missed: `NODEINFO_APP` and `POSITION_APP`
built `Partial<DbNode>` rows without setting `channel`, so the column
stayed NULL.

`filterNodesByChannelPermission` in `nodeEnhancer.ts` then evaluated
`node.channel ?? 0` for every MQTT-sourced node, falling back to slot
0 and requiring `permissions[channel_0]?.viewOnMap`. But the #3108 UI
hides the channel_0..7 toggles for MQTT scopes and directs admins to
Virtual Channel Permissions — leaving no way to grant map access.
Result: anonymous and non-admin users saw "No nodes visible" on the
map for every MQTT source, no matter what they granted.

Fix: pass `channel: effectiveChannel` into the upsertNode calls in the
NODEINFO_APP and POSITION_APP branches, matching the message-side
encoding. Existing rows that already have channel=NULL recover as
each node re-broadcasts NODEINFO (typically every few hours under
default firmware config); no migration backfill needed for short-
term correctness.

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

github-actions Bot commented May 20, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude finished @Yeraze's task —— View job

Code Review Complete ✅

Reviewing PR #3110: fix(mqtt): stamp node.channel on ingest so the map honors VC permissions

Todo List

  • Analyze the code changes in mqttIngestion.ts
  • Review the test additions in mqttIngestion.test.ts
  • Check for potential bugs or edge cases
  • Evaluate security and performance considerations
  • Assess test coverage completeness
  • Provide comprehensive feedback

Code Quality & Best Practices ⭐

Excellent implementation! The fix is precise, well-targeted, and maintains consistency with existing patterns:

  1. Consistency: The fix correctly applies the same effectiveChannel encoding used for messages and traceroutes to node upserts
  2. Clarity: The extensive comments in both functions clearly explain why the channel stamping is necessary for permission filtering
  3. Code Style: Follows existing patterns perfectly - the implementation mirrors the comment style and variable naming conventions used throughout the file

Bug Analysis 🔍

No bugs detected. The implementation is correct:

  1. Root cause addressed: The fix targets exactly the issue described - missing channel values on NODEINFO_APP and POSITION_APP node upserts
  2. Fallback handling: Proper fallback to raw slot when channelDatabaseId is null (lines mqttIngestion.ts:189-190)
  3. Data integrity: No risk of data corruption - the channel value is computed the same way for all packet types

Performance Considerations 🚀

No performance concerns:

  1. Minimal overhead: The fix reuses the already-computed effectiveChannel value - no additional database calls
  2. Efficient caching: The channel name resolution already has proper caching mechanisms (channelNameToDbIdCache)
  3. Scope: Changes are surgical and don't impact any hot paths

Security Review 🔒

Security-positive change:

  1. Permission enforcement: The fix actually improves security by ensuring MQTT nodes are properly subjected to Virtual Channel Permission filtering
  2. No new attack vectors: The change only adds a computed field to existing upsert operations
  3. Source isolation: Maintains proper sourceId scoping throughout

Test Coverage Assessment 🧪

Comprehensive and high-quality tests:

  1. Critical scenarios covered: Tests verify channel stamping for both NODEINFO_APP (line 588) and POSITION_APP (line 607)
  2. Edge case handling: Test for null fallback behavior (line 620) ensures robustness
  3. Integration testing: Tests exercise the full ingestion pipeline, not just isolated functions
  4. Clear assertions: Each test verifies the exact expected behavior with specific channel values

Test quality highlights:

  • Uses CHANNEL_DB_OFFSET + id assertions matching the production encoding
  • Properly mocks the findOrCreatePassiveByNameAsync dependency
  • Tests both success and fallback paths

Minor Suggestions

  1. Documentation: Consider adding a brief comment in the main ingestServiceEnvelope function (around line 187) explaining the channel resolution strategy for future maintainers

  2. Test naming: The test descriptions are excellent - they clearly describe the regression being prevented

Overall Assessment

This is a high-quality fix that:

  • ✅ Correctly addresses the described permission filtering issue
  • ✅ Maintains code consistency and follows established patterns
  • ✅ Has excellent test coverage with meaningful assertions
  • ✅ Improves security by ensuring proper permission enforcement
  • ✅ Introduces no performance or reliability concerns

Recommendation: Approve and merge 🚢

Job Run

@Yeraze @claude
fix(sources): channel-gate traceroutes and neighbor-info on the map
5052671 
Companion to the node.channel stamping fix in the same PR. With nodes
correctly filtered out for users who lack viewOnMap on the channel,
two endpoints still leaked enough position data to render lines:

1. GET /api/sources/:id/traceroutes returned every row including the
   embedded `routePositions` JSON snapshot. The frontend's
   `useTraceroutePaths` hook prefers that snapshot over the live nodes
   list, so it drew line segments between coordinates of nodes the
   user had no permission to see. Apply `maskTraceroutesByChannel` to
   the response.
2. GET /api/sources/:id/neighbor-info enriched each record with
   positions pulled from `getNodesByNums` without checking channel
   access. Build the visible-node set via `filterNodesByChannelPermission`
   and drop neighbor records whose endpoints aren't in it.

User-visible symptom this fixes: anonymous viewers seeing "lines
floating in space" on the map for MQTT sources whose nodes had been
correctly filtered out of the nodes endpoint.

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

github-actions Bot commented May 20, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude finished @Yeraze's task —— View job

Code Review Complete ✅

Reviewing PR #3110: fix(mqtt): stamp node.channel on ingest so the map honors VC permissions

Todo List

  • Analyze the code changes in mqttIngestion.ts
  • Review the test additions in mqttIngestion.test.ts
  • Check for potential bugs or edge cases
  • Evaluate security and performance considerations
  • Assess test coverage completeness
  • Provide comprehensive feedback

Code Quality & Best Practices ⭐

Excellent implementation! The fix is precise, well-targeted, and maintains consistency with existing patterns:

  1. Consistency: The fix correctly applies the same effectiveChannel encoding used for messages and traceroutes to node upserts
  2. Clarity: The extensive comments in both functions clearly explain why the channel stamping is necessary for permission filtering
  3. Code Style: Follows existing patterns perfectly - the implementation mirrors the comment style and variable naming conventions used throughout the file

Bug Analysis 🔍

No bugs detected. The implementation is correct:

  1. Root cause addressed: The fix targets exactly the issue described - missing channel values on NODEINFO_APP and POSITION_APP node upserts
  2. Fallback handling: Proper fallback to raw slot when channelDatabaseId is null (lines mqttIngestion.ts:189-190)
  3. Data integrity: No risk of data corruption - the channel value is computed the same way for all packet types

Performance Considerations 🚀

No performance concerns:

  1. Minimal overhead: The fix reuses the already-computed effectiveChannel value - no additional database calls
  2. Efficient caching: The channel name resolution already has proper caching mechanisms (channelNameToDbIdCache)
  3. Scope: Changes are surgical and don't impact any hot paths

Security Review 🔒

Security-positive change:

  1. Permission enforcement: The fix actually improves security by ensuring MQTT nodes are properly subjected to Virtual Channel Permission filtering
  2. No new attack vectors: The change only adds a computed field to existing upsert operations
  3. Source isolation: Maintains proper sourceId scoping throughout

Test Coverage Assessment 🧪

Comprehensive and high-quality tests:

  1. Critical scenarios covered: Tests verify channel stamping for both NODEINFO_APP (line 588) and POSITION_APP (line 607)
  2. Edge case handling: Test for null fallback behavior (line 620) ensures robustness
  3. Integration testing: Tests exercise the full ingestion pipeline, not just isolated functions
  4. Clear assertions: Each test verifies the exact expected behavior with specific channel values

Test quality highlights:

  • Uses CHANNEL_DB_OFFSET + id assertions matching the production encoding
  • Properly mocks the findOrCreatePassiveByNameAsync dependency
  • Tests both success and fallback paths

Minor Suggestions

  1. Documentation: Consider adding a brief comment in the main ingestServiceEnvelope function (around line 187) explaining the channel resolution strategy for future maintainers

  2. Test naming: The test descriptions are excellent - they clearly describe the regression being prevented

Overall Assessment

This is a high-quality fix that:

  • ✅ Correctly addresses the described permission filtering issue
  • ✅ Maintains code consistency and follows established patterns
  • ✅ Has excellent test coverage with meaningful assertions
  • ✅ Improves security by ensuring proper permission enforcement
  • ✅ Introduces no performance or reliability concerns

Recommendation: Approve and merge 🚢

@Yeraze Yeraze merged commit 6373b78 into main May 20, 2026
20 checks passed
@Yeraze Yeraze deleted the fix/mqtt-node-channel branch May 20, 2026 18:16
@Yeraze Yeraze mentioned this pull request May 20, 2026
Merged
4 tasks
Yeraze added a commit that referenced this pull request May 20, 2026
@Yeraze @claude
chore(release): bump version to 4.6.3 (#3111)
f63d708 
* chore(release): bump version to 4.6.3

Five user-visible files bumped per the CLAUDE.md version recipe:
package.json, package-lock.json (regenerated), helm/meshmonitor/Chart.yaml,
desktop/src-tauri/tauri.conf.json, desktop/package.json. CLAUDE.md
banner line bumped to match.

CHANGELOG entry covers the five PRs since 4.6.2-1:
- #3105 unified tapback metadata fix
- #3106 docs: drop worktree restriction
- #3107 meshcore contact advType persistence
- #3108 MQTT channel permissions via channel_database
- #3109 hint banner Catppuccin restyle
- #3110 node.channel ingest + traceroute/neighbor channel gate

Companion blog post (docs/blog/2026-05-20-v4.6.3-permissions.md)
walks operators through the new Virtual Channel Permissions flow,
the map-visibility behavior change, and the floating-lines fix.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore(release): regenerate desktop/package-lock.json for 4.6.3

The desktop sub-project carries its own lockfile and the bump to 4.6.3
left it pinned to 4.6.1. The Windows Desktop CI job runs `npm install`
without `--legacy-peer-deps` and fails on the package.json /
package-lock.json version mismatch. Regenerate to match.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@Yeraze