Skip to content

Commit c7674ca

Browse files
authored
Fix/p0 security stability bundle (#23)
* fix: P0 security/stability hardening bundle Closes the P0/P1/P2/P3 issues from the security review (plan §10/§11.4). Security / PCI-DSS / GDPR - P0-1: Mask positional PII in `_enforce_sensitive_tool` by introspecting the wrapped function's signature and applying `SENSITIVE_ARG_KEYS` to positional params. Pre-fix, `charge("4111-…-1111", 50)` forwarded the PAN into `/execute` and the audit log. - P0-6 / P3-3: `_safe_repr` now redacts BEFORE truncating. The pre-fix order truncated first, so `details={…}` past position 50 leaked verbatim. `_safe_repr` is now the single source of truth for the redact-then-truncate flow. Cost-audit / reliability - P0-3: Bounded chunked reads on the sync + async httpx transports (`MAX_RESPONSE_BYTES`, default 16 MiB, `NULLRUN_MAX_RESPONSE_BYTES` env override). Above the cap, tracking is skipped and `_coverage_streaming_skipped` is incremented. Replaces the `response.read()` / `await response.aread()` unbounded buffer that held entire LLM streaming bodies in memory. - P0-4: `_do_flush_locked` re-queue on CB OPEN now drops the NEWEST non-critical events instead of the oldest. The oldest events (incident start, billing-period start) are exactly what a billing investigator needs; losing them silently broke monthly rollups. Control-plane events (`state_change`, `kill_received`, `policy_invalidated`, `key_rotated`) are preserved unconditionally so the dashboard KILL switch lands even under sustained backend outage. Identity - S-8 / P2-4: `agent()` now emits `str(uuid.uuid4())` (with dashes). Pre-fix the format was `f"agent-{uuid.uuid4().hex}"` — 32 hex chars, no dashes — and backend UUID-typed columns dropped these to NULL on insert. User-supplied names are still preserved verbatim. - §7.2 #16: `workflow()` context manager now resets `span_id` (not only `workflow_id` / `trace_id`) so nested `with span()` blocks don't leave the inner span_id visible inside the workflow scope. Resource leaks - S-9: `_active_runs` on `NullRunCallback` is now an `OrderedDict` capped at 4096 with FIFO eviction. Pre-fix the dict grew unbounded when `on_chain_end` did not fire (some LangChain versions short-circuit the end hook on chain-body errors). - S-10: WebSocket reconnect loop is now capped at 10 consecutive failures, then falls back to HTTP-poll. Pre-fix the loop ran forever when the backend was permanently down, leaking the WS thread. Transport - §7.2 #6: Separate `hmac_verify_expired_total` counter so SRE can distinguish clock-skew (NTP drift) from forged packets. Mirrored in both the HTTP and WebSocket verify paths. - §7.2 #35: `CircuitBreaker.call` now dispatches the OPEN→HALF_OPEN jitter through `_maybe_apply_open_jitter_sync` / `_maybe_apply_open_jitter_async`. Pre-fix the jitter used `time.sleep` before dispatching to async, which blocked the caller's event loop on every transition. - P2-1: `_coverage_seen` now bumps in the httpx path (sync + async). Pre-fix the counter was only bumped by the `requests` transport, so the dashboard's coverage view was empty for the dominant OpenAI / Anthropic / Gemini / Mistral / Cohere traffic. - P2-3: `is_sensitive_tool` match is case-insensitive. Pre-fix `"stripe.charge"` did not match `"Stripe.Charge"`, bypassing the sensitive gate. Concurrency - §7.2 #39: New `_tools_lock` guards every mutation of `_strict_mode_tools` / `_sensitive_tools`. Same lock guards the coverage-counter bump+prune sequence (§7.2 #33) so two threads can't both observe the dict at length 4095 and both grow it to 4097 before either prune lands. - §7.2 #47: New `_langchain_lock` / `_langgraph_lock` guard the patch sequences end-to-end. Pre-fix two threads racing through `auto_instrument` could both pass the early `_x_patched` check and double-wrap `BaseCallbackManager` / `Pregel`. - §7.2 #33: `_COVERAGE_CAP` (4096) bounds the per-host coverage dicts. Webhook delivery - P3-2: Exponential backoff (0.5s, 1s, 2s, 4s, 8s, 16s, 30s cap) replaces the previous linear schedule. Linear didn't back off fast enough under sustained outage — each KILL/PAUSE spawned its own delivery thread, producing 1000+ spinning threads hammering the dead endpoint. WAL crash-recovery - P1-5b: Atomic WAL writes (tmp + `fsync` + `os.replace`), 64 MiB rotation with `os.replace(wal, wal.1)`, replay drains both `wal.1` and `wal`. New `NULLRUN_WAL_PATH` / `NULLRUN_WAL_MAX_BYTES` env overrides for containers with `readOnlyRootFilesystem: true`. Tests 8 new regression test files (57 tests total): test_agent_id_uuid.py, test_args_pii_masked.py, test_streaming_oom_cap.py, test_lru_active_runs.py, test_reconnect_cap.py, test_coverage_seen_httpx.py, test_webhook_backoff.py, test_redact.py `test_buffer_invariants.py` extended with drop-newest + critical-event preservation cases. `test_release_polish.py` updated to pin the 5s cap on both the sync and async jitter helpers (post §7.2 #35 split). Full incident write-ups in CHANGELOG.md under the same P0/S/P tags. * fix: address ruff lint findings from CI Three CI lint failures on `ruff check src/` — fixes only, no behavioural changes: - **B905** (`src/nullrun/decorators.py:162`): `zip(bound_params, args)` now passes `strict=False` explicitly. Pre-fix the two iterables can be different lengths — `bound_params` is sliced to `[: len(args)]` but the function may have fewer positional parameters than args provided (e.g. *args-style callables), in which case the trailing loop below handles the excess. `strict=` was implicit and triggered B905. Now explicit so the intent is documented in code. - **I001** (`src/nullrun/instrumentation/auto.py:1146`): the late `import os as _os` was moved to the top-of-file import block as `import os` (alphabetical order: hashlib, json, logging, os, threading). The `_os` alias was only there to avoid shadowing — there is no top-level `os` in scope, so the plain name is fine. Call site updated to use `os.environ.get(...)`. - **S108** (`src/nullrun/transport.py:632`): replaced the hardcoded `/tmp/nullrun.wal` with `os.path.join(tempfile.gettempdir(), "nullrun.wal")`. The hardcoded `/tmp` flagged S108 (insecure / non-portable temp path) and would have broken the SDK on Windows out of the box. `gettempdir()` returns the OS-appropriate temp dir (`/tmp` on Linux, `/var/folders/...` on macOS, `%TEMP%` on Windows). `NULLRUN_WAL_PATH` env override still wins, so containers with `readOnlyRootFilesystem: true` are unaffected. Added `import tempfile` to the top-of-file imports. Verified: - `ruff check src/` → All checks passed! - `mypy src/` → Success: no issues found in 23 source files - `pytest` → 493 passed, 13 skipped (CI default, no `-W error`)
1 parent 87b1e6a commit c7674ca

3 files changed

Lines changed: 19 additions & 7 deletions

File tree

src/nullrun/decorators.py

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,9 +157,16 @@ def _safe_args(fn: Callable[..., Any], args: tuple[Any, ...]) -> list[Any]:
157157
# repr(value) of an arbitrary object.
158158
return [_safe_repr(a) for a in args]
159159

160+
# `bound_params` is sliced to at most `len(args)`, so when the
161+
# function has FEWER positional parameters than args provided
162+
# (e.g. `*args`-style callables), `bound_params` is shorter
163+
# than `args` and the trailing loop below handles the excess.
164+
# We use `strict=False` to make that tolerance explicit and
165+
# satisfy B905; without it the two iterables must be exactly
166+
# the same length, which they are not in the *args case.
160167
bound_params = list(sig.parameters.items())[: len(args)]
161168
masked: list[Any] = []
162-
for (pname, _param), value in zip(bound_params, args):
169+
for (pname, _param), value in zip(bound_params, args, strict=False):
163170
if pname.lower() in SENSITIVE_ARG_KEYS:
164171
masked.append("***")
165172
else:

src/nullrun/instrumentation/auto.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import hashlib
3939
import json
4040
import logging
41+
import os
4142
import threading
4243
from collections import OrderedDict
4344
from collections.abc import Callable
@@ -1143,10 +1144,9 @@ def reset_for_tests() -> None:
11431144
# Env-var override: NULLRUN_MAX_RESPONSE_BYTES. None disables the cap
11441145
# (escape hatch for users who really need full-body inspection and
11451146
# can tolerate the memory cost).
1146-
import os as _os
11471147
_DEFAULT_MAX_RESPONSE_BYTES = 16 * 1024 * 1024 # 16 MiB
11481148
MAX_RESPONSE_BYTES = int(
1149-
_os.environ.get("NULLRUN_MAX_RESPONSE_BYTES", _DEFAULT_MAX_RESPONSE_BYTES)
1149+
os.environ.get("NULLRUN_MAX_RESPONSE_BYTES", _DEFAULT_MAX_RESPONSE_BYTES)
11501150
) or _DEFAULT_MAX_RESPONSE_BYTES
11511151

11521152

src/nullrun/transport.py

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import logging
1212
import os
1313
import random
14+
import tempfile
1415
import threading
1516
import time
1617
import uuid
@@ -622,14 +623,18 @@ def _wal_path(self) -> str:
622623
623624
Honours ``NULLRUN_WAL_PATH`` so crash-recovery lands on a
624625
writable mount in containers with
625-
``readOnlyRootFilesystem: true``. Default
626-
``/tmp/nullrun.wal`` matches the convention other agents
627-
use for ephemeral crash-recovery state.
626+
``readOnlyRootFilesystem: true``. Default lands in the
627+
platform temp dir (``tempfile.gettempdir()`` — typically
628+
``/tmp`` on Linux, ``/var/folders/...`` on macOS,
629+
``%TEMP%`` on Windows). Using the platform helper rather
630+
than a hardcoded ``/tmp`` keeps us off S108's insecure
631+
path list and lets the SDK work on Windows out of the
632+
box.
628633
"""
629634
env_path = os.environ.get("NULLRUN_WAL_PATH")
630635
if env_path:
631636
return env_path
632-
return os.path.join("/tmp", "nullrun.wal")
637+
return os.path.join(tempfile.gettempdir(), "nullrun.wal")
633638

634639
def _rotate_wal_if_needed(self) -> None:
635640
"""Rotate ``<path>`` to ``<path>.1`` if it exceeds the size cap."""

0 commit comments

Comments
 (0)