From 26b501b600f6978143b27271c04e83efc17579ba Mon Sep 17 00:00:00 2001 From: Myron Blair Date: Thu, 2 Jul 2026 10:26:47 -0500 Subject: [PATCH] fix: address 8 code-review findings in KB topic manager - Fix 1 (ORDER BY): use table alias t.id to force integer PK ordering; topic_id alias was causing alphabetical sort, breaking rotation offsets - Fix 2 (PDOException): wrap INSERT/UPDATE in try/catch in kb_topic_save; duplicate topic_id now returns clean JSON error instead of HTTP 500 - Fix 3 (XSS/onclick): DEL button passes only t.id; tmDelete looks up name from _topicsData, removing JSON.stringify from onclick attribute - Fix 4 (validation): topic_id must contain at least one [a-z0-9] after sanitization; "@@@" to "___" no longer passes required-field check - Fix 5 (stale logs): guard comment and log messages updated from 20h to 4h to match the actual 14400s threshold - Fix 6 (dedup): tmEsc() replaced with existing esc() throughout; removed duplicate function definition - Fix 7 (toggle rollback): tmToggle uses inline fetch to revert el.checked on API failure instead of leaving the UI in wrong state - Fix 8 (confirm): tmDelete uses openModal confirmation instead of confirm() dialog, consistent with project live-popup convention Co-Authored-By: Claude Sonnet 4.6 Claude-Session: https://claude.ai/code/session_014p87VFec84hNaf2WpvmLrW --- api/endpoints/kb_intent_generator.php | 10 ++-- public_html/admin/index.php | 72 ++++++++++++++++----------- 2 files changed, 48 insertions(+), 34 deletions(-) diff --git a/api/endpoints/kb_intent_generator.php b/api/endpoints/kb_intent_generator.php index 064c868..1bc4c26 100644 --- a/api/endpoints/kb_intent_generator.php +++ b/api/endpoints/kb_intent_generator.php @@ -69,24 +69,24 @@ function normalize_pattern(string $pat): string { return '/' . $pat . '/i'; } -/* ── daily guard: skip if already ran within last 20 hours ── */ +/* ── run guard: skip if ran within last 4 hours ── */ /* Set JARVIS_FORCE_RUN=1 (env) or pass --force (argv) to bypass */ $lastRun = JarvisDB::single( "SELECT updated_at FROM kb_facts WHERE category='kb_generator' AND fact_key='last_run'" ); $forceRun = !empty(getenv('JARVIS_FORCE_RUN')) || (isset($argv[1]) && $argv[1] === '--force'); if (!$forceRun && $lastRun && (time() - strtotime($lastRun['updated_at'])) < 14400) { - log_line('Skipping – ran within last 20 hours (next run tomorrow 3am). Use --force to override.'); + log_line('Skipping – ran within last 4 hours. Use --force to override.'); exit(0); } -if ($forceRun) log_line('Force-run flag set — bypassing 20-hour guard.'); +if ($forceRun) log_line('Force-run flag set — bypassing 4-hour guard.'); log_line('Starting daily KB intent generation run.'); /* ── load active topics from database ── */ $BATCHES = JarvisDB::query( - "SELECT topic_id AS id, category, topic_name AS topic, description AS `desc` - FROM kb_generator_topics WHERE active=1 ORDER BY id ASC" + "SELECT t.topic_id AS id, t.category, t.topic_name AS topic, t.description AS `desc` + FROM kb_generator_topics t WHERE t.active=1 ORDER BY t.id ASC" ); if (empty($BATCHES)) { log_line('ERROR: No active topics in kb_generator_topics table. Add topics via the JARVIS admin panel.'); diff --git a/public_html/admin/index.php b/public_html/admin/index.php index 37837bf..c858e63 100644 --- a/public_html/admin/index.php +++ b/public_html/admin/index.php @@ -641,20 +641,26 @@ if ($action) { $name = trim($_POST['topic_name'] ?? ''); $desc = trim($_POST['description'] ?? ''); $act = (int)!empty($_POST['active']); - if (!$tid||!$cat||!$name||!$desc) bad('All fields required'); - if ($id) { - JarvisDB::execute( - 'UPDATE kb_generator_topics SET topic_id=?,category=?,topic_name=?,description=?,active=?,updated_at=NOW() WHERE id=?', - [$tid,$cat,$name,$desc,$act,$id] - ); - j(['ok'=>true,'msg'=>'Topic updated']); - } else { - JarvisDB::execute( - 'INSERT INTO kb_generator_topics (topic_id,category,topic_name,description,active) VALUES (?,?,?,?,?)', - [$tid,$cat,$name,$desc,$act] - ); - j(['ok'=>true,'msg'=>'Topic created']); + if (!$tid || !preg_match('/[a-z0-9]/', $tid) || !$cat || !$name || !$desc) + bad('All fields required — topic_id must contain at least one letter or digit'); + try { + if ($id) { + JarvisDB::execute( + 'UPDATE kb_generator_topics SET topic_id=?,category=?,topic_name=?,description=?,active=?,updated_at=NOW() WHERE id=?', + [$tid,$cat,$name,$desc,$act,$id] + ); + } else { + JarvisDB::execute( + 'INSERT INTO kb_generator_topics (topic_id,category,topic_name,description,active) VALUES (?,?,?,?,?)', + [$tid,$cat,$name,$desc,$act] + ); + } + } catch (\PDOException $e) { + bad(strpos($e->getMessage(), 'Duplicate entry') !== false + ? 'topic_id already in use — choose a different slug' + : 'Database error'); } + j(['ok'=>true,'msg'=> $id ? 'Topic updated' : 'Topic created']); case 'kb_topic_delete': $id = (int)($_POST['id'] ?? 0); if (!$id) bad('Missing id'); @@ -2557,16 +2563,16 @@ function tmFilter() { ${rows.map(t => ` - ${tmEsc(t.topic_id)} - ${tmEsc(t.category)} - ${tmEsc(t.topic_name)} + ${esc(t.topic_id)} + ${esc(t.category)} + ${esc(t.topic_name)} ${t.run_count||0} - + `).join('')} `; @@ -2630,24 +2636,32 @@ async function tmSaveTopic() { } async function tmToggle(id, el) { - apiPost('kb_topic_toggle', {id}, (res) => { + const prev = !el.checked; + const fd = new FormData(); + fd.append('action', 'kb_topic_toggle'); + fd.append('id', id); + try { + const r = await fetch(location.href, {method:'POST', body:fd}); + const d = await r.json(); + if (d.error) { toast(d.error, 'err'); el.checked = prev; return; } const t = _topicsData.find(x => x.id == id); - if (t) t.active = res.active ? 1 : 0; + if (t) t.active = d.active ? 1 : 0; tmFilter(); - }); + } catch(e) { toast('Toggle failed', 'err'); el.checked = prev; } } -async function tmDelete(id, name) { - if (!confirm('Delete topic "' + name + '"?\nThis cannot be undone.')) return; - apiPost('kb_topic_delete', {id}, async () => { - toast('Topic deleted', 'ok'); - await tmLoadTopics(); - }); +async function tmDelete(id) { + const t = _topicsData.find(x => x.id == id); + const name = t?.topic_name || 'this topic'; + openModal('CONFIRM DELETE', + `
Delete topic:
` + + `${esc(name)}
` + + `This cannot be undone.
`, + () => { apiPost('kb_topic_delete', {id}, async () => { toast('Topic deleted', 'ok'); await openTopicsManager(); }); }, + 'DELETE'); } -function tmEsc(s) { - return String(s||'').replace(/&/g,'&').replace(//g,'>').replace(/"/g,'"'); -} +