fix(cache): канонизация ключа канала (Durov/durov/@name -> один ключ) + one-shot миграция #47
Reference in New Issue
Block a user
Delete Branch "fix/24-channel-key"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Один канал жил под ключами
Durov/durov/@nameв трёх слоях (tgcache-файлы,data/cache/каталоги, SQLite-строки) → дубли кеша, двойные походы в Telegram, осиротевшие деревья. ID↔username НЕ унифицируем (сознательная не-цель).channel_key.py—canonical_channel_key(strip@,-100…числовые verbatim, иначе lowercase; безопасно и для Telegram API-вызовов).tg_cache._cache_file_pathпрогоняет ключ через canonical.post_parser.get_channel_username→ lowercase (обе ветки — канонизирует SQLite-строки, media-URL, t.me-ссылки). Диф — ровно эти 2 строки, числовая ветка не тронута.api_server.get_media:fs_channel = canonical_channel_key(channel)ПОСЛЕ проверки дайджеста (дайджест — по ИСХОДНОМУ url, иначе ломаются все выданные ссылки);fs_channelво всех путях/ключах/download ниже.migrate_channel_keys_syncвlifespan(послеinit_db_sync, доclient.start, черезasyncio.to_thread): канал целиком FS-потом-SQL. Обязательныйos.path.samefileguard ДО merge/rmtree — на case-insensitive FSDurov//durov/один inode, иначе снесли бы весь кеш канала. FS-сбой → SQL-шаг пропускается (строки старорегистровые, старое дерево видно свиперу — нет вечных orphan). SQL-merge PK-safe (twin → mergeadded=max/mimenon-NULL +DELETE, иначе plainUPDATE; НЕ bareSET channel=lower()). Идемпотентно.closes #24
Важно про миграцию
How verified
.venv/bin/python -m pytest tests/ -q→ 571 passed (+16 новых вtest_channel_key.py): unit-ключ (Durov/@durov/-1001…int&str); tgcache-путь идентичен для Durov/durov;get_channel_username('MixedCase')→'mixedcase'(обе ветки); миграция — SQL-merge → одна строкаmax(added)+non-NULL mime; samefile-guard no-op без потери данных; merge двух реально разных каталогов (target выигрывает); повторный запуск no-op.Внутренний цикл: 1 проход coder → внутреннее ревреview → APPROVE (без правок; 2 косметических нита).
Checklist
Ревью — #47
fix(cache): канонизация ключа канала (Durov/durov/@name -> один ключ) + one-shot миграцияВердикт: ПРАВКИ (CHANGES) — 2 правки (тест data-loss-guard'а миграции + traversal-гард на деструктивные FS-операции). Ядро корректно.
Полный веер из 9 аспектов по всему диффу PR (merge-base
d5a38c3f..11bfd2b3). Гейт зелёный:pytest tests/ -p no:cacheprovider→ 571 passed.Что проверено и корректно (клик)
canonical_channel_key): username → lower,-100…+isdigit → точная форма,@стрипается. Применена КОНСИСТЕНТНО на всех сайтах ключа (api_server/tg_cache/post_parser, read+write) — сплит-кеша не остаётся (подтверждено stability).migrate_channel_keys_sync, авто на старте черезto_thread): collision-мерж корректен (dst/twin выигрывает,added=max, non-NULL mime; DELETE старой строки без PK-конфликта). Forward FS-fail консистентность есть (FS упал → SQL пропущен,continue:122). Case-insensitive FS guard (samefile) не даёт снести кеш на macOS/APFS. Идемпотентно и дёшево: кандидаты =WHERE channel != lower(channel)→ после миграции пусто, FS-обход не запускается (marker-флаг не нужен).Правки (сделать, потом ре-ревью)
[test-coverage] Не покрыт data-loss-guard миграции (FS-fail → SQL пропуск) + однонаправленный max —
migrate_channel_keys.py:114-122, тестыtests/test_channel_key.pyГард «FS-шаг упал → пропустить SQL, оставить строки old-cased» (защита от орфанов/потери) НЕ тестируется: мутация — убрать
continueна :122 → все тесты зелёные. Плюсmax(added)тестируется только в одну сторону (Durov=200/durov=100→200); мутацияmax(added,t_added)→addedпроходит (данные маскируют баг). Fix: (a)test_migration_fs_failure_skips_sql— замокатьos.renameнаOSError, ассертить что миграция не падает, DB-строки остались old-cased, счётчик failures вырос; (b) реверс-тест max: Durov=100/durov=300 → 300.[security] Деструктивная миграция делает FS-операции по имени канала из БД без traversal-гарда —
migrate_channel_keys.py:97-113src/dst = os.path.join(cache_dir, name)гдеnameизSELECT DISTINCT channel, затемos.rename/_merge_dir_tree/shutil.rmtree. Если в БД попадёт канал с/или..(напр. через pre-existing route-traversal/media/{channel}— см. #46, вне scope), миграция переименует/удалит директории ВНЕcache_dir. Это новая деструктивная поверхность (#47 добавляет rename/rmtree на потенциально грязных данных). Fix: перед FS-шагом отбрасывать (skip + log, НЕ падать на старте) каналы, чьё имя содержит/или..(илиos.path.basename).canonical_channel_key— естественная точка гарда, но так, чтобы миграция скипала, а не крашила стартап.⛔ DROP — кодеру НЕ делать (лог калибровки)
[speculative]high→low[security] symlink-атака в_merge_dir_tree(src — symlink наружу → файлы уезжают, rmtree молча не чистит) — требует, чтобы атакующий уже имел FS-доступ на запись вcache_dir(тогда игра и так проиграна). Нереалистичная модель угрозы.[below-threshold]medium→low[stability] обратный кейс (FS успел, SQL упал → DB old-cased, файлы lowercase) не обработан — само-заживает (промах → ре-даунлоуд, не потеря данных), триггер редкий (простой UPDATE, busy_timeout=5000); two-phase-commit несоразмерен.[below-threshold]low[coherence]remove_old_cached_files_sync/download_new_filesстроят путь инлайн вместоmedia_cache_path()— работает (каналы в БД канонические), централизация — nice-to-have.[below-threshold]low[arch] нет migration-version маркера — идемпотентно и дёшево (см. выше), маркер это микро-оптимизация.[below-threshold]low[documentation/simplification] докстринг «SAFE for API» размыт; редундантныеstr()передmedia_cache_path— косметика.Канонизация корректна и полна, миграция аккуратная (collision-мерж, forward-консистентность, идемпотентность). Остались: тест на data-loss-guard (мутационно не покрыт) + traversal-гард на деструктивные FS-операции. После правок — PASS.
9 аспектов (security · stability · regressions · test-coverage · conventions · documentation · simplification · architecture · coherence). Консистентность всех сайтов ключа, идемпотентность-дёшево и FS-ops-по-БД-имени проверены мной по коду. Две «CRITICAL/HIGH ESCALATE» security-находки ревьюера пересмотрены: traversal → MEDIUM DO (реальная деструктивная поверхность, но эксплойт требует pre-existing route-traversal), symlink → DROP (модель угрозы). Escalate нет.
Поправил обе правки.
[security] Traversal-гард на деструктивные FS-операции миграции — добавлен.
_is_safe_channel_segment(name)отвергает имя канала с/,\, компонентом..или гдеos.path.basename(name) != name. Вызывается в НАЧАЛЕ цикла миграции — до вычисленияcanonicalи до любогоos.rename/_merge_dir_tree/shutil.rmtree; т.к.candidates= union БД-строк и листинга каталогов, один гард покрывает оба источника. Грязный канал (например если пролез через pre-existing route-traversal/media/{channel}) → warning +failures+++continue: старт не падает, деструктивная rename/rmtree не уходит заcache_dir, старорегистровые строки/каталоги доживают до 20-дневного свипа как любой немигрированный канал.[test-coverage] добавлены 3 теста (мутационно проверены):
test_migration_fs_failure_skips_sql:os.rename→OSError→ миграция не падает, DB-строки остались old-cased (SQL пропущен), в сводкеfailures 1. Снятьcontinueпосле FS-сбоя → тест краснеет.test_migration_sql_merge_reverse_max:Durovadded=100 /durovadded=300 → выжившийdurovсadded=300(max в обратную сторону) + non-NULL mime.test_migration_traversal_guard_skips_dirty_channel: каналы'../Evil'/'a/B'→ миграция скипает,os.rename/shutil.rmtreeНЕ вызваны, без краха. Отключить гард → тест краснеет.Проверка:
.venv/bin/python -m pytest tests/ -q→ 574 passed;git diff --stat— толькоmigrate_channel_keys.py+tests/test_channel_key.py. Готово к повторному ревью.(Заодно этот гард защищает миграцию от того самого path-traversal в
/media, который я вынес тебе отдельным вопросом — независимо от того, чинишь ли ты сам роут.)Ревью (раунд 2) — #47
fix(cache): канонизация ключа канала (Durov/durov/@name -> один ключ) + one-shot миграцияВердикт: PASS ✅ — обе правки раунда 1 закрыты.
Полный веер из 9 аспектов по всему диффу PR (merge-base
d5a38c3f..76f79a1c). Гейт зелёный:pytest tests/ -p no:cacheprovider→ 574 passed (+3 к раунду 1). Обе правки проверил адверсариально (мутации).Закрытие правок раунда 1 (мутационно подтверждено)
_is_safe_channel_segment(name)(migrate_channel_keys.py): отвергает/,\,..-компонент, non-basename, пустое/.; вызывается на :115 ПЕРЕДos.rename/_merge_dir_tree/shutil.rmtree, грязный канал скипается (log +continue, не крашит стартап), FS и SQL для него не трогаются. Легитимные каналы (username,-100…) все проходят — ложных скипов нет. Мутация:return Trueв гарде →test_migration_traversal_guard_skips_dirty_channelпадает.test_migration_fs_failure_skips_sql— мокаетos.rename→OSError, ассертит: миграция не падает, DB-строки остались old-cased,failuresвырос. Мутация: убратьcontinueна FS-fail (:153) → тест падает (был не покрыт в раунде 1 — теперь держит).test_migration_sql_merge_reverse_max— Durov=100/durov=300 → 300. Мутация:max(added,t_added)→added→ тест падает.Контекст раунда 1 (всё подтверждено) — клик
Канонизация корректна и применена на всех сайтах ключа (нет сплит-кеша); миграция: collision-мерж (dst/twin, max added, non-NULL mime), forward FS-fail консистентность, case-insensitive FS guard, идемпотентна и дёшева. DROP'ы раунда 1 (symlink — нереалистичная модель угрозы; reverse FS/SQL — само-заживает; path-DRY; no-marker) остаются в силе.
DO/ESCALATE нет. Обе правки раунда 1 закрыты корректно и мутационно-подтверждены; traversal-гард полон (проверено на 18 edge-кейсах: абсолютные пути,
.., null-байты, сепараторы — всё блокируется, легит проходит); гейт зелёный. Approve.9 аспектов. Обе правки проверены мной адверсариально (3 мутации: FS-fail continue, max-direction, guard) + независимо двумя субагентами; полнота гарда подтверждена. Escalate нет.