Skip to content

Commit 0310cc2

Browse files
WEB-4656: address review findings — MDM creds, secret leak, matcher allowlist
Three CRITICAL fixes from elite-pr-reviewer plus two WARNINGs. CRITICAL #1: MDM install now writes ~/.unbound/config.json per user. The hook scripts read this file at runtime via _common.py::load_credentials. Without it, every PreToolUse silently fail-opens and org policy is never enforced on managed devices. New _write_unbound_config_payload mirrors claude-code/hooks/mdm/setup.py's pattern: privilege-dropped fork, tmp + rename, O_NOFOLLOW + 0o600, dir 0o700. Called BEFORE the settings.json merge so a settings failure never strands a device with an entry-point hook pointing at missing creds. CRITICAL #2: API keys no longer leak via curl argv. Both notify_setup_complete (user + MDM) and fetch_api_key_from_mdm now use stdlib urllib.request so the Authorization / X-API-KEY header stays inside our process. Previously ps auxe / /proc/<pid>/cmdline exposed the secret to every other user on the device. The hook-script POST in scripts/_common.py is left as-is per review notes — it runs under the user's own UID so the leak is bounded by user permissions, not cross-user. CRITICAL #3: PreToolUse / PostToolUse matchers switched from the "Bash|bash|Write|Edit|Read|Glob|Grep|Task" allowlist to the catch-all "*". Antigravity 2.0 ships at minimum with WebFetch, WebSearch, MultiEdit, NotebookEdit, TodoWrite (none of which were in the allowlist) — every unfamiliar tool was silently bypassing the policy gate. Server-side filtering via the gateway's APP_NATIVE_FILE_TOOLS / tools_to_check is the correct defang point, not the matcher. WARNING fixes: - mdm/setup.py settings.json read now uses O_NOFOLLOW, consistent with the script-write loop above it. - scripts/_common.py drops the -L flag from the hook-POST curl. Gateway is one hop; following redirects masks misconfig instead of surfacing it. Tests: - antigravity/hooks/test_setup.py: matcher tests rewritten to assert catch-all "*" (per-event), regression assert no "|" allowlist, plus a new TestNotifySetupCompleteNoCurl class that puts a fake curl shim on PATH and asserts it's never invoked. - antigravity/hooks/mdm/test_setup.py: new file. Covers the credentials write (api_key present, 0o600 mode, 0o700 dir mode, preserves unrelated fields), the full install payload (config + scripts + settings all land), the ordering invariant (config written before settings), the catch-all matcher shape, and curl-shim assertions for both notify_setup_complete and fetch_api_key_from_mdm. Skipped per brief: - WARNING #4 (gateway URL string replace): speculative, "don't design for hypothetical futures." - WARNING #5 (--foo=bar form): matches claude-code convention. - WARNING #8 (operator log on hook failures): out of scope for v1. - INFO #9-12: cosmetic / matches claude-code / out of scope. Decisions made: - Catch-all matcher uses "*", not "" or omitted, to match claude-code's established settings.json convention (verified in tree). Both forms work per Antigravity docs; "*" is what every other tool's setup ships. - fetch_api_key_from_mdm was also migrated to urllib though the brief only explicitly named notify_setup_complete — same exact leak (bearer token on curl argv), same fix, same blast-radius file. - MDM --clear path does NOT remove ~/.unbound/config.json (matches the claude-code MDM clear behaviour; config is shared across tools so we don't own removal). Test commands run (all green): cd antigravity/hooks && python3 -m unittest test_setup.py -v # 13 ok cd antigravity/hooks/scripts && python3 -m unittest test_hooks.py -v # 14 ok cd claude-code/hooks && python3 -m unittest test_setup.py -v # 7 ok cd antigravity/hooks/mdm && python3 -m unittest test_setup.py -v # 10 ok Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 8542e68 commit 0310cc2

5 files changed

Lines changed: 584 additions & 54 deletions

File tree

antigravity/hooks/mdm/setup.py

Lines changed: 141 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@
2525
import shutil
2626
import subprocess
2727
import sys
28+
import urllib.error
29+
import urllib.request
2830
from pathlib import Path
2931
from typing import Dict, List, Optional, Tuple
3032

@@ -45,9 +47,14 @@
4547
("SessionStart", "unbound_session_start.py"),
4648
]
4749

50+
# Catch-all matchers for tool-scoped events. A regex allowlist here silently
51+
# bypasses our hook for any tool not in the list (WebFetch, WebSearch,
52+
# MultiEdit, NotebookEdit, TodoWrite, future tools...). Server-side filtering
53+
# (gateway's APP_NATIVE_FILE_TOOLS / tools_to_check) is the right gate; the
54+
# matcher is the wrong place to defang.
4855
HOOK_EVENT_MATCHERS: Dict[str, Optional[str]] = {
49-
"PreToolUse": "Bash|bash|Write|Edit|Read|Glob|Grep|Task",
50-
"PostToolUse": "Bash|bash|Write|Edit|Read|Glob|Grep|Task",
56+
"PreToolUse": "*",
57+
"PostToolUse": "*",
5158
"UserPromptSubmit": None,
5259
"SessionStart": None,
5360
}
@@ -281,24 +288,39 @@ def get_device_identifier() -> Optional[str]:
281288
def fetch_api_key_from_mdm(
282289
base_url: str, app_name: Optional[str], auth_api_key: str, device_id: str
283290
) -> Optional[str]:
291+
# Use stdlib urllib instead of shelling out to curl: passing the bearer
292+
# token via curl's argv leaks it through ``ps auxe`` /
293+
# ``/proc/<pid>/cmdline`` to any other user on the device. urllib sets the
294+
# header inside our own process — argv stays secret-free.
284295
params = f"serial_number={device_id}&app_type={UNBOUND_APP_LABEL}"
285296
if app_name:
286297
params = f"app_name={app_name}&{params}"
287298
url = f"{base_url.rstrip('/')}/api/v1/automations/mdm/get_application_api_key/?{params}"
288299
debug_print(f"Fetching API key from: {url}")
289300
try:
290-
result = subprocess.run(
291-
["curl", "-fsSL", "-w", "\n%{http_code}",
292-
"-H", f"Authorization: Bearer {auth_api_key}", url],
293-
capture_output=True, text=True, timeout=30,
301+
req = urllib.request.Request(
302+
url,
303+
method="GET",
304+
headers={"Authorization": f"Bearer {auth_api_key}"},
294305
)
295-
output_lines = result.stdout.strip().split("\n")
296-
if len(output_lines) < 2:
297-
print("Invalid response from server")
306+
try:
307+
with urllib.request.urlopen(req, timeout=30) as resp:
308+
http_code = resp.getcode()
309+
body = resp.read().decode("utf-8")
310+
except urllib.error.HTTPError as e:
311+
try:
312+
err_body = e.read().decode("utf-8", errors="replace")
313+
except Exception:
314+
err_body = ""
315+
debug_print(f"HTTP {e.code} body: {err_body[:200]}")
316+
print(f"API request failed with status {e.code}")
317+
return None
318+
except urllib.error.URLError as e:
319+
debug_print(f"URL error: {e}")
320+
print("Failed to reach backend")
298321
return None
299-
http_code = output_lines[-1]
300-
body = "\n".join(output_lines[:-1])
301-
if http_code != "200":
322+
323+
if http_code != 200:
302324
print(f"API request failed with status {http_code}")
303325
return None
304326
data = json.loads(body)
@@ -307,9 +329,6 @@ def fetch_api_key_from_mdm(
307329
print("No api_key in response")
308330
return None
309331
return api_key
310-
except subprocess.TimeoutExpired:
311-
print("Request timed out")
312-
return None
313332
except (json.JSONDecodeError, ValueError):
314333
print("Invalid JSON response from server")
315334
return None
@@ -407,10 +426,81 @@ def _atomic_write_json(path: Path, data: Dict) -> None:
407426
os.replace(tmp, path)
408427

409428

410-
def install_for_user_payload(home_dir: Path, gateway_url: str, script_templates: Dict[str, bytes]) -> bool:
429+
def _write_unbound_config_payload(home_dir: Path, api_key: str, gateway_url: str, backend_url: str) -> bool:
430+
"""Body of the per-user ~/.unbound/config.json write. Runs inside the
431+
privilege-dropped fork so attacker-planted symlinks in $HOME can't
432+
redirect a root write. Mirrors claude-code/hooks/mdm/setup.py's
433+
write_unbound_config_for_user pattern: tighten perms, atomic write,
434+
O_NOFOLLOW + 0o600.
435+
436+
This is what the runtime hook scripts read in
437+
``scripts/_common.py::load_credentials`` to authenticate to the gateway —
438+
without it every PreToolUse silently fail-opens and the org policy is
439+
never enforced."""
440+
try:
441+
config_dir = home_dir / ".unbound"
442+
config_file = config_dir / "config.json"
443+
config_dir.mkdir(mode=0o700, parents=True, exist_ok=True)
444+
if platform.system().lower() != "windows":
445+
try:
446+
os.chmod(config_dir, 0o700)
447+
except OSError:
448+
pass
449+
450+
# Preserve unrelated fields a previous tool's setup may have written
451+
# (e.g. claude-code writes the same file). Only api_key/gateway_url/
452+
# backend_url are owned by us.
453+
config: Dict = {}
454+
if config_file.exists():
455+
try:
456+
with open(config_file, "r", encoding="utf-8") as f:
457+
config = json.loads(f.read())
458+
if not isinstance(config, dict):
459+
config = {}
460+
except (json.JSONDecodeError, OSError):
461+
config = {}
462+
463+
config["api_key"] = api_key
464+
if gateway_url:
465+
config["gateway_url"] = gateway_url
466+
if backend_url:
467+
config["backend_url"] = backend_url
468+
469+
# Atomic write via tmp + rename. O_NOFOLLOW on both the tmp open AND
470+
# the rename target so a symlink at either path is refused. 0o600 so
471+
# only the target user can read the secret.
472+
flags = os.O_WRONLY | os.O_CREAT | os.O_TRUNC | getattr(os, "O_NOFOLLOW", 0)
473+
tmp = config_file.with_suffix(config_file.suffix + ".tmp")
474+
if tmp.exists():
475+
try:
476+
tmp.unlink()
477+
except OSError:
478+
pass
479+
fd = os.open(str(tmp), flags, 0o600)
480+
with os.fdopen(fd, "w", encoding="utf-8") as f:
481+
f.write(json.dumps(config, indent=2))
482+
os.replace(str(tmp), str(config_file))
483+
# os.replace preserves the source's mode, but be defensive in case the
484+
# rename target pre-existed with a wider mode and somehow survives.
485+
if platform.system().lower() != "windows":
486+
try:
487+
os.chmod(config_file, 0o600)
488+
except OSError:
489+
pass
490+
return True
491+
except Exception as e:
492+
debug_print(f"per-user config write failed in {home_dir}: {e}")
493+
return False
494+
495+
496+
def install_for_user_payload(home_dir: Path, gateway_url: str, backend_url: str, api_key: str, script_templates: Dict[str, bytes]) -> bool:
411497
"""Body of the per-user install. Runs inside the privilege-dropped fork.
412498
All arguments are pickled across the fork boundary, so script bytes are
413-
passed by value (we already read them as root before dropping)."""
499+
passed by value (we already read them as root before dropping).
500+
501+
Order matters: write ~/.unbound/config.json BEFORE settings.json so a
502+
settings.json failure never leaves the user with an entry-point hook
503+
that can't authenticate (which would fail-open every call)."""
414504
try:
415505
antigravity_dir = home_dir / ".antigravity"
416506
hooks_dir = antigravity_dir / "hooks"
@@ -419,6 +509,12 @@ def install_for_user_payload(home_dir: Path, gateway_url: str, script_templates:
419509
antigravity_dir.mkdir(parents=True, exist_ok=True)
420510
hooks_dir.mkdir(parents=True, exist_ok=True)
421511

512+
# 0. Write the credentials file FIRST. Hook scripts read this at
513+
# runtime via _common.py::load_credentials; without it the PreToolUse
514+
# gate silently fail-opens on every call.
515+
if not _write_unbound_config_payload(home_dir, api_key, gateway_url, backend_url):
516+
return False
517+
422518
# 1. Write the shared helper, with the gateway URL baked in.
423519
common_bytes = script_templates["_common.py"]
424520
common_text = common_bytes.decode("utf-8")
@@ -444,7 +540,13 @@ def install_for_user_payload(home_dir: Path, gateway_url: str, script_templates:
444540
# 3. Non-destructive settings.json merge.
445541
if settings_path.exists():
446542
try:
447-
with open(settings_path, "r", encoding="utf-8") as f:
543+
# O_NOFOLLOW so an attacker-planted symlink at this path can't
544+
# redirect our read (and the subsequent write) to a file outside
545+
# the user's home. Matches the write side and the script-write
546+
# loop above.
547+
read_flags = os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0)
548+
fd = os.open(str(settings_path), read_flags)
549+
with os.fdopen(fd, "r", encoding="utf-8") as f:
448550
settings = json.load(f)
449551
if not isinstance(settings, dict):
450552
return False
@@ -616,20 +718,28 @@ def remove_policy_marker() -> None:
616718

617719

618720
def notify_setup_complete(api_key: str, backend_url: str, device_id: Optional[str]) -> None:
721+
# Use stdlib urllib so the API key never appears on the curl argv —
722+
# see fetch_api_key_from_mdm above for the same reason.
619723
try:
620724
url = f"{backend_url.rstrip('/')}/api/v1/setup/complete/"
621-
body = {"tool_type": UNBOUND_APP_LABEL}
725+
body: Dict = {"tool_type": UNBOUND_APP_LABEL}
622726
if device_id:
623727
body["serial_number"] = device_id
624-
data = json.dumps(body)
625-
subprocess.run(
626-
["curl", "-fsSL", "-X", "POST",
627-
"-H", f"X-API-KEY: {api_key}",
628-
"-H", "Content-Type: application/json",
629-
"--data-binary", "@-", url],
630-
input=data.encode(),
631-
capture_output=True, timeout=10,
728+
data = json.dumps(body).encode("utf-8")
729+
req = urllib.request.Request(
730+
url,
731+
data=data,
732+
method="POST",
733+
headers={
734+
"X-API-KEY": api_key,
735+
"Content-Type": "application/json",
736+
},
632737
)
738+
try:
739+
with urllib.request.urlopen(req, timeout=10):
740+
pass
741+
except urllib.error.HTTPError as e:
742+
debug_print(f"Setup-complete returned HTTP {e.code}")
633743
debug_print("Setup completion notification sent")
634744
except Exception as e:
635745
debug_print(f"Could not notify backend: {e}")
@@ -659,7 +769,10 @@ def run_install(api_key: str, gateway_url: str, backend_url: str, device_id: Opt
659769

660770
success_count = 0
661771
for username, home_dir in user_homes:
662-
ok = _run_as_user(username, install_for_user_payload, home_dir, gateway_url, templates)
772+
ok = _run_as_user(
773+
username, install_for_user_payload,
774+
home_dir, gateway_url, backend_url, api_key, templates,
775+
)
663776
if ok:
664777
success_count += 1
665778
debug_print(f"Installed for {username}")

0 commit comments

Comments
 (0)