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>
This commit is contained in:
dan 2026-01-10 20:26:38 -08:00
parent 02b7054709
commit 200c040d3a
2 changed files with 28 additions and 17 deletions

View file

@ -6,16 +6,22 @@
## - BEGIN IMMEDIATE for write transactions ## - BEGIN IMMEDIATE for write transactions
## - Explicit ack for at-least-once delivery ## - 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 tiny_sqlite
import ./types import ./types
# Helper for generating unique IDs # Helper for generating unique IDs
proc genOid*(): string = proc genOid*(): string =
## Generate a simple unique ID ## Generate a cryptographically random ID (16 lowercase chars)
result = "" 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: for i in 0..<16:
result.add(chr(ord('a') + rand(25))) result[i] = chr(ord('a') + (bytes[i] mod 26))
const Schema* = """ const Schema* = """
-- Messages table (seq is rowid-based for true auto-increment) -- Messages table (seq is rowid-based for true auto-increment)

View file

@ -8,7 +8,7 @@
## - Channel for stop signal and status updates ## - Channel for stop signal and status updates
## - Non-blocking tryRecv to check for commands ## - Non-blocking tryRecv to check for commands
import std/[os, times] import std/os
import tiny_sqlite import tiny_sqlite
import ./types import ./types
import ./db import ./db
@ -24,7 +24,8 @@ type
task*: string task*: string
progress*: float progress*: float
HeartbeatThread* = object HeartbeatThread* = ref object
## GC-managed heartbeat thread handle
thread: Thread[HeartbeatArgs] thread: Thread[HeartbeatArgs]
channel: Channel[HeartbeatMsg] channel: Channel[HeartbeatMsg]
running: bool running: bool
@ -34,11 +35,15 @@ type
agentId: string agentId: string
channelPtr: ptr Channel[HeartbeatMsg] channelPtr: ptr Channel[HeartbeatMsg]
var globalChannel: Channel[HeartbeatMsg]
proc heartbeatWorker(args: HeartbeatArgs) {.thread.} = proc heartbeatWorker(args: HeartbeatArgs) {.thread.} =
## Dedicated heartbeat thread - owns its own DB connection ## 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 status = hsIdle
var task = "" var task = ""
var progress = 0.0 var progress = 0.0
@ -60,14 +65,14 @@ proc heartbeatWorker(args: HeartbeatArgs) {.thread.} =
try: try:
db.writeHeartbeat(args.agentId, status, task, progress) db.writeHeartbeat(args.agentId, status, task, progress)
except CatchableError as e: except CatchableError as e:
# Log but don't crash
stderr.writeLine("Heartbeat error: ", e.msg) stderr.writeLine("Heartbeat error: ", e.msg)
sleep(HeartbeatIntervalMs) sleep(HeartbeatIntervalMs)
proc startHeartbeat*(dbPath, agentId: string): ptr HeartbeatThread = proc startHeartbeat*(dbPath, agentId: string): HeartbeatThread =
## Start the heartbeat thread. Returns handle for control. ## 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.channel.open()
result.running = true result.running = true
@ -79,7 +84,7 @@ proc startHeartbeat*(dbPath, agentId: string): ptr HeartbeatThread =
createThread(result.thread, heartbeatWorker, args) createThread(result.thread, heartbeatWorker, args)
proc updateStatus*(hb: ptr HeartbeatThread, status: HeartbeatStatus, proc updateStatus*(hb: HeartbeatThread, status: HeartbeatStatus,
task: string = "", progress: float = 0.0) = task: string = "", progress: float = 0.0) =
## Update the status reported in heartbeats ## Update the status reported in heartbeats
if hb != nil and hb.running: if hb != nil and hb.running:
@ -90,17 +95,17 @@ proc updateStatus*(hb: ptr HeartbeatThread, status: HeartbeatStatus,
progress: progress progress: progress
)) ))
proc stopHeartbeat*(hb: ptr HeartbeatThread) = proc stopHeartbeat*(hb: HeartbeatThread) =
## Stop the heartbeat thread and clean up ## Stop the heartbeat thread and clean up
if hb != nil and hb.running: if hb != nil and hb.running:
hb.channel.send(HeartbeatMsg(cmd: hbStop)) hb.channel.send(HeartbeatMsg(cmd: hbStop))
joinThread(hb.thread) joinThread(hb.thread)
hb.channel.close() hb.channel.close()
hb.running = false hb.running = false
dealloc(hb) # No dealloc needed - GC handles ref type
# Simpler API using a global channel (for single-threaded CLI usage) # Simpler API using global state (for single-threaded CLI usage)
var globalHeartbeat: ptr HeartbeatThread = nil var globalHeartbeat: HeartbeatThread = nil
proc startGlobalHeartbeat*(dbPath, agentId: string) = proc startGlobalHeartbeat*(dbPath, agentId: string) =
## Start heartbeat using global state (simpler API for CLI) ## Start heartbeat using global state (simpler API for CLI)