diff --git a/pyproject.toml b/pyproject.toml index 62a30cd..145605c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -77,4 +77,5 @@ where = ["src"] [tool.pytest.ini_options] testpaths = ["tests"] +pythonpath = ["src"] asyncio_mode = "auto" diff --git a/src/factor/app.py b/src/factor/app.py index 4d80f8e..cd31961 100644 --- a/src/factor/app.py +++ b/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: diff --git a/src/factor/harness/circuit_breaker.py b/src/factor/harness/circuit_breaker.py index ed4f7eb..412ced7 100644 --- a/src/factor/harness/circuit_breaker.py +++ b/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", diff --git a/src/factor/tools/export.py b/src/factor/tools/export.py index 43a4b94..cc3d2b5 100644 --- a/src/factor/tools/export.py +++ b/src/factor/tools/export.py @@ -228,17 +228,17 @@ def export_html(report: dict, output_path: str) -> str:
⚠️ Note: Content in this section may reference synthetic data from Taylor658/synthetic-legal. All citations are artificially generated.
- {% if section.items %} + {% if section['items'] %} - {% for key in section.items[0].keys() %} + {% for key in section['items'][0].keys() %} {% endfor %} - {% for item in section.items %} + {% for item in section['items'] %} {% for val in item.values() %} @@ -260,7 +260,7 @@ def export_html(report: dict, output_path: str) -> str: """ - env = Environment(loader=BaseLoader()) + env = Environment(loader=BaseLoader(), autoescape=True) template = env.from_string(template_str) html_content = template.render(report=report) diff --git a/tests/test_app_upload.py b/tests/test_app_upload.py new file mode 100644 index 0000000..33ebbf7 --- /dev/null +++ b/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// + assert not (tmp_path / "escape.txt").exists() + assert not (tmp_path.parent / "escape.txt").exists() + assert not (tmp_path.parent.parent / "escape.txt").exists() diff --git a/tests/test_harness.py b/tests/test_harness.py index c31ec24..f71e2bb 100644 --- a/tests/test_harness.py +++ b/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") diff --git a/tests/test_tools/test_export.py b/tests/test_tools/test_export.py index e01141a..d3a8a2f 100644 --- a/tests/test_tools/test_export.py +++ b/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": "", + "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 "
{{ key | replace('_', ' ') | title }}
{{ val }}