Skip to content

Commit 150d9f9

Browse files
PederHPclaude
PederHP
and
claude
committed
Complete earlier fixes and align the reader further with reference parsers
Five earlier review threads were addressed for the case raised but not its neighbour. This commit closes them properly: - InMemoryMcpSkillCatalog now hands out copies from ListAsync and GetAsync, not only on construction, so a mutated result cannot change later responses. - URI validation decodes each path segment before checking for '.', '..', and separators, since System.Uri (and therefore the resource collection) treats "%2e%2e" as "..". - An unterminated quoted scalar is malformed and no longer reaches the explicit-frontmatter escape hatch; a quote closed on a later line is still classified as unsupported multi-line YAML and remains bypassable. - Directory loading reads each file with a buffer of the length it checked and fails if the file changed size, so a file growing between the check and the read can neither exceed the limit nor be served with a manifest that does not describe it. - URI aliases that differ only in scheme or authority case are rejected even with identical content, because the resource collection would serve one resource whose contents carry the first URI, failing a verified read of the second. Further alignment with reference YAML parsers, each confirmed against the yaml npm package: plain mapping keys resolve like values ("TRUE:" is the key "true", "0x10:" is "16"), flow mappings honour YAML's separator rules ("{version:1}" is the key "version:1" with a null value), a plain scalar ended by a comment cannot continue on the next line, and only space and tab are trimmed, so non-breaking spaces stay in scalars. Nesting is capped at 32 levels so a hostile file cannot exhaust the stack. Skill names accept Unicode lowercase and caseless letters, per the Agent Skills wording "unicode lowercase alphanumeric characters". A test that relied on reflection-based serialization now uses the SDK's options, so the skills tests pass on net9.0 and net8.0 as well as net10.0. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01P6omaTvQiryHtzFHRqUE3b
1 parent 8e8379a commit 150d9f9

11 files changed

Lines changed: 338 additions & 52 deletions

File tree

‎docs/concepts/skills/skills.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,8 +88,8 @@ names containing characters with URI syntax (such as `{`, `?`, or a space) are p
8888

8989
<xref:ModelContextProtocol.Extensions.Skills.SkillFrontmatter> reads the YAML frontmatter of a `SKILL.md` into a
9090
<xref:System.Text.Json.Nodes.JsonObject> without a YAML library. It accepts the subset of YAML that Agent Skills
91-
frontmatter uses: block mappings nested to any depth, block and flow sequences, plain, quoted, and block scalars,
92-
and comments. Unquoted scalars are resolved per the YAML 1.2 core schema (`null`, booleans, integers, finite
91+
frontmatter uses: block mappings (nested up to 32 levels), block and flow sequences, plain, quoted, and block
92+
scalars, and comments. Unquoted scalars are resolved per the YAML 1.2 core schema (`null`, booleans, integers, finite
9393
floats, otherwise strings), matching the YAML libraries used by other SDKs and by hosts. That matters because a
9494
host verifies a skill by parsing the fetched `SKILL.md` itself and comparing field by field against the published
9595
entry; a value that one side types as a number and the other as a string is a verification failure. Quote values

‎src/ModelContextProtocol.Extensions.Skills/Server/InMemoryMcpSkillCatalog.cs‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@ namespace ModelContextProtocol.Extensions.Skills;
1111
/// so a server cannot publish an entry a conforming host would refuse to load.
1212
/// </para>
1313
/// <para>
14-
/// The catalog keeps its own copy of every entry, so later changes to the objects passed to the constructor do
15-
/// not affect what is served. Entries are ordered by URI so that pagination is stable across calls. Cursors are
14+
/// The catalog keeps its own copy of every entry and hands out copies, so neither later changes to the objects
15+
/// passed to the constructor nor changes to a returned entry affect what is served. Entries are ordered by URI so that pagination is stable across calls. Cursors are
1616
/// keyset cursors over that order rather than offsets.
1717
/// </para>
1818
/// <para>
@@ -91,7 +91,10 @@ public ValueTask<McpSkillPage> ListAsync(string? cursor, McpSkillRequestContext
9191
}
9292

9393
var page = new Skill[count];
94-
Array.Copy(_ordered, start, page, 0, count);
94+
for (int i = 0; i < count; i++)
95+
{
96+
page[i] = SkillValidation.Snapshot(_ordered[start + i]);
97+
}
9598

9699
bool hasMore = start + count < _ordered.Length;
97100
return new ValueTask<McpSkillPage>(new McpSkillPage
@@ -112,8 +115,7 @@ public ValueTask<McpSkillPage> ListAsync(string? cursor, McpSkillRequestContext
112115

113116
cancellationToken.ThrowIfCancellationRequested();
114117

115-
_byUri.TryGetValue(uri, out var skill);
116-
return new ValueTask<Skill?>(skill);
118+
return new ValueTask<Skill?>(_byUri.TryGetValue(uri, out var skill) ? SkillValidation.Snapshot(skill) : null);
117119
}
118120

119121
private static string EncodeCursor(string uri) => Convert.ToBase64String(Encoding.UTF8.GetBytes(uri));

‎src/ModelContextProtocol.Extensions.Skills/Server/McpServerSkill.cs‎

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -484,11 +484,40 @@ private static void CollectFiles(string root, string directory, List<McpServerSk
484484
files.Add(new McpServerSkillFile
485485
{
486486
Path = relativePath,
487-
Content = File.ReadAllBytes(entry),
487+
Content = ReadExactly(entry, length),
488488
});
489489
}
490490
}
491491

492+
/// <summary>
493+
/// Reads a file whose length was checked against the per-skill limits a moment ago, allocating only that
494+
/// length, and fails if the file turns out to be a different size, so a file that grows or shrinks between the
495+
/// check and the read can neither exceed the limit nor be served with a manifest that does not describe it.
496+
/// </summary>
497+
private static byte[] ReadExactly(string path, long expectedLength)
498+
{
499+
var buffer = new byte[expectedLength];
500+
using var stream = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.Read);
501+
int total = 0;
502+
while (total < buffer.Length)
503+
{
504+
int read = stream.Read(buffer, total, buffer.Length - total);
505+
if (read == 0)
506+
{
507+
break;
508+
}
509+
510+
total += read;
511+
}
512+
513+
if (total != buffer.Length || stream.ReadByte() != -1)
514+
{
515+
throw new ArgumentException($"'{path}' changed size while it was being read.", "directoryPath");
516+
}
517+
518+
return buffer;
519+
}
520+
492521
/// <summary>
493522
/// Percent-encodes each segment of a normalized relative path so that characters with URI syntax (such as
494523
/// <c>{</c>, <c>?</c>, <c>#</c>, or a space) in a file name stay literal. Without this, a file named

‎src/ModelContextProtocol.Extensions.Skills/Server/McpSkillsBuilderExtensions.cs‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,8 @@ public static class McpSkillsBuilderExtensions
3232
/// </para>
3333
/// <para>
3434
/// Nested skills may legitimately list the same file. A file URI shared by several skills is registered once,
35-
/// provided every skill lists it with the same digest.
35+
/// provided every skill lists it with the same digest and the identical URI text. URIs that differ only in the
36+
/// case of their scheme or authority are rejected, because the server's resource collection treats them as one.
3637
/// </para>
3738
/// <para>
3839
/// Every caller sees every skill. <c>skills/list</c> and <c>skills/get</c> are raw request handlers and do not
@@ -77,12 +78,20 @@ public static IMcpServerBuilder WithSkills(
7778
var key = new Uri(entry.Uri, UriKind.Absolute);
7879
if (registeredFiles.TryGetValue(key, out var existing))
7980
{
81+
if (!string.Equals(existing.Uri, entry.Uri, StringComparison.Ordinal))
82+
{
83+
// Even with identical content the alias cannot be served: the collection registers one
84+
// resource, whose contents carry the first URI, so a verified read of the second fails.
85+
throw new ArgumentException(
86+
$"The file '{entry.Uri}' is equivalent to '{existing.Uri}', listed by another skill. Resource URIs are compared " +
87+
"case-insensitively in their scheme and authority, so the two would be served as one. Use identical URIs for a shared file.",
88+
nameof(skills));
89+
}
90+
8091
if (!string.Equals(existing.Digest, entry.Digest, StringComparison.Ordinal))
8192
{
8293
throw new ArgumentException(
83-
string.Equals(existing.Uri, entry.Uri, StringComparison.Ordinal)
84-
? $"The file '{entry.Uri}' is listed by more than one skill with different content."
85-
: $"The file '{entry.Uri}' is equivalent to '{existing.Uri}', which another skill lists with different content. Resource URIs are compared case-insensitively in their scheme and authority.",
94+
$"The file '{entry.Uri}' is listed by more than one skill with different content.",
8695
nameof(skills));
8796
}
8897

0 commit comments

Comments
 (0)