fix(m4-1b-ii): complete §1.1 retirement in MCP content filters + review-round-1 witnesses
ci/woodpecker/pr/ci Pipeline was successful
ci/woodpecker/pr/ci Pipeline was successful
GLM round-1 findings on PR #1465: - BLOCKING 1: remove role-keyed content widening from mcp.service.ts — isGlobalAdminActor/isTenantAdminActor/matchesTenant and every short-circuit keyed on users.role are gone; project/mission/task visibility is ownership + derived membership only, task create scope is unconditional. Spec test rewritten to witness that admin-role and platform-admin-role actors see only owned content. - MINOR 2: writer-coverage header updated to the non-empty allowlist. - MINOR 3: §6.9 witness — a directory-listed company still refuses non-granted callers (granted-read exclusion + mutation oracle). - MINOR 4: §6.4 commit legs for renameCompany (previousName in the audited event) and direct revokeGrant (row deletion + grant_revoke).
This commit is contained in:
@@ -210,6 +210,67 @@ describe('hierarchy commands integration', (): void => {
|
|||||||
|
|
||||||
// ── §6.4 rollback legs (one per mutation class) ────────────────────────────
|
// ── §6.4 rollback legs (one per mutation class) ────────────────────────────
|
||||||
|
|
||||||
|
it('renameCompany commits the rename with an audited event carrying previousName', async () => {
|
||||||
|
const created = expectOk(
|
||||||
|
await repo.createCompany({ actorId: OWNER, name: 'Old Name Co', slug: slug('rename') }),
|
||||||
|
);
|
||||||
|
const key = `key-${randomUUID()}`;
|
||||||
|
const renamed = expectOk(
|
||||||
|
await repo.renameCompany({
|
||||||
|
actorId: OWNER,
|
||||||
|
companyId: created.company.id,
|
||||||
|
name: 'New Name Co',
|
||||||
|
idempotencyKey: key,
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
expect(renamed.company.name).toBe('New Name Co');
|
||||||
|
|
||||||
|
const events = await eventsForKey(key);
|
||||||
|
expect(events).toHaveLength(1);
|
||||||
|
expect(events[0]).toMatchObject({ verb: 'rename', targetId: created.company.id });
|
||||||
|
// §6.4: the audited rename carries the old and new names.
|
||||||
|
expect(events[0]!.targetSnapshot).toMatchObject({
|
||||||
|
name: 'New Name Co',
|
||||||
|
previousName: 'Old Name Co',
|
||||||
|
});
|
||||||
|
expect(await outboxForKey(key)).toHaveLength(1);
|
||||||
|
|
||||||
|
const rows = await handle.db
|
||||||
|
.select()
|
||||||
|
.from(companies)
|
||||||
|
.where(eq(companies.id, created.company.id));
|
||||||
|
expect(rows[0]!.name).toBe('New Name Co');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('revokeGrant commits the row deletion with one audited grant_revoke event', async () => {
|
||||||
|
const grant = expectOk(
|
||||||
|
await repo.createGrant({
|
||||||
|
actorId: OWNER,
|
||||||
|
userId: SUBJECT,
|
||||||
|
targetKind: 'company',
|
||||||
|
targetId: companyId,
|
||||||
|
role: 'viewer',
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
const key = `key-${randomUUID()}`;
|
||||||
|
const revoked = expectOk(
|
||||||
|
await repo.revokeGrant({ actorId: OWNER, grantId: grant.grant.id, idempotencyKey: key }),
|
||||||
|
);
|
||||||
|
expect(revoked.revokedId).toBe(grant.grant.id);
|
||||||
|
|
||||||
|
const events = await eventsForKey(key);
|
||||||
|
expect(events).toHaveLength(1);
|
||||||
|
expect(events[0]).toMatchObject({ verb: 'grant_revoke', targetId: grant.grant.id });
|
||||||
|
expect(await outboxForKey(key)).toHaveLength(1);
|
||||||
|
|
||||||
|
// §6 revocation = row deletion: the grant row is gone.
|
||||||
|
const rows = await handle.db
|
||||||
|
.select()
|
||||||
|
.from(hierarchyGrants)
|
||||||
|
.where(eq(hierarchyGrants.id, grant.grant.id));
|
||||||
|
expect(rows).toHaveLength(0);
|
||||||
|
});
|
||||||
|
|
||||||
it('rolls back a create: no estate row survives the aborted transaction', async () => {
|
it('rolls back a create: no estate row survives the aborted transaction', async () => {
|
||||||
const estateSlug = slug('rb-create');
|
const estateSlug = slug('rb-create');
|
||||||
const key = `key-${randomUUID()}`;
|
const key = `key-${randomUUID()}`;
|
||||||
@@ -517,6 +578,44 @@ describe('hierarchy commands integration', (): void => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('directory disclosure confers no authority: a listed company still refuses non-granted callers (§6.9)', async () => {
|
||||||
|
const listed = expectOk(
|
||||||
|
await repo.createCompany({ actorId: OWNER, name: 'Exposed Co', slug: slug('exposed') }),
|
||||||
|
);
|
||||||
|
expectOk(
|
||||||
|
await repo.changeCompanyVisibility({
|
||||||
|
actorId: ADMIN,
|
||||||
|
companyId: listed.company.id,
|
||||||
|
visibility: 'directory',
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
|
||||||
|
// The company is directory-listed for the whole probe window...
|
||||||
|
expect((await repo.listDirectory()).map((entry) => entry.id)).toContain(listed.company.id);
|
||||||
|
|
||||||
|
// ...but the non-granted reader's granted-read surface still excludes it:
|
||||||
|
// directory disclosure adds existence/name/slug only, never content access.
|
||||||
|
expect(await repo.listGrantedCompanies(STRANGER)).toEqual([]);
|
||||||
|
|
||||||
|
// A stranger mutation of the listed company is refused exactly like a
|
||||||
|
// missing node — the §6.7 carve-out covers the listing, not commands.
|
||||||
|
const realProbe = await repo.renameCompany({
|
||||||
|
actorId: STRANGER,
|
||||||
|
companyId: listed.company.id,
|
||||||
|
name: 'x',
|
||||||
|
});
|
||||||
|
const missingProbe = await repo.renameCompany({
|
||||||
|
actorId: STRANGER,
|
||||||
|
companyId: randomUUID(),
|
||||||
|
name: 'x',
|
||||||
|
});
|
||||||
|
expect(realProbe).toEqual(missingProbe);
|
||||||
|
expect(await repo.deleteCompany({ actorId: STRANGER, companyId: listed.company.id })).toEqual({
|
||||||
|
ok: false,
|
||||||
|
error: 'not_found',
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
it('granted companies are the reader control: owner sees them, a stranger sees nothing (§2.8)', async () => {
|
it('granted companies are the reader control: owner sees them, a stranger sees nothing (§2.8)', async () => {
|
||||||
const ownerCompanies = await repo.listGrantedCompanies(OWNER);
|
const ownerCompanies = await repo.listGrantedCompanies(OWNER);
|
||||||
expect(ownerCompanies.map((c) => c.id)).toContain(companyId);
|
expect(ownerCompanies.map((c) => c.id)).toContain(companyId);
|
||||||
|
|||||||
@@ -322,14 +322,20 @@ describe('MCP actor identity and tool scope enforcement', () => {
|
|||||||
]);
|
]);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('enforces tenant boundaries for tenant-admin brain project, mission, and task reads', async () => {
|
it('gives admin-role and platform-admin-role actors only owned content on brain reads (§1.1 retirement)', async () => {
|
||||||
const { service } = makeService({
|
// Contract 2 §1.1: users.role confers no content visibility. An actor whose
|
||||||
|
// role is 'admin', 'platform-admin', or 'super-admin' but who holds no
|
||||||
|
// ownership sees exactly what an unprivileged member with the same
|
||||||
|
// ownership would see — here, only the one project they own, and nothing
|
||||||
|
// tenant-wide or platform-wide.
|
||||||
|
const fixtures = {
|
||||||
projects: [
|
projects: [
|
||||||
|
{ id: 'project-owned', ownerId: 'role-bearing-user', teamId: 'tenant-a', name: 'owned' },
|
||||||
{
|
{
|
||||||
id: 'project-tenant-a',
|
id: 'project-tenant-a',
|
||||||
ownerId: 'other-user-a',
|
ownerId: 'other-user-a',
|
||||||
teamId: 'tenant-a',
|
teamId: 'tenant-a',
|
||||||
name: 'same tenant',
|
name: 'same tenant, unowned',
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
id: 'project-tenant-b',
|
id: 'project-tenant-b',
|
||||||
@@ -339,33 +345,42 @@ describe('MCP actor identity and tool scope enforcement', () => {
|
|||||||
},
|
},
|
||||||
],
|
],
|
||||||
missions: [
|
missions: [
|
||||||
|
{ id: 'mission-owned', projectId: 'project-owned' },
|
||||||
{ id: 'mission-tenant-a', tenantId: 'tenant-a', projectId: 'project-tenant-a' },
|
{ id: 'mission-tenant-a', tenantId: 'tenant-a', projectId: 'project-tenant-a' },
|
||||||
{ id: 'mission-tenant-b', tenantId: 'tenant-b', projectId: 'project-tenant-b' },
|
{ id: 'mission-tenant-b', tenantId: 'tenant-b', projectId: 'project-tenant-b' },
|
||||||
],
|
],
|
||||||
tasks: [
|
tasks: [
|
||||||
|
{ id: 'task-owned', projectId: 'project-owned', status: 'not-started' },
|
||||||
{ id: 'task-tenant-a', projectId: 'project-tenant-a', status: 'not-started' },
|
{ id: 'task-tenant-a', projectId: 'project-tenant-a', status: 'not-started' },
|
||||||
{ id: 'task-tenant-b', projectId: 'project-tenant-b', status: 'not-started' },
|
{ id: 'task-tenant-b', projectId: 'project-tenant-b', status: 'not-started' },
|
||||||
],
|
],
|
||||||
});
|
};
|
||||||
const { server, tools } = makeCapturingServer();
|
|
||||||
const actor = makeAdminActor('tenant-admin-user', 'tenant-a');
|
|
||||||
|
|
||||||
|
const actors = [
|
||||||
|
makeAdminActor('role-bearing-user', 'tenant-a'),
|
||||||
|
makePlatformAdminActor('role-bearing-user'),
|
||||||
|
];
|
||||||
|
|
||||||
|
for (const actor of actors) {
|
||||||
|
const { service } = makeService(fixtures);
|
||||||
|
const { server, tools } = makeCapturingServer();
|
||||||
service.registerTools(server, actor);
|
service.registerTools(server, actor);
|
||||||
|
|
||||||
const projects = JSON.parse(
|
const projects = JSON.parse(
|
||||||
(await getTool(tools, 'brain_list_projects').handler({})).content[0]!.text,
|
(await getTool(tools, 'brain_list_projects').handler({})).content[0]!.text,
|
||||||
);
|
);
|
||||||
expect(projects.map((project: { id: string }) => project.id)).toEqual(['project-tenant-a']);
|
expect(projects.map((project: { id: string }) => project.id)).toEqual(['project-owned']);
|
||||||
|
|
||||||
const missions = JSON.parse(
|
const missions = JSON.parse(
|
||||||
(await getTool(tools, 'brain_list_missions').handler({})).content[0]!.text,
|
(await getTool(tools, 'brain_list_missions').handler({})).content[0]!.text,
|
||||||
);
|
);
|
||||||
expect(missions.map((mission: { id: string }) => mission.id)).toEqual(['mission-tenant-a']);
|
expect(missions.map((mission: { id: string }) => mission.id)).toEqual(['mission-owned']);
|
||||||
|
|
||||||
const tasks = JSON.parse(
|
const tasks = JSON.parse(
|
||||||
(await getTool(tools, 'brain_list_tasks').handler({})).content[0]!.text,
|
(await getTool(tools, 'brain_list_tasks').handler({})).content[0]!.text,
|
||||||
);
|
);
|
||||||
expect(tasks.map((task: { id: string }) => task.id)).toEqual(['task-tenant-a']);
|
expect(tasks.map((task: { id: string }) => task.id)).toEqual(['task-owned']);
|
||||||
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
it('denies tenant-admin task writes outside the authenticated tenant', async () => {
|
it('denies tenant-admin task writes outside the authenticated tenant', async () => {
|
||||||
|
|||||||
@@ -156,41 +156,22 @@ type TaskLike = TenantScopedLike & {
|
|||||||
userId?: string | null;
|
userId?: string | null;
|
||||||
};
|
};
|
||||||
|
|
||||||
function isGlobalAdminActor(actor: McpActorContext): boolean {
|
/**
|
||||||
return actor.role === 'platform-admin' || actor.role === 'super-admin';
|
* Contract 2 §1.1: `users.role` confers NO content visibility — the former
|
||||||
}
|
* global-admin/tenant-admin filter short-circuits keyed on the platform role
|
||||||
|
* are retired along with the role-derived scope sets. Content reaches an MCP
|
||||||
function isTenantAdminActor(actor: McpActorContext): boolean {
|
* actor through ownership only; widened access arrives as explicit hierarchy
|
||||||
return actor.role === 'admin';
|
* grants when the MCP grant mapping lands.
|
||||||
}
|
*/
|
||||||
|
|
||||||
function matchesTenant(actor: McpActorContext, record: TenantScopedLike): boolean {
|
|
||||||
return (
|
|
||||||
record.tenantId === actor.tenantId ||
|
|
||||||
record.organizationId === actor.tenantId ||
|
|
||||||
record.teamId === actor.tenantId
|
|
||||||
);
|
|
||||||
}
|
|
||||||
|
|
||||||
function filterProjectsForActor<T extends ProjectLike>(actor: McpActorContext, projects: T[]): T[] {
|
function filterProjectsForActor<T extends ProjectLike>(actor: McpActorContext, projects: T[]): T[] {
|
||||||
if (isGlobalAdminActor(actor)) return projects;
|
return projects.filter((project) => project.ownerId === actor.userId);
|
||||||
return projects.filter(
|
|
||||||
(project) =>
|
|
||||||
project.ownerId === actor.userId ||
|
|
||||||
(isTenantAdminActor(actor) && matchesTenant(actor, project)),
|
|
||||||
);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
function filterMissionsByDirectActorScope<T extends MissionLike>(
|
function filterMissionsByDirectActorScope<T extends MissionLike>(
|
||||||
actor: McpActorContext,
|
actor: McpActorContext,
|
||||||
missions: T[],
|
missions: T[],
|
||||||
): T[] {
|
): T[] {
|
||||||
if (isGlobalAdminActor(actor)) return missions;
|
return missions.filter((mission) => mission.userId === actor.userId);
|
||||||
return missions.filter(
|
|
||||||
(mission) =>
|
|
||||||
mission.userId === actor.userId ||
|
|
||||||
(isTenantAdminActor(actor) && matchesTenant(actor, mission)),
|
|
||||||
);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
function scopesEqual(left: ReadonlySet<McpToolScope>, right: ReadonlySet<McpToolScope>): boolean {
|
function scopesEqual(left: ReadonlySet<McpToolScope>, right: ReadonlySet<McpToolScope>): boolean {
|
||||||
@@ -281,7 +262,6 @@ export class McpService implements OnModuleDestroy {
|
|||||||
}
|
}
|
||||||
|
|
||||||
private async isProjectAuthorized(actor: McpActorContext, projectId: string): Promise<boolean> {
|
private async isProjectAuthorized(actor: McpActorContext, projectId: string): Promise<boolean> {
|
||||||
if (isGlobalAdminActor(actor)) return true;
|
|
||||||
const project = (await this.brain.projects.findById(projectId)) as ProjectLike | undefined;
|
const project = (await this.brain.projects.findById(projectId)) as ProjectLike | undefined;
|
||||||
return project ? filterProjectsForActor(actor, [project]).length === 1 : false;
|
return project ? filterProjectsForActor(actor, [project]).length === 1 : false;
|
||||||
}
|
}
|
||||||
@@ -290,8 +270,6 @@ export class McpService implements OnModuleDestroy {
|
|||||||
actor: McpActorContext,
|
actor: McpActorContext,
|
||||||
missions: T[],
|
missions: T[],
|
||||||
): Promise<T[]> {
|
): Promise<T[]> {
|
||||||
if (isGlobalAdminActor(actor)) return missions;
|
|
||||||
|
|
||||||
const projects = (await this.brain.projects.findAll()) as ProjectLike[];
|
const projects = (await this.brain.projects.findAll()) as ProjectLike[];
|
||||||
const projectIds = new Set(
|
const projectIds = new Set(
|
||||||
filterProjectsForActor(actor, projects).map((project) => project.id),
|
filterProjectsForActor(actor, projects).map((project) => project.id),
|
||||||
@@ -305,7 +283,6 @@ export class McpService implements OnModuleDestroy {
|
|||||||
}
|
}
|
||||||
|
|
||||||
private async isMissionAuthorized(actor: McpActorContext, missionId: string): Promise<boolean> {
|
private async isMissionAuthorized(actor: McpActorContext, missionId: string): Promise<boolean> {
|
||||||
if (isGlobalAdminActor(actor)) return true;
|
|
||||||
const mission = (await this.brain.missions.findById(missionId)) as MissionLike | undefined;
|
const mission = (await this.brain.missions.findById(missionId)) as MissionLike | undefined;
|
||||||
if (!mission) return false;
|
if (!mission) return false;
|
||||||
return (await this.filterMissionsForActor(actor, [mission])).length === 1;
|
return (await this.filterMissionsForActor(actor, [mission])).length === 1;
|
||||||
@@ -327,7 +304,7 @@ export class McpService implements OnModuleDestroy {
|
|||||||
actor: McpActorContext,
|
actor: McpActorContext,
|
||||||
refs: { projectId?: string | null; missionId?: string | null },
|
refs: { projectId?: string | null; missionId?: string | null },
|
||||||
): Promise<void> {
|
): Promise<void> {
|
||||||
if (!isGlobalAdminActor(actor) && !refs.projectId && !refs.missionId) {
|
if (!refs.projectId && !refs.missionId) {
|
||||||
throw new Error('MCP task scope denied');
|
throw new Error('MCP task scope denied');
|
||||||
}
|
}
|
||||||
await this.assertTaskReferencesAuthorized(actor, refs);
|
await this.assertTaskReferencesAuthorized(actor, refs);
|
||||||
@@ -337,8 +314,6 @@ export class McpService implements OnModuleDestroy {
|
|||||||
actor: McpActorContext,
|
actor: McpActorContext,
|
||||||
tasks: T[],
|
tasks: T[],
|
||||||
): Promise<T[]> {
|
): Promise<T[]> {
|
||||||
if (isGlobalAdminActor(actor)) return tasks;
|
|
||||||
|
|
||||||
const [projects, missions] = await Promise.all([
|
const [projects, missions] = await Promise.all([
|
||||||
this.brain.projects.findAll(),
|
this.brain.projects.findAll(),
|
||||||
this.brain.missions.findAll(),
|
this.brain.missions.findAll(),
|
||||||
@@ -355,7 +330,6 @@ export class McpService implements OnModuleDestroy {
|
|||||||
return tasks.filter(
|
return tasks.filter(
|
||||||
(task) =>
|
(task) =>
|
||||||
task.userId === actor.userId ||
|
task.userId === actor.userId ||
|
||||||
(isTenantAdminActor(actor) && matchesTenant(actor, task)) ||
|
|
||||||
(typeof task.projectId === 'string' && projectIds.has(task.projectId)) ||
|
(typeof task.projectId === 'string' && projectIds.has(task.projectId)) ||
|
||||||
(typeof task.missionId === 'string' && missionIds.has(task.missionId)),
|
(typeof task.missionId === 'string' && missionIds.has(task.missionId)),
|
||||||
);
|
);
|
||||||
|
|||||||
@@ -135,9 +135,9 @@
|
|||||||
* production code is anomalous and review-visible; that blind spot is
|
* production code is anomalous and review-visible; that blind spot is
|
||||||
* accepted as a residual, not closed.
|
* accepted as a residual, not closed.
|
||||||
*
|
*
|
||||||
* The writer allowlist names hierarchy command/repository modules ONLY. It is
|
* The writer allowlist names hierarchy command/repository modules ONLY. Its
|
||||||
* empty today: the hierarchy command family (M4-1b-ii) has not landed, so no
|
* single entry is the M4-1b-ii hierarchy command repository — the sole
|
||||||
* production module may write the class tables. The infrastructure register
|
* production module permitted to write the class tables. The infrastructure register
|
||||||
* holds legitimate non-hierarchy raw execution; registered modules are exempt
|
* holds legitimate non-hierarchy raw execution; registered modules are exempt
|
||||||
* from prong (iii) only — prongs (i) and (ii) apply to them with no
|
* from prong (iii) only — prongs (i) and (ii) apply to them with no
|
||||||
* exemption, and no registered module may appear on the writer allowlist.
|
* exemption, and no registered module may appear on the writer allowlist.
|
||||||
|
|||||||
Reference in New Issue
Block a user