git-sync red-team #7/#8: ядро Docmost — move-TOCTOU цикл A↔B + рекурсивные CTE без cycle-guard #207

New Issue

claude_code · 2026-06-26T01:38:11+03:00

claude_code commented

2026-06-26 01:38:11 +03:00

Найдено ред-тимом git-sync (PR #119), но это пре-existing баги ЯДРА Docmost (shared PageService/page.repo), а не git-sync — поэтому выносится в отдельный issue для фикса в мейнлайне (по решению владельца). git-sync лишь делает их более достижимыми (конкурентные правки структуры дерева через два писателя).

#7 — TOCTOU в `PageService.movePage`: два конкурентных move создают цикл A↔B

page.service.ts (~923-945): проверка циклов (getPageBreadCrumbs) и сам updatePage — два НЕтранзакционных, неблокируемых стейтмента. Если move-1 («A под B») и move-2 («B под A») оба читают ДО-записанный ацикличный снапшот до того, как любой из них закоммитит — оба проходят guard и персистят A.parentPageId=B.id И B.parentPageId=A.id → цикл родитель/потомок в дереве.

Репро (jest, реальная БД): засидить A,B как корни в одном спейсе; параллельно (Promise.all + барьер на getPageBreadCrumbs) вызвать movePage(A под B) и movePage(B под A); обойти parentPageId от A и от B — оказывается цикл (не доходит до корня, повтор id).

Fix: выполнять cycle-check и UPDATE в ОДНОЙ транзакции с блокировкой (SELECT ... FOR UPDATE по затрагиваемым строкам / advisory-lock на спейс), либо проверять цикл в самом UPDATE.

#8 — рекурсивные CTE (ancestors/descendants) без CYCLE/depth-guard

getPageBreadCrumbs (page.service.ts ~986-1016) и descendant-CTE в removePage (~1135-1147) используют withRecursive+unionAll БЕЗ CYCLE-клаузы и без ограничения глубины. Если в БД уже есть цикл (см. #7), эти запросы зависают/таймаутят — и сам move-guard зовёт тот же CTE, т.е. цикл «самозащищается» (move-guard перестаёт работать). Движок git-sync (layout.ts ~91-105) имеет visited-set — асимметрия, доказывающая, что в ядре защиты нет.

Репро (jest, реальный PG): INSERT/UPDATE напрямую так, чтобы A.parentPageId=B.id и B.parentPageId=A.id (обойти guard); поставить короткий statement_timeout; вызвать getPageBreadCrumbs(A) → таймаут/переполнение вместо ограниченного списка.

Fix: добавить CYCLE-клаузу (Postgres 14+) или depth-counter с порогом во все рекурсивные CTE обхода дерева (ancestors + descendants).

Источник: ред-тим PR #119 (findings #7/#8), оба воспроизведены чтением кода. Тесты-репродукции пока не закоммичены (требуют реального PG в CI; см. также test:int-инфраструктуру).

Найдено ред-тимом git-sync (PR #119), но это **пре-existing баги ЯДРА Docmost** (shared `PageService`/`page.repo`), а не git-sync — поэтому выносится в отдельный issue для фикса в мейнлайне (по решению владельца). git-sync лишь делает их более достижимыми (конкурентные правки структуры дерева через два писателя). ## #7 — TOCTOU в `PageService.movePage`: два конкурентных move создают цикл A↔B `page.service.ts` (~923-945): проверка циклов (`getPageBreadCrumbs`) и сам `updatePage` — два НЕтранзакционных, неблокируемых стейтмента. Если move-1 («A под B») и move-2 («B под A») оба читают ДО-записанный ацикличный снапшот до того, как любой из них закоммитит — оба проходят guard и персистят `A.parentPageId=B.id` И `B.parentPageId=A.id` → цикл родитель/потомок в дереве. **Репро (jest, реальная БД):** засидить A,B как корни в одном спейсе; параллельно (Promise.all + барьер на `getPageBreadCrumbs`) вызвать `movePage(A под B)` и `movePage(B под A)`; обойти `parentPageId` от A и от B — оказывается цикл (не доходит до корня, повтор id). **Fix:** выполнять cycle-check и UPDATE в ОДНОЙ транзакции с блокировкой (SELECT ... FOR UPDATE по затрагиваемым строкам / advisory-lock на спейс), либо проверять цикл в самом UPDATE. ## #8 — рекурсивные CTE (ancestors/descendants) без CYCLE/depth-guard `getPageBreadCrumbs` (`page.service.ts` ~986-1016) и descendant-CTE в `removePage` (~1135-1147) используют `withRecursive`+`unionAll` БЕЗ `CYCLE`-клаузы и без ограничения глубины. Если в БД уже есть цикл (см. #7), эти запросы **зависают/таймаутят** — и сам move-guard зовёт тот же CTE, т.е. цикл «самозащищается» (move-guard перестаёт работать). Движок git-sync (`layout.ts` ~91-105) имеет visited-set — асимметрия, доказывающая, что в ядре защиты нет. **Репро (jest, реальный PG):** INSERT/UPDATE напрямую так, чтобы `A.parentPageId=B.id` и `B.parentPageId=A.id` (обойти guard); поставить короткий `statement_timeout`; вызвать `getPageBreadCrumbs(A)` → таймаут/переполнение вместо ограниченного списка. **Fix:** добавить `CYCLE`-клаузу (Postgres 14+) или depth-counter с порогом во все рекурсивные CTE обхода дерева (ancestors + descendants). --- _Источник: ред-тим PR #119 (findings #7/#8), оба воспроизведены чтением кода. Тесты-репродукции пока не закоммичены (требуют реального PG в CI; см. также `test:int`-инфраструктуру)._

vvzvlad added the bug label 2026-06-26 05:06:29 +03:00

claude_code referenced a pull request that will close this issue

2026-06-26 06:27:08 +03:00

fix: bug batch — #161 #190 #207 #159 + #206 findings #212

claude_code referenced this issue from a commit

2026-06-26 06:27:08 +03:00

fix(page): make movePage cycle-check + update atomic to prevent concurrent A<->B cycles (#207)

claude_code referenced this issue from a commit

2026-06-26 06:27:08 +03:00

fix(page): add cycle/depth guard to recursive tree-traversal CTEs (#207)

vvzvlad referenced this issue

2026-06-26 16:42:25 +03:00