mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-09-30 05:43:14 +00:00
harden PR guards + sidebar after security review
- Mirror upstream #1786's nested-shell / executable-normalization hardening into the fork-only pr_verdict_guard.py (verdict-gating is a real fork control), keeping it in parity with pr_creation_guard.py. - Close the glued short-flag bypass (bash -c'...') in BOTH guards: a shell's -c argument can be concatenated into the same argv token, which the space-separated -c detection missed. Diverges pr_creation_guard.py from upstream #1786 by design; to be upstreamed. - Gate the new #1799 sidebar active-thread refresh on ownership so a non-owner viewing a shared thread reads last-known state without persisting a metadata write (mirrors the is_owner gate on the single-thread read path). - Fix an F821 in the #1799 cherry-pick (Mapping import / concrete dict type). Guards remain intentionally fail-open per the honest-agent threat model; docstrings narrowed to name the residual exotic-shell / stdin-fed vectors.
This commit is contained in:
parent
e02de83f57
commit
1ba5ccc65d
5 changed files with 103 additions and 8 deletions
|
|
@ -8,7 +8,7 @@ import binascii
|
|||
import json
|
||||
import logging
|
||||
import os
|
||||
from collections.abc import AsyncIterator
|
||||
from collections.abc import AsyncIterator, Mapping
|
||||
from datetime import UTC, datetime
|
||||
from typing import Any
|
||||
|
||||
|
|
@ -726,7 +726,7 @@ async def _sidebar_active_thread_summary(
|
|||
client: Any,
|
||||
active_thread_id: str | None,
|
||||
*,
|
||||
fallback_threads: Mapping[str, ThreadLike],
|
||||
fallback_threads: Mapping[str, dict[str, Any]],
|
||||
visible_thread_ids: set[str],
|
||||
login: str,
|
||||
email: str | None,
|
||||
|
|
@ -752,11 +752,17 @@ async def _sidebar_active_thread_summary(
|
|||
_assert_thread_readable(metadata)
|
||||
except HTTPException:
|
||||
return None
|
||||
# Refreshing persists latest-run metadata back to the thread; only do that
|
||||
# when the caller owns it, mirroring the is_owner gate on the single-thread
|
||||
# read path. A non-owner viewing a shared thread reads its last-known state
|
||||
# without mutating it.
|
||||
owns_thread = _user_owns_thread(metadata, login, email)
|
||||
summary = await _summarize_thread(
|
||||
client,
|
||||
thread,
|
||||
owner_login=None if include_all else login,
|
||||
owner_email=None if include_all else email,
|
||||
refresh_active_run=owns_thread,
|
||||
)
|
||||
return summary, _is_thread_resolved(metadata)
|
||||
|
||||
|
|
|
|||
|
|
@ -68,6 +68,9 @@ def _shell_command_argument(tokens: list[str], shell_index: int) -> str | None:
|
|||
if token == "-c" or (
|
||||
token.startswith("-") and not token.startswith("--") and "c" in token[1:]
|
||||
):
|
||||
_, _, glued = token[1:].partition("c")
|
||||
if glued:
|
||||
return glued
|
||||
if index + 1 < len(tokens) and tokens[index + 1] not in _SHELL_SEPARATORS:
|
||||
return tokens[index + 1]
|
||||
return None
|
||||
|
|
|
|||
|
|
@ -3,6 +3,7 @@
|
|||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import os
|
||||
import re
|
||||
import shlex
|
||||
from collections.abc import Awaitable, Callable, Mapping
|
||||
|
|
@ -14,6 +15,9 @@ from langgraph.prebuilt.tool_node import ToolCallRequest
|
|||
from langgraph.types import Command
|
||||
|
||||
_SHELL_SEPARATORS = {";", "&&", "||", "|", "&"}
|
||||
_SHELL_EXECUTABLES = {"bash", "dash", "sh", "zsh"}
|
||||
_MAX_SHELL_EXPANSION_DEPTH = 3
|
||||
_SHELL_EXPANSION_DEPTH_LIMIT_TOKEN = "__pr_verdict_guard_shell_expansion_depth_limit__"
|
||||
_GITHUB_REVIEWS_ENDPOINT = re.compile(
|
||||
r"(?:^|/)repos/[^/\s]+/[^/\s]+/pulls/\d+/reviews(?:/\d+/events)?/?$"
|
||||
)
|
||||
|
|
@ -55,13 +59,62 @@ def _tool_call_id(request: ToolCallRequest) -> str | None:
|
|||
return None
|
||||
|
||||
|
||||
def _shell_tokens(command: str) -> list[str]:
|
||||
def _split_shell_tokens(command: str) -> list[str]:
|
||||
try:
|
||||
return shlex.split(command, posix=True)
|
||||
except ValueError:
|
||||
return command.split()
|
||||
|
||||
|
||||
def _executable_name(token: str) -> str:
|
||||
return os.path.basename(token.strip("'\""))
|
||||
|
||||
|
||||
def _shell_command_argument(tokens: list[str], shell_index: int) -> str | None:
|
||||
for index, token in enumerate(tokens[shell_index + 1 :], start=shell_index + 1):
|
||||
if token in _SHELL_SEPARATORS:
|
||||
return None
|
||||
if token == "-c" or (
|
||||
token.startswith("-") and not token.startswith("--") and "c" in token[1:]
|
||||
):
|
||||
_, _, glued = token[1:].partition("c")
|
||||
if glued:
|
||||
return glued
|
||||
if index + 1 < len(tokens) and tokens[index + 1] not in _SHELL_SEPARATORS:
|
||||
return tokens[index + 1]
|
||||
return None
|
||||
return None
|
||||
|
||||
|
||||
def _has_nested_shell_command(tokens: list[str]) -> bool:
|
||||
return any(
|
||||
_executable_name(token) in _SHELL_EXECUTABLES
|
||||
and _shell_command_argument(tokens, index) is not None
|
||||
for index, token in enumerate(tokens)
|
||||
)
|
||||
|
||||
|
||||
def _expand_nested_shell_tokens(tokens: list[str], depth: int = 0) -> list[str]:
|
||||
if depth >= _MAX_SHELL_EXPANSION_DEPTH:
|
||||
if _has_nested_shell_command(tokens):
|
||||
return [*tokens, _SHELL_EXPANSION_DEPTH_LIMIT_TOKEN]
|
||||
return tokens
|
||||
|
||||
expanded = list(tokens)
|
||||
for index, token in enumerate(tokens):
|
||||
if _executable_name(token) not in _SHELL_EXECUTABLES:
|
||||
continue
|
||||
inner_command = _shell_command_argument(tokens, index)
|
||||
if inner_command is None:
|
||||
continue
|
||||
expanded.extend(_expand_nested_shell_tokens(_split_shell_tokens(inner_command), depth + 1))
|
||||
return expanded
|
||||
|
||||
|
||||
def _shell_tokens(command: str) -> list[str]:
|
||||
return _expand_nested_shell_tokens(_split_shell_tokens(command))
|
||||
|
||||
|
||||
def _subtokens_after(tokens: list[str], index: int) -> list[str]:
|
||||
subtokens: list[str] = []
|
||||
for token in tokens[index + 1 :]:
|
||||
|
|
@ -73,7 +126,7 @@ def _subtokens_after(tokens: list[str], index: int) -> list[str]:
|
|||
|
||||
def _contains_gh_pr_review_verdict(tokens: list[str]) -> bool:
|
||||
for index, token in enumerate(tokens):
|
||||
if token != "gh":
|
||||
if _executable_name(token) != "gh":
|
||||
continue
|
||||
subtokens = _subtokens_after(tokens, index)
|
||||
is_pr_review = any(
|
||||
|
|
@ -92,7 +145,7 @@ def _contains_gh_pr_review_verdict(tokens: list[str]) -> bool:
|
|||
|
||||
def _contains_gh_api_review_verdict(tokens: list[str]) -> bool:
|
||||
for index, token in enumerate(tokens):
|
||||
if token != "gh":
|
||||
if _executable_name(token) != "gh":
|
||||
continue
|
||||
subtokens = _subtokens_after(tokens, index)
|
||||
if "api" not in subtokens:
|
||||
|
|
@ -111,7 +164,7 @@ def _contains_gh_api_review_verdict(tokens: list[str]) -> bool:
|
|||
|
||||
def _contains_direct_review_verdict(tokens: list[str]) -> bool:
|
||||
for index, token in enumerate(tokens):
|
||||
if token != "curl":
|
||||
if _executable_name(token) != "curl":
|
||||
continue
|
||||
subtokens = _subtokens_after(tokens, index)
|
||||
if not any(_GITHUB_REVIEWS_URL.search(subtoken) for subtoken in subtokens):
|
||||
|
|
@ -129,7 +182,13 @@ def is_pr_verdict_fallback_command(command: str) -> bool:
|
|||
``event=APPROVE|REQUEST_CHANGES`` to a ``/pulls/N/reviews`` endpoint, and
|
||||
``curl`` to the reviews URL with a verdict event in the body) but is
|
||||
intentionally fail-open: shell aliases, ``gh`` aliases, ``--input``
|
||||
JSON-file bodies, and non-curl HTTP clients will not be blocked. This is
|
||||
JSON-file bodies, and non-curl HTTP clients will not be blocked. The
|
||||
``-c`` form of a known shell (``bash``/``dash``/``sh``/``zsh``, whether
|
||||
space-separated or glued as ``-c'...'``) is expanded to a fixed depth and
|
||||
quoted/path-prefixed executables are normalized, so those do not slip
|
||||
through; other shells (``ash``/``busybox``/``fish``) and stdin-fed
|
||||
programs (``... | bash``, here-strings, ``bash -s``) remain fail-open.
|
||||
This is
|
||||
acceptable because the threat model is an honest agent papering over a
|
||||
``publish_review`` limitation or refusal, not an adversary trying to
|
||||
bypass the guardrail. Plain ``gh pr review`` (no verdict flag) and
|
||||
|
|
@ -138,7 +197,8 @@ def is_pr_verdict_fallback_command(command: str) -> bool:
|
|||
"""
|
||||
tokens = _shell_tokens(command)
|
||||
return (
|
||||
_contains_gh_pr_review_verdict(tokens)
|
||||
_SHELL_EXPANSION_DEPTH_LIMIT_TOKEN in tokens
|
||||
or _contains_gh_pr_review_verdict(tokens)
|
||||
or _contains_gh_api_review_verdict(tokens)
|
||||
or _contains_direct_review_verdict(tokens)
|
||||
)
|
||||
|
|
|
|||
|
|
@ -43,6 +43,8 @@ def test_detects_pr_creation_fallback_commands() -> None:
|
|||
"/usr/bin/curl -X POST https://api.github.com/repos/langchain-ai/open-swe/pulls -d '{}'"
|
||||
)
|
||||
assert is_pr_creation_fallback_command("bash -c 'gh pr create --draft'")
|
||||
assert is_pr_creation_fallback_command("bash -c'gh pr create --draft'")
|
||||
assert is_pr_creation_fallback_command("zsh -lc'gh pr create --draft'")
|
||||
assert is_pr_creation_fallback_command("GH_TOKEN=dummy sh -c 'gh pr create --draft'")
|
||||
assert is_pr_creation_fallback_command(
|
||||
"zsh -lc 'gh api repos/langchain-ai/open-swe/pulls -X POST -f title=x'"
|
||||
|
|
|
|||
|
|
@ -60,6 +60,28 @@ def test_detects_curl_review_verdicts() -> None:
|
|||
)
|
||||
|
||||
|
||||
def test_detects_nested_and_normalized_verdict_commands() -> None:
|
||||
assert is_pr_verdict_fallback_command("/usr/bin/gh pr review 12 --approve")
|
||||
assert is_pr_verdict_fallback_command(
|
||||
"/usr/bin/curl -X POST https://api.github.com/repos/o/r/pulls/5/reviews "
|
||||
'-d \'{"event": "APPROVE"}\''
|
||||
)
|
||||
assert is_pr_verdict_fallback_command("bash -c 'gh pr review 12 --approve'")
|
||||
assert is_pr_verdict_fallback_command("bash -c'gh pr review 12 --approve'")
|
||||
assert is_pr_verdict_fallback_command("zsh -lc'gh pr review 12 --approve'")
|
||||
assert is_pr_verdict_fallback_command("GH_TOKEN=dummy sh -c 'gh pr review -a'")
|
||||
assert is_pr_verdict_fallback_command(
|
||||
"zsh -lc 'gh api repos/o/r/pulls/5/reviews -X POST -f event=REQUEST_CHANGES'"
|
||||
)
|
||||
assert is_pr_verdict_fallback_command(
|
||||
'bash -c "curl -X POST https://api.github.com/repos/o/r/pulls/5/reviews '
|
||||
'-d \'{\\"event\\": \\"APPROVE\\"}\'"'
|
||||
)
|
||||
assert is_pr_verdict_fallback_command(
|
||||
'bash -c \'sh -c "dash -c \\"zsh -c \\\\\\"gh pr review 12 --approve\\\\\\"\\""\''
|
||||
)
|
||||
|
||||
|
||||
def test_allows_safe_review_commands() -> None:
|
||||
assert not is_pr_verdict_fallback_command("gh pr review 12 --comment -b 'looks good'")
|
||||
assert not is_pr_verdict_fallback_command("gh pr review -c")
|
||||
|
|
@ -69,6 +91,8 @@ def test_allows_safe_review_commands() -> None:
|
|||
assert not is_pr_verdict_fallback_command("gh api repos/o/r/pulls/5/reviews")
|
||||
assert not is_pr_verdict_fallback_command("gh pr create --draft")
|
||||
assert not is_pr_verdict_fallback_command("curl https://api.github.com/repos/o/r/pulls/5")
|
||||
assert not is_pr_verdict_fallback_command("bash -c 'gh pr review 12 --comment -b ok'")
|
||||
assert not is_pr_verdict_fallback_command("/usr/bin/gh pr view 12 --json reviews")
|
||||
|
||||
|
||||
async def test_middleware_blocks_execute_verdict_fallbacks() -> None:
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue