fix(html-embed): execute embeds on public shares; toggle is server-side kill switch
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>
This commit is contained in:
@@ -62,6 +62,16 @@ export default function HtmlEmbedView(props: NodeViewProps) {
|
|||||||
const workspace = useAtomValue(workspaceAtom);
|
const workspace = useAtomValue(workspaceAtom);
|
||||||
const htmlEmbedEnabled = workspace?.settings?.htmlEmbed === true;
|
const htmlEmbedEnabled = workspace?.settings?.htmlEmbed === true;
|
||||||
|
|
||||||
|
// Execution policy split by editor mode:
|
||||||
|
// - READ-ONLY / public-share view: the SERVER already decided whether to
|
||||||
|
// include the embed (it strips htmlEmbed from shared content when the
|
||||||
|
// workspace toggle is OFF). An anonymous viewer has no workspace and thus
|
||||||
|
// reads `htmlEmbedEnabled` as false, so we must NOT gate execution on it
|
||||||
|
// here — we execute exactly the `source` the server chose to serve.
|
||||||
|
// - EDITABLE editor (admin authoring): keep gating on the per-workspace
|
||||||
|
// toggle so an admin sees the inert placeholder when the feature is OFF.
|
||||||
|
const shouldExecute = !editor.isEditable || htmlEmbedEnabled;
|
||||||
|
|
||||||
const contentRef = useRef<HTMLDivElement | null>(null);
|
const contentRef = useRef<HTMLDivElement | null>(null);
|
||||||
const [modalOpen, setModalOpen] = useState(false);
|
const [modalOpen, setModalOpen] = useState(false);
|
||||||
const [draft, setDraft] = useState<string>(source || "");
|
const [draft, setDraft] = useState<string>(source || "");
|
||||||
@@ -72,12 +82,12 @@ export default function HtmlEmbedView(props: NodeViewProps) {
|
|||||||
// feature toggle is OFF we clear the container and inject/execute nothing.
|
// feature toggle is OFF we clear the container and inject/execute nothing.
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
if (!contentRef.current) return;
|
if (!contentRef.current) return;
|
||||||
if (htmlEmbedEnabled) {
|
if (shouldExecute) {
|
||||||
renderRawHtml(contentRef.current, source || "");
|
renderRawHtml(contentRef.current, source || "");
|
||||||
} else {
|
} else {
|
||||||
contentRef.current.innerHTML = "";
|
contentRef.current.innerHTML = "";
|
||||||
}
|
}
|
||||||
}, [source, htmlEmbedEnabled]);
|
}, [source, shouldExecute]);
|
||||||
|
|
||||||
const openEditor = useCallback(() => {
|
const openEditor = useCallback(() => {
|
||||||
setDraft(source || "");
|
setDraft(source || "");
|
||||||
@@ -116,9 +126,12 @@ export default function HtmlEmbedView(props: NodeViewProps) {
|
|||||||
</div>
|
</div>
|
||||||
)}
|
)}
|
||||||
|
|
||||||
{!htmlEmbedEnabled ? (
|
{!shouldExecute ? (
|
||||||
// Feature disabled for this workspace: never inject/execute the source.
|
// Feature disabled for this workspace AND we're in the editable editor:
|
||||||
// Show a neutral placeholder so an existing embed is visibly inert.
|
// never inject/execute the source. Show a neutral placeholder so an
|
||||||
|
// existing embed is visibly inert for the authoring admin. Read-only /
|
||||||
|
// share viewers never hit this branch (`shouldExecute` is always true
|
||||||
|
// there) — they execute exactly the source the server chose to serve.
|
||||||
<div className={classes.htmlEmbedPlaceholder}>
|
<div className={classes.htmlEmbedPlaceholder}>
|
||||||
<IconCode size={18} />
|
<IconCode size={18} />
|
||||||
<Text size="sm">
|
<Text size="sm">
|
||||||
|
|||||||
133
apps/server/src/core/share/share-html-embed.spec.ts
Normal file
133
apps/server/src/core/share/share-html-embed.spec.ts
Normal file
@@ -0,0 +1,133 @@
|
|||||||
|
import { ShareService } from './share.service';
|
||||||
|
import { hasHtmlEmbedNode } from '../../common/helpers/prosemirror/html-embed.util';
|
||||||
|
|
||||||
|
// Exercises the REAL ShareService server-authoritative htmlEmbed kill-switch for
|
||||||
|
// shared content. An anonymous public-share viewer cannot read the per-workspace
|
||||||
|
// htmlEmbed toggle, so the SERVER must decide what to serve: when the toggle is
|
||||||
|
// OFF, htmlEmbed nodes are stripped from the shared doc; when ON they are kept so
|
||||||
|
// the read-only client executes them. All repos / token service are mocked so the
|
||||||
|
// real prepareContentForShare logic runs end-to-end via getSharedPage.
|
||||||
|
|
||||||
|
const WS = 'ws-1';
|
||||||
|
const PAGE = 'page-1';
|
||||||
|
|
||||||
|
const pageContentWithEmbed = () => ({
|
||||||
|
type: 'doc',
|
||||||
|
content: [
|
||||||
|
{ type: 'paragraph', content: [{ type: 'text', text: 'shared body' }] },
|
||||||
|
{ type: 'htmlEmbed', attrs: { source: '<script>track()</script>' } },
|
||||||
|
],
|
||||||
|
});
|
||||||
|
|
||||||
|
function buildService(opts: {
|
||||||
|
// undefined => workspaceRepo.findById returns undefined (fail-closed case)
|
||||||
|
htmlEmbed?: boolean | undefined;
|
||||||
|
workspaceMissing?: boolean;
|
||||||
|
}) {
|
||||||
|
const shareRepo = { findById: jest.fn() };
|
||||||
|
|
||||||
|
const pageRepo = {
|
||||||
|
findById: jest.fn(async () => ({
|
||||||
|
id: PAGE,
|
||||||
|
workspaceId: WS,
|
||||||
|
spaceId: 'space-1',
|
||||||
|
deletedAt: null,
|
||||||
|
content: pageContentWithEmbed(),
|
||||||
|
})),
|
||||||
|
};
|
||||||
|
|
||||||
|
const pagePermissionRepo = {
|
||||||
|
hasRestrictedAncestor: jest.fn(async () => false),
|
||||||
|
};
|
||||||
|
|
||||||
|
const tokenService = {
|
||||||
|
generateAttachmentToken: jest.fn(async () => 'tok'),
|
||||||
|
};
|
||||||
|
|
||||||
|
const workspaceRepo = {
|
||||||
|
findById: jest.fn(async () =>
|
||||||
|
opts.workspaceMissing
|
||||||
|
? undefined
|
||||||
|
: { id: WS, settings: { htmlEmbed: opts.htmlEmbed } },
|
||||||
|
),
|
||||||
|
};
|
||||||
|
|
||||||
|
const service = new ShareService(
|
||||||
|
shareRepo as any,
|
||||||
|
pageRepo as any,
|
||||||
|
pagePermissionRepo as any,
|
||||||
|
{} as any, // db (unused on this path)
|
||||||
|
tokenService as any,
|
||||||
|
{} as any, // transclusionService (unused)
|
||||||
|
workspaceRepo as any,
|
||||||
|
);
|
||||||
|
|
||||||
|
// getSharedPage resolves the share via getShareForPage (a raw db query).
|
||||||
|
// Stub it so we exercise prepareContentForShare deterministically.
|
||||||
|
jest
|
||||||
|
.spyOn(service, 'getShareForPage')
|
||||||
|
.mockResolvedValue({ pageId: PAGE, key: 'k', id: 's1' } as any);
|
||||||
|
|
||||||
|
return { service, workspaceRepo };
|
||||||
|
}
|
||||||
|
|
||||||
|
describe('ShareService htmlEmbed server-authoritative kill-switch (real code)', () => {
|
||||||
|
it('toggle ON: shared content keeps the htmlEmbed (served to anonymous viewer)', async () => {
|
||||||
|
const { service } = buildService({ htmlEmbed: true });
|
||||||
|
const { page } = await service.getSharedPage(
|
||||||
|
{ pageId: PAGE } as any,
|
||||||
|
WS,
|
||||||
|
);
|
||||||
|
expect(hasHtmlEmbedNode(page.content)).toBe(true);
|
||||||
|
expect(JSON.stringify(page.content)).toContain('shared body');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('toggle OFF: htmlEmbed stripped from shared content', async () => {
|
||||||
|
const { service } = buildService({ htmlEmbed: false });
|
||||||
|
const { page } = await service.getSharedPage(
|
||||||
|
{ pageId: PAGE } as any,
|
||||||
|
WS,
|
||||||
|
);
|
||||||
|
expect(hasHtmlEmbedNode(page.content)).toBe(false);
|
||||||
|
// Non-embed content is preserved.
|
||||||
|
expect(JSON.stringify(page.content)).toContain('shared body');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('toggle ABSENT: defaults OFF and strips', async () => {
|
||||||
|
const { service } = buildService({ htmlEmbed: undefined });
|
||||||
|
const { page } = await service.getSharedPage(
|
||||||
|
{ pageId: PAGE } as any,
|
||||||
|
WS,
|
||||||
|
);
|
||||||
|
expect(hasHtmlEmbedNode(page.content)).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('workspace missing: fails closed (stripped)', async () => {
|
||||||
|
const { service } = buildService({ workspaceMissing: true });
|
||||||
|
const { page } = await service.getSharedPage(
|
||||||
|
{ pageId: PAGE } as any,
|
||||||
|
WS,
|
||||||
|
);
|
||||||
|
expect(hasHtmlEmbedNode(page.content)).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('updatePublicAttachments strips htmlEmbed when toggle OFF', async () => {
|
||||||
|
const { service } = buildService({ htmlEmbed: false });
|
||||||
|
const out = await service.updatePublicAttachments({
|
||||||
|
id: PAGE,
|
||||||
|
workspaceId: WS,
|
||||||
|
content: pageContentWithEmbed(),
|
||||||
|
} as any);
|
||||||
|
expect(hasHtmlEmbedNode(out)).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('updatePublicAttachments keeps htmlEmbed when toggle ON', async () => {
|
||||||
|
const { service } = buildService({ htmlEmbed: true });
|
||||||
|
const out = await service.updatePublicAttachments({
|
||||||
|
id: PAGE,
|
||||||
|
workspaceId: WS,
|
||||||
|
content: pageContentWithEmbed(),
|
||||||
|
} as any);
|
||||||
|
expect(hasHtmlEmbedNode(out)).toBe(true);
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -26,6 +26,11 @@ import { validate as isValidUUID } from 'uuid';
|
|||||||
import { sql } from 'kysely';
|
import { sql } from 'kysely';
|
||||||
import { TransclusionService } from '../page/transclusion/transclusion.service';
|
import { TransclusionService } from '../page/transclusion/transclusion.service';
|
||||||
import { TransclusionLookup } from '../page/transclusion/transclusion.types';
|
import { TransclusionLookup } from '../page/transclusion/transclusion.types';
|
||||||
|
import { WorkspaceRepo } from '@docmost/db/repos/workspace/workspace.repo';
|
||||||
|
import {
|
||||||
|
isHtmlEmbedFeatureEnabled,
|
||||||
|
stripHtmlEmbedNodes,
|
||||||
|
} from '../../common/helpers/prosemirror/html-embed.util';
|
||||||
|
|
||||||
@Injectable()
|
@Injectable()
|
||||||
export class ShareService {
|
export class ShareService {
|
||||||
@@ -38,8 +43,22 @@ export class ShareService {
|
|||||||
@InjectKysely() private readonly db: KyselyDB,
|
@InjectKysely() private readonly db: KyselyDB,
|
||||||
private readonly tokenService: TokenService,
|
private readonly tokenService: TokenService,
|
||||||
private readonly transclusionService: TransclusionService,
|
private readonly transclusionService: TransclusionService,
|
||||||
|
private readonly workspaceRepo: WorkspaceRepo,
|
||||||
) {}
|
) {}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Resolve whether the htmlEmbed feature toggle is ON for a workspace.
|
||||||
|
* Fail-closed: a missing workspace (or absent/non-true setting) => OFF, so
|
||||||
|
* share content gets the embed stripped when we can't positively confirm the
|
||||||
|
* feature is enabled.
|
||||||
|
*/
|
||||||
|
private async isHtmlEmbedEnabledForWorkspace(
|
||||||
|
workspaceId: string,
|
||||||
|
): Promise<boolean> {
|
||||||
|
const workspace = await this.workspaceRepo.findById(workspaceId);
|
||||||
|
return isHtmlEmbedFeatureEnabled(workspace?.settings);
|
||||||
|
}
|
||||||
|
|
||||||
async getShareTree(shareId: string, workspaceId: string) {
|
async getShareTree(shareId: string, workspaceId: string) {
|
||||||
const share = await this.shareRepo.findById(shareId);
|
const share = await this.shareRepo.findById(shareId);
|
||||||
if (!share || share.workspaceId !== workspaceId) {
|
if (!share || share.workspaceId !== workspaceId) {
|
||||||
@@ -360,6 +379,11 @@ export class ShareService {
|
|||||||
workspaceId,
|
workspaceId,
|
||||||
);
|
);
|
||||||
|
|
||||||
|
// Resolve the workspace htmlEmbed toggle once for this share request; all
|
||||||
|
// transcluded items belong to the same workspace as the host share.
|
||||||
|
const htmlEmbedEnabled =
|
||||||
|
await this.isHtmlEmbedEnabledForWorkspace(workspaceId);
|
||||||
|
|
||||||
// Sanitize each item's content for public delivery
|
// Sanitize each item's content for public delivery
|
||||||
// generate per-attachment tokens scoped to the source page
|
// generate per-attachment tokens scoped to the source page
|
||||||
// and strip comment marks.
|
// and strip comment marks.
|
||||||
@@ -370,6 +394,7 @@ export class ShareService {
|
|||||||
item.content,
|
item.content,
|
||||||
item.sourcePageId,
|
item.sourcePageId,
|
||||||
workspaceId,
|
workspaceId,
|
||||||
|
htmlEmbedEnabled,
|
||||||
);
|
);
|
||||||
return { ...item, content: doc?.toJSON() ?? item.content };
|
return { ...item, content: doc?.toJSON() ?? item.content };
|
||||||
}),
|
}),
|
||||||
@@ -417,10 +442,14 @@ export class ShareService {
|
|||||||
}
|
}
|
||||||
|
|
||||||
async updatePublicAttachments(page: Page): Promise<any> {
|
async updatePublicAttachments(page: Page): Promise<any> {
|
||||||
|
const htmlEmbedEnabled = await this.isHtmlEmbedEnabledForWorkspace(
|
||||||
|
page.workspaceId,
|
||||||
|
);
|
||||||
const doc = await this.prepareContentForShare(
|
const doc = await this.prepareContentForShare(
|
||||||
page.content,
|
page.content,
|
||||||
page.id,
|
page.id,
|
||||||
page.workspaceId,
|
page.workspaceId,
|
||||||
|
htmlEmbedEnabled,
|
||||||
);
|
);
|
||||||
return doc?.toJSON() ?? page.content;
|
return doc?.toJSON() ?? page.content;
|
||||||
}
|
}
|
||||||
@@ -441,6 +470,13 @@ export class ShareService {
|
|||||||
* not leak structure (existence, location, count, resolved state, or
|
* not leak structure (existence, location, count, resolved state, or
|
||||||
* comment ids) to public viewers.
|
* comment ids) to public viewers.
|
||||||
*
|
*
|
||||||
|
* 3. Strip `htmlEmbed` nodes when the workspace feature toggle is OFF. This
|
||||||
|
* makes the toggle a SERVER-AUTHORITATIVE kill-switch for shared content:
|
||||||
|
* when OFF the embed is never served to the anonymous viewer (who can't
|
||||||
|
* read the per-workspace toggle), when ON the embed is served so the
|
||||||
|
* read-only client executes it. `htmlEmbedEnabled` is resolved fail-closed
|
||||||
|
* by the callers (missing workspace => OFF => strip).
|
||||||
|
*
|
||||||
* Both share-content paths — the host page (`updatePublicAttachments`) and
|
* Both share-content paths — the host page (`updatePublicAttachments`) and
|
||||||
* the share-scoped transclusion lookup (`lookupTransclusionForShare`) —
|
* the share-scoped transclusion lookup (`lookupTransclusionForShare`) —
|
||||||
* call into this single helper so the two paths can never drift on
|
* call into this single helper so the two paths can never drift on
|
||||||
@@ -450,8 +486,16 @@ export class ShareService {
|
|||||||
content: unknown,
|
content: unknown,
|
||||||
attachmentOwnerPageId: string,
|
attachmentOwnerPageId: string,
|
||||||
workspaceId: string,
|
workspaceId: string,
|
||||||
|
htmlEmbedEnabled: boolean,
|
||||||
): Promise<Node | null> {
|
): Promise<Node | null> {
|
||||||
const pmJson = getProsemirrorContent(content);
|
let pmJson = getProsemirrorContent(content);
|
||||||
|
|
||||||
|
// Kill-switch: when the workspace toggle is OFF, never serve htmlEmbed
|
||||||
|
// nodes to public viewers. Strip before tokenizing/serializing.
|
||||||
|
if (!htmlEmbedEnabled) {
|
||||||
|
pmJson = stripHtmlEmbedNodes(pmJson);
|
||||||
|
}
|
||||||
|
|
||||||
const attachmentIds = getAttachmentIds(pmJson);
|
const attachmentIds = getAttachmentIds(pmJson);
|
||||||
|
|
||||||
const tokenMap = new Map<string, string>();
|
const tokenMap = new Map<string, string>();
|
||||||
|
|||||||
Reference in New Issue
Block a user