fix(share): SEO route must not leak a restricted page's title (#159)
`ShareSeoController.getShare` resolved the inherited share with the RAW `getShareForPage`, which does NOT run the restricted-ancestor gate. So for a page shared with includeSubPages whose descendant is permission-restricted, the SEO route served that descendant's real title in <title>/og:title/twitter:title to anonymous visitors and crawlers — even though the content API returns 404 for it (red-team finding #3). Funnel the SEO path through the canonical `resolveReadableSharePage` boundary (the single place that checks `hasRestrictedAncestor`): a non-readable page now serves the plain SPA index with no meta. Also honour `isSharingAllowed` — a share whose workspace/space sharing toggle was flipped off after creation no longer leaks its title via SEO. Title comes from the server-resolved page; `buildShareMetaHtml` already emits robots=noindex when the share opted out of indexing. Tests (controller routing, fs spied at call time so bcrypt's native loader is untouched): non-readable page => plain index, no title; sharing-disabled => plain index; readable+indexing => title + og:title, no noindex; readable+no- indexing => noindex. Asserts getShareForPage is never called by the SEO path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
claude code agent 227
133
apps/server/src/core/share/share-seo.controller.routing.spec.ts
Normal file
133
apps/server/src/core/share/share-seo.controller.routing.spec.ts
Normal file
@@ -0,0 +1,133 @@
|
||||
import * as fs from 'node:fs';
|
||||
import { ShareSeoController } from './share-seo.controller';
|
||||
|
||||
/**
|
||||
* Routing guard for ShareSeoController.getShare (red-team finding #3).
|
||||
*
|
||||
* The SEO route must NOT leak a shared page's <title>/og:title to anonymous
|
||||
* visitors / crawlers when the page is not publicly readable. It previously
|
||||
* called the raw `getShareForPage`, which skips the restricted-ancestor gate, so
|
||||
* a permission-restricted descendant of an includeSubPages share leaked its
|
||||
* title. The fix funnels through `resolveReadableSharePage` (the canonical gate)
|
||||
* AND honours `isSharingAllowed`. These tests pin that routing: a non-readable
|
||||
* page or sharing-disabled space serves the plain SPA index (no title); only a
|
||||
* readable, still-shared page gets meta tags.
|
||||
*/
|
||||
|
||||
const SECRET_TITLE = 'Restricted Quarterly Numbers';
|
||||
const INDEX_HTML = `<!doctype html><html><head><title>App</title><!--meta-tags--></head><body></body></html>`;
|
||||
const STREAM_SENTINEL = { __isStream: true } as unknown as fs.ReadStream;
|
||||
|
||||
// Stub fs at CALL time (jest.spyOn), NOT module load (jest.mock): the controller
|
||||
// transitively pulls bcrypt, whose native module is located by node-gyp-build
|
||||
// reading the filesystem at import time — a module-level fs mock breaks that.
|
||||
beforeEach(() => {
|
||||
jest.spyOn(fs, 'existsSync').mockReturnValue(true);
|
||||
jest.spyOn(fs, 'readFileSync').mockReturnValue(INDEX_HTML);
|
||||
jest.spyOn(fs, 'createReadStream').mockReturnValue(STREAM_SENTINEL);
|
||||
});
|
||||
afterEach(() => jest.restoreAllMocks());
|
||||
|
||||
function makeRes() {
|
||||
const res: any = {
|
||||
sent: undefined as unknown,
|
||||
type: jest.fn(() => res),
|
||||
send: jest.fn((v: unknown) => {
|
||||
res.sent = v;
|
||||
}),
|
||||
};
|
||||
return res;
|
||||
}
|
||||
|
||||
function makeController(opts: {
|
||||
resolved: { share: any; page: any } | null;
|
||||
sharingAllowed?: boolean;
|
||||
}) {
|
||||
const shareService = {
|
||||
resolveReadableSharePage: jest.fn(async () => opts.resolved),
|
||||
isSharingAllowed: jest.fn(async () => opts.sharingAllowed ?? true),
|
||||
// Must NEVER be used by the SEO path anymore (the bypass is the bug).
|
||||
getShareForPage: jest.fn(async () => {
|
||||
throw new Error('getShareForPage must not be called by the SEO path');
|
||||
}),
|
||||
};
|
||||
const workspaceRepo = {
|
||||
findFirst: async () => ({ id: 'ws-1', settings: {} }),
|
||||
};
|
||||
const environmentService = { isSelfHosted: () => true };
|
||||
const controller = new ShareSeoController(
|
||||
shareService as any,
|
||||
workspaceRepo as any,
|
||||
environmentService as any,
|
||||
);
|
||||
return { controller, shareService };
|
||||
}
|
||||
|
||||
const req: any = { raw: { headers: { host: 'self' } } };
|
||||
|
||||
describe('ShareSeoController.getShare routing (#3 title-leak gate)', () => {
|
||||
it('serves the plain index (NO title) when the page is not publicly readable', async () => {
|
||||
const { controller, shareService } = makeController({ resolved: null });
|
||||
const res = makeRes();
|
||||
|
||||
await controller.getShare(res, req, 'share-key', `slug-pageB`);
|
||||
|
||||
// The restricted-ancestor gate ran; the raw bypass did not.
|
||||
expect(shareService.resolveReadableSharePage).toHaveBeenCalled();
|
||||
expect(shareService.getShareForPage).not.toHaveBeenCalled();
|
||||
// The plain index stream was sent — NOT the title-bearing meta HTML.
|
||||
expect(res.sent).toBe(STREAM_SENTINEL);
|
||||
});
|
||||
|
||||
it('serves the plain index when sharing was disabled at the workspace/space level', async () => {
|
||||
const { controller } = makeController({
|
||||
resolved: {
|
||||
share: { spaceId: 'sp-1', searchIndexing: true },
|
||||
page: { title: SECRET_TITLE },
|
||||
},
|
||||
sharingAllowed: false,
|
||||
});
|
||||
const res = makeRes();
|
||||
|
||||
await controller.getShare(res, req, 'share-key', 'slug-pageB');
|
||||
|
||||
// The plain index stream was sent, so the restricted title never reached
|
||||
// the response (it is only ever interpolated into the meta HTML string).
|
||||
expect(res.sent).toBe(STREAM_SENTINEL);
|
||||
expect(res.sent).not.toBe(SECRET_TITLE);
|
||||
});
|
||||
|
||||
it('injects the title + meta for a readable, still-shared page', async () => {
|
||||
const { controller } = makeController({
|
||||
resolved: {
|
||||
share: { spaceId: 'sp-1', searchIndexing: true },
|
||||
page: { title: 'Public Handbook' },
|
||||
},
|
||||
sharingAllowed: true,
|
||||
});
|
||||
const res = makeRes();
|
||||
|
||||
await controller.getShare(res, req, 'share-key', 'slug-pageA');
|
||||
|
||||
expect(typeof res.sent).toBe('string');
|
||||
expect(res.sent as string).toContain('<title>Public Handbook</title>');
|
||||
expect(res.sent as string).toContain('og:title');
|
||||
// searchIndexing on => crawlable (no noindex).
|
||||
expect(res.sent as string).not.toContain('content="noindex"');
|
||||
});
|
||||
|
||||
it('adds robots=noindex when the share opted out of search indexing', async () => {
|
||||
const { controller } = makeController({
|
||||
resolved: {
|
||||
share: { spaceId: 'sp-1', searchIndexing: false },
|
||||
page: { title: 'Internal Notes' },
|
||||
},
|
||||
sharingAllowed: true,
|
||||
});
|
||||
const res = makeRes();
|
||||
|
||||
await controller.getShare(res, req, 'share-key', 'slug-pageA');
|
||||
|
||||
expect(res.sent as string).toContain('content="noindex"');
|
||||
});
|
||||
});
|
||||
@@ -63,19 +63,38 @@ export class ShareSeoController {
|
||||
|
||||
const pageId = this.extractPageSlugId(pageSlug);
|
||||
|
||||
const share = await this.shareService.getShareForPage(
|
||||
// Funnel through the canonical readable-share boundary (NOT the raw
|
||||
// getShareForPage) so the restricted-ancestor gate runs: a permission-
|
||||
// restricted descendant of an includeSubPages share must NOT leak its
|
||||
// title to anonymous visitors / crawlers (red-team finding #3). null =>
|
||||
// not publicly readable => serve the plain SPA index with no meta.
|
||||
const resolved = await this.shareService.resolveReadableSharePage(
|
||||
undefined,
|
||||
pageId,
|
||||
workspace.id,
|
||||
);
|
||||
|
||||
if (!share) {
|
||||
if (!resolved) {
|
||||
return this.sendIndex(indexFilePath, res);
|
||||
}
|
||||
|
||||
// Honour a workspace/space-level sharing toggle flipped off AFTER this
|
||||
// share was created: the content API gates on isSharingAllowed, so the SEO
|
||||
// path must too or it keeps serving the title for a no-longer-shared page.
|
||||
const sharingAllowed = await this.shareService.isSharingAllowed(
|
||||
workspace.id,
|
||||
resolved.share.spaceId,
|
||||
);
|
||||
if (!sharingAllowed) {
|
||||
return this.sendIndex(indexFilePath, res);
|
||||
}
|
||||
|
||||
const html = fs.readFileSync(indexFilePath, 'utf8');
|
||||
// Title of the PAGE being viewed (server-resolved), and noindex unless the
|
||||
// share opted into search indexing (buildShareMetaHtml injects it).
|
||||
let transformedHtml = buildShareMetaHtml(html, {
|
||||
title: share?.sharedPage.title,
|
||||
searchIndexing: share.searchIndexing,
|
||||
title: resolved.page.title,
|
||||
searchIndexing: resolved.share.searchIndexing,
|
||||
});
|
||||
|
||||
// Deliberate same-origin tracker surface: this is the ONE place where an
|
||||
|
||||
Reference in New Issue
Block a user