feat: add sync rsync foundation - #95
Conversation
axisrow
left a comment
axisrow
left a comment
There was a problem hiding this comment.
Ревью PR #95 — sync rsync foundation
Вердикт: одобряю (approve). Изменение маленькое, аккуратное, с TDD-покрытием; полный прогон python -m pytest tests/ даёт 378 passed (совпадает с заявленным в PR). Блокирующих проблем нет.
Что проверено
rsyncдобавлен в базовый apt-слой Dockerfile в алфавитном порядке (междуpython3-venvиtini), с--no-install-recommends. Соответствует конвенции. Новых env-переменных нет →.env.example/AGENTS.mdправить не нужно. ✓ensure_ssh_dirпереиспользует существующийensure_dir_owned(создание + рекурсивный chown + откат вверх доHAPI_USER_HOME), затемchmod 700. Порядок корректный. ✓ensure_gitconfig_fileгейтит на! -e && ! -L— не перезаписывает существующий файл и не пишет сквозь симлинк. Создаёт пустой.gitconfig(валиден для git). Тестtest_existing_gitconfig_is_not_overwrittenэто подтверждает. ✓- Тестовая инфраструктура корректна: фейковый
chownтолько логирует (тест не под root), фейковыйchmodлогирует и реально применяет режим — поэтомуssh_mode == 0o700проверяется по-настоящему, а не только по факту вызова. ✓ - CLAUDE.md обновлён и честно отмечает, что оркестрация синка вне bootstrap (#17). ✓
Замечание (не блокер)
Один inline-комментарий ниже: симлинк-безопасность .ssh при multi-tenant модели угрозы. Это не регрессия (тот же паттерн ensure_dir_owned уже применяется к .config/gh и .claude), поэтому не блокирует слияние — но стоит держать в уме, учитывая, что этот же файл специально хардили против hapi-подложенных симлинков в ensure_claude_settings.
| ensure_ssh_dir() { | ||
| local ssh_dir="${HAPI_USER_HOME}/.ssh" | ||
| ensure_dir_owned "${ssh_dir}" | ||
| chmod 700 "${ssh_dir}" |
There was a problem hiding this comment.
Симлинк: chmod/chown -R идут по симлинку (не блокер, замечание).
Я смоделировал сценарий, где hapi заранее подложил .ssh как симлинк на чужую директорию: ensure_dir_owned (рекурсивный chown -R hapi:hapi) и последующий chmod 700 под root следуют по симлинку и меняют владельца/режим цели (в моём тесте директория-цель из 755 стала drwx------, а .ssh осталась симлинком). chown -R при этом рекурсивно обходит содержимое чужой директории.
Это не регрессия и не блокер: тот же незащищённый ensure_dir_owned уже применяется к .config/gh и .claude, и этот PR лишь добавляет ещё один его вызов. Но обратите внимание: этот же entrypoint.sh специально хардили против ровно такого класса атак в ensure_claude_settings (строки 140–156: гейт на -L, «treat ANY pre-existing destination as hands-off, never follow symlinks», зеркалит uploads.py), потому что /home/hapi в multi-tenant форках доступен на запись потенциально недоверенному hapi. Если модель угрозы .ssh считается такой же, стоит добавить симлинк-гейт (например, ранний выход, если .ssh — симлинк, по аналогии с ensure_claude_settings).
Cycle-review (PR #95) — both Codex and the Claude reviewer independently flagged (and reproduced) a critical: ensure_ssh_dir ran `ensure_dir_owned` (mkdir -p + chown -R) then `chmod 700` on ~/.ssh with NO symlink guard. This runs as root before the privilege drop, and /home/hapi is hapi-writable via the persistent volume, so a hapi-planted `~/.ssh` symlink (e.g. -> /etc/ssh or another tenant's home) is dereferenced by GNU chown -R / chmod — root re-perms and re-owns the attacker-chosen target (arbitrary-path chmod+chown / DoS / ownership hijack). Same vuln class already fixed in ensure_claude_settings (#90); the new helper had reintroduced it, and the shipped happy-path tests were green over the hole. Fix mirrors ensure_claude_settings: bail on any pre-existing `~/.ssh` symlink or non-directory, and only chmod after re-confirming it is a real non-symlink dir. Test: test_ssh_dir_symlink_attack_is_refused plants ~/.ssh -> victim and uses the REAL chmod (not the fake logger) so a follow-through would actually mutate the victim — asserts victim stays 0755, is never chowned, and the symlink is left as-is. Fails on the old code, passes on the fix. pytest tests/ -> 379 passed, 205 subtests. Refs #91 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vecu5xo9D9Cs2vmeohKVKv
🔍 Local review (cycle 1)Reviewed locally (Claude subagent + Codex companion), no bots pinged.
FIX applied ( SKIP #3 kept intentionally: Verified behaviorally: symlinked Cycle 1/3 complete: 2 FIX (both fixed), 1 SKIP (kept with rationale). No remaining FIX → review cycle done. Merge is yours to trigger (local mode does not auto-merge). |
) Six P1 correctness/reliability fixes from the #101 code review. #3 (proxy.py): enable SO_KEEPALIVE on both tunnel sockets so a client that vanishes without a FIN (laptop sleep, NAT timeout) is detected by the OS and the idle tunnel thread + sockets are freed instead of leaking forever. #4 (ratelimit.py): use time.monotonic() instead of wall-clock time.time(), so a backward clock jump (NTP correction, suspend/resume, VM migration) cannot freeze the window and lock out legitimate users past 60s/300s. #5 (app.py): _check_auth now honours redirect=True in the removed-user branch — a browser navigation with a valid cookie whose account was deleted/renamed lands on /login (302) instead of a raw 403 JSON blob. #6 (proxy.py): add "cookie" to HOP_BY_HOP_HEADERS so the client's signed ttyd_session + csrf_token is not forwarded to the internal ttyd (defense depth). #10 (bin/clihost-sync.sh): the remote symlink guards were byte-identical triplicates (local pair + one copy in each of two `bash -s` heredocs), so hardening one silently bypassed the others — the class of bug caught 3× before (#88/#90/#95). Define them once in a shared emit_remote_prelude injected into both remote payloads. #11 (.env.example): document SESSION_TIMEOUT, CLEANUP_ROOT, ROOT_PASSWORD, HAPI_USER, HERMES_AUTO_UPDATE (read by code + CLAUDE.md but missing from the example), plus the REQUEST_TIMEOUT slowloris knob for completeness. New/changed env vars documented in .env.example: SESSION_TIMEOUT, CLEANUP_ROOT, ROOT_PASSWORD, HAPI_USER, HERMES_AUTO_UPDATE, REQUEST_TIMEOUT. No port or volume changes. Closes #101 findings #3, #4, #5, #6, #10, #11. Claude-Session: https://claude.ai/code/session_01KgvJhzd8gEhXVE4MVu18nn Co-authored-by: axisrow <axisrow@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
rsyncin the base apt layer./home/hapi/.sshwith mode0700and create/home/hapi/.gitconfigonly when missing.CLAUDE.md.Validation
python -m pytest tests/(378 passed)Closes #91