Skip to content

Commit 3660b96

Browse files
fix(mcp): write_acl sets accessTo to the resource URL, not './' (fixes owner lockout #575) (#576)
* fix(mcp): write_acl sets accessTo to the resource URL, not './' — fixes owner lockout (#575) buildAclDoc hardcoded `acl:accessTo: './'`, which resolves against the .acl document's own URL. For a container ACL that lands on the container (correct), but for a resource ACL (<resource>.acl) it resolves to the parent container. checkAuthorizations() requires an exact accessTo match, so the resource was left with zero authorizations — denying everyone, including the owner who just granted themselves Control. - buildAclDoc now takes the explicit absolute target URL (as parser.js already does everywhere) and emits acl:default only for containers. - Harden the lockout safety net: a controlling authorization must also *apply to the target* (accessTo/default covers it), so a mismatched accessTo is refused instead of silently locking the owner out. - Add a resource-level write_acl/read_acl regression test; the existing round-trip test only exercised a container path, which is why this slipped through. Closes #575. Reported by @NilsBM. * write_acl: use relative target IRI (host-portable) instead of absolute URL Copilot review: writing an absolute accessTo bakes the current origin into stored ACLs, breaking host-portability (#428). Emit the correct *relative* target instead: './' for a container, './<basename>' for a resource. It still resolves to the right resource (the parser resolves it against the .acl URL at check time, fixing #575) while keeping pods movable across origins. The lockout safety check keeps using the absolute targetUrl, which parseAcl produces by resolving the relative ref. * write_acl: correct the accessTo-resolution comment (Copilot review) The relative accessTo is resolved against the ACL base URL (parser.js getBaseUrl(), i.e. the parent container directory), not the .acl document's own URL. Comment-only; the targetRef logic is unchanged. * write_acl: distinguish the two lockout-refusal causes (Copilot review) The appliesToTarget gate gave the refusal a second trigger (Control granted but accessTo/default doesn't cover the target). Split the message so the caller knows whether to fix the agent WebIDs (not granted Control) or the accessTo target (granted but doesn't apply here).
1 parent 45d34bd commit 3660b96

2 files changed

Lines changed: 96 additions & 15 deletions

File tree

‎src/mcp/tools.js‎

Lines changed: 58 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -300,12 +300,22 @@ async function read_acl({ path }, ctx) {
300300
});
301301
}
302302

303-
function buildAclDoc(structured) {
303+
function buildAclDoc(structured, targetRef, isContainer) {
304304
const graph = structured.authorizations.map((auth, i) => {
305305
const node = {
306306
'@id': `#auth${i}`,
307307
'@type': 'acl:Authorization',
308-
'acl:accessTo': { '@id': './' },
308+
// accessTo is a *relative* IRI; the parser resolves it against the
309+
// ACL's base URL (parser.js getBaseUrl()), which is the parent
310+
// container directory for BOTH container and resource ACLs. So './'
311+
// resolves to that container — correct for a container ACL, but for a
312+
// *resource* ACL it points at the parent container, not the resource,
313+
// leaving checkAuthorizations() (which requires an exact accessTo
314+
// match) with zero authorizations and locking out even the owner who
315+
// just granted themselves Control (#575). targetRef is therefore './'
316+
// for a container and './<basename>' for a resource. Relative IRIs
317+
// keep stored ACLs host-portable across origins (#428).
318+
'acl:accessTo': { '@id': targetRef },
309319
'acl:mode': (auth.modes || []).map(m => ({ '@id': `acl:${m}` }))
310320
};
311321
if (auth.agents && auth.agents.length) {
@@ -316,8 +326,10 @@ function buildAclDoc(structured) {
316326
'@id': fullAgentClass(c)
317327
}));
318328
}
319-
if (auth.isDefault) {
320-
node['acl:default'] = { '@id': './' };
329+
// acl:default only has meaning on a container ACL (it supplies the
330+
// defaults inherited by contained resources), where targetRef is './'.
331+
if (auth.isDefault && isContainer) {
332+
node['acl:default'] = { '@id': targetRef };
321333
}
322334
return node;
323335
});
@@ -340,33 +352,65 @@ async function write_acl({ path, authorizations }, ctx) {
340352
return toolError(`access denied: control ${path}`);
341353
}
342354
const aclPath = aclUrlFor(path);
343-
const doc = buildAclDoc({ authorizations });
355+
const isContainer = path.endsWith('/');
356+
// Relative target IRI for the stored ACL (host-portable, #428): './' for
357+
// a container, './<basename>' for a resource. The parser resolves it
358+
// against the ACL base URL (the parent container directory), so the
359+
// basename lands on the resource.
360+
const targetRef = isContainer
361+
? './'
362+
: './' + path.replace(/\/+$/, '').split('/').pop();
363+
const targetUrl = buildUrl(ctx, path); // absolute, for the lockout check below
364+
const doc = buildAclDoc({ authorizations }, targetRef, isContainer);
344365
const serialized = serializeAcl(doc);
345366

346367
// Safety: refuse to write an ACL that would lock the caller out of
347-
// future Control. This is the most common write_acl footgun —
348-
// typically caused by relative WebID paths in `agents` resolving
349-
// against the .acl URL to a different absolute URI than the caller's
350-
// actual WebID. Parse the proposed ACL with its real URL so relative
351-
// agents resolve correctly, then check whether any authorization
352-
// grants Control to the caller.
368+
// future Control. Two footguns are covered:
369+
// 1. relative WebID paths in `agents` resolving against the .acl URL
370+
// to a different absolute URI than the caller's actual WebID;
371+
// 2. an authorization whose `accessTo` does not actually cover this
372+
// resource (e.g. #575), which the checker would skip entirely.
373+
// Parse the proposed ACL with its real URL so relative refs resolve
374+
// correctly, then require an authorization that both grants Control to
375+
// the caller *and* applies to this target.
353376
const aclAbsUrl = buildUrl(ctx, aclPath);
354377
const proposed = await parseAcl(serialized, aclAbsUrl);
355-
const callerHasControl = proposed.some(auth => {
378+
const normUrl = u => String(u).replace(/\/$/, '');
379+
const grantsCallerControl = auth => {
356380
if (!(auth.modes || []).includes(AccessMode.CONTROL)) return false;
357381
if (ctx.webId && (auth.agents || []).includes(ctx.webId)) return true;
358382
if ((auth.agentClasses || []).includes(FOAF_AGENT)) return true;
359383
if (ctx.webId && (auth.agentClasses || []).includes(ACL_AUTH_AGENT)) return true;
360384
return false;
361-
});
362-
if (!callerHasControl) {
385+
};
386+
const appliesToTarget = auth => {
387+
const t = normUrl(targetUrl);
388+
return (auth.accessTo || []).some(a => normUrl(a) === t) ||
389+
(auth.default || []).some(d => {
390+
const p = normUrl(d);
391+
return t === p || t.startsWith(p + '/');
392+
});
393+
};
394+
// Distinguish the two failure modes so the caller can fix the right thing:
395+
// (a) no authorization grants Control to the caller at all, vs
396+
// (b) one does, but its accessTo/default does not cover this target.
397+
const controlAuths = proposed.filter(grantsCallerControl);
398+
if (controlAuths.length === 0) {
363399
return toolError(
364400
`write_acl refused: the proposed ACL would not grant Control to the caller (${ctx.webId || 'anonymous'}). ` +
365401
'This is typically caused by relative WebID paths in agents resolving against the .acl URL — use absolute WebIDs. ' +
366402
'If you really want to remove your own access (e.g. transferring ownership), do it in two steps: ' +
367403
'first grant Control to the new owner, then have the new owner write_acl without you.'
368404
);
369405
}
406+
if (!controlAuths.some(appliesToTarget)) {
407+
return toolError(
408+
`write_acl refused: the proposed ACL grants Control to the caller (${ctx.webId || 'anonymous'}) ` +
409+
`but none of those authorizations apply to ${path} — their accessTo/default targets a different ` +
410+
'resource (commonly the parent container), so the resource would be left with no effective Control. ' +
411+
'Ensure each authorization\'s accessTo covers this resource.'
412+
);
413+
}
370414

371415
await storage.write(aclPath, Buffer.from(serialized, 'utf8'), {
372416
contentType: 'application/ld+json'

‎test/mcp.test.js‎

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import {
2222
import { emitChange } from '../src/notifications/events.js';
2323

2424
let token;
25+
let ownerWebId;
2526

2627
async function rpc(body, opts = {}) {
2728
const headers = { 'Content-Type': 'application/json' };
@@ -39,8 +40,9 @@ async function rpc(body, opts = {}) {
3940
describe('MCP server (--mcp enabled)', () => {
4041
before(async () => {
4142
await startTestServer({ mcp: true });
42-
await createTestPod('mcptest');
43+
const pod = await createTestPod('mcptest');
4344
token = getPodToken('mcptest');
45+
ownerWebId = pod.webId;
4446
});
4547

4648
after(async () => {
@@ -322,6 +324,41 @@ describe('MCP server (--mcp enabled)', () => {
322324
assert.match(body.result.content[0].text, /control/i);
323325
});
324326

327+
it('write_acl on a resource does not lock the owner out (#575)', async () => {
328+
// Regression: write_acl on a non-container resource used to set
329+
// accessTo='./', which resolves to the *parent container* rather than
330+
// the resource, leaving the resource with zero matching authorizations
331+
// and locking out even the owner who just granted themselves Control.
332+
const resPath = '/mcptest/public/acl575.txt';
333+
const cr = await rpc({
334+
jsonrpc: '2.0', id: 220, method: 'tools/call',
335+
params: { name: 'write_resource', arguments: { path: resPath, content: 'secret', contentType: 'text/plain' } }
336+
}, { token });
337+
assert.strictEqual(cr.body.result.isError, false, cr.body.result.content?.[0]?.text);
338+
339+
// Grant the owner Read/Write/Control on the resource itself.
340+
const wr = await rpc({
341+
jsonrpc: '2.0', id: 221, method: 'tools/call',
342+
params: { name: 'write_acl', arguments: {
343+
path: resPath,
344+
authorizations: [{ agents: [ownerWebId], modes: ['Read', 'Write', 'Control'] }]
345+
} }
346+
}, { token });
347+
assert.strictEqual(wr.body.result.isError, false, wr.body.result.content?.[0]?.text);
348+
349+
// read_acl requires Control on the resource. Before the fix the owner
350+
// was locked out and this was denied; it must now succeed.
351+
const rd = await rpc({
352+
jsonrpc: '2.0', id: 222, method: 'tools/call',
353+
params: { name: 'read_acl', arguments: { path: resPath } }
354+
}, { token });
355+
assert.strictEqual(rd.body.result.isError, false,
356+
'owner locked out of resource ACL (#575): ' + rd.body.result.content?.[0]?.text);
357+
const payload = JSON.parse(rd.body.result.content[0].text);
358+
assert.ok(payload.authorizations.some(a => a.modes.includes('Control')),
359+
'resource ACL should grant the owner Control');
360+
});
361+
325362
// --- call_remote_pod (#495) ---
326363

327364
it('call_remote_pod denied without federation gate access', async () => {

0 commit comments

Comments
 (0)