Follow-up fixes to the htmlEmbed-sandbox / trackerHead change:
- share-seo: inject trackerHead via a function replacer so `$`-sequences
($&, $', $`, $$) in the admin snippet are inserted literally instead of
being treated as String.replace substitution patterns; warn when the
</head> marker is absent instead of silently skipping injection.
- mcp: register a passthrough `htmlEmbed` node in the schema mirror so an
AI/MCP edit of a page containing an embed no longer throws
"Unknown node type: htmlEmbed" in TiptapTransformer.toYdoc.
- editor-ext + client: treat a non-finite `data-height` as auto (null) so a
crafted/corrupted height cannot disable auto-resize or yield a NaN iframe
height; extract a shared clampHeight helper.
- client: rename render-raw-html.{ts,test.ts} -> html-embed-sandbox.{...} and
shouldExecute -> shouldRender so the seam name matches the sandbox model.
- client: i18n the iframe title; surface the real error reason in
tracker-settings (console.error + err.response.data.message).
- docs: note hasHtmlEmbedNode is now a test-only helper; add an Unreleased
CHANGELOG entry; drop the dangling "arbitrary HTML embed" planning-doc ref.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
collectPageEmbedsFromPmJson (and the sibling collectors/remap) recursed with no
guard, so a pathological/cyclic non-JSON input could stack-overflow (RangeError).
Add a depth cap (1000, far above any real doc nesting) so such input is handled
gracefully. Normal documents are unaffected. Updates a stale test that asserted
the old throwing behavior.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
trustProxy was unconditionally true, so req.ip came from a client-forgeable
X-Forwarded-For and the per-IP throttles (share-AI, /mcp brute-force) were
spoofable. Make it env-configurable (TRUST_PROXY) with a safe default that
trusts XFF only from loopback/private proxies, documented in .env.example.
NOTE: this changes the default from trust-all; deployments whose proxy is on a
public IP must set TRUST_PROXY (caveat documented).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
WS events missed during a disconnect (wifi blip, sleep) were lost, so the
sidebar tree silently diverged until a manual reload. On RECONNECT (not the
first connect) invalidate the root-sidebar-pages + sidebar-pages queries so the
tree refetches through the authorized API and re-converges.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
After a merge decideEmbedState became the canonical guard and inlines the
cycle/too-deep logic, leaving these predicates called only by their own tests.
Remove them (and their test blocks); keep PAGE_EMBED_MAX_DEPTH (used by
decideEmbedState). Production behavior stays covered by decide-embed-state.test.ts.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The encode catch comment promised 'fall back to raw' but the code returns '';
returning raw source wouldn't help anyway (un-encoded markup can't be atob-decoded
downstream, so decode would yield '' regardless), and a raw value in data-source
breaks the inert-storage guarantee. '' is the correct decode-symmetric failure —
fix the misleading comment to say so. Adds a codec test for the encode-throw path.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A custom AI-role's text preceded the only SAFETY_FRAMEWORK block and replaced
the persona, so a jailbreak in the role text sat before the safety rules.
buildSystemPrompt now emits SAFETY both before AND after the persona, with the
role/persona delimited as lower-trust (<role_persona note=...>); the default
persona is sandwiched too. Context (currently-viewing-page) preserved.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
#67: movePage didn't check the destination wasn't the page itself or inside its
own subtree, so MCP/REST/agent/fast-drag could persist+broadcast a cycle. Reject
before the update (self-parent, or moved page among the destination parent's
ancestors via getPageBreadCrumbs).
#64: movePage emitted PAGE_MOVED from a stale pre-read even when the row didn't
change / was concurrently deleted (phantom move). Gate the emit on
updateResult.numUpdatedRows !== 0n. Both are movePage hardening in one method.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
#60: streamText had no maxOutputTokens, so one anonymous request could run up
the provider bill. Add maxOutputTokens (env SHARE_AI_MAX_OUTPUT_TOKENS, default
512) via resolveShareAiMaxOutputTokens().
#95: the anonymous path hand-built error strings, diverging from the unified
describeProviderError format used on the authenticated path; both onError blocks
now call describeProviderError so a share reader sees 402/429/503 causes in the
same form (and the stack is still logged). Both changes are in this one file and
share hunks, hence one commit.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A user with password=NULL passed the missing/disabled guard and reached
comparePasswordHash(pw, null), which native bcrypt rejects -> 500 on
/api/auth/login and, on /mcp, a leaky 401 that the brute-force limiter ignored
(enumeration oracle + limiter evasion). Treat a null/empty password like a
missing user in verifyUserCredentials (dummy compare for timing parity + unified
CREDENTIALS_MISMATCH_MESSAGE) and reject early in changePassword before bcrypt.
Contract spec asserts the null-password guard.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The widget hardcoded a generic 'Something went wrong' body and ignored
error.message, violating AGENTS.md. Render describeChatError(error.message, t) —
the same helper the internal chat uses — so a reader sees the real 402/429/503
cause instead of a bare 'try again'.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
MAX_SHARE_MESSAGE_CHARS only counted text parts, so a forged non-text part
(tool-result/file/data) bypassed the cap and bloated the model input
(token-DoS); convertToModelMessages would also expand a forged tool-result. The
anonymous path runs no tools, so a client non-text part is never legitimate —
reject any message with a non-text part (isTextUIPart) before the size check.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The per-workspace anonymous share-AI cost cap failed OPEN on a Redis error
(return true => admit), so a Redis outage removed the cap entirely (unmetered
billable anonymous calls). The feature is optional, so unavailability is
harmless: fail CLOSED (return false => controller 429s) instead.
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>
Move Page templates (#17), Public-share AI assistant (#14/#25/#41) and
Footnotes (#18) from "Planned" to "Done" in both README.md and
README.ru.md — they are already implemented on develop. Drop their stale
links to deleted plan docs (page-templates-plan.md, footnotes-plan.md,
public-share-assistant-plan.md). Offline mode and the rest of the list
are left unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Harden the anonymous public-share AI assistant against token-cost abuse
before exposing it to the internet:
- Add an env-tunable per-request output ceiling (maxOutputTokens) to the
public-share streamText call so one anonymous request cannot run up the
provider bill even if the per-IP throttle is evaded. New
resolveShareAiMaxOutputTokens() / SHARE_AI_MAX_OUTPUT_TOKENS_DEFAULT
(env SHARE_AI_MAX_OUTPUT_TOKENS, default 512), mirroring
resolveShareAiWorkspaceMax().
- Flip the per-workspace cost limiter to FAIL CLOSED on Redis failure
(was fail-open): if Redis is unavailable we cannot prove the workspace is
under its cap, so deny rather than admit an unmetered, billable call.
- Update the limiter spec (fail-open -> fail-closed) and add resolver tests;
document both knobs in .env.example.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
APP_SECRET does double duty: it signs JWTs and derives the AES-256-GCM key
that encrypts stored AI-provider credentials. Rotating it makes every saved
AI API key undecryptable and invalidates existing sessions. Document this
footgun where operators set the value (RT-30 from the red-team report).
- .env.example: dual-role warning block above APP_SECRET
- README.md / README.ru.md: warning callout in the upgrade section
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolve the html-embed.spec.ts conflict as a union: both #46 and #49 (already in
develop) added different test cases to the same file. Keep all of them —
stripHtmlEmbedNodes gets #46's root-node case plus develop's deeply-nested,
non-object and empty-content cases; #46's collectHtmlEmbedSources and
stripDisallowedHtmlEmbedNodes suites and develop's hasHtmlEmbedNode suite all
kept; imports unioned. No production code conflicted.
Full suite green: server 651, client (16 files), editor-ext 56, mcp 247.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolve conflicts from the parallel page-embed refactor that landed in develop
via #49:
- page-embed-view.tsx: keep develop's canonical decideEmbedState for the
cycle/depth/availability guard; keep #45's #39 chrome cleanup (single source
link, IconFileText fallback) and #40 refresh remount key. Drop #45's now-unused
isPageEmbedCycle/isPageEmbedTooDeep wiring.
- page-embed-picker.tsx: use develop's excludeHost util; drop #45's duplicate
filterPageEmbedOptions and its test.
- page-embed-ancestry-context.test.tsx: keep #45's superset suite.
- page-template-access.spec.ts: keep develop's constructor args; update the two
deleteByReferenceAndSources assertions to the new 4-arg workspace-scoped
signature introduced by #45 (#36 defense-in-depth).
Full suite green: server 624, client 219, editor-ext 56, mcp 247.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
After develop merged, mcp.service.ts calls decideBasicGate from mcp-auth.helpers.
The gate spec mocked the whole module returning only FailedLoginLimiter, so the
merged code crashed with 'decideBasicGate is not a function' (7/7 failing).
Spread jest.requireActual('./mcp-auth.helpers') so the real helpers are kept and
the gate exercises real logic; keep only FailedLoginLimiter stubbed so its
constructor runs without a real sweep timer.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
isExternalHttpUrl treated any http(s):// URL as external, so an absolute link
back to the app's own host (e.g. https://self/p/{uuid}, /settings/members)
emitted by the assistant stayed clickable on the anonymous share, leaking
internal UUIDs/structure and pointing at auth-gated routes. Classify a link as
external only when its host differs from window.location.host; unparseable URLs
are treated as internal (fail-closed). Tests cover own-origin absolute (flag
on -> inert), external host (kept with safe rel/target), dangerous schemes, and
no behavior change for the internal chat (flag off).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Docker-image builds ran independently of the Test workflow, so a
failing test would not block publishing the :develop image (or a
release). GitHub Actions `needs:` only works within one workflow, so the
two separate workflows didn't depend on each other.
Make test.yml a reusable workflow (workflow_call) and call it from
develop.yml and release.yml as a `test` job that `build` depends on
(`needs: test`); release's `release` job already needs `build`, so it
waits transitively. test.yml keeps its pull_request trigger for PR
gating; its redundant push:develop trigger is dropped (develop.yml now
calls it on push).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add .github/workflows/test.yml (pnpm + Node 22): on pull_request and push
to develop it installs, builds @docmost/editor-ext and runs `pnpm -r test`
across all packages (server Jest, client Vitest, editor-ext Vitest,
packages/mcp node:test). So tests now run automatically in CI, not just
on demand.
To make the run green, quarantine the 16 pre-existing stock NestJS
`should be defined` scaffold specs via jest `testPathIgnorePatterns` —
they never compiled (missing DI providers / lib0 ESM) and assert nothing
useful. Tracked for a proper fix/removal in issue #56. Verified each
pattern drops only its scaffold (46 of 62 suites still collected) and the
full `pnpm -r test` is green: server 587, client 185, editor-ext 56,
mcp 247.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Keep the backlog focused on deferred TESTS; the related non-test gaps
(model-allow-list, restriction-cache invalidation, server embed-recursion
guard, collectPageEmbeds cycle guard, jest DI/lib0-ESM debt) are now
tracked as issues #52-#56 and only linked from the backlog.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Captures what PR #49 intentionally left out: DB-integration tests (need a
test Postgres), the public-share XFF e2e + real-Redis Lua check (need an
HTTP/Redis harness), the full AiChatService.stream integration (R1-stream
seam), and the related non-test findings (no server-side model allow-list,
unreferenced restriction-cache invalidation, client-only embed recursion
cap, missing cycle guard, and the pre-existing jest DI/lib0-ESM debt).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The public-share widget was a separate minimal impl: plain-text answer, static
'Thinking…', no markdown, no tool-cards. Now it renders through the internal
chat's debugged presentational layer (MessageList/MessageItem/TypingIndicator/
ToolCallCard), so a share gets the same incremental streaming, animated typing
indicator, markdown, and tool-call cards. The share keeps its anonymous
transport (useChat + DefaultChatTransport '/api/shares/ai/stream',
credentials:'omit').
The shared components were already prop-driven (UIMessage[] + isStreaming) with
no transport/auth coupling; made the new props additive optionals (emptyState,
showCitations, neutralizeInternalLinks) all defaulting to current behavior, so
the internal chat is unchanged.
Security (review-caught): rendering assistant markdown on the ANONYMOUS share
made internal links (/p/{id}, /settings/...) clickable, which the old plain-text
render didn't. renderChatMarkdown gains neutralizeInternalLinks (true only on
the share): a one-shot DOMPurify afterSanitizeAttributes hook (added/removed by
reference around a single sanitize) strips href from internal/relative/non-http(s)
links (rendered inert) and keeps external http(s) links with
rel=noopener noreferrer nofollow target=_blank. Tests cover both the link
neutralization and the absence of any global-hook leak into internal renders.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The floating chat window covered page content; you could only collapse it
manually. Now it auto-collapses to its header (visual collapse only — ChatThread
stays mounted so an in-flight stream isn't interrupted) when you interact with
the page, and expands again from the header.
- document mousedown listener in the CAPTURE phase, armed only when
windowOpen && !minimized; collapses on a pointer-down outside the window.
Guards: ignore clicks inside the window and inside any Mantine [data-portal]
(the chat-list kebab menu + delete-confirm modal render in portals).
- Header click expands: startDrag distinguishes click vs drag by a 4px
threshold (minimizedRef avoids a stale closure); an expand-click doesn't
persist geometry.
- Reset minimized=false when the window opens (no sticky collapsed state).
- a11y: when minimized, the title is the keyboard expand affordance
(role=button, tabIndex, aria-label Expand, Enter/Space) — kept off the
dragBar container so no role=button wraps the Minimize/Close buttons.
- Pure helpers shouldCollapseOnOutsidePointer + isHeaderClick with vitest tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add ~330 tests across server (Jest), client (Vitest), editor-ext (Vitest)
and packages/mcp (node:test) for the gitmost features added since
053a9c0d: AI chat, AI agent roles, public-share assistant, MCP per-user
auth, HTML embed, page templates/embed, realtime tree, tree
expand/collapse, and the AI-settings UI.
Test-tooling fixes (prerequisite, were silently hiding coverage):
- Repair 3 page-template specs broken by the 11-arg TransclusionService
constructor; they never compiled, so template access-control / content
-leak / unsync-strip coverage was fictitious.
- Build @docmost/editor-ext before server tests via a `pretest` hook;
the stale dist omitted the new HtmlEmbed/PageEmbed exports (TS2305).
- Let jest resolve the .tsx email templates: add `tsx` to
moduleFileExtensions and widen the ts-jest transform to (t|j)sx?.
Behaviour-preserving "extract pure core" refactors that the tests drive:
- server: resolveShareAssistantRequest + uiMessageTextLength
(public-share controller), decideBasicGate + mapAuthResultToResponse
(mcp), buildErrorAssistantRecord (ai-chat), jsonbObject export (roles).
- client: render-raw-html + shouldExecute/canEdit, decide-embed-state,
page-embed picker utils, tree-socket reducers, open/close branch maps,
isEndpointConfigured/resolveKeyField; buildTreeWithChildren now treats
a permission-trimmed orphan as a root instead of crashing.
Deferred (need a test DB or HTTP harness, documented in the specs):
repo-level Postgres integration tests and the public-share XFF E2E.
Pre-existing DI/lib0-ESM suite failures are untouched and out of scope.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Post-merge hardening from the #13 security review:
- isInitializeRequestBody now delegates to the SDK isInitializeRequest (same
predicate as packages/mcp/http.ts), so a bare {method:'initialize'} with no
id/params no longer triggers the side-effecting login() (audit-spam /
user_sessions growth) before http.ts 400s it.
- Bind the Bearer path to the instance workspace: verifyBearerAccess rejects a
token whose payload.workspaceId != the instance workspace (resolved via
workspaceRepo.findFirst, consistent with the Basic path); optional param so
it's a no-op when unset.
- Close the user-enumeration timing oracle in verifyUserCredentials: the
missing/disabled branch now runs a bcrypt compare against a module-level dummy
hash whose cost (12) matches production saltRounds, so both paths take one
equal-cost bcrypt compare; the exact CREDENTIALS_MISMATCH_MESSAGE is preserved.
- Document the trusted-proxy requirement for the spoofable per-IP brute-force
limiter in .env.example (trustProxy is on; deploy behind a trusted proxy).
- Add real-execution coverage for enforceBasicLoginGate (SSO enforced / EE-MFA
bundled vs not / user-MFA / workspace-enforced-MFA) instead of stubbing the gate.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A non-admin's transient htmlEmbed could execute in other open editors until the
debounced (10s) onStoreDocument strip. Add a ~300ms onChange-debounced early
strip (guardHtmlEmbed) that converges the shared ydoc for everyone far sooner.
Safety-critical details:
- Scheduled from onChange ONLY for non-admins AND only when the workspace toggle
is ON (cached per-document in onLoadDocument), so the common toggle-OFF case
does zero extra work.
- guardHtmlEmbed does ALL async work (toggle + persisted allow-list read) FIRST,
then performs fromYdoc -> strip -> fragment.delete -> applyUpdate in a single
SYNCHRONOUS, await-free block, so no inbound Yjs update can interleave and a
concurrent edit can never be clobbered. Bails if document.isDestroyed.
- Reuses the #29 preserve logic (admin-vetted embeds survive; only the non-admin's
new ones are stripped). Loop-safe (corrective update has null origin -> no
reschedule; post-strip no embed -> cheap no-op). Per-document timer cleared on
unload. onStoreDocument stays the authoritative backstop.
The irreducible residual is only the very first inbound broadcast before the
debounce fires — Hocuspocus exposes no synchronous beforeBroadcast filter.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The collab persist strip keyed to the storing connection's user, so when a
non-admin co-editor stored, it removed an admin's legitimately-authored embed
too (data loss). Now: toggle OFF still strips all (feature disabled); toggle ON
+ non-admin storer strips only NEWLY-introduced embeds and preserves those
already present in the persisted content (admin-vetted), via new helpers
collectHtmlEmbedSources + stripDisallowedHtmlEmbedNodes (identity = attrs.source,
already-vetted HTML). The ydoc reflect is now guarded by a deep-equal check so
an unrelated non-admin edit that touches no new embed doesn't churn the doc.
A non-admin still cannot add a new embed. Documents the allow-list TOCTOU
(best-effort snapshot read outside the lock; converges on next store).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The create/duplicate/import gate tests asserted gate presence via brittle
expect(SRC).toMatch(/regex/) over the source text plus a reimplemented
applyGate() stand-in, so a refactor could break the real gate while they still
passed. Rewrite both specs to execute the REAL methods (PageService.create /
duplicatePage; ImportService.importPage; FileImportTaskService.processGenericImport)
with each caller role and assert on the PERSISTED content via hasHtmlEmbedNode:
member -> stripped, admin/owner+toggle ON -> preserved, toggle OFF -> stripped
for everyone, unknown/missing role -> fail-closed. No source-regex assertions
remain.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- The security-relevant catch->not_found branch in lookupTemplate (returns
not_found instead of raw content when comment-mark stripping throws) is now
tested by forcing the strip to throw with a malformed text node, asserting no
content/marks leak.
- not_found for a soft-deleted source resolved through the REAL
filterViewerAccessiblePageIds (deletedAt-excluded), not the stubbed filter.
- Rename the misleading 'honours <=50 cap' test to reflect it only exercises
dedup (the cap lives in the DTO, never engaged in the service unit).
- Cover the onlyTemplates search filter (restricts to is_template=true).
Also fix two pre-existing failing 'should be defined' specs (search service +
controller) that couldn't resolve the @InjectKysely token via createTestingModule.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
