Fix 30 issues: security vulns, correctness bugs, formula errors, robustness

RFingAdam · claude · RFingAdam · commit 98bc9a7da01f · 2026-03-20T23:15:24.000-05:00
Security (S1-S3): - Fix Zip Slip path traversal in ODB++ archive extraction (#42) - Fix XSS via unescaped user data in web session renderer (#43) - Fix SVG injection in HTML report inline embedding (#44) Correctness (C1-C10): - Map detected PCIe generation instead of hardcoding GEN3 (#45) - Fix IPC-2581 copper layer double-counting (#46) - Fix shielding aperture leakage power-sum formula (#47) - Fix BOM parser instance state leaking between calls (#48) - Fix Gerber step-repeat using wrong per-type feature counts (#49) - Fix KiCad parser dropping escaped characters in strings (#50) - Fix HTML report section title double-escaping score badges (#51) - Fix IPC-2581 _find()/_findall() dead branch for empty namespace (#52) - Fix ODB++ blank line prematurely terminating layer blocks (#53) - Fix ground plane layer_number using loop index vs actual layer (#54) Engineering formulas (E1-E5): - Fix decoupling trace ESL from 1 nH/mm to 0.03 nH/mm (#55) - Add Hammerstad correction to er_eff in antenna analyzers (#56) - Fix DDR microstrip er_eff formula (#57) - Fix swapped resonance/anti-resonance comments in PDN analyzer (#58) - Remove dead code from incomplete filter design refactor (#59) Robustness (R1-R12): - Add session memory limits with TTL eviction (#60) - Log full traceback on tool call exceptions (#61) - Document sync event-loop blocking as known limitation (#62) - Add session ID collision check (#63) - Guard against empty trace_lengths_mm in length matching (#64) - Validate CPW geometry before impedance calculation (#65) - Handle STEP escaped quotes and unterminated strings (#66) - Fix PDF schematic file handle leak with try/finally (#67) - Simplify IBIS c_comp_pf dead conversion branch (#68) - Fix eye diagram div-by-zero when height equals swing (#69) - Fix cavity resonance plot crash on empty modes (#70) - Replace O(N^2) recommendation lookup with pre-indexed dict (#71) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
diff --git a/src/mcp_pcb_emcopilot/analyzers/antenna/slot_antenna.py b/src/mcp_pcb_emcopilot/analyzers/antenna/slot_antenna.py
@@ -154,7 +154,8 @@ def calculate_slot_resonance(self, length_mm: float) -> float:
             Resonant frequency in MHz
         """
         length_m = length_mm / 1000
-        er_eff = (self.er + 1) / 2  # Effective dielectric
+        # Hammerstad approximation assuming typical w/h ≈ 1
+        er_eff = (self.er + 1) / 2 + (self.er - 1) / 2 * 0.277
 
         freq_hz = self.C / (2 * length_m * math.sqrt(er_eff))
         return freq_hz / 1e6
diff --git a/src/mcp_pcb_emcopilot/analyzers/antenna/trace_antenna.py b/src/mcp_pcb_emcopilot/analyzers/antenna/trace_antenna.py
@@ -141,8 +141,9 @@ def calculate_resonant_frequency(
         """
         length_m = length_mm / 1000
 
-        # Effective dielectric for microstrip (approximate)
-        er_eff = (self.er + 1) / 2
+        # Hammerstad approximation assuming typical w/h ≈ 1
+        # εr_eff = (εr+1)/2 + (εr-1)/2 * 1/sqrt(1+12h/w); for w/h=1: F≈0.277
+        er_eff = (self.er + 1) / 2 + (self.er - 1) / 2 * 0.277
 
         # Velocity factor
         vf = 1 / math.sqrt(er_eff)
diff --git a/src/mcp_pcb_emcopilot/analyzers/emc/filter_design.py b/src/mcp_pcb_emcopilot/analyzers/emc/filter_design.py
@@ -92,17 +92,7 @@ def _pi_filter_transfer(
     a21 = y_c1
     a22 = y_c1 * z_l + 1
 
-    # (M_c1 * M_l) * M_c2
-    A = a11
-    B = a12
-    C = a21 + a11 * y_c2
-    D = a22 + a12 * y_c2
-
-    # Correct: multiply on right by M_c2
-    A_final = a11 + a12 * y_c2  # Wait -- let me redo properly.
-    # Actually for right multiplication by [[1,0],[y_c2,1]]:
-    # [A B] = [a11 a12] * [[1,0],[y_c2,1]] = [a11+a12*y_c2, a12]
-    # [C D]   [a21 a22]   [[y_c2,1]]        [a21+a22*y_c2, a22]
+    # (M_c1 * M_l) * M_c2 — right multiply by [[1,0],[y_c2,1]]
     A_final = a11 + a12 * y_c2
     B_final = a12
     C_final = a21 + a22 * y_c2
diff --git a/src/mcp_pcb_emcopilot/analyzers/emc/shielding.py b/src/mcp_pcb_emcopilot/analyzers/emc/shielding.py
@@ -330,8 +330,8 @@ def _calculate_aperture_leakage(
             count = ap.get("count", 1)
 
             # Aperture coupling is additive in power
-            leakage_linear = 10 ** (-se_reduction / 20) * count
-            total_leakage_linear += leakage_linear ** 2
+            single_leakage = 10 ** (-se_reduction / 20)
+            total_leakage_linear += count * single_leakage ** 2
 
         # Convert back to dB reduction
         if total_leakage_linear > 0:
diff --git a/src/mcp_pcb_emcopilot/analyzers/high_speed/ddr_analyzer.py b/src/mcp_pcb_emcopilot/analyzers/high_speed/ddr_analyzer.py
@@ -304,10 +304,9 @@ def _calculate_prop_delay_from_stackup(
 
         # Calculate effective dielectric constant
         if is_outer:
-            # Microstrip: effective Er is approximately (Er + 1) / 2 for wide traces
-            # More accurate: Er_eff ≈ (Er + 1) / 2 + (Er - 1) / 2 * 1/sqrt(1 + 12h/w)
-            # Simplified approximation for typical DDR traces
-            er_eff = (dielectric_constant + 1) / 2 + (dielectric_constant - 1) / 3
+            # Microstrip Hammerstad: Er_eff ≈ (Er+1)/2 + (Er-1)/2 * 1/sqrt(1+12h/w)
+            # For typical DDR traces (w/h ≈ 1.5): F ≈ 0.217
+            er_eff = (dielectric_constant + 1) / 2 + (dielectric_constant - 1) / 2 * 0.217
         else:
             # Stripline: effective Er equals substrate Er
             er_eff = dielectric_constant
diff --git a/src/mcp_pcb_emcopilot/analyzers/power_integrity/decap_placement.py b/src/mcp_pcb_emcopilot/analyzers/power_integrity/decap_placement.py
@@ -280,8 +280,8 @@ def estimate_esl(
         # Via ESL
         via_esl = via_count * self.via_inductance
 
-        # Trace ESL (approximately 1 nH/mm for typical width)
-        trace_esl = trace_length_mm * 1.0
+        # Trace ESL over ground plane (~0.03 nH/mm for 0.2mm trace at 0.1mm height)
+        trace_esl = trace_length_mm * 0.03
 
         return pkg_esl + via_esl + trace_esl
 
diff --git a/src/mcp_pcb_emcopilot/analyzers/power_integrity/pdn_analyzer.py b/src/mcp_pcb_emcopilot/analyzers/power_integrity/pdn_analyzer.py
@@ -349,10 +349,10 @@ def analyze(
             is_resonance = False
             is_anti_resonance = False
             if prev_phase is not None:
-                # Resonance: phase crosses from negative to positive
+                # Anti-resonance (impedance peak): phase crosses negative to positive
                 if prev_phase < 0 and z_phase > 0:
                     is_anti_resonance = True
-                # Anti-resonance: phase crosses from positive to negative
+                # Series resonance (impedance minimum): phase crosses positive to negative
                 elif prev_phase > 0 and z_phase < 0:
                     is_resonance = True
 
diff --git a/src/mcp_pcb_emcopilot/orchestrator.py b/src/mcp_pcb_emcopilot/orchestrator.py
@@ -349,10 +349,11 @@ def _run_grounding_analysis(design: PCBDesignData) -> DomainResult:
         # Build ground planes from design zones
         planes = []
         gnd_zones = [z for z in design.zones if z.net_name and "gnd" in z.net_name.lower()]
+        layer_num_map = {l.name: idx for idx, l in enumerate(design.layers)} if design.layers else {}
         if gnd_zones:
             for i, zone in enumerate(gnd_zones):
                 planes.append(GroundPlane(
-                    layer_number=i,
+                    layer_number=layer_num_map.get(zone.layer, i),
                     name=zone.layer,
                     coverage_percent=80.0,
                     width_mm=design.board_width_mm or 100,
@@ -863,8 +864,12 @@ def _run_pcie_analysis(
                             lane[key] = length
             lanes.append(lane)
 
+        gen_map = {"pcie_gen1": PCIeGeneration.GEN1, "pcie_gen2": PCIeGeneration.GEN2, "pcie_gen3": PCIeGeneration.GEN3, "pcie_gen4": PCIeGeneration.GEN4, "pcie_gen5": PCIeGeneration.GEN5, "pcie_gen6": PCIeGeneration.GEN6}
+        itype = pcie_iface.interface_type.lower() if hasattr(pcie_iface, 'interface_type') else ""
+        pcie_gen = next((v for k, v in gen_map.items() if k in itype), PCIeGeneration.GEN3)
+
         pcie_result = analyzer.analyze(
-            generation=PCIeGeneration.GEN3,
+            generation=pcie_gen,
             lanes=lanes,
         )
 
@@ -1187,20 +1192,25 @@ def _build_recommendations(
     recs = []
     seen = set()
 
+    # Pre-build finding index by title for O(1) lookup
+    finding_by_title: dict[str, Any] = {}
+    for dr in domain_results:
+        for f in dr.findings:
+            if f.title not in finding_by_title and f.recommendation:
+                finding_by_title[f.title] = f
+
     # From risk matrix (highest risk first)
     for risk in risk_matrix:
         if risk.risk_score >= 4:
-            # Find the matching finding
-            for dr in domain_results:
-                for f in dr.findings:
-                    if f.title == risk.finding_title and f.recommendation and f.recommendation not in seen:
-                        seen.add(f.recommendation)
-                        recs.append({
-                            "priority": "high" if risk.risk_score >= 6 else "medium",
-                            "domain": f.domain,
-                            "recommendation": f.recommendation,
-                            "risk_score": risk.risk_score,
-                        })
+            matched = finding_by_title.get(risk.finding_title)
+            if matched and matched.recommendation not in seen:
+                seen.add(matched.recommendation)
+                recs.append({
+                    "priority": "high" if risk.risk_score >= 6 else "medium",
+                    "domain": matched.domain,
+                    "recommendation": matched.recommendation,
+                    "risk_score": risk.risk_score,
+                })
 
     # From cross-correlations
     for cc in cross_correlations:
diff --git a/src/mcp_pcb_emcopilot/parsers/bom_parser.py b/src/mcp_pcb_emcopilot/parsers/bom_parser.py
@@ -79,6 +79,9 @@ def parse(self, file_path: str) -> ParsedBOMData:
         Raises:
             ValueError: If file format is invalid or unsupported
         """
+        self.items = []
+        self.warnings = []
+
         logger.info(f"Parsing BOM file: {file_path}")
 
         path = Path(file_path)
diff --git a/src/mcp_pcb_emcopilot/parsers/gerber_parser.py b/src/mcp_pcb_emcopilot/parsers/gerber_parser.py
@@ -649,6 +649,11 @@ def _apply_step_repeat(self, sr: StepRepeat, data: GerberData) -> None:
         if sr.x_repeats <= 1 and sr.y_repeats <= 1:
             return
 
+        # Count features by type from the SR block
+        sr_trace_count = sum(1 for ftype, _ in sr.features if ftype == 'trace')
+        sr_pad_count = sum(1 for ftype, _ in sr.features if ftype == 'pad')
+        sr_arc_count = sum(1 for ftype, _ in sr.features if ftype == 'arc')
+
         original_traces = len(data.traces)
         original_pads = len(data.pads)
         original_arcs = len(data.arcs)
@@ -663,7 +668,7 @@ def _apply_step_repeat(self, sr: StepRepeat, data: GerberData) -> None:
                 dy = yi * sr.y_step
 
                 # Replicate traces
-                for i in range(original_traces - len(sr.features), original_traces):
+                for i in range(original_traces - sr_trace_count, original_traces):
                     if i >= 0 and i < len(data.traces):
                         t = data.traces[i]
                         data.traces.append(GerberTrace(
@@ -676,7 +681,7 @@ def _apply_step_repeat(self, sr: StepRepeat, data: GerberData) -> None:
                         ))
 
                 # Replicate pads
-                for i in range(original_pads - len(sr.features), original_pads):
+                for i in range(original_pads - sr_pad_count, original_pads):
                     if i >= 0 and i < len(data.pads):
                         p = data.pads[i]
                         data.pads.append(GerberPad(
diff --git a/src/mcp_pcb_emcopilot/parsers/ibis_parser.py b/src/mcp_pcb_emcopilot/parsers/ibis_parser.py
@@ -594,7 +594,8 @@ def analyze_ibis_timing(
 
     # C_comp loading effect
     c_comp = model_data.get("c_comp", {})
-    c_comp_pf = c_comp.get("typ", 0.0) * 1e12 if isinstance(c_comp.get("typ"), float) and c_comp.get("typ", 0) < 1e-6 else c_comp.get("typ", 0.0)
+    # parse_eng_notation stores in Farads; convert to pF
+    c_comp_pf = c_comp.get("typ", 0.0) * 1e12
 
     # Timing margin
     timing_margin_ps = eye_width_ps - (ui_ps * 0.3)  # 30% UI minimum opening
diff --git a/src/mcp_pcb_emcopilot/parsers/ipc2581_parser.py b/src/mcp_pcb_emcopilot/parsers/ipc2581_parser.py
@@ -394,7 +394,11 @@ def _find(self, element: ET.Element, path: str) -> Optional[ET.Element]:
             if prefix:
                 ns_path = "/".join(f"{{{uri}}}{p}" for p in path.split("/"))
             else:
-                ns_path = "/".join(f"{{{uri}}}{p}" for p in path.split("/"))
+                # Empty prefix: try bare path without namespace
+                result = element.find(path)
+                if result is not None:
+                    return result
+                continue
             result = element.find(ns_path)
             if result is not None:
                 return result
@@ -497,7 +501,6 @@ def _parse_stackup(self, root: ET.Element, data: IPC2581Data) -> None:
                 side = "TOP"
             elif layer_type in ("SIGNAL", "PLANE"):
                 side = "INTERNAL"
-                copper_layers += 1
             else:
                 side = "INTERNAL"
 
diff --git a/src/mcp_pcb_emcopilot/parsers/kicad_pcb_parser.py b/src/mcp_pcb_emcopilot/parsers/kicad_pcb_parser.py
@@ -280,19 +280,22 @@ def _parse_list(self) -> List:
     def _parse_string(self) -> str:
         """Parse a quoted string."""
         self.pos += 1  # Skip opening quote
-        start = self.pos
+        result_chars: list[str] = []
 
         while self.pos < self.length:
-            if self.text[self.pos] == '"':
-                result = self.text[start:self.pos]
+            ch = self.text[self.pos]
+            if ch == '"':
+                self.pos += 1
+                return ''.join(result_chars)
+            elif ch == '\\' and self.pos + 1 < self.length:
+                self.pos += 1
+                result_chars.append(self.text[self.pos])
                 self.pos += 1
-                return result
-            elif self.text[self.pos] == '\\' and self.pos + 1 < self.length:
-                self.pos += 2  # Skip escape sequence
             else:
+                result_chars.append(ch)
                 self.pos += 1
 
-        return self.text[start:]
+        return ''.join(result_chars)
 
     def _parse_atom(self) -> str:
         """Parse an unquoted atom."""
diff --git a/src/mcp_pcb_emcopilot/parsers/odb_parser.py b/src/mcp_pcb_emcopilot/parsers/odb_parser.py
@@ -8,6 +8,7 @@
 import shutil
 import tarfile
 import tempfile
+import zipfile
 from pathlib import Path
 from typing import Any, BinaryIO, Dict, List, Optional, Tuple
 
@@ -170,19 +171,59 @@ def _get_extract_dir(self, archive_path: Path) -> Path:
             self._temp_dir = Path(tempfile.mkdtemp(prefix="odb_"))
             return self._temp_dir
 
+    @staticmethod
+    def _validate_tar_members(
+        tar: tarfile.TarFile, extract_dir: Path
+    ) -> list[tarfile.TarInfo]:
+        """Return tar members whose paths resolve within *extract_dir*.
+
+        Raises ``ValueError`` on any path-traversal attempt (Zip-Slip).
+        """
+        safe: list[tarfile.TarInfo] = []
+        resolved = extract_dir.resolve()
+        for member in tar.getmembers():
+            target = (resolved / member.name).resolve()
+            if not str(target).startswith(str(resolved) + os.sep) and target != resolved:
+                raise ValueError(
+                    f"Blocked path-traversal in tar member: {member.name}"
+                )
+            safe.append(member)
+        return safe
+
+    @staticmethod
+    def _validate_zip_members(
+        zip_ref: zipfile.ZipFile, extract_dir: Path
+    ) -> list[str]:
+        """Return zip member names whose paths resolve within *extract_dir*.
+
+        Raises ``ValueError`` on any path-traversal attempt (Zip-Slip).
+        """
+        safe: list[str] = []
+        resolved = extract_dir.resolve()
+        for name in zip_ref.namelist():
+            target = (resolved / name).resolve()
+            if not str(target).startswith(str(resolved) + os.sep) and target != resolved:
+                raise ValueError(
+                    f"Blocked path-traversal in zip member: {name}"
+                )
+            safe.append(name)
+        return safe
+
     def _extract_archive(self, archive_path: Path, extract_dir: Path) -> None:
-        """Extract ODB++ archive (tgz or zip)"""
+        """Extract ODB++ archive (tgz or zip) with path-traversal protection."""
         archive_str = str(archive_path).lower()
 
         if archive_str.endswith(('.tgz', '.tar.gz')):
             with tarfile.open(archive_path, 'r:gz') as tar:
-                tar.extractall(extract_dir)
+                safe_members = self._validate_tar_members(tar, extract_dir)
+                tar.extractall(extract_dir, members=safe_members)
         elif archive_str.endswith('.tar'):
             with tarfile.open(archive_path, 'r') as tar:
-                tar.extractall(extract_dir)
+                safe_members = self._validate_tar_members(tar, extract_dir)
+                tar.extractall(extract_dir, members=safe_members)
         elif archive_str.endswith('.zip'):
-            import zipfile
             with zipfile.ZipFile(archive_path, 'r') as zip_ref:
+                self._validate_zip_members(zip_ref, extract_dir)
                 zip_ref.extractall(extract_dir)
         else:
             raise ValueError(f"Unsupported archive format: {archive_path.suffix}")
@@ -245,7 +286,9 @@ def _parse_matrix(self, odb_root: Path, data: ODBData) -> None:
 
                     current_layer = {}
 
-                elif line.startswith('END_LAYER') or line == '':
+                elif line == '':
+                    continue
+                elif line.startswith('END_LAYER'):
                     if current_layer:
                         data.layers.append(self._create_layer(current_layer))
                         current_layer = None
diff --git a/src/mcp_pcb_emcopilot/parsers/pdf_schematic_parser.py b/src/mcp_pcb_emcopilot/parsers/pdf_schematic_parser.py
@@ -320,25 +320,25 @@ def render_page_image(
 
         fitz = self._fitz
         doc = fitz.open(file_path)  # type: ignore[union-attr]
-
-        if page_number < 1 or page_number > len(doc):
+        try:
+            if page_number < 1 or page_number > len(doc):
+                raise ValueError(
+                    f"Page {page_number} out of range (1-{len(doc)})"
+                )
+
+            page = doc[page_number - 1]
+            zoom = dpi / 72.0
+            mat = fitz.Matrix(zoom, zoom)  # type: ignore[union-attr]
+            pix = page.get_pixmap(matrix=mat)
+
+            os.makedirs(output_dir, exist_ok=True)
+            stem = Path(file_path).stem
+            out_path = os.path.join(output_dir, f"{stem}_page{page_number}.png")
+            pix.save(out_path)
+
+            return out_path
+        finally:
             doc.close()
-            raise ValueError(
-                f"Page {page_number} out of range (1-{len(doc)})"
-            )
-
-        page = doc[page_number - 1]
-        zoom = dpi / 72.0
-        mat = fitz.Matrix(zoom, zoom)  # type: ignore[union-attr]
-        pix = page.get_pixmap(matrix=mat)
-
-        os.makedirs(output_dir, exist_ok=True)
-        stem = Path(file_path).stem
-        out_path = os.path.join(output_dir, f"{stem}_page{page_number}.png")
-        pix.save(out_path)
-
-        doc.close()
-        return out_path
 
     # ------------------------------------------------------------------
     # Text-analysis helpers
diff --git a/src/mcp_pcb_emcopilot/parsers/step_parser.py b/src/mcp_pcb_emcopilot/parsers/step_parser.py
diff --git a/src/mcp_pcb_emcopilot/reports/html_report.py b/src/mcp_pcb_emcopilot/reports/html_report.py
diff --git a/src/mcp_pcb_emcopilot/reports/simulation_plots.py b/src/mcp_pcb_emcopilot/reports/simulation_plots.py
diff --git a/src/mcp_pcb_emcopilot/server.py b/src/mcp_pcb_emcopilot/server.py
diff --git a/src/mcp_pcb_emcopilot/session.py b/src/mcp_pcb_emcopilot/session.py
diff --git a/src/mcp_pcb_emcopilot/web/app.py b/src/mcp_pcb_emcopilot/web/app.py
diff --git a/tests/test_ipc2581_parser.py b/tests/test_ipc2581_parser.py
