mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
review: fix mypy, tighten env scrub, bridge storage safety
Address Copilot + code review feedback: - Fix mypy: rename tool_names → _policy_names in CLI to avoid type clash - Tighten env scrub from substring to suffix matching (_KEY, _TOKEN, etc.) to avoid false positives on MONKEYTYPE, KEYBOARD_LAYOUT - Add DATABASE_URL/TURNSTONE_DB_URL to explicit scrub list - Bridge: use _storage directly instead of get_storage() which auto-initializes a local SQLite DB with no admin policies - Discord bot: add self.storage None guard - Add tests for suffix-only matching and false positive avoidance
This commit is contained in:
committed by
Patrick Buckley
parent
771d03b8e6
commit
58e2d9348f
@@ -15,18 +15,22 @@ class TestIsSecret:
|
||||
assert _is_secret("TURNSTONE_JWT_SECRET") is True
|
||||
assert _is_secret("AWS_SECRET_ACCESS_KEY") is True
|
||||
|
||||
def test_pattern_matching(self):
|
||||
def test_suffix_matching(self):
|
||||
assert _is_secret("MY_CUSTOM_API_KEY") is True
|
||||
assert _is_secret("DB_PASSWORD") is True
|
||||
assert _is_secret("AUTH_TOKEN") is True
|
||||
assert _is_secret("SERVICE_CREDENTIAL") is True
|
||||
assert _is_secret("GCP_CREDENTIALS") is True
|
||||
|
||||
def test_safe_vars_not_secret(self):
|
||||
assert _is_secret("PATH") is False
|
||||
assert _is_secret("HOME") is False
|
||||
assert _is_secret("LANG") is False
|
||||
|
||||
def test_non_secret_vars(self):
|
||||
def test_no_false_positives_on_substring(self):
|
||||
"""Suffix matching avoids false positives like MONKEYTYPE."""
|
||||
assert _is_secret("MONKEYTYPE") is False
|
||||
assert _is_secret("KEYBOARD_LAYOUT") is False
|
||||
assert _is_secret("PYTHONPATH") is False
|
||||
assert _is_secret("EDITOR") is False
|
||||
assert _is_secret("GOPATH") is False
|
||||
|
||||
@@ -120,8 +120,8 @@ class TestBridgePolicyEnforcement:
|
||||
return_value={"bash": "deny"},
|
||||
),
|
||||
patch(
|
||||
"turnstone.core.storage._registry.get_storage",
|
||||
return_value=MagicMock(),
|
||||
"turnstone.core.storage._registry._storage",
|
||||
new=MagicMock(),
|
||||
),
|
||||
patch.object(bridge, "_api_approve") as mock_approve,
|
||||
patch.object(bridge, "_publish_ws"),
|
||||
@@ -162,8 +162,8 @@ class TestBridgePolicyEnforcement:
|
||||
return_value={"bash": "deny", "read_file": "allow"},
|
||||
),
|
||||
patch(
|
||||
"turnstone.core.storage._registry.get_storage",
|
||||
return_value=MagicMock(),
|
||||
"turnstone.core.storage._registry._storage",
|
||||
new=MagicMock(),
|
||||
),
|
||||
patch.object(bridge, "_api_approve") as mock_approve,
|
||||
patch.object(bridge, "_publish_ws"),
|
||||
|
||||
+3
-3
@@ -143,13 +143,13 @@ class TerminalUI(SessionUI):
|
||||
|
||||
storage = get_storage()
|
||||
if storage is not None:
|
||||
tool_names = [
|
||||
_policy_names = [
|
||||
it.get("approval_label", "") or it.get("func_name", "")
|
||||
for it in pending
|
||||
if it.get("func_name")
|
||||
]
|
||||
if tool_names:
|
||||
verdicts = evaluate_tool_policies_batch(storage, tool_names)
|
||||
if _policy_names:
|
||||
verdicts = evaluate_tool_policies_batch(storage, _policy_names)
|
||||
for it in pending:
|
||||
policy_name = it.get("approval_label", "") or it.get("func_name", "")
|
||||
verdict = verdicts.get(policy_name)
|
||||
|
||||
+12
-8
@@ -48,13 +48,15 @@ _SAFE_NAMES: frozenset[str] = frozenset(
|
||||
# Prefixes that are always preserved (locale, XDG, etc.).
|
||||
_SAFE_PREFIXES: tuple[str, ...] = ("LC_", "XDG_")
|
||||
|
||||
# Substrings that cause a variable to be scrubbed.
|
||||
_SECRET_SUBSTRINGS: tuple[str, ...] = (
|
||||
"KEY",
|
||||
"SECRET",
|
||||
"TOKEN",
|
||||
"PASSWORD",
|
||||
"CREDENTIAL",
|
||||
# Suffixes that cause a variable to be scrubbed (e.g. *_KEY, *_TOKEN).
|
||||
# Suffix matching avoids false positives on MONKEYTYPE, KEYBOARD_LAYOUT, etc.
|
||||
_SECRET_SUFFIXES: tuple[str, ...] = (
|
||||
"_KEY",
|
||||
"_SECRET",
|
||||
"_TOKEN",
|
||||
"_PASSWORD",
|
||||
"_CREDENTIAL",
|
||||
"_CREDENTIALS",
|
||||
)
|
||||
|
||||
# Exact names that are always scrubbed (even if they don't match patterns).
|
||||
@@ -72,6 +74,8 @@ _EXPLICIT_SCRUB: frozenset[str] = frozenset(
|
||||
"AZURE_CLIENT_SECRET",
|
||||
"GCP_SERVICE_ACCOUNT_KEY",
|
||||
"GOOGLE_APPLICATION_CREDENTIALS",
|
||||
"DATABASE_URL",
|
||||
"TURNSTONE_DB_URL",
|
||||
}
|
||||
)
|
||||
|
||||
@@ -81,7 +85,7 @@ def _is_secret(name: str) -> bool:
|
||||
if name in _EXPLICIT_SCRUB:
|
||||
return True
|
||||
upper = name.upper()
|
||||
return any(sub in upper for sub in _SECRET_SUBSTRINGS)
|
||||
return any(upper.endswith(sfx) for sfx in _SECRET_SUFFIXES)
|
||||
|
||||
|
||||
def _is_safe(name: str) -> bool:
|
||||
|
||||
@@ -713,9 +713,11 @@ class Bridge:
|
||||
if tool_names:
|
||||
try:
|
||||
from turnstone.core.policy import evaluate_tool_policies_batch
|
||||
from turnstone.core.storage._registry import get_storage
|
||||
from turnstone.core.storage import _registry as _stor_reg
|
||||
|
||||
storage = get_storage()
|
||||
# Use _storage directly — get_storage() auto-initializes
|
||||
# a local SQLite DB which would have no admin policies.
|
||||
storage = _stor_reg._storage
|
||||
if storage is not None:
|
||||
verdicts = evaluate_tool_policies_batch(storage, list(tool_names))
|
||||
if any(v == "deny" for v in verdicts.values()):
|
||||
|
||||
Reference in New Issue
Block a user