Address PR review comments: security, validation, and code quality fixes

tomusdrw · tomusdrw · commit baed359aecdf · 2026-02-08T11:51:35.000+01:00
Major fixes:

- Links.tsx: Add rel="noopener noreferrer" to external links for security

- host-call-trace.ts: Add Zod validation for trace structure

- debuggerSlice.ts: Add input validation in setHostCallsTrace reducer

- workersSlice.ts: Fix auto-continue to advance host-call index and await resume

- useDebuggerActions.ts: Add guard for undefined memoryRanges

- findHostCallEntry: Prefer exact host call index matches with early return

Minor fixes:

- trace-loading.spec.ts: Use shared Playwright fixtures

- Create tests/utils/fixtures.ts for shared test utilities
diff --git a/src/components/ProgramLoader/Links.tsx b/src/components/ProgramLoader/Links.tsx
@@ -5,7 +5,12 @@ export const Links = () => {
     <ul className="list-none sm:text-sm">
       <li>
         <div className="flex gap-2 text-[11px]">
-          <a className="flex" href="https://github.com/w3f/jamtestvectors/pull/3/files" target="_blank">
+          <a
+            className="flex"
+            href="https://github.com/w3f/jamtestvectors/pull/3/files"
+            target="_blank"
+            rel="noopener noreferrer"
+          >
             <ExternalLink className="inline w-4 mb-1 mr-2 text-xs text-brand-dark dark:text-brand" />
           </a>
           <div>
@@ -25,7 +30,7 @@ export const Links = () => {
 
       <li>
         <div className="flex gap-2 text-[11px] mt-3">
-          <a href="https://graypaper.fluffylabs.dev/#/5b732de/2a7e022a7e02" target="_blank">
+          <a href="https://graypaper.fluffylabs.dev/#/5b732de/2a7e022a7e02" target="_blank" rel="noopener noreferrer">
             <ExternalLink className="inline w-4 mb-1 mr-2 text-brand-dark dark:text-brand" />
           </a>
           <div>
@@ -46,7 +51,7 @@ export const Links = () => {
       </li>
       <li>
         <div className="flex gap-2 text-[11px] mt-3">
-          <a href="https://graypaper.fluffylabs.dev/#/5b732de/23c60023c600" target="_blank">
+          <a href="https://graypaper.fluffylabs.dev/#/5b732de/23c60023c600" target="_blank" rel="noopener noreferrer">
             <ExternalLink className="inline w-4 mb-1 mr-2 text-brand-dark dark:text-brand" />
           </a>
           <div>
diff --git a/src/hooks/useDebuggerActions.ts b/src/hooks/useDebuggerActions.ts
@@ -79,7 +79,8 @@ export const useDebuggerActions = () => {
       dispatch(resetHostCallIndex());
 
       // Save memory ranges before destroying workers
-      const memoryRanges = workers[0]?.memoryRanges ?? [];
+      // Guard: ensure memoryRanges is always an array
+      const memoryRanges = Array.isArray(workers[0]?.memoryRanges) ? workers[0].memoryRanges : [];
 
       // Destroy all existing workers to clear message listeners
       await Promise.all(workers.map((worker: WorkerState) => dispatch(destroyWorker(worker.id)).unwrap()));
@@ -176,7 +177,8 @@ export const useDebuggerActions = () => {
     async (selectedPvms: SelectedPvmWithPayload[]) => {
       logger.debug("selectedPvms vs workers ", selectedPvms, workers);
 
-      const memoryRanges = workers[0]?.memoryRanges ?? [];
+      // Guard: ensure memoryRanges is always an array
+      const memoryRanges = Array.isArray(workers[0]?.memoryRanges) ? workers[0].memoryRanges : [];
 
       // Destroy all existing workers
       await Promise.all(workers.map((worker: WorkerState) => dispatch(destroyWorker(worker.id)).unwrap()));
diff --git a/src/lib/host-call-trace.ts b/src/lib/host-call-trace.ts
@@ -8,6 +8,8 @@
  * @see https://github.com/tomusdrw/JIPs/blob/td-jip6-ecalliloggin/JIP-6.md
  */
 
+import { z } from "zod";
+
 /** Register dump: mapping from register index to value */
 export type RegisterDump = Map<number, bigint>;
 
@@ -121,6 +123,128 @@ export interface StateMismatch {
   details?: string;
 }
 
+// ============================================================================
+// Zod Validation Schemas
+// ============================================================================
+
+/** Zod schema for MemoryRead */
+const MemoryReadSchema = z.object({
+  address: z.number(),
+  length: z.number(),
+  data: z.instanceof(Uint8Array),
+});
+
+/** Zod schema for MemoryWrite */
+const MemoryWriteSchema = z.object({
+  address: z.number(),
+  length: z.number(),
+  data: z.instanceof(Uint8Array),
+});
+
+/** Zod schema for RegisterWrite */
+const RegisterWriteSchema = z.object({
+  index: z.number(),
+  value: z.bigint(),
+});
+
+/** Zod schema for HostCallEntry */
+const HostCallEntrySchema = z.object({
+  index: z.number(),
+  pc: z.number(),
+  gas: z.bigint(),
+  registers: z.instanceof(Map),
+  memoryReads: z.array(MemoryReadSchema),
+  memoryWrites: z.array(MemoryWriteSchema),
+  registerWrites: z.array(RegisterWriteSchema),
+  gasAfter: z.bigint().nullable(),
+  lineNumber: z.number(),
+});
+
+/** Zod schema for StartEntry */
+const StartEntrySchema = z.object({
+  pc: z.number(),
+  gas: z.bigint(),
+  registers: z.instanceof(Map),
+});
+
+/** Zod schema for TracePrelude */
+const TracePreludeSchema = z.object({
+  program: z.string().nullable(),
+  start: StartEntrySchema.nullable(),
+  initialMemoryWrites: z.array(MemoryWriteSchema),
+});
+
+/** Zod schema for TerminationReason */
+const TerminationReasonSchema = z.union([
+  z.object({ type: z.literal("HALT") }),
+  z.object({ type: z.literal("PANIC"), argument: z.number() }),
+  z.object({ type: z.literal("OOG") }),
+]);
+
+/** Zod schema for TerminationEntry */
+const TerminationEntrySchema = z.object({
+  reason: TerminationReasonSchema,
+  pc: z.number(),
+  gas: z.bigint(),
+  registers: z.instanceof(Map),
+  lineNumber: z.number(),
+});
+
+/** Zod schema for TraceParseError */
+const TraceParseErrorSchema = z.object({
+  lineNumber: z.number(),
+  line: z.string(),
+  message: z.string(),
+});
+
+/** Zod schema for ParsedTrace */
+const ParsedTraceSchema = z.object({
+  contextLines: z.array(z.string()),
+  prelude: TracePreludeSchema,
+  hostCalls: z.array(HostCallEntrySchema),
+  termination: TerminationEntrySchema.nullable(),
+  errors: z.array(TraceParseErrorSchema),
+});
+
+/**
+ * Validate a parsed trace structure using Zod
+ * @param trace The parsed trace to validate
+ * @returns Zod safe parse result
+ */
+export function validateParsedTrace(trace: unknown) {
+  return ParsedTraceSchema.safeParse(trace);
+}
+
+/**
+ * Validate trace content string and return detailed validation result
+ * @param content The trace content to validate
+ * @returns Object containing validation result and any errors
+ */
+export function validateTraceContent(content: string): {
+  success: boolean;
+  errors: TraceParseError[];
+  parseErrors?: z.ZodError;
+} {
+  const parsed = parseTrace(content);
+
+  // Check for parse errors first
+  if (parsed.errors.length > 0) {
+    return { success: false, errors: parsed.errors };
+  }
+
+  // Validate structure with Zod
+  const validation = validateParsedTrace(parsed);
+  if (!validation.success) {
+    return {
+      success: false,
+      errors: parsed.errors,
+      parseErrors: validation.error,
+    };
+  }
+
+  return { success: true, errors: [] };
+}
+
 // ============================================================================
 // Parsing utilities
 // ============================================================================
@@ -646,17 +770,74 @@ export function findHostCallEntry(
   hostCallIndex: number,
   readMemory?: (address: number, length: number) => Uint8Array | null,
 ): HostCallLookupResult {
-  // Try to find an exact match by PC
-  const matchingEntries = trace.hostCalls.slice(indexInTrace).filter((hc) => hc.pc === pc && hc.gas <= gas);
+  // Get remaining entries from the current index
+  const remainingEntries = trace.hostCalls.slice(indexInTrace);
+
+  // Try to find an exact match by PC and host call index
+  const matchingEntries = remainingEntries.filter((hc) => hc.pc === pc && hc.gas <= gas);
 
   if (matchingEntries.length === 0) {
     return { entry: null, mismatches: [] };
   }
 
-  // First try to find an entry whose hostCallIndex matches and has acceptable gas
-  const exactMatch = matchingEntries.find((hc) => hc.index === hostCallIndex && hc.gas <= gas);
-  // Use exact match if found, otherwise fall back to the first matching entry
-  const entry = exactMatch ?? matchingEntries[0];
+  // First try to find an entry whose hostCallIndex matches exactly
+  // Prefer exact index match even if gas is slightly higher
+  const exactIndexMatch = remainingEntries.find((hc) => hc.index === hostCallIndex && hc.pc === pc);
+
+  // If we have an exact index match with acceptable gas, use it
+  if (exactIndexMatch && exactIndexMatch.gas <= gas) {
+    const mismatches: StateMismatch[] = [];
+
+    // Check gas
+    if (exactIndexMatch.gas !== gas) {
+      mismatches.push({
+        field: "gas",
+        expected: exactIndexMatch.gas.toString(),
+        actual: gas.toString(),
+      });
+    }
+
+    // Check registers
+    for (const [idx, expectedValue] of exactIndexMatch.registers) {
+      const actualValue = registers[idx] ?? 0n;
+      if (expectedValue !== actualValue) {
+        mismatches.push({
+          field: "register",
+          expected: `r${idx}=0x${expectedValue.toString(16)}`,
+          actual: `r${idx}=0x${actualValue.toString(16)}`,
+          details: `Register ${idx}`,
+        });
+      }
+    }
+
+    // Check memory reads if readMemory function is provided
+    if (readMemory) {
+      for (const mr of exactIndexMatch.memoryReads) {
+        const actualData = readMemory(mr.address, mr.length);
+        if (actualData) {
+          const expectedHex = Array.from(mr.data)
+            .map((b) => b.toString(16).padStart(2, "0"))
+            .join("");
+          const actualHex = Array.from(actualData)
+            .map((b) => b.toString(16).padStart(2, "0"))
+            .join("");
+          if (expectedHex !== actualHex) {
+            mismatches.push({
+              field: "memread",
+              expected: `0x${expectedHex}`,
+              actual: `0x${actualHex}`,
+              details: `Memory at 0x${mr.address.toString(16)} (${mr.length} bytes)`,
+            });
+          }
+        }
+      }
+    }
+
+    return { entry: exactIndexMatch, mismatches };
+  }
+
+  // Fall back to first matching entry by PC and gas
+  const entry = matchingEntries[0];
   const mismatches: StateMismatch[] = [];
 
   // Check PC
diff --git a/src/store/debugger/debuggerSlice.ts b/src/store/debugger/debuggerSlice.ts
@@ -13,7 +13,7 @@ import { RootState } from "@/store";
 import { SelectedPvmWithPayload } from "@/components/PvmSelect";
 import { PvmTypes } from "@/packages/web-worker/types.ts";
 import { logger } from "@/utils/loggerService.tsx";
-import { HostCallEntry, ParsedTrace, parseTrace, StateMismatch, validateTrace } from "@/lib/host-call-trace";
+import { HostCallEntry, ParsedTrace, parseTrace, StateMismatch, validateTraceContent } from "@/lib/host-call-trace";
 
 export type UiRefreshMode = "instructions" | "block";
 
@@ -229,11 +229,17 @@ const debuggerSlice = createSlice({
         state.nextHostCallIndex = 0;
         state.pendingHostCall = null;
       } else {
-        // Validate trace before parsing
-        const validationErrors = validateTrace(action.payload);
-        if (validationErrors.length > 0) {
-          console.error("Trace validation errors:", validationErrors);
+        // Validate trace content (includes both parse errors and Zod structure validation)
+        const validation = validateTraceContent(action.payload);
+        if (!validation.success) {
+          console.error("Trace validation errors:", validation.errors);
+          if (validation.parseErrors) {
+            console.error("Trace structure validation errors:", validation.parseErrors);
+          }
           // Don't set invalid trace - keep existing state or set to null
+          state.hostCallsTrace = null;
+          state.nextHostCallIndex = 0;
+          state.pendingHostCall = null;
           return;
         }
 
diff --git a/src/store/workers/workersSlice.ts b/src/store/workers/workersSlice.ts
@@ -358,10 +358,16 @@ export const handleHostCall = createAsyncThunk(
         }),
     );
 
-    logger.info("  [handleHostCall] Done - returning without opening dialog");
-    // Advance host call index for all workers and await resuming execution
+    logger.info("  [handleHostCall] Done - auto-continuing without opening dialog");
+    // Advance host call index before resuming execution
+    dispatch(
+      setPendingHostCall({
+        pendingHostCall: null,
+        nextHostCallIndex: nextHostCallIndex + 1,
+      }),
+    );
+    // Resume execution
     await dispatch(runAllWorkers()).unwrap();
-    return;
   },
 );
 
diff --git a/tests/trace-loading.spec.ts b/tests/trace-loading.spec.ts
@@ -1,17 +1,13 @@
 import { test, expect } from "@playwright/test";
-import * as path from "path";
-import { fileURLToPath } from "url";
-
-const __filename = fileURLToPath(import.meta.url);
-const __dirname = path.dirname(__filename);
+import { FIXTURES, getFixturePath } from "./utils/fixtures";
 
 test.describe("Trace file loading", () => {
   test("should load io-trace-output.log without crashing", async ({ page }) => {
     await page.goto("/", { waitUntil: "domcontentloaded" });
     await page.evaluate(() => window.localStorage.clear());
 
     const fileInput = page.locator('input[type="file"]');
-    const tracePath = path.join(__dirname, "../io-trace-output.log");
+    const tracePath = getFixturePath(FIXTURES.IO_TRACE_OUTPUT);
 
     await fileInput.setInputFiles(tracePath);
 
@@ -49,7 +45,7 @@ test.describe("Trace file loading", () => {
     await page.evaluate(() => window.localStorage.clear());
 
     const fileInput = page.locator('input[type="file"]');
-    const tracePath = path.join(__dirname, "../io-trace-output.log");
+    const tracePath = getFixturePath(FIXTURES.IO_TRACE_OUTPUT);
 
     await fileInput.setInputFiles(tracePath);
 
diff --git a/tests/utils/fixtures.ts b/tests/utils/fixtures.ts
