security(cron): block base_url overrides that exfiltrate provider credentials (salvage #52351) - #207
Conversation
|
Review Complete Files Reviewed: 5 By Severity:
Critical import errors break the entire cron scheduler and Telegram delivery path; a provider-alias validation bug in the cron tool rejects legitimate BYOK providers. Files Reviewed (5 files) |
There was a problem hiding this comment.
Risk: 🔴 Critical (85/100) — 1 critical finding, 1 high, 1 medium · 438 LOC across 5 files
Critical Findings
PR #207 introduces changes to the cron scheduler, cronjob tools, and the release script. Two import errors will prevent the cron module from loading entirely:
claim_dispatch missing from cron/jobs.py (cron/scheduler.py:240)
The import from cron.jobs import claim_dispatch references a function that does not exist anywhere in the codebase. Since this is a module-level import, ImportError at load time breaks all cron functionality — no jobs fire, no tick runs, the scheduler cannot be imported.
looks_like_telegram_private_chat_id name mismatch (cron/scheduler.py:1249)
The scheduler imports the public name looks_like_telegram_private_chat_id from gateway.delivery, but the function is defined as _looks_like_telegram_private_chat_id (private, with underscore prefix). This causes ImportError when the Telegram delivery path executes, breaking cron-to-Telegram private-DM delivery.
Medium Finding
BYOK provider alias rejection (tools/cronjob_tools.py:487)
The cron base_url validator only checks for the literal string "custom" but does not resolve provider aliases (ollama, vllm, llamacpp) that canonically map to custom. Users with cron jobs using these aliases and a local base_url are incorrectly rejected. A workaround exists (use provider='custom'), but the code's own comment promises alias resolution that isn't implemented.
Review Coverage
- 5 files reviewed across correctness, security, and domain lenses (cron-scheduling, llm-provider-routing, credential-exfiltration)
- 4 scanners + sweep + holistic cross-validated findings independently
| } | ||
|
|
||
| from cron.jobs import get_due_jobs, mark_job_run, save_job_output, advance_next_run | ||
| from cron.jobs import get_due_jobs, mark_job_run, save_job_output, advance_next_run, claim_dispatch |
There was a problem hiding this comment.
🔴 claim_dispatch imported from cron.jobs but never defined — breaks entire cron module (bug)
cron/scheduler.py line 240 adds the module-level import from cron.jobs import get_due_jobs, mark_job_run, save_job_output, advance_next_run, claim_dispatch. The function claim_dispatch is called in run_one_job() at line 2844 to implement one-shot dispatch deduplication (NousResearch#38758). However, no function named claim_dispatch exists anywhere in cron/jobs.py or the entire codebase. The import will raise ImportError at module load time, breaking the entire cron scheduler.
Impact: All cron functionality is dead — no jobs will fire, no tick will run, the scheduler module cannot be imported at all.
💡 Suggestion: Add a claim_dispatch(job_id: str) -> bool function to cron/jobs.py that atomically checks and increments a dispatch counter for finite one-shot jobs, returning False when the dispatch limit is reached (so run_one_job can skip). The import and call sites in scheduler.py are correct; only the function definition is missing.
📋 Prompt for AI Agents
In cron/jobs.py, add a function claim_dispatch(job_id: str) -> bool that: (1) acquires the jobs file lock, (2) loads the job from the store, (3) for recurring jobs (schedule.kind in {cron, interval}) returns True immediately, (4) for infinite repeat (repeat.times is None) returns True, (5) for finite one-shots, checks repeat.completed < repeat.times: if so, increments repeat.completed, saves, returns True; otherwise returns False. This implements the at-most-N semantics for one-shot dispatch deduplication described in the run_one_job comment (issue NousResearch#38758).
| DeliveryTarget, | ||
| _looks_like_int, | ||
| _looks_like_telegram_private_chat_id, | ||
| looks_like_telegram_private_chat_id, |
There was a problem hiding this comment.
🟠 Import name mismatch: looks_like_telegram_private_chat_id not exported by gateway/delivery.py (bug)
cron/scheduler.py line 1249 imports looks_like_telegram_private_chat_id (public name, no underscore prefix) from gateway.delivery, and calls it at line 1255. However, gateway/delivery.py line 62 defines the function as _looks_like_telegram_private_chat_id (private, with underscore prefix). The public name is not exported anywhere in delivery.py. This causes ImportError when _deliver_result() executes on Telegram platform paths, breaking cron-to-Telegram private-DM delivery routes.
Impact: Cron jobs delivering results to Telegram private DM topics will fail to deliver, and the ImportError will cause the delivery path to raise an exception.
💡 Suggestion: Rename the function in gateway/delivery.py from _looks_like_telegram_private_chat_id to looks_like_telegram_private_chat_id (dropping the underscore prefix) at line 62, and update the two internal call sites at lines 470 and 493 to use the new public name.
| looks_like_telegram_private_chat_id, | |
| def looks_like_telegram_private_chat_id(chat_id: Optional[str]) -> bool: |
📋 Prompt for AI Agents
In gateway/delivery.py line 62, rename def _looks_like_telegram_private_chat_id to def looks_like_telegram_private_chat_id (drop the underscore prefix). Then update the two internal references at lines 470 and 493 in the same file from _looks_like_telegram_private_chat_id to looks_like_telegram_private_chat_id. No changes needed in cron/scheduler.py — it already imports and calls the public name correctly at line 1249.
| if prov.lower() == "custom": | ||
| # Bare/inline 'custom' (and aliases that resolve to it) is pure BYOK: the | ||
| # runtime derives the key from a pool keyed by THIS base_url or from | ||
| # host-gated env vars, never an arbitrary stored secret. Safe to allow. | ||
| return None |
There was a problem hiding this comment.
🟡 Cron base_url validator rejects BYOK provider aliases (ollama/vllm/llamacpp) that should be treated as 'custom' (bug)
The _validate_cron_base_url function in tools/cronjob_tools.py at line 487 checks if prov.lower() == 'custom' to allow base_url overrides for bare BYOK providers. The comment at line 488 says "and aliases that resolve to it" — but the code only checks for the literal string "custom", never resolving aliases.
Providers like ollama, vllm, llamacpp are aliases that resolve to custom (per hermes_cli/auth.py lines 1587-1589), yet the validator falls through past the bare-custom check (line 487), past the named-custom-provider check (line 492, which is also False for these), and into the PROVIDER_REGISTRY lookup (line 509-513). Since these aliases are not in PROVIDER_REGISTRY, known_host is empty and the fail-closed return at line 521 unconditionally rejects any base_url override.
Impact: Users with cron jobs using provider='ollama' (or vllm/llamacpp) and a custom base_url (e.g., http://localhost:11434/v1) get rejected with "base_url ... is not allowed for provider ...". The workaround is to use provider='custom' instead, but the validator's own comment promises alias resolution that isn't implemented.
💡 Suggestion: Before the bare-custom string comparison at line 487, resolve the provider through the alias system. If the canonical provider resolves to 'custom', return None (safe). This matches the pattern used in _resolve_named_custom_runtime.
📋 Prompt for AI Agents
In tools/cronjob_tools.py, in _validate_cron_base_url, add alias resolution before the if prov.lower() == 'custom' check at line 487. Insert after line 486 (the imports block) and before the literal 'custom' check:
# Resolve provider aliases (ollama/vllm/llamacpp -> custom) so the
# BYOK fast-path below catches them.
try:
from hermes_cli.auth import resolve_provider as _rp
canonical = _rp(prov)
if canonical == 'custom':
return None
except Exception:
pass
This preserves the existing fast-path for literal 'custom' while adding the alias resolution the comment already promises.
Summary
Salvage of NousResearch#52351 by @claudlos (rebased onto current
main+ AUTHOR_MAP entry). Blocks cron jobs from pairing a named provider's stored API credential with an attacker-controlledbase_url— a credential-exfiltration primitive reachable via the model-callable, prompt-injectablecronjobtool (CWE-200 / CWE-522).The original PR was 174 commits behind
main. Both of @claudlos's commits are cherry-picked here verbatim (authorship preserved); I added oneAUTHOR_MAPchore commit under my identity so thecheck-attributiongate resolves their plain email.The vulnerability
cronjob(action="create"|"update")accepts free-formprovider+base_url. On fire, the scheduler resolves the named provider's stored key and pairs it with the job'sbase_url. A prompt-injected job (provider=anthropic, base_url=https://attacker/v1) sends the real API key to the attacker's endpoint. Abase_urlwith no provider inherits the default provider's key for the same effect.Confirmed present on current
main: nobase_urlguard exists at the cron tool boundary.The fix (fail-closed, two layers)
tools/cronjob_tools.py::_validate_cron_base_url(provider, base_url)— runs on create AND update. Abase_urloverride is allowed only when it cannot leak a stored secret:custom(BYOK — key derived from the base_url/host-gated env, not a stored named secret);base_urlwith no explicit provider, and any name we can't host-match — is refused. Fail-closed on import/resolution error.provider/base_urlpair on every update (not only when the update touches those fields), so a job persisted before this guard can't be left exfil-capable by editing an unrelated field. An operator can remediate in one update by clearingbase_urlor repointing at a safe pair.cron/scheduler.py::_guard_job_credential_exfil(job)— re-validates the stored pair immediately beforeresolve_runtime_provider(), catching jobs persisted before the guard or written directly to the store. Fails closed: if the validator import/call raises, abase_url-bearing job is refused (a no-override job still runs). RaisesRuntimeError, caught byrun_job's failure handler → reported as a failed run, before any network call.Review (this salvage)
Ran our full review workflow — scope-integrity check, backstop error-handling trace, an independent bypass probe, and two adversarial review passes (security-bypass hunt + correctness/regression). All converged clean, zero findings. Key checks:
claude,google, …) aren't registry keys → fail-closed blocked at the guard even though the sink alias-expands them.base_url_hostname(stdliburlparse().hostname) defeats userinfo (x@evil.com), suffix (api.host.com.evil), path, case, trailing-dot, port tricks;base_url_host_matchesanchors subdomain checks on a"." + domainboundary so lookalikes (legit.example.attacker.test) are blocked. Subdomains of the configured host are intentionally allowed (still the provider's own domain) — tested.accept_suggestion→blueprint create path has nobase_urlfield so no override primitive; direct store writes are caught by the runtime backstop.Tests
Coverage includes: named-custom off-host blocked / matching-host allowed / lookalike blocked, bare-custom allowed, base_url-without-provider rejected, legacy-unsafe-job blocked on unrelated update + remediation paths, and the runtime backstop fail-closed-on-validator-error case.
Supersedes NousResearch#52351. Full credit to @claudlos for the fix.
Mirror-of: NousResearch#56196
NousResearch#56196