Skip to content

Commit 01274eb

Browse files
os-zhuangclaude
andauthored
fix(sharing): verified authz on share-link routes (#2851) (#2853)
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=<victim>, 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=<victim> 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 <noreply@anthropic.com>
1 parent c044f08 commit 01274eb

6 files changed

Lines changed: 157 additions & 27 deletions

File tree

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
---
2+
'@objectstack/plugin-sharing': minor
3+
'@objectstack/spec': minor
4+
---
5+
6+
**Security fix (#2851): the share-link HTTP routes no longer trust spoofable identity headers, and the service enforces ownership.**
7+
8+
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=<victim>` → tokens that resolve records under a system context, bypassing RLS), and revoke arbitrary users' links.
9+
10+
Fixes:
11+
12+
- **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`.
13+
- **List scoping.** `GET /api/v1/share-links` is forced to the caller's own links — a client can no longer pass `?createdBy=<victim>` to enumerate others' tokens.
14+
- **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).
15+
- **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.
16+
17+
`ShareLinkExecutionContext` gains optional `positions` / `permissions` so the record-access check evaluates the real principal.
18+
19+
Found by an adversarial security review of the request→ExecutionContext trust boundary (companion to the settings-routes fix, #2848).

packages/plugins/plugin-sharing/src/share-link-routes.ts

Lines changed: 29 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -30,20 +30,23 @@ const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const;
3030

3131
export interface ShareLinkRoutesOptions {
3232
basePath?: string;
33-
/** Read caller identity for authenticated routes. */
34-
contextFromRequest?: (req: IHttpRequest) => ShareLinkExecutionContext;
33+
/**
34+
* Derive the VERIFIED caller identity for the authenticated routes
35+
* (create / list / revoke). Production wiring (`SharingServicePlugin`) passes
36+
* a resolver backed by `resolveAuthzContext` (session / API key / OAuth).
37+
*
38+
* [Finding-2] The default is SECURE: it trusts NO identity header and yields
39+
* an anonymous context (the authenticated routes then 401). The old default
40+
* trusted `x-user-id` / `x-tenant-id`, which let a client forge attribution
41+
* and enumerate/revoke other users' links.
42+
*/
43+
contextFromRequest?: (req: IHttpRequest) => ShareLinkExecutionContext | Promise<ShareLinkExecutionContext>;
3544
}
3645

37-
const defaultContext = (req: IHttpRequest): ShareLinkExecutionContext => {
38-
const header = (name: string): string | undefined => {
39-
const v = req.headers?.[name];
40-
return Array.isArray(v) ? v[0] : v;
41-
};
42-
return {
43-
userId: header('x-user-id'),
44-
tenantId: header('x-tenant-id'),
45-
};
46-
};
46+
// [Finding-2] Secure default: anonymous (no identity read from headers). A
47+
// deployment that wants authenticated share-link management must wire a
48+
// verified `contextFromRequest` (the plugin does).
49+
const defaultContext = (_req: IHttpRequest): ShareLinkExecutionContext => ({});
4750

4851
function sendError(res: IHttpResponse, status: number, code: string, message: string) {
4952
res.status(status).json({ error: { code, message } });
@@ -73,7 +76,9 @@ export function registerShareLinkRoutes(
7376
// ── CREATE ─────────────────────────────────────────────────────
7477
http.post(base, (async (req, res) => {
7578
try {
76-
const ctx = ctxOf(req);
79+
const ctx = await ctxOf(req);
80+
// [Finding-2] Managing links requires a verified principal.
81+
if (!ctx.userId) return sendError(res, 401, 'UNAUTHENTICATED', 'Sign in to create share links');
7782
const body: any = req.body ?? {};
7883
if (!body.object || !body.recordId) {
7984
return sendError(res, 400, 'VALIDATION_FAILED', 'object and recordId are required');
@@ -105,13 +110,16 @@ export function registerShareLinkRoutes(
105110
// ── LIST ───────────────────────────────────────────────────────
106111
http.get(base, (async (req, res) => {
107112
try {
108-
const ctx = ctxOf(req);
113+
const ctx = await ctxOf(req);
114+
if (!ctx.userId) return sendError(res, 401, 'UNAUTHENTICATED', 'Sign in to list share links');
109115
const q = req.query ?? {};
110116
const link = await service.listLinks(
111117
{
112118
object: typeof q.object === 'string' ? q.object : undefined,
113119
recordId: typeof q.recordId === 'string' ? q.recordId : undefined,
114-
createdBy: typeof q.createdBy === 'string' ? q.createdBy : undefined,
120+
// [Finding-2] Force the caller's own id — a client can no longer pass
121+
// `?createdBy=<victim>` to enumerate another user's link tokens.
122+
createdBy: ctx.userId,
115123
includeRevoked: q.includeRevoked === 'true' || q.includeRevoked === '1',
116124
},
117125
ctx,
@@ -125,7 +133,8 @@ export function registerShareLinkRoutes(
125133
// ── REVOKE ─────────────────────────────────────────────────────
126134
http.delete(`${base}/:idOrToken`, (async (req, res) => {
127135
try {
128-
const ctx = ctxOf(req);
136+
const ctx = await ctxOf(req);
137+
if (!ctx.userId) return sendError(res, 401, 'UNAUTHENTICATED', 'Sign in to revoke share links');
129138
await service.revokeLink(req.params.idOrToken, ctx);
130139
await res.status(200).json({ ok: true });
131140
} catch (err: any) {
@@ -140,10 +149,10 @@ export function registerShareLinkRoutes(
140149
http.get(`${base}/:token/resolve`, (async (req, res) => {
141150
try {
142151
const q = req.query ?? {};
143-
const signedInUserId = (() => {
144-
const v = req.headers?.['x-user-id'];
145-
return Array.isArray(v) ? v[0] : v;
146-
})();
152+
// [Finding-2] The `audience: 'signed_in'` gate must key off the VERIFIED
153+
// session, not a spoofable `x-user-id` header — otherwise anyone can pass
154+
// the "must be signed in" check by inventing a user id.
155+
const signedInUserId = (await ctxOf(req)).userId;
147156
const recipientEmail = typeof q.email === 'string' ? q.email : undefined;
148157
const providedPassword =
149158
typeof q.password === 'string'

packages/plugins/plugin-sharing/src/share-link-service.test.ts

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,4 +163,49 @@ describe('ShareLinkService', () => {
163163
];
164164
expect(await service.resolveToken('expired-token-xyz-123')).toBeNull();
165165
});
166+
167+
// ── [Finding-2] verified-authz enforcement ────────────────────────────────
168+
describe('authorization (Finding-2)', () => {
169+
it('only the creator may revoke a link (a different user is denied)', async () => {
170+
const link = await service.createLink(
171+
{ object: 'ai_conversations', recordId: 'c1', audience: 'link_only', permission: 'view' },
172+
{ userId: 'owner' },
173+
);
174+
// A different signed-in user cannot revoke someone else's link.
175+
await expect(service.revokeLink(link.id, { userId: 'attacker' })).rejects.toMatchObject({ status: 403 });
176+
// The row is untouched.
177+
expect(engine._tables.sys_share_link[0].revoked_at).toBeNull();
178+
// The creator can.
179+
await expect(service.revokeLink(link.id, { userId: 'owner' })).resolves.toBeUndefined();
180+
expect(engine._tables.sys_share_link[0].revoked_at).not.toBeNull();
181+
});
182+
183+
it('a system/internal caller may revoke any link', async () => {
184+
const link = await service.createLink(
185+
{ object: 'ai_conversations', recordId: 'c1', audience: 'link_only', permission: 'view' },
186+
{ userId: 'owner' },
187+
);
188+
await expect(service.revokeLink(link.id, { isSystem: true })).resolves.toBeUndefined();
189+
expect(engine._tables.sys_share_link[0].revoked_at).not.toBeNull();
190+
});
191+
192+
it('an HTTP caller cannot mint a link for a record it cannot see (403, not 404)', async () => {
193+
// The record-access re-read runs under the caller context; a record the
194+
// caller cannot see (here: does not exist) yields a fail-closed 403 for an
195+
// untrusted caller — never a link, and without leaking existence.
196+
await expect(
197+
service.createLink(
198+
{ object: 'ai_conversations', recordId: 'ghost', audience: 'link_only', permission: 'view' },
199+
{ userId: 'u1' },
200+
),
201+
).rejects.toMatchObject({ status: 403 });
202+
// A system caller still gets the plain 404.
203+
await expect(
204+
service.createLink(
205+
{ object: 'ai_conversations', recordId: 'ghost', audience: 'link_only', permission: 'view' },
206+
{ isSystem: true },
207+
),
208+
).rejects.toMatchObject({ status: 404 });
209+
});
210+
});
166211
});

packages/plugins/plugin-sharing/src/share-link-service.ts

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -234,16 +234,23 @@ export class ShareLinkService implements IShareLinkService {
234234
throw makeError(400, 'VALIDATION_FAILED', 'emailAllowlist is required when audience=email');
235235
}
236236

237-
// Confirm the target record actually exists — silently issuing
238-
// links against ghost rows is a footgun.
237+
// Confirm the target record exists AND — for an HTTP caller — that the
238+
// caller may actually SEE it. [Finding-2] Reading under the caller's own
239+
// context (positions/permissions/RLS) means you can only mint a link for a
240+
// record you can access; a client can no longer share arbitrary rows of a
241+
// publicSharing-enabled object it cannot see. Internal (isSystem) callers
242+
// read under the system context as before.
239243
const exists = await this.engine.find(input.object, {
240244
where: { id: input.recordId },
241245
fields: ['id'],
242246
limit: 1,
243-
context: SYSTEM_CTX,
247+
context: context.isSystem ? SYSTEM_CTX : context,
244248
} as any);
245249
if (!Array.isArray(exists) || exists.length === 0) {
246-
throw makeError(404, 'RECORD_NOT_FOUND', `${input.object}/${input.recordId} does not exist`);
250+
// Don't distinguish "missing" from "not visible" for an untrusted caller.
251+
throw context.isSystem
252+
? makeError(404, 'RECORD_NOT_FOUND', `${input.object}/${input.recordId} does not exist`)
253+
: makeError(403, 'FORBIDDEN', `Not permitted to share ${input.object}/${input.recordId}`);
247254
}
248255

249256
const maxDays = policy.maxExpiryDays ?? DEFAULT_MAX_EXPIRY_DAYS;
@@ -277,17 +284,23 @@ export class ShareLinkService implements IShareLinkService {
277284
return row;
278285
}
279286

280-
async revokeLink(idOrToken: string, _context: ShareLinkExecutionContext): Promise<void> {
287+
async revokeLink(idOrToken: string, context: ShareLinkExecutionContext): Promise<void> {
281288
if (!idOrToken) throw makeError(400, 'VALIDATION_FAILED', 'id or token is required');
282289
const filter = idOrToken.startsWith('shl_') ? { id: idOrToken } : { token: idOrToken };
283290
const rows = await this.engine.find('sys_share_link', {
284291
where: filter,
285-
fields: ['id', 'revoked_at'],
292+
fields: ['id', 'revoked_at', 'created_by'],
286293
limit: 1,
287294
context: SYSTEM_CTX,
288295
} as any);
289296
const row = Array.isArray(rows) ? (rows[0] as any) : undefined;
290297
if (!row) return; // No-op when missing
298+
// [Finding-2] Only the link's creator may revoke it (internal/system callers
299+
// bypass). Previously the caller context was ignored, so any client could
300+
// revoke any user's link by id/token (sharing DoS).
301+
if (!context.isSystem && row.created_by !== context.userId) {
302+
throw makeError(403, 'FORBIDDEN', 'Not permitted to revoke this share link');
303+
}
291304
if (row.revoked_at) return; // Already revoked
292305
await this.engine.update(
293306
'sys_share_link',

packages/plugins/plugin-sharing/src/sharing-plugin.ts

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
22

33
import type { Plugin, PluginContext } from '@objectstack/core';
4+
import { resolveAuthzContext } from '@objectstack/core';
45
import type { EngineMiddleware, OperationContext } from '@objectstack/objectql';
5-
import type { IHttpServer } from '@objectstack/spec/contracts';
6+
import type { IHttpServer, IHttpRequest, ShareLinkExecutionContext } from '@objectstack/spec/contracts';
67
import { SysRecordShare, SysSharingRule, SysShareLink } from './objects/index.js';
78
import { SysBusinessUnit, SysBusinessUnitMember } from '@objectstack/platform-objects/identity';
89
import { SharingService, type SharingEngine } from './sharing-service.js';
@@ -288,8 +289,43 @@ export class SharingServicePlugin implements Plugin {
288289
// No HTTP server — service still reachable via getService.
289290
}
290291
if (http) {
292+
// [Finding-2] Derive the caller from the platform's VERIFIED
293+
// resolution (session / API key / OAuth), never from spoofable
294+
// `x-user-id` headers. `positions`/`permissions` flow through so the
295+
// createLink record-access check evaluates real RLS. An
296+
// unresolvable request → anonymous (the authed routes then 401).
297+
const ql: any = engine;
298+
const verifiedContextFromRequest = async (req: IHttpRequest): Promise<ShareLinkExecutionContext> => {
299+
try {
300+
const headers = new Headers();
301+
for (const [k, v] of Object.entries(req.headers ?? {})) {
302+
if (v == null) continue;
303+
headers.set(String(k), Array.isArray(v) ? v.join(',') : String(v));
304+
}
305+
const getSession = async (h: any) => {
306+
try {
307+
const authService: any = ctx.getService('auth');
308+
let api: any = authService?.api;
309+
if (!api && typeof authService?.getApi === 'function') api = await authService.getApi();
310+
return await api?.getSession?.({ headers: h });
311+
} catch {
312+
return undefined;
313+
}
314+
};
315+
const authz = await resolveAuthzContext({ ql, headers, getSession });
316+
return {
317+
userId: authz.userId,
318+
tenantId: authz.tenantId,
319+
positions: authz.positions,
320+
permissions: authz.permissions,
321+
};
322+
} catch {
323+
return {}; // anonymous → authed routes 401
324+
}
325+
};
291326
registerShareLinkRoutes(http, this.linkService, engine as SharingEngine, {
292327
basePath: this.options.shareLinkBasePath,
328+
contextFromRequest: verifiedContextFromRequest,
293329
});
294330
ctx.logger.info(
295331
'SharingServicePlugin: share-link routes mounted at ' +

packages/spec/src/contracts/share-link-service.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,14 @@ export interface ShareLinkExecutionContext {
101101
userId?: string;
102102
tenantId?: string;
103103
isSystem?: boolean;
104+
/**
105+
* [Finding-2] The caller's resolved positions / permissions. Populated by the
106+
* verified route wiring so RLS-aware checks (e.g. "can this caller actually
107+
* read the record being shared?") evaluate against the real principal rather
108+
* than a spoofable header identity.
109+
*/
110+
positions?: string[];
111+
permissions?: string[];
104112
}
105113

106114
/**

0 commit comments

Comments
 (0)