diff --git a/src/SkillServer/Endpoints.cs b/src/SkillServer/Endpoints.cs index 7c40021..255cdd5 100644 --- a/src/SkillServer/Endpoints.cs +++ b/src/SkillServer/Endpoints.cs @@ -228,43 +228,34 @@ private static async Task UploadSkill( }); } - var referenceFiles = request.Form.Files.GetFiles("references"); - var hasReferences = referenceFiles.Count > 0; - - if (hasReferences) + var resourceFiles = request.Form.Files.GetFiles("resources"); + var resources = new List<(ResourcePath Path, Stream Content)>(); + try { - var resources = new List<(ResourcePath Path, Stream Content)>(); - foreach (var refFile in referenceFiles) + foreach (var resourceFile in resourceFiles) { - var relativePath = $"references/{refFile.FileName}"; - if (!ResourcePath.TryCreate(relativePath, out var resourcePath)) + if (!ResourcePath.TryCreate(resourceFile.FileName, out var resourcePath)) { return Results.BadRequest(new ErrorResponse { Error = "invalid_resource_path", - Message = $"Invalid resource path: '{relativePath}'. Must be in references/, scripts/, or assets/ directories." + Message = $"Invalid resource path: '{resourceFile.FileName}'. Must be a relative path in a subdirectory with no path traversal." }); } - resources.Add((resourcePath.Value, refFile.OpenReadStream())); + resources.Add((resourcePath.Value, resourceFile.OpenReadStream())); } await using var stream = file.OpenReadStream(); var result = await uploadService.UploadSkillWithResourcesAsync( skillName.Value, skillVersion.Value, stream, resources, category, ct); - foreach (var (_, content) in resources) - await content.DisposeAsync(); - return HandleUploadResult(result, configuration); } - else + finally { - await using var stream = file.OpenReadStream(); - var result = await uploadService.UploadSkillMdAsync( - skillName.Value, skillVersion.Value, stream, category, ct); - - return HandleUploadResult(result, configuration); + foreach (var (_, content) in resources) + await content.DisposeAsync(); } } diff --git a/src/SkillServer/Models/ValueObjects.cs b/src/SkillServer/Models/ValueObjects.cs index 1a6632a..9f545f0 100644 --- a/src/SkillServer/Models/ValueObjects.cs +++ b/src/SkillServer/Models/ValueObjects.cs @@ -144,8 +144,6 @@ public static Sha256Digest FromHex(string hexValue) /// public readonly record struct ResourcePath { - private static readonly string[] AllowedPrefixes = ["references/", "scripts/", "assets/"]; - public string Value { get; } private ResourcePath(string value) => Value = value; @@ -157,15 +155,13 @@ public static bool TryCreate(string? value, [NotNullWhen(true)] out ResourcePath if (string.IsNullOrWhiteSpace(value)) return false; - // No path traversal if (value.Contains("..") || value.StartsWith('/') || value.StartsWith('\\')) return false; - // Normalize separators var normalized = value.Replace('\\', '/'); - // Must be in allowed directories - if (!AllowedPrefixes.Any(p => normalized.StartsWith(p, StringComparison.OrdinalIgnoreCase))) + // Must be in a subdirectory — bare filenames at the root are not resources + if (!normalized.Contains('/') || normalized.IndexOf('/') == normalized.Length - 1) return false; result = new ResourcePath(normalized); @@ -175,7 +171,7 @@ public static bool TryCreate(string? value, [NotNullWhen(true)] out ResourcePath public static ResourcePath Create(string value) { if (!TryCreate(value, out var result)) - throw new ArgumentException($"Invalid resource path: '{value}'. Must be in references/, scripts/, or assets/ directories with no path traversal.", nameof(value)); + throw new ArgumentException($"Invalid resource path: '{value}'. Must be a relative path in a subdirectory with no path traversal.", nameof(value)); return result.Value; } diff --git a/tests/SkillServer.Integration.Tests/SkillServerIntegrationTests.cs b/tests/SkillServer.Integration.Tests/SkillServerIntegrationTests.cs index 5a0cd78..95a4249 100644 --- a/tests/SkillServer.Integration.Tests/SkillServerIntegrationTests.cs +++ b/tests/SkillServer.Integration.Tests/SkillServerIntegrationTests.cs @@ -608,7 +608,7 @@ public async Task SearchSkills_WithPorterStemming_MatchesStemmedTerms() } [Fact] - public async Task UploadSkillWithReferences_EndToEnd() + public async Task UploadSkillWithResources_EndToEnd() { var ct = TestContext.Current.CancellationToken; var skillName = $"ref-test-{Guid.NewGuid():N}"[..20]; @@ -616,10 +616,10 @@ public async Task UploadSkillWithReferences_EndToEnd() var skillContent = $""" --- name: {skillName} - description: Testing reference file uploads + description: Testing resource file uploads --- - # Reference Upload Test + # Resource Upload Test See references/guide.md for details. """; @@ -636,13 +636,11 @@ First section content. Second section content. """; - var referenceContent2 = """ - # Patterns - - Common usage patterns for this skill. + var scriptContent = """ + #!/bin/bash + echo "setup complete" """; - // Upload skill with two reference files using var content = new MultipartFormDataContent(); content.Add(new StringContent(skillName), "name"); content.Add(new StringContent("1.0.0"), "version"); @@ -653,25 +651,22 @@ Second section content. var refFileContent = new ByteArrayContent(Encoding.UTF8.GetBytes(referenceContent)); refFileContent.Headers.ContentType = new MediaTypeHeaderValue("text/markdown"); - content.Add(refFileContent, "references", "guide.md"); + content.Add(refFileContent, "resources", "references/guide.md"); - var refFileContent2 = new ByteArrayContent(Encoding.UTF8.GetBytes(referenceContent2)); - refFileContent2.Headers.ContentType = new MediaTypeHeaderValue("text/markdown"); - content.Add(refFileContent2, "references", "patterns.md"); + var scriptFileContent = new ByteArrayContent(Encoding.UTF8.GetBytes(scriptContent)); + scriptFileContent.Headers.ContentType = new MediaTypeHeaderValue("application/x-sh"); + content.Add(scriptFileContent, "resources", "scripts/setup.sh"); var uploadResponse = await _fixture.AuthenticatedHttpClient.PostAsync("/skills", content, ct); Assert.Equal(HttpStatusCode.Created, uploadResponse.StatusCode); - // Verify the version shows file count (SKILL.md + 2 references) var version = await _fixture.Client.GetVersionAsync(skillName, "1.0.0", ct); Assert.NotNull(version); Assert.Equal(2, version.FileCount); - // Download SKILL.md var downloadedSkill = await _fixture.Client.GetSkillFileAsStringAsync(skillName, "1.0.0", ct: ct); - Assert.Contains("# Reference Upload Test", downloadedSkill); + Assert.Contains("# Resource Upload Test", downloadedSkill); - // Download reference files via resource endpoint var refResponse = await _fixture.HttpClient.GetAsync( $"/skills/{skillName}/1.0.0/references/guide.md", ct); Assert.Equal(HttpStatusCode.OK, refResponse.StatusCode); @@ -679,13 +674,12 @@ Second section content. Assert.Contains("# Guide", refBody); Assert.Contains("Section One", refBody); - var refResponse2 = await _fixture.HttpClient.GetAsync( - $"/skills/{skillName}/1.0.0/references/patterns.md", ct); - Assert.Equal(HttpStatusCode.OK, refResponse2.StatusCode); - var refBody2 = await refResponse2.Content.ReadAsStringAsync(ct); - Assert.Contains("# Patterns", refBody2); + var scriptResponse = await _fixture.HttpClient.GetAsync( + $"/skills/{skillName}/1.0.0/scripts/setup.sh", ct); + Assert.Equal(HttpStatusCode.OK, scriptResponse.StatusCode); + var scriptBody = await scriptResponse.Content.ReadAsStringAsync(ct); + Assert.Contains("setup complete", scriptBody); - // Verify resources appear in RFC index var index = await _fixture.Client.GetRfcIndexAsync(ct); Assert.NotNull(index); var indexSkill = index.Skills.FirstOrDefault(s => s.Name == skillName); @@ -694,11 +688,11 @@ Second section content. Assert.NotNull(resources); Assert.Equal(2, resources!.Count); Assert.Contains(resources, r => r.Path == "references/guide.md"); - Assert.Contains(resources, r => r.Path == "references/patterns.md"); + Assert.Contains(resources, r => r.Path == "scripts/setup.sh"); } [Fact] - public async Task UploadSkillWithReferences_WithoutReferences_StillWorks() + public async Task UploadSkillWithResources_WithoutResources_StillWorks() { var ct = TestContext.Current.CancellationToken; var skillName = $"noref-{Guid.NewGuid():N}"[..20]; @@ -706,10 +700,10 @@ public async Task UploadSkillWithReferences_WithoutReferences_StillWorks() var skillContent = $""" --- name: {skillName} - description: Testing upload without references still works + description: Testing upload without resources still works --- - # No References Test + # No Resources Test """; using var content = new MultipartFormDataContent(); @@ -727,4 +721,35 @@ public async Task UploadSkillWithReferences_WithoutReferences_StillWorks() Assert.NotNull(version); Assert.Equal(0, version.FileCount); } + + [Fact] + public async Task UploadSkillWithResources_InvalidPath_ReturnsBadRequest() + { + var ct = TestContext.Current.CancellationToken; + var skillName = $"badpath-{Guid.NewGuid():N}"[..20]; + + var skillContent = $""" + --- + name: {skillName} + description: Testing invalid resource path rejection + --- + + # Bad Path Test + """; + + using var content = new MultipartFormDataContent(); + content.Add(new StringContent(skillName), "name"); + content.Add(new StringContent("1.0.0"), "version"); + + var fileContent = new ByteArrayContent(Encoding.UTF8.GetBytes(skillContent)); + fileContent.Headers.ContentType = new MediaTypeHeaderValue("text/markdown"); + content.Add(fileContent, "file", "SKILL.md"); + + var badFile = new ByteArrayContent(Encoding.UTF8.GetBytes("exploit")); + badFile.Headers.ContentType = new MediaTypeHeaderValue("text/plain"); + content.Add(badFile, "resources", "../etc/passwd"); + + var uploadResponse = await _fixture.AuthenticatedHttpClient.PostAsync("/skills", content, ct); + Assert.Equal(HttpStatusCode.BadRequest, uploadResponse.StatusCode); + } } diff --git a/tests/SkillServer.Tests/ValueObjectTests.cs b/tests/SkillServer.Tests/ValueObjectTests.cs index 6d764b4..19e4f9c 100644 --- a/tests/SkillServer.Tests/ValueObjectTests.cs +++ b/tests/SkillServer.Tests/ValueObjectTests.cs @@ -115,10 +115,12 @@ public sealed class ResourcePathTests [InlineData("scripts/setup.sh", true)] [InlineData("assets/logo.png", true)] [InlineData("references/deep/nested/file.md", true)] + [InlineData("custom/file.txt", true)] + [InlineData("examples/demo.py", true)] [InlineData("../secret.txt", false)] [InlineData("/etc/passwd", false)] - [InlineData("SKILL.md", false)] // Must be in allowed directories - [InlineData("random/file.txt", false)] + [InlineData("SKILL.md", false)] // Bare filenames are not resources + [InlineData("justadirectory/", false)] [InlineData("", false)] public void TryCreate_ValidatesCorrectly(string input, bool expectedValid) {