mirror of
https://github.com/iflytek/skillhub.git
synced 2026-09-12 23:01:05 +00:00
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
This commit is contained in:
parent
7bc152a744
commit
ddbf92435f
11 changed files with 380 additions and 141 deletions
|
|
@ -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<SkillCheckResponse> check(@RequestParam("file") MultipartFile file) throws IOException {
|
||||
List<PackageEntry> entries = extractZipEntries(file);
|
||||
List<PackageEntry> 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<PackageEntry> extractZipEntries(MultipartFile file) throws IOException {
|
||||
List<PackageEntry> 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";
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<PackageEntry> entries = extractZipEntries(file);
|
||||
List<PackageEntry> 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<PackageEntry> extractZipEntries(MultipartFile file) throws IOException {
|
||||
List<PackageEntry> 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";
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<PackageEntry> entries = extractZipEntries(file);
|
||||
List<PackageEntry> 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<PackageEntry> extractZipEntries(MultipartFile file) throws IOException {
|
||||
List<PackageEntry> 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";
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<PackageEntry> 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<PackageEntry> 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";
|
||||
}
|
||||
}
|
||||
|
|
@ -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();
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
}
|
||||
}
|
||||
|
|
@ -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<String> 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);
|
||||
}
|
||||
}
|
||||
|
|
@ -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<String> 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<PackageEntry> entries) {
|
||||
List<String> errors = new ArrayList<>();
|
||||
Set<String> 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);
|
||||
|
|
|
|||
|
|
@ -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<PackageEntry> 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<PackageEntry> 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")));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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());
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue