Skip to content

Commit 5ac96ee

Browse files
charlesvardemanclaude
charlesvardeman
and
claude
committed
fix(mcp): reject malformed lws:// percent-sequences + single surface registry
Review fixes JavaScriptSolidServer#4 and JavaScriptSolidServer#11: - JavaScriptSolidServer#4: parseUri validates decodeURIComponent up front and returns null on a malformed '%', so resources/read yields invalid-params instead of a raw URIError surfacing as -32603 INTERNAL_ERROR. Raw path kept (storage decodes). - JavaScriptSolidServer#11: new src/mcp/surface.js is the single declarative registry; uri.js parse sets and resources.js advertisement + dispatch derive from it. Guard test asserts parse/dispatch/advertisement stay in lockstep. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 4401ff8 commit 5ac96ee

4 files changed

Lines changed: 95 additions & 17 deletions

File tree

‎src/mcp/resources.js‎

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
// same read logic + wac() as the former read tools, so the no-oracle
66
// property is inherited, not reimplemented.
77
import { parseUri, fixedUri } from './uri.js';
8+
import { SURFACE_TEMPLATES, SURFACE_FIXED } from './surface.js';
89
import { wac, buildUrl, parentPath } from './wac.js';
910
import { ResourceError } from './errors.js';
1011
import { RPC_ERRORS } from './protocol.js';
@@ -17,25 +18,20 @@ import { readDeclaredTypes } from '../lws/type-metadata.js';
1718
import { describedbyTargets } from '../lws/constraint.js';
1819
import { buildStorageDescription } from '../lws/storage-description.js';
1920

20-
// --- template + fixed advertisement -----------------------------------------
21+
// --- template + fixed advertisement (derived from the surface registry) ------
2122

2223
export function listResourceTemplates() {
23-
return [
24-
{ uriTemplate: 'lws://resource/{+path}', name: 'resource', description: 'A resource body (any content type), enveloped as untrusted data.', mimeType: 'text/plain' },
25-
{ uriTemplate: 'lws://container/{+path}', name: 'container', description: 'A container listing (ldp:contains children).', mimeType: 'application/json' },
26-
{ uriTemplate: 'lws://linkset/{+path}', name: 'linkset', description: 'RFC 9264 linkset: anchor/up/type/describedby.', mimeType: 'application/linkset+json' },
27-
{ uriTemplate: 'lws://meta/{+path}', name: 'meta', description: 'Resource metadata (size/modified).', mimeType: 'application/json' },
28-
{ uriTemplate: 'lws://acl/{+path}', name: 'acl', description: 'Structured ACL (requires acl:Control).', mimeType: 'application/json' },
29-
{ uriTemplate: 'lws://skill/{+path}', name: 'skill', description: 'A skill file body.', mimeType: 'application/json' },
30-
];
24+
return SURFACE_TEMPLATES.map(t => ({
25+
uriTemplate: `lws://${t.kind}/{+path}`, name: t.kind,
26+
description: t.description, mimeType: t.mimeType,
27+
}));
3128
}
3229

3330
export function listFixedResources() {
34-
return [
35-
{ uri: 'lws://storage-description', name: 'storage-description', description: 'The LWS storage description (type:Storage + services).', mimeType: 'application/json' },
36-
{ uri: 'lws://pod-info', name: 'pod-info', description: 'Pod identity + MCP capabilities.', mimeType: 'application/json' },
37-
{ uri: 'lws://skills', name: 'skills', description: 'Skill index (WAC-filtered, no-oracle).', mimeType: 'application/json' },
38-
];
31+
return SURFACE_FIXED.map(f => ({
32+
uri: `lws://${f.name}`, name: f.name,
33+
description: f.description, mimeType: f.mimeType,
34+
}));
3935
}
4036

4137
// --- helpers ----------------------------------------------------------------
@@ -192,6 +188,11 @@ const KIND = {
192188
skill: readSkillResource,
193189
};
194190

191+
// Exposed so a guard test can assert the resolver maps cover exactly the
192+
// surface registry (no advertise-without-resolver / resolver-without-parse
193+
// drift — review #11). The dispatch below reads from these same maps.
194+
export const RESOLVERS = { KIND, FIXED };
195+
195196
// --- dispatch ---------------------------------------------------------------
196197

197198
export async function readResource(uri, ctx) {

‎src/mcp/surface.js‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// src/mcp/surface.js
2+
// The single declarative registry for the MCP Resources surface. One entry per
3+
// lws:// surface; the parse set (uri.js), the dispatch map + advertisement
4+
// (resources.js) all DERIVE from these arrays, so adding a surface is one entry
5+
// here + one resolver binding — never three hand-synced tables (review #11).
6+
7+
// Templated, path-addressed resources: lws://<kind>/<path>.
8+
export const SURFACE_TEMPLATES = [
9+
{ kind: 'resource', description: 'A resource body (any content type), enveloped as untrusted data.', mimeType: 'text/plain' },
10+
{ kind: 'container', description: 'A container listing (ldp:contains children).', mimeType: 'application/json' },
11+
{ kind: 'linkset', description: 'RFC 9264 linkset: anchor/up/type/describedby.', mimeType: 'application/linkset+json' },
12+
{ kind: 'meta', description: 'Resource metadata (size/modified).', mimeType: 'application/json' },
13+
{ kind: 'acl', description: 'Structured ACL (requires acl:Control).', mimeType: 'application/json' },
14+
{ kind: 'skill', description: 'A skill file body.', mimeType: 'application/json' },
15+
];
16+
17+
// Fixed, singleton resources: lws://<name>.
18+
export const SURFACE_FIXED = [
19+
{ name: 'storage-description', description: 'The LWS storage description (type:Storage + services).', mimeType: 'application/json' },
20+
{ name: 'pod-info', description: 'Pod identity + MCP capabilities.', mimeType: 'application/json' },
21+
{ name: 'skills', description: 'Skill index (WAC-filtered, no-oracle).', mimeType: 'application/json' },
22+
];
23+
24+
export const PATH_KINDS = new Set(SURFACE_TEMPLATES.map(t => t.kind));
25+
export const FIXED_NAMES = new Set(SURFACE_FIXED.map(f => f.name));

‎src/mcp/uri.js‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,27 @@
22
// The lws:// URI scheme for MCP Resources. Two shapes:
33
// templated: lws://<kind>/<path> kind ∈ resource|container|linkset|meta|acl|skill
44
// fixed: lws://<name> name ∈ storage-description|pod-info|skills
5+
// The valid kind/name sets come from the single surface registry (surface.js).
56
// <path> is an LDP pod path and may contain '/' (RFC 6570 {+path} reserved
67
// expansion). parseUri maps a concrete URI back to { kind, path } | { fixed };
7-
// an unknown scheme/kind/name → null (caller returns a not-found error).
8+
// an unknown scheme/kind/name OR a malformed percent-sequence → null (caller
9+
// returns a not-found / invalid-params error, never a raw URIError).
810

9-
export const PATH_KINDS = new Set(['resource', 'container', 'linkset', 'meta', 'acl', 'skill']);
10-
export const FIXED_NAMES = new Set(['storage-description', 'pod-info', 'skills']);
11+
import { PATH_KINDS, FIXED_NAMES } from './surface.js';
12+
13+
export { PATH_KINDS, FIXED_NAMES };
1114

1215
const SCHEME = 'lws://';
1316

17+
// A path is valid only if it survives decodeURIComponent — the storage layer
18+
// decodes it later, so a malformed '%' here would otherwise throw URIError deep
19+
// in the WAC/exists probe and surface as -32603 instead of invalid-params
20+
// (review #4). Validate here, but keep the RAW path so storage decodes once.
21+
function decodable(path) {
22+
try { decodeURIComponent(path); return true; }
23+
catch { return false; }
24+
}
25+
1426
export function parseUri(uri) {
1527
if (typeof uri !== 'string' || !uri.startsWith(SCHEME)) return null;
1628
const rest = uri.slice(SCHEME.length);
@@ -22,6 +34,7 @@ export function parseUri(uri) {
2234
if (!PATH_KINDS.has(kind)) return null;
2335
let path = rest.slice(slash); // includes the leading '/'
2436
if (!path.startsWith('/')) path = '/' + path;
37+
if (!decodable(path)) return null;
2538
return { kind, path };
2639
}
2740

‎test/mcp-v2-review-fixes.test.js‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
// test/mcp-v2-review-fixes.test.js
2+
// Unit coverage for the MCP v2 review-fix round (12 findings). Pure/unit
3+
// checks live here; live-pod behavior stays in the lws-pod make test-mcp-v2 gate.
4+
import { test } from 'node:test';
5+
import assert from 'node:assert/strict';
6+
import { parseUri } from '../src/mcp/uri.js';
7+
import { listResourceTemplates, listFixedResources, RESOLVERS } from '../src/mcp/resources.js';
8+
import { SURFACE_TEMPLATES, SURFACE_FIXED } from '../src/mcp/surface.js';
9+
10+
// --- #4: malformed percent-encoding in an lws:// path -----------------------
11+
12+
test('#4 parseUri rejects a malformed percent-sequence (no raw URIError downstream)', () => {
13+
// A lone/invalid % would reach decodeURIComponent in the storage layer and
14+
// throw URIError → -32603. parseUri must reject it up front → invalid-params.
15+
assert.equal(parseUri('lws://resource/dir/50%off'), null);
16+
assert.equal(parseUri('lws://resource/a%'), null);
17+
assert.equal(parseUri('lws://resource/a%zz'), null);
18+
});
19+
20+
test('#4 parseUri still accepts a valid percent-sequence (keeps it raw for storage)', () => {
21+
assert.deepEqual(parseUri('lws://resource/a%20b'), { kind: 'resource', path: '/a%20b' });
22+
});
23+
24+
// --- #11: one declarative registry, no hand-synced tables -------------------
25+
26+
test('#11 the parse set, dispatch map, and advertisement all derive from one registry', () => {
27+
const metaKinds = SURFACE_TEMPLATES.map(t => t.kind).sort();
28+
const resolverKinds = Object.keys(RESOLVERS.KIND).sort();
29+
const advertisedKinds = listResourceTemplates()
30+
.map(t => t.uriTemplate.replace('lws://', '').split('/')[0]).sort();
31+
assert.deepEqual(resolverKinds, metaKinds, 'every template kind has a resolver and vice versa');
32+
assert.deepEqual(advertisedKinds, metaKinds, 'advertisement matches the registry');
33+
34+
const metaFixed = SURFACE_FIXED.map(f => f.name).sort();
35+
const resolverFixed = Object.keys(RESOLVERS.FIXED).sort();
36+
const advertisedFixed = listFixedResources().map(r => r.uri.replace('lws://', '')).sort();
37+
assert.deepEqual(resolverFixed, metaFixed, 'every fixed name has a resolver and vice versa');
38+
assert.deepEqual(advertisedFixed, metaFixed, 'fixed advertisement matches the registry');
39+
});

0 commit comments

Comments
 (0)