Review — end-to-end ✅ (one R2 regression fixed)

What this ships

New "Providers" tab in Settings lets users add/update/remove API keys for the direct-API providers (Anthropic, OpenAI, Google, DeepSeek, xAI, Mistral, MiniMax, Z.AI, Kimi, Ollama, Ollama Cloud, OpenCode Zen/Go) without editing .env. OAuth providers (Copilot, Nous, OpenAI Codex) render as read-only. Closes #586.

Scope: +1041 lines, one new backend module (api/providers.py, 328 lines), one new test file (tests/test_provider_management.py, 16 tests), Settings-panel additions in panels.js / index.html / style.css, and 19 i18n keys × 6 locales.

Comes in as a release PR combining @bergeouss's original PR #867, @frap129's Docker openssh-client PR #868, and the B1/B2/R1/R2/XSS/i18n review-feedback commit. All the surface-level fixes from the cover letter landed. But the R2 fix wasn't quite what the commit message claims.

What I found — R2 was incomplete

The fix commit says:

R2: _ENV_LOCK now covers the full load/modify/write cycle in _write_env_file, not just os.environ mutations — prevents TOCTOU race between two concurrent POST /api/providers calls losing each other's key additions

But in the code as pushed, the with _ENV_LOCK: block ended before the file write:

with _ENV_LOCK:
    current = _load_env_file(env_path)
    for key, value in updates.items():
        ...
        current[key] = clean
        os.environ[key] = clean

env_path.parent.mkdir(parents=True, exist_ok=True)   # ← OUTSIDE lock
lines = [f"{key}={current[key]}" for key in sorted(current)]
env_path.write_text(...)                              # ← OUTSIDE lock
env_path.chmod(_stat.S_IRUSR | _stat.S_IWUSR)         # ← OUTSIDE lock

Meaning two concurrent POSTs could still race:

Thread A: acquire lock → read .env (has KEY_X) → add KEY_Y to current → release lock
Thread B: acquire lock → read .env (still has KEY_X only, A hasn't written) → add KEY_Z → release
Thread A: write_text({KEY_X, KEY_Y})
Thread B: write_text({KEY_X, KEY_Z})   ← clobbers A's KEY_Y

Low-probability (two users saving keys at the same millisecond), but the exact scenario R2 claimed to close. And the lost key would fail silently — user sees the success toast on both sides, reopens the page, one key is just missing.

What I pushed — 6bd6197

Moved file I/O inside the lock block. env_path.parent.mkdir, the write, and the final chmod all now execute under _ENV_LOCK.

Tightened the 0600 window further. Original fix did write_text() then chmod(0o600), which leaves a brief window where the file exists at the process umask (typically 0o644) before chmod. Replaced with os.open(path, O_CREAT|O_WRONLY|O_TRUNC, 0o600) + os.fdopen(...) so the file is created at owner-only mode from the start. Kept the trailing chmod as a belt-and-braces guard for the case where the file already existed at a looser mode (O_CREAT doesn't re-apply mode to pre-existing files).

Two regression tests added:

• test_env_file_created_with_0600_mode — asserts stat.S_IMODE(env_path.stat().st_mode) & 0o077 == 0. Catches any future refactor that regresses mode to world- or group-readable.
• test_write_inside_lock_not_after — structural check via inspect.getsource(_write_env_file) that scans for env_path.write_text, env_path.chmod, os.open, os.fdopen, and env_path.parent.mkdir call sites and asserts their indentation is deeper than the with _ENV_LOCK: line. This locks R2 mechanically — any future reviewer can't accidentally re-introduce the bug by dedenting the I/O out again.

Full security review — what I checked

B1 (auth gate) — POST endpoints at routes.py:960-973 use the same is_auth_enabled() or HERMES_WEBUI_ONBOARDING_OPEN or local-network pattern as /api/onboarding/setup at routes.py:1402-1415. Precedent-matching. The X-Forwarded-For trust is implicit in the existing model (only safe behind a trusted reverse proxy); not worse than the rest of the codebase. ✓

Worth noting: check_auth middleware at auth.py:158-180 gates authenticated paths when auth is enabled, so the gate here correctly only runs the local-network fallback when auth is disabled. The two layers compose correctly — no double-gate confusion.

B2 (_provider_has_key false-positive) — fixed. providers.py:145-152 explicitly only checks providers.<id>.api_key, not model.api_key. Comment explains why. The test at test_provider_entries_have_required_fields confirms providers without keys show has_key=False. ✓

R1 (0600 mode) — re-tightened in my patch from "chmod after write" to "open with mode + chmod backup". New test locks this invariant. ✓

R2 (full lock coverage) — fixed in my patch. New test locks this invariant. ✓

XSS (error handler) — panels.js:1384 now uses esc(e.message) inside the innerHTML template. ✓ All other HTML construction in the panel uses createElement + .textContent (lines 1389-1452) — safe by construction. showToast (ui.js:865) writes via textContent. ✓

GET /api/providers leakage check — get_providers() returns {id, display_name, has_key, configurable, key_source, models}. No raw key values at all, just presence booleans. Even key_source only indicates which storage location (env_file / env_var / config_yaml / oauth / none), never the value. ✓

Input validation — set_provider_key() enforces minimum length (≥8), rejects newlines/CRs (prevents .env injection via \nEVIL_KEY=x), OAuth providers explicitly rejected, unknown provider_id rejected. .strip().lower() normalization prevents ANTHROPIC and Anthropic submitting as different keys. ✓

Eval / subprocess / shell — none in the new code. grep -n "eval\|exec\|subprocess\|shell" api/providers.py returns nothing. ✓

Circular import risk — _write_env_file does from api.streaming import _ENV_LOCK inside the function body. Deliberate (avoids module-init-time cycle); works at call time since both modules are fully imported by then. ✓

i18n coverage

6 locales have the full providers_* key set (grep -c 'providers_tab_title' i18n.js → 6). Reasonable to ship with English fallback strings in the non-en locales — the cover letter flags "native translations welcome in follow-up PRs", which is the right ship-it-now trade-off. ✓

Settings-tab wiring

• index.html:462-464 — tab button with onclick="switchSettingsSection('providers')"
• panels.js:1131,1145 — switchSettingsSection recognises 'providers' and calls loadProvidersPanel() on activation
• panels.js:1353 — also preloads the panel in background when Settings opens
• Matches the pattern of the other Settings tabs. ✓

Edge-case trace

Tests

• 18/18 tests in tests/test_provider_management.py pass (16 original + 2 new regression tests)
• Full local suite: 1871 passed, 47 skipped. 4 failures, all pre-existing ordering flakes that pass in isolation:
  • test_issue798::test_concurrent_new_sessions_get_correct_profiles
  • test_sprint46::test_session_compress_roundtrip
  • test_set_key_writes_to_env_file (ordering with test_issue798's profiles monkeypatch)
  • test_env_file_created_with_0600_mode (same cause)
• The last two match the PR body's "5 pre-existing ordering-sensitive flakies, unchanged from baseline" — same behaviour as before the PR, not a regression.

Design notes (non-blocking)

• provider_id normalization at set_provider_key:273 does .strip().lower() — good. But get_providers() emits IDs as the keys of _PROVIDER_ENV_VAR / _PROVIDER_DISPLAY, which are already canonical. If a future caller passes provider=Anthropic, normalization handles it. Defensive. ✓
• ollama and ollama-cloud both map to OLLAMA_API_KEY (same env var). This means saving an Ollama Cloud key and then saving an Ollama key will overwrite each other — but they use the same key server-side anyway, so this is correct. Worth a note in a follow-up if the two diverge.
• _write_env_file reads the entire .env on every call. Fine — these files are small and writes are rare.
• The 15-line diff bump in _write_env_file from my patch isn't strictly necessary for correctness — a with open(..., 'w') as f: + trailing chmod would work identically under the moved lock. I kept the os.open variant because it eliminates one mode-transition window, which seems worth the ~5 extra lines for a credentials file.

Approval

Feature is well-scoped and closes a real UX gap (no more hand-editing .env). Six review-round fixes all landed. One of them (R2) had a subtle gap that my patch closed and a regression test now pins. Test coverage is strong (16 → 18 targeted tests); i18n coverage is complete; security posture matches or exceeds the onboarding endpoint precedent.

✅ Approved after the fix. Ready for merge + v0.50.159 tag (depending on what's already been tagged — the CHANGELOG currently says v0.50.158, but #863 and #864 also propose v0.50.158; first merger wins the slot).