OpenClaw: fail-closed auth + lazy key resolution + skill_store TRUNCATE - #947
Open
ziomik wants to merge 6 commits into
Open
OpenClaw: fail-closed auth + lazy key resolution + skill_store TRUNCATE#947ziomik wants to merge 6 commits into
ziomik wants to merge 6 commits into
Conversation
In stdio MCP mode (PINKY_SHARED_MCP unset, the default in prod), the per-agent pinky-memory subprocess env never included OPENAI_API_KEY. embeddings.py only resolves the key from an explicit arg or os.environ, and pinky_memory/__main__.py calls build_embedding_client() without one, so every agent silently fell back to the NoOp embedder and reflect() always returned embedded: false, even with a valid key saved via set_setting(). Read the key the same way PINKY_AGENT_KEY already is, just above, and apply stdio_env (which already carried PINKY_AGENT_KEY/PINKY_AGENTS_DB for pinky-self/pinky-messaging) to the pinky-memory server config too, since it was being built before stdio_env existed. Verified live: reflect() went from embedded: false to embedded: true after this change + daemon restart. (cherry picked from commit 8eee87a53d6defaa8f0ea72aec361104bbfebe25)
Test test_stdio_mode_injects_per_agent_key was failing with 'TypeError: Object of type MagicMock is not JSON serializable' because agent_registry.get_setting() returned a MagicMock in tests, which was truthy and got directly serialized into mcp_config. Add isinstance(openai_key, str) guard before assigning to stdio_env, matching the pattern already used for db_path (line 1283). This prevents non-string values (test mocks, DB corruption, etc.) from reaching json.dumps(mcp_config), which would fail .mcp.json generation and leave the agent without MCP on restart. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> (cherry picked from commit 2a59bbcc5ab9be1e20bc0a1f9e98ba95ea76d764)
This 1627-line module has been running in production while completely
untracked by git — not ignored either, just never added. Consequences:
it has never passed through CI or review, it is unrecoverable if deleted
or overwritten, and no fix to it can be proposed as a reviewable diff.
Committing it as a baseline so subsequent changes (notably the WS
handshake auth work) are reviewable. No functional change to the
protocol handling, session registry, chat routing or voice pipeline.
One change was required before this file could safely enter git history:
line 66 carried a live Deepgram API key as the hardcoded fallback of
os.getenv("DEEPGRAM_API_KEY", "<key>"). It has been moved to
~/.pinkybot/.env (gitignored) and the fallback is now "". The key never
reached a remote, since the file was never committed, so this is a
cleanup rather than a leak response — rotation is optional.
Verified the key still resolves: __main__._load_dotenv() runs at line 85,
before "from pinky_daemon.api import create_api" at line 124, and the
daemon's cwd is /home/pinky/.pinkybot, so the .env value is in os.environ
by the time this module is imported. Confirmed by replaying _load_dotenv()
against the real .env — variable present, same length as the removed
literal. Takes effect on the next daemon restart; the running process
keeps the old value in memory, so voice is uninterrupted either way.
Known issue NOT addressed here, deliberately, to keep this diff a pure
baseline: _auth_ok() at line 706 returns (True, "") when
OPENCLAW_GATEWAY_TOKEN is unset, so the WS accepts unauthenticated
clients by default. Tracked separately.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 40ef9ce797846a50c477223077b5990b87ce9e18)
The module read DEEPGRAM_API_KEY once at import time and trusted that __main__._load_dotenv() had already populated os.environ. Not every restart path guarantees that ordering, and an empty constant made _stt_deepgram raise "DEEPGRAM_API_KEY not configured" for the whole process lifetime — voice transcription dead until the next restart. Extract _load_env_key(name), the generic form of the existing _load_openai_key(), and use it for both keys. _stt_deepgram now resolves at call time (`DEEPGRAM_API_KEY or _load_deepgram_key()`), mirroring what _tts_openai already did. No hardcoded key is reintroduced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit bf218bc684301bafe8620739a611c4bcc4e683f4)
conversations_skills.db was the last daemon DB still on WAL. When its -wal/-shm sidecars are unlinked while the daemon holds the file open, committed writes (skill assignments) become invisible to every later reader while the daemon keeps reporting success. conversations_agents.db already avoids this via TRUNCATE (bradbrok#797/bradbrok#220); this applies the same journal mode here, converting an existing WAL in place before table init. Fails loud: if the connection cannot be confirmed in truncate mode after bounded retries, _configure_skills_db_connection raises SkillDbConfigError rather than silently running on WAL. Adds tests/test_skills_db_no_wal.py (5 tests) covering the journal mode after open, migration of an existing WAL database, and absence of the sidecars. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit a32f841f110141b43279e28207cc2b68ffe0842d)
The gateway relays device methods (camera, SMS, location, contacts, callLog) to the phone, so an unset OPENCLAW_GATEWAY_TOKEN must not mean "anyone who completes the handshake gets in". _auth_ok() now refuses the connect when no token is configured; the owner can opt out explicitly with OPENCLAW_GATEWAY_ALLOW_ANON=1. The token is resolved with _load_env_key(), so a restart that skips _load_dotenv() falls back to ~/.pinkybot/.env instead of silently dropping enforcement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 88269fc70e8eba573f19f26288e9a29b57b59c1a)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three fixes found while running the OpenClaw gateway on a production deployment, plus the baseline commit that puts the gateway under version control.
openclaw_gateway.pywas untrackedsrc/pinky_daemon/openclaw_gateway.pydid not exist in git — it was living untracked on the deployment. The first commit here (Track openclaw_gateway.py in git (baseline)) adds the file as-is so the two behavioural fixes on top of it are reviewable as diffs rather than as a 1600-line blob. It is a baseline, not a change (the only edit it carries is moving the Deepgram key out of the source and into.env)._auth_okis now fail-closedThe gateway relays device methods to the phone — camera, SMS, location, contacts, call log. Previously an unset
OPENCLAW_GATEWAY_TOKENmeant "permissive": anyone completing the handshake got in. That is the wrong default for that capability set._auth_ok()now refuses the connect when no token is configured. The owner can opt out explicitly withOPENCLAW_GATEWAY_ALLOW_ANON=1. The token is resolved through_load_env_key(), so a restart that skips_load_dotenv()falls back to~/.pinkybot/.envinstead of silently dropping enforcement.Covered by
tests/test_openclaw_auth.py.Lazy key resolution with
.envfallbackDeepgram and OpenAI keys were read at import time from
os.environ. When the daemon restarted without_load_dotenv()having run, the keys resolved to empty and the failure surfaced far from its cause. They are now resolved lazily at call time with a.envfallback.Covered by
tests/test_openclaw_key_resolution.py.skill_store:journal_mode=TRUNCATEconversations_skills.dbwas running in WAL. Under WAL, a-walfile unlinked out from under live connections (which is what happened in #797/#220 — other stores were moved off WAL and survived, this one was not and lost every skill assignment) leaves committed writes invisible to later readers while the daemon keeps reporting success. Rollback mode keeps committed data in the main DB file._configure_skills_db_connection()runs before table init, so an existing WAL DB is converted in place, and it fails loud —SkillDbConfigErrorrather than silently continuing on WAL. It is applied per thread-local connection, following the thread-local store refactor in #937. Thejournal_modeassertion intest_skill_store.py::test_point_read_hammer_uses_thread_local_connectionsis updated fromwaltotruncateaccordingly.Covered by
tests/test_skills_db_no_wal.py.Testing
pytest tests/test_openclaw_auth.py tests/test_openclaw_key_resolution.py tests/test_skills_db_no_wal.py tests/test_skill_store.py— 60 passed.ruff checkclean on all touched files.🤖 Opened by Engineer