ledadmin/meshcentral-openclaw-bridge
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
eb833dc990fef656e587bf7d6d6d3ee1dc5b2b3d
meshcentral-openclaw-bridge/review/REVIEW.md
T

18 KiB
Raw Blame History

Review Report: MeshCentral ↔ OpenClaw Bridge

Review Date: 2026-08-24
Reviewer: Independent subagent (clawdia review flow)
Scope: Implementation under /home/ledadmin/projects/meshcentral-openclaw-bridge/implementation/
Reference Docs: PROJECT.md, SECURITY.md, research/INTEGRATION-RESEARCH.md

Executive Summary

The implementation provides a well-structured scaffold for the MeshCentral ↔ OpenClaw bridge with solid contracts, typed schemas via Zod, and comprehensive mock-based testing. However, the critical "real" adapter is completely unimplemented, leaving only the mock path operational. The current codebase has three categories of issues:

  1. Critical: Unimplemented real adapter (MeshCentralRealAdapter) with dangerous defaults (insecure TLS, no auth).
  2. High: Test failures indicate missing error message alignment; linting configuration incomplete.
  3. Medium: Diagnostic output sanitization and prompt-injection protection not addressed.

Test Results:

  • 50 tests pass
  • 2 tests fail (expectation mismatch on error message string, not logic)
  • npm run build fails with TypeScript errors in test setup
  • npm run lint fails due to missing ESLint configuration

Recommendation: Do not proceed with live MeshCentral integration until the real adapter is implemented according to security requirements and all test/lint failures are resolved.

Severity-Ranked Findings

🔴 CRITICAL: Real adapter unimplemented with dangerous TLS defaults

Location: src/adapter.ts:35-48, healthCheck() method
Severity: Critical

The MeshCentralRealAdapter.healthCheck() method uses raw fetch with no TLS validation options:

async healthCheck(): Promise<boolean> {
  try {
    const response = await fetch(`${this.config.meshCentralUrl}/api`, {
      method: 'GET',
      headers: { 'Content-Type': 'application/json' },
    });
    return response.ok;
  } catch {
    return false;
  }
}

Issues:

  • No certificate verification (relies on Node.js default, which may not validate against internal CA).
  • fetch does not support WebSocket connections; MeshCentral's control API uses wss://.../control.ashx, not /api.
  • Research found upstream meshctrl.js uses rejectUnauthorized: false—this should be explicitly avoided.
  • No support for meshctrl-style WebSocket authentication (x-meshauth header with base64-encoded credentials).

Attack Surface: MITM attack, credential theft via insecure WebSocket upgrade.

Required Fix:

  • Replace fetch with a proper WebSocket client (ws package, already in dependencies).
  • Enable TLS certificate validation (do not disable it).
  • Support custom CA bundle for internal MeshCentral deployments.
  • Implement meshctrl protocol handshake (research confirms wss://host:port/control.ashx).

🔴 CRITICAL: No authentication or authorization implementation

Location: src/adapter.ts (entire file)
Severity: Critical

The research document explicitly identifies two authentication modes:

  1. Username/password via x-meshauth WebSocket header (base64 encoded).
  2. Server login key (highly sensitive—should not be used for the bridge).

Current State: The real adapter has no authentication mechanism. It does not:

  • Accept or validate credentials.
  • Support login keys (even in config).
  • Manage WebSocket sessions or reconnects.

Required Fix:

  • Add username, password (or loginKey) to BridgeConfig.
  • Implement x-meshauth header encoding per upstream meshctrl.js source.
  • Store secrets in environment or protected file with restrictive permissions (0600).
  • Fail closed if credentials are missing or invalid.

🟠 HIGH: Test expectation mismatch (non-blocking but indicates incomplete validation)

Location: src/__tests__/mock-adapter.test.ts:56,86
Severity: High

Tests expect error message substring "Resource not found" but actual errors use more specific messages:

Test Expected Actual
getDevice non-existent "Resource not found" "Device not found: nonexistent"
getGroup non-existent "Resource not found" "Group not found: nonexistent"

Fix: Update test expectations to match the current error messages, or standardize error messages across the codebase.

// Recommendation: Use consistent prefix for all resource-not-found errors
`Resource not found: device ${deviceId}`

🟠 HIGH: TypeScript build fails in test setup

Location: src/__tests__/setup.ts
Severity: High

Build errors prevent compilation and CI validation:

src/__tests__/setup.ts(13,15): error TS2428: All declarations of 'Expect' must have identical type parameters.
src/__tests__/setup.ts(23,13): error TS7006: Parameter 'received' implicitly has an 'any' type.

Root Cause: Conflicting jest and jest-extended types with mismatched generic parameters. The custom matchers declare Expect<T> without the required type argument.

Required Fix:

  • Align custom matcher types with jest v29 expectations.
  • Remove duplicate import of 'jest-extended'.
  • Use proper typing for received parameter.

🟠 HIGH: ESLint configuration missing

Location: Project root
Severity: High

Lint command fails:

ESLint couldn't find a configuration file.

Required Fix:

  • Add .eslintrc.yml or eslint.config.js.
  • Enforce TypeScript rules (@typescript-eslint/).
  • Configure import ordering, unused variables, and strict null checks.

Example minimal config (eslint.config.js):

module.exports = {
  parser: '@typescript-eslint/parser',
  plugins: ['@typescript-eslint'],
  extends: [
    'eslint:recommended',
    'plugin:@typescript-eslint/recommended',
  ],
  rules: {
    '@typescript-eslint/no-explicit-any': 'error',
    '@typescript-eslint/strict-boolean-expressions': 'off',
  },
};

🟠 HIGH: Diagnostics produce raw output without sanitization

Location: src/mock-adapter.ts:54-103, DIAGNOSTIC_OUTPUTS
Severity: High

Diagnostic output is hard-coded static strings. Research document warns:

  • Endpoint command output may contain secrets (passwords, keys).
  • Device names, logs, and process lists can expose personal data.
  • Prompt injection via untrusted output is a valid threat.

Current State: No sanitization or escape of special characters, secrets, or prompt-injection vectors.

Required Fix:

  • Implement output sanitization: replace known secret patterns (password=, token=, api_key=).
  • Truncate or mask sensitive fields in logs.
  • Add a sanitizeOutput() function before returning results.
  • Consider adding an explicit "safe output only" mode.

🟡 MEDIUM: No device/group allowlists

Location: src/adapter.ts, contracts.ts
Severity: Medium

Research document explicitly requires:

Restrict targets using explicit device/group allowlists.

Current State:

  • Configuration has no allowedDeviceIds or allowedGroupIds.
  • No validation that requested devices/groups are in an allowlist.

Required Fix:

  • Add allowedDeviceIds?: string[] and allowedGroupIds?: string[] to BridgeConfig.
  • Add validation in adapter methods: reject if target not in allowlist (or empty allowlist = all allowed).
  • Audit log must record whether allowlist check passed.

🟡 MEDIUM: No audit logging

Location: src/ (all files)
Severity: Medium

Security requirements mandate:

Log authentication events, requests, target, operation, confirmation, result, and timestamp.

Current State: No logging implementation. Debug logs or console.log statements are absent.

Required Fix:

  • Introduce BridgeLogger interface with methods:
    • logAuth(principal, success, details)
    • logRequest(requestId, principal, operation, target, timestamp)
    • logResult(requestId, success, details)
  • Use a structured formatter (JSON) for downstream processing.
  • Never log secrets (passwords, keys, tokens).

🟡 MEDIUM: No connection resilience

Location: src/adapter.ts
Severity: Medium

Research document notes:

Whether sessions expire, how reconnects behave, and whether a reconnect can replay an action.

Current State: No retry logic, backoff, or session management.

Required Fix:

  • Add maxRetries, retryBackoffMs to config.
  • Implement exponential backoff on WebSocket connect failures.
  • Add reconnect guard to prevent duplicate action replay (e.g., request ID deduplication).

🟡 MEDIUM: No version compatibility checking

Location: src/adapter.ts, contracts.ts
Severity: Medium

Research document warns:

The bridge should record the tested server version and client/source commit, use semver/commit compatibility checks, and fail closed or enter degraded read-only mode when an unknown version is detected.

Current State: No MeshCentral version check.

Required Fix:

  • Add serverVersion field to adapter metadata.
  • Fetch version on connect (e.g., via ServerInfo control message).
  • Define supported version range; fail closed if incompatible.

🟡 MEDIUM: Diagnostics allow all check types without policy

Location: src/contracts.ts, mock-adapter.ts
Severity: Medium

Research document requires:

Define an allowlisted set of diagnostic checks... Restrict execution by device/group allowlist and command policy.

Current State:

  • All DiagnosticCheckType values are enabled by default.
  • No policy mapping check types to permitted device groups.

Required Fix:

  • Add diagnosticPolicy config with allowed check types per group/device.
  • Example: 
    diagnosticPolicy: {
  'grp-servers': ['diskSpace', 'uptime'],
  '*': [],
}
  • Reject diagnostic requests not in policy.

🟡 MEDIUM: No timeout enforcement on real adapter

Location: src/adapter.ts
Severity: Medium

The healthCheck() method has no timeout; fetch will hang indefinitely by default.

Required Fix:

  • Use an AbortController with a configurable timeout.
  • Reject with ErrorCodes.TimeoutExceeded if exceeded.

🟢 LOW: Diagnostic output truncation inconsistent

Location: src/mock-adapter.ts
Severity: Low

Truncation is simulated but not tested for edge cases:

  • What happens if maxOutputLength is 0?
  • What happens if output is exactly at the limit?

Test Gap: timeout.test.ts has no test for zero-length max output.

Verification Results

Test Execution (npm test -- --coverage)

Test Suites: 1 failed, 7 passed, 8 total
Tests:       2 failed, 50 passed, 52 total
Coverage:    92.3% statements, 91.66% branches, 72.72% functions

Passing suites:

  • contracts.test.ts — All Zod schema validations pass.
  • adapter-factory.test.ts — Factory correctly returns mock by default.
  • group-filtering.test.ts — Device filtering by group works.
  • timeout.test.ts — Timeout and output limits respected (except edge cases).
  • device-status.test.ts — Status enum handling correct.
  • mock-data-loader.test.ts — JSON fixture loading works.
  • integration.test.ts — Full workflow mocks succeed.

Failing tests (non-blocking, expectation issue):

  • mock-adapter.test.ts — 2 tests fail due to error message mismatch (not logic):
    • getDevice non-existent expects "Resource not found" but receives "Device not found: nonexistent"
    • getGroup non-existent expects "Resource not found" but receives "Group not found: nonexistent"

Build (npm run build)

node_modules/@types/jest/index.d.ts(14,23): error TS2314
node_modules/@types/jest/index.d.ts(683,15): error TS2428
src/__tests__/setup.ts(13,15): error TS2428
src/__tests__/setup.ts(23,13): error TS7006
src/__tests__/setup.ts(30,14): error TS7006
src/__tests__/setup.ts(37,14): error TS7006

Status: Build fails—cannot produce dist artifacts. All errors are in test setup, not source code.

Lint (npm run lint)

ESLint couldn't find a configuration file. To set up a configuration file for this project, please run:
    npm init @eslint/config

Status: Linting entirely skipped—no code style enforcement. Missing .eslintrc.* or eslint.config.js.

Code Coverage

----------------------|---------|----------|---------|---------|-------------------
File                  | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------------------|---------|----------|---------|---------|-------------------
All files             |    92.3 |    91.66 |   72.72 |   91.83 |                   
 src                  |   91.83 |    91.66 |   71.87 |    91.3 |                   
  adapter.ts          |      60 |     87.5 |   44.44 |      60 | 41-65             
  contracts.ts        |     100 |      100 |     100 |     100 |                   
  index.ts            |     100 |      100 |      50 |     100 |                   
  mock-adapter.ts     |     100 |       90 |    92.3 |     100 | 197               
 src/__tests__        |     100 |      100 |     100 |     100 |

Coverage Gaps:

  • adapter.ts lines 41–65: Unimplemented real adapter methods.
  • mock-adapter.ts line 197: Edge case in diagnostic output handling (uncovered due to fixed mock data sizes).
  • index.ts line 50: Factory export (uncovered in isolation tests).

Scope Creep Assessment

Current scope aligns with PROJECT.md Goal 1 (read-only foundation).

Observed scope creep:

  • None detected—adapter factory and config correctly default to 'mock'.
  • example-config-real.json suggests real adapter is WIP, not production-ready.

Not yet in scope (per TODO.md):

  • Diagnostics beyond mock data.
  • State-changing actions (reboot, service restart).
  • Confirmation flows or dry-run.

TLS and Auth Assumptions (Per Research)

Requirement Current Implementation Research Finding
TLS certificate validation Unknown (no actual connection) Must be enabled; no rejectUnauthorized: false
WebSocket endpoint Not implemented wss://host:port/control.ashx
Auth header Not implemented x-meshauth: base64(username,password,2fa?)
Login key support Not implemented High sensitivity—avoid unless explicitly chosen

Conclusion: The real adapter is currently a stub that will fail or expose credentials if used with live MeshCentral.

Test Failure Classification

Test Failure Type Fix Required
mock-adapter.test.ts:56 Expectation mismatch (error message) Update test expectation
mock-adapter.test.ts:86 Expectation mismatch (error message) Update test expectation
TypeScript build errors Type definition conflict Fix setup.ts types
ESLint failure Missing config Add .eslintrc.*

Note: No test failures indicate logic defects in core functionality.

Recommendations

Immediate (Before Live Integration)

  1. Fix TypeScript errors in src/__tests__/setup.ts to enable build.
  2. Add ESLint configuration and enforce linting in CI.
  3. Fix test expectations to align with actual error messages (or standardize messages).
  4. Implement MeshCentralRealAdapter with:
    • Secure TLS (enabled validation).
    • WebSocket connection to /control.ashx.
    • x-meshauth header authentication.
    • Configurable username, password, or explicit opt-in for login key.
  5. Add audit logging with structured JSON output.

Short-Term (Before Production)

  1. Implement device/group allowlists.
  2. Add output sanitization for diagnostics.
  3. Define and enforce diagnostic policies.
  4. Add version compatibility checks.
  5. Implement retry logic with backoff.

Medium-Term (After Security Review)

  1. Add confirmation flow for reversible actions.
  2. Implement dry-run support.
  3. Add idempotency and duplicate-execution protection.

Conclusion

The implementation is a well-structured mock-based scaffold suitable for local development and testing, but it is not production-ready. The real adapter is unimplemented and contains no security controls beyond basic config structure.

Do not proceed with live MeshCentral integration until:

  • The real adapter is implemented per security requirements.
  • All build/lint/test failures are resolved.
  • An independent security review confirms TLS, auth, and logging.

Appendix A: File Inventory

File Status Notes
src/index.ts ✅ Clean Exports adapter factory and contracts
src/adapter.ts ⚠️ Stub Real adapter unimplemented; healthCheck uses wrong endpoint
src/mock-adapter.ts ✅ Complete Full mock with test fixtures
src/contracts.ts ✅ Complete Zod schemas, enums, types
src/__tests__/adapter-factory.test.ts ✅ Pass Factory correctly returns mock by default
src/__tests__/contracts.test.ts ✅ Pass All Zod schema validations pass
src/__tests__/device-status.test.ts ✅ Pass Status enum handling correct
src/__tests__/group-filtering.test.ts ✅ Pass Device filtering by group works
src/__tests__/integration.test.ts ✅ Pass Full workflow mocks succeed
src/__tests__/mock-adapter.test.ts ⚠️ 2/6 fail Expectation mismatch on error messages
src/__tests__/mock-data-loader.test.ts ✅ Pass JSON fixture loading works
src/__tests__/setup.ts ❌ Broken TypeScript errors block build and coverage
src/__tests__/timeout.test.ts ✅ Pass Timeout and output limits respected

Test Summary:

  • 50 tests pass (96.2%)
  • 2 tests fail due to error message expectation mismatch
  • Build fails due to TypeScript type conflicts in test setup
  • Linting skipped due to missing ESLint configuration

End of Review Report

Reference in New Issue View Git Blame Copy Permalink