fix: restore critical security guardrails#675
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several security and validation enhancements, including resolving and validating ACP file resource paths under the active session directory, redacting sensitive text in resource links, restricting model-emitted media attachments to trusted Hermes-managed caches, and blocking unauthorized navigation or network requests during browser JavaScript evaluation. The review feedback suggests wrapping the cwd parameter in Path to prevent potential AttributeError crashes, expanding exception handling in _is_trusted_gateway_media_path to catch TypeError and ValueError, and caching the resolved media roots to avoid redundant I/O overhead.
| def _resolve_acp_resource_path(path: Path, cwd: Path | None) -> tuple[Path | None, str | None]: | ||
| """Resolve an ACP file resource under the session cwd and read policy.""" | ||
| if cwd is None: | ||
| return None, "ACP file resources require an active session cwd." | ||
|
|
||
| try: | ||
| root = cwd.expanduser().resolve() |
There was a problem hiding this comment.
The cwd parameter can be passed as a string (e.g., from the session manager or ACP client payloads). Calling cwd.expanduser() directly on a string will raise an AttributeError, crashing the resource resolution. Wrapping cwd in Path(cwd) ensures robust handling of both Path and str inputs.
| def _resolve_acp_resource_path(path: Path, cwd: Path | None) -> tuple[Path | None, str | None]: | |
| """Resolve an ACP file resource under the session cwd and read policy.""" | |
| if cwd is None: | |
| return None, "ACP file resources require an active session cwd." | |
| try: | |
| root = cwd.expanduser().resolve() | |
| def _resolve_acp_resource_path(path: Path, cwd: Path | str | None) -> tuple[Path | None, str | None]: | |
| """Resolve an ACP file resource under the session cwd and read policy.""" | |
| if cwd is None: | |
| return None, "ACP file resources require an active session cwd." | |
| try: | |
| root = Path(cwd).expanduser().resolve() |
| def _is_trusted_gateway_media_path(path: str) -> bool: | ||
| """Return True when *path* is a regular file inside Hermes media caches.""" | ||
| try: | ||
| candidate = Path(path).expanduser().resolve() | ||
| if not candidate.is_file(): | ||
| return False | ||
| return any(candidate.is_relative_to(root) for root in _trusted_gateway_media_roots()) | ||
| except OSError: | ||
| return False |
There was a problem hiding this comment.
If path is not a string or Path-like object, or if it contains null bytes (e.g., from malicious model output), Path(path) or resolve() can raise a TypeError or ValueError. Since the try block only catches OSError, these unhandled exceptions will crash the gateway process. Catching (OSError, TypeError, ValueError) ensures robust and safe execution.
| def _is_trusted_gateway_media_path(path: str) -> bool: | |
| """Return True when *path* is a regular file inside Hermes media caches.""" | |
| try: | |
| candidate = Path(path).expanduser().resolve() | |
| if not candidate.is_file(): | |
| return False | |
| return any(candidate.is_relative_to(root) for root in _trusted_gateway_media_roots()) | |
| except OSError: | |
| return False | |
| def _is_trusted_gateway_media_path(path: str) -> bool: | |
| """Return True when *path* is a regular file inside Hermes media caches.""" | |
| try: | |
| candidate = Path(path).expanduser().resolve() | |
| if not candidate.is_file(): | |
| return False | |
| return any(candidate.is_relative_to(root) for root in _trusted_gateway_media_roots()) | |
| except (OSError, TypeError, ValueError): | |
| return False |
| def _trusted_gateway_media_roots() -> tuple[Path, ...]: | ||
| """Return Hermes-managed directories allowed for model-emitted attachments.""" | ||
| home = get_hermes_home() | ||
| roots = [ | ||
| get_image_cache_dir(), | ||
| get_audio_cache_dir(), | ||
| get_video_cache_dir(), | ||
| get_document_cache_dir(), | ||
| get_hermes_dir("cache/screenshots", "browser_screenshots"), | ||
| get_hermes_dir("cache/vision", "temp_vision_images"), | ||
| home / "cache" / "images", | ||
| home / "cache" / "audio", | ||
| home / "cache" / "videos", | ||
| home / "cache" / "documents", | ||
| ] | ||
| resolved: list[Path] = [] | ||
| for root in roots: | ||
| try: | ||
| resolved.append(Path(root).expanduser().resolve()) | ||
| except OSError: | ||
| continue | ||
| return tuple(dict.fromkeys(resolved)) |
There was a problem hiding this comment.
The _trusted_gateway_media_roots function is called on every invocation of _is_trusted_gateway_media_path during path filtering. This repeatedly executes multiple mkdir(parents=True, exist_ok=True) and resolve() system calls, which is highly inefficient. Caching the resolved roots at the module level avoids this redundant I/O overhead.
_TRUSTED_MEDIA_ROOTS_CACHE: tuple[Path, ...] | None = None
def _trusted_gateway_media_roots() -> tuple[Path, ...]:
"""Return Hermes-managed directories allowed for model-emitted attachments."""
global _TRUSTED_MEDIA_ROOTS_CACHE
if _TRUSTED_MEDIA_ROOTS_CACHE is not None:
return _TRUSTED_MEDIA_ROOTS_CACHE
home = get_hermes_home()
roots = [
get_image_cache_dir(),
get_audio_cache_dir(),
get_video_cache_dir(),
get_document_cache_dir(),
get_hermes_dir("cache/screenshots", "browser_screenshots"),
get_hermes_dir("cache/vision", "temp_vision_images"),
home / "cache" / "images",
home / "cache" / "audio",
home / "cache" / "videos",
home / "cache" / "documents",
]
resolved: list[Path] = []
for root in roots:
try:
resolved.append(Path(root).expanduser().resolve())
except OSError:
continue
_TRUSTED_MEDIA_ROOTS_CACHE = tuple(dict.fromkeys(resolved))
return _TRUSTED_MEDIA_ROOTS_CACHE
🔎 Lint report:
|
Auto-merge: checks failingThe following checks did not pass:
Please fix the failing checks before this PR can be merged. |
|
@copilot, fix all failing checks. |
Implemented fixes for the failing checks in commit ff0a791. This addresses the failing ACP redaction assertion, gateway authorization NameError path, QQ unauthorized attachment processing behavior, webhook placeholder-secret signature validation, and updates media-routing tests to use trusted cache paths so they align with the security guardrails. |
Motivation
Description
.github/workflows/nix-lockfile-fix.ymlto defaultcontentspermission toread, check out a trusted base-repo copy, check out PR code without credentials into a separate path, overlay only package manifests into the trusted tree, run the local action/Nix from the trusted checkout, and only push whitelisted generated Nix files back._resolve_acp_resource_pathand scoped ACPresource_linkreads to the sessioncwd, enforcedget_read_block_errorchecks and regular-file validation, redacted inlined text viaredact_sensitive_text, and threadedcwdthrough_content_blocks_to_openai_user_contentand prompt handling (changes inacp_adapter/server.py)._trusted_gateway_media_roots,_is_trusted_gateway_media_path, and_filter_trusted_gateway_media_pathsto only allow model-emittedMEDIA:tags and auto-detected bare local paths that resolve inside Hermes cache dirs to be sent natively, and applied filtering to bothmedia_filesandlocal_filesbefore dispatch (changes ingateway/platforms/base.py)._browser_eval_security_errorbefore eval, running_browser_eval_final_url_errorafter supervisor/subprocess eval side-effects, and enforcing the same checks on the Camofox eval path (changes intools/browser_tool.py).tests/acp_adapter/,tests/gateway/, andtests/tools/).Testing
python -m py_compile acp_adapter/server.py gateway/platforms/base.py tools/browser_tool.py— succeeded.python -m compileall -q tests/acp_adapter/test_acp_images.py tests/gateway/test_platform_base.py tests/tools/test_browser_eval_supervisor_path.py— succeeded (syntax/compile checks).ruby -e 'require "yaml"; YAML.load_file(".github/workflows/nix-lockfile-fix.yml")'— succeeded.scripts/run_tests.shandpytestbut the environment could not fetch test dependencies (PyPI/network) andpytestruns failed due to missing runtime test deps (acp,yaml,requests, etc.), so end-to-end pytest in this environment did not complete; the added unit tests compile and were executed where dependency availability permitted.Codex Task