From d49df49adb3d3fe106a1fd42023343a7292a87e3 Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Sun, 12 Jul 2026 17:27:30 +0800 Subject: [PATCH] fix(sharing): verified authz on share-link routes (#2851) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The raw-app share-link routes trusted x-user-id/x-tenant-id headers and the service ignored the caller on revoke — so a client could forge link attribution, enumerate another user's link tokens (?createdBy=, which then resolve records under a system context bypassing RLS), and revoke arbitrary users' links. - Verified identity: SharingServicePlugin derives the caller (+ their positions/permissions) from resolveAuthzContext (session/API key/OAuth), never headers. Route default is SECURE (anonymous). Create/list/revoke require a signed-in principal (401); the public /:token/resolve stays public but keys its audience:'signed_in' check off the verified session, not a spoofable x-user-id. - List scoping: GET /share-links is forced to the caller's own links — no more ?createdBy= enumeration. - Revoke ownership: revokeLink requires the caller to be the creator (system callers bypass); previously the context was ignored. - Create access check: createLink verifies the record is visible to the caller (read under the caller's RLS) before minting — you can only share a record you can see. ShareLinkExecutionContext gains optional positions/permissions for the record-access check. Companion to #2848 (settings routes). Tests: revoke-ownership (non-owner denied, creator + system allowed), create-access (untrusted caller → 403 for an unseen record, system → 404). plugin-sharing 79 green; spec api-surface unchanged; tsc clean. Co-Authored-By: Claude Opus 4.8 --- .../share-link-routes-verified-authz.md | 19 +++++++ .../plugin-sharing/src/share-link-routes.ts | 49 +++++++++++-------- .../src/share-link-service.test.ts | 45 +++++++++++++++++ .../plugin-sharing/src/share-link-service.ts | 25 +++++++--- .../plugin-sharing/src/sharing-plugin.ts | 38 +++++++++++++- .../spec/src/contracts/share-link-service.ts | 8 +++ 6 files changed, 157 insertions(+), 27 deletions(-) create mode 100644 .changeset/share-link-routes-verified-authz.md diff --git a/.changeset/share-link-routes-verified-authz.md b/.changeset/share-link-routes-verified-authz.md new file mode 100644 index 0000000000..ec4b670751 --- /dev/null +++ b/.changeset/share-link-routes-verified-authz.md @@ -0,0 +1,19 @@ +--- +'@objectstack/plugin-sharing': minor +'@objectstack/spec': minor +--- + +**Security fix (#2851): the share-link HTTP routes no longer trust spoofable identity headers, and the service enforces ownership.** + +The raw-app share-link routes (`POST/GET/DELETE /api/v1/share-links`, registered by `SharingServicePlugin`) derived the caller from `x-user-id` / `x-tenant-id` request headers, and the service ignored the caller context on revoke. So a client could forge link attribution, enumerate another user's link tokens (`GET ?createdBy=` → tokens that resolve records under a system context, bypassing RLS), and revoke arbitrary users' links. + +Fixes: + +- **Verified identity.** `SharingServicePlugin` now derives the caller (and their positions/permissions) from the platform's verified resolution (`resolveAuthzContext` — session / API key / OAuth), never from headers. The route default is SECURE (anonymous). Create / list / revoke require a signed-in principal (401 otherwise); the public `/:token/resolve` route stays public (the token is the authorization) but keys its `audience: 'signed_in'` check off the verified session rather than a spoofable `x-user-id`. +- **List scoping.** `GET /api/v1/share-links` is forced to the caller's own links — a client can no longer pass `?createdBy=` to enumerate others' tokens. +- **Revoke ownership.** `revokeLink` now requires the caller to be the link's creator (system/internal callers bypass). Previously the caller context was ignored, so anyone could revoke any link (sharing DoS). +- **Create access check.** `createLink` verifies the record is visible to the caller (read under the caller's own RLS) before minting a link — you can only share a record you can actually see. Internal (system) callers are unchanged. + +`ShareLinkExecutionContext` gains optional `positions` / `permissions` so the record-access check evaluates the real principal. + +Found by an adversarial security review of the request→ExecutionContext trust boundary (companion to the settings-routes fix, #2848). diff --git a/packages/plugins/plugin-sharing/src/share-link-routes.ts b/packages/plugins/plugin-sharing/src/share-link-routes.ts index 237fa69b42..754a2a8998 100644 --- a/packages/plugins/plugin-sharing/src/share-link-routes.ts +++ b/packages/plugins/plugin-sharing/src/share-link-routes.ts @@ -30,20 +30,23 @@ const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const; export interface ShareLinkRoutesOptions { basePath?: string; - /** Read caller identity for authenticated routes. */ - contextFromRequest?: (req: IHttpRequest) => ShareLinkExecutionContext; + /** + * Derive the VERIFIED caller identity for the authenticated routes + * (create / list / revoke). Production wiring (`SharingServicePlugin`) passes + * a resolver backed by `resolveAuthzContext` (session / API key / OAuth). + * + * [Finding-2] The default is SECURE: it trusts NO identity header and yields + * an anonymous context (the authenticated routes then 401). The old default + * trusted `x-user-id` / `x-tenant-id`, which let a client forge attribution + * and enumerate/revoke other users' links. + */ + contextFromRequest?: (req: IHttpRequest) => ShareLinkExecutionContext | Promise; } -const defaultContext = (req: IHttpRequest): ShareLinkExecutionContext => { - const header = (name: string): string | undefined => { - const v = req.headers?.[name]; - return Array.isArray(v) ? v[0] : v; - }; - return { - userId: header('x-user-id'), - tenantId: header('x-tenant-id'), - }; -}; +// [Finding-2] Secure default: anonymous (no identity read from headers). A +// deployment that wants authenticated share-link management must wire a +// verified `contextFromRequest` (the plugin does). +const defaultContext = (_req: IHttpRequest): ShareLinkExecutionContext => ({}); function sendError(res: IHttpResponse, status: number, code: string, message: string) { res.status(status).json({ error: { code, message } }); @@ -73,7 +76,9 @@ export function registerShareLinkRoutes( // ── CREATE ───────────────────────────────────────────────────── http.post(base, (async (req, res) => { try { - const ctx = ctxOf(req); + const ctx = await ctxOf(req); + // [Finding-2] Managing links requires a verified principal. + if (!ctx.userId) return sendError(res, 401, 'UNAUTHENTICATED', 'Sign in to create share links'); const body: any = req.body ?? {}; if (!body.object || !body.recordId) { return sendError(res, 400, 'VALIDATION_FAILED', 'object and recordId are required'); @@ -105,13 +110,16 @@ export function registerShareLinkRoutes( // ── LIST ─────────────────────────────────────────────────────── http.get(base, (async (req, res) => { try { - const ctx = ctxOf(req); + const ctx = await ctxOf(req); + if (!ctx.userId) return sendError(res, 401, 'UNAUTHENTICATED', 'Sign in to list share links'); const q = req.query ?? {}; const link = await service.listLinks( { object: typeof q.object === 'string' ? q.object : undefined, recordId: typeof q.recordId === 'string' ? q.recordId : undefined, - createdBy: typeof q.createdBy === 'string' ? q.createdBy : undefined, + // [Finding-2] Force the caller's own id — a client can no longer pass + // `?createdBy=` to enumerate another user's link tokens. + createdBy: ctx.userId, includeRevoked: q.includeRevoked === 'true' || q.includeRevoked === '1', }, ctx, @@ -125,7 +133,8 @@ export function registerShareLinkRoutes( // ── REVOKE ───────────────────────────────────────────────────── http.delete(`${base}/:idOrToken`, (async (req, res) => { try { - const ctx = ctxOf(req); + const ctx = await ctxOf(req); + if (!ctx.userId) return sendError(res, 401, 'UNAUTHENTICATED', 'Sign in to revoke share links'); await service.revokeLink(req.params.idOrToken, ctx); await res.status(200).json({ ok: true }); } catch (err: any) { @@ -140,10 +149,10 @@ export function registerShareLinkRoutes( http.get(`${base}/:token/resolve`, (async (req, res) => { try { const q = req.query ?? {}; - const signedInUserId = (() => { - const v = req.headers?.['x-user-id']; - return Array.isArray(v) ? v[0] : v; - })(); + // [Finding-2] The `audience: 'signed_in'` gate must key off the VERIFIED + // session, not a spoofable `x-user-id` header — otherwise anyone can pass + // the "must be signed in" check by inventing a user id. + const signedInUserId = (await ctxOf(req)).userId; const recipientEmail = typeof q.email === 'string' ? q.email : undefined; const providedPassword = typeof q.password === 'string' diff --git a/packages/plugins/plugin-sharing/src/share-link-service.test.ts b/packages/plugins/plugin-sharing/src/share-link-service.test.ts index 0f8fd9d7b4..300f5983ad 100644 --- a/packages/plugins/plugin-sharing/src/share-link-service.test.ts +++ b/packages/plugins/plugin-sharing/src/share-link-service.test.ts @@ -163,4 +163,49 @@ describe('ShareLinkService', () => { ]; expect(await service.resolveToken('expired-token-xyz-123')).toBeNull(); }); + + // ── [Finding-2] verified-authz enforcement ──────────────────────────────── + describe('authorization (Finding-2)', () => { + it('only the creator may revoke a link (a different user is denied)', async () => { + const link = await service.createLink( + { object: 'ai_conversations', recordId: 'c1', audience: 'link_only', permission: 'view' }, + { userId: 'owner' }, + ); + // A different signed-in user cannot revoke someone else's link. + await expect(service.revokeLink(link.id, { userId: 'attacker' })).rejects.toMatchObject({ status: 403 }); + // The row is untouched. + expect(engine._tables.sys_share_link[0].revoked_at).toBeNull(); + // The creator can. + await expect(service.revokeLink(link.id, { userId: 'owner' })).resolves.toBeUndefined(); + expect(engine._tables.sys_share_link[0].revoked_at).not.toBeNull(); + }); + + it('a system/internal caller may revoke any link', async () => { + const link = await service.createLink( + { object: 'ai_conversations', recordId: 'c1', audience: 'link_only', permission: 'view' }, + { userId: 'owner' }, + ); + await expect(service.revokeLink(link.id, { isSystem: true })).resolves.toBeUndefined(); + expect(engine._tables.sys_share_link[0].revoked_at).not.toBeNull(); + }); + + it('an HTTP caller cannot mint a link for a record it cannot see (403, not 404)', async () => { + // The record-access re-read runs under the caller context; a record the + // caller cannot see (here: does not exist) yields a fail-closed 403 for an + // untrusted caller — never a link, and without leaking existence. + await expect( + service.createLink( + { object: 'ai_conversations', recordId: 'ghost', audience: 'link_only', permission: 'view' }, + { userId: 'u1' }, + ), + ).rejects.toMatchObject({ status: 403 }); + // A system caller still gets the plain 404. + await expect( + service.createLink( + { object: 'ai_conversations', recordId: 'ghost', audience: 'link_only', permission: 'view' }, + { isSystem: true }, + ), + ).rejects.toMatchObject({ status: 404 }); + }); + }); }); diff --git a/packages/plugins/plugin-sharing/src/share-link-service.ts b/packages/plugins/plugin-sharing/src/share-link-service.ts index 9d002cd759..0a0a49fdfd 100644 --- a/packages/plugins/plugin-sharing/src/share-link-service.ts +++ b/packages/plugins/plugin-sharing/src/share-link-service.ts @@ -234,16 +234,23 @@ export class ShareLinkService implements IShareLinkService { throw makeError(400, 'VALIDATION_FAILED', 'emailAllowlist is required when audience=email'); } - // Confirm the target record actually exists — silently issuing - // links against ghost rows is a footgun. + // Confirm the target record exists AND — for an HTTP caller — that the + // caller may actually SEE it. [Finding-2] Reading under the caller's own + // context (positions/permissions/RLS) means you can only mint a link for a + // record you can access; a client can no longer share arbitrary rows of a + // publicSharing-enabled object it cannot see. Internal (isSystem) callers + // read under the system context as before. const exists = await this.engine.find(input.object, { where: { id: input.recordId }, fields: ['id'], limit: 1, - context: SYSTEM_CTX, + context: context.isSystem ? SYSTEM_CTX : context, } as any); if (!Array.isArray(exists) || exists.length === 0) { - throw makeError(404, 'RECORD_NOT_FOUND', `${input.object}/${input.recordId} does not exist`); + // Don't distinguish "missing" from "not visible" for an untrusted caller. + throw context.isSystem + ? makeError(404, 'RECORD_NOT_FOUND', `${input.object}/${input.recordId} does not exist`) + : makeError(403, 'FORBIDDEN', `Not permitted to share ${input.object}/${input.recordId}`); } const maxDays = policy.maxExpiryDays ?? DEFAULT_MAX_EXPIRY_DAYS; @@ -277,17 +284,23 @@ export class ShareLinkService implements IShareLinkService { return row; } - async revokeLink(idOrToken: string, _context: ShareLinkExecutionContext): Promise { + async revokeLink(idOrToken: string, context: ShareLinkExecutionContext): Promise { if (!idOrToken) throw makeError(400, 'VALIDATION_FAILED', 'id or token is required'); const filter = idOrToken.startsWith('shl_') ? { id: idOrToken } : { token: idOrToken }; const rows = await this.engine.find('sys_share_link', { where: filter, - fields: ['id', 'revoked_at'], + fields: ['id', 'revoked_at', 'created_by'], limit: 1, context: SYSTEM_CTX, } as any); const row = Array.isArray(rows) ? (rows[0] as any) : undefined; if (!row) return; // No-op when missing + // [Finding-2] Only the link's creator may revoke it (internal/system callers + // bypass). Previously the caller context was ignored, so any client could + // revoke any user's link by id/token (sharing DoS). + if (!context.isSystem && row.created_by !== context.userId) { + throw makeError(403, 'FORBIDDEN', 'Not permitted to revoke this share link'); + } if (row.revoked_at) return; // Already revoked await this.engine.update( 'sys_share_link', diff --git a/packages/plugins/plugin-sharing/src/sharing-plugin.ts b/packages/plugins/plugin-sharing/src/sharing-plugin.ts index 0d040a454c..7abf592a16 100644 --- a/packages/plugins/plugin-sharing/src/sharing-plugin.ts +++ b/packages/plugins/plugin-sharing/src/sharing-plugin.ts @@ -1,8 +1,9 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import type { Plugin, PluginContext } from '@objectstack/core'; +import { resolveAuthzContext } from '@objectstack/core'; import type { EngineMiddleware, OperationContext } from '@objectstack/objectql'; -import type { IHttpServer } from '@objectstack/spec/contracts'; +import type { IHttpServer, IHttpRequest, ShareLinkExecutionContext } from '@objectstack/spec/contracts'; import { SysRecordShare, SysSharingRule, SysShareLink } from './objects/index.js'; import { SysBusinessUnit, SysBusinessUnitMember } from '@objectstack/platform-objects/identity'; import { SharingService, type SharingEngine } from './sharing-service.js'; @@ -288,8 +289,43 @@ export class SharingServicePlugin implements Plugin { // No HTTP server — service still reachable via getService. } if (http) { + // [Finding-2] Derive the caller from the platform's VERIFIED + // resolution (session / API key / OAuth), never from spoofable + // `x-user-id` headers. `positions`/`permissions` flow through so the + // createLink record-access check evaluates real RLS. An + // unresolvable request → anonymous (the authed routes then 401). + const ql: any = engine; + const verifiedContextFromRequest = async (req: IHttpRequest): Promise => { + try { + const headers = new Headers(); + for (const [k, v] of Object.entries(req.headers ?? {})) { + if (v == null) continue; + headers.set(String(k), Array.isArray(v) ? v.join(',') : String(v)); + } + const getSession = async (h: any) => { + try { + const authService: any = ctx.getService('auth'); + let api: any = authService?.api; + if (!api && typeof authService?.getApi === 'function') api = await authService.getApi(); + return await api?.getSession?.({ headers: h }); + } catch { + return undefined; + } + }; + const authz = await resolveAuthzContext({ ql, headers, getSession }); + return { + userId: authz.userId, + tenantId: authz.tenantId, + positions: authz.positions, + permissions: authz.permissions, + }; + } catch { + return {}; // anonymous → authed routes 401 + } + }; registerShareLinkRoutes(http, this.linkService, engine as SharingEngine, { basePath: this.options.shareLinkBasePath, + contextFromRequest: verifiedContextFromRequest, }); ctx.logger.info( 'SharingServicePlugin: share-link routes mounted at ' + diff --git a/packages/spec/src/contracts/share-link-service.ts b/packages/spec/src/contracts/share-link-service.ts index b59e46fb77..ce458b8684 100644 --- a/packages/spec/src/contracts/share-link-service.ts +++ b/packages/spec/src/contracts/share-link-service.ts @@ -101,6 +101,14 @@ export interface ShareLinkExecutionContext { userId?: string; tenantId?: string; isSystem?: boolean; + /** + * [Finding-2] The caller's resolved positions / permissions. Populated by the + * verified route wiring so RLS-aware checks (e.g. "can this caller actually + * read the record being shared?") evaluate against the real principal rather + * than a spoofable header identity. + */ + positions?: string[]; + permissions?: string[]; } /**