From b35ff7f5632f7fbec224d6556498bc210e90f2db Mon Sep 17 00:00:00 2001 From: Therealdk8890 <35633053+Therealdk8890@users.noreply.github.com> Date: Fri, 17 Jul 2026 12:22:53 -0500 Subject: [PATCH] fix: resolve 17 findings from deep code review (10 high + 7 medium) Deep review of the Python port surfaced correctness, security, and robustness defects; each was reproduced before fixing and is covered by a regression test proven to fail against the pre-fix code. Security: - visualizer render_trace_html: stored XSS via unescaped trace data in the ` in trace data cannot break out, + and the client-side inspector escapes values before assigning `innerHTML`. +- **The GitHub Action regression gate can no longer be bypassed by a crafted trace.** + `action/run_gate.py` wrote the multi-line `summary` (which embeds attacker-influenceable step + `type_identifier`s) to `$GITHUB_OUTPUT` with a fixed heredoc delimiter, so a candidate step name + containing that delimiter line plus a forged `passed=true` could close the heredoc early and + override the real verdict — silently passing a real regression. Each value now uses a random + per-write delimiter verified absent from the content. The `$GITHUB_STEP_SUMMARY` code fence is + likewise sized to exceed any backtick run it encloses, so a step name cannot inject markdown. +- **The LlamaIndex adapter redacts secrets from captured payloads.** With payload capture on + (the default) it stringified every non-structural value, including `EventPayload.SERIALIZED` — + the serialized LLM config, which has shipped an `api_key` in some llama-index versions — into a + trace store this toolkit encourages committing as a golden baseline. Secret-keyed values, nested + ones included, are now replaced with a redaction placeholder while non-secret structure (model + name, token counts) is preserved. +- **The local trace viewer validates the `Host` header on loopback binds.** The API returned + trace prompts/outputs with no `Host`/`Origin` check, so a malicious page could reach it via DNS + rebinding (rebinding its hostname to `127.0.0.1`). A loopback-bound viewer now rejects requests + whose `Host` isn't loopback; an explicit non-loopback `--host` bind leaves filtering off. + +### Fixed + +- **Nested boolean queries return correct results on the SQLite backend.** The query compiler + joined compound `AND`/`OR`/`missing_step` members with bare `INTERSECT`/`UNION`/`EXCEPT`; SQLite + gives those operators equal, left-to-right precedence, so a nested member was silently re-grouped + and diverged from the in-memory evaluator (e.g. `has(a) OR missing(b)` returned the wrong runs). + Each compiled member is now isolated in a sub-select. +- **The write buffer no longer over-sheds a run's events after a capacity burst.** The per-run depth + counter was written back from a value captured before global-capacity eviction, so evicting a row + from the enqueuing run left the counter permanently inflated, spuriously tripping the soft per-run + cap. The counter now tracks actual occupancy. +- **Early-terminating a `@traced` generator records a normal end, not a CRITICAL error.** Breaking + out of a traced generator (or async generator) raised `GeneratorExit`, which was recorded as a + spurious CRITICAL `.error`, diverging partially-consumed streams from fully-consumed ones and + tripping error-keyed anomaly rules. It now records `.end`. +- **The google-genai wrapper nests calls under the enclosing span.** It set only the current span, + leaving the parent pointing at the enclosing span's parent (the grandparent), so nested + `generate_content` calls attached to the wrong node. It now sets the parent span too. +- **The alignment engine detects reordering under every profile, keyed on sequence.** Reorder + detection was suppressed in `LINEAR` mode, so the strictest audit profile (`strict_audit_v1`) + never flagged critical-step reordering that the debug profile caught; and it compared list + position rather than the authoritative `sequence`, flagging logically identical but unsorted + runs as a spurious HIGH regression. `align()` now sorts by sequence and reorder detection is + mode-independent. +- **Trace replay surfaces span-cycle events as orphans instead of dropping them.** A parent cycle + (`A↔B`) or self-parent left its spans neither rooted nor orphaned, so their events vanished from + the reconstructed tree while the manifest still counted them. Unreachable spans are now reported + as orphaned events. +- **The trace-graph cycle validator catches cycles on partial graphs and survives deep chains.** It + seeded the search only from `graph.nodes`, missing cycles among nodes that appear solely in edges + (as `lineage()`/`impact()` can produce), and recursed per hop so a long valid causal chain raised + `RecursionError`. The search now seeds from all edge endpoints and is iterative. +- **`UnregisteredToolRule` no longer fails open on a string registry.** When the registry field was + a bare string rather than a list, `tool_name not in registry` degraded to substring matching, so + an unregistered tool whose name was a substring of the registry string was treated as allowed. A + string registry is now compared as a single exact entry (fail closed). +- **The in-memory live-subscription consumer survives a raising subscriber.** A subscriber callback + raising once killed the shared daemon consumer thread, silently stopping *all* live delivery while + `record` kept enqueuing into an unbounded queue. The consumer now logs and continues. +- **Framework adapters and the SQLite writer log previously-silent failures.** The CrewAI listener + swallowed every translation error with no trace (a version whose events lack the assumed + correlation fields produced empty traces with no clue why); `SQLiteTraceStore.flush` swallowed a + failed runs-table write that leaves events durable but unreadable. Both now log while preserving + the non-fatal behavior. +- **The trace viewer serializes payloads via `to_dict()`.** `_json_serializable` checked `__dict__` + first, which dataclasses always have, so the `to_dict()` branch was dead and the viewer showed + internal field names instead of each payload's canonical, export-consistent shape. + ## [0.6.1] - 2026-07-15 ### Security diff --git a/action/run_gate.py b/action/run_gate.py index 6a57c5c..7fa578e 100644 --- a/action/run_gate.py +++ b/action/run_gate.py @@ -18,11 +18,26 @@ import json import os +import secrets import subprocess import sys -# An unlikely delimiter for the $GITHUB_OUTPUT multiline (heredoc) format. -_DELIM = "__DPROV_OUTPUT_EOF__" + +def _fresh_delimiter(value: str) -> str: + """A random heredoc delimiter guaranteed absent from ``value``. + + The ``$GITHUB_OUTPUT`` multiline format is ``key<\\nDELIM``. A *fixed* + delimiter lets attacker-influenced content (a candidate step's ``type_identifier``, + which flows verbatim into the multi-line ``summary``) embed a line equal to the + delimiter, closing the heredoc early and injecting forged output commands such as + ``passed=true`` — silently bypassing the gate. A fresh random delimiter per write, + verified absent from the value, is GitHub's recommended defense. + """ + lines = value.splitlines() + while True: + delim = f"ghadelimiter_{secrets.token_hex(16)}" + if delim not in lines: + return delim def resolve_run(env, run_key, context_key, db, run=subprocess.run): @@ -87,12 +102,18 @@ def render_outputs(report): def write_outputs(pairs, path): - """Append ``pairs`` to ``$GITHUB_OUTPUT`` using the multiline heredoc format.""" + """Append ``pairs`` to ``$GITHUB_OUTPUT`` using the multiline heredoc format. + + Each value gets its own random delimiter that is verified absent from that value, so + no attacker-influenced content can close the heredoc early and forge later outputs. + """ if not path: return with open(path, "a", encoding="utf-8") as fh: for key, value in pairs: - fh.write(f"{key}<<{_DELIM}\n{value}\n{_DELIM}\n") + value = str(value) + delim = _fresh_delimiter(value) + fh.write(f"{key}<<{delim}\n{value}\n{delim}\n") def main(env=None): @@ -135,10 +156,27 @@ def main(env=None): step_summary = env.get("GITHUB_STEP_SUMMARY") if step_summary: with open(step_summary, "a", encoding="utf-8") as fh: - fh.write("### DProvenanceKit regression gate\n\n```\n" + summary + "\n```\n") + fh.write("### DProvenanceKit regression gate\n\n" + _fenced(summary) + "\n") return 0 +def _fenced(text: str) -> str: + """Wrap ``text`` in a Markdown code fence that it cannot break out of. + + ``summary`` embeds attacker-influenced step ``type_identifier``s; a fixed 3-backtick + fence lets a step name containing ```` ``` ```` close the block and inject arbitrary + Markdown into the job summary. Markdown allows fences longer than any backtick run + they enclose, so size the fence to one more than the longest run in ``text``. + """ + longest = 0 + run = 0 + for ch in text: + run = run + 1 if ch == "`" else 0 + longest = max(longest, run) + fence = "`" * max(3, longest + 1) + return f"{fence}\n{text}\n{fence}" + + if __name__ == "__main__": raise SystemExit(main()) diff --git a/dprovenancekit/alignment_engine.py b/dprovenancekit/alignment_engine.py index ce04be9..60fb167 100644 --- a/dprovenancekit/alignment_engine.py +++ b/dprovenancekit/alignment_engine.py @@ -55,6 +55,14 @@ def align( comp_events = [ e for e in comparison.events if e.payload.priority >= minimum_priority ] + # Reorder detection compares list position, so order by the authoritative + # ``sequence`` first. ``align()`` is public and accepts arbitrary runs (merged + # shards, OTel-ingested, hand-built) that need not arrive sequence-sorted; without + # this, two logically identical traces whose events merely arrive in a different + # list order were flagged as a spurious HIGH reordering regression. A stable sort + # is a no-op for store-backed runs, which already emerge ``ORDER BY sequence``. + base_events.sort(key=lambda e: e.sequence) + comp_events.sort(key=lambda e: e.sequence) collector = ( AlignmentEvidenceCollector() diff --git a/dprovenancekit/alignment_interpreter.py b/dprovenancekit/alignment_interpreter.py index abb71d0..22503e1 100644 --- a/dprovenancekit/alignment_interpreter.py +++ b/dprovenancekit/alignment_interpreter.py @@ -6,7 +6,6 @@ from typing import Callable, List, Optional from .alignment_contract import AlignmentExecutionContract -from .alignment_config import AlignmentMode from .alignment_evidence import EvidenceCollector, InterpretationStep from .alignment_meta import AlignmentMetaEvent from .alignment_models import ( @@ -162,10 +161,14 @@ def emit_meta(payload: AlignmentMetaEvent) -> None: used_comparison_indices.add(match_idx) else: used_comparison_indices.add(match_idx) - is_reordered = ( - config.profile.alignment_mode != AlignmentMode.LINEAR - and b_event.id in reordered_base_ids - ) + # Reorder detection is a pure matched-pair inversion check; it does not + # depend on span-aware *scoring*, so it must not be suppressed in LINEAR + # mode. Gating it on ``!= LINEAR`` (as before) made the strictest audit + # profile — strict_audit_v1, which is LINEAR — blind to critical-step + # dependency inversion (e.g. GenerateInvoice before CreateCustomer), + # the exact HIGH-risk failure the engine documents it exists to catch, + # so it detected strictly *less* than the developer_debug profile. + is_reordered = b_event.id in reordered_base_ids if is_reordered: state = AlignmentState.reordered( b_event.sequence, c_event.sequence diff --git a/dprovenancekit/instrument.py b/dprovenancekit/instrument.py index 7d7a191..3cf7e6f 100644 --- a/dprovenancekit/instrument.py +++ b/dprovenancekit/instrument.py @@ -298,6 +298,22 @@ async def agwrapper(*args, **kwargs): try: async for item in func(*args, **kwargs): yield item + except GeneratorExit: + # The consumer stopped early (``aclose()``/``break``/cancellation of + # the iterating task). That is ordinary control flow, not a failure — + # record a normal ``.end`` so a partially-consumed stream matches a + # fully-consumed one instead of emitting a spurious CRITICAL error. + end_id = _record_in_span( + f"{step_name}.end", + priority, + {"name": step_name}, + engine=step_name, + span_id=span_id, + parent_span_id=parent, + ) + if link_lifecycle: + _link(start_id, end_id, TraceEdgeType.DERIVED_FROM) + raise except BaseException as error: # noqa: BLE001 - record then re-raise err_id = _record_in_span( f"{step_name}.error", @@ -345,6 +361,22 @@ def gwrapper(*args, **kwargs): _link(_enclosing_step.get(), start_id, TraceEdgeType.INFORMED) try: result = yield from func(*args, **kwargs) + except GeneratorExit: + # The consumer stopped early (``close()``/``break``/``islice``). That + # is ordinary control flow, not a failure — record a normal ``.end`` + # so a partially-consumed stream matches a fully-consumed one instead + # of emitting a spurious CRITICAL error that trips anomaly rules. + end_id = _record_in_span( + f"{step_name}.end", + priority, + {"name": step_name}, + engine=step_name, + span_id=span_id, + parent_span_id=parent, + ) + if link_lifecycle: + _link(start_id, end_id, TraceEdgeType.DERIVED_FROM) + raise except BaseException as error: # noqa: BLE001 - record then re-raise err_id = _record_in_span( f"{step_name}.error", diff --git a/dprovenancekit/integrations/crewai.py b/dprovenancekit/integrations/crewai.py index d1c2ea5..59af5bb 100644 --- a/dprovenancekit/integrations/crewai.py +++ b/dprovenancekit/integrations/crewai.py @@ -66,6 +66,7 @@ from __future__ import annotations import json +import logging import threading import uuid from dataclasses import dataclass, field @@ -75,6 +76,8 @@ from ..event import TraceableEvent, TraceEvent from ..priority import TracePriority +logger = logging.getLogger(__name__) + # Subclass CrewAI's ``BaseEventListener`` when installed so we are a first-class listener # whose construction registers handlers on the global bus; fall back to ``object`` # otherwise so the translation logic stays importable and unit-testable without the @@ -527,7 +530,15 @@ def handle(self, kind: str, phase: str, source: Any, event: Any) -> None: with self._lock: self._handle_locked(kind, phase, source, event) except Exception: # noqa: BLE001 - instrumentation must never break the crew - pass + # Still swallowed so a translation bug can't crash the crew, but logged at + # debug: a silently-broken adapter (e.g. a CrewAI version whose events lack + # the assumed correlation fields) otherwise produces empty traces with no clue. + logger.debug( + "[DProvenanceKit] failed to record CrewAI %s.%s event", + kind, + phase, + exc_info=True, + ) def _handle_locked(self, kind: str, phase: str, source: Any, event: Any) -> None: if kind == "crew": diff --git a/dprovenancekit/integrations/google_genai.py b/dprovenancekit/integrations/google_genai.py index 9cb9a64..9c0ecd7 100644 --- a/dprovenancekit/integrations/google_genai.py +++ b/dprovenancekit/integrations/google_genai.py @@ -237,10 +237,15 @@ def __getattr__(self, name: str) -> Any: def generate_content(self, *args, **kwargs): model_name = _model_name(args, kwargs) # Start and end of one generate_content call share a single span so the - # pair reads as one node in the span tree. `record()` reads the span from - # the contextvar (there is no span_id kwarg), so set it for the call. + # pair reads as one node in the span tree. `record()` reads both the span and + # its parent from contextvars (there is no span_id kwarg), so set both for the + # call: the previously-current span becomes this call's parent. Setting only + # current_span_id (as before) left parent_span_id pointing at the *enclosing* + # span's parent — the grandparent — so a nested call attached to the wrong node. call_span = str(uuid.uuid4()) + parent_span = TraceContext.current_span_id.get() span_token = TraceContext.current_span_id.set(call_span) + parent_token = TraceContext.parent_span_id.set(parent_span) try: start_event = GoogleGenAITraceEvent.make( type_name="generateContentStarted", @@ -273,6 +278,7 @@ def generate_content(self, *args, **kwargs): return response finally: + TraceContext.parent_span_id.reset(parent_token) TraceContext.current_span_id.reset(span_token) diff --git a/dprovenancekit/integrations/llama_index.py b/dprovenancekit/integrations/llama_index.py index b22edd2..c2dc6c5 100644 --- a/dprovenancekit/integrations/llama_index.py +++ b/dprovenancekit/integrations/llama_index.py @@ -139,6 +139,49 @@ def _key(key: Any) -> str: # Bulky payload lists reduced to counts instead of being captured or dropped. _COUNTED_KEYS = {"nodes": "node_count", "chunks": "chunk_count"} +_REDACTED = "***redacted***" + +# Key-name fragments that mark a value as a secret. Deliberately precise — bare "token" +# is excluded so token-count telemetry (prompt_tokens/total_tokens) is still captured. +_SENSITIVE_KEY_FRAGMENTS = ( + "api_key", + "apikey", + "secret", + "password", + "passwd", + "authorization", + "credential", + "bearer", + "private_key", + "access_token", + "refresh_token", + "auth_token", + "session_token", +) + + +def _is_sensitive_key(key: Any) -> bool: + normalized = str(key).lower().replace("-", "_") + return any(fragment in normalized for fragment in _SENSITIVE_KEY_FRAGMENTS) + + +def _redact_secrets(value: Any) -> Any: + """Recursively replace secret-keyed values with a placeholder. + + LlamaIndex's ``EventPayload.SERIALIZED`` is a nested LLM/embedding config dict that has + shipped an ``api_key`` in some versions; stringifying it verbatim leaked the key into + the trace store (which this toolkit encourages committing as a golden baseline). This + scrubs sensitive sub-keys while preserving useful structure like the model name. + """ + if isinstance(value, Mapping): + return { + _key(k): (_REDACTED if _is_sensitive_key(k) else _redact_secrets(v)) + for k, v in value.items() + } + if isinstance(value, (list, tuple)): + return [_redact_secrets(v) for v in value] + return value + def _payload_attributes( payload: Optional[Mapping[Any, Any]], capture: bool @@ -147,7 +190,8 @@ def _payload_attributes( Node/chunk lists become counts (always — they are structural metadata); every other value is captured only when ``capture`` is on, stringified and truncated so no - single attribute can exceed the truncation limit. The exception payload is handled + single attribute can exceed the truncation limit, with secret-keyed values (including + those nested inside the serialized config) redacted. The exception payload is handled by the error path, never here. """ attrs: Dict[str, Any] = {} @@ -161,7 +205,10 @@ def _payload_attributes( if counted is not None and isinstance(v, (list, tuple)): attrs[counted] = len(v) elif capture: - attrs[key] = _truncate(str(v)) + if _is_sensitive_key(key): + attrs[key] = _REDACTED + else: + attrs[key] = _truncate(str(_redact_secrets(v))) return attrs diff --git a/dprovenancekit/query.py b/dprovenancekit/query.py index dcfa657..bdf997b 100644 --- a/dprovenancekit/query.py +++ b/dprovenancekit/query.py @@ -321,6 +321,19 @@ class CompiledSQLQuery: bindings: List[str] +def _isolate(sql: str) -> str: + """Wrap a compiled member as ``SELECT run_id FROM ()``. + + SQLite gives ``UNION``/``INTERSECT``/``EXCEPT`` equal precedence and evaluates + them strictly left-to-right, so a compound member spliced directly into a parent + compound is silently re-grouped: ``A UNION (B INTERSECT C)`` flattens to + ``(A UNION B) INTERSECT C``. Every leaf/compound this compiler emits selects a + ``run_id`` column, so wrapping each member in a sub-select restores the AST's + grouping and keeps the SQLite backend in parity with the in-memory evaluator. + """ + return f"SELECT run_id FROM (\n{sql}\n)" + + class TraceQueryCompiler: @staticmethod def compile(node: TraceQueryNode) -> CompiledSQLQuery: @@ -332,7 +345,7 @@ def _compile_node(node: TraceQueryNode) -> CompiledSQLQuery: if not node.nodes: return CompiledSQLQuery("SELECT run_id FROM runs", []) compiled = [TraceQueryCompiler._compile_node(n) for n in node.nodes] - sql = "\nINTERSECT\n".join(c.sql for c in compiled) + sql = "\nINTERSECT\n".join(_isolate(c.sql) for c in compiled) bindings: List[str] = [b for c in compiled for b in c.bindings] return CompiledSQLQuery(sql, bindings) @@ -340,14 +353,15 @@ def _compile_node(node: TraceQueryNode) -> CompiledSQLQuery: if not node.nodes: return CompiledSQLQuery("SELECT run_id FROM runs", []) compiled = [TraceQueryCompiler._compile_node(n) for n in node.nodes] - sql = "\nUNION\n".join(c.sql for c in compiled) + sql = "\nUNION\n".join(_isolate(c.sql) for c in compiled) bindings = [b for c in compiled for b in c.bindings] return CompiledSQLQuery(sql, bindings) if isinstance(node, NotNode): inner = TraceQueryCompiler._compile_node(node.node) return CompiledSQLQuery( - f"SELECT run_id FROM runs EXCEPT\n{inner.sql}", inner.bindings + f"SELECT run_id FROM runs EXCEPT\n{_isolate(inner.sql)}", + inner.bindings, ) if isinstance(node, ContextIDEquals): diff --git a/dprovenancekit/replay.py b/dprovenancekit/replay.py index f70a8d5..b230c57 100644 --- a/dprovenancekit/replay.py +++ b/dprovenancekit/replay.py @@ -179,18 +179,28 @@ def snapshot(self, at: Optional[int] = None) -> ReplaySnapshot: else: roots.append(node) - roots.extend(root_builders) + # Pass 3: any span not reachable from a genuine root (parent None) is orphaned. + # This covers both the classic case — an ancestor chain ending at a *missing* + # parent — and parent *cycles* (A→B→A, or a self-parent S→S), which the old + # "parent missing" test skipped entirely, leaving those spans in neither the tree + # nor the orphan list so their events silently disappeared while the manifest + # still counted them. The ``reachable`` visited-guard also stops a self-referential + # child list from looping forever. + reachable = set() + stack = list(roots) + while stack: + n = stack.pop() + if id(n) in reachable: + continue + reachable.add(id(n)) + stack.extend(n.children) - # Pass 3: collect orphaned events (subtrees whose parent span is entirely missing). orphaned_events: List[ReplayEvent] = [] for node in span_map.values(): - pid = node.parent_span_id - if pid is not None and pid not in span_map: - stack = [node] - while stack: - n = stack.pop() - orphaned_events.extend(n.events) - stack.extend(n.children) + if id(node) not in reachable: + orphaned_events.extend(node.events) + + roots.extend(root_builders) true_roots = [b.build() for b in roots] true_roots.sort(key=lambda n: n.start_sequence) diff --git a/dprovenancekit/rules.py b/dprovenancekit/rules.py index d431f0a..81c4c80 100644 --- a/dprovenancekit/rules.py +++ b/dprovenancekit/rules.py @@ -247,6 +247,13 @@ def is_anomalous(self, run: TraceRun) -> bool: continue tool_name = payload.get("tool_name") or payload.get("name") registry = payload.get(self._registry_field) or [] + if isinstance(registry, str): + # A registry serialized as a bare string must not let ``not in`` degrade + # into substring matching: an unregistered tool whose name is a substring + # of the string (e.g. "arc" in "search,calc") would slip through this + # allow-list check. Treat the string as a single opaque entry so the + # comparison stays an exact match — fail closed, not open. + registry = [registry] if tool_name and tool_name not in registry: return True return False diff --git a/dprovenancekit/sqlite_store.py b/dprovenancekit/sqlite_store.py index be04b35..d5ceafd 100644 --- a/dprovenancekit/sqlite_store.py +++ b/dprovenancekit/sqlite_store.py @@ -153,7 +153,12 @@ def flush(self) -> None: staged = self._flush_runs_table(force=True) self._mark_runs_clean(staged) except Exception: # pragma: no cover - defensive - pass + # A failed runs-table write leaves events durable in trace_events but + # unreadable (get_run/get_events JOIN runs). Don't fail flush over it, but + # log it — a silent pass made the metadata loss impossible to diagnose. + logger.exception( + "[DProvenanceKit] SQLiteWriter failed to flush runs-table metadata" + ) def shutdown(self) -> None: self._shutting_down.set() diff --git a/dprovenancekit/store.py b/dprovenancekit/store.py index c460159..f89c2c6 100644 --- a/dprovenancekit/store.py +++ b/dprovenancekit/store.py @@ -2,6 +2,7 @@ from __future__ import annotations +import logging import queue import threading import uuid @@ -14,6 +15,8 @@ from .graph import TraceExplanation, TraceGraph from .query import TraceQueryDSL, TraceQueryPlanner, TraceRun +logger = logging.getLogger(__name__) + class TraceError(Exception): pass @@ -169,7 +172,16 @@ def _drain_live(self) -> None: if item is None: # sentinel return event, run = item - self._live_engine.process(event=event, run=run) + try: + self._live_engine.process(event=event, run=run) + except Exception: + # A subscriber callback raising must not kill this shared daemon consumer + # and silently stop *all* live delivery (while `record` keeps enqueuing + # into an unbounded queue). Log and continue with the next event. + logger.exception( + "[DProvenanceKit] live subscription handler raised; " + "continuing delivery" + ) def close(self) -> None: if self._live_queue is not None and self._live_thread is not None: diff --git a/dprovenancekit/ui_server.py b/dprovenancekit/ui_server.py index b2270a3..492cf38 100644 --- a/dprovenancekit/ui_server.py +++ b/dprovenancekit/ui_server.py @@ -16,16 +16,40 @@ class ThreadingHTTPServer(ThreadingMixIn, HTTPServer): def _json_serializable(obj): + # Prefer the payload's own to_dict() (its canonical, export-consistent shape) over the + # raw __dict__ of internal fields. Dataclasses always have __dict__, so checking it + # first left to_dict() unreachable and served internal attribute names to the viewer. + if hasattr(obj, "to_dict"): + try: + return obj.to_dict() + except Exception: + pass if hasattr(obj, "__dict__"): return obj.__dict__ - if hasattr(obj, "to_dict"): - return obj.to_dict() return str(obj) -def create_handler(db_path: str): +def _host_only(host_header: str) -> str: + """The hostname from a ``Host`` header, without the optional ``:port`` (IPv6-aware).""" + host_header = host_header.strip() + if host_header.startswith("["): # bracketed IPv6 literal, e.g. [::1]:8080 + return host_header[1 : host_header.find("]")] if "]" in host_header else host_header + return host_header.rsplit(":", 1)[0] if ":" in host_header else host_header + + +def create_handler(db_path: str, allowed_hosts=None): class UIHandler(BaseHTTPRequestHandler): def do_GET(self): + # Reject requests whose Host header isn't in the allow-list. For a loopback + # bind this defeats DNS rebinding: a malicious page that rebinds its hostname + # to 127.0.0.1 sends its own Host, which we refuse — without this, that page + # could read unauthenticated trace prompts/outputs from the local viewer. + if allowed_hosts is not None: + host = _host_only(self.headers.get("Host", "")) + if host not in allowed_hosts: + self.send_error(403, "Forbidden") + return + parsed = urlparse(self.path) path = parsed.path @@ -221,11 +245,19 @@ def _server_error(self): return UIHandler +_LOOPBACK_HOSTS = frozenset({"127.0.0.1", "localhost", "::1"}) + + def create_server(db_path: str, port: int = 8080, host: str = "127.0.0.1"): """Build the UI server bound to ``host`` (loopback by default: trace databases hold prompts and outputs, so exposing them beyond the machine must be a - deliberate choice).""" - return ThreadingHTTPServer((host, port), create_handler(db_path)) + deliberate choice). + + When bound to loopback, the handler enforces a loopback-only ``Host`` allow-list to + block DNS rebinding. A non-loopback bind (via ``--host``) is an explicit choice to + expose the viewer, so Host filtering is left off there.""" + allowed_hosts = _LOOPBACK_HOSTS if host in _LOOPBACK_HOSTS else None + return ThreadingHTTPServer((host, port), create_handler(db_path, allowed_hosts)) def run_ui_server(db_path: str, port: int = 8080, host: str = "127.0.0.1"): diff --git a/dprovenancekit/verification.py b/dprovenancekit/verification.py index bd114ae..ef120c7 100644 --- a/dprovenancekit/verification.py +++ b/dprovenancekit/verification.py @@ -212,26 +212,43 @@ def validate_structural_integrity(self, graph: TraceGraph) -> None: for edge in causal: adjacency.setdefault(edge.source_id, []).append(edge.target_id) - visited = set() - rec_stack = set() - path: List[uuid.UUID] = [] - - def has_cycle(node: uuid.UUID) -> None: - visited.add(node) - rec_stack.add(node) - path.append(node) - for neighbor in adjacency.get(node, []): - if neighbor not in visited: - has_cycle(neighbor) - elif neighbor in rec_stack: - path.append(neighbor) - raise StructuralCycleDetected(list(path)) - rec_stack.discard(node) - path.pop() - - for node in graph.nodes.keys(): - if node not in visited: - has_cycle(node) + # Seed the search from every node that participates in a causal edge (as source + # *or* target), not only ``graph.nodes``: ``store.lineage()``/``impact()`` return + # partial graphs whose ``nodes`` dict need not cover every edge endpoint, and a + # cycle among edge-only nodes must still be detected. Sorted for deterministic + # cycle reporting. + seeds = set(adjacency.keys()) + for targets in adjacency.values(): + seeds.update(targets) + seeds.update(graph.nodes.keys()) + + # Iterative DFS (an explicit stack, not recursion) so a long-but-valid causal + # chain cannot overflow Python's recursion limit and raise RecursionError from a + # validator whose whole job is to *not* reject well-formed traces. + visited: set = set() # fully explored + for start in sorted(seeds, key=str): + if start in visited: + continue + stack = [(start, iter(adjacency.get(start, [])))] + on_path = {start} + path: List[uuid.UUID] = [start] + while stack: + node, neighbors = stack[-1] + advanced = False + for neighbor in neighbors: + if neighbor in on_path: + raise StructuralCycleDetected(path + [neighbor]) + if neighbor not in visited: + stack.append((neighbor, iter(adjacency.get(neighbor, [])))) + on_path.add(neighbor) + path.append(neighbor) + advanced = True + break + if not advanced: + stack.pop() + on_path.discard(node) + path.pop() + visited.add(node) class TraceGraphProvenanceValidator: diff --git a/dprovenancekit/visualizer.py b/dprovenancekit/visualizer.py index 0ba51eb..c418f52 100644 --- a/dprovenancekit/visualizer.py +++ b/dprovenancekit/visualizer.py @@ -6,11 +6,31 @@ from __future__ import annotations +import html import json from .graph import TraceGraph +def _script_safe_json(payload: object) -> str: + """Serialize ``payload`` to JSON that is safe to inline inside a ```` would otherwise terminate the script element and inject markup. Escaping + those characters (plus the U+2028/U+2029 line terminators, which are newlines in JS + string literals) keeps attacker-influenced trace content inert. The result is still + valid JSON — the escapes are the standard ``\\uXXXX`` forms. + """ + return ( + json.dumps(payload) + .replace("<", "\\u003c") + .replace(">", "\\u003e") + .replace("&", "\\u0026") + .replace("
", "\\u2028") + .replace("
", "\\u2029") + ) + + _CSS = """ :root { --bg-dark: #0f111a; @@ -243,6 +263,17 @@ }); } +function escapeHtml(value) { + // Trace data (type ids, engine names) is attacker-influenced and is assigned via + // innerHTML below, so escape it before interpolation. + return String(value) + .replace(/&/g, '&') + .replace(//g, '>') + .replace(/"/g, '"') + .replace(/'/g, '''); +} + function selectNode(nodeId) { document.querySelectorAll('.timeline-item').forEach(el => el.classList.remove('active')); const el = document.getElementById('node-' + nodeId); @@ -271,7 +302,7 @@ derivedFrom.forEach(sourceId => { const srcNode = window.graphData.nodes[sourceId]; edgesHtml += `
  • - ${srcNode.type_identifier} (${srcNode.engine_name}) + ${escapeHtml(srcNode.type_identifier)} (${escapeHtml(srcNode.engine_name)}) DERIVED_FROM
    • `; }); @@ -283,7 +314,7 @@ informedBy.forEach(sourceId => { const srcNode = window.graphData.nodes[sourceId]; edgesHtml += `
  • - ${srcNode.type_identifier} (${srcNode.engine_name}) + ${escapeHtml(srcNode.type_identifier)} (${escapeHtml(srcNode.engine_name)}) INFORMED_BY
    • `; }); @@ -299,9 +330,9 @@ inspector.innerHTML = `
    - ${node.type_identifier} - ${node.engine_name} - Seq: ${node.sequence} + ${escapeHtml(node.type_identifier)} + ${escapeHtml(node.engine_name)} + Seq: ${escapeHtml(node.sequence)}
    ${edgesHtml}
    Payload
    @@ -346,7 +377,11 @@ def render_trace_html(graph: TraceGraph, title: str = "Visual Debugger") -> str: "type": e.type.name }) - graph_data_json = json.dumps({"nodes": js_nodes, "edges": js_edges}) + # Attacker-influenced trace data (payloads, engine names, type ids) must be escaped + # before it reaches the browser: for the script block via _script_safe_json, and for + # the server-rendered timeline via html.escape. The client-side inspector escapes the + # same fields again with escapeHtml() before assigning innerHTML. + graph_data_json = _script_safe_json({"nodes": js_nodes, "edges": js_edges}) # Generate timeline HTML timeline_html = [] @@ -354,25 +389,26 @@ def render_trace_html(graph: TraceGraph, title: str = "Visual Debugger") -> str: type_id = n.payload.type_identifier if hasattr(n.payload, "type_identifier") else str(n.payload) timeline_html.append(f'''
    -
    {n.engine_name}
    -
    {type_id}
    +
    {html.escape(str(n.engine_name))}
    +
    {html.escape(str(type_id))}
    seq: {n.sequence}
    ''') timeline_str = "".join(timeline_html) + title_safe = html.escape(str(title)) return f""" - {title} + {title_safe}
    -

    {title}

    +

    {title_safe}

    DProvenanceKit
    diff --git a/dprovenancekit/write_buffer.py b/dprovenancekit/write_buffer.py index 98782d5..2ee9684 100644 --- a/dprovenancekit/write_buffer.py +++ b/dprovenancekit/write_buffer.py @@ -165,7 +165,14 @@ def over_capacity() -> bool: ) self._total_count += 1 self._total_bytes += event_bytes - self._queue_depth_by_run[event.run_id] = run_depth + 1 + # Re-read the per-run depth rather than reusing the ``run_depth`` captured + # before eviction: global-capacity eviction above may have popped a victim + # belonging to *this* run and decremented the counter, so ``run_depth + 1`` + # would write back a stale, permanently-inflated value that spuriously trips + # the soft per-run cap once the run's real occupancy is far below it. + self._queue_depth_by_run[event.run_id] = ( + self._queue_depth_by_run.get(event.run_id, 0) + 1 + ) def enqueue_edge(self, edge: TraceEdge) -> None: with self._lock: diff --git a/tests/integrations/test_llama_index.py b/tests/integrations/test_llama_index.py index 1d5b420..1e50f5e 100644 --- a/tests/integrations/test_llama_index.py +++ b/tests/integrations/test_llama_index.py @@ -246,6 +246,37 @@ def drive(handler): assert llm_end.payload.attributes["response"] == "R" * 2000 + "…" +def test_serialized_config_secrets_are_redacted(): + """LlamaIndex's serialized LLM config has shipped an api_key in some versions. With + capture on, secret-keyed values (including those nested in the serialized dict) must + be redacted so the key never lands in a trace store shared as a golden baseline — + while non-secret structure like the model name is preserved.""" + def drive(handler): + handler.on_event_start( + LLM, + payload={ + "serialized": {"model": "gpt-4o", "api_key": "sk-SECRET123"}, + "api_key": "sk-TOPLEVEL", + "total_tokens": 42, + }, + event_id="l", + parent_id="root", + ) + handler.on_event_end(LLM, payload={}, event_id="l") + + store, run = _run_handler(drive) + llm_start, _ = _recorded(store, run) + attrs = llm_start.payload.attributes + # No secret material anywhere in the recorded attributes. + assert "sk-SECRET123" not in repr(attrs) + assert "sk-TOPLEVEL" not in repr(attrs) + assert attrs["api_key"] == "***redacted***" + # Useful structure survives: model name kept, token counts not treated as secrets. + assert "gpt-4o" in attrs["serialized"] + assert "***redacted***" in attrs["serialized"] + assert attrs["total_tokens"] == "42" + + def test_node_lists_become_counts(): def drive(handler): handler.on_event_start(RETRIEVE, event_id="r", parent_id="root") diff --git a/tests/test_action_scripts.py b/tests/test_action_scripts.py index ca6112d..ea8c309 100644 --- a/tests/test_action_scripts.py +++ b/tests/test_action_scripts.py @@ -128,6 +128,64 @@ def test_run_gate_publishes_regression_without_failing_wrapper(trace_db, tmp_pat assert parsed["regression-level"] == "high" +def _github_parse(text): + """Parse ``$GITHUB_OUTPUT`` the way the runner does: both the ``key=value`` short form + and the ``key< run wrongly + # excluded even though has(errorDetected) already satisfies the OR. + ( + only("errorDetected", "stepCompleted"), + TraceQueryDSL() + .requiring_step("errorDetected") + .or_(TraceQueryDSL().missing_step("stepCompleted")), + ["case"], + ), + # has(errorDetected) OR ((stepCompleted OR processStarted) AND processFinished); + # the run has ONLY errorDetected. Flat left-to-right groups as + # (((ED UNION SC) UNION PS) INTERSECT PF) -> wrongly excluded. + ( + only("errorDetected"), + TraceQueryDSL() + .requiring_step("errorDetected") + .or_( + TraceQueryDSL() + .requiring_step("stepCompleted") + .or_(TraceQueryDSL().requiring_step("processStarted")) + .requiring_step("processFinished") + ), + ["case"], + ), + ] + + for i, (scenario, query, expected) in enumerate(cases): + db_path = str(tmp_path / f"nested-{i}.sqlite") + mem, sql = _matches(scenario, query, db_path) + assert mem == sql == expected, f"case {i}: mem={mem} sql={sql} expected={expected}" diff --git a/tests/test_regression_gate.py b/tests/test_regression_gate.py index fd8db6c..5a1b4ad 100644 --- a/tests/test_regression_gate.py +++ b/tests/test_regression_gate.py @@ -233,10 +233,10 @@ def test_custom_minimum_priority_is_honored(): assert lifted.passed -# ── Reorder detection depends on the profile (documented limitation) ───────────── +# ── Reorder detection fires regardless of profile ──────────────────────────────── -def test_reordering_only_caught_with_a_span_aware_profile(): +def test_reordering_is_caught_regardless_of_profile(): store = InMemoryTraceStore() golden = build_run(store, "golden") # retrieved, verified, decided @@ -251,9 +251,13 @@ def test_reordering_only_caught_with_a_span_aware_profile(): kit.record(FCEvent("verified", "2 of 3 agree")) reordered = store.get_run(run.run_id) - # The default linear profile does NOT catch a pure reorder (it binds 1:1). - assert RegressionGate().check(golden, reordered).passed - # A span-aware profile does. + # Reorder detection is a pure matched-pair inversion check, not a span-aware scoring + # feature, so the default (strict_audit_v1 / LINEAR) profile catches a pure reorder + # just as a span-aware profile does — the strictest audit profile must not detect + # *less* than the debug one. + strict = RegressionGate().check(golden, reordered) + assert not strict.passed + assert strict.regression_level == RegressionLevel.HIGH # critical steps reordered span_aware = RegressionGate(profile=AlignmentProfile.developer_debug_v1) assert not span_aware.check(golden, reordered).passed diff --git a/tests/test_replay_engine.py b/tests/test_replay_engine.py index 4b77d70..34834e3 100644 --- a/tests/test_replay_engine.py +++ b/tests/test_replay_engine.py @@ -113,3 +113,45 @@ def test_sequence_gaps(): assert (gaps[0].lower_bound, gaps[0].upper_bound) == (0, 0) assert (gaps[1].lower_bound, gaps[1].upper_bound) == (3, 4) assert (gaps[2].lower_bound, gaps[2].upper_bound) == (7, 9) + + +def test_span_parent_cycle_events_are_orphaned_not_dropped(): + """A parent cycle (A<->B) or a self-parent leaves its spans neither rooted nor + reachable. The old orphan test only fired on a *missing* parent, so cycle members + silently vanished from the tree while the manifest still counted them. They must now + surface as orphaned events so nothing is lost without accounting.""" + run_id = uuid.uuid4() + events = [ + _event(run_id, 0, "A", "B", MockEvent("a")), # A's parent is B + _event(run_id, 1, "B", "A", MockEvent("b")), # B's parent is A -> cycle + _event(run_id, 2, "C", None, MockEvent("c")), # healthy root span + ] + snap = TraceReplayEngine(events).snapshot() + + def tree_event_count(roots): + total, stack, seen = 0, list(roots), set() + while stack: + node = stack.pop() + if id(node) in seen: + continue + seen.add(id(node)) + total += len(node.events) + stack.extend(node.children) + return total + + in_tree = tree_event_count(snap.roots) + assert snap.manifest.total_events == 3 + assert snap.manifest.orphaned_events == 2 # A and B + # Every event is accounted for: in the tree or explicitly orphaned, none dropped. + assert in_tree + snap.manifest.orphaned_events == snap.manifest.total_events + + +def test_self_parent_span_is_orphaned_not_dropped(): + run_id = uuid.uuid4() + events = [ + _event(run_id, 0, "S", "S", MockEvent("s")), # self-parent + _event(run_id, 1, "C", None, MockEvent("c")), + ] + snap = TraceReplayEngine(events).snapshot() + assert snap.manifest.total_events == 2 + assert snap.manifest.orphaned_events == 1 diff --git a/tests/test_rules.py b/tests/test_rules.py index 89269e3..90f83b4 100644 --- a/tests/test_rules.py +++ b/tests/test_rules.py @@ -320,6 +320,24 @@ def test_unregistered_tool_rule_silent_when_all_calls_registered(): assert AnomalyDetector(store).detect_anomalies([rule]) == [] +def test_unregistered_tool_rule_string_registry_is_not_substring_matched(): + """A registry serialized as a bare string must not degrade membership into substring + matching: 'arc' is a substring of 'search,calc' but is not a registered tool, so the + rogue call must still be flagged — the rule fails closed, not open.""" + from dprovenancekit.rules import UnregisteredToolRule + + store = InMemoryTraceStore() + kit = DProvenanceKit(ToolCallStep) + with kit.run(context_id="rogue", store=store) as run: + # registered_tools as a STRING whose text contains the tool name as a substring. + kit.record(ToolCallStep(name="arc", registered_tools="search,calc")) + rogue = run.run_id + + rule = UnregisteredToolRule("tool_call", "registered_tools") + flagged = {a.run_id for a in AnomalyDetector(store).detect_anomalies([rule])} + assert rogue in flagged + + def test_unregistered_tool_rule_validates_args(): from dprovenancekit.rules import UnregisteredToolRule diff --git a/tests/test_trace_graph.py b/tests/test_trace_graph.py index 32813d4..061c3d9 100644 --- a/tests/test_trace_graph.py +++ b/tests/test_trace_graph.py @@ -79,6 +79,36 @@ def test_structural_validator_cycle_throws(): TraceGraphValidator().validate_structural_integrity(graph) +def test_structural_validator_detects_cycle_among_edge_only_nodes(): + """lineage()/impact() return partial graphs whose ``nodes`` dict need not cover every + edge endpoint. Seeding the search only from ``graph.nodes`` missed cycles among nodes + that appear solely in edges — a validator that silently passes a real cycle.""" + a, b = uuid.uuid4(), uuid.uuid4() + graph = TraceGraph( + nodes={}, # neither endpoint present as a node + edges=[ + TraceEdge(a, b, TraceEdgeType.DERIVED_FROM), + TraceEdge(b, a, TraceEdgeType.DERIVED_FROM), + ], + ) + with pytest.raises(StructuralCycleDetected): + TraceGraphValidator().validate_structural_integrity(graph) + + +def test_structural_validator_handles_deep_acyclic_chain_without_overflow(): + """A long-but-valid causal chain must validate, not raise RecursionError: the search + is iterative, so depth is bounded by heap, not Python's recursion limit.""" + ids = [uuid.uuid4() for _ in range(5000)] + graph = TraceGraph( + nodes={i: _node(TestEvent.process_started(), i) for i in ids}, + edges=[ + TraceEdge(ids[k], ids[k + 1], TraceEdgeType.DERIVED_FROM) + for k in range(len(ids) - 1) + ], + ) + TraceGraphValidator().validate_structural_integrity(graph) # no raise + + def test_provenance_validator_flags_orphan_section_and_unused_fact(): fact, section = uuid.uuid4(), uuid.uuid4() graph = TraceGraph( diff --git a/tests/test_ui_server_security.py b/tests/test_ui_server_security.py index 2a7ad76..5471db7 100644 --- a/tests/test_ui_server_security.py +++ b/tests/test_ui_server_security.py @@ -66,3 +66,48 @@ def test_run_picker_uses_keyboard_accessible_buttons(): assert "button.type = 'button'" in html assert "aria-pressed" in html assert ".run-item:focus-visible" in html + + +def test_host_header_allowlist_blocks_dns_rebinding(): + """A loopback-bound viewer must reject requests whose Host header isn't loopback, so a + malicious page that rebinds its hostname to 127.0.0.1 cannot read trace data. Requests + with a loopback Host still succeed.""" + import http.client + import threading + + server = create_server(db_path="unused.sqlite", port=0) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + try: + port = server.server_address[1] + + def get_status(host_header): + conn = http.client.HTTPConnection("127.0.0.1", port, timeout=5) + try: + conn.putrequest("GET", "/", skip_host=True, skip_accept_encoding=True) + conn.putheader("Host", host_header) + conn.endheaders() + return conn.getresponse().status + finally: + conn.close() + + # Rebinding attack: attacker-controlled Host pointing at the loopback server. + assert get_status("evil.example.com") == 403 + # Legitimate local access serves the viewer. + assert get_status(f"localhost:{port}") == 200 + assert get_status(f"127.0.0.1:{port}") == 200 + finally: + server.shutdown() + server.server_close() + thread.join(timeout=5) + + +def test_non_loopback_bind_does_not_enforce_host_allowlist(): + """Binding to a non-loopback host is an explicit choice to expose the viewer, so Host + filtering is left off (there is no practical allow-list for an 0.0.0.0 bind).""" + from dprovenancekit.ui_server import create_handler, _LOOPBACK_HOSTS + + # Loopback bind gets an allow-list; a wildcard bind gets None (no filtering). + assert "127.0.0.1" in _LOOPBACK_HOSTS + handler = create_handler("unused.sqlite", allowed_hosts=None) + assert handler is not None # constructs without a Host allow-list diff --git a/tests/test_visualizer.py b/tests/test_visualizer.py index 79232b8..5bb1458 100644 --- a/tests/test_visualizer.py +++ b/tests/test_visualizer.py @@ -40,3 +40,54 @@ def test_visualizer_renders_html(): # Check that javascript block is present assert "window.graphData =" in html assert "function selectNode" in html + + +def test_visualizer_escapes_malicious_trace_data(): + """Trace payloads/engine names are attacker-influenced (LLM/tool output). They must be + neutralized both in the server-rendered HTML and inside the inlined `` in the data cannot break out and execute — the stored-XSS class the 0.6.1 + security release fixed in index.html/report.py but originally missed here.""" + from dataclasses import dataclass + from dprovenancekit import TraceableEvent, TracePriority + + breakout = "" + + @dataclass(frozen=True) + class Evil(TraceableEvent): + @property + def type_identifier(self) -> str: + return breakout + + @property + def priority(self) -> TracePriority: + return TracePriority.STRUCTURAL + + def to_dict(self) -> dict: + return {"data": breakout} + + from dprovenancekit import TraceEvent + + nid = uuid.uuid4() + # Inject via engine_name too (also interpolated into the timeline HTML). + node = TraceEvent( + id=nid, + run_id=uuid.uuid4(), + context_id="test", + engine_name=breakout, + schema_version=1, + sequence=1, + span_id=None, + parent_span_id=None, + payload=Evil(), + ) + graph = TraceGraph(nodes={nid: node}, edges=[]) + + html = render_trace_html(graph, title=breakout) + + # The raw breakout string must never appear verbatim, and there must be exactly one + # real closing tag (the document's own), not one smuggled in via the data. + assert breakout not in html + assert html.count("") == 1 + # Data reaches the page in escaped form: < in the script JSON, < in the HTML. + assert "\\u003c" in html + assert "</script>" in html diff --git a/tests/test_write_buffer.py b/tests/test_write_buffer.py index be523b5..bc60e96 100644 --- a/tests/test_write_buffer.py +++ b/tests/test_write_buffer.py @@ -22,6 +22,25 @@ def _make_row(run_id, seq, priority): ) +def test_per_run_depth_counter_does_not_drift_after_global_eviction(): + """The per-run depth counter must track actual occupancy. When global-capacity eviction + pops a victim from the enqueuing run, writing back a pre-eviction ``run_depth + 1`` + left the counter permanently inflated, spuriously tripping the soft per-run cap and + shedding a run's events while its real occupancy was far below the limit.""" + buffer = TraceWriteBuffer(max_global_buffer=10, max_per_run_buffer=40) + for i in range(60): # far past the global cap, one run + buffer.enqueue(_make_row("r", i, TracePriority.TELEMETRY)) + + # Only 10 rows can be buffered; the counter must agree, not read 40. + assert buffer.current_depth == 10 + assert buffer._queue_depth_by_run.get("r") == 10 + + buffer.flush_all() + # Emptied buffer -> no residual phantom count for the run. + assert buffer.current_depth == 0 + assert not buffer._queue_depth_by_run.get("r") + + def test_drain_preserves_global_insertion_order(): buffer = TraceWriteBuffer(max_global_buffer=10_000, max_per_run_buffer=10_000) priorities = [