fix(m4-1b-ii): complete §1.1 retirement in MCP content filters + review-round-1 witnesses
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:
fred
2026-08-28 20:22:03 -05:00
parent a3446a13e2
commit f55d62cf92
4 changed files with 146 additions and 58 deletions
@@ -210,6 +210,67 @@ describe('hierarchy commands integration', (): void => {
// ── §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 () => {
const estateSlug = slug('rb-create');
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 () => {
const ownerCompanies = await repo.listGrantedCompanies(OWNER);
expect(ownerCompanies.map((c) => c.id)).toContain(companyId);
+24 -9
View File
@@ -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 () => {
const { service } = makeService({
it('gives admin-role and platform-admin-role actors only owned content on brain reads (§1.1 retirement)', async () => {
// 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: [
{ id: 'project-owned', ownerId: 'role-bearing-user', teamId: 'tenant-a', name: 'owned' },
{
id: 'project-tenant-a',
ownerId: 'other-user-a',
teamId: 'tenant-a',
name: 'same tenant',
name: 'same tenant, unowned',
},
{
id: 'project-tenant-b',
@@ -339,33 +345,42 @@ describe('MCP actor identity and tool scope enforcement', () => {
},
],
missions: [
{ id: 'mission-owned', projectId: 'project-owned' },
{ id: 'mission-tenant-a', tenantId: 'tenant-a', projectId: 'project-tenant-a' },
{ id: 'mission-tenant-b', tenantId: 'tenant-b', projectId: 'project-tenant-b' },
],
tasks: [
{ id: 'task-owned', projectId: 'project-owned', 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' },
],
});
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);
const projects = JSON.parse(
(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(
(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(
(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 () => {
+10 -36
View File
@@ -156,41 +156,22 @@ type TaskLike = TenantScopedLike & {
userId?: string | null;
};
function isGlobalAdminActor(actor: McpActorContext): boolean {
return actor.role === 'platform-admin' || actor.role === 'super-admin';
}
function isTenantAdminActor(actor: McpActorContext): boolean {
return actor.role === 'admin';
}
function matchesTenant(actor: McpActorContext, record: TenantScopedLike): boolean {
return (
record.tenantId === actor.tenantId ||
record.organizationId === actor.tenantId ||
record.teamId === actor.tenantId
);
}
/**
* 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
* actor through ownership only; widened access arrives as explicit hierarchy
* grants when the MCP grant mapping lands.
*/
function filterProjectsForActor<T extends ProjectLike>(actor: McpActorContext, projects: T[]): T[] {
if (isGlobalAdminActor(actor)) return projects;
return projects.filter(
(project) =>
project.ownerId === actor.userId ||
(isTenantAdminActor(actor) && matchesTenant(actor, project)),
);
return projects.filter((project) => project.ownerId === actor.userId);
}
function filterMissionsByDirectActorScope<T extends MissionLike>(
actor: McpActorContext,
missions: T[],
): T[] {
if (isGlobalAdminActor(actor)) return missions;
return missions.filter(
(mission) =>
mission.userId === actor.userId ||
(isTenantAdminActor(actor) && matchesTenant(actor, mission)),
);
return missions.filter((mission) => mission.userId === actor.userId);
}
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> {
if (isGlobalAdminActor(actor)) return true;
const project = (await this.brain.projects.findById(projectId)) as ProjectLike | undefined;
return project ? filterProjectsForActor(actor, [project]).length === 1 : false;
}
@@ -290,8 +270,6 @@ export class McpService implements OnModuleDestroy {
actor: McpActorContext,
missions: T[],
): Promise<T[]> {
if (isGlobalAdminActor(actor)) return missions;
const projects = (await this.brain.projects.findAll()) as ProjectLike[];
const projectIds = new Set(
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> {
if (isGlobalAdminActor(actor)) return true;
const mission = (await this.brain.missions.findById(missionId)) as MissionLike | undefined;
if (!mission) return false;
return (await this.filterMissionsForActor(actor, [mission])).length === 1;
@@ -327,7 +304,7 @@ export class McpService implements OnModuleDestroy {
actor: McpActorContext,
refs: { projectId?: string | null; missionId?: string | null },
): Promise<void> {
if (!isGlobalAdminActor(actor) && !refs.projectId && !refs.missionId) {
if (!refs.projectId && !refs.missionId) {
throw new Error('MCP task scope denied');
}
await this.assertTaskReferencesAuthorized(actor, refs);
@@ -337,8 +314,6 @@ export class McpService implements OnModuleDestroy {
actor: McpActorContext,
tasks: T[],
): Promise<T[]> {
if (isGlobalAdminActor(actor)) return tasks;
const [projects, missions] = await Promise.all([
this.brain.projects.findAll(),
this.brain.missions.findAll(),
@@ -355,7 +330,6 @@ export class McpService implements OnModuleDestroy {
return tasks.filter(
(task) =>
task.userId === actor.userId ||
(isTenantAdminActor(actor) && matchesTenant(actor, task)) ||
(typeof task.projectId === 'string' && projectIds.has(task.projectId)) ||
(typeof task.missionId === 'string' && missionIds.has(task.missionId)),
);
@@ -135,9 +135,9 @@
* production code is anomalous and review-visible; that blind spot is
* accepted as a residual, not closed.
*
* The writer allowlist names hierarchy command/repository modules ONLY. It is
* empty today: the hierarchy command family (M4-1b-ii) has not landed, so no
* production module may write the class tables. The infrastructure register
* The writer allowlist names hierarchy command/repository modules ONLY. Its
* single entry is the M4-1b-ii hierarchy command repository — the sole
* production module permitted to write the class tables. The infrastructure register
* holds legitimate non-hierarchy raw execution; registered modules are exempt
* from prong (iii) only — prongs (i) and (ii) apply to them with no
* exemption, and no registered module may appear on the writer allowlist.