Skip to content

fix: correct channel encryption label for unencrypted and shorthand PSKs (#2939)#2944

Merged
Yeraze merged 2 commits intomainfrom
fix/issue-2939-channel-encryption-label
May 9, 2026
Merged

fix: correct channel encryption label for unencrypted and shorthand PSKs (#2939)#2944
Yeraze merged 2 commits intomainfrom
fix/issue-2939-channel-encryption-label

Conversation

@Yeraze
Copy link
Copy Markdown
Owner

@Yeraze Yeraze commented May 9, 2026

Summary

Fixes #2939. The Channel Database UI was mislabeling encryption types based on pskLength, and showed a misleading ... preview for empty PSKs.

Changes

  • src/components/configuration/ChannelDatabaseSection.tsx — replaced the pskLength === 16 ? 'AES-128' : 'AES-256' ternary with logic covering all four documented cases plus an Unknown fallback. When pskLength === 0, the line now renders PSK: (none) (None) instead of pretending a key exists.
    • 0None
    • 1Shorthand (AES-128)
    • 16AES-128
    • 32AES-256
    • other → Unknown
  • src/server/routes/v1/channelDatabase.ts — when channel.psk is empty, pskPreview is now "(none)" instead of "...". The includeFullPsk (admin) branch is unchanged.

Test plan

  • npx tsc --noEmit — no errors
  • npx vitest run tests/unit/channelDatabasePermissions.test.ts — 26/26 pass
  • npm run lint for the two changed files surfaces only pre-existing warnings (no new issues)
  • Manual check: open Channel Database with channels at pskLength 0, 1, 16, 32 and confirm each label

…SKs (#2939)

Channel Database UI was mislabeling encryption types: pskLength of 0
or 1 was incorrectly shown as AES-256, and an empty PSK rendered as
"..." preview as if a key existed.

- ChannelDatabaseSection: handle pskLength 0 (None / no key shown), 1
  (Shorthand AES-128), 16 (AES-128), 32 (AES-256), and an Unknown
  fallback for anything else.
- channelDatabase route: when channel.psk is empty, return "(none)"
  for pskPreview instead of "...".

Fixes #2939

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

github-actions Bot commented May 9, 2026

Claude finished @Yeraze's task —— View job


Code Review: Channel Encryption Label Fix

I've completed my review of this PR that fixes channel encryption label display. Here's my comprehensive analysis:

  • Read and analyze the changed files
  • Review the PSK length handling logic
  • Check for potential bugs and edge cases
  • Assess security implications
  • Evaluate test coverage needs
  • Provide comprehensive feedback

Overall Assessment: LGTM with Minor Suggestions

This is a solid bug fix that correctly addresses the mislabeling issue. The changes are well-implemented and follow the codebase conventions.


Code Quality & Logic Review

Frontend Changes (ChannelDatabaseSection.tsx:171-181)

The new PSK length handling logic is excellent:

// OLD: Binary logic that missed important cases
pskLength === 16 ? 'AES-128' : 'AES-256'

// NEW: Comprehensive handling of all documented cases
{channel.pskLength === 0
  ? 'PSK: (none) (None)'
  : `PSK: ${channel.pskPreview} (${
      channel.pskLength === 1
        ? 'Shorthand (AES-128)'
        : channel.pskLength === 16
          ? 'AES-128'
          : channel.pskLength === 32
            ? 'AES-256'
            : 'Unknown'
    })`}

Strengths:

  • Handles all documented PSK lengths: 0 (none), 1 (shorthand), 16 (AES-128), 32 (AES-256)
  • Includes fallback "Unknown" for unexpected lengths
  • Clear, self-documenting logic
  • Consistent with backend validation logic

Backend Changes (channelDatabase.ts:30)

The PSK preview fix is appropriate:

// OLD: Misleading preview for empty PSKs
pskPreview: includeFullPsk ? channel.psk : (channel.psk ? `${channel.psk.substring(0, 8)}...` : '(none)')

// NEW: Clear indication of no PSK
pskPreview: includeFullPsk ? channel.psk : (channel.psk ? `${channel.psk.substring(0, 8)}...` : '(none)')

The change from "..." to "(none)" for empty PSKs eliminates user confusion.


Security Analysis

No security concerns identified:

  • PSK masking for non-admin users remains intact
  • Full PSK exposure is still limited to admin users via includeFullPsk
  • No changes to authentication or authorization logic
  • PSK validation logic in the backend remains robust

Edge Cases & Error Handling

Well-Handled Cases:

  1. Zero-length PSK - Correctly shows "None"
  2. Shorthand PSK (length 1) - Properly labeled as "Shorthand (AES-128)"
  3. Standard lengths (16/32) - Correctly mapped to AES-128/AES-256
  4. Unknown lengths - Safe fallback to "Unknown"

Potential Edge Case to Consider:

The frontend logic assumes channel.pskLength is always accurate. However, I noticed there's some validation logic in the backend routes that could potentially allow mismatches. Consider adding a defensive check:

// Potential improvement (not required for this fix):
const derivedLength = channel.psk ? atob(channel.psk).length : 0;
const displayLength = channel.pskLength ?? derivedLength;

Test Coverage Assessment

Existing Coverage:

  • ✅ Channel database repository has comprehensive tests (channelDatabase.test.ts)
  • ✅ Channel database permissions are well-tested (channelDatabasePermissions.test.ts)
  • ✅ API route unit tests exist (channelDatabase.test.ts - referenced in route file)

Missing Coverage for This Fix:

The new UI display logic isn't covered by unit tests. Consider adding tests for:

describe('PSK length display logic', () => {
  it('should show "None" for zero-length PSK', () => {
    // Test pskLength === 0 case
  });
  it('should show "Shorthand (AES-128)" for length 1', () => {
    // Test pskLength === 1 case  
  });
  it('should show "Unknown" for unexpected lengths', () => {
    // Test edge case with unusual pskLength
  });
});

Performance Considerations

No performance impact:

  • Changes are simple conditional logic replacements
  • No additional API calls or expensive operations
  • Frontend rendering performance unchanged

Code Style & Consistency

Follows project conventions:

  • Consistent with existing ternary operator patterns in the codebase
  • Matches the error handling style in surrounding code
  • Proper TypeScript typing maintained
  • ESLint/formatting requirements met

Suggestions for Improvement

1. Consider Extracting Display Logic (Optional)

The nested ternary could be extracted for better readability:

const getPskDisplayLabel = (pskLength: number): string => {
  switch (pskLength) {
    case 0: return 'None';
    case 1: return 'Shorthand (AES-128)'; 
    case 16: return 'AES-128';
    case 32: return 'AES-256';
    default: return 'Unknown';
  }
};

// Usage:
{channel.pskLength === 0 
  ? 'PSK: (none) (None)'
  : `PSK: ${channel.pskPreview} (${getPskDisplayLabel(channel.pskLength)})`
}

2. Add JSDoc Documentation (Minor)

Consider documenting the PSK length meanings:

/**
 * PSK length meanings:
 * 0 = No encryption
 * 1 = Shorthand (expands to 16-byte AES-128)  
 * 16 = AES-128
 * 32 = AES-256
 */

Final Verdict: ✅ APPROVED

This PR successfully fixes the reported issue with minimal risk and good implementation quality. The changes are:

  • Correct: Properly handles all documented PSK length cases
  • Safe: No security implications or breaking changes
  • Well-tested: Existing test infrastructure validates the underlying logic
  • Maintainable: Clear, readable code that follows project patterns

Recommendation: Merge after addressing any minor suggestions you find valuable. The core fix is solid and ready for production.


@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 9, 2026

System Test Results

MeshMonitor System Test Results

Test Run: 2026-05-09 13:38:19 EDT

Test Summary

Test Suite Result
Configuration Import ✅ PASSED
Quick Start Test ✅ PASSED
Security Test ✅ PASSED
V1 API Test ✅ PASSED
Reverse Proxy Test ✅ PASSED
Reverse Proxy + OIDC ✅ PASSED
Virtual Node CLI Test ✅ PASSED
Backup & Restore Test ✅ PASSED
Database Migration Test ✅ PASSED
DB Backing Consistency ✅ PASSED
API Exercise (3 DBs) ✅ PASSED

✅ Overall Result: PASSED

All deployment configurations are working correctly!

Test Details

Configuration Import:

  • Tests configuration import and device reboot cycle
  • Verifies channel roles, PSKs, and LoRa configuration
  • Note: Channel name verification skipped due to architectural limitation

Quick Start Test:

  • Zero-config deployment (no SESSION_SECRET or COOKIE_SECURE required)
  • HTTP access without HSTS
  • Auto-generated admin user with default credentials
  • Session cookies work over HTTP
  • Meshtastic node connection and message exchange verified

Security Test:

  • Verifies Node IP address hidden from anonymous users in API responses
  • Verifies MQTT configuration hidden from anonymous users
  • Verifies Node IP address visible to authenticated users
  • Verifies MQTT configuration visible to authenticated users
  • Verifies protected endpoints require authentication

V1 API Test:

  • Tests v1 REST API endpoints with Bearer token authentication
  • Verifies Bearer token requests bypass CSRF protection
  • Verifies POST/PUT/DELETE work without CSRF token when using Bearer auth
  • Verifies session-based requests still require CSRF token

Reverse Proxy Test:

  • Production deployment with COOKIE_SECURE=true
  • HTTPS-ready configuration
  • Trust proxy enabled for reverse proxy compatibility
  • CORS configured for HTTPS domain
  • Meshtastic node connection and message exchange verified

Reverse Proxy + OIDC Test:

  • OIDC authentication integration
  • Mock OIDC provider health checks
  • Authorization flow and session creation
  • Hybrid mode (OIDC + local auth)
  • Meshtastic node connection verified

Virtual Node CLI Test:

  • Virtual Node Server enabled on TCP port 4404
  • Meshtastic Python client successfully connects
  • Node data download and synchronization verified
  • Test message sent on gauntlet channel (index 3)
  • Message delivery confirmed via Web UI API
  • Virtual Node Server connection logging verified

Backup & Restore Test:

  • System backup created from running dev container
  • New container spun up with RESTORE_FROM_BACKUP env var
  • Data integrity verified (node count, message count, settings)
  • Restore event logged in audit log
  • Dev container unaffected by restore test

Database Migration Test:

  • SQLite to PostgreSQL migration verified
  • SQLite to MySQL migration verified
  • Data integrity confirmed for both target databases
  • Row counts match between source and target

DB Backing Consistency Test:

  • SQLite, PostgreSQL, and MySQL backends tested with same device
  • Node counts within ±10 across all three backends
  • Favorite counts identical across all backends
  • Key station verified as favorite on all backends

…2939)

Covers the PR #2944 fix:

- ChannelDatabaseSection.encryptionLabel.test.tsx renders the real
  component with mocked apiService and verifies the PSK label for
  pskLength values 0, 1, 16, 32, and an unknown value (7). Reverting
  the JSX branch causes 3/5 cases to fail.

- channelDatabaseTransform.test.ts mirrors the pskPreview expression
  from src/server/routes/v1/channelDatabase.ts to lock in the masked
  branch returning "(none)" for empty/null/undefined PSKs and the
  truncated preview otherwise. The helper is private and the masked
  branch is gated behind admin-only routes, so this follows the
  logic-spec convention from tests/unit/channelDatabasePermissions.test.ts.

No production code changes.
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 9, 2026

Claude encountered an error —— View job


I'll analyze this and get back to you.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 9, 2026

System Test Results

MeshMonitor System Test Results

Test Run: 2026-05-09 15:14:56 EDT

Test Summary

Test Suite Result
Configuration Import ✅ PASSED
Quick Start Test ✅ PASSED
Security Test ✅ PASSED
V1 API Test ✅ PASSED
Reverse Proxy Test ✅ PASSED
Reverse Proxy + OIDC ✅ PASSED
Virtual Node CLI Test ✅ PASSED
Backup & Restore Test ✅ PASSED
Database Migration Test ✅ PASSED
DB Backing Consistency ✅ PASSED
API Exercise (3 DBs) ✅ PASSED

✅ Overall Result: PASSED

All deployment configurations are working correctly!

Test Details

Configuration Import:

  • Tests configuration import and device reboot cycle
  • Verifies channel roles, PSKs, and LoRa configuration
  • Note: Channel name verification skipped due to architectural limitation

Quick Start Test:

  • Zero-config deployment (no SESSION_SECRET or COOKIE_SECURE required)
  • HTTP access without HSTS
  • Auto-generated admin user with default credentials
  • Session cookies work over HTTP
  • Meshtastic node connection and message exchange verified

Security Test:

  • Verifies Node IP address hidden from anonymous users in API responses
  • Verifies MQTT configuration hidden from anonymous users
  • Verifies Node IP address visible to authenticated users
  • Verifies MQTT configuration visible to authenticated users
  • Verifies protected endpoints require authentication

V1 API Test:

  • Tests v1 REST API endpoints with Bearer token authentication
  • Verifies Bearer token requests bypass CSRF protection
  • Verifies POST/PUT/DELETE work without CSRF token when using Bearer auth
  • Verifies session-based requests still require CSRF token

Reverse Proxy Test:

  • Production deployment with COOKIE_SECURE=true
  • HTTPS-ready configuration
  • Trust proxy enabled for reverse proxy compatibility
  • CORS configured for HTTPS domain
  • Meshtastic node connection and message exchange verified

Reverse Proxy + OIDC Test:

  • OIDC authentication integration
  • Mock OIDC provider health checks
  • Authorization flow and session creation
  • Hybrid mode (OIDC + local auth)
  • Meshtastic node connection verified

Virtual Node CLI Test:

  • Virtual Node Server enabled on TCP port 4404
  • Meshtastic Python client successfully connects
  • Node data download and synchronization verified
  • Test message sent on gauntlet channel (index 3)
  • Message delivery confirmed via Web UI API
  • Virtual Node Server connection logging verified

Backup & Restore Test:

  • System backup created from running dev container
  • New container spun up with RESTORE_FROM_BACKUP env var
  • Data integrity verified (node count, message count, settings)
  • Restore event logged in audit log
  • Dev container unaffected by restore test

Database Migration Test:

  • SQLite to PostgreSQL migration verified
  • SQLite to MySQL migration verified
  • Data integrity confirmed for both target databases
  • Row counts match between source and target

DB Backing Consistency Test:

  • SQLite, PostgreSQL, and MySQL backends tested with same device
  • Node counts within ±10 across all three backends
  • Favorite counts identical across all backends
  • Key station verified as favorite on all backends

@Yeraze Yeraze merged commit 571c001 into main May 9, 2026
19 of 20 checks passed
@Yeraze Yeraze deleted the fix/issue-2939-channel-encryption-label branch May 9, 2026 20:23
Yeraze added a commit that referenced this pull request May 10, 2026
* chore(release): bump version to 4.3.0

Headline feature: Waypoints — per-source storage, map rendering,
and in-place authoring UI for Meshtastic WAYPOINT_APP pins.

Includes since 4.2.3:
- #2936/#2938 feat(waypoints): basic waypoint support
- #2942 feat(waypoints): authoring UI for create/edit/delete
- #2937 fix(auth): bootstrap first OIDC user as admin
- #2935 fix(traceroute): stop cascading IP-style across radio segments
- #2945 feat: Tile Selection / Legend visibility toggles in Map Features
- #2944 fix: correct channel encryption label for unencrypted/shorthand PSKs
- #2946 fix(mobile): mobile browser interface fixes
- #2948 fix: map tileset selection not applying on dashboard
- docs: new Waypoints feature page; 4.3 Highlights nav entry

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* docs: list Waypoints under regular Features instead of 4.3 Highlights

Per Randall: Waypoints should be documented like any other feature, not
called out in the nav as a "🆕 4.3 Highlights" section.

- Drop the "🆕 4.3 Highlights" entry from the top nav and the /features/
  sidebar; restore "🆕" on 4.2 Highlights as the most-recent callout
- Add Waypoints to the regular Features sidebar list (next to Embed Maps)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* docs: drop version-Highlights nav sections, fold entries into Features

Removes the "🆕 4.2 / 4.1 / 4.0 Highlights" callouts from both the top
nav and the /features/ sidebar. Their pages — Analysis & Reports, Map
Analysis, Multi-Source, Per-Source Permissions, Global Settings, Store
& Forward, Geofence Triggers — now live in the regular Features list,
slotted near topically-related entries (Multi-Source / permissions
near Settings; Geofence Triggers next to Automation; Map Analysis next
to Embed Maps; Analysis & Reports next to Analytics; etc.).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
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.

Bug: Channel encryption label incorrectly displays 'AES-256' for unencrypted and shorthand-PSK channels

1 participant