fix: review/red-team batch 2 — 30 issues (security, ws, page-templates, html-embed, mcp, tests, docs) #101
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vvzvlad/gitmost#101
Reference in New Issue
Block a user
Delete Branch "fix/review-batch-2"
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?
Прогон по второй пачке ревью/ред-тим issues (47 шт) на смерженном develop. Одна ветка от develop, по коммиту на issue (29 коммитов), каждое код-изменение прошло ревью-сабагента; верифицировано-перед-фиксом (часть issues оказалась уже сделана прошлыми PR — закрыта отдельно с ссылкой на коммит).
Closes #52, #53, #54, #55, #56, #60, #61, #62, #63, #64, #66, #67, #68, #70, #71, #72, #75, #78, #83, #84, #85, #87, #88, #89, #90, #91, #92, #93, #94, #95
Что сделано (30 issues)
RT / безопасность (анонимный share-AI, auth, mcp, ws):
maxOutputTokensна анонимном стриме; #62 limiter fail-closed при Redis-сбое; #63 reject non-text parts (обход size-cap); #95 describeProviderError на анонимном пути + единый live-enabled резолв роли (findLiveEnabled).tryReserve(+ release на ранних throw, чтобы SSO/MFA/конфиг-ошибки не жгли бюджет жертвы); #61 trustProxy env-настраиваемый с безопасным дефолтом.page-templates / transclusion / html-embed:
resolveReadableSharePage(нашёл и закрыл недостающую deletedAt-проверку в воронке); #94 коллекторы →collectNodes, write-only граф задокументирован.Тесты / инфра / доки:
Косяки, пойманные ревью (и доделанные)
Проверка
Полный серверный сьют: 70 suites, 721 tests — зелёные. Server + client
tsc --noEmit— exit 0.Закрыто отдельно (вне этого PR — уже в develop)
17 issues закрыты комментарием со ссылкой на смерженный коммит: #26,27,29,44,65,69,73,74,76,77,79,80,81,82 (уже сделаны прошлыми PR), #86 (покрытие уже есть), #96,#97 (зонтики — дети закрыты).
НЕ вошло (блокировано)
#98, #99, #100 — тест-покрытие + 2 бага (BUG-1
$&-hazard в инъекции trackerHead, BUG-2 height NaN) для рефактора sandbox html-embed (commit81823fce), которого НЕТ в develop (он в отдельной несмерженной ветке). С ветки от develop их сделать нельзя — они должны лечь на/после мержа sandbox-рефактора. Флагнул.🤖 Generated with Claude Code
Обновление: смержен текущий develop (полный sandbox html-embed) + добавлено тест-покрытие
После того как в develop приехал sandbox-рефактор html-embed (commit
81823fce, «перешли полностью на песочницу»), влил develop в эту ветку и разрулил 13 конфликтов:bc0c49db(dead-DI #92 + docs).Добавил тест-покрытие, заведённое ревью PR #101 и sandbox-рефактора:
Closes #98, #99, #100, #102, #103, #104, #105, #106, #108, #109
Полный прогон после всего: server 69 suites / 689 tests, client 20 files / 240 tests, server+client tsc — чисто.
The 4-step html-embed gate (feature-enabled AND role-allowed -> stripHtmlEmbedNodes) was replicated across call-sites, pinned only by brittle source-regex tests. Add stripHtmlEmbedIfNotAllowed(json, {featureEnabled, role, onStrip}) and migrate the 5 plain strip-all sites (collab handler, page create+duplicate, both import paths, transclusion) to it, each keeping its own feature/role resolve + log via onStrip. Left the 2 sites with different semantics: persistence.extension (#29 preserve- admin) and share.service (feature-only kill-switch, no role gate). Real unit tests replace the regex pins; behavior identical. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>The '(shareId,pageId) -> usable non-restricted page in THIS share' boundary was written as 3 must-be-identical async sequences. They weren't: the chat funnel omitted an explicit page.deletedAt check (latently safe via getShareForPage's CTE) and layered isSharingAllowed separately. Add ShareService.resolveReadable- SharePage(shareId,pageId,workspaceId) running the single canonical sequence (getShareForPage -> id match (skipped when null) -> findById -> !deletedAt -> !hasRestrictedAncestor) returning {share,page}|null; getSharedPage, the funnel, and the getSharePage tool all use it. hasRestrictedAncestor now lives in the one method no caller can skip; the funnel still returns uniform 404s and keeps isSharingAllowed. Adds a direct security-invariant test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>Ghost referenced this pull request2026-06-21 14:10:36 +03:00