The three collect*FromPmJson collectors shared the same recursion (and the #55
depth cap) but were copy-pasted, so a future edit could diverge them. Extract a
generic collectNodes(doc, {type, map, key, lastWins, skipChildrenOfType}) and
reimplement all three on it, byte-output-identical (transclusions last-wins;
references/embeds first-wins + transclusionSource skip). Documents (not removes)
the write-only page_template_references graph and the near-duplicate client
lookup-context as tracked follow-ups, per the issue's guidance.
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>
resolveRoleForRequest and resolveShareRole duplicated the security invariant
'role exists, not soft-deleted, enabled, workspace-scoped, else null'. Move it to
AiAgentRoleRepo.findLiveEnabled(id, workspaceId) (deletedAt IS NULL + enabled +
workspace scope) and have both services call it, preserving each one's roleId
derivation + null handling. (describeProviderError half of #95 was done earlier.)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
emitTreeEvent and emitCommentEvent were byte-identical (same room resolution,
spaceHasRestrictions gate, hasRestrictedAncestor, authorized-only vs broadcast
fallback). Collapse the body into one private emitRestrictedAwareToSpace; both
stay thin wrappers with unchanged signatures, so the restriction-routing gate
lives in exactly one place. Adds coverage for the comment entry point.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
McpService.enforceBasicLoginGate re-implements AuthController.login's pre-token
SSO/MFA gate; silent drift would re-open the bypass. Add an AST contract test
(comments stripped) asserting BOTH method bodies contain validateSsoEnforcement,
the EE-MFA require, and checkMfaRequirements — so dropping the gate from either
side fails CI. Test-only (no core/auth refactor).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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>
16 suites were disabled via testPathIgnorePatterns due to two root causes: lib0
ESM not transformed (the @hocuspocus/server -> lib0/decoding.js chain) and stock
'should be defined' specs built via Test.createTestingModule without providers.
Add lib0 to transformIgnorePatterns; convert the 14 DI placeholders to direct
new X(...) instantiation with stub deps (keeping a real construct smoke test);
re-enable the suites. Also updates the public-share limiter test to assert the
fail-closed behavior from #62 (surfaced now that the suite runs). Full server
suite: 67 passed, 689 tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fix doc drift: /mcp per-user auth + X-MCP-Token (was 'service account + optional
MCP_TOKEN'); CI builds :develop on push to develop (was main); add
page_template_references to the fork-tables list + is_template schema; mark
arbitrary HTML embed as shipped (was in-progress plan); remove the dead
page-templates-plan.md README link and move Page templates to implemented.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The shared MCP_TOKEN guard moved from 'Authorization: Bearer <MCP_TOKEN>' to the
X-MCP-Token header (Authorization is now per-user Basic/Bearer), silently breaking
existing /mcp clients. Document it as a Breaking Change in CHANGELOG (reconfigure
to X-MCP-Token). Add a once-per-process migration warning: when MCP_TOKEN is set,
no x-mcp-token is present, and Authorization carries the old 'Bearer <MCP_TOKEN>',
log a hint to migrate — without changing the auth decision (still rejected) or
logging the token value.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Assessment of the page-embed depth/cycle cap: the server /pages/template/lookup
returns FLAT single-level content and does NOT recurse into embedded pages — the
recursive expansion + the PAGE_EMBED_MAX_DEPTH cap are entirely client render
concerns, and a scripted client is already bounded by the per-user throttle
(30/min) + the ArrayMaxSize(50) per-call cap. So no server-side depth guard is
needed; documented at lookupTemplate so future readers don't add a redundant one
or assume server recursion exists.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
handleMessage became a no-op and PageWsListener intentionally ignored
PAGE_UPDATED, so a rename/icon change (client operation:updateOne) was no longer
rebroadcast -> other clients saw stale title/icon in the sidebar+breadcrumbs
until a reload (create/duplicate/restore were covered; updateOne regressed).
Add a server-authoritative onPageUpdated handler: PageService.update detects a
real title/icon change (DTO carries the field AND value differs; no-op/content-
only saves excluded) and attaches a treeUpdate snapshot to PAGE_UPDATED; the
listener broadcasts a tree updateOne via the restriction-aware emitTreeEvent
(so a restricted page's title never leaks). Content-only saves attach nothing.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
sidebar-pages-tree.spec tested a LOCAL COPY of the tree-shaping (so a regression
in the real getSidebarPagesTree was invisible) and justified it with a false
jest-config claim (the ^src mapping exists). Extract the pure shaping into
shapeSidebarPagesTree(); the service now calls it and the spec imports the REAL
helper. Behavior unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The concurrent-soft-delete guard was already covered; add the missing assertion
that update() returns toView(updated) from the post-update re-fetch (full
AgentRoleView shape, distinct second findById row), so a regression returning the
stale pre-update view or leaking columns is caught.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The anonymous share page-fetch tool's positive branch (sanitize via
updatePublicAttachments then jsonToMarkdown before returning to the model) was
untested, so a dropped/reordered sanitizer would ship a comment-mark/raw-
attachment leak with green tests. Add a positive-branch test pinning the
sanitizer call + that markdown derives from sanitized content, and a soft-deleted
test asserting a generic error with no content fetch.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
invalidateSpaceRestrictionCache has no callers because no restriction-mutation
path exists yet (PagePermissionRepo mutators are uncalled; there is no
restrict/grant/revoke endpoint), so the 30s spaceHasRestrictions cache could
serve a stale 'no restrictions' verdict. Until a mutation endpoint exists to
wire the direct invalidation, lower the TTL (30s -> 3s) to bound the worst-case
window; the invalidation primitive is kept for that future endpoint.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
chatModel was a free string accepted with empty/garbage values, failing only at
runtime as a provider 503; tighten it (trim + non-empty + max 200). Driver was
already @IsIn(AI_DRIVERS). Collapse the client driver list to one AI_DRIVER_VALUES
source and add a contract test that reads the server AI_DRIVERS and fails on
client/server drift.
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>
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>
