fix: address 4 Bugbot review comments

1. High Severity: Add session ID validation to prevent path traversal
   - Import validateIdentifier and check sessionId before file operations
   - Prevents ../../../ injection attacks on terminal endpoint

2. Medium Severity: Fix process cleanup race condition
   - Check instance identity before deletion in exit/error handlers
   - Prevents deleting newly spawned instance when old one exits

3. Low Severity: Fix trailing separators in meta chips
   - Only render separator if there's content after it
   - Prevents dangling · at end of chip row

4. Low Severity: Preserve full content for non-Bugbot comments
   - Check if comment is Bugbot format before extracting
   - Use full body as description for regular human comments

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit is contained in:
Prateek 2026-02-15 02:12:10 +05:30
parent f1c5269206
commit be7413338a
2 changed files with 44 additions and 16 deletions

View File

@ -82,15 +82,23 @@ function relativeTime(iso: string): string {
/** Clean up Bugbot comment body - extract title and description, remove HTML junk */
function cleanBugbotComment(body: string): { title: string; description: string } {
// Extract title (first ### heading)
const titleMatch = body.match(/###\s+(.+?)(?:\n|$)/);
const title = titleMatch ? titleMatch[1].replace(/\*\*/g, "").trim() : "Comment";
// Check if this is a Bugbot comment (has structured markers)
const isBugbot = body.includes("<!-- DESCRIPTION START -->") || body.includes("### ");
// Extract description between DESCRIPTION START/END comments
const descMatch = body.match(/<!-- DESCRIPTION START -->\s*([\s\S]*?)\s*<!-- DESCRIPTION END -->/);
const description = descMatch ? descMatch[1].trim() : body.split("\n")[0] || "No description";
if (isBugbot) {
// Extract title (first ### heading)
const titleMatch = body.match(/###\s+(.+?)(?:\n|$)/);
const title = titleMatch ? titleMatch[1].replace(/\*\*/g, "").trim() : "Comment";
return { title, description };
// Extract description between DESCRIPTION START/END comments
const descMatch = body.match(/<!-- DESCRIPTION START -->\s*([\s\S]*?)\s*<!-- DESCRIPTION END -->/);
const description = descMatch ? descMatch[1].trim() : body.split("\n")[0] || "No description";
return { title, description };
} else {
// For non-Bugbot comments, use full body as description
return { title: "Comment", description: body.trim() };
}
}
/** Builds a GitHub branch URL from PR owner/repo/branch. */
@ -217,7 +225,9 @@ export function SessionDetail({ session }: SessionDetailProps) {
>
#{pr.number}
</a>
<span className="text-[var(--color-text-muted)]">&middot;</span>
{(session.branch || session.issueUrl) && (
<span className="text-[var(--color-text-muted)]">&middot;</span>
)}
</>
)}
@ -237,7 +247,9 @@ export function SessionDetail({ session }: SessionDetailProps) {
{session.branch}
</span>
)}
<span className="text-[var(--color-text-muted)]">&middot;</span>
{session.issueUrl && (
<span className="text-[var(--color-text-muted)]">&middot;</span>
)}
</>
)}

View File

@ -18,6 +18,7 @@ import { createServer, request } from "node:http";
import { existsSync } from "node:fs";
import { join } from "node:path";
import { loadConfig } from "@agent-orchestrator/core";
import { validateIdentifier } from "../lib/validation.js";
interface TtydInstance {
sessionId: string;
@ -154,17 +155,24 @@ function getOrSpawnTtyd(sessionId: string): TtydInstance {
// Use once() for cleanup handlers to prevent race condition when both exit and error fire
proc.once("exit", (code) => {
console.log(`[Terminal] ttyd ${sessionId} exited with code ${code}`);
instances.delete(sessionId);
// Recycle port for reuse
availablePorts.add(port);
// Only delete if this is still the current instance (prevents race with error handler)
const current = instances.get(sessionId);
if (current?.process === proc) {
instances.delete(sessionId);
// Recycle port for reuse
availablePorts.add(port);
}
});
proc.once("error", (err) => {
console.error(`[Terminal] ttyd ${sessionId} error:`, err.message);
// Clean up instance on spawn error to prevent leak
instances.delete(sessionId);
// Recycle port for reuse
availablePorts.add(port);
// Only delete if this is still the current instance (prevents race with exit handler)
const current = instances.get(sessionId);
if (current?.process === proc) {
instances.delete(sessionId);
// Recycle port for reuse
availablePorts.add(port);
}
// Kill any running process
try {
proc.kill();
@ -216,6 +224,14 @@ const server = createServer(async (req, res) => {
return;
}
// Validate session ID to prevent path traversal and injection
const idErr = validateIdentifier(sessionId, "session");
if (idErr) {
res.writeHead(400, { "Content-Type": "application/json" });
res.end(JSON.stringify({ error: idErr }));
return;
}
// Validate session exists before spawning ttyd using configured dataDir
const sessionPath = join(config.dataDir, sessionId);
if (!existsSync(sessionPath)) {