Fix review feedback: Proxmox API safety, type deduplication, accessibility, and code cleanup

.kiro/steering/security-best-practices.md

@@ -10,7 +10,7 @@ inclusion: always
 - Validate all user inputs
 - Use parameterized queries to prevent SQL injection
 - Implement proper authentication and authorization
-- Use pragma: allowlist secret comment to lint secrets catched by precommit
+- Use pragma: allowlist secret comment to allowlist secrets caught by pre-commit
 
 ## Dependency Management
 

backend/src/integrations/IntegrationManager.ts

@@ -912,145 +912,20 @@ export class IntegrationManager {
     return true;
   }
 
-  /**
-   * Deduplicate nodes by ID, preferring nodes from higher priority sources
+    /**
+   * Deduplicate and link nodes by matching identifiers.
    *
-   * @param nodes - Array of nodes potentially with duplicates
-   * @returns Deduplicated array of nodes
+   * When multiple sources provide the same node (matched by identifiers like certname,
+   * hostname, or URI), merge them into a single node entry with all sources tracked.
+   * The node data is taken from the highest priority source, but all sources and URIs
+   * are recorded in sourceData.
+   *
+   * @param nodes - Array of nodes from all sources
+   * @returns Deduplicated and linked array of nodes with source attribution
    */
-  /**
-     * Deduplicate and link nodes by ID
-     *
-     * When multiple sources provide the same node (by ID), merge them into a single
-     * node entry with all sources tracked. This matches the behavior of group linking.
-     * The node data is taken from the highest priority source, but all sources are
-     * recorded in the sources array.
-     *
-     * @param nodes - Array of nodes from all sources
-     * @returns Deduplicated and linked array of nodes with source attribution
-     */
-    /**
-       * Deduplicate and link nodes by ID
-       *
-       * When multiple sources provide the same node (by ID), merge them into a single
-       * node entry with all sources tracked. This matches the behavior of group linking.
-       * The node data is taken from the highest priority source, but all sources are
-       * recorded in the sources array.
-       *
-       * @param nodes - Array of nodes from all sources
-       * @returns Deduplicated and linked array of nodes with source attribution
-       */
-      /**
-         * Deduplicate and link nodes by matching identifiers
-         *
-         * When multiple sources provide the same node (matched by identifiers like certname,
-         * hostname, URI), merge them into a single node entry with all sources tracked.
-         * This matches the behavior of group linking. The node data is taken from the highest
-         * priority source, but all sources and URIs are recorded.
-         *
-         * @param nodes - Array of nodes from all sources
-         * @returns Deduplicated and linked array of nodes with source attribution
-         */
-        private deduplicateNodes(nodes: Node[]): LinkedNode[] {
-                  // Use NodeLinkingService to link nodes by identifiers
-                  // This already handles source-specific data correctly
-                  return this.nodeLinkingService.linkNodes(nodes);
-                }
-
-        /**
-         * Check if a node matches a linked node by comparing identifiers
-         *
-         * @param node - Node to check
-         * @param linkedNode - Linked node to match against
-         * @returns True if nodes match
-         * @private
-         */
-        private nodesMatch(node: Node, linkedNode: LinkedNode): boolean {
-          const nodeIdentifiers = this.extractNodeIdentifiers(node);
-          const linkedIdentifiers = this.extractNodeIdentifiers(linkedNode);
-
-          // Check if any identifiers match
-          for (const id of nodeIdentifiers) {
-            if (linkedIdentifiers.includes(id)) {
-              return true;
-            }
-          }
-
-          return false;
-        }
-
-        /**
-         * Extract all possible identifiers from a node
-         *
-         * @param node - Node to extract identifiers from
-         * @returns Array of identifiers (normalized to lowercase)
-         * @private
-         */
-        private extractNodeIdentifiers(node: Node): string[] {
-          const identifiers: string[] = [];
-
-          // Add node ID
-          if (node.id) {
-            identifiers.push(node.id.toLowerCase());
-          }
-
-          // Add node name (certname)
-          // Skip empty names to prevent incorrect linking
-          if (node.name && node.name.trim() !== "") {
-            // Normalize hostname: trim, lowercase, remove domain suffix
-            const normalizedName = node.name.trim().toLowerCase();
-            identifiers.push(normalizedName);
-
-            // Also add short hostname (without domain)
-            const shortName = normalizedName.split('.')[0];
-            if (shortName !== normalizedName) {
-              identifiers.push(shortName);
-            }
-          }
-
-          // Add URI hostname (extract from URI)
-          // Skip Proxmox URIs as they use format proxmox://node/vmid where 'node' is not unique per VM
-          if (node.uri && !node.uri.startsWith("proxmox://")) {
-            try {
-              // Extract hostname from URI
-              // URIs can be in formats like:
-              // - ssh://hostname
-              // - hostname
-              // - hostname:port
-              const uriParts = node.uri.split("://");
-              const hostPart = uriParts.length > 1 ? uriParts[1] : uriParts[0];
-              const hostname = hostPart.split(":")[0].split("/")[0].trim().toLowerCase();
-
-              if (hostname) {
-                identifiers.push(hostname);
-
-                // Also add short hostname
-                const shortHostname = hostname.split('.')[0];
-                if (shortHostname !== hostname) {
-                  identifiers.push(shortHostname);
-                }
-              }
-            } catch {
-              // Ignore URI parsing errors
-            }
-          }
-
-          // Add hostname from config if available
-          const nodeConfig = node.config as { hostname?: string } | undefined;
-          if (nodeConfig?.hostname) {
-            const normalizedHostname = nodeConfig.hostname.trim().toLowerCase();
-            identifiers.push(normalizedHostname);
-
-            // Also add short hostname
-            const shortHostname = normalizedHostname.split('.')[0];
-            if (shortHostname !== normalizedHostname) {
-              identifiers.push(shortHostname);
-            }
-          }
-
-          // Remove duplicates
-          return Array.from(new Set(identifiers));
-        }
+  private deduplicateNodes(nodes: Node[]): LinkedNode[] {
+    return this.nodeLinkingService.linkNodes(nodes);
+  }
 
   /**
    * Link groups with the same name across multiple sources

backend/src/integrations/NodeLinkingService.ts

@@ -168,8 +168,10 @@ export class NodeLinkingService {
           }
         }
 
-        // Combine all URIs
-        linkedNode.uri = Array.from(new Set(allUris)).join(", ");
+        // Keep uri as the primary URI from the first non-empty source.
+        // Source-specific URIs are preserved in sourceData[source].uri.
+        const primaryUri = allUris.find((u) => u) ?? linkedNode.uri;
+        linkedNode.uri = primaryUri;
 
         // Mark as linked if from multiple sources
         linkedNode.linked = linkedNode.sources.length > 1;
@@ -326,7 +328,6 @@ export class NodeLinkingService {
    */
   private extractIdentifiers(node: Node): string[] {
     const identifiers: string[] = [];
-    const nodeSource = (node as Node & { source?: string }).source ?? "bolt";
 
     // Add node ID (always unique per source)
     if (node.id) {

backend/src/integrations/proxmox/ProxmoxClient.ts

@@ -45,14 +45,20 @@ export class ProxmoxClient {
     this.logger = logger;
     this.baseUrl = `https://${config.host}:${String(config.port ?? 8006)}`;
 
-    // Configure SSL for fetch - set environment variable if needed
-    // This affects all HTTPS requests in the process, but is necessary for Node.js fetch
+    // Configure SSL behavior
+    // NOTE: Setting NODE_TLS_REJECT_UNAUTHORIZED is process-wide and would disable TLS verification
+    // for ALL HTTPS traffic, not just Proxmox. Instead, log a warning to guide the operator.
+    // Per-client TLS bypass via a custom HTTPS agent/dispatcher is the correct solution.
     if (config.ssl && config.ssl.rejectUnauthorized === false) {
-      process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0';
-      this.logger.debug("Disabled TLS certificate verification for Proxmox", {
-        component: "ProxmoxClient",
-        operation: "constructor",
-      });
+      this.logger.warn(
+        "Proxmox ssl.rejectUnauthorized=false is set, but per-client TLS bypass is not yet implemented. " +
+        "Proxmox connections will use the default TLS verification. " +
+        "Configure a trusted CA certificate (ssl.ca) to connect to Proxmox with self-signed certs.",
+        {
+          component: "ProxmoxClient",
+          operation: "constructor",
+        }
+      );
     }
 
     // Configure retry logic

backend/src/integrations/proxmox/types.ts

@@ -4,7 +4,9 @@
  * Type definitions for the Proxmox VE integration plugin.
  */
 
-import type { Capability } from "../types";
+import type { ProvisioningCapability } from "../types";
+
+export type { ProvisioningCapability };
 
 /**
  * Proxmox configuration
@@ -136,13 +138,6 @@ export interface ProxmoxTaskStatus {
   upid: string;
 }
 
-/**
- * Provisioning capability interface
- */
-export interface ProvisioningCapability extends Capability {
-  operation: "create" | "destroy";
-}
-
 /**
  * Retry configuration
  */

backend/src/integrations/types.ts

@@ -97,6 +97,12 @@ export interface CapabilityParameter {
   required: boolean;
   description?: string;
   default?: unknown;
+  validation?: {
+    min?: number;
+    max?: number;
+    pattern?: string;
+    enum?: string[];
+  };
 }
 
 /**

backend/src/routes/integrations/provisioning.ts

@@ -1,33 +1,10 @@
 import { Router, type Request, type Response } from "express";
 import type { IntegrationManager } from "../../integrations/IntegrationManager";
 import type { ProxmoxIntegration } from "../../integrations/proxmox/ProxmoxIntegration";
+import type { ProvisioningCapability } from "../../integrations/types";
 import { asyncHandler } from "../asyncHandler";
 import { createLogger } from "./utils";
 
-/**
- * Response types for provisioning integrations API
- */
-interface CapabilityParameter {
-  name: string;
-  type: 'string' | 'number' | 'boolean' | 'object' | 'array';
-  required: boolean;
-  description?: string;
-  default?: unknown;
-  validation?: {
-    min?: number;
-    max?: number;
-    pattern?: string;
-    enum?: string[];
-  };
-}
-
-interface ProvisioningCapability {
-  name: string;
-  description: string;
-  operation: 'create' | 'destroy';
-  parameters: CapabilityParameter[];
-}
-
 interface ProvisioningIntegration {
   name: string;
   displayName: string;
@@ -88,42 +65,7 @@ export function createProvisioningRouter(
           displayName: "Proxmox VE",
           type: "virtualization",
           status,
-          capabilities: [
-            {
-              name: "create_vm",
-              description: "Create a new virtual machine",
-              operation: "create",
-              parameters: [
-                { name: "vmid", type: "number", required: true, description: "Unique VM identifier", validation: { min: 100, max: 999999999 } },
-                { name: "name", type: "string", required: true, description: "VM name", validation: { max: 50 } },
-                { name: "node", type: "string", required: true, description: "Proxmox node name", validation: { max: 20 } },
-                { name: "cores", type: "number", required: false, description: "Number of CPU cores", validation: { min: 1, max: 128 } },
-                { name: "memory", type: "number", required: false, description: "Memory in MB", validation: { min: 16 } },
-                { name: "sockets", type: "number", required: false, description: "Number of CPU sockets", validation: { min: 1, max: 4 } },
-                { name: "cpu", type: "string", required: false, description: "CPU type" },
-                { name: "scsi0", type: "string", required: false, description: "SCSI disk configuration" },
-                { name: "ide2", type: "string", required: false, description: "IDE device configuration" },
-                { name: "net0", type: "string", required: false, description: "Network interface configuration" },
-                { name: "ostype", type: "string", required: false, description: "Operating system type" },
-              ],
-            },
-            {
-              name: "create_lxc",
-              description: "Create a new LXC container",
-              operation: "create",
-              parameters: [
-                { name: "vmid", type: "number", required: true, description: "Unique container identifier", validation: { min: 100, max: 999999999 } },
-                { name: "hostname", type: "string", required: true, description: "Container hostname", validation: { max: 50 } },
-                { name: "node", type: "string", required: true, description: "Proxmox node name", validation: { max: 20 } },
-                { name: "ostemplate", type: "string", required: true, description: "OS template name" },
-                { name: "cores", type: "number", required: false, description: "Number of CPU cores", validation: { min: 1, max: 128 } },
-                { name: "memory", type: "number", required: false, description: "Memory in MB", validation: { min: 16 } },
-                { name: "rootfs", type: "string", required: false, description: "Root filesystem configuration" },
-                { name: "net0", type: "string", required: false, description: "Network interface configuration" },
-                { name: "password", type: "string", required: false, description: "Root password" },
-              ],
-            },
-          ],
+          capabilities: proxmox.listProvisioningCapabilities(),
         };
 
         integrations.push(proxmoxIntegration);