Fix circuit-breaker false trips, upload path traversal, and HTML expo…

pyproject.toml

@@ -77,4 +77,5 @@ where = ["src"]
 
 [tool.pytest.ini_options]
 testpaths = ["tests"]
+pythonpath = ["src"]
 asyncio_mode = "auto"

src/factor/app.py

@@ -117,8 +117,18 @@ async def analyze_documents(files: list[UploadFile] = File(...)):
                 detail=f"File {f.filename} exceeds {settings.factor_max_upload_mb}MB limit",
             )
 
-        file_path = upload_dir / (f.filename or f"document_{len(saved_paths)}")
+        safe_name = Path(f.filename).name if f.filename else ""
+        if not safe_name or safe_name in (".", ".."):
+            safe_name = f"document_{len(saved_paths)}"
+        file_path = (upload_dir / safe_name).resolve()
+        if not file_path.is_relative_to(upload_dir.resolve()):
+            raise HTTPException(status_code=400, detail=f"Invalid filename: {f.filename}")
         content = await f.read()
+        if len(content) > settings.max_upload_bytes:
+            raise HTTPException(
+                status_code=400,
+                detail=f"File {f.filename} exceeds {settings.factor_max_upload_mb}MB limit",
+            )
         file_path.write_bytes(content)
         saved_paths.append(str(file_path))
 
@@ -169,7 +179,7 @@ async def event_stream() -> AsyncGenerator[dict, None]:
 
             for doc_id, provisions in all_provisions.items():
                 detected_types = []
-                for prov in provisions:
+                for prov_index, prov in enumerate(provisions):
                     detection = detect_provision_type(provision_text=prov["text"])
                     prov["provision_type"] = detection["provision_type"]
                     detected_types.append(detection["provision_type"])
@@ -179,7 +189,10 @@ async def event_stream() -> AsyncGenerator[dict, None]:
                     all_risk_scores.append(risk)
 
                     if breaker:
-                        breaker.record_step(action="score_risk", meta={"doc_id": doc_id})
+                        breaker.record_step(
+                            action="score_risk",
+                            meta={"doc_id": doc_id, "provision_index": prov_index},
+                        )
 
                 gaps = find_gaps(detected_provisions=detected_types, doc_type="unknown")
                 for gap in gaps:

src/factor/harness/circuit_breaker.py

@@ -55,7 +55,10 @@ def record_step(
     ) -> None:
         """Record a reasoning step and check trip conditions."""
         self.budget.record(input_tokens, output_tokens, meta)
-        self.loop_detector.record(action)
+        signature = action
+        if meta:
+            signature = f"{action}:{sorted(meta.items())!r}"
+        self.loop_detector.record(signature)
 
         logger.debug(
             "Session %s step: action=%s in=%d out=%d cost=$%.4f",

src/factor/tools/export.py

@@ -228,17 +228,17 @@ def export_html(report: dict, output_path: str) -> str:
     <div class="disclaimer">
         <strong>⚠️ Note:</strong> Content in this section may reference synthetic data from Taylor658/synthetic-legal. All citations are artificially generated.
     </div>
-    {% if section.items %}
+    {% if section['items'] %}
     <table>
         <thead>
             <tr>
-                {% for key in section.items[0].keys() %}
+                {% for key in section['items'][0].keys() %}
                 <th>{{ key | replace('_', ' ') | title }}</th>
                 {% endfor %}
             </tr>
         </thead>
         <tbody>
-            {% for item in section.items %}
+            {% for item in section['items'] %}
             <tr>
                 {% for val in item.values() %}
                 <td>{{ val }}</td>
@@ -260,7 +260,7 @@ def export_html(report: dict, output_path: str) -> str:
 </body>
 </html>"""
 
-    env = Environment(loader=BaseLoader())
+    env = Environment(loader=BaseLoader(), autoescape=True)
     template = env.from_string(template_str)
     html_content = template.render(report=report)
 

tests/test_app_upload.py

@@ -0,0 +1,27 @@
+"""Tests for upload handling in the analyze endpoint."""
+
+from __future__ import annotations
+
+
+from fastapi.testclient import TestClient
+
+from factor.app import app
+
+
+def test_upload_filename_traversal_is_sanitized(tmp_path, monkeypatch):
+    """A filename containing path traversal must not escape the upload dir."""
+    monkeypatch.chdir(tmp_path)
+    client = TestClient(app)
+
+    response = client.post(
+        "/api/v1/analyze",
+        files={"files": ("../../escape.txt", b"Section 1. Test provision text.", "text/plain")},
+    )
+    assert response.status_code == 200
+    # Analysis ran (session event streamed), i.e. the file was accepted
+    assert "session_id" in response.text
+
+    # Nothing may be written outside uploads/<session_id>/
+    assert not (tmp_path / "escape.txt").exists()
+    assert not (tmp_path.parent / "escape.txt").exists()
+    assert not (tmp_path.parent.parent / "escape.txt").exists()

tests/test_harness.py

@@ -165,6 +165,38 @@ def test_loop_trip(self):
         assert exc_info.value.status["reason"] == "reasoning_loop"
         assert exc_info.value.status["loop_signature"] == "stuck_tool"
 
+    def test_distinct_meta_does_not_trip_loop(self):
+        """Repeating the same action over distinct items is not a loop."""
+        breaker = CircuitBreaker(
+            session_id="test-session",
+            max_budget_usd=1000.0,
+            max_steps=1000,
+            loop_window=10,
+            loop_threshold=5,
+        )
+        for i in range(20):
+            breaker.record_step(
+                action="score_risk",
+                meta={"doc_id": "doc-1", "provision_index": i},
+            )
+        assert not breaker.is_tripped
+
+    def test_identical_meta_still_trips_loop(self):
+        breaker = CircuitBreaker(
+            session_id="test-session",
+            max_budget_usd=1000.0,
+            max_steps=1000,
+            loop_window=10,
+            loop_threshold=3,
+        )
+        with pytest.raises(ReasoningLoopError):
+            for _ in range(5):
+                breaker.record_step(
+                    action="score_risk",
+                    meta={"doc_id": "doc-1", "provision_index": 0},
+                )
+        assert breaker.is_tripped
+
     def test_status_report(self):
         breaker = CircuitBreaker(session_id="test-session", max_budget_usd=5.0)
         breaker.record_step(input_tokens=1000, output_tokens=500, action="analyze")

tests/test_tools/test_export.py

@@ -78,6 +78,31 @@ def test_export_html_creates_file(sample_analysis_results, temp_dir):
     assert "synthetic" in content.lower()
 
 
+def test_export_html_escapes_document_content(temp_dir):
+    """Document-derived text must be HTML-escaped in the exported report."""
+    results = {
+        "risk_scores": [
+            {
+                "provision_id": "prov-1",
+                "risk_level": "high",
+                "score": 7.5,
+                "factors": [],
+                "explanation": "<script>alert('xss')</script>",
+                "is_synthetic": True,
+            },
+        ],
+        "gaps": [],
+        "comparisons": [],
+        "document_count": 1,
+    }
+    report = build_risk_report(analysis_results=results)
+    path = f"{temp_dir}/test_report_escaping.html"
+    export_html(report=report, output_path=path)
+    content = Path(path).read_text()
+    assert "<script>alert" not in content
+    assert "&lt;script&gt;" in content
+
+
 def test_export_html_contains_risk_sections(sample_analysis_results, temp_dir):
     """HTML report should render all sections from the report."""
     report = build_risk_report(analysis_results=sample_analysis_results)