diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/compat/ClawHubRegistrySecurityConfig.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/compat/ClawHubRegistrySecurityConfig.java index e0df66d5..4f2c5969 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/compat/ClawHubRegistrySecurityConfig.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/compat/ClawHubRegistrySecurityConfig.java @@ -4,6 +4,7 @@ import com.iflytek.skillhub.auth.token.ApiTokenAuthenticationFilter; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.core.annotation.Order; +import org.springframework.http.HttpMethod; import org.springframework.security.config.annotation.web.builders.HttpSecurity; import org.springframework.security.config.http.SessionCreationPolicy; import org.springframework.security.web.SecurityFilterChain; @@ -24,12 +25,11 @@ public class ClawHubRegistrySecurityConfig { http .securityMatcher( new OrRequestMatcher( - new AntPathRequestMatcher("/api/v1/labels"), - new AntPathRequestMatcher("/api/web/labels") + new AntPathRequestMatcher("/api/v1/labels", HttpMethod.GET.name()), + new AntPathRequestMatcher("/api/web/labels", HttpMethod.GET.name()) ) ) .authorizeHttpRequests(auth -> auth.anyRequest().permitAll()) - .csrf(csrf -> csrf.disable()) .requestCache(cache -> cache.disable()) .securityContext(context -> context.disable()) .sessionManagement(session -> session.sessionCreationPolicy(SessionCreationPolicy.STATELESS)); diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/LabelControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/LabelControllerTest.java index 7e93b54c..7fc33e42 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/LabelControllerTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/LabelControllerTest.java @@ -14,6 +14,7 @@ import org.springframework.test.web.servlet.MockMvc; import static org.mockito.Mockito.when; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; @@ -60,4 +61,10 @@ class LabelControllerTest { .andExpect(jsonPath("$.data[0].slug").value("code-generation")) .andExpect(jsonPath("$.data[0].displayName").value("Code Generation")); } + + @Test + void labelMutationShouldNotUseThePublicGetOnlySecurityChain() throws Exception { + mockMvc.perform(post("/api/v1/labels")) + .andExpect(status().isUnauthorized()); + } } diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/metadata/SkillMetadataParser.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/metadata/SkillMetadataParser.java index c44a4516..4c5055b1 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/metadata/SkillMetadataParser.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/metadata/SkillMetadataParser.java @@ -1,7 +1,9 @@ package com.iflytek.skillhub.domain.skill.metadata; import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException; +import org.yaml.snakeyaml.LoaderOptions; import org.yaml.snakeyaml.Yaml; +import org.yaml.snakeyaml.constructor.SafeConstructor; import java.util.LinkedHashMap; import java.util.Map; @@ -13,6 +15,9 @@ import java.util.Map; public class SkillMetadataParser { private static final String FRONTMATTER_DELIMITER = "---"; + private static final int MAX_YAML_ALIASES = 20; + private static final int MAX_YAML_NESTING_DEPTH = 20; + private static final int MAX_YAML_CODE_POINTS = 200_000; public SkillMetadata parse(String content) { if (content == null || content.isBlank()) { @@ -59,7 +64,12 @@ public class SkillMetadataParser { private Map parseFrontmatter(String yamlContent) { try { - Yaml yaml = new Yaml(); + LoaderOptions loaderOptions = new LoaderOptions(); + loaderOptions.setAllowDuplicateKeys(false); + loaderOptions.setMaxAliasesForCollections(MAX_YAML_ALIASES); + loaderOptions.setNestingDepthLimit(MAX_YAML_NESTING_DEPTH); + loaderOptions.setCodePointLimit(MAX_YAML_CODE_POINTS); + Yaml yaml = new Yaml(new SafeConstructor(loaderOptions)); Object parsed = yaml.load(yamlContent); if (!(parsed instanceof Map)) { throw new DomainBadRequestException("error.skill.metadata.yaml.notMap"); diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/BasicPrePublishValidator.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/BasicPrePublishValidator.java index 19473d61..dbd93c5d 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/BasicPrePublishValidator.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/BasicPrePublishValidator.java @@ -6,6 +6,7 @@ import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.List; import java.util.Locale; +import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -16,9 +17,9 @@ import java.util.regex.Pattern; @Component public class BasicPrePublishValidator implements PrePublishValidator { - private static final Pattern PLACEHOLDER_VALUE = Pattern.compile( - "(?i).*(your|example|sample|placeholder|changeme|replace|dummy|mock|test|fake|todo|xxx|redacted).*" - ); + private static final Set PLACEHOLDER_MARKERS = Set.of( + "your", "example", "sample", "placeholder", "changeme", "replace", "dummy", + "mock", "test", "fake", "todo", "xxx", "redacted"); private static final List SECRET_RULES = List.of( new SecretRule(Pattern.compile("(AKIA[0-9A-Z]{16})"), 1, "cloud access key"), new SecretRule(Pattern.compile("(ghp_[A-Za-z0-9]{20,})"), 1, "GitHub token"), @@ -83,7 +84,8 @@ public class BasicPrePublishValidator implements PrePublishValidator { if (value == null || value.isBlank()) { return false; } - return PLACEHOLDER_VALUE.matcher(value).matches() + String normalizedValue = value.toLowerCase(Locale.ROOT); + return PLACEHOLDER_MARKERS.stream().anyMatch(normalizedValue::contains) || value.chars().allMatch(ch -> ch == 'x' || ch == 'X' || ch == '*' || ch == '-'); } diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/metadata/SkillMetadataParserTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/metadata/SkillMetadataParserTest.java index aececcd2..ba449ee1 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/metadata/SkillMetadataParserTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/metadata/SkillMetadataParserTest.java @@ -199,6 +199,22 @@ class SkillMetadataParserTest { assertEquals("1.0.0", metadata.version()); } + @Test + void testDoesNotInstantiateUnsafeGlobalYamlTags() { + String content = """ + --- + name: safe-skill + description: Reject unsafe YAML object construction + payload: !!java.util.Date [] + --- + Body + """; + + SkillMetadata metadata = parser.parse(content); + + assertEquals("!!java.util.Date []", metadata.frontmatter().get("payload")); + } + @Test void testThrowsWhenNoClosingDelimiter() { String content = """ diff --git a/web/e2e/helpers/session.ts b/web/e2e/helpers/session.ts index 91554c62..27e5e4e2 100644 --- a/web/e2e/helpers/session.ts +++ b/web/e2e/helpers/session.ts @@ -1,4 +1,5 @@ import { expect, type Page, type TestInfo } from '@playwright/test' +import { randomUUID } from 'node:crypto' import { csrfHeaders } from './csrf' const password = 'Passw0rd!123' @@ -38,7 +39,7 @@ function usernameForWorker(testInfo?: TestInfo): string { function uniqueUsernameForWorker(testInfo?: TestInfo): string { const worker = testInfo?.parallelIndex ?? 0 - const suffix = `${Date.now().toString(36)}${Math.random().toString(36).slice(2, 7)}` + const suffix = `${Date.now().toString(36)}${randomUUID().slice(0, 8)}` return `e2e_w${worker}_${suffix}` } diff --git a/web/e2e/register-email-required.spec.ts b/web/e2e/register-email-required.spec.ts index 54d2476a..8bdefefa 100644 --- a/web/e2e/register-email-required.spec.ts +++ b/web/e2e/register-email-required.spec.ts @@ -1,8 +1,9 @@ import { expect, test } from '@playwright/test' +import { randomUUID } from 'node:crypto' import { setEnglishLocale } from './helpers/auth-fixtures' function buildUniqueUser() { - const suffix = `${Date.now().toString(36)}${Math.random().toString(36).slice(2, 7)}` + const suffix = `${Date.now().toString(36)}${randomUUID().slice(0, 8)}` return { username: `e2e_reg_${suffix}`, email: `e2e_reg_${suffix}@example.test`, diff --git a/web/src/pages/cli-auth.test.ts b/web/src/pages/cli-auth.test.ts index a1611850..6b3a3e9a 100644 --- a/web/src/pages/cli-auth.test.ts +++ b/web/src/pages/cli-auth.test.ts @@ -1,9 +1,5 @@ import { describe, expect, it, vi } from 'vitest' -// CliAuthPage has internal helpers isValidRedirectUri and decodeLabel which are -// not exported. We test the component render paths and validate the redirect -// URI logic via the rendered error states. - vi.mock('@tanstack/react-router', () => ({ useNavigate: () => vi.fn(), })) @@ -36,7 +32,32 @@ vi.mock('@/app/router', () => ({ ORIGINAL_URL_SEARCH: '', })) -import { CliAuthPage, resolveCliRegistryUrl } from './cli-auth' +import { CliAuthPage, resolveCliRegistryUrl, resolveLoopbackRedirectUri } from './cli-auth' + +describe('resolveLoopbackRedirectUri', () => { + it.each([ + 'http://localhost:4312/callback?source=cli', + 'http://127.0.0.1:4312/callback', + 'http://[::1]:4312/callback', + ])('accepts an HTTP loopback callback: %s', (uri) => { + expect(resolveLoopbackRedirectUri(uri)?.href).toBe(uri) + }) + + it.each([ + 'https://localhost:4312/callback', + 'http://localhost.example.com/callback', + 'http://example.com/callback', + 'http://user:password@localhost:4312/callback', + 'javascript:alert(1)', + 'not-a-url', + ])('rejects a non-loopback or unsafe callback: %s', (uri) => { + expect(resolveLoopbackRedirectUri(uri)).toBeNull() + }) + + it('removes an attacker-provided fragment before adding CLI credentials', () => { + expect(resolveLoopbackRedirectUri('http://localhost:4312/callback#attacker')?.hash).toBe('') + }) +}) describe('resolveCliRegistryUrl', () => { it('uses the configured public base URL for the CLI registry', () => { diff --git a/web/src/pages/cli-auth.tsx b/web/src/pages/cli-auth.tsx index a6be44a4..2c247012 100644 --- a/web/src/pages/cli-auth.tsx +++ b/web/src/pages/cli-auth.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect } from 'react' +import { useEffect, useMemo, useState } from 'react' import { useNavigate } from '@tanstack/react-router' import { useTranslation } from 'react-i18next' import { Card } from '@/shared/ui/card' @@ -12,14 +12,19 @@ import { resolvePublicRegistryUrl } from '@/shared/lib/registry-url' // Parse the original URL params captured before TanStack Router rewrites const ORIGINAL_PARAMS = new URLSearchParams(ORIGINAL_URL_SEARCH) -function isValidRedirectUri(uri: string): boolean { +const LOOPBACK_HOSTS = new Set(['localhost', '127.0.0.1', '[::1]', '::1']) + +export function resolveLoopbackRedirectUri(uri: string): URL | null { try { const url = new URL(uri) - // Only allow localhost/127.0.0.1/::1 on HTTP - const validHosts = ['localhost', '127.0.0.1', '[::1]', '::1'] - return url.protocol === 'http:' && validHosts.includes(url.hostname.toLowerCase()) + const isLoopbackHttp = url.protocol === 'http:' && LOOPBACK_HOSTS.has(url.hostname.toLowerCase()) + if (!isLoopbackHttp || url.username || url.password) { + return null + } + url.hash = '' + return url } catch { - return false + return null } } @@ -55,13 +60,10 @@ export function CliAuthPage() { const labelB64 = ORIGINAL_PARAMS.get('label_b64')?.trim() || undefined const labelPlain = ORIGINAL_PARAMS.get('label')?.trim() || undefined const label = decodeLabel(labelB64, labelPlain) - - // Debug: log search params and raw URL - console.log('CLI Auth - Original search (from router.tsx):', ORIGINAL_URL_SEARCH) - console.log('CLI Auth - Current URL:', typeof window !== 'undefined' ? window.location.href : 'SSR') - console.log('CLI Auth - redirectUri:', redirectUri) - console.log('CLI Auth - state:', state) - console.log('CLI Auth - label:', label) + const redirectTarget = useMemo( + () => redirectUri ? resolveLoopbackRedirectUri(redirectUri) : null, + [redirectUri], + ) useEffect(() => { // Check authentication status @@ -89,7 +91,7 @@ export function CliAuthPage() { } // Validate redirect_uri - if (!redirectUri || !isValidRedirectUri(redirectUri)) { + if (!redirectTarget) { setStatus('error') setErrorMessage(t('cliAuth.invalidRedirectUri')) return @@ -125,16 +127,17 @@ export function CliAuthPage() { hashParams.set('registry', registryUrl) hashParams.set('state', state) - const redirectUrl = `${redirectUri}#${hashParams.toString()}` + const redirectUrl = new URL(redirectTarget.href) + redirectUrl.hash = hashParams.toString() // Redirect to CLI's loopback server - window.location.assign(redirectUrl) + window.location.assign(redirectUrl.href) }) .catch((error) => { setStatus('error') setErrorMessage(error instanceof Error ? error.message : t('cliAuth.tokenCreationFailed')) }) - }, [user, redirectUri, state, label, t]) + }, [user, redirectUri, redirectTarget, state, label, t]) if (status === 'validating') { return (