Skip to content

Commit fdbd2a7

Browse files
AarononthewebAaronontheweb
authored
Clean up resource upload endpoint (#53)
* Clean up resource upload endpoint Fix stream leak on validation failure by wrapping resource handling in try/finally. Remove unnecessary if/else branch — always route through UploadSkillWithResourcesAsync since it handles empty resource lists. Rename form field from "references" to "resources" and let clients specify the full relative path (e.g. references/guide.md, scripts/setup.sh) so all allowed directories are accessible, not just references/. Add integration test for path traversal rejection. * Allow arbitrary resource subdirectories per AgentSkills.io spec The spec allows any additional files and directories, not just references/, scripts/, and assets/. Replace the hardcoded allowlist in ResourcePath with a check that the path is in any subdirectory. Safety rules remain: no path traversal, no absolute paths, no bare filenames at the root level.
1 parent 5cd51cd commit fdbd2a7

4 files changed

Lines changed: 68 additions & 54 deletions

File tree

‎src/SkillServer/Endpoints.cs‎

Lines changed: 10 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -228,43 +228,34 @@ private static async Task<IResult> UploadSkill(
228228
});
229229
}
230230

231-
var referenceFiles = request.Form.Files.GetFiles("references");
232-
var hasReferences = referenceFiles.Count > 0;
233-
234-
if (hasReferences)
231+
var resourceFiles = request.Form.Files.GetFiles("resources");
232+
var resources = new List<(ResourcePath Path, Stream Content)>();
233+
try
235234
{
236-
var resources = new List<(ResourcePath Path, Stream Content)>();
237-
foreach (var refFile in referenceFiles)
235+
foreach (var resourceFile in resourceFiles)
238236
{
239-
var relativePath = $"references/{refFile.FileName}";
240-
if (!ResourcePath.TryCreate(relativePath, out var resourcePath))
237+
if (!ResourcePath.TryCreate(resourceFile.FileName, out var resourcePath))
241238
{
242239
return Results.BadRequest(new ErrorResponse
243240
{
244241
Error = "invalid_resource_path",
245-
Message = $"Invalid resource path: '{relativePath}'. Must be in references/, scripts/, or assets/ directories."
242+
Message = $"Invalid resource path: '{resourceFile.FileName}'. Must be a relative path in a subdirectory with no path traversal."
246243
});
247244
}
248245

249-
resources.Add((resourcePath.Value, refFile.OpenReadStream()));
246+
resources.Add((resourcePath.Value, resourceFile.OpenReadStream()));
250247
}
251248

252249
await using var stream = file.OpenReadStream();
253250
var result = await uploadService.UploadSkillWithResourcesAsync(
254251
skillName.Value, skillVersion.Value, stream, resources, category, ct);
255252

256-
foreach (var (_, content) in resources)
257-
await content.DisposeAsync();
258-
259253
return HandleUploadResult(result, configuration);
260254
}
261-
else
255+
finally
262256
{
263-
await using var stream = file.OpenReadStream();
264-
var result = await uploadService.UploadSkillMdAsync(
265-
skillName.Value, skillVersion.Value, stream, category, ct);
266-
267-
return HandleUploadResult(result, configuration);
257+
foreach (var (_, content) in resources)
258+
await content.DisposeAsync();
268259
}
269260
}
270261

‎src/SkillServer/Models/ValueObjects.cs‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,6 @@ public static Sha256Digest FromHex(string hexValue)
144144
/// </summary>
145145
public readonly record struct ResourcePath
146146
{
147-
private static readonly string[] AllowedPrefixes = ["references/", "scripts/", "assets/"];
148-
149147
public string Value { get; }
150148

151149
private ResourcePath(string value) => Value = value;
@@ -157,15 +155,13 @@ public static bool TryCreate(string? value, [NotNullWhen(true)] out ResourcePath
157155
if (string.IsNullOrWhiteSpace(value))
158156
return false;
159157

160-
// No path traversal
161158
if (value.Contains("..") || value.StartsWith('/') || value.StartsWith('\\'))
162159
return false;
163160

164-
// Normalize separators
165161
var normalized = value.Replace('\\', '/');
166162

167-
// Must be in allowed directories
168-
if (!AllowedPrefixes.Any(p => normalized.StartsWith(p, StringComparison.OrdinalIgnoreCase)))
163+
// Must be in a subdirectory — bare filenames at the root are not resources
164+
if (!normalized.Contains('/') || normalized.IndexOf('/') == normalized.Length - 1)
169165
return false;
170166

171167
result = new ResourcePath(normalized);
@@ -175,7 +171,7 @@ public static bool TryCreate(string? value, [NotNullWhen(true)] out ResourcePath
175171
public static ResourcePath Create(string value)
176172
{
177173
if (!TryCreate(value, out var result))
178-
throw new ArgumentException($"Invalid resource path: '{value}'. Must be in references/, scripts/, or assets/ directories with no path traversal.", nameof(value));
174+
throw new ArgumentException($"Invalid resource path: '{value}'. Must be a relative path in a subdirectory with no path traversal.", nameof(value));
179175
return result.Value;
180176
}
181177

‎tests/SkillServer.Integration.Tests/SkillServerIntegrationTests.cs‎

Lines changed: 51 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -608,18 +608,18 @@ public async Task SearchSkills_WithPorterStemming_MatchesStemmedTerms()
608608
}
609609

610610
[Fact]
611-
public async Task UploadSkillWithReferences_EndToEnd()
611+
public async Task UploadSkillWithResources_EndToEnd()
612612
{
613613
var ct = TestContext.Current.CancellationToken;
614614
var skillName = $"ref-test-{Guid.NewGuid():N}"[..20];
615615

616616
var skillContent = $"""
617617
---
618618
name: {skillName}
619-
description: Testing reference file uploads
619+
description: Testing resource file uploads
620620
---
621621
622-
# Reference Upload Test
622+
# Resource Upload Test
623623
624624
See references/guide.md for details.
625625
""";
@@ -636,13 +636,11 @@ First section content.
636636
Second section content.
637637
""";
638638

639-
var referenceContent2 = """
640-
# Patterns
641-
642-
Common usage patterns for this skill.
639+
var scriptContent = """
640+
#!/bin/bash
641+
echo "setup complete"
643642
""";
644643

645-
// Upload skill with two reference files
646644
using var content = new MultipartFormDataContent();
647645
content.Add(new StringContent(skillName), "name");
648646
content.Add(new StringContent("1.0.0"), "version");
@@ -653,39 +651,35 @@ Second section content.
653651

654652
var refFileContent = new ByteArrayContent(Encoding.UTF8.GetBytes(referenceContent));
655653
refFileContent.Headers.ContentType = new MediaTypeHeaderValue("text/markdown");
656-
content.Add(refFileContent, "references", "guide.md");
654+
content.Add(refFileContent, "resources", "references/guide.md");
657655

658-
var refFileContent2 = new ByteArrayContent(Encoding.UTF8.GetBytes(referenceContent2));
659-
refFileContent2.Headers.ContentType = new MediaTypeHeaderValue("text/markdown");
660-
content.Add(refFileContent2, "references", "patterns.md");
656+
var scriptFileContent = new ByteArrayContent(Encoding.UTF8.GetBytes(scriptContent));
657+
scriptFileContent.Headers.ContentType = new MediaTypeHeaderValue("application/x-sh");
658+
content.Add(scriptFileContent, "resources", "scripts/setup.sh");
661659

662660
var uploadResponse = await _fixture.AuthenticatedHttpClient.PostAsync("/skills", content, ct);
663661
Assert.Equal(HttpStatusCode.Created, uploadResponse.StatusCode);
664662

665-
// Verify the version shows file count (SKILL.md + 2 references)
666663
var version = await _fixture.Client.GetVersionAsync(skillName, "1.0.0", ct);
667664
Assert.NotNull(version);
668665
Assert.Equal(2, version.FileCount);
669666

670-
// Download SKILL.md
671667
var downloadedSkill = await _fixture.Client.GetSkillFileAsStringAsync(skillName, "1.0.0", ct: ct);
672-
Assert.Contains("# Reference Upload Test", downloadedSkill);
668+
Assert.Contains("# Resource Upload Test", downloadedSkill);
673669

674-
// Download reference files via resource endpoint
675670
var refResponse = await _fixture.HttpClient.GetAsync(
676671
$"/skills/{skillName}/1.0.0/references/guide.md", ct);
677672
Assert.Equal(HttpStatusCode.OK, refResponse.StatusCode);
678673
var refBody = await refResponse.Content.ReadAsStringAsync(ct);
679674
Assert.Contains("# Guide", refBody);
680675
Assert.Contains("Section One", refBody);
681676

682-
var refResponse2 = await _fixture.HttpClient.GetAsync(
683-
$"/skills/{skillName}/1.0.0/references/patterns.md", ct);
684-
Assert.Equal(HttpStatusCode.OK, refResponse2.StatusCode);
685-
var refBody2 = await refResponse2.Content.ReadAsStringAsync(ct);
686-
Assert.Contains("# Patterns", refBody2);
677+
var scriptResponse = await _fixture.HttpClient.GetAsync(
678+
$"/skills/{skillName}/1.0.0/scripts/setup.sh", ct);
679+
Assert.Equal(HttpStatusCode.OK, scriptResponse.StatusCode);
680+
var scriptBody = await scriptResponse.Content.ReadAsStringAsync(ct);
681+
Assert.Contains("setup complete", scriptBody);
687682

688-
// Verify resources appear in RFC index
689683
var index = await _fixture.Client.GetRfcIndexAsync(ct);
690684
Assert.NotNull(index);
691685
var indexSkill = index.Skills.FirstOrDefault(s => s.Name == skillName);
@@ -694,22 +688,22 @@ Second section content.
694688
Assert.NotNull(resources);
695689
Assert.Equal(2, resources!.Count);
696690
Assert.Contains(resources, r => r.Path == "references/guide.md");
697-
Assert.Contains(resources, r => r.Path == "references/patterns.md");
691+
Assert.Contains(resources, r => r.Path == "scripts/setup.sh");
698692
}
699693

700694
[Fact]
701-
public async Task UploadSkillWithReferences_WithoutReferences_StillWorks()
695+
public async Task UploadSkillWithResources_WithoutResources_StillWorks()
702696
{
703697
var ct = TestContext.Current.CancellationToken;
704698
var skillName = $"noref-{Guid.NewGuid():N}"[..20];
705699

706700
var skillContent = $"""
707701
---
708702
name: {skillName}
709-
description: Testing upload without references still works
703+
description: Testing upload without resources still works
710704
---
711705
712-
# No References Test
706+
# No Resources Test
713707
""";
714708

715709
using var content = new MultipartFormDataContent();
@@ -727,4 +721,35 @@ public async Task UploadSkillWithReferences_WithoutReferences_StillWorks()
727721
Assert.NotNull(version);
728722
Assert.Equal(0, version.FileCount);
729723
}
724+
725+
[Fact]
726+
public async Task UploadSkillWithResources_InvalidPath_ReturnsBadRequest()
727+
{
728+
var ct = TestContext.Current.CancellationToken;
729+
var skillName = $"badpath-{Guid.NewGuid():N}"[..20];
730+
731+
var skillContent = $"""
732+
---
733+
name: {skillName}
734+
description: Testing invalid resource path rejection
735+
---
736+
737+
# Bad Path Test
738+
""";
739+
740+
using var content = new MultipartFormDataContent();
741+
content.Add(new StringContent(skillName), "name");
742+
content.Add(new StringContent("1.0.0"), "version");
743+
744+
var fileContent = new ByteArrayContent(Encoding.UTF8.GetBytes(skillContent));
745+
fileContent.Headers.ContentType = new MediaTypeHeaderValue("text/markdown");
746+
content.Add(fileContent, "file", "SKILL.md");
747+
748+
var badFile = new ByteArrayContent(Encoding.UTF8.GetBytes("exploit"));
749+
badFile.Headers.ContentType = new MediaTypeHeaderValue("text/plain");
750+
content.Add(badFile, "resources", "../etc/passwd");
751+
752+
var uploadResponse = await _fixture.AuthenticatedHttpClient.PostAsync("/skills", content, ct);
753+
Assert.Equal(HttpStatusCode.BadRequest, uploadResponse.StatusCode);
754+
}
730755
}

‎tests/SkillServer.Tests/ValueObjectTests.cs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,10 +115,12 @@ public sealed class ResourcePathTests
115115
[InlineData("scripts/setup.sh", true)]
116116
[InlineData("assets/logo.png", true)]
117117
[InlineData("references/deep/nested/file.md", true)]
118+
[InlineData("custom/file.txt", true)]
119+
[InlineData("examples/demo.py", true)]
118120
[InlineData("../secret.txt", false)]
119121
[InlineData("/etc/passwd", false)]
120-
[InlineData("SKILL.md", false)] // Must be in allowed directories
121-
[InlineData("random/file.txt", false)]
122+
[InlineData("SKILL.md", false)] // Bare filenames are not resources
123+
[InlineData("justadirectory/", false)]
122124
[InlineData("", false)]
123125
public void TryCreate_ValidatesCorrectly(string input, bool expectedValid)
124126
{

0 commit comments

Comments
 (0)