Security hardening: validate dynamic imports, URLs, and credential handling - #42
Merged
Merged
Conversation
Wrap unguarded file reads and parse calls with specific exception handlers that include the file path in the error message: - config.py: load_config catches FileNotFoundError, PermissionError, yaml.YAMLError and re-raises as ConfigError - otel.py: _parse_otlp_json catches FileNotFoundError, JSONDecodeError - tools.py: load_toolset_file catches FileNotFoundError, yaml.YAMLError - systematization_convert.py: run_systematization_to_policy catches FileNotFoundError, JSONDecodeError
- CallableSession.open: catch ModuleNotFoundError, AttributeError with actionable messages including the callable ref string - OTelTracedSession.open: same pattern for OTel-traced callable imports - HTTPEndpointSession.run_turn: catch aiohttp.ClientResponseError and ClientError with endpoint URL and HTTP status in the message
- judge.py: catch JSONDecodeError when loading policy file, add LLM error types and json/ValueError before the catch-all in the worker, log traceback via log.debug for all caught worker exceptions - policy.py: include model name and raw response text in the error message when policy generation returns non-JSON output
- rollout.py worker: add LLM error types re-raise before catch-all, add ValueError/KeyError handler, log.debug with traceback and seed_id for all caught worker exceptions - rollout.py auditor: log.debug with traceback on each retry failure - systematization.py: catch JSONDecodeError on json.loads fallback with raw text snippet in the error message
Catch ConnectionError separately from generic Exception in PhoenixCollector.get_spans with a targeted message about Phoenix connectivity. Validate 'context.trace_id' column existence before filtering.
Cover all new error handling across config, otel, tools, session, otel_session, judge, systematization_convert, rollout, and collector. Verify exception types, error messages include context (file paths, module names, endpoint URLs), and debug logging fires with tracebacks in worker failures.
CI environment doesn't include pandas (optional dependency). The PhoenixCollector.get_spans method does 'import pandas' at the top, which fails before our exception handling runs.
…cache Align artifact_cache.py and viewer_read_model.py with the centralized logging convention from PR #22. Replace 10 print(file=sys.stderr) calls with log.warning() using structured placeholders so they respect --quiet, --log-file, and --output json. Remove unused sys imports from both modules.
… test - Move 'import aiohttp' from run_turn to open(), store as self._aiohttp to avoid re-importing on every turn call - Use port 59123 instead of port 1 in HTTPEndpointSession test for better portability across CI/container environments
…ndling Add p2m/core/security.py with centralized security validation: - validate_callable_ref(): block dangerous path segments in callable target references (FINDING-001, CWE-94) - validate_sys_path_addition(): scope sys.path additions to workspace boundaries (FINDING-002, CWE-427) - validate_endpoint_url(): block SSRF via private IPs and cloud metadata endpoints (FINDING-003, CWE-918) - sanitize_payload(): redact credentials from artifact files (FINDING-004, CWE-532) - validate_module_ref(): block dangerous segments in connector/tool module references (FINDING-006, CWE-94) - _validate_module_file_path(): enforce workspace boundaries on direct .py file loading (FINDING-010, CWE-94) - validate_dotenv_location(): warn on suspicious .env locations (FINDING-009, CWE-426) - P2M_NO_AUTO_ENV opt-out for auto_envvar_prefix (FINDING-005, CWE-15) - _require_within() on resolved artifact paths (FINDING-007, CWE-22) Opt-out environment variables for backward compatibility: - P2M_TRUST_CALLABLE=1: suppress callable import warnings - P2M_ALLOW_PRIVATE_ENDPOINTS=1: allow localhost/private endpoints - P2M_NO_DOTENV=1: disable automatic .env loading - P2M_NO_AUTO_ENV=1: disable P2M_* env var overrides Update test_exception_handling.py to use P2M_ALLOW_PRIVATE_ENDPOINTS for localhost endpoint test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…credential leak - validate_endpoint_url(): resolve hostnames via socket.getaddrinfo() and validate resulting IPs against blocked ranges (FINDING-002, CWE-918) - sanitize_payload(): redact at max_depth instead of passing through unsanitized (FINDING-003, CWE-200) - HTTPEndpointSession: add _sanitize_response_text() to redact credential patterns before writing to transcripts (FINDING-004, CWE-532) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove P2M_NO_DOTENV, P2M_NO_AUTO_ENV, P2M_TRUST_CALLABLE and associated code (should_load_dotenv, validate_dotenv_location, is_callable_trusted, P2MSecurityWarning). These addressed scenarios from an incorrect threat model — load_dotenv(override=False) already safe, CI owns its env vars, and callable import warnings block the primary workflow. Keep: format validation, blocklist, SSRF prevention, credential sanitization.
All other modules use 'log = logging.getLogger(__name__)' per PR #22.
AaronAspinwall123
left a comment
AaronAspinwall123
left a comment
Collaborator
There was a problem hiding this comment.
Do we want to add some UTs for this?
50 tests covering validate_callable_ref, validate_module_ref, validate_sys_path_addition, validate_endpoint_url (SSRF/DNS rebinding), sanitize_payload, and _sanitize_response_text.
Collaborator
Author
Good call. The security assessment agent wrote tests in a gitignored folder, so they weren't part of the PR. Added test_security.py with 50 unit tests covering all the security functions (input validation, SSRF/DNS rebinding, credential sanitization, response text scrubbing). |
huliang-microsoft
approved these changes
May 15, 2026
AaronAspinwall123
approved these changes
May 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Security hardening for p2m's own behavior — fixes bugs where p2m could harm the developer, not where the developer's own code does what they told it to do.
Threat Model
p2m is a local CLI tool analogous to
pytest— it runs whatever code the developer points it at. Dynamic import of user-specified callables, connectors, and tool modules is the core feature. The user who runsp2m runis the same person who wrote or chose to trust the config.This PR focuses on risks where p2m's own behavior could unexpectedly leak credentials, make unsafe network requests, or write sensitive data to artifacts.
Findings Addressed (Run 2 — Corrected Threat Model)
validate_endpoint_url()sanitize_payload()max_depth bypass leaks deeply nested secretsSecurity Fixes
p2m/core/security.py_validate_resolved_ips()— resolves hostnames viasocket.getaddrinfo()and validates IPs against blocked ranges (SSRF)p2m/core/security.pysanitize_payload()now redacts at max_depth instead of passing through unsanitizedp2m/core/security.py168.63.129.16) added to SSRF blocklistp2m/core/session.py_sanitize_response_text()— regex-based credential pattern scanning on HTTP endpoint responsesp2m/core/model_client.pybuild_llm_call_trace()sanitizes request payloads before writing to artifactsInput Validation (retained as config mistake catchers)
validate_callable_ref():) and accidental__pycache__/.gitimportsvalidate_module_ref()validate_sys_path_addition()_validate_module_file_path().pytool paths_require_within()on artifact pathsOpt-out Environment Variable
P2M_ALLOW_PRIVATE_ENDPOINTS=1— allow localhost/private IP endpoints (for local development)Removed (vestigial from incorrect threat model)
The following were added in the initial assessment run but removed after review:
P2M_TRUST_CALLABLE— warned on every normal callable importP2M_NO_DOTENV/validate_dotenv_location()—load_dotenv(override=False)is already safeP2M_NO_AUTO_ENV— CI owns its own environment variablesTesting
log = logging.getLogger(__name__)consistently per PR feat: centralized logging with --verbose/--quiet/--log-file/--output json CLI flags #22