From d3dcf4b7deb6808fdf2851128b64e45828eff9df Mon Sep 17 00:00:00 2001 From: Melvin Carvalho Date: Sun, 21 Jun 2026 18:58:51 +0200 Subject: [PATCH 1/4] =?UTF-8?q?fix(mcp):=20write=5Facl=20sets=20accessTo?= =?UTF-8?q?=20to=20the=20resource=20URL,=20not=20'./'=20=E2=80=94=20fixes?= =?UTF-8?q?=20owner=20lockout=20(#575)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (.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. --- src/mcp/tools.js | 47 ++++++++++++++++++++++++++++++++++++----------- test/mcp.test.js | 39 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 74 insertions(+), 12 deletions(-) diff --git a/src/mcp/tools.js b/src/mcp/tools.js index 0363c42a..2586a4ac 100644 --- a/src/mcp/tools.js +++ b/src/mcp/tools.js @@ -300,12 +300,21 @@ async function read_acl({ path }, ctx) { }); } -function buildAclDoc(structured) { +function buildAclDoc(structured, targetUrl, isContainer) { const graph = structured.authorizations.map((auth, i) => { const node = { '@id': `#auth${i}`, '@type': 'acl:Authorization', - 'acl:accessTo': { '@id': './' }, + // Point accessTo at the explicit absolute resource URL, NOT './'. A + // relative './' resolves against the .acl document's own URL, which + // only lands on the right target for a *container* ACL + // (/.acl -> the container). For a *resource* ACL + // (.acl) it resolves to the parent container, so + // checkAuthorizations() — which requires an exact accessTo match — + // finds nothing and leaves the resource with zero authorizations, + // locking out even the owner who just granted themselves Control + // (#575). parser.js already writes the explicit resourceUrl; match it. + 'acl:accessTo': { '@id': targetUrl }, 'acl:mode': (auth.modes || []).map(m => ({ '@id': `acl:${m}` })) }; if (auth.agents && auth.agents.length) { @@ -316,8 +325,11 @@ function buildAclDoc(structured) { '@id': fullAgentClass(c) })); } - if (auth.isDefault) { - node['acl:default'] = { '@id': './' }; + // acl:default only has meaning on a container ACL (it supplies the + // defaults inherited by contained resources). Emit it for containers + // only, pointing at the container itself. + if (auth.isDefault && isContainer) { + node['acl:default'] = { '@id': targetUrl }; } return node; }); @@ -340,20 +352,33 @@ async function write_acl({ path, authorizations }, ctx) { return toolError(`access denied: control ${path}`); } const aclPath = aclUrlFor(path); - const doc = buildAclDoc({ authorizations }); + const targetUrl = buildUrl(ctx, path); + const doc = buildAclDoc({ authorizations }, targetUrl, path.endsWith('/')); const serialized = serializeAcl(doc); // Safety: refuse to write an ACL that would lock the caller out of - // future Control. This is the most common write_acl footgun — - // typically caused by relative WebID paths in `agents` resolving - // against the .acl URL to a different absolute URI than the caller's - // actual WebID. Parse the proposed ACL with its real URL so relative - // agents resolve correctly, then check whether any authorization - // grants Control to the caller. + // future Control. Two footguns are covered: + // 1. relative WebID paths in `agents` resolving against the .acl URL + // to a different absolute URI than the caller's actual WebID; + // 2. an authorization whose `accessTo` does not actually cover this + // resource (e.g. #575), which the checker would skip entirely. + // Parse the proposed ACL with its real URL so relative refs resolve + // correctly, then require an authorization that both grants Control to + // the caller *and* applies to this target. const aclAbsUrl = buildUrl(ctx, aclPath); const proposed = await parseAcl(serialized, aclAbsUrl); + const normUrl = u => String(u).replace(/\/$/, ''); + const appliesToTarget = auth => { + const t = normUrl(targetUrl); + return (auth.accessTo || []).some(a => normUrl(a) === t) || + (auth.default || []).some(d => { + const p = normUrl(d); + return t === p || t.startsWith(p + '/'); + }); + }; const callerHasControl = proposed.some(auth => { if (!(auth.modes || []).includes(AccessMode.CONTROL)) return false; + if (!appliesToTarget(auth)) return false; if (ctx.webId && (auth.agents || []).includes(ctx.webId)) return true; if ((auth.agentClasses || []).includes(FOAF_AGENT)) return true; if (ctx.webId && (auth.agentClasses || []).includes(ACL_AUTH_AGENT)) return true; diff --git a/test/mcp.test.js b/test/mcp.test.js index ada34f61..21fe2ea0 100644 --- a/test/mcp.test.js +++ b/test/mcp.test.js @@ -22,6 +22,7 @@ import { import { emitChange } from '../src/notifications/events.js'; let token; +let ownerWebId; async function rpc(body, opts = {}) { const headers = { 'Content-Type': 'application/json' }; @@ -39,8 +40,9 @@ async function rpc(body, opts = {}) { describe('MCP server (--mcp enabled)', () => { before(async () => { await startTestServer({ mcp: true }); - await createTestPod('mcptest'); + const pod = await createTestPod('mcptest'); token = getPodToken('mcptest'); + ownerWebId = pod.webId; }); after(async () => { @@ -322,6 +324,41 @@ describe('MCP server (--mcp enabled)', () => { assert.match(body.result.content[0].text, /control/i); }); + it('write_acl on a resource does not lock the owner out (#575)', async () => { + // Regression: write_acl on a non-container resource used to set + // accessTo='./', which resolves to the *parent container* rather than + // the resource, leaving the resource with zero matching authorizations + // and locking out even the owner who just granted themselves Control. + const resPath = '/mcptest/public/acl575.txt'; + const cr = await rpc({ + jsonrpc: '2.0', id: 220, method: 'tools/call', + params: { name: 'write_resource', arguments: { path: resPath, content: 'secret', contentType: 'text/plain' } } + }, { token }); + assert.strictEqual(cr.body.result.isError, false, cr.body.result.content?.[0]?.text); + + // Grant the owner Read/Write/Control on the resource itself. + const wr = await rpc({ + jsonrpc: '2.0', id: 221, method: 'tools/call', + params: { name: 'write_acl', arguments: { + path: resPath, + authorizations: [{ agents: [ownerWebId], modes: ['Read', 'Write', 'Control'] }] + } } + }, { token }); + assert.strictEqual(wr.body.result.isError, false, wr.body.result.content?.[0]?.text); + + // read_acl requires Control on the resource. Before the fix the owner + // was locked out and this was denied; it must now succeed. + const rd = await rpc({ + jsonrpc: '2.0', id: 222, method: 'tools/call', + params: { name: 'read_acl', arguments: { path: resPath } } + }, { token }); + assert.strictEqual(rd.body.result.isError, false, + 'owner locked out of resource ACL (#575): ' + rd.body.result.content?.[0]?.text); + const payload = JSON.parse(rd.body.result.content[0].text); + assert.ok(payload.authorizations.some(a => a.modes.includes('Control')), + 'resource ACL should grant the owner Control'); + }); + // --- call_remote_pod (#495) --- it('call_remote_pod denied without federation gate access', async () => { From 1437456b46789e0b9457208dd641d259479d54d4 Mon Sep 17 00:00:00 2001 From: Melvin Carvalho Date: Sun, 21 Jun 2026 19:58:07 +0200 Subject: [PATCH 2/4] 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, './' 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. --- src/mcp/tools.js | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/src/mcp/tools.js b/src/mcp/tools.js index 2586a4ac..2fadfbcd 100644 --- a/src/mcp/tools.js +++ b/src/mcp/tools.js @@ -300,21 +300,22 @@ async function read_acl({ path }, ctx) { }); } -function buildAclDoc(structured, targetUrl, isContainer) { +function buildAclDoc(structured, targetRef, isContainer) { const graph = structured.authorizations.map((auth, i) => { const node = { '@id': `#auth${i}`, '@type': 'acl:Authorization', - // Point accessTo at the explicit absolute resource URL, NOT './'. A - // relative './' resolves against the .acl document's own URL, which - // only lands on the right target for a *container* ACL - // (/.acl -> the container). For a *resource* ACL - // (.acl) it resolves to the parent container, so - // checkAuthorizations() — which requires an exact accessTo match — - // finds nothing and leaves the resource with zero authorizations, - // locking out even the owner who just granted themselves Control - // (#575). parser.js already writes the explicit resourceUrl; match it. - 'acl:accessTo': { '@id': targetUrl }, + // accessTo is a *relative* IRI resolved against the .acl document's + // own URL, so stored ACLs stay host-portable across origins (#428). + // The bare './' the old code always used only lands on the right + // target for a *container* ACL (/.acl -> the container). + // For a *resource* ACL (.acl) './' resolves to the parent + // container, so checkAuthorizations() — which requires an exact + // accessTo match — finds nothing and leaves the resource with zero + // authorizations, locking out even the owner who just granted + // themselves Control (#575). targetRef is therefore './' for a + // container and './' for a resource. + 'acl:accessTo': { '@id': targetRef }, 'acl:mode': (auth.modes || []).map(m => ({ '@id': `acl:${m}` })) }; if (auth.agents && auth.agents.length) { @@ -326,10 +327,9 @@ function buildAclDoc(structured, targetUrl, isContainer) { })); } // acl:default only has meaning on a container ACL (it supplies the - // defaults inherited by contained resources). Emit it for containers - // only, pointing at the container itself. + // defaults inherited by contained resources), where targetRef is './'. if (auth.isDefault && isContainer) { - node['acl:default'] = { '@id': targetUrl }; + node['acl:default'] = { '@id': targetRef }; } return node; }); @@ -352,8 +352,15 @@ async function write_acl({ path, authorizations }, ctx) { return toolError(`access denied: control ${path}`); } const aclPath = aclUrlFor(path); - const targetUrl = buildUrl(ctx, path); - const doc = buildAclDoc({ authorizations }, targetUrl, path.endsWith('/')); + const isContainer = path.endsWith('/'); + // Relative target IRI for the stored ACL (host-portable, #428): './' for + // a container, './' for a resource. Resolved against the .acl + // URL by the parser at check time. + const targetRef = isContainer + ? './' + : './' + path.replace(/\/+$/, '').split('/').pop(); + const targetUrl = buildUrl(ctx, path); // absolute, for the lockout check below + const doc = buildAclDoc({ authorizations }, targetRef, isContainer); const serialized = serializeAcl(doc); // Safety: refuse to write an ACL that would lock the caller out of From 7312cc253ebc8c3ef13eafa17c5b4002e0052c41 Mon Sep 17 00:00:00 2001 From: Melvin Carvalho Date: Sun, 21 Jun 2026 20:33:46 +0200 Subject: [PATCH 3/4] 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. --- src/mcp/tools.js | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/mcp/tools.js b/src/mcp/tools.js index 2fadfbcd..e8aafbad 100644 --- a/src/mcp/tools.js +++ b/src/mcp/tools.js @@ -305,16 +305,16 @@ function buildAclDoc(structured, targetRef, isContainer) { const node = { '@id': `#auth${i}`, '@type': 'acl:Authorization', - // accessTo is a *relative* IRI resolved against the .acl document's - // own URL, so stored ACLs stay host-portable across origins (#428). - // The bare './' the old code always used only lands on the right - // target for a *container* ACL (/.acl -> the container). - // For a *resource* ACL (.acl) './' resolves to the parent - // container, so checkAuthorizations() — which requires an exact - // accessTo match — finds nothing and leaves the resource with zero - // authorizations, locking out even the owner who just granted - // themselves Control (#575). targetRef is therefore './' for a - // container and './' for a resource. + // accessTo is a *relative* IRI; the parser resolves it against the + // ACL's base URL (parser.js getBaseUrl()), which is the parent + // container directory for BOTH container and resource ACLs. So './' + // resolves to that container — correct for a container ACL, but for a + // *resource* ACL it points at the parent container, not the resource, + // leaving checkAuthorizations() (which requires an exact accessTo + // match) with zero authorizations and locking out even the owner who + // just granted themselves Control (#575). targetRef is therefore './' + // for a container and './' for a resource. Relative IRIs + // keep stored ACLs host-portable across origins (#428). 'acl:accessTo': { '@id': targetRef }, 'acl:mode': (auth.modes || []).map(m => ({ '@id': `acl:${m}` })) }; @@ -354,8 +354,9 @@ async function write_acl({ path, authorizations }, ctx) { const aclPath = aclUrlFor(path); const isContainer = path.endsWith('/'); // Relative target IRI for the stored ACL (host-portable, #428): './' for - // a container, './' for a resource. Resolved against the .acl - // URL by the parser at check time. + // a container, './' for a resource. The parser resolves it + // against the ACL base URL (the parent container directory), so the + // basename lands on the resource. const targetRef = isContainer ? './' : './' + path.replace(/\/+$/, '').split('/').pop(); From 0f9a308fc1f531cdc995f1a4e0e96cea8badb4a2 Mon Sep 17 00:00:00 2001 From: Melvin Carvalho Date: Sun, 21 Jun 2026 21:03:05 +0200 Subject: [PATCH 4/4] 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). --- src/mcp/tools.js | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/src/mcp/tools.js b/src/mcp/tools.js index e8aafbad..d825faa0 100644 --- a/src/mcp/tools.js +++ b/src/mcp/tools.js @@ -376,6 +376,13 @@ async function write_acl({ path, authorizations }, ctx) { const aclAbsUrl = buildUrl(ctx, aclPath); const proposed = await parseAcl(serialized, aclAbsUrl); const normUrl = u => String(u).replace(/\/$/, ''); + const grantsCallerControl = auth => { + if (!(auth.modes || []).includes(AccessMode.CONTROL)) return false; + if (ctx.webId && (auth.agents || []).includes(ctx.webId)) return true; + if ((auth.agentClasses || []).includes(FOAF_AGENT)) return true; + if (ctx.webId && (auth.agentClasses || []).includes(ACL_AUTH_AGENT)) return true; + return false; + }; const appliesToTarget = auth => { const t = normUrl(targetUrl); return (auth.accessTo || []).some(a => normUrl(a) === t) || @@ -384,15 +391,11 @@ async function write_acl({ path, authorizations }, ctx) { return t === p || t.startsWith(p + '/'); }); }; - const callerHasControl = proposed.some(auth => { - if (!(auth.modes || []).includes(AccessMode.CONTROL)) return false; - if (!appliesToTarget(auth)) return false; - if (ctx.webId && (auth.agents || []).includes(ctx.webId)) return true; - if ((auth.agentClasses || []).includes(FOAF_AGENT)) return true; - if (ctx.webId && (auth.agentClasses || []).includes(ACL_AUTH_AGENT)) return true; - return false; - }); - if (!callerHasControl) { + // Distinguish the two failure modes so the caller can fix the right thing: + // (a) no authorization grants Control to the caller at all, vs + // (b) one does, but its accessTo/default does not cover this target. + const controlAuths = proposed.filter(grantsCallerControl); + if (controlAuths.length === 0) { return toolError( `write_acl refused: the proposed ACL would not grant Control to the caller (${ctx.webId || 'anonymous'}). ` + 'This is typically caused by relative WebID paths in agents resolving against the .acl URL — use absolute WebIDs. ' + @@ -400,6 +403,14 @@ async function write_acl({ path, authorizations }, ctx) { 'first grant Control to the new owner, then have the new owner write_acl without you.' ); } + if (!controlAuths.some(appliesToTarget)) { + return toolError( + `write_acl refused: the proposed ACL grants Control to the caller (${ctx.webId || 'anonymous'}) ` + + `but none of those authorizations apply to ${path} — their accessTo/default targets a different ` + + 'resource (commonly the parent container), so the resource would be left with no effective Control. ' + + 'Ensure each authorization\'s accessTo covers this resource.' + ); + } await storage.write(aclPath, Buffer.from(serialized, 'utf8'), { contentType: 'application/ld+json'