Skip to content

Commit b2fbc5e

Browse files
melvincarvalhomelvincarvalho
authored
fix(mcp): write_acl safety + footgun docs (JavaScriptSolidServer#498)
Live-fire smoke of JavaScriptSolidServer#497 surfaced two real gotchas with the new write_acl tool: 1. Relative WebID paths in 'agents' resolve against the .acl URL, not the pod root. ../profile/card.jsonld#me writes to a different absolute URI depending on where the .acl is — easy to lock yourself out. 2. There was no server-side check that the resulting ACL would keep the caller in control. This commit: - Adds a safety check in write_acl that parses the proposed ACL with its real URL (so relative agents resolve correctly), then checks whether any authorization grants Control to the caller. If not, refuses with an explanatory error pointing at the most common cause (relative WebIDs) and the workaround for legitimate ownership transfer. - Adds a 'Footguns' section to docs/mcp.md documenting the relative- vs-absolute WebID rule with concrete right/wrong examples, the new safety check, and the SSE keep-alive caveat. Tests: 27 passing (was 26). New test: write_acl refuses an ACL that grants Control only to a foreign WebID.
1 parent beb0cfb commit b2fbc5e

3 files changed

Lines changed: 88 additions & 2 deletions

File tree

‎docs/mcp.md‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,40 @@ http://localhost:4443/mcp
171171

172172
For authenticated access, configure the client to send `Authorization: Bearer <token>`. Tokens come from `POST /idp/credentials` (username/password) or from any compatible OIDC/DPoP flow.
173173

174+
## Footguns
175+
176+
A short list of real gotchas to know about, learned from live-fire use:
177+
178+
### Use absolute WebIDs in `write_acl` agents
179+
180+
The `agents` array in a `write_acl` authorization is interpreted as a list of URIs. Relative paths (e.g. `../profile/card.jsonld#me`) resolve against the **.acl file's URL**, not the pod root — and the .acl URL changes depending on which resource the ACL applies to. Two pitfalls:
181+
182+
```json
183+
// Pod owner WebID: http://example.com/profile/card.jsonld#me
184+
// Writing this ACL to /public/forum/.acl:
185+
{
186+
"agents": ["../profile/card.jsonld#me"], // wrong — resolves to /public/profile/card.jsonld#me
187+
"agents": ["./profile/card.jsonld#me"], // wrong — resolves to /public/forum/profile/card.jsonld#me
188+
"agents": ["/profile/card.jsonld#me"], // right — absolute path, resolves to /profile/card.jsonld#me
189+
"agents": ["http://example.com/profile/card.jsonld#me"] // right — absolute URL, host-portable
190+
}
191+
```
192+
193+
**Always use absolute WebID URLs unless you know exactly what relative-URL resolution will give you.** Absolute URLs also make the ACL portable across hostnames.
194+
195+
### `write_acl` will refuse if you'd lock yourself out
196+
197+
If the proposed ACL doesn't grant `Control` to the caller (typically a relative-URL mistake), `write_acl` refuses with an explanatory error. This is a safety, not a permission check — it's stopping you from breaking your own access.
198+
199+
If you really want to transfer ownership (remove your own access), do it in two steps:
200+
201+
1. First `write_acl` granting Control to the new owner *in addition to* yourself
202+
2. Then the new owner calls `write_acl` removing you
203+
204+
### Subscribe over long-running connections needs a keep-alive client
205+
206+
The `subscribe` tool keeps an HTTP+SSE connection open indefinitely. Some HTTP clients, proxies, or load balancers will time out idle streams. If you're subscribing for hours, ensure your client handles SSE reconnect (most do; raw `curl` does not).
207+
174208
## What's not included (yet)
175209

176210
The current cut ships CRUD, structured ACL editing, subscribe, federation, skills, docs, and introspection. Deferred:

‎src/mcp/tools.js‎

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
import * as storage from '../storage/filesystem.js';
1313
import { checkAccess } from '../wac/checker.js';
1414
import { AccessMode, parseAcl, serializeAcl } from '../wac/parser.js';
15-
import { resourceEvents } from '../notifications/events.js';
15+
import { resourceEvents, emitChange } from '../notifications/events.js';
1616
import { toolText, toolError, toolJson } from './protocol.js';
1717
import { discoverSkills, readSkill, readPodSkill } from './skills.js';
1818
import { readFile, readdir, stat as fsStat } from 'fs/promises';
@@ -139,6 +139,7 @@ async function write_resource({ path, content, contentType }, ctx) {
139139
await storage.write(path, Buffer.from(content, 'utf8'), {
140140
contentType: contentType || 'text/plain'
141141
});
142+
emitChange(buildUrl(ctx, path));
142143
return toolText(`wrote ${path} (${Buffer.byteLength(content, 'utf8')} bytes)`);
143144
}
144145

@@ -156,11 +157,13 @@ async function create_resource({ container, slug, content, contentType, isContai
156157
const childPath = `${container}${name}${isContainer ? '/' : ''}`;
157158
if (isContainer) {
158159
await storage.createContainer(childPath);
160+
emitChange(buildUrl(ctx, childPath));
159161
return toolText(`created container ${childPath}`);
160162
}
161163
await storage.write(childPath, Buffer.from(content || '', 'utf8'), {
162164
contentType: contentType || 'text/plain'
163165
});
166+
emitChange(buildUrl(ctx, childPath));
164167
return toolText(`created ${childPath}`);
165168
}
166169

@@ -173,6 +176,7 @@ async function delete_resource({ path }, ctx) {
173176
return toolError(`not found: ${path}`);
174177
}
175178
await storage.remove(path);
179+
emitChange(buildUrl(ctx, path));
176180
return toolText(`deleted ${path}`);
177181
}
178182

@@ -337,9 +341,37 @@ async function write_acl({ path, authorizations }, ctx) {
337341
}
338342
const aclPath = aclUrlFor(path);
339343
const doc = buildAclDoc({ authorizations });
340-
await storage.write(aclPath, Buffer.from(serializeAcl(doc), 'utf8'), {
344+
const serialized = serializeAcl(doc);
345+
346+
// 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.
353+
const aclAbsUrl = buildUrl(ctx, aclPath);
354+
const proposed = await parseAcl(serialized, aclAbsUrl);
355+
const callerHasControl = proposed.some(auth => {
356+
if (!(auth.modes || []).includes(AccessMode.CONTROL)) return false;
357+
if (ctx.webId && (auth.agents || []).includes(ctx.webId)) return true;
358+
if ((auth.agentClasses || []).includes(FOAF_AGENT)) return true;
359+
if (ctx.webId && (auth.agentClasses || []).includes(ACL_AUTH_AGENT)) return true;
360+
return false;
361+
});
362+
if (!callerHasControl) {
363+
return toolError(
364+
`write_acl refused: the proposed ACL would not grant Control to the caller (${ctx.webId || 'anonymous'}). ` +
365+
'This is typically caused by relative WebID paths in agents resolving against the .acl URL — use absolute WebIDs. ' +
366+
'If you really want to remove your own access (e.g. transferring ownership), do it in two steps: ' +
367+
'first grant Control to the new owner, then have the new owner write_acl without you.'
368+
);
369+
}
370+
371+
await storage.write(aclPath, Buffer.from(serialized, 'utf8'), {
341372
contentType: 'application/ld+json'
342373
});
374+
emitChange(buildUrl(ctx, aclPath));
343375
return toolText(`wrote ${aclPath} (${authorizations.length} authorization${authorizations.length === 1 ? '' : 's'})`);
344376
}
345377

‎test/mcp.test.js‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,26 @@ describe('MCP server (--mcp enabled)', () => {
290290
assert.ok(classes.includes('foaf:Agent'));
291291
});
292292

293+
it('write_acl refuses to lock caller out (safety)', async () => {
294+
// Try to write an ACL that grants Control only to a foreign WebID
295+
// — would lock the caller (mcptest owner) out. Safety should refuse.
296+
const { body } = await rpc({
297+
jsonrpc: '2.0', id: 204, method: 'tools/call',
298+
params: { name: 'write_acl', arguments: {
299+
path: '/mcptest/public/',
300+
authorizations: [
301+
{
302+
agents: ['https://stranger.example/profile#me'],
303+
modes: ['Read', 'Write', 'Control'],
304+
isDefault: true
305+
}
306+
]
307+
} }
308+
}, { token });
309+
assert.ok(body.result.isError);
310+
assert.match(body.result.content[0].text, /lock the caller out|would not grant Control/i);
311+
});
312+
293313
it('write_acl denied without Control', async () => {
294314
const { body } = await rpc({
295315
jsonrpc: '2.0', id: 203, method: 'tools/call',

0 commit comments

Comments
 (0)