Initial project documentation and status
This commit is contained in:
@@ -0,0 +1,71 @@
|
||||
# MeshCentral ↔ OpenClaw Bridge — Final Hardening Review
|
||||
|
||||
**Date:** 2026-08-25
|
||||
**Reviewer:** Clawdia (manager review)
|
||||
**Scope:** Current implementation under `implementation/src/`
|
||||
**Purpose:** Disposition the earlier independent review against the hardened local source.
|
||||
|
||||
## Verdict
|
||||
|
||||
**Local read-only MVP gate: PASS.** The implementation passes the local verification gate and contains the required controls for preparing a tightly scoped live read-only test.
|
||||
|
||||
**Live test: NOT YET APPROVED.** A live connection remains gated on deployment-specific checks and Rob's explicit approval immediately before execution.
|
||||
|
||||
## Verification evidence
|
||||
|
||||
- `npm test -- --runInBand`: **11 suites, 90 tests passed**
|
||||
- `npm run build`: **passed**
|
||||
- `npm run lint`: **passed with no errors or warnings**
|
||||
- No live MeshCentral connection has been attempted.
|
||||
|
||||
## Prior finding disposition
|
||||
|
||||
| Finding | Current disposition | Evidence / residual risk |
|
||||
|---|---|---|
|
||||
| Credential/error leakage | **Addressed locally** | Credentials are validated before use; user-facing configuration failures are generic; audit/logging paths use redacted summaries. Residual risk: live secret handling still requires a protected runtime mechanism and operational verification. |
|
||||
| Credential validation/sanitization | **Addressed locally** | `validateCredentials()` is called before encoding/storage; credentials are never intentionally logged. Residual risk: the control-channel auth header necessarily contains the credential material in memory during connection. |
|
||||
| TLS validation | **Addressed locally** | WebSocket options explicitly set `rejectUnauthorized: true`; no bypass is present. Residual risk: the actual deployment certificate chain/private CA must be verified from the bridge host. |
|
||||
| Response validation and size limits | **Addressed locally** | Responses are size-bounded and parsed/validated; action-specific schemas and action matching are enforced. Residual risk: protocol compatibility with the deployed server remains unverified until a read-only test. |
|
||||
| Request concurrency/rate limiting | **Addressed locally** | Semaphore and token-bucket controls apply to outbound requests; configuration has bounded validation and safe defaults. Residual risk: limits are protective controls, not a substitute for MeshCentral capacity testing. |
|
||||
| Device/group allowlists | **Addressed locally** | Adapter validates allowlist entries and filters/checks device/group targets. Residual risk: the intended PVE3 identifier and group mapping must be confirmed before live use. |
|
||||
| Sensitive data in logs | **Addressed locally with operational caveat** | Structured logger and audit utilities redact credential keys, auth headers, token-like values, and encoded secret patterns. Residual risk: logs still contain inventory metadata and must be stored with appropriate permissions/retention. |
|
||||
| Separate handshake/request timeout | **Deferred, non-blocking** | A single bounded timeout is used for connection and requests. This does not expand capability or permit unsafe actions; it should be revisited after live read-only validation. |
|
||||
| Automatic reconnection/session renewal | **Deferred, non-blocking for first test** | No automatic reconnect is enabled, reducing replay risk. A dropped session fails rather than autonomously replaying requests. |
|
||||
| Structured audit logging | **Addressed locally** | Adapter and client use the controlled logger for audit events; redaction is applied. Operational sink/retention must be selected before deployment. |
|
||||
|
||||
## Live-test entry criteria still open
|
||||
|
||||
These are deployment facts, not local code failures:
|
||||
|
||||
1. Confirm `mesh.ledrew.me` resolves to the intended internal MeshCentral service from the proposed bridge host.
|
||||
2. Confirm the correct control-channel URL/path and whether TLS is terminated at a reverse proxy.
|
||||
3. Verify the certificate chain or approved private CA bundle from the bridge host; never disable verification.
|
||||
4. Confirm `sa_clawdia` is dedicated and read-only for this first test.
|
||||
5. Confirm the exact PVE3 device ID/group and configure it as the sole allowed target.
|
||||
6. Inject the credential through a protected runtime secret mechanism; do not place it in source, prompts, command arguments, or logs.
|
||||
7. Confirm audit-log destination, permissions, and retention.
|
||||
8. Obtain Rob's explicit approval immediately before the live connection.
|
||||
|
||||
## Scope restrictions for first live test
|
||||
|
||||
Allowed only:
|
||||
|
||||
- Authentication/connection health check
|
||||
- Server version
|
||||
- Device groups
|
||||
- Device inventory
|
||||
- Selected metadata for the single approved PVE3 target
|
||||
|
||||
Prohibited:
|
||||
|
||||
- Diagnostics that execute commands
|
||||
- Remote shell, file transfer, or arbitrary command execution
|
||||
- Reboots, shutdowns, service changes, or any state-changing action
|
||||
- PVE1/PVE2 production-cluster targeting
|
||||
- Public exposure of the bridge or MeshCentral
|
||||
|
||||
## Risk acceptance
|
||||
|
||||
No deferred feature is accepted as a reason to enable remote actions. The deferred timeout separation and reconnect behavior are explicitly **non-blocking only for a single, read-only, single-target test** and remain unsuitable for production fleet operation without further design/review.
|
||||
|
||||
The live-test gate remains closed until the entry criteria above are checked and Rob approves the specific read-only procedure immediately before execution.
|
||||
@@ -0,0 +1,376 @@
|
||||
# Live Adapter Security Review
|
||||
|
||||
**Date:** 2026-08-25
|
||||
**Scope:** `/home/ledadmin/projects/meshcentral-openclaw-bridge/implementation`
|
||||
**Reviewer:** Subagent (depth 1/1)
|
||||
**Status:** Independent security/protocol review
|
||||
|
||||
---
|
||||
|
||||
## Executive Summary
|
||||
|
||||
The MeshCentral ↔ OpenClaw bridge implementation demonstrates **good security hygiene in design intent**, but contains **multiple high-severity implementation gaps** that prevent safe live testing in its current state. The TLS security controls are correctly configured (no `rejectUnauthorized: false`), but the codebase lacks credential sanitization, output validation, and proper error masking—critical for production deployment.
|
||||
|
||||
**Recommendation:** Live testing is **NOT SAFE** in its current state. Address all high-severity findings before any live deployment.
|
||||
|
||||
---
|
||||
|
||||
## Overview of Reviewed Files
|
||||
|
||||
| File | Purpose | Reviewed |
|
||||
|------|---------|----------|
|
||||
| `research/INTEGRATION-RESEARCH.md` | Protocol analysis and threat model | ✅ |
|
||||
| `SECURITY.md` | Security requirements and action classes | ✅ |
|
||||
| `src/meshctrl-client.ts` | WebSocket client for MeshCentral protocol | ✅ |
|
||||
| `src/adapter.ts` | Real adapter implementation | ✅ |
|
||||
| `src/contracts.ts` | Zod schemas and type definitions | ✅ |
|
||||
| `package.json` | Dependencies and scripts | ✅ |
|
||||
|
||||
---
|
||||
|
||||
## Test Results
|
||||
|
||||
The implementation has a robust test suite:
|
||||
|
||||
| Command | Result |
|
||||
|---------|--------|
|
||||
| `npm run build` | ✅ Pass (no errors) |
|
||||
| `npm run lint` | ✅ Pass (no errors) |
|
||||
| `npm test` | ✅ 52/52 tests pass (8 test suites) |
|
||||
|
||||
Tests cover:
|
||||
- Device status handling
|
||||
- Adapter factory patterns
|
||||
- Contract validation (Zod schemas)
|
||||
- Output truncation and limits
|
||||
- Group filtering
|
||||
- Integration workflows
|
||||
|
||||
---
|
||||
|
||||
## Security Findings by Severity
|
||||
|
||||
### 🔴 HIGH SEVERITY (Blocking live testing)
|
||||
|
||||
#### 1. **Credentials in Error Messages**
|
||||
**Location:** `adapter.ts`, `_validateConfig()`
|
||||
|
||||
```typescript
|
||||
if (hasUsername !== hasPassword) {
|
||||
throw new BridgeError(
|
||||
ErrorCodes.ConfigurationInvalid,
|
||||
'Both username and password must be provided (or neither for mock mode)',
|
||||
);
|
||||
}
|
||||
```
|
||||
|
||||
**Issue:** The error handling does not sanitize or mask credential-related information. If credentials are partially configured, the system should not reveal which component is missing.
|
||||
|
||||
**Risk:** Credential enumeration and configuration leakage in logs/monitoring.
|
||||
|
||||
**Remediation:**
|
||||
- Use generic error messages for credential validation failures
|
||||
- Log detailed errors only to secure audit logs (not user-facing responses)
|
||||
- Never include credential field names in error messages
|
||||
|
||||
#### 2. **No Credential Sanitization Before Use**
|
||||
**Location:** `meshctrl-client.ts`, `encodeAuthHeader()`
|
||||
|
||||
```typescript
|
||||
export function encodeAuthHeader(credentials: AuthCredentials): string {
|
||||
const { username, password, totpToken } = credentials;
|
||||
if (totpToken) {
|
||||
return Buffer.from(`${username}:${password}:${totpToken}`).toString('base64');
|
||||
}
|
||||
return Buffer.from(`${username}:${password}`).toString('base64');
|
||||
}
|
||||
```
|
||||
|
||||
**Issue:** Credentials are used directly without validation or sanitization. No check for special characters, empty strings, or malicious payloads in username/password.
|
||||
|
||||
**Risk:**
|
||||
- Authentication bypass via malformed credentials
|
||||
- Protocol injection via special characters in username/password
|
||||
- TOTP token leakage if exposed
|
||||
|
||||
**Remediation:**
|
||||
- Validate username/password meet minimum length requirements
|
||||
- Sanitize/escape special characters before encoding
|
||||
- Add explicit input validation before credential handling
|
||||
|
||||
#### 3. **Missing Certificate Validation Configuration**
|
||||
**Location:** `meshctrl-client.ts`, `MeshCtrlClient.connect()`
|
||||
|
||||
```typescript
|
||||
this.ws = new WebSocket(wsUrl, {
|
||||
perMessageDeflate: false,
|
||||
headers: {
|
||||
'x-meshauth': authHeader,
|
||||
},
|
||||
// Important: no TLS bypass - use default certificate validation
|
||||
});
|
||||
```
|
||||
|
||||
**Issue:** While the code comments state "no TLS bypass", it does not explicitly configure certificate validation or support custom CA bundles. The `ws` library defaults may vary by environment.
|
||||
|
||||
**Risk:** Inconsistent certificate validation behavior across deployment environments.
|
||||
|
||||
**Remediation:**
|
||||
- Explicitly set `rejectUnauthorized: true` (default, but make explicit)
|
||||
- Add support for custom CA bundle via environment variable
|
||||
- Document TLS configuration requirements
|
||||
|
||||
#### 4. **No Response Validation on WebSocket Messages**
|
||||
**Location:** `meshctrl-client.ts`, `_sendRequestInternal()`
|
||||
|
||||
```typescript
|
||||
const handleResponse = (data: Buffer | string): void => {
|
||||
try {
|
||||
clearTimeout(timeout);
|
||||
const response = parseResponse(data.toString());
|
||||
// ...
|
||||
} catch (error) {
|
||||
clearTimeout(timeout);
|
||||
this._cleanup();
|
||||
reject(error instanceof Error ? error : new Error('Unknown error'));
|
||||
}
|
||||
};
|
||||
```
|
||||
|
||||
**Issue:** Responses are parsed without schema validation against the expected `MeshCtrlResponse` contract. Malformed or malicious responses could cause undefined behavior.
|
||||
|
||||
**Risk:** Protocol downgrade attacks, response injection, or memory exhaustion from oversized responses.
|
||||
|
||||
**Remediation:**
|
||||
- Validate all responses against Zod schemas (already available in `contracts.ts`)
|
||||
- Enforce `MAX_RESPONSE_SIZE` limit before parsing
|
||||
- Reject responses with unexpected fields
|
||||
|
||||
---
|
||||
|
||||
### 🟡 MEDIUM SEVERITY (Should address before production)
|
||||
|
||||
#### 5. **No Rate Limiting or Concurrent Request Throttling**
|
||||
**Location:** `meshctrl-client.ts`, `_sendWithRetry()`
|
||||
|
||||
```typescript
|
||||
async _sendWithRetry(request: MeshCtrlRequest, maxRetries: number): Promise<MeshCtrlResponse> {
|
||||
let lastError: Error | null = null;
|
||||
for (let attempt = 1; attempt <= maxRetries; attempt++) {
|
||||
try {
|
||||
return await this._sendRequestInternal(request);
|
||||
} catch (error) {
|
||||
lastError = error instanceof Error ? error : new Error('Unknown error');
|
||||
if (attempt < maxRetries) {
|
||||
await new Promise((resolve) => setTimeout(resolve, 100 * attempt));
|
||||
}
|
||||
}
|
||||
}
|
||||
throw new Error(`Request failed after ${maxRetries} attempts: ${lastError?.message}`);
|
||||
}
|
||||
```
|
||||
|
||||
**Issue:** No global rate limiting. Multiple concurrent requests can flood the MeshCentral server, potentially triggering denial-of-service protections.
|
||||
|
||||
**Risk:** Collateral damage to other services, connection throttling, or account lockout.
|
||||
|
||||
**Remediation:**
|
||||
- Implement per-adapter request rate limiting (e.g., 5 requests/second)
|
||||
- Add circuit breaker for repeated failures
|
||||
- Document rate limits in security configuration
|
||||
|
||||
#### 6. **No Input Validation on Device/Group IDs**
|
||||
**Location:** `adapter.ts`, `_isDeviceAllowed()`
|
||||
|
||||
```typescript
|
||||
private _isDeviceAllowed(deviceId: string): boolean {
|
||||
const { deviceAllowlist, groupAllowlist } = this.config;
|
||||
if (!deviceAllowlist && !groupAllowlist) {
|
||||
return true;
|
||||
}
|
||||
if (deviceAllowlist && deviceAllowlist.includes(deviceId)) {
|
||||
return true;
|
||||
}
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
**Issue:** Device/group IDs are used directly in allowlist lookups without sanitization or regex validation.
|
||||
|
||||
**Risk:** Path traversal or injection via malicious device IDs in allowlist configuration.
|
||||
|
||||
**Remediation:**
|
||||
- Validate IDs against allowed character set (alphanumeric, hyphens, underscores)
|
||||
- Escape special regex characters if used in allowlist patterns
|
||||
- Document allowed ID formats
|
||||
|
||||
#### 7. **Sensitive Data in Logs**
|
||||
**Location:** Multiple places in `adapter.ts`
|
||||
|
||||
The code uses generic error handling but does not explicitly prevent credential or token leakage in logs.
|
||||
|
||||
**Risk:** Accidental credential exposure via logging systems.
|
||||
|
||||
**Remediation:**
|
||||
- Implement structured logging with automatic secret redaction
|
||||
- Add `secret` annotation to credential fields for automatic masking
|
||||
- Review all error paths for potential leakage
|
||||
|
||||
---
|
||||
|
||||
### 🟢 LOW SEVERITY (Nice to have)
|
||||
|
||||
#### 8. **No Timeout Configuration for WebSocket Handshake**
|
||||
**Location:** `meshctrl-client.ts`, `connect()`
|
||||
|
||||
```typescript
|
||||
await new Promise<void>((resolve, reject) => {
|
||||
const timeout = setTimeout(() => {
|
||||
this._cleanup();
|
||||
reject(new Error(`Connection timed out after ${this.timeoutMs}ms`));
|
||||
}, this.timeoutMs);
|
||||
// ...
|
||||
});
|
||||
```
|
||||
|
||||
**Issue:** The same timeout is used for both connection establishment and request/response cycles. Connection timeouts are typically shorter than request timeouts.
|
||||
|
||||
**Recommendation:** Separate `connectionTimeoutMs` from `requestTimeoutMs`.
|
||||
|
||||
#### 9. **No Session Renewal or Reconnection Strategy**
|
||||
**Location:** `adapter.ts`, `_ensureConnected()`
|
||||
|
||||
```typescript
|
||||
private async _ensureConnected(): Promise<void> {
|
||||
if (this.connected) {
|
||||
return;
|
||||
}
|
||||
// ...
|
||||
await this.client!.connect(this.credentials);
|
||||
this.connected = true;
|
||||
}
|
||||
```
|
||||
|
||||
**Issue:** If the WebSocket connection drops, there is no automatic reconnection logic. The adapter must be destroyed and recreated.
|
||||
|
||||
**Recommendation:** Implement exponential backoff reconnection with maximum retry count.
|
||||
|
||||
#### 10. **Missing Audit Logging**
|
||||
**Location:** `adapter.ts`, all public methods
|
||||
|
||||
The code lacks structured audit logging for security events.
|
||||
|
||||
**Recommendation:**
|
||||
- Log authentication successes/failures
|
||||
- Log allowlist violations
|
||||
- Log request IDs and timestamps for all operations
|
||||
|
||||
---
|
||||
|
||||
## Protocol Analysis Summary
|
||||
|
||||
### ✅ Correctly Implemented
|
||||
|
||||
| Feature | Status |
|
||||
|---------|--------|
|
||||
| TLS validation (no `rejectUnauthorized: false`) | ✅ Correct |
|
||||
| WebSocket URL construction (`wss://` for HTTPS) | ✅ Correct |
|
||||
| `x-meshauth` header encoding (base64 username:password) | ✅ Correct |
|
||||
| Request/response ID matching | ✅ Correct |
|
||||
| Typed contract schemas (Zod) | ✅ Correct |
|
||||
| Allowlist-based filtering | ✅ Correct |
|
||||
|
||||
### ⚠️ Needs Hardening
|
||||
|
||||
| Feature | Issue |
|
||||
|---------|-------|
|
||||
| Credential validation | Missing input sanitization |
|
||||
| Response validation | No schema enforcement |
|
||||
| Error handling | Potential credential leakage |
|
||||
| Rate limiting | Not implemented |
|
||||
|
||||
---
|
||||
|
||||
## Live Testing Safety Assessment
|
||||
|
||||
### ❌ Current State: **NOT SAFE**
|
||||
|
||||
**Blocking Issues:**
|
||||
1. Credentials in error messages
|
||||
2. No credential sanitization
|
||||
3. Missing response validation
|
||||
|
||||
**Conditional Safety:**
|
||||
- If deployed on an isolated, air-gapped management network with:
|
||||
- Dedicated non-admin MeshCentral account
|
||||
- Strict firewall rules (outbound only to MeshCentral)
|
||||
- Audit logging enabled
|
||||
- No public ingress
|
||||
|
||||
### ✅ After Remediation: **CONDITIONALLY SAFE**
|
||||
|
||||
The implementation can be made safe for live testing with the following changes:
|
||||
|
||||
1. **Input Validation:** Add sanitization for all credential and ID fields
|
||||
2. **Response Validation:** Enforce Zod schema validation on all WebSocket responses
|
||||
3. **Error Handling:** Remove credential-related details from user-facing errors
|
||||
4. **Rate Limiting:** Implement request throttling to prevent abuse
|
||||
|
||||
---
|
||||
|
||||
## Recommendations Summary
|
||||
|
||||
| Priority | Action |
|
||||
|----------|--------|
|
||||
| 🔴 **HIGH** | Implement credential sanitization and validation |
|
||||
| 🔴 **HIGH** | Add response schema validation (Zod) for all WebSocket responses |
|
||||
| 🔴 **HIGH** | Sanitize error messages to prevent credential leakage |
|
||||
| 🟡 **MEDIUM** | Add rate limiting for concurrent requests |
|
||||
| 🟡 **MEDIUM** | Validate device/group IDs against allowed character set |
|
||||
| 🟢 **LOW** | Implement structured audit logging |
|
||||
| 🟢 **LOW** | Add separate connection/request timeout configuration |
|
||||
|
||||
---
|
||||
|
||||
## Appendices
|
||||
|
||||
### A. Test Coverage Summary
|
||||
|
||||
| Test Suite | Tests Passed | Status |
|
||||
|-----------|-------------|--------|
|
||||
| timeout.test.ts | 4/4 | ✅ |
|
||||
| device-status.test.ts | 6/6 | ✅ |
|
||||
| adapter-factory.test.ts | 3/3 | ✅ |
|
||||
| contracts.test.ts | 5/5 | ✅ |
|
||||
| group-filtering.test.ts | 4/4 | ✅ |
|
||||
| mock-data-loader.test.ts | 2/2 | ✅ |
|
||||
| integration.test.ts | 8/8 | ✅ |
|
||||
| mock-adapter.test.ts | 20/20 | ✅ |
|
||||
|
||||
### B. Dependency Security
|
||||
|
||||
| Package | Version | Known Vulnerabilities |
|
||||
|---------|---------|----------------------|
|
||||
| `ws` | 8.21.3 | None reported (as of 2026-08-25) |
|
||||
| `zod` | 3.24.2 | None reported (as of 2026-08-25) |
|
||||
| `axios` | 1.8.1 | None reported (as of 2026-08-25) |
|
||||
|
||||
### C. Protocol References
|
||||
|
||||
- MeshCentral version: 1.2.5 (released 2026-08-12)
|
||||
- Control endpoint: `/control.ashx` (WebSocket)
|
||||
- Auth header: `x-meshauth` (base64 encoded credentials)
|
||||
- Actions: `serverversion`, `userinfo`, `meshes`, `nodes`, `events`, `deviceinfo`
|
||||
|
||||
---
|
||||
|
||||
## Conclusion
|
||||
|
||||
The bridge implementation demonstrates a solid understanding of security principles in its design, particularly around TLS validation and allowlist enforcement. However, the current code contains critical gaps in credential handling, input validation, and error sanitization that must be addressed before any live deployment.
|
||||
|
||||
**Final Verdict:** **DO NOT ENABLE LIVE TESTING** until the three high-severity findings are remediated.
|
||||
|
||||
---
|
||||
|
||||
*Review completed: 2026-08-25 01:45 EDT*
|
||||
*Subagent session: ca174557-cd3e-4d97-90a9-31efee68aebd*
|
||||
@@ -0,0 +1,491 @@
|
||||
# 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:
|
||||
|
||||
```typescript
|
||||
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.
|
||||
|
||||
```typescript
|
||||
// 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`):
|
||||
```javascript
|
||||
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:
|
||||
```typescript
|
||||
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)
|
||||
|
||||
6. Implement device/group allowlists.
|
||||
7. Add output sanitization for diagnostics.
|
||||
8. Define and enforce diagnostic policies.
|
||||
9. Add version compatibility checks.
|
||||
10. Implement retry logic with backoff.
|
||||
|
||||
### Medium-Term (After Security Review)
|
||||
|
||||
11. Add confirmation flow for reversible actions.
|
||||
12. Implement dry-run support.
|
||||
13. 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
Block a user