fix(keychain): single source of truth for key list + robust USER fallback
Addresses Greptile review on PR #407: - P1: setup-keychain.sh ALL_KEYS was missing GOOGLE_GENAI_API_KEY and XIAOHONGSHU_API_BASE relative to _load_keychain's inline list, so users manually storing those keys would not see them in --list and the interactive prompt would never offer to set them. Hoist the canonical key list into lib/env.py::KEYCHAIN_KEYS, have get_config() pass it through, and add a parity test that parses ALL_KEYS out of setup-keychain.sh and asserts equality. Drift is now caught at CI time instead of after a user reports a missing key. - P2: os.environ.get("USER", "") silently returned "" under sudo, in Docker without --env USER, or in CI runners that strip USER. The resulting `security find-generic-password -a ""` call would never match items stored by setup-keychain.sh, so all lookups silently returned nothing. Fall back to pwd.getpwuid(os.getuid()).pw_name when USER is absent. The P2 process-listing comment ("secret visible briefly via ps because security has no stdin path for -w") has no clean fix — the README already documents the manual `security add-generic-password` invocation as an alternative for users with strict secret hygiene.
This commit is contained in:
@@ -34,6 +34,18 @@ CODEX_AUTH_FILE = Path(os.environ.get("CODEX_AUTH_FILE", str(Path.home() / ".cod
|
|||||||
# Example: `security add-generic-password -a "$USER" -s last30days-XAI_API_KEY -w "xai-..."`.
|
# Example: `security add-generic-password -a "$USER" -s last30days-XAI_API_KEY -w "xai-..."`.
|
||||||
KEYCHAIN_SERVICE_PREFIX = "last30days-"
|
KEYCHAIN_SERVICE_PREFIX = "last30days-"
|
||||||
|
|
||||||
|
# Single source of truth for which credentials the Keychain loader looks up.
|
||||||
|
# The setup-keychain.sh helper mirrors this list and is held in sync via
|
||||||
|
# tests/test_env_keychain.py::test_keychain_keys_match_setup_script.
|
||||||
|
KEYCHAIN_KEYS = (
|
||||||
|
"OPENAI_API_KEY", "XAI_API_KEY", "GOOGLE_API_KEY", "GEMINI_API_KEY",
|
||||||
|
"GOOGLE_GENAI_API_KEY", "SCRAPECREATORS_API_KEY", "APIFY_API_TOKEN",
|
||||||
|
"AUTH_TOKEN", "CT0", "BSKY_HANDLE", "BSKY_APP_PASSWORD",
|
||||||
|
"TRUTHSOCIAL_TOKEN", "BRAVE_API_KEY", "EXA_API_KEY", "SERPER_API_KEY",
|
||||||
|
"OPENROUTER_API_KEY", "PARALLEL_API_KEY", "XQUIK_API_KEY",
|
||||||
|
"XIAOHONGSHU_API_BASE",
|
||||||
|
)
|
||||||
|
|
||||||
AuthSource = Literal["api_key", "codex", "none"]
|
AuthSource = Literal["api_key", "codex", "none"]
|
||||||
AuthStatus = Literal["ok", "missing", "expired", "missing_account_id"]
|
AuthStatus = Literal["ok", "missing", "expired", "missing_account_id"]
|
||||||
|
|
||||||
@@ -114,7 +126,11 @@ def _load_keychain(keys: list[str]) -> dict[str, str]:
|
|||||||
return {}
|
return {}
|
||||||
|
|
||||||
import subprocess
|
import subprocess
|
||||||
user = os.environ.get("USER", "")
|
import pwd
|
||||||
|
# USER can be unset under sudo, in Docker without --env USER, or in some CI
|
||||||
|
# runners; fall back to the OS user record so lookups still match items
|
||||||
|
# stored by setup-keychain.sh (which uses $USER).
|
||||||
|
user = os.environ.get("USER") or pwd.getpwuid(os.getuid()).pw_name
|
||||||
env: dict[str, str] = {}
|
env: dict[str, str] = {}
|
||||||
for key in keys:
|
for key in keys:
|
||||||
try:
|
try:
|
||||||
@@ -269,14 +285,7 @@ def get_config() -> dict[str, Any]:
|
|||||||
|
|
||||||
# Keychain is the lowest-priority source (Darwin only; no-op elsewhere).
|
# Keychain is the lowest-priority source (Darwin only; no-op elsewhere).
|
||||||
# Loaded before openai_auth so OPENAI_API_KEY can come from Keychain too.
|
# Loaded before openai_auth so OPENAI_API_KEY can come from Keychain too.
|
||||||
keychain_env = _load_keychain([
|
keychain_env = _load_keychain(list(KEYCHAIN_KEYS))
|
||||||
'OPENAI_API_KEY', 'XAI_API_KEY', 'GOOGLE_API_KEY', 'GEMINI_API_KEY',
|
|
||||||
'GOOGLE_GENAI_API_KEY', 'SCRAPECREATORS_API_KEY', 'APIFY_API_TOKEN',
|
|
||||||
'AUTH_TOKEN', 'CT0', 'BSKY_HANDLE', 'BSKY_APP_PASSWORD',
|
|
||||||
'TRUTHSOCIAL_TOKEN', 'BRAVE_API_KEY', 'EXA_API_KEY', 'SERPER_API_KEY',
|
|
||||||
'OPENROUTER_API_KEY', 'PARALLEL_API_KEY', 'XQUIK_API_KEY',
|
|
||||||
'XIAOHONGSHU_API_BASE',
|
|
||||||
])
|
|
||||||
merged_env = {**keychain_env, **merged_env}
|
merged_env = {**keychain_env, **merged_env}
|
||||||
|
|
||||||
openai_auth = get_openai_auth(merged_env)
|
openai_auth = get_openai_auth(merged_env)
|
||||||
|
|||||||
@@ -17,11 +17,14 @@
|
|||||||
set -euo pipefail
|
set -euo pipefail
|
||||||
|
|
||||||
PREFIX="last30days-"
|
PREFIX="last30days-"
|
||||||
|
# Mirrors lib/env.py::KEYCHAIN_KEYS — kept in sync via
|
||||||
|
# tests/test_env_keychain.py::test_keychain_keys_match_setup_script.
|
||||||
ALL_KEYS=(
|
ALL_KEYS=(
|
||||||
OPENAI_API_KEY
|
OPENAI_API_KEY
|
||||||
XAI_API_KEY
|
XAI_API_KEY
|
||||||
GOOGLE_API_KEY
|
GOOGLE_API_KEY
|
||||||
GEMINI_API_KEY
|
GEMINI_API_KEY
|
||||||
|
GOOGLE_GENAI_API_KEY
|
||||||
SCRAPECREATORS_API_KEY
|
SCRAPECREATORS_API_KEY
|
||||||
APIFY_API_TOKEN
|
APIFY_API_TOKEN
|
||||||
AUTH_TOKEN
|
AUTH_TOKEN
|
||||||
@@ -35,6 +38,7 @@ ALL_KEYS=(
|
|||||||
OPENROUTER_API_KEY
|
OPENROUTER_API_KEY
|
||||||
PARALLEL_API_KEY
|
PARALLEL_API_KEY
|
||||||
XQUIK_API_KEY
|
XQUIK_API_KEY
|
||||||
|
XIAOHONGSHU_API_BASE
|
||||||
)
|
)
|
||||||
|
|
||||||
if [[ "${OSTYPE:-}" != darwin* ]]; then
|
if [[ "${OSTYPE:-}" != darwin* ]]; then
|
||||||
|
|||||||
@@ -10,6 +10,7 @@ Covers:
|
|||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import re
|
||||||
import subprocess
|
import subprocess
|
||||||
import sys
|
import sys
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
@@ -21,6 +22,8 @@ sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "skills" / "last30d
|
|||||||
|
|
||||||
from lib import env # noqa: E402
|
from lib import env # noqa: E402
|
||||||
|
|
||||||
|
SETUP_KEYCHAIN_SH = Path(__file__).resolve().parents[1] / "skills" / "last30days" / "scripts" / "setup-keychain.sh"
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
# _load_keychain unit tests
|
# _load_keychain unit tests
|
||||||
@@ -150,3 +153,30 @@ def test_get_config_openai_key_can_come_from_keychain(clean_env):
|
|||||||
cfg = env.get_config()
|
cfg = env.get_config()
|
||||||
assert cfg["OPENAI_API_KEY"] == "sk-from-kc"
|
assert cfg["OPENAI_API_KEY"] == "sk-from-kc"
|
||||||
assert cfg["OPENAI_AUTH_SOURCE"] == "api_key"
|
assert cfg["OPENAI_AUTH_SOURCE"] == "api_key"
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# Drift guard: lib/env.py KEYCHAIN_KEYS and setup-keychain.sh ALL_KEYS must
|
||||||
|
# stay in lockstep. A mismatch means users storing a key via the helper script
|
||||||
|
# wouldn't see it picked up by the loader, or vice versa.
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
def _parse_all_keys_from_shell(script: Path) -> list[str]:
|
||||||
|
text = script.read_text(encoding="utf-8")
|
||||||
|
match = re.search(r"ALL_KEYS=\(\s*(.*?)\s*\)", text, re.DOTALL)
|
||||||
|
if not match:
|
||||||
|
raise AssertionError(f"ALL_KEYS=( ... ) array not found in {script}")
|
||||||
|
body = match.group(1)
|
||||||
|
# Strip shell comments and split on whitespace
|
||||||
|
body = re.sub(r"#[^\n]*", "", body)
|
||||||
|
return [tok for tok in body.split() if tok]
|
||||||
|
|
||||||
|
|
||||||
|
def test_keychain_keys_match_setup_script():
|
||||||
|
shell_keys = _parse_all_keys_from_shell(SETUP_KEYCHAIN_SH)
|
||||||
|
python_keys = list(env.KEYCHAIN_KEYS)
|
||||||
|
assert shell_keys == python_keys, (
|
||||||
|
"lib/env.py::KEYCHAIN_KEYS and scripts/setup-keychain.sh::ALL_KEYS "
|
||||||
|
f"have drifted.\n python: {python_keys}\n shell: {shell_keys}"
|
||||||
|
)
|
||||||
|
|||||||
Reference in New Issue
Block a user