Approve-with-comments follow-ups (no blockers):
- callout: unify the GitHub-callout feature ticket on #192 (the callout-paste
feature the CHANGELOG already tracks); #218 is the public-share security work.
Fixed the code comment and test reference.
- export/utils.spec: pin current behavior of a leading-dot name (".gitignore" ->
"") — same bug class as #204 but unreachable via the sole caller, so document
not change.
- share.types: narrow ISharedPage to the actual /shares/page-info allowlist
(page -> Pick of id/slugId/title/icon/content; trimmed share; dropped the
spurious `extends IShare`). Verified all three consumers (shared-page,
link-view, mention-view) read only allowlist fields.
- editor-ext: extract shared CALLOUT_TYPES / normalizeCalloutType /
renderCalloutHtml into callout-common.marked.ts; both tokenizers
(`:::type` and `> [!type]`) now share the renderer + type dict while staying
separate. Eliminates the byte-identical renderer + duplicated type list.
- share.service: extract named predicate shareIdGrantsAccess(requestedShareId,
resolvedShare) for the id-or-key fast path (naming only, no control-flow
change); kept narrower than resolveReadableSharePage's id-only gate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Approve-with-comments follow-ups:
- breadcrumb: fix the reverse regression where navigating A->B to a page absent
from the lazily-built tree (before its ancestors load) left the previous
page's clickable chain on screen. New pure computeBreadcrumbState clears a
stale chain that doesn't end at the current page, while keeping one that does
(no blank flash for an already-resolved page); unit-tested for the
navigated-to-absent-page case.
- share.service: getShareAncestorPage no longer swallows DB errors silently —
now a live public-share path (isPageReachableThroughShare), so a transient
error is logged with ancestor/child ids and still fails closed (caller 404s)
instead of becoming a traceless misleading "not found".
- i18n: register the new "Connecting… (read-only)" key (U+2026 ellipsis) in
en-US (source of truth) and ru-RU (Подключение… (только чтение)).
- share.service: correct the FUTURE note — 3 callers pass no shareId
(share-alias.controller/.service, share-seo.controller); the two ai-chat
callers already pass a real shareId.
- CHANGELOG: add Unreleased Changed/Fixed/Security entries for #216 opt-in
sub-pages default, #218 trimmed page-info payload + forged-shareId 404, #204
export internal-link name, #206/#218 breadcrumb, #192 callout paste, #218
editor pre-sync read-only gate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review follow-ups for the combined QA-UI fixes (#216/#206/#204/#218/#192):
- export/utils: correct the misleading getInternalLinkPageName comment — a
bare `v1.2` loses its last dot-segment (`v1`); dots survive only in
multi-segment names like `v1.2.md` -> `v1.2`.
- share: extract toPublicSharePayload(page, share): PublicSharePayload, an
explicit allowlist type+mapper replacing the inline literal in the
/shares/page-info anonymous path (#218). Add share.controller.spec.ts that
stubs getSharedPage returning internal fields and asserts the response key
set EXACTLY equals the whitelist (page + share), so any `...shareData`
regression or new leaking field fails. Also key-tests the extracted mapper.
- breadcrumb: extract pure resolveBreadcrumbNodes(treeData, ancestors, pageId)
(tree-hit -> tree; tree-miss -> map ancestors via canonical pageToTreeNode,
dropping the as-any casts; else null) and unit-test all three branches.
- share-modal: RTL test asserting enabling a share calls mutateAsync with
includeSubPages: false (#216 security default).
- share.service: one-line note at getSharedPage on the deferred consolidation
of the ancestor-aware match into resolveReadableSharePage.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Public sharing (#218):
- Bind public-share content to the requested shareId. getSharedPage now
enforces dto.shareId (forwarded from /share/:shareId/p/:slug): the page must
be reachable THROUGH that exact share (its own share, or an includeSubPages
ancestor that contains it). A forged/mismatched shareId 404s instead of
rendering off the slug alone and no longer leaks the real canonical key via
redirect. A request with no shareId keeps the legacy slug-capability path.
- Trim /shares/page-info: drop internal metadata (creatorId, spaceId,
workspaceId, contributorIds, lastUpdated*, parent/position, lock/template
flags, timestamps) from the anonymous payload.
- Default share-to-web includeSubPages to false (opt-in), so enabling a share
no longer silently exposes the whole sub-tree (#216).
Editor (#218):
- Harden the new-page pre-sync window: the body editor is kept read-only until
the collab provider is Connected and synced, so early keystrokes can't land
only in local ProseMirror and then be clobbered by the server's empty doc.
- Surface a "Connecting… (read-only)" affordance during the static phase so
input isn't silently swallowed.
Other:
- Breadcrumb: resolve from the page's own ancestor data (/pages/breadcrumbs)
instead of waiting for the lazily-built sidebar tree, so deep pages don't
render a blank breadcrumb for seconds.
- Pasting GitHub `> [!type]` callouts now converts to a callout node instead of
a literal blockquote (new marked extension wired into markdownToHtml).
Tests: editor-sync-state gate (client), getSharedPage share-binding (server),
github-callout markdown conversion (editor-ext).
Co-Authored-By: Claude Opus 4.8 (1M context) <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>
Convert the htmlEmbed node from same-origin raw-HTML execution to a sandboxed
iframe (sandbox="allow-scripts allow-popups allow-forms", no allow-same-origin,
srcdoc) with postMessage auto-resize (validated by event.source) and an optional
manual height attr. The block now runs in an opaque origin and cannot reach the
viewer's cookies/session/API, so it is safe for any member.
Because the block is now harmless, remove the entire admin/role gating apparatus:
drop htmlEmbedAllowed/canAuthorHtmlEmbed/stripDisallowedHtmlEmbedNodes/
collectHtmlEmbedSources and every role-based strip on the write paths (collab
REST/MCP + socket, page create/duplicate, import x2, transclusion unsync), along
with the now-unused WorkspaceRepo/UserRepo injections and the PageService.create
callerRole param. Keep one strip: prepareContentForShare still removes htmlEmbed
on the anonymous public-share read path when the workspace master toggle is OFF.
The workspace settings.htmlEmbed toggle is now a plain feature switch (gates the
slash-menu and share rendering); when ON the block is available to all members.
Add settings.trackerHead: an admin-only raw HTML/JS analytics snippet injected
verbatim into the <head> of public share pages only (ShareSeoController), for
trackers that genuinely need same-origin. Admin-gated via the existing CASL
Manage/Settings ability; never injected into the authenticated app shell.
Closes security-review findings #1, #2, #4, #5, #10 (and #3 as a security issue).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The html-embed feature toggle was enforced CLIENT-side in the NodeView (reads
settings.htmlEmbed from the logged-in workspace), so an anonymous public-share
viewer — who has no workspace context — always saw it as OFF and got a
placeholder instead of the executing embed. That broke the whole point (a
tracker must run for anonymous visitors).
Make it server-authoritative:
- share.service prepareContentForShare (the single path both share-content
flows use) strips htmlEmbed from served content when the workspace toggle is
OFF; both callers (updatePublicAttachments host page + lookupTransclusionForShare)
resolve the toggle once and pass it. Fail-closed: missing workspace -> OFF ->
stripped.
- NodeView executes whatever it was served in read-only/share mode
(shouldExecute = !editor.isEditable || htmlEmbedEnabled); the disabled
placeholder now only shows in the editable editor when OFF.
Net: anonymous share + toggle ON -> server serves the (admin-authored) embed ->
it executes for everyone; toggle OFF -> stripped server-side from every
share-content path (true kill switch); a non-admin embed can never be served
(save-path strip). No XSS regression in the editable editor.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix tree nodes sort
* remove comment mark in shares
* remove clickoutside hook for now
* feat: search in shared pages
* fix user-select
* use Link
* render page icons
