feat(publish): skill upload limits, nested SKILL.md, session fix (#364)

* feat(publish): increase max file count from 100 to 500

Configurable via SKILLHUB_PUBLISH_MAX_FILE_COUNT env var.

* feat(publish): support SKILL.md in subdirectory with warning for ignored files

When SKILL.md is found in a single subdirectory (e.g. my-skill/SKILL.md),
promote that directory's contents to root and discard files outside it.
Discarded files are reported as warnings through the existing confirm flow.

* feat(publish): pass extraction warnings through confirm flow

When files are ignored during SKILL.md subdirectory promotion,
warnings are surfaced to the user via the existing precheck confirm dialog.

* fix(security): add invalidSessionStrategy to return 401 on expired session

Handles the case where Spring Security detects an invalid session cookie,
returning a clean 401 JSON response instead of triggering cascading exceptions.
Closes #360 (part 1/2)

* fix(security): handle session invalidation IllegalStateException as 401

Catches IllegalStateException with "Session was invalidated" message and
returns 401 instead of letting it fall through to the generic 500 handler.
Non-session IllegalStateExceptions are re-thrown to the catch-all handler.
Closes #360 (part 2/2)

* feat(publish): filter macOS metadata and add integration tests

Skip __MACOSX/, .DS_Store, and ._ resource fork entries during zip
extraction. Add integration tests for nested SKILL.md warning flow,
session invalidation 401 response, and macOS metadata filtering.

* test(publish): add real-world macOS zip and edge case integration tests

Covers: macOS zip with nested SKILL.md + __MACOSX + .DS_Store + stray files,
simple macOS single-folder case, and missing SKILL.md fallback behavior.
This commit is contained in:
XiaoSeS 2026-04-30 11:20:51 +08:00 • committed by GitHub
parent f70c1c6d99
commit a3c07725e5
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
10 changed files with 342 additions and 5 deletions

View file

@ -11,7 +11,7 @@ import java.util.Set;
@ConfigurationProperties(prefix = "skillhub.publish")
public class SkillPublishProperties {
private int maxFileCount = 100;
private int maxFileCount = 500;
private long maxSingleFileSize = 10 * 1024 * 1024; // 10MB
private long maxPackageSize = 100 * 1024 * 1024;
private Set<String> allowedFileExtensions = new LinkedHashSet<>(SkillPackagePolicy.ALLOWED_EXTENSIONS);

View file

@ -59,12 +59,22 @@ public class SkillPublishController extends BaseApiController {
SkillVisibility skillVisibility = SkillVisibility.valueOf(visibility.toUpperCase());
List<PackageEntry> entries;
List<String> extractionWarnings;
try {
entries = skillPackageArchiveExtractor.extract(file);
SkillPackageArchiveExtractor.ExtractionResult extractionResult =
skillPackageArchiveExtractor.extractWithWarnings(file);
entries = extractionResult.entries();
extractionWarnings = extractionResult.warnings();
} catch (IllegalArgumentException e) {
throw new DomainBadRequestException("error.skill.publish.package.invalid", e.getMessage());
}
if (!confirmWarnings && !extractionWarnings.isEmpty()) {
throw new DomainBadRequestException(
"error.skill.publish.precheck.confirmRequired",
String.join("\n", extractionWarnings));
}
SkillPublishService.PublishResult publishResult = skillPublishService.publishFromEntries(
namespace,
entries,

View file

@ -18,6 +18,8 @@ import java.util.zip.ZipInputStream;
@Component
public class SkillPackageArchiveExtractor {
public record ExtractionResult(List<PackageEntry> entries, List<String> warnings) {}
private final long maxTotalPackageSize;
private final long maxSingleFileSize;
private final int maxFileCount;
@ -47,6 +49,11 @@ public class SkillPackageArchiveExtractor {
continue;
}
if (isOsMetadataEntry(zipEntry.getName())) {
zis.closeEntry();
continue;
}
if (entries.size() >= maxFileCount) {
throw new IllegalArgumentException(
"Too many files: more than " + maxFileCount
@ -76,6 +83,11 @@ public class SkillPackageArchiveExtractor {
return stripSingleRootDirectory(entries);
}
public ExtractionResult extractWithWarnings(MultipartFile file) throws IOException {
List<PackageEntry> entries = extract(file);
return promoteSingleSkillMdDirectory(entries);
}
/**
* If all file paths share a single root directory prefix (e.g., "my-skill/xxx"),
* strip that prefix. Otherwise return entries unchanged.
@ -107,6 +119,58 @@ public class SkillPackageArchiveExtractor {
.toList();
}
static ExtractionResult promoteSingleSkillMdDirectory(List<PackageEntry> entries) {
boolean hasRootSkillMd = entries.stream()
.anyMatch(e -> SkillPackagePolicy.SKILL_MD_PATH.equals(e.path()));
if (hasRootSkillMd) {
return new ExtractionResult(entries, List.of());
}
Set<String> skillMdDirs = new HashSet<>();
for (PackageEntry entry : entries) {
int slashIndex = entry.path().indexOf('/');
if (slashIndex > 0) {
String relativePath = entry.path().substring(slashIndex + 1);
if (SkillPackagePolicy.SKILL_MD_PATH.equals(relativePath)) {
skillMdDirs.add(entry.path().substring(0, slashIndex));
}
}
}
if (skillMdDirs.isEmpty()) {
return new ExtractionResult(entries, List.of());
}
if (skillMdDirs.size() > 1) {
throw new IllegalArgumentException(
"Ambiguous package: SKILL.md found in multiple directories: " + skillMdDirs);
}
String prefix = skillMdDirs.iterator().next() + "/";
List<PackageEntry> promoted = new ArrayList<>();
List<String> warnings = new ArrayList<>();
for (PackageEntry entry : entries) {
if (entry.path().startsWith(prefix)) {
promoted.add(new PackageEntry(
entry.path().substring(prefix.length()),
entry.content(),
entry.size(),
entry.contentType()));
} else {
warnings.add("Ignored file outside skill directory: " + entry.path());
}
}
return new ExtractionResult(promoted, warnings);
}
private static boolean isOsMetadataEntry(String name) {
String normalized = name.replace('\\', '/');
if (normalized.startsWith("__MACOSX/") || normalized.equals("__MACOSX")) return true;
String fileName = normalized.contains("/") ? normalized.substring(normalized.lastIndexOf('/') + 1) : normalized;
return fileName.equals(".DS_Store") || fileName.startsWith("._");
}
private byte[] readEntry(ZipInputStream zis, String path) throws IOException {
ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
byte[] buffer = new byte[8192];

View file

@ -95,6 +95,17 @@ public class GlobalExceptionHandler {
apiResponseFactory.error(403, "error.forbidden"));
}
@ExceptionHandler(IllegalStateException.class)
public ResponseEntity<ApiResponse<Void>> handleSessionInvalidated(
IllegalStateException ex, HttpServletRequest request) {
if (ex.getMessage() != null && ex.getMessage().contains("Session was invalidated")) {
logHandledException(HttpStatus.UNAUTHORIZED, "error.session.expired", request);
return ResponseEntity.status(HttpStatus.UNAUTHORIZED)
.body(apiResponseFactory.error(401, "error.session.expired"));
}
throw ex;
}
@ExceptionHandler(StorageAccessException.class)
public ResponseEntity<ApiResponse<Void>> handleStorageAccess(StorageAccessException ex, HttpServletRequest request) {
metrics.incrementStorageAccessFailure(ex.getOperation());

View file

@ -3,6 +3,7 @@ package com.iflytek.skillhub.controller.portal;
import static org.mockito.ArgumentMatchers.anyList;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import com.iflytek.skillhub.domain.skill.validation.PackageEntry;
@ -161,6 +162,64 @@ class SkillPublishControllerTest {
.andExpect(jsonPath("$.code").value(0));
}
@Test
void publish_nestedSkillMdReturnsWarningForIgnoredFiles() throws Exception {
PlatformPrincipal principal = new PlatformPrincipal(
"usr_1", "publisher", "publisher@example.com", "", "local", Set.of("SUPER_ADMIN"));
var auth = new UsernamePasswordAuthenticationToken(
principal, null, List.of(new SimpleGrantedAuthority("ROLE_SUPER_ADMIN")));
MockMultipartFile file = new MockMultipartFile(
"file", "skill.zip", "application/zip",
buildZipWithNestedSkillMd());
mockMvc.perform(multipart("/api/v1/skills/global/publish")
.file(file)
.param("visibility", "PUBLIC")
.with(authentication(auth))
.with(csrf()))
.andExpect(status().isBadRequest())
.andExpect(jsonPath("$.msg").value(
org.hamcrest.Matchers.containsString("stray.txt")));
verify(skillPublishService, never()).publishFromEntries(
eq("global"), anyList(), eq("usr_1"),
eq(SkillVisibility.PUBLIC), eq(Set.of("SUPER_ADMIN")), eq(false));
}
@Test
void publish_nestedSkillMdSucceedsWithConfirmWarnings() throws Exception {
SkillVersion version = new SkillVersion(12L, "1.0.0", "usr_1");
version.setStatus(SkillVersionStatus.PENDING_REVIEW);
version.setFileCount(1);
version.setTotalSize(128L);
ReflectionTestUtils.setField(version, "id", 34L);
given(skillPublishService.publishFromEntries(
eq("global"), ArgumentMatchers.<List<PackageEntry>>any(),
eq("usr_1"), eq(SkillVisibility.PUBLIC),
eq(Set.of("SUPER_ADMIN")), eq(true)))
.willReturn(new SkillPublishService.PublishResult(12L, "demo-skill", version));
PlatformPrincipal principal = new PlatformPrincipal(
"usr_1", "publisher", "publisher@example.com", "", "local", Set.of("SUPER_ADMIN"));
var auth = new UsernamePasswordAuthenticationToken(
principal, null, List.of(new SimpleGrantedAuthority("ROLE_SUPER_ADMIN")));
MockMultipartFile file = new MockMultipartFile(
"file", "skill.zip", "application/zip",
buildZipWithNestedSkillMd());
mockMvc.perform(multipart("/api/v1/skills/global/publish")
.file(file)
.param("visibility", "PUBLIC")
.param("confirmWarnings", "true")
.with(authentication(auth))
.with(csrf()))
.andExpect(status().isOk())
.andExpect(jsonPath("$.code").value(0));
}
private byte[] buildZipBytes() throws Exception {
try (ByteArrayOutputStream output = new ByteArrayOutputStream();
ZipOutputStream zip = new ZipOutputStream(output, StandardCharsets.UTF_8)) {
@ -176,4 +235,23 @@ class SkillPublishControllerTest {
return output.toByteArray();
}
}
private byte[] buildZipWithNestedSkillMd() throws Exception {
try (ByteArrayOutputStream output = new ByteArrayOutputStream();
ZipOutputStream zip = new ZipOutputStream(output, StandardCharsets.UTF_8)) {
zip.putNextEntry(new ZipEntry("my-skill/SKILL.md"));
zip.write("""
---
name: Demo Skill
version: 1.0.0
---
""".getBytes(StandardCharsets.UTF_8));
zip.closeEntry();
zip.putNextEntry(new ZipEntry("stray.txt"));
zip.write("ignored file".getBytes(StandardCharsets.UTF_8));
zip.closeEntry();
zip.finish();
return output.toByteArray();
}
}
}

View file

@ -14,6 +14,7 @@ import java.util.zip.ZipEntry;
import java.util.zip.ZipOutputStream;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
@ -126,6 +127,145 @@ class SkillPackageArchiveExtractorTest {
assertTrue(entries.stream().anyMatch(e -> e.path().equals("dir-b/other.md")));
}
@Test
void promotesSkillMdFromSubdirectoryAndDiscardsRootFiles() throws Exception {
byte[] zipBytes = createZip(Map.of(
"my-skill/SKILL.md", "---\nname: test\n---\n".getBytes(),
"my-skill/README.md", "# readme".getBytes(),
"other.txt", "stray file".getBytes()
));
MockMultipartFile file = new MockMultipartFile("file", "test.zip", "application/zip", zipBytes);
SkillPackageArchiveExtractor.ExtractionResult result = extractor.extractWithWarnings(file);
assertEquals(2, result.entries().size());
assertTrue(result.entries().stream().anyMatch(e -> e.path().equals("SKILL.md")));
assertTrue(result.entries().stream().anyMatch(e -> e.path().equals("README.md")));
assertTrue(result.warnings().stream().anyMatch(w -> w.contains("other.txt")));
}
@Test
void rejectsAmbiguousMultipleSkillMdInSubdirectories() throws Exception {
byte[] zipBytes = createZip(Map.of(
"dir1/SKILL.md", "---\nname: a\n---\n".getBytes(),
"dir2/SKILL.md", "---\nname: b\n---\n".getBytes()
));
MockMultipartFile file = new MockMultipartFile("file", "test.zip", "application/zip", zipBytes);
IllegalArgumentException error = assertThrows(IllegalArgumentException.class,
() -> extractor.extractWithWarnings(file));
assertTrue(error.getMessage().contains("Ambiguous"));
}
@Test
void noPromotionWhenSkillMdAtRoot() throws Exception {
byte[] zipBytes = createZip(Map.of(
"SKILL.md", "---\nname: test\n---\n".getBytes(),
"sub/file.txt", "content".getBytes()
));
MockMultipartFile file = new MockMultipartFile("file", "test.zip", "application/zip", zipBytes);
SkillPackageArchiveExtractor.ExtractionResult result = extractor.extractWithWarnings(file);
assertEquals(2, result.entries().size());
assertTrue(result.warnings().isEmpty());
}
@Test
void promotesSubdirectoryPreservingNestedPaths() throws Exception {
byte[] zipBytes = createZip(Map.of(
"my-skill/SKILL.md", "---\nname: test\n---\n".getBytes(),
"my-skill/sub/deep.md", "nested".getBytes(),
"stray.txt", "ignored".getBytes()
));
MockMultipartFile file = new MockMultipartFile("file", "test.zip", "application/zip", zipBytes);
SkillPackageArchiveExtractor.ExtractionResult result = extractor.extractWithWarnings(file);
assertEquals(2, result.entries().size());
assertTrue(result.entries().stream().anyMatch(e -> e.path().equals("SKILL.md")));
assertTrue(result.entries().stream().anyMatch(e -> e.path().equals("sub/deep.md")));
assertTrue(result.warnings().stream().anyMatch(w -> w.contains("stray.txt")));
}
@Test
void filtersMacOsMetadataEntries() throws Exception {
byte[] zipBytes = createZip(Map.of(
"my-skill/SKILL.md", "---\nname: test\n---\n".getBytes(),
"my-skill/README.md", "# readme".getBytes(),
"__MACOSX/my-skill/._SKILL.md", "resource fork".getBytes(),
"my-skill/.DS_Store", "binary".getBytes()
));
MockMultipartFile file = new MockMultipartFile("file", "test.zip", "application/zip", zipBytes);
List<PackageEntry> entries = extractor.extract(file);
assertEquals(2, entries.size());
assertTrue(entries.stream().noneMatch(e -> e.path().contains("MACOSX")));
assertTrue(entries.stream().noneMatch(e -> e.path().contains(".DS_Store")));
}
@Test
void realWorldMacZipWithNestedSkillMd() throws Exception {
// Simulates: ui-ux-pro-max/uiux/SKILL.md + __MACOSX + .DS_Store + stray csv
byte[] zipBytes = createZip(Map.of(
"ui-ux-pro-max/uiux/SKILL.md", "---\nname: uiux\nversion: 1.0.0\n---\nBody".getBytes(),
"ui-ux-pro-max/uiux/scripts/core.py", "# code".getBytes(),
"ui-ux-pro-max/uiux/data/styles.csv", "col1,col2".getBytes(),
"ui-ux-pro-max/stray.csv", "stray data".getBytes(),
"__MACOSX/ui-ux-pro-max/._stray.csv", "resource fork".getBytes(),
"ui-ux-pro-max/.DS_Store", "binary".getBytes(),
"__MACOSX/._ui-ux-pro-max", "resource fork".getBytes()
));
MockMultipartFile file = new MockMultipartFile("file", "test.zip", "application/zip", zipBytes);
SkillPackageArchiveExtractor.ExtractionResult result = extractor.extractWithWarnings(file);
// macOS files filtered, root stripped to ui-ux-pro-max/, then uiux/ promoted
assertTrue(result.entries().stream().anyMatch(e -> e.path().equals("SKILL.md")));
assertTrue(result.entries().stream().anyMatch(e -> e.path().equals("scripts/core.py")));
assertTrue(result.entries().stream().anyMatch(e -> e.path().equals("data/styles.csv")));
assertTrue(result.entries().stream().noneMatch(e -> e.path().contains("MACOSX")));
assertTrue(result.entries().stream().noneMatch(e -> e.path().contains(".DS_Store")));
// stray.csv outside uiux/ should be in warnings
assertFalse(result.warnings().isEmpty());
assertTrue(result.warnings().stream().anyMatch(w -> w.contains("stray.csv")));
}
@Test
void macZipWithSingleFolderAndSkillMdAtRoot() throws Exception {
// All files under one folder, SKILL.md at folder root — simplest macOS case
byte[] zipBytes = createZip(Map.of(
"my-skill/SKILL.md", "---\nname: test\nversion: 1.0.0\n---\nBody".getBytes(),
"my-skill/README.md", "# readme".getBytes(),
"__MACOSX/my-skill/._SKILL.md", "fork".getBytes(),
"__MACOSX/._my-skill", "fork".getBytes()
));
MockMultipartFile file = new MockMultipartFile("file", "test.zip", "application/zip", zipBytes);
SkillPackageArchiveExtractor.ExtractionResult result = extractor.extractWithWarnings(file);
assertEquals(2, result.entries().size());
assertTrue(result.entries().stream().anyMatch(e -> e.path().equals("SKILL.md")));
assertTrue(result.entries().stream().anyMatch(e -> e.path().equals("README.md")));
assertTrue(result.warnings().isEmpty());
}
@Test
void extractWithWarningsNoSkillMdAnywhere() throws Exception {
byte[] zipBytes = createZip(Map.of(
"README.md", "# no skill".getBytes(),
"config.json", "{}".getBytes()
));
MockMultipartFile file = new MockMultipartFile("file", "test.zip", "application/zip", zipBytes);
SkillPackageArchiveExtractor.ExtractionResult result = extractor.extractWithWarnings(file);
// No SKILL.md found — entries returned as-is, validator will catch the error
assertEquals(2, result.entries().size());
assertTrue(result.warnings().isEmpty());
}
private byte[] createZip(String entryName, String content) throws Exception {
return createZip(entryName, content.getBytes(StandardCharsets.UTF_8));
}

View file

@ -1,6 +1,7 @@
package com.iflytek.skillhub.exception;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.Mockito.when;
import com.iflytek.skillhub.dto.ApiResponse;
@ -70,4 +71,25 @@ class GlobalExceptionHandlerTest {
assertThat(body.code()).isEqualTo(408);
assertThat(body.msg()).isEqualTo("Request timed out");
}
@Test
void handleSessionInvalidated_shouldReturn401ForSessionException() {
when(request.getMethod()).thenReturn("GET");
when(sensitiveLogSanitizer.sanitizeRequestTarget(request)).thenReturn("/api/v1/skills");
IllegalStateException ex = new IllegalStateException("Session was invalidated");
ResponseEntity<ApiResponse<Void>> response = handler.handleSessionInvalidated(ex, request);
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.UNAUTHORIZED);
assertThat(response.getBody()).isNotNull();
assertThat(response.getBody().code()).isEqualTo(401);
}
@Test
void handleSessionInvalidated_shouldRethrowNonSessionException() {
IllegalStateException ex = new IllegalStateException("Some other error");
assertThatThrownBy(() -> handler.handleSessionInvalidated(ex, request))
.isSameAs(ex);
}
}

View file

@ -9,10 +9,12 @@ import com.iflytek.skillhub.auth.mock.MockAuthFilter;
import com.iflytek.skillhub.auth.policy.RouteSecurityPolicyRegistry;
import com.iflytek.skillhub.auth.token.ApiTokenAuthenticationFilter;
import com.iflytek.skillhub.auth.token.ApiTokenScopeFilter;
import jakarta.servlet.http.HttpServletResponse;
import org.springframework.beans.factory.ObjectProvider;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.http.HttpStatus;
import org.springframework.http.MediaType;
import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder;
import org.springframework.security.crypto.password.PasswordEncoder;
import org.springframework.security.web.AuthenticationEntryPoint;
@ -133,6 +135,11 @@ public class SecurityConfig {
)
.sessionManagement(session -> session
.sessionCreationPolicy(SessionCreationPolicy.IF_REQUIRED)
.invalidSessionStrategy((request, response) -> {
response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
response.setContentType(MediaType.APPLICATION_JSON_VALUE);
response.getWriter().write("{\"code\":401,\"msg\":\"Session expired\"}");
})
)
.exceptionHandling(exceptions -> exceptions
.accessDeniedHandler(apiAccessDeniedHandler)

View file

@ -15,7 +15,7 @@ import java.util.Set;
*/
public final class SkillPackagePolicy {
public static final int MAX_FILE_COUNT = 100;
public static final int MAX_FILE_COUNT = 500;
public static final long MAX_SINGLE_FILE_SIZE = 10 * 1024 * 1024; // 10MB
public static final long MAX_TOTAL_PACKAGE_SIZE = 100 * 1024 * 1024; // 100MB
public static final String SKILL_MD_PATH = "SKILL.md";

View file

@ -101,14 +101,19 @@ class SkillPackageValidatorTest {
Body
""";
// Use a custom validator with a small file count limit to test the logic
SkillPackageValidator smallValidator = new SkillPackageValidator(
new SkillMetadataParser(), 10, SkillPackagePolicy.MAX_SINGLE_FILE_SIZE,
SkillPackagePolicy.MAX_TOTAL_PACKAGE_SIZE, SkillPackagePolicy.ALLOWED_EXTENSIONS);
List<PackageEntry> entries = new ArrayList<>();
entries.add(new PackageEntry("SKILL.md", skillMdContent.getBytes(), skillMdContent.length(), "text/markdown"));
for (int i = 0; i < 100; i++) {
for (int i = 0; i < 11; i++) {
entries.add(new PackageEntry("file" + i + ".txt", "content".getBytes(), 7, "text/plain"));
}
ValidationResult result = validator.validate(entries);
ValidationResult result = smallValidator.validate(entries);
assertFalse(result.passed());
assertTrue(result.errors().stream().anyMatch(e -> e.contains("Too many files")));