From ddbf92435fe40d4b225c0669d0ea1efefe2d6605 Mon Sep 17 00:00:00 2001 From: yun-zhi-ztl <15071461069@163.com> Date: Fri, 13 Mar 2026 10:20:34 +0800 Subject: [PATCH] fix(upload): harden package extraction and storage boundaries - add shared package safety policy for path normalization and size limits - stream zip extraction for cli check and publish flows to reject traversal and oversized entries - confine local storage keys to the configured base path and add regression coverage --- .../skillhub/controller/CliController.java | 53 ++++------- .../controller/cli/CliPublishController.java | 46 +++------- .../portal/SkillPublishController.java | 46 +++------- .../support/SkillPackageArchiveExtractor.java | 92 +++++++++++++++++++ .../controller/CliControllerTest.java | 30 ++++++ .../SkillPackageArchiveExtractorTest.java | 61 ++++++++++++ .../skill/validation/SkillPackagePolicy.java | 56 +++++++++++ .../validation/SkillPackageValidator.java | 66 ++++++------- .../validation/SkillPackageValidatorTest.java | 45 +++++++++ .../storage/LocalFileStorageService.java | 10 +- .../storage/LocalFileStorageServiceTest.java | 16 ++++ 11 files changed, 380 insertions(+), 141 deletions(-) create mode 100644 server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/support/SkillPackageArchiveExtractor.java create mode 100644 server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/support/SkillPackageArchiveExtractorTest.java create mode 100644 server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/SkillPackagePolicy.java diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/CliController.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/CliController.java index cf0273ec..5363eab7 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/CliController.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/CliController.java @@ -1,5 +1,6 @@ package com.iflytek.skillhub.controller; +import com.iflytek.skillhub.controller.support.SkillPackageArchiveExtractor; import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; import com.iflytek.skillhub.domain.skill.validation.PackageEntry; import com.iflytek.skillhub.domain.skill.validation.SkillPackageValidator; @@ -14,21 +15,21 @@ import org.springframework.web.bind.annotation.*; import org.springframework.web.multipart.MultipartFile; import java.io.IOException; -import java.util.ArrayList; import java.util.List; -import java.util.zip.ZipEntry; -import java.util.zip.ZipInputStream; @RestController @RequestMapping("/api/v1/cli") public class CliController extends BaseApiController { private final SkillPackageValidator skillPackageValidator; + private final SkillPackageArchiveExtractor skillPackageArchiveExtractor; public CliController(ApiResponseFactory responseFactory, - SkillPackageValidator skillPackageValidator) { + SkillPackageValidator skillPackageValidator, + SkillPackageArchiveExtractor skillPackageArchiveExtractor) { super(responseFactory); this.skillPackageValidator = skillPackageValidator; + this.skillPackageArchiveExtractor = skillPackageArchiveExtractor; } @GetMapping("/whoami") @@ -42,7 +43,18 @@ public class CliController extends BaseApiController { @PostMapping("/check") public ApiResponse check(@RequestParam("file") MultipartFile file) throws IOException { - List entries = extractZipEntries(file); + List entries; + try { + entries = skillPackageArchiveExtractor.extract(file); + } catch (IllegalArgumentException e) { + SkillCheckResponse response = new SkillCheckResponse( + false, + List.of(e.getMessage()), + 0, + 0L + ); + return ok("response.success.validated", response); + } ValidationResult result = skillPackageValidator.validate(entries); SkillCheckResponse response = new SkillCheckResponse( @@ -54,35 +66,4 @@ public class CliController extends BaseApiController { return ok("response.success.validated", response); } - - private List extractZipEntries(MultipartFile file) throws IOException { - List entries = new ArrayList<>(); - - try (ZipInputStream zis = new ZipInputStream(file.getInputStream())) { - ZipEntry zipEntry; - while ((zipEntry = zis.getNextEntry()) != null) { - if (!zipEntry.isDirectory()) { - byte[] content = zis.readAllBytes(); - entries.add(new PackageEntry( - zipEntry.getName(), - content, - content.length, - determineContentType(zipEntry.getName()) - )); - } - zis.closeEntry(); - } - } - - return entries; - } - - private String determineContentType(String filename) { - if (filename.endsWith(".py")) return "text/x-python"; - if (filename.endsWith(".json")) return "application/json"; - if (filename.endsWith(".yaml") || filename.endsWith(".yml")) return "application/x-yaml"; - if (filename.endsWith(".txt")) return "text/plain"; - if (filename.endsWith(".md")) return "text/markdown"; - return "application/octet-stream"; - } } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/cli/CliPublishController.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/cli/CliPublishController.java index 76bae215..953ee4f8 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/cli/CliPublishController.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/cli/CliPublishController.java @@ -1,6 +1,8 @@ package com.iflytek.skillhub.controller.cli; import com.iflytek.skillhub.controller.BaseApiController; +import com.iflytek.skillhub.controller.support.SkillPackageArchiveExtractor; +import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException; import com.iflytek.skillhub.domain.skill.SkillVisibility; import com.iflytek.skillhub.domain.skill.service.SkillPublishService; import com.iflytek.skillhub.domain.skill.validation.PackageEntry; @@ -12,21 +14,21 @@ import org.springframework.web.bind.annotation.*; import org.springframework.web.multipart.MultipartFile; import java.io.IOException; -import java.util.ArrayList; import java.util.List; -import java.util.zip.ZipEntry; -import java.util.zip.ZipInputStream; @RestController @RequestMapping("/api/v1/cli") public class CliPublishController extends BaseApiController { private final SkillPublishService skillPublishService; + private final SkillPackageArchiveExtractor skillPackageArchiveExtractor; public CliPublishController(SkillPublishService skillPublishService, + SkillPackageArchiveExtractor skillPackageArchiveExtractor, ApiResponseFactory responseFactory) { super(responseFactory); this.skillPublishService = skillPublishService; + this.skillPackageArchiveExtractor = skillPackageArchiveExtractor; } @PostMapping("/publish") @@ -39,7 +41,12 @@ public class CliPublishController extends BaseApiController { SkillVisibility skillVisibility = SkillVisibility.valueOf(visibility.toUpperCase()); - List entries = extractZipEntries(file); + List entries; + try { + entries = skillPackageArchiveExtractor.extract(file); + } catch (IllegalArgumentException e) { + throw new DomainBadRequestException("error.skill.publish.package.invalid", e.getMessage()); + } SkillPublishService.PublishResult publishResult = skillPublishService.publishFromEntries( namespace, @@ -60,35 +67,4 @@ public class CliPublishController extends BaseApiController { return ok("response.success.published", response); } - - private List extractZipEntries(MultipartFile file) throws IOException { - List entries = new ArrayList<>(); - - try (ZipInputStream zis = new ZipInputStream(file.getInputStream())) { - ZipEntry zipEntry; - while ((zipEntry = zis.getNextEntry()) != null) { - if (!zipEntry.isDirectory()) { - byte[] content = zis.readAllBytes(); - entries.add(new PackageEntry( - zipEntry.getName(), - content, - content.length, - determineContentType(zipEntry.getName()) - )); - } - zis.closeEntry(); - } - } - - return entries; - } - - private String determineContentType(String filename) { - if (filename.endsWith(".py")) return "text/x-python"; - if (filename.endsWith(".json")) return "application/json"; - if (filename.endsWith(".yaml") || filename.endsWith(".yml")) return "application/x-yaml"; - if (filename.endsWith(".txt")) return "text/plain"; - if (filename.endsWith(".md")) return "text/markdown"; - return "application/octet-stream"; - } } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillPublishController.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillPublishController.java index e984e31e..7d96a749 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillPublishController.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillPublishController.java @@ -1,6 +1,8 @@ package com.iflytek.skillhub.controller.portal; import com.iflytek.skillhub.controller.BaseApiController; +import com.iflytek.skillhub.controller.support.SkillPackageArchiveExtractor; +import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException; import com.iflytek.skillhub.domain.skill.SkillVisibility; import com.iflytek.skillhub.domain.skill.service.SkillPublishService; import com.iflytek.skillhub.domain.skill.validation.PackageEntry; @@ -12,21 +14,21 @@ import org.springframework.web.bind.annotation.*; import org.springframework.web.multipart.MultipartFile; import java.io.IOException; -import java.util.ArrayList; import java.util.List; -import java.util.zip.ZipEntry; -import java.util.zip.ZipInputStream; @RestController @RequestMapping("/api/v1/skills") public class SkillPublishController extends BaseApiController { private final SkillPublishService skillPublishService; + private final SkillPackageArchiveExtractor skillPackageArchiveExtractor; public SkillPublishController(SkillPublishService skillPublishService, + SkillPackageArchiveExtractor skillPackageArchiveExtractor, ApiResponseFactory responseFactory) { super(responseFactory); this.skillPublishService = skillPublishService; + this.skillPackageArchiveExtractor = skillPackageArchiveExtractor; } @PostMapping("/{namespace}/publish") @@ -39,7 +41,12 @@ public class SkillPublishController extends BaseApiController { SkillVisibility skillVisibility = SkillVisibility.valueOf(visibility.toUpperCase()); - List entries = extractZipEntries(file); + List entries; + try { + entries = skillPackageArchiveExtractor.extract(file); + } catch (IllegalArgumentException e) { + throw new DomainBadRequestException("error.skill.publish.package.invalid", e.getMessage()); + } SkillPublishService.PublishResult publishResult = skillPublishService.publishFromEntries( namespace, @@ -60,35 +67,4 @@ public class SkillPublishController extends BaseApiController { return ok("response.success.published", response); } - - private List extractZipEntries(MultipartFile file) throws IOException { - List entries = new ArrayList<>(); - - try (ZipInputStream zis = new ZipInputStream(file.getInputStream())) { - ZipEntry zipEntry; - while ((zipEntry = zis.getNextEntry()) != null) { - if (!zipEntry.isDirectory()) { - byte[] content = zis.readAllBytes(); - entries.add(new PackageEntry( - zipEntry.getName(), - content, - content.length, - determineContentType(zipEntry.getName()) - )); - } - zis.closeEntry(); - } - } - - return entries; - } - - private String determineContentType(String filename) { - if (filename.endsWith(".py")) return "text/x-python"; - if (filename.endsWith(".json")) return "application/json"; - if (filename.endsWith(".yaml") || filename.endsWith(".yml")) return "application/x-yaml"; - if (filename.endsWith(".txt")) return "text/plain"; - if (filename.endsWith(".md")) return "text/markdown"; - return "application/octet-stream"; - } } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/support/SkillPackageArchiveExtractor.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/support/SkillPackageArchiveExtractor.java new file mode 100644 index 00000000..b9193b8e --- /dev/null +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/support/SkillPackageArchiveExtractor.java @@ -0,0 +1,92 @@ +package com.iflytek.skillhub.controller.support; + +import com.iflytek.skillhub.domain.skill.validation.PackageEntry; +import com.iflytek.skillhub.domain.skill.validation.SkillPackagePolicy; +import org.springframework.stereotype.Component; +import org.springframework.web.multipart.MultipartFile; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.zip.ZipEntry; +import java.util.zip.ZipInputStream; + +@Component +public class SkillPackageArchiveExtractor { + + public List extract(MultipartFile file) throws IOException { + if (file.getSize() > SkillPackagePolicy.MAX_TOTAL_PACKAGE_SIZE) { + throw new IllegalArgumentException( + "Package too large: " + file.getSize() + " bytes (max: " + + SkillPackagePolicy.MAX_TOTAL_PACKAGE_SIZE + ")" + ); + } + + List entries = new ArrayList<>(); + long totalSize = 0; + + try (ZipInputStream zis = new ZipInputStream(file.getInputStream())) { + ZipEntry zipEntry; + while ((zipEntry = zis.getNextEntry()) != null) { + if (zipEntry.isDirectory()) { + zis.closeEntry(); + continue; + } + + if (entries.size() >= SkillPackagePolicy.MAX_FILE_COUNT) { + throw new IllegalArgumentException( + "Too many files: more than " + SkillPackagePolicy.MAX_FILE_COUNT + ); + } + + String normalizedPath = SkillPackagePolicy.normalizeEntryPath(zipEntry.getName()); + byte[] content = readEntry(zis, normalizedPath); + totalSize += content.length; + if (totalSize > SkillPackagePolicy.MAX_TOTAL_PACKAGE_SIZE) { + throw new IllegalArgumentException( + "Package too large: " + totalSize + " bytes (max: " + + SkillPackagePolicy.MAX_TOTAL_PACKAGE_SIZE + ")" + ); + } + + entries.add(new PackageEntry( + normalizedPath, + content, + content.length, + determineContentType(normalizedPath) + )); + zis.closeEntry(); + } + } + + return entries; + } + + private byte[] readEntry(ZipInputStream zis, String path) throws IOException { + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + byte[] buffer = new byte[8192]; + long totalRead = 0; + int read; + while ((read = zis.read(buffer)) != -1) { + totalRead += read; + if (totalRead > SkillPackagePolicy.MAX_SINGLE_FILE_SIZE) { + throw new IllegalArgumentException( + "File too large: " + path + " (" + totalRead + " bytes, max: " + + SkillPackagePolicy.MAX_SINGLE_FILE_SIZE + ")" + ); + } + outputStream.write(buffer, 0, read); + } + return outputStream.toByteArray(); + } + + private String determineContentType(String filename) { + if (filename.endsWith(".py")) return "text/x-python"; + if (filename.endsWith(".json")) return "application/json"; + if (filename.endsWith(".yaml") || filename.endsWith(".yml")) return "application/x-yaml"; + if (filename.endsWith(".txt")) return "text/plain"; + if (filename.endsWith(".md")) return "text/markdown"; + return "application/octet-stream"; + } +} diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/CliControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/CliControllerTest.java index 13a781b2..58d721ba 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/CliControllerTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/CliControllerTest.java @@ -132,6 +132,25 @@ class CliControllerTest { .andExpect(jsonPath("$.data.errors").isNotEmpty()); } + @Test + void checkShouldReturnInvalidForPathTraversalEntry() throws Exception { + byte[] zipBytes = createZipWithUnsafePath(); + MockMultipartFile file = new MockMultipartFile( + "file", + "skill.zip", + "application/zip", + zipBytes + ); + + mockMvc.perform(multipart("/api/v1/cli/check").file(file)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.code").value(0)) + .andExpect(jsonPath("$.data.valid").value(false)) + .andExpect(jsonPath("$.data.errors[0]").value(org.hamcrest.Matchers.containsString("escapes package root"))) + .andExpect(jsonPath("$.data.fileCount").value(0)) + .andExpect(jsonPath("$.data.totalSize").value(0)); + } + private byte[] createValidSkillZip() throws Exception { ByteArrayOutputStream baos = new ByteArrayOutputStream(); try (ZipOutputStream zos = new ZipOutputStream(baos)) { @@ -191,4 +210,15 @@ class CliControllerTest { } return baos.toByteArray(); } + + private byte[] createZipWithUnsafePath() throws Exception { + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + try (ZipOutputStream zos = new ZipOutputStream(baos)) { + ZipEntry unsafeEntry = new ZipEntry("../secrets.txt"); + zos.putNextEntry(unsafeEntry); + zos.write("hidden".getBytes()); + zos.closeEntry(); + } + return baos.toByteArray(); + } } diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/support/SkillPackageArchiveExtractorTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/support/SkillPackageArchiveExtractorTest.java new file mode 100644 index 00000000..d5464a60 --- /dev/null +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/support/SkillPackageArchiveExtractorTest.java @@ -0,0 +1,61 @@ +package com.iflytek.skillhub.controller.support; + +import org.junit.jupiter.api.Test; +import org.springframework.mock.web.MockMultipartFile; + +import java.io.ByteArrayOutputStream; +import java.nio.charset.StandardCharsets; +import java.util.zip.ZipEntry; +import java.util.zip.ZipOutputStream; + +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class SkillPackageArchiveExtractorTest { + + private final SkillPackageArchiveExtractor extractor = new SkillPackageArchiveExtractor(); + + @Test + void shouldRejectPathTraversalEntry() throws Exception { + MockMultipartFile file = new MockMultipartFile( + "file", + "skill.zip", + "application/zip", + createZip("../secrets.txt", "hidden") + ); + + IllegalArgumentException error = assertThrows(IllegalArgumentException.class, () -> extractor.extract(file)); + + assertTrue(error.getMessage().contains("escapes package root")); + } + + @Test + void shouldRejectOversizedZipEntry() throws Exception { + byte[] content = new byte[1024 * 1024 + 1]; + MockMultipartFile file = new MockMultipartFile( + "file", + "skill.zip", + "application/zip", + createZip("large.txt", content) + ); + + IllegalArgumentException error = assertThrows(IllegalArgumentException.class, () -> extractor.extract(file)); + + assertTrue(error.getMessage().contains("File too large: large.txt")); + } + + private byte[] createZip(String entryName, String content) throws Exception { + return createZip(entryName, content.getBytes(StandardCharsets.UTF_8)); + } + + private byte[] createZip(String entryName, byte[] content) throws Exception { + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + try (ZipOutputStream zos = new ZipOutputStream(baos)) { + ZipEntry entry = new ZipEntry(entryName); + zos.putNextEntry(entry); + zos.write(content); + zos.closeEntry(); + } + return baos.toByteArray(); + } +} diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/SkillPackagePolicy.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/SkillPackagePolicy.java new file mode 100644 index 00000000..3c0e5498 --- /dev/null +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/SkillPackagePolicy.java @@ -0,0 +1,56 @@ +package com.iflytek.skillhub.domain.skill.validation; + +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Set; + +public final class SkillPackagePolicy { + + public static final int MAX_FILE_COUNT = 100; + public static final long MAX_SINGLE_FILE_SIZE = 1024 * 1024; // 1MB + public static final long MAX_TOTAL_PACKAGE_SIZE = 10 * 1024 * 1024; // 10MB + public static final String SKILL_MD_PATH = "SKILL.md"; + public static final Set ALLOWED_EXTENSIONS = Set.of( + ".md", ".txt", ".json", ".yaml", ".yml", + ".js", ".ts", ".py", ".sh", + ".png", ".jpg", ".svg" + ); + + private SkillPackagePolicy() { + } + + public static String normalizeEntryPath(String rawPath) { + if (rawPath == null) { + throw new IllegalArgumentException("Package entry path is missing"); + } + + String sanitized = rawPath.replace('\\', '/').trim(); + if (sanitized.isEmpty()) { + throw new IllegalArgumentException("Package entry path is empty"); + } + if (sanitized.startsWith("/") || sanitized.startsWith("\\")) { + throw new IllegalArgumentException("Package entry path must be relative: " + rawPath); + } + if (sanitized.contains(":")) { + throw new IllegalArgumentException("Package entry path contains an invalid drive or scheme prefix: " + rawPath); + } + + Path normalized = Paths.get(sanitized).normalize(); + String canonical = normalized.toString().replace('\\', '/'); + if (normalized.isAbsolute() || canonical.isBlank()) { + throw new IllegalArgumentException("Package entry path is invalid: " + rawPath); + } + if (canonical.equals(".") || canonical.equals("..") || canonical.startsWith("../")) { + throw new IllegalArgumentException("Package entry path escapes package root: " + rawPath); + } + if (!sanitized.equals(canonical)) { + throw new IllegalArgumentException("Package entry path must be normalized: " + rawPath); + } + + return canonical; + } + + public static boolean hasAllowedExtension(String path) { + return ALLOWED_EXTENSIONS.stream().anyMatch(path::endsWith); + } +} diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/SkillPackageValidator.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/SkillPackageValidator.java index 57977dc6..950417fc 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/SkillPackageValidator.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/SkillPackageValidator.java @@ -4,21 +4,12 @@ import com.iflytek.skillhub.domain.shared.exception.LocalizedDomainException; import com.iflytek.skillhub.domain.skill.metadata.SkillMetadataParser; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Set; public class SkillPackageValidator { - private static final int MAX_FILE_COUNT = 100; - private static final long MAX_SINGLE_FILE_SIZE = 1024 * 1024; // 1MB - private static final long MAX_TOTAL_PACKAGE_SIZE = 10 * 1024 * 1024; // 10MB - private static final String SKILL_MD_PATH = "SKILL.md"; - private static final Set ALLOWED_EXTENSIONS = Set.of( - ".md", ".txt", ".json", ".yaml", ".yml", - ".js", ".ts", ".py", ".sh", - ".png", ".jpg", ".svg" - ); - private final SkillMetadataParser metadataParser; public SkillPackageValidator(SkillMetadataParser metadataParser) { @@ -27,13 +18,32 @@ public class SkillPackageValidator { public ValidationResult validate(List entries) { List errors = new ArrayList<>(); + Set normalizedPaths = new HashSet<>(); + PackageEntry skillMd = null; + + for (PackageEntry entry : entries) { + String normalizedPath; + try { + normalizedPath = SkillPackagePolicy.normalizeEntryPath(entry.path()); + } catch (IllegalArgumentException e) { + errors.add(e.getMessage()); + continue; + } + + if (!normalizedPaths.add(normalizedPath)) { + errors.add("Duplicate package entry path: " + normalizedPath); + } + + if (!SkillPackagePolicy.hasAllowedExtension(normalizedPath)) { + errors.add("Disallowed file extension: " + normalizedPath); + } + + if (SkillPackagePolicy.SKILL_MD_PATH.equals(normalizedPath) && skillMd == null) { + skillMd = entry; + } + } // 1. Check SKILL.md exists at root - PackageEntry skillMd = entries.stream() - .filter(e -> e.path().equals(SKILL_MD_PATH)) - .findFirst() - .orElse(null); - if (skillMd == null) { errors.add("Missing required file: SKILL.md at root"); return ValidationResult.fail(errors); @@ -51,31 +61,21 @@ public class SkillPackageValidator { } // 3. Check file count - if (entries.size() > MAX_FILE_COUNT) { - errors.add("Too many files: " + entries.size() + " (max: " + MAX_FILE_COUNT + ")"); + if (entries.size() > SkillPackagePolicy.MAX_FILE_COUNT) { + errors.add("Too many files: " + entries.size() + " (max: " + SkillPackagePolicy.MAX_FILE_COUNT + ")"); } - // 4. Check file extensions + // 4. Check single file size for (PackageEntry entry : entries) { - String path = entry.path(); - boolean hasAllowedExtension = ALLOWED_EXTENSIONS.stream() - .anyMatch(path::endsWith); - if (!hasAllowedExtension) { - errors.add("Disallowed file extension: " + path); + if (entry.size() > SkillPackagePolicy.MAX_SINGLE_FILE_SIZE) { + errors.add("File too large: " + entry.path() + " (" + entry.size() + " bytes, max: " + SkillPackagePolicy.MAX_SINGLE_FILE_SIZE + ")"); } } - // 5. Check single file size - for (PackageEntry entry : entries) { - if (entry.size() > MAX_SINGLE_FILE_SIZE) { - errors.add("File too large: " + entry.path() + " (" + entry.size() + " bytes, max: " + MAX_SINGLE_FILE_SIZE + ")"); - } - } - - // 6. Check total package size + // 5. Check total package size long totalSize = entries.stream().mapToLong(PackageEntry::size).sum(); - if (totalSize > MAX_TOTAL_PACKAGE_SIZE) { - errors.add("Package too large: " + totalSize + " bytes (max: " + MAX_TOTAL_PACKAGE_SIZE + ")"); + if (totalSize > SkillPackagePolicy.MAX_TOTAL_PACKAGE_SIZE) { + errors.add("Package too large: " + totalSize + " bytes (max: " + SkillPackagePolicy.MAX_TOTAL_PACKAGE_SIZE + ")"); } return errors.isEmpty() ? ValidationResult.pass() : ValidationResult.fail(errors); diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/validation/SkillPackageValidatorTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/validation/SkillPackageValidatorTest.java index bd0a6542..c36f4f42 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/validation/SkillPackageValidatorTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/validation/SkillPackageValidatorTest.java @@ -176,4 +176,49 @@ class SkillPackageValidatorTest { assertFalse(result.passed()); assertTrue(result.errors().stream().anyMatch(e -> e.contains("Package too large"))); } + + @Test + void testPathTraversalEntryRejected() { + String skillMdContent = """ + --- + name: test-skill + description: A test skill + version: 1.0.0 + --- + Body + """; + + List entries = List.of( + new PackageEntry("SKILL.md", skillMdContent.getBytes(), skillMdContent.length(), "text/markdown"), + new PackageEntry("../secrets.txt", "hidden".getBytes(), 6, "text/plain") + ); + + ValidationResult result = validator.validate(entries); + + assertFalse(result.passed()); + assertTrue(result.errors().stream().anyMatch(e -> e.contains("escapes package root"))); + } + + @Test + void testDuplicateNormalizedPathRejected() { + String skillMdContent = """ + --- + name: test-skill + description: A test skill + version: 1.0.0 + --- + Body + """; + + List entries = List.of( + new PackageEntry("SKILL.md", skillMdContent.getBytes(), skillMdContent.length(), "text/markdown"), + new PackageEntry("docs\\guide.md", "first".getBytes(), 5, "text/markdown"), + new PackageEntry("docs/guide.md", "second".getBytes(), 6, "text/markdown") + ); + + ValidationResult result = validator.validate(entries); + + assertFalse(result.passed()); + assertTrue(result.errors().stream().anyMatch(e -> e.contains("Duplicate package entry path: docs/guide.md"))); + } } diff --git a/server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/LocalFileStorageService.java b/server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/LocalFileStorageService.java index 5fd9ee8d..26889aa0 100644 --- a/server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/LocalFileStorageService.java +++ b/server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/LocalFileStorageService.java @@ -15,7 +15,7 @@ public class LocalFileStorageService implements ObjectStorageService { private final Path basePath; public LocalFileStorageService(StorageProperties properties) { - this.basePath = Paths.get(properties.getLocal().getBasePath()); + this.basePath = Paths.get(properties.getLocal().getBasePath()).toAbsolutePath().normalize(); } @Override @@ -58,5 +58,11 @@ public class LocalFileStorageService implements ObjectStorageService { } catch (IOException e) { throw new UncheckedIOException("Failed to get metadata: " + key, e); } } - private Path resolve(String key) { return basePath.resolve(key); } + private Path resolve(String key) { + Path resolved = basePath.resolve(key).normalize(); + if (!resolved.startsWith(basePath)) { + throw new IllegalArgumentException("Invalid storage key: " + key); + } + return resolved; + } } diff --git a/server/skillhub-storage/src/test/java/com/iflytek/skillhub/storage/LocalFileStorageServiceTest.java b/server/skillhub-storage/src/test/java/com/iflytek/skillhub/storage/LocalFileStorageServiceTest.java index dfaf2b72..d2e98611 100644 --- a/server/skillhub-storage/src/test/java/com/iflytek/skillhub/storage/LocalFileStorageServiceTest.java +++ b/server/skillhub-storage/src/test/java/com/iflytek/skillhub/storage/LocalFileStorageServiceTest.java @@ -61,4 +61,20 @@ class LocalFileStorageServiceTest { assertEquals(content.length, metadata.size()); assertNotNull(metadata.lastModified()); } + + @Test void shouldRejectPathTraversalKeys() { + byte[] content = "data".getBytes(StandardCharsets.UTF_8); + + IllegalArgumentException putError = assertThrows( + IllegalArgumentException.class, + () -> storageService.putObject("../escape.txt", new ByteArrayInputStream(content), content.length, "text/plain") + ); + assertEquals("Invalid storage key: ../escape.txt", putError.getMessage()); + + IllegalArgumentException getError = assertThrows( + IllegalArgumentException.class, + () -> storageService.getObject("..\\escape.txt") + ); + assertEquals("Invalid storage key: ..\\escape.txt", getError.getMessage()); + } }