From 200c040d3a941483bfb996c9ac971f5dad1042c8 Mon Sep 17 00:00:00 2001
From: dan <dan@delpad>
Date: Sat, 10 Jan 2026 20:26:38 -0800
Subject: [PATCH] fix: P1 security bugs - cryptographic IDs and GC-managed
 heartbeat

- genOid: use std/sysrand for cryptographic randomness instead of rand()
- HeartbeatThread: change from ptr with manual alloc/dealloc to ref object
- Add error handling for DB open in heartbeat thread
- Remove unused globalChannel and times import

Closes: skills-0wk, skills-bk7x, skills-69sz, skills-ib9u, skills-kvdl, skills-n6zf

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
---
 src/worker/db.nim        | 14 ++++++++++----
 src/worker/heartbeat.nim | 31 ++++++++++++++++++-------------
 2 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/src/worker/db.nim b/src/worker/db.nim
index 7e59d99..fe220f6 100644
--- a/src/worker/db.nim
+++ b/src/worker/db.nim
@@ -6,16 +6,22 @@
 ## - BEGIN IMMEDIATE for write transactions
 ## - Explicit ack for at-least-once delivery
 
-import std/[os, json, strformat, options, strutils, random]
+import std/[os, json, options, sysrand, strutils]
 import tiny_sqlite
 import ./types
 
 # Helper for generating unique IDs
 proc genOid*(): string =
-  ## Generate a simple unique ID
-  result = ""
+  ## Generate a cryptographically random ID (16 lowercase chars)
+  var bytes: array[16, byte]
+  if not urandom(bytes):
+    # Fallback: use timestamp + process ID if urandom fails
+    let ts = epochMs()
+    let pid = getCurrentProcessId()
+    return "f" & toHex(ts, 12) & toHex(pid, 3)
+  result = newString(16)
   for i in 0..<16:
-    result.add(chr(ord('a') + rand(25)))
+    result[i] = chr(ord('a') + (bytes[i] mod 26))
 
 const Schema* = """
 -- Messages table (seq is rowid-based for true auto-increment)
diff --git a/src/worker/heartbeat.nim b/src/worker/heartbeat.nim
index 2013dbd..eb4142f 100644
--- a/src/worker/heartbeat.nim
+++ b/src/worker/heartbeat.nim
@@ -8,7 +8,7 @@
 ## - Channel for stop signal and status updates
 ## - Non-blocking tryRecv to check for commands
 
-import std/[os, times]
+import std/os
 import tiny_sqlite
 import ./types
 import ./db
@@ -24,7 +24,8 @@ type
     task*: string
     progress*: float
 
-  HeartbeatThread* = object
+  HeartbeatThread* = ref object
+    ## GC-managed heartbeat thread handle
     thread: Thread[HeartbeatArgs]
     channel: Channel[HeartbeatMsg]
     running: bool
@@ -34,11 +35,15 @@ type
     agentId: string
     channelPtr: ptr Channel[HeartbeatMsg]
 
-var globalChannel: Channel[HeartbeatMsg]
-
 proc heartbeatWorker(args: HeartbeatArgs) {.thread.} =
   ## Dedicated heartbeat thread - owns its own DB connection
-  let db = openBusDb(args.dbPath)
+  var db: DbConn
+  try:
+    db = openBusDb(args.dbPath)
+  except CatchableError as e:
+    stderr.writeLine("Heartbeat: failed to open database: ", e.msg)
+    return
+
   var status = hsIdle
   var task = ""
   var progress = 0.0
@@ -60,14 +65,14 @@ proc heartbeatWorker(args: HeartbeatArgs) {.thread.} =
     try:
       db.writeHeartbeat(args.agentId, status, task, progress)
     except CatchableError as e:
-      # Log but don't crash
       stderr.writeLine("Heartbeat error: ", e.msg)
 
     sleep(HeartbeatIntervalMs)
 
-proc startHeartbeat*(dbPath, agentId: string): ptr HeartbeatThread =
+proc startHeartbeat*(dbPath, agentId: string): HeartbeatThread =
   ## Start the heartbeat thread. Returns handle for control.
-  result = cast[ptr HeartbeatThread](alloc0(sizeof(HeartbeatThread)))
+  ## The returned ref is GC-managed - no manual cleanup needed.
+  result = HeartbeatThread()
   result.channel.open()
   result.running = true
 
@@ -79,7 +84,7 @@ proc startHeartbeat*(dbPath, agentId: string): ptr HeartbeatThread =
 
   createThread(result.thread, heartbeatWorker, args)
 
-proc updateStatus*(hb: ptr HeartbeatThread, status: HeartbeatStatus,
+proc updateStatus*(hb: HeartbeatThread, status: HeartbeatStatus,
                    task: string = "", progress: float = 0.0) =
   ## Update the status reported in heartbeats
   if hb != nil and hb.running:
@@ -90,17 +95,17 @@ proc updateStatus*(hb: ptr HeartbeatThread, status: HeartbeatStatus,
       progress: progress
     ))
 
-proc stopHeartbeat*(hb: ptr HeartbeatThread) =
+proc stopHeartbeat*(hb: HeartbeatThread) =
   ## Stop the heartbeat thread and clean up
   if hb != nil and hb.running:
     hb.channel.send(HeartbeatMsg(cmd: hbStop))
     joinThread(hb.thread)
     hb.channel.close()
     hb.running = false
-    dealloc(hb)
+    # No dealloc needed - GC handles ref type
 
-# Simpler API using a global channel (for single-threaded CLI usage)
-var globalHeartbeat: ptr HeartbeatThread = nil
+# Simpler API using global state (for single-threaded CLI usage)
+var globalHeartbeat: HeartbeatThread = nil
 
 proc startGlobalHeartbeat*(dbPath, agentId: string) =
   ## Start heartbeat using global state (simpler API for CLI)