From 4109b2ef7ff5ee54b286628e5e4fa1b0de82194d Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 11 Jul 2026 22:45:28 +0300 Subject: [PATCH] =?UTF-8?q?fix(security):=20share-alias=20reassign=20409?= =?UTF-8?q?=20=E2=80=94=20=D0=B3=D0=B5=D0=B9=D1=82=20=D1=80=D0=B0=D1=81?= =?UTF-8?q?=D0=BA=D1=80=D1=8B=D1=82=D0=B8=D1=8F=20=D0=BF=D0=BE=20view-?= =?UTF-8?q?=D0=BF=D1=80=D0=B0=D0=B2=D1=83=20(=D0=B4=D0=BE=D0=B2=D0=B5?= =?UTF-8?q?=D0=B4=D0=B5=D0=BD=D0=B8=D0=B5=20#495=20=D0=BA.4)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Коммит 4 закрыл утечку currentPageId на анонимном /availability, но эквивалентная (и более широкая) дыра оставалась на POST /aliases/set (reassign): при занятом алиасе и confirmReassign=false 409 отдавал currentPageId И currentPageTitle ЦЕЛЕВОЙ страницы, а контроллер гейтил только validateCanEdit на ИСХОДНОЙ странице. Любой участник с одной редактируемой+расшаренной страницей мог перебирать имена алиасов и мапить их на (id, title) чужих страниц без права просмотра — тот же класс перечисления, плюс ещё и заголовок. Фикс: setAlias теперь гейтит раскрытие. currentPageId НЕ отдаётся никогда (клиент им не пользуется, это перечислимая идентичность). currentPageTitle — только если validateCanView(целевая, user) проходит; иначе голый «занят» (клиент и так показывает generic confirm-модалку без заголовка — UX не ломается). Гейт живёт в сервисе, где строится раскрытие (PageAccessService — @Global, без цикла); контроллер прокидывает user. Поправлен неточный коммент checkAvailability. Тесты: viewer → 409 с title, БЕЗ id; не-viewer → 409 без title и без id. Mutation-verify: вернул утечку (id + безусловный title) → оба теста краснеют. Контроллер-спек и int-spec обновлены под новую сигнатуру; tsc чист. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../core/share/share-alias.controller.spec.ts | 3 + .../src/core/share/share-alias.controller.ts | 3 + .../core/share/share-alias.service.spec.ts | 83 +++++++++++++++++-- .../src/core/share/share-alias.service.ts | 47 ++++++++--- .../share-alias-one-per-page.int-spec.ts | 18 ++++ 5 files changed, 137 insertions(+), 17 deletions(-) diff --git a/apps/server/src/core/share/share-alias.controller.spec.ts b/apps/server/src/core/share/share-alias.controller.spec.ts index 0ff1d2fc..eb030eac 100644 --- a/apps/server/src/core/share/share-alias.controller.spec.ts +++ b/apps/server/src/core/share/share-alias.controller.spec.ts @@ -133,6 +133,9 @@ describe('ShareAliasController authz gates', () => { creatorId: 'u-1', alias: 'promo', confirmReassign: true, + // The requesting user is forwarded so setAlias can gate the reassign + // 409 title disclosure on target-page view permission (#495). + user, }); expect(result).toEqual({ id: 'alias-1' }); }); diff --git a/apps/server/src/core/share/share-alias.controller.ts b/apps/server/src/core/share/share-alias.controller.ts index 5c50f8b7..3b8e81db 100644 --- a/apps/server/src/core/share/share-alias.controller.ts +++ b/apps/server/src/core/share/share-alias.controller.ts @@ -79,6 +79,9 @@ export class ShareAliasController { creatorId: user.id, alias: dto.alias, confirmReassign: dto.confirmReassign, + // Gates whether the reassign 409 may reveal the current target's title + // (view-permission check on that page) — see setAlias (#495). + user, }); } diff --git a/apps/server/src/core/share/share-alias.service.spec.ts b/apps/server/src/core/share/share-alias.service.spec.ts index 63f16a1c..23931d52 100644 --- a/apps/server/src/core/share/share-alias.service.spec.ts +++ b/apps/server/src/core/share/share-alias.service.spec.ts @@ -1,4 +1,8 @@ -import { BadRequestException, ConflictException } from '@nestjs/common'; +import { + BadRequestException, + ConflictException, + ForbiddenException, +} from '@nestjs/common'; import { NoResultError } from 'kysely'; import { ShareAliasService } from './share-alias.service'; @@ -7,6 +11,8 @@ import { ShareAliasService } from './share-alias.service'; * 409 reassign guard, uniqueness-race handling, availability probe, and the * request-time readable-target resolution (which re-runs the share boundary). */ +const USER = { id: 'u-1' } as any; + describe('ShareAliasService', () => { // Sentinel handed to repo calls so tests can assert they ran inside the tx. const trx = { __trx: true }; @@ -27,6 +33,10 @@ describe('ShareAliasService', () => { resolveReadableSharePage: jest.fn(), isSharingAllowed: jest.fn(), }; + // Default: the requester CAN view the target page (validateCanView resolves), + // so the reassign 409 may disclose its title. Tests override to reject to + // assert the no-leak path. + const pageAccessService = { validateCanView: jest.fn().mockResolvedValue(undefined) }; // Fake kysely db: only .transaction().execute(cb) is used by setAlias. const db = { transaction: jest.fn(() => ({ @@ -37,9 +47,10 @@ describe('ShareAliasService', () => { shareAliasRepo as any, pageRepo as any, shareService as any, + pageAccessService as any, db as any, ); - return { service, shareAliasRepo, pageRepo, shareService, db }; + return { service, shareAliasRepo, pageRepo, shareService, pageAccessService, db }; } describe('setAlias', () => { @@ -50,6 +61,7 @@ describe('ShareAliasService', () => { workspaceId: 'ws-1', pageId: 'p-1', creatorId: 'u-1', + user: USER, alias: 'A', // too short + uppercase }), ).rejects.toBeInstanceOf(BadRequestException); @@ -66,6 +78,7 @@ describe('ShareAliasService', () => { workspaceId: 'ws-1', pageId: 'p-1', creatorId: 'u-1', + user: USER, alias: ' My Page ', }); @@ -114,6 +127,7 @@ describe('ShareAliasService', () => { workspaceId: 'ws-1', pageId: 'p-1', creatorId: 'u-1', + user: USER, alias: 'ted', }); @@ -144,6 +158,7 @@ describe('ShareAliasService', () => { workspaceId: 'ws-1', pageId: 'p-1', creatorId: 'u-1', + user: USER, alias: 'foo', }); @@ -179,6 +194,7 @@ describe('ShareAliasService', () => { workspaceId: 'ws-1', pageId: 'p-1', creatorId: 'u-1', + user: USER, alias: 'new', }); @@ -190,30 +206,77 @@ describe('ShareAliasService', () => { ); }); - it('throws 409 with current target when name is taken and not confirmed', async () => { - const { service, shareAliasRepo, pageRepo } = makeService(); + it('throws 409 with the target TITLE (never its id) when the requester CAN view it', async () => { + const { service, shareAliasRepo, pageRepo, pageAccessService } = + makeService(); shareAliasRepo.findByAliasAndWorkspace.mockResolvedValue({ id: 'a-1', alias: 'foo', pageId: 'p-other', }); pageRepo.findById.mockResolvedValue({ id: 'p-other', title: 'Other' }); + pageAccessService.validateCanView.mockResolvedValue(undefined); // can view try { await service.setAlias({ workspaceId: 'ws-1', pageId: 'p-1', creatorId: 'u-1', + user: USER, alias: 'foo', }); fail('expected ConflictException'); } catch (err) { expect(err).toBeInstanceOf(ConflictException); - expect((err as ConflictException).getResponse()).toMatchObject({ + const body = (err as ConflictException).getResponse(); + expect(body).toMatchObject({ code: 'ALIAS_REASSIGN_REQUIRED', - currentPageId: 'p-other', currentPageTitle: 'Other', }); + // SECURITY (#495): the page id is NEVER disclosed, even to a viewer. + expect(body).not.toHaveProperty('currentPageId'); + expect(pageAccessService.validateCanView).toHaveBeenCalledWith( + expect.objectContaining({ id: 'p-other' }), + USER, + ); + } + expect(shareAliasRepo.updatePageId).not.toHaveBeenCalled(); + }); + + it('throws 409 WITHOUT the title or id when the requester CANNOT view the target (#495)', async () => { + const { service, shareAliasRepo, pageRepo, pageAccessService } = + makeService(); + shareAliasRepo.findByAliasAndWorkspace.mockResolvedValue({ + id: 'a-1', + alias: 'foo', + pageId: 'p-secret', + }); + pageRepo.findById.mockResolvedValue({ id: 'p-secret', title: 'Secret' }); + // No view permission on the target page -> validateCanView throws. + pageAccessService.validateCanView.mockRejectedValue( + new ForbiddenException(), + ); + + try { + await service.setAlias({ + workspaceId: 'ws-1', + pageId: 'p-1', + creatorId: 'u-1', + user: USER, + alias: 'foo', + }); + fail('expected ConflictException'); + } catch (err) { + expect(err).toBeInstanceOf(ConflictException); + const body = (err as ConflictException).getResponse() as Record< + string, + unknown + >; + expect(body).toMatchObject({ code: 'ALIAS_REASSIGN_REQUIRED' }); + // The enumeration hole: neither the id nor the title of a page the + // requester cannot see may leak. + expect(body).not.toHaveProperty('currentPageId'); + expect(body.currentPageTitle ?? null).toBeNull(); } expect(shareAliasRepo.updatePageId).not.toHaveBeenCalled(); }); @@ -231,6 +294,7 @@ describe('ShareAliasService', () => { workspaceId: 'ws-1', pageId: 'p-1', creatorId: 'u-1', + user: USER, alias: 'foo', confirmReassign: true, }); @@ -269,6 +333,7 @@ describe('ShareAliasService', () => { workspaceId: 'ws-1', pageId: 'p-1', creatorId: 'u-1', + user: USER, alias: 'foo', }); fail('expected ConflictException'); @@ -294,6 +359,7 @@ describe('ShareAliasService', () => { workspaceId: 'ws-1', pageId: 'p-1', creatorId: 'u-1', + user: USER, alias: 'foo', }); fail('expected ConflictException'); @@ -317,6 +383,7 @@ describe('ShareAliasService', () => { workspaceId: 'ws-1', pageId: 'p-1', creatorId: 'u-1', + user: USER, alias: 'foo', }); fail('expected ConflictException'); @@ -346,6 +413,7 @@ describe('ShareAliasService', () => { workspaceId: 'ws-1', pageId: 'p-1', creatorId: 'u-1', + user: USER, alias: 'foo', }); fail('expected ConflictException'); @@ -375,6 +443,7 @@ describe('ShareAliasService', () => { workspaceId: 'ws-1', pageId: 'p-1', creatorId: 'u-1', + user: USER, alias: 'foo', confirmReassign: true, }); @@ -406,6 +475,7 @@ describe('ShareAliasService', () => { workspaceId: 'ws-1', pageId: 'p-1', creatorId: 'u-1', + user: USER, alias: 'ted', }); fail('expected ConflictException'); @@ -428,6 +498,7 @@ describe('ShareAliasService', () => { workspaceId: 'ws-1', pageId: 'p-1', creatorId: 'u-1', + user: USER, alias: 'foo', }), ).rejects.toBeInstanceOf(BadRequestException); diff --git a/apps/server/src/core/share/share-alias.service.ts b/apps/server/src/core/share/share-alias.service.ts index 6af94692..bb3fa5cd 100644 --- a/apps/server/src/core/share/share-alias.service.ts +++ b/apps/server/src/core/share/share-alias.service.ts @@ -7,7 +7,8 @@ import { import { ShareAliasRepo } from '@docmost/db/repos/share-alias/share-alias.repo'; import { PageRepo } from '@docmost/db/repos/page/page.repo'; import { ShareService } from './share.service'; -import { Page, ShareAlias } from '@docmost/db/types/entity.types'; +import { PageAccessService } from '../page/page-access/page-access.service'; +import { Page, ShareAlias, User } from '@docmost/db/types/entity.types'; import { isValidShareAlias, normalizeShareAlias } from './share-alias.util'; import { InjectKysely } from 'nestjs-kysely'; import { KyselyDB } from '@docmost/db/types/kysely.types'; @@ -43,6 +44,7 @@ export class ShareAliasService { private readonly shareAliasRepo: ShareAliasRepo, private readonly pageRepo: PageRepo, private readonly shareService: ShareService, + private readonly pageAccessService: PageAccessService, @InjectKysely() private readonly db: KyselyDB, ) {} @@ -55,9 +57,13 @@ export class ShareAliasService { * `/l/` link survives * - name already points at pageId -> no-op (idempotent) * - name points at ANOTHER page -> the "swap". Without confirmReassign - * we throw 409 carrying the current target so the client can confirm; - * with it we UPDATE the single row's page_id (every /l/ link - * follows the 302 to the new page instantly — no stale cache). + * we throw 409 so the client can confirm. SECURITY (#495): the 409 reveals + * the current target's title ONLY when `user` may VIEW that page, and never + * its id — otherwise any member with one editable+shared page could iterate + * alias names with confirmReassign=false and map them to (id, title) of + * pages they cannot see. With confirmReassign we UPDATE the single row's + * page_id (every /l/ link follows the 302 to the new page instantly + * — no stale cache). * * To keep the invariant self-healing we DELETE every other alias row still * pointing at this page (a legacy duplicate, or the target page's own former @@ -77,8 +83,12 @@ export class ShareAliasService { creatorId: string; alias: string; confirmReassign?: boolean; + // The requesting user — used ONLY to gate whether the reassign 409 may reveal + // the current target page's title (view-permission check). Not an authz gate + // for the write itself (the controller already validated edit on `pageId`). + user: User; }): Promise { - const { workspaceId, pageId, creatorId, confirmReassign } = opts; + const { workspaceId, pageId, creatorId, confirmReassign, user } = opts; const alias = normalizeShareAlias(opts.alias); if (!isValidShareAlias(alias)) { throw new BadRequestException( @@ -97,14 +107,30 @@ export class ShareAliasService { // The name is occupied by a DIFFERENT (or dangling) target page. if (byName && byName.pageId !== pageId) { if (!confirmReassign) { + // SECURITY (#495): only disclose the current target's TITLE, and only + // when the requester may VIEW that page. Never disclose its id (the + // client's confirm-reassign UX doesn't use it, and it is an enumerable + // identity). A member with one editable+shared page must NOT be able to + // iterate alias names and map them to (id, title) of pages they cannot + // see. When view is denied (or the alias is dangling) the 409 is the + // bare "occupied" fact — the client still shows a generic confirm modal. const currentPage = byName.pageId ? await this.pageRepo.findById(byName.pageId) : null; + let currentPageTitle: string | null = null; + if (currentPage) { + try { + await this.pageAccessService.validateCanView(currentPage, user); + currentPageTitle = currentPage.title ?? null; + } catch { + // No view permission on the target -> do not reveal its title. + currentPageTitle = null; + } + } throw new ConflictException({ message: 'Alias already in use', code: 'ALIAS_REASSIGN_REQUIRED', - currentPageId: byName.pageId, - currentPageTitle: currentPage?.title ?? null, + currentPageTitle, }); } // Confirmed swap. ORDER MATTERS: the partial unique index on @@ -237,10 +263,9 @@ export class ShareAliasService { // to ANY authenticated workspace member, with no view-permission check on that // page. An attacker could enumerate alias names and map them to page ids they // have no access to. The taken/free bit is all the "is this address free" - // probe needs; the reassign flow surfaces the target's title only AFTER a real - // setAlias attempt (the 409 ALIAS_REASSIGN_REQUIRED path), which is access- - // gated. If a caller ever needs the target page id, it must be returned only - // behind an explicit `validateCanView` on that page. + // probe needs. The reassign flow (setAlias 409) may surface the target's + // TITLE, but only behind a `validateCanView` on that page (see setAlias); it + // never returns the page id. return { alias, valid: true, diff --git a/apps/server/test/integration/share-alias-one-per-page.int-spec.ts b/apps/server/test/integration/share-alias-one-per-page.int-spec.ts index b372a996..440f5a88 100644 --- a/apps/server/test/integration/share-alias-one-per-page.int-spec.ts +++ b/apps/server/test/integration/share-alias-one-per-page.int-spec.ts @@ -33,6 +33,11 @@ describe('share_aliases one-per-page invariant [integration]', () => { const pageRepo = { findById: async (id: string) => ({ id, title: `title-${id}` }), }; + // The requester can view the target page (permissive), so the reassign 409 may + // include its title — these tests exercise the one-per-page invariant, not the + // #495 disclosure gate (that is unit-tested in share-alias.service.spec.ts). + const pageAccessService = { validateCanView: async () => {} }; + const USER = { id: 'u-int' } as any; beforeAll(async () => { db = getTestDb(); @@ -41,6 +46,7 @@ describe('share_aliases one-per-page invariant [integration]', () => { repo as any, pageRepo as any, {} as any, // shareService — unused by setAlias + pageAccessService as any, db as any, ); wsId = (await createWorkspace(db)).id; @@ -188,6 +194,7 @@ describe('share_aliases one-per-page invariant [integration]', () => { workspaceId: wsId, pageId, creatorId, + user: USER, alias: 'te', }); expect(first.alias).toBe('te'); @@ -196,6 +203,7 @@ describe('share_aliases one-per-page invariant [integration]', () => { workspaceId: wsId, pageId, creatorId, + user: USER, alias: 'ted', }); // Same row id — a RENAME, not a new insert. @@ -217,12 +225,14 @@ describe('share_aliases one-per-page invariant [integration]', () => { workspaceId: wsId, pageId, creatorId: null as any, + user: USER, alias: 'hello', }); const again = await service.setAlias({ workspaceId: wsId, pageId, creatorId: null as any, + user: USER, alias: 'hello', }); expect(again.id).toBe(inserted.id); @@ -244,6 +254,7 @@ describe('share_aliases one-per-page invariant [integration]', () => { flakyRepo as any, pageRepo as any, {} as any, + pageAccessService as any, db as any, ); @@ -252,6 +263,7 @@ describe('share_aliases one-per-page invariant [integration]', () => { workspaceId: wsId, pageId, creatorId: null as any, + user: USER, alias: 'rollback-me', }), ).rejects.toBeInstanceOf(BadRequestException); @@ -275,6 +287,7 @@ describe('share_aliases one-per-page invariant [integration]', () => { workspaceId: wsId, pageId: pageA, creatorId: null as any, + user: USER, alias: 'shared', }); @@ -283,6 +296,7 @@ describe('share_aliases one-per-page invariant [integration]', () => { workspaceId: wsId, pageId: pageB, creatorId: null as any, + user: USER, alias: 'shared', }), ).rejects.toBeInstanceOf(ConflictException); @@ -291,6 +305,7 @@ describe('share_aliases one-per-page invariant [integration]', () => { workspaceId: wsId, pageId: pageB, creatorId: null as any, + user: USER, alias: 'shared', confirmReassign: true, }); @@ -317,12 +332,14 @@ describe('share_aliases one-per-page invariant [integration]', () => { workspaceId: wsId, pageId: pageA, creatorId: null as any, + user: USER, alias: 'shared-target', }); await service.setAlias({ workspaceId: wsId, pageId: pageB, creatorId: null as any, + user: USER, alias: 'bee', }); @@ -330,6 +347,7 @@ describe('share_aliases one-per-page invariant [integration]', () => { workspaceId: wsId, pageId: pageB, creatorId: null as any, + user: USER, alias: 'shared-target', confirmReassign: true, });