diff --git a/Makefile b/Makefile index bf3e7439..778d680d 100644 --- a/Makefile +++ b/Makefile @@ -296,12 +296,12 @@ db-reset: ## 重置数据库 validate-release-config: ## 校验发布环境变量文件（默认 .env.release） ./scripts/validate-release-config.sh .env.release -staging: ## 构建并启动 staging 环境，运行 smoke test（混合模式：后端镜像 + 前端静态文件） +staging: ## 构建并启动 staging 环境，运行 smoke test（混合模式：后端镜像 + 前端镜像） @echo "=== [1/5] Building backend JAR and Docker image ===" cd server && ./mvnw package -DskipTests -B -q docker build -t $(STAGING_SERVER_IMAGE) -f server/Dockerfile.dev server - @echo "=== [2/5] Building frontend static files ===" - cd web && pnpm run build + @echo "=== [2/5] Building frontend Docker image ===" + docker build -t skillhub-web:staging -f web/Dockerfile web @echo "=== [3/5] Starting dependency services ===" $(STAGING_BASE_COMPOSE) up -d --wait @echo "=== [4/5] Starting staging services ===" diff --git a/cli/src/shared/skill-name-parser.ts b/cli/src/shared/skill-name-parser.ts index 05e0662b..8ed66174 100644 --- a/cli/src/shared/skill-name-parser.ts +++ b/cli/src/shared/skill-name-parser.ts @@ -5,6 +5,7 @@ export interface ParsedSkillName { export function parseSkillName(skillName: string, defaultNamespace = 'global'): ParsedSkillName { const separatorIndex = skillName.indexOf('--') + const trailingSeparatorIndex = skillName.lastIndexOf('--') if (separatorIndex <= 0) { return { @@ -13,7 +14,7 @@ export function parseSkillName(skillName: string, defaultNamespace = 'global'): } } - if (separatorIndex === skillName.length - 2) { + if (trailingSeparatorIndex === separatorIndex && trailingSeparatorIndex + 2 === skillName.length) { return { namespace: defaultNamespace, slug: skillName.slice(0, -2) diff --git a/cli/test/unit/shared/skill-name-parser.test.ts b/cli/test/unit/shared/skill-name-parser.test.ts index b86771ce..011a50ca 100644 --- a/cli/test/unit/shared/skill-name-parser.test.ts +++ b/cli/test/unit/shared/skill-name-parser.test.ts @@ -26,6 +26,14 @@ describe('parseSkillName', () => { slug: 'slug--with--dashes' }) }) + + test('should preserve namespace when namespaced slug ends with separator characters', () => { + const result = parseSkillName('namespace--slug--') + expect(result).toEqual({ + namespace: 'namespace', + slug: 'slug--' + }) + }) }) describe('with slug only format', () => { diff --git a/docker-compose.staging.yml b/docker-compose.staging.yml index 48d1fd1a..8e96e4b3 100644 --- a/docker-compose.staging.yml +++ b/docker-compose.staging.yml @@ -1,6 +1,6 @@ # Staging environment: hybrid mode # - Backend: locally built Docker image -# - Frontend: locally built static files mounted into Nginx +# - Frontend: locally built Docker image # - Dependencies: reuses docker-compose.yml (postgres, redis, minio) # # Usage: make staging @@ -23,7 +23,7 @@ services: REDIS_PORT: 6379 SESSION_COOKIE_SECURE: "false" SKILLHUB_PUBLIC_BASE_URL: "http://localhost" - DEVICE_AUTH_VERIFICATION_URI: "http://localhost/cli/auth" + DEVICE_AUTH_VERIFICATION_URI: "http://localhost/device" SKILLHUB_STORAGE_PROVIDER: s3 STORAGE_BASE_PATH: /var/lib/skillhub/storage SKILLHUB_STORAGE_S3_ENDPOINT: http://minio:9000 @@ -61,12 +61,12 @@ services: start_period: 60s web: - image: nginx:alpine + image: skillhub-web:staging + build: + context: ./web + dockerfile: Dockerfile ports: - "80:80" - volumes: - - ./web/dist:/usr/share/nginx/html:ro - - ./web/nginx.conf.template:/etc/nginx/templates/default.conf.template:ro environment: SKILLHUB_API_UPSTREAM: http://server:8080 SKILLHUB_WEB_API_BASE_URL: "" 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..8f826e7b 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 @@ -10,6 +10,7 @@ import org.springframework.security.web.SecurityFilterChain; import org.springframework.security.web.authentication.UsernamePasswordAuthenticationFilter; import org.springframework.security.web.util.matcher.AntPathRequestMatcher; import org.springframework.security.web.util.matcher.OrRequestMatcher; +import org.springframework.security.web.util.matcher.RequestMatcher; /** * Declares a dedicated stateless security chain for public compatibility endpoints used by @@ -22,14 +23,8 @@ public class ClawHubRegistrySecurityConfig { @Order(0) public SecurityFilterChain publicLabelFilterChain(HttpSecurity http) throws Exception { http - .securityMatcher( - new OrRequestMatcher( - new AntPathRequestMatcher("/api/v1/labels"), - new AntPathRequestMatcher("/api/web/labels") - ) - ) + .securityMatcher(publicLabelRequestMatcher()) .authorizeHttpRequests(auth -> auth.anyRequest().permitAll()) - .csrf(csrf -> csrf.disable()) .requestCache(cache -> cache.disable()) .securityContext(context -> context.disable()) .sessionManagement(session -> session.sessionCreationPolicy(SessionCreationPolicy.STATELESS)); @@ -37,6 +32,13 @@ public class ClawHubRegistrySecurityConfig { return http.build(); } + static RequestMatcher publicLabelRequestMatcher() { + return new OrRequestMatcher( + new AntPathRequestMatcher("/api/v1/labels", "GET"), + new AntPathRequestMatcher("/api/web/labels", "GET") + ); + } + @Bean @Order(1) public SecurityFilterChain clawHubRegistryFilterChain( diff --git a/server/skillhub-app/src/main/resources/application.yml b/server/skillhub-app/src/main/resources/application.yml index 286ff05f..b89a9897 100644 --- a/server/skillhub-app/src/main/resources/application.yml +++ b/server/skillhub-app/src/main/resources/application.yml @@ -163,7 +163,7 @@ skillhub: editable: false requires-review: false device-auth: - verification-uri: ${DEVICE_AUTH_VERIFICATION_URI:${skillhub.public.base-url:}/cli/auth} + verification-uri: ${DEVICE_AUTH_VERIFICATION_URI:${skillhub.public.base-url:}/device} security: scanner: enabled: ${SKILLHUB_SECURITY_SCANNER_ENABLED:true} diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/compat/ClawHubRegistrySecurityConfigTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/compat/ClawHubRegistrySecurityConfigTest.java new file mode 100644 index 00000000..7d1b0585 --- /dev/null +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/compat/ClawHubRegistrySecurityConfigTest.java @@ -0,0 +1,27 @@ +package com.iflytek.skillhub.compat; + +import org.junit.jupiter.api.Test; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.security.web.util.matcher.RequestMatcher; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class ClawHubRegistrySecurityConfigTest { + + @Test + void publicLabelMatcherOnlyMatchesGetRequests() { + RequestMatcher matcher = ClawHubRegistrySecurityConfig.publicLabelRequestMatcher(); + + assertTrue(matcher.matches(request("GET", "/api/v1/labels"))); + assertTrue(matcher.matches(request("GET", "/api/web/labels"))); + assertFalse(matcher.matches(request("POST", "/api/v1/labels"))); + assertFalse(matcher.matches(request("POST", "/api/web/labels"))); + } + + private MockHttpServletRequest request(String method, String path) { + MockHttpServletRequest request = new MockHttpServletRequest(method, path); + request.setServletPath(path); + return request; + } +} diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/device/DeviceAuthService.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/device/DeviceAuthService.java index e838c9a8..54738da4 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/device/DeviceAuthService.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/device/DeviceAuthService.java @@ -38,7 +38,7 @@ public class DeviceAuthService { public DeviceAuthService(RedisTemplate redisTemplate, ApiTokenService apiTokenService, - @Value("${skillhub.device-auth.verification-uri:/cli/auth}") String verificationUri) { + @Value("${skillhub.device-auth.verification-uri:/device}") String verificationUri) { this.redisTemplate = redisTemplate; this.apiTokenService = apiTokenService; this.verificationUri = verificationUri; 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..87cad7c4 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,8 @@ import java.util.Map; public class SkillMetadataParser { private static final String FRONTMATTER_DELIMITER = "---"; + private static final int FRONTMATTER_CODE_POINT_LIMIT = 64 * 1024; + private static final int FRONTMATTER_NESTING_DEPTH_LIMIT = 20; public SkillMetadata parse(String content) { if (content == null || content.isBlank()) { @@ -58,8 +62,11 @@ public class SkillMetadataParser { } private Map parseFrontmatter(String yamlContent) { + if (containsExplicitYamlTag(yamlContent)) { + throw new DomainBadRequestException("error.skill.metadata.yaml.invalid", "Explicit YAML tags are not allowed"); + } try { - Yaml yaml = new Yaml(); + Yaml yaml = newSafeYamlParser(); Object parsed = yaml.load(yamlContent); if (!(parsed instanceof Map)) { throw new DomainBadRequestException("error.skill.metadata.yaml.notMap"); @@ -78,6 +85,134 @@ public class SkillMetadataParser { } } + private Yaml newSafeYamlParser() { + LoaderOptions options = new LoaderOptions(); + options.setAllowDuplicateKeys(false); + options.setCodePointLimit(FRONTMATTER_CODE_POINT_LIMIT); + options.setNestingDepthLimit(FRONTMATTER_NESTING_DEPTH_LIMIT); + return new Yaml(new SafeConstructor(options)); + } + + private boolean containsExplicitYamlTag(String yamlContent) { + return yamlContent.lines() + .map(String::trim) + .filter(line -> !line.isEmpty() && !line.startsWith("#")) + .anyMatch(this::containsExplicitYamlTagInLine); + } + + private boolean containsExplicitYamlTagInLine(String line) { + String candidate = stripLeadingIndicatorsAndAnchors(line); + if (startsWithExplicitYamlTag(candidate)) { + return true; + } + if (candidate.startsWith("[") || candidate.startsWith("{")) { + return containsFlowExplicitYamlTag(candidate); + } + + int separatorIndex = line.indexOf(':'); + if (separatorIndex <= 0) { + return false; + } + + String value = stripLeadingAnchors(line.substring(separatorIndex + 1)); + if (startsWithExplicitYamlTag(value)) { + return true; + } + return (value.startsWith("[") || value.startsWith("{")) && containsFlowExplicitYamlTag(value); + } + + private String stripLeadingIndicatorsAndAnchors(String line) { + String candidate = line.trim(); + boolean advanced; + do { + advanced = false; + if (candidate.startsWith("- ") || candidate.startsWith("? ") || candidate.startsWith(": ")) { + candidate = candidate.substring(1).trim(); + advanced = true; + continue; + } + + String withoutAnchor = stripLeadingAnchors(candidate); + if (!withoutAnchor.equals(candidate)) { + candidate = withoutAnchor; + advanced = true; + } + } while (advanced); + return candidate; + } + + private String stripLeadingAnchors(String value) { + String candidate = value.trim(); + while (candidate.startsWith("&") && candidate.length() > 1) { + int end = 1; + while (end < candidate.length() + && !Character.isWhitespace(candidate.charAt(end)) + && !isYamlFlowDelimiter(candidate.charAt(end))) { + end++; + } + if (end == 1) { + break; + } + candidate = candidate.substring(end).trim(); + } + return candidate; + } + + private boolean startsWithExplicitYamlTag(String value) { + return value.startsWith("!") && value.length() > 1 && !Character.isWhitespace(value.charAt(1)); + } + + private boolean containsFlowExplicitYamlTag(String value) { + int flowDepth = 0; + char quote = '\0'; + for (int i = 0; i < value.length(); i++) { + char current = value.charAt(i); + if (quote != '\0') { + if (current == quote && !isEscapedDoubleQuote(value, i, quote)) { + quote = '\0'; + } + continue; + } + if (current == '\'' || current == '"') { + quote = current; + continue; + } + + if (current == '[' || current == '{') { + flowDepth++; + if (startsWithExplicitYamlTag(stripLeadingAnchors(value.substring(i + 1)))) { + return true; + } + continue; + } + if (current == ']' || current == '}') { + flowDepth = Math.max(0, flowDepth - 1); + continue; + } + if (flowDepth > 0 + && (current == ',' || current == ':') + && startsWithExplicitYamlTag(stripLeadingAnchors(value.substring(i + 1)))) { + return true; + } + } + return false; + } + + private boolean isEscapedDoubleQuote(String value, int index, char quote) { + if (quote != '"' || index == 0) { + return false; + } + int backslashCount = 0; + for (int i = index - 1; i >= 0 && value.charAt(i) == '\\'; i--) { + backslashCount++; + } + return backslashCount % 2 == 1; + } + + private boolean isYamlFlowDelimiter(char current) { + return current == '[' || current == ']' || current == '{' || current == '}' || current == ','; + } + private Map parseLooseFrontmatter(String yamlContent) { Map values = new LinkedHashMap<>(); for (String rawLine : yamlContent.split("\\R")) { 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..a6098e8f 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 @@ -16,8 +16,20 @@ 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 List PLACEHOLDER_MARKERS = List.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"), @@ -83,7 +95,8 @@ public class BasicPrePublishValidator implements PrePublishValidator { if (value == null || value.isBlank()) { return false; } - return PLACEHOLDER_VALUE.matcher(value).matches() + String lowerValue = value.toLowerCase(Locale.ROOT); + return PLACEHOLDER_MARKERS.stream().anyMatch(lowerValue::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..98d6dac3 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 @@ -181,6 +181,110 @@ class SkillMetadataParserTest { assertEquals("1.0.0", metadata.version()); } + @Test + void rejectsExplicitYamlTagsInsteadOfFallingBackToLooseParsing() { + String content = """ + --- + name: !!java.net.URL ["https://example.test"] + description: malicious tag should not be accepted + version: 1.0.0 + --- + Body + """; + + DomainBadRequestException exception = assertThrows( + DomainBadRequestException.class, + () -> parser.parse(content) + ); + assertEquals("error.skill.metadata.yaml.invalid", exception.messageCode()); + } + + @Test + void rejectsSingleBangYamlTagsInsteadOfFallingBackToLooseParsing() { + String content = """ + --- + name: !java.net.URL ["https://example.test"] + description: malicious tag should not be accepted + version: 1.0.0 + --- + Body + """; + + DomainBadRequestException exception = assertThrows( + DomainBadRequestException.class, + () -> parser.parse(content) + ); + assertEquals("error.skill.metadata.yaml.invalid", exception.messageCode()); + } + + @Test + void rejectsUriStyleYamlTagsInsteadOfFallingBackToLooseParsing() { + String content = """ + --- + name: ! ["https://example.test"] + description: malicious tag should not be accepted + version: 1.0.0 + --- + Body + """; + + DomainBadRequestException exception = assertThrows( + DomainBadRequestException.class, + () -> parser.parse(content) + ); + assertEquals("error.skill.metadata.yaml.invalid", exception.messageCode()); + } + + @Test + void rejectsAnchoredExplicitYamlTagsInsteadOfFallingBackToLooseParsing() { + String content = """ + --- + name: &x !!java.net.URL ["https://example.test"] + description: anchored malicious tag should not be accepted + version: 1.0.0 + --- + Body + """; + + DomainBadRequestException exception = assertThrows( + DomainBadRequestException.class, + () -> parser.parse(content) + ); + assertEquals("error.skill.metadata.yaml.invalid", exception.messageCode()); + } + + @Test + void allowsPlainScalarExclamationMarks() { + String content = """ + --- + name: Excited Skill + description: This is important! + version: 1.0.0 + --- + Body + """; + + SkillMetadata metadata = parser.parse(content); + + assertEquals("This is important!", metadata.description()); + } + + @Test + void allowsPlainScalarCommaBeforeExclamationWord() { + String content = """ + --- + name: css-guidance + description: Avoid CSS, !important when possible + version: 1.0.0 + --- + Body + """; + + SkillMetadata metadata = parser.parse(content); + + assertEquals("Avoid CSS, !important when possible", metadata.description()); + } + @Test void testAllowsColonInDescriptionWithoutStrictYamlQuoting() { String content = """ diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/validation/BasicPrePublishValidatorTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/validation/BasicPrePublishValidatorTest.java index f428559c..3b67900c 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/validation/BasicPrePublishValidatorTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/validation/BasicPrePublishValidatorTest.java @@ -98,4 +98,25 @@ class BasicPrePublishValidatorTest { assertTrue(result.passed()); } + + @Test + void shouldTreatVeryLongPlaceholderValuesLinearly() { + String content = "token=" + "x".repeat(20_000); + PackageEntry env = new PackageEntry( + ".env", + content.getBytes(StandardCharsets.UTF_8), + content.length(), + "text/plain" + ); + + ValidationResult result = validator.validate(new PrePublishValidator.SkillPackageContext( + List.of(env), + new SkillMetadata("Example Skill", "desc", "1.0.0", "body", Map.of()), + "user-1", + 1L + )); + + assertTrue(result.passed()); + assertFalse(result.hasWarnings()); + } } diff --git a/web/e2e/cli-auth.spec.ts b/web/e2e/cli-auth.spec.ts index bdf21320..56ee48c2 100644 --- a/web/e2e/cli-auth.spec.ts +++ b/web/e2e/cli-auth.spec.ts @@ -6,8 +6,30 @@ test.describe('CLI Auth (Real API)', () => { await setEnglishLocale(page) }) - test('shows error for missing redirect params', async ({ page }) => { + test('shows legacy callback notice and sends users to device authorization', async ({ page }) => { await page.goto('/cli/auth') - await expect(page.getByRole('heading', { name: 'Authorization failed' })).toBeVisible() + + await expect(page.getByRole('heading', { name: 'Use device authorization' })).toBeVisible() + await page.getByRole('button', { name: 'Open Device Authorization' }).click() + + await expect(page).toHaveURL(/\/login\?returnTo=%2Fdevice$/) + }) + + test('redirects anonymous users from device authorization to login', async ({ page }) => { + await page.goto('/device') + + await expect(page).toHaveURL(/\/login\?returnTo=%2Fdevice$/) + }) + + test('shows device authorization form for authenticated users', async ({ page }) => { + await page.context().setExtraHTTPHeaders({ + 'X-Mock-User-Id': 'local-user', + }) + + await page.goto('/device') + + await expect(page.getByRole('heading', { name: 'Device Authorization' })).toBeVisible() + await expect(page.getByText('User Code', { exact: true })).toBeVisible() + await expect(page.getByRole('button', { name: 'Authorize Device' })).toBeVisible() }) }) diff --git a/web/e2e/helpers/session.ts b/web/e2e/helpers/session.ts index 91554c62..5c5f3651 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().replace(/-/g, '').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..e26eb28a 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().replace(/-/g, '').slice(0, 8)}` return { username: `e2e_reg_${suffix}`, email: `e2e_reg_${suffix}@example.test`, diff --git a/web/src/app/router.tsx b/web/src/app/router.tsx index c9b3bac8..3ace5689 100644 --- a/web/src/app/router.tsx +++ b/web/src/app/router.tsx @@ -109,6 +109,7 @@ const MySubscriptionsPage = createLazyRouteComponent(() => import('@/pages/dashb const NotificationsPage = createLazyRouteComponent(() => import('@/pages/notifications'), 'NotificationsPage') const TokensPage = createLazyRouteComponent(() => import('@/pages/dashboard/tokens'), 'TokensPage') const CliAuthPage = createLazyRouteComponent(() => import('@/pages/cli-auth'), 'CliAuthPage') +const DeviceAuthPage = createLazyRouteComponent(() => import('@/pages/device'), 'DeviceAuthPage') const SecuritySettingsPage = createLazyRouteComponent( () => import('@/pages/settings/security'), 'SecuritySettingsPage', @@ -383,6 +384,13 @@ const cliAuthRoute = createRoute({ }, }) +const deviceAuthRoute = createRoute({ + getParentRoute: () => rootRoute, + path: 'device', + beforeLoad: requireAuth, + component: DeviceAuthPage, +}) + const settingsSecurityRoute = createRoute({ getParentRoute: () => rootRoute, path: 'settings/security', @@ -463,6 +471,7 @@ const routeTree = rootRoute.addChildren([ dashboardNotificationsRoute, dashboardTokensRoute, cliAuthRoute, + deviceAuthRoute, settingsSecurityRoute, settingsProfileRoute, settingsNotificationsRoute, diff --git a/web/src/i18n/locales/en.json b/web/src/i18n/locales/en.json index 2e7d4f28..9eae0d1a 100644 --- a/web/src/i18n/locales/en.json +++ b/web/src/i18n/locales/en.json @@ -294,6 +294,9 @@ "notice": "After authorization, the device will have access to your account" }, "cliAuth": { + "legacyDisabledTitle": "Use device authorization", + "legacyDisabledDescription": "This legacy CLI callback no longer creates browser tokens. Continue with the device authorization page to finish CLI sign-in safely.", + "openDeviceAuth": "Open Device Authorization", "validating": "Validating...", "pleaseWait": "Please wait", "creatingToken": "Creating token...", diff --git a/web/src/i18n/locales/zh.json b/web/src/i18n/locales/zh.json index 2281121a..8edb95dc 100644 --- a/web/src/i18n/locales/zh.json +++ b/web/src/i18n/locales/zh.json @@ -294,6 +294,9 @@ "notice": "授权后,设备将可以访问你的账户" }, "cliAuth": { + "legacyDisabledTitle": "请使用设备授权", + "legacyDisabledDescription": "旧版 CLI 回调页不再在浏览器中创建令牌。请前往设备授权页，安全完成 CLI 登录。", + "openDeviceAuth": "打开设备授权", "validating": "验证中...", "pleaseWait": "请稍候", "creatingToken": "创建令牌中...", diff --git a/web/src/pages/cli-auth.test.ts b/web/src/pages/cli-auth.test.ts index 0f6cdf24..fc380b1c 100644 --- a/web/src/pages/cli-auth.test.ts +++ b/web/src/pages/cli-auth.test.ts @@ -1,11 +1,15 @@ -import { describe, expect, it, vi } from 'vitest' +import React from 'react' +import { renderToStaticMarkup } from 'react-dom/server' +import { beforeEach, 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. +const navigateMock = vi.hoisted(() => vi.fn()) +const createTokenMock = vi.hoisted(() => vi.fn()) +const buttonState = vi.hoisted(() => ({ + onClick: undefined as React.MouseEventHandler | undefined, +})) vi.mock('@tanstack/react-router', () => ({ - useNavigate: () => vi.fn(), + useNavigate: () => navigateMock, })) vi.mock('react-i18next', async () => { @@ -18,28 +22,54 @@ vi.mock('react-i18next', async () => { } }) -vi.mock('@/shared/ui/card', () => ({ - Card: ({ children }: { children: unknown }) => children, -})) +vi.mock('@/shared/ui/card', async () => { + const ReactModule = await vi.importActual('react') + return { + Card: ({ children }: { children: React.ReactNode }) => ReactModule.createElement('div', null, children), + } +}) -vi.mock('@/shared/ui/button', () => ({ - Button: ({ children }: { children: unknown }) => children, -})) +vi.mock('@/shared/ui/button', async () => { + const ReactModule = await vi.importActual('react') + return { + Button: ({ + children, + onClick, + ...props + }: React.ButtonHTMLAttributes & { children: React.ReactNode }) => { + buttonState.onClick = onClick + return ReactModule.createElement('button', props, children) + }, + } +}) vi.mock('@/api/client', () => ({ - getCurrentUser: vi.fn().mockResolvedValue(null), - tokenApi: { createToken: vi.fn() }, -})) - -vi.mock('@/app/router', () => ({ - ORIGINAL_URL_SEARCH: '', + tokenApi: { createToken: createTokenMock }, })) import { CliAuthPage } from './cli-auth' describe('CliAuthPage', () => { + beforeEach(() => { + navigateMock.mockReset() + createTokenMock.mockReset() + buttonState.onClick = undefined + }) + it('exports a named component function', () => { expect(typeof CliAuthPage).toBe('function') expect(CliAuthPage.name).toBe('CliAuthPage') }) + + it('disables legacy loopback token redirects and opens device authorization instead', () => { + const html = renderToStaticMarkup(React.createElement(CliAuthPage)) + + expect(html).toContain('cliAuth.legacyDisabledTitle') + expect(createTokenMock).not.toHaveBeenCalled() + + buttonState.onClick?.({} as React.MouseEvent) + + expect(navigateMock).toHaveBeenCalledWith({ to: '/device' }) + expect(createTokenMock).not.toHaveBeenCalled() + }) }) diff --git a/web/src/pages/cli-auth.tsx b/web/src/pages/cli-auth.tsx index 73679948..73b202ed 100644 --- a/web/src/pages/cli-auth.tsx +++ b/web/src/pages/cli-auth.tsx @@ -1,222 +1,28 @@ -import { useState, useEffect } from 'react' import { useNavigate } from '@tanstack/react-router' +import { ArrowRight, ShieldAlert } from 'lucide-react' import { useTranslation } from 'react-i18next' import { Card } from '@/shared/ui/card' import { Button } from '@/shared/ui/button' -import { getCurrentUser, tokenApi } from '@/api/client' -import type { User } from '@/api/types' -import { ORIGINAL_URL_SEARCH } from '@/app/router' - -// Parse the original URL params captured before TanStack Router rewrites -const ORIGINAL_PARAMS = new URLSearchParams(ORIGINAL_URL_SEARCH) - -function isValidRedirectUri(uri: string): boolean { - 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()) - } catch { - return false - } -} - -function decodeLabel(labelB64?: string, labelPlain?: string): string { - if (labelB64) { - try { - // Base64-URL decode - const base64 = labelB64.replace(/-/g, '+').replace(/_/g, '/') - return atob(base64) - } catch { - // Fallback to plain label - } - } - return labelPlain || 'CLI token' -} export function CliAuthPage() { const { t } = useTranslation() const navigate = useNavigate() - const [user, setUser] = useState(undefined) - const [status, setStatus] = useState<'validating' | 'creating' | 'redirecting' | 'error'>('validating') - const [errorMessage, setErrorMessage] = useState('') - const [token, setToken] = useState('') - - // Use the captured original params from module load time - const redirectUri = ORIGINAL_PARAMS.get('redirect_uri')?.trim() || undefined - const state = ORIGINAL_PARAMS.get('state')?.trim() || undefined - 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) - - useEffect(() => { - // Check authentication status - getCurrentUser() - .then((currentUser) => { - setUser(currentUser) - }) - .catch(() => { - setUser(null) - }) - }, []) - - useEffect(() => { - // Once we know the user status, proceed with token creation - if (user === undefined) { - // Still loading - return - } - - if (user === null) { - // Not authenticated - user needs to log in - setStatus('error') - setErrorMessage(t('cliAuth.notAuthenticated')) - return - } - - // Validate redirect_uri - if (!redirectUri || !isValidRedirectUri(redirectUri)) { - setStatus('error') - setErrorMessage(t('cliAuth.invalidRedirectUri')) - return - } - - // Validate state - if (!state) { - setStatus('error') - // Special error message for Windows users with missing state - if (redirectUri && typeof window !== 'undefined' && navigator.platform.includes('Win')) { - setErrorMessage(t('cliAuth.windowsUrlBug')) - } else { - setErrorMessage(t('cliAuth.missingState')) - } - return - } - - // Create token and redirect - setStatus('creating') - tokenApi - .createToken({ - name: label, - scopes: ['skill:read', 'skill:publish'], - }) - .then((response) => { - setToken(response.token) - setStatus('redirecting') - - // Construct redirect URL with token in hash fragment - const registryUrl = window.location.origin - const hashParams = new URLSearchParams() - hashParams.set('token', response.token) - hashParams.set('registry', registryUrl) - hashParams.set('state', state) - - const redirectUrl = `${redirectUri}#${hashParams.toString()}` - - // Redirect to CLI's loopback server - window.location.assign(redirectUrl) - }) - .catch((error) => { - setStatus('error') - setErrorMessage(error instanceof Error ? error.message : t('cliAuth.tokenCreationFailed')) - }) - }, [user, redirectUri, state, label, t]) - - if (status === 'validating') { - return ( -
- -
- - - -
-

{t('cliAuth.validating')}

-

{t('cliAuth.pleaseWait')}

- -
- ) - } - - if (status === 'creating') { - return ( -
- -
- - - -
-

{t('cliAuth.creatingToken')}

-

{t('cliAuth.almostThere')}

- -
- ) - } - - if (status === 'redirecting') { - return ( -
- -
- - - -
-

{t('cliAuth.success')}

-

{t('cliAuth.redirecting')}

- - {token && ( -
-

{t('cliAuth.fallbackInstructions')}

- - {token} - -
- )} - -
- ) - } - - // Error state return (
-
- - - +
+
-

{t('cliAuth.error')}

-

{errorMessage}

+

{t('cliAuth.legacyDisabledTitle')}

+

{t('cliAuth.legacyDisabledDescription')}

- {user === null && ( -
-

- {t('cliAuth.loginRequired')} -

- -
- )} +
) diff --git a/web/src/pages/device.test.ts b/web/src/pages/device.test.ts index 39585f83..1e599a6c 100644 --- a/web/src/pages/device.test.ts +++ b/web/src/pages/device.test.ts @@ -1,4 +1,15 @@ -import { describe, expect, it, vi } from 'vitest' +/** @vitest-environment jsdom */ +import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react' +import { createElement } from 'react' +import { afterEach, describe, expect, it, vi } from 'vitest' + +const fetchJsonMock = vi.hoisted(() => vi.fn()) +const getCsrfHeadersMock = vi.hoisted(() => + vi.fn((headers: Record) => ({ + ...headers, + 'X-CSRF-TOKEN': 'csrf-test', + })), +) vi.mock('react-i18next', async () => { const actual = await vi.importActual('react-i18next') @@ -10,25 +21,42 @@ vi.mock('react-i18next', async () => { } }) -vi.mock('@/shared/ui/card', () => ({ - Card: ({ children }: { children: unknown }) => children, -})) +vi.mock('@/shared/ui/card', async () => { + const React = await vi.importActual('react') + return { + Card: ({ children, ...props }: import('react').HTMLAttributes) => + React.createElement('div', props, children), + } +}) -vi.mock('@/shared/ui/button', () => ({ - Button: ({ children }: { children: unknown }) => children, -})) +vi.mock('@/shared/ui/button', async () => { + const React = await vi.importActual('react') + return { + Button: ({ children, ...props }: import('react').ButtonHTMLAttributes) => + React.createElement('button', props, children), + } +}) -vi.mock('@/shared/ui/input', () => ({ - Input: () => null, -})) +vi.mock('@/shared/ui/input', async () => { + const React = await vi.importActual('react') + return { + Input: React.forwardRef>((props, ref) => + React.createElement('input', { ...props, ref }), + ), + } +}) -vi.mock('@/shared/ui/label', () => ({ - Label: ({ children }: { children: unknown }) => children, -})) +vi.mock('@/shared/ui/label', async () => { + const React = await vi.importActual('react') + return { + Label: ({ children, ...props }: import('react').LabelHTMLAttributes) => + React.createElement('label', props, children), + } +}) vi.mock('@/api/client', () => ({ - fetchJson: vi.fn(), - getCsrfHeaders: () => ({}), + fetchJson: fetchJsonMock, + getCsrfHeaders: getCsrfHeadersMock, })) vi.mock('@/shared/lib/error-display', () => ({ @@ -37,9 +65,51 @@ vi.mock('@/shared/lib/error-display', () => ({ import { DeviceAuthPage } from './device' +afterEach(() => { + cleanup() + vi.clearAllMocks() +}) + describe('DeviceAuthPage', () => { it('exports a named component function', () => { expect(typeof DeviceAuthPage).toBe('function') expect(DeviceAuthPage.name).toBe('DeviceAuthPage') }) + + it('normalizes the user code and submits it with CSRF headers', async () => { + fetchJsonMock.mockResolvedValueOnce(undefined) + + render(createElement(DeviceAuthPage)) + + const [part1Input, part2Input] = screen.getAllByPlaceholderText('XXXX') + fireEvent.change(part1Input, { target: { value: 'ab12zz' } }) + fireEvent.change(part2Input, { target: { value: 'cd34' } }) + fireEvent.click(screen.getByRole('button', { name: 'device.submit' })) + + await waitFor(() => { + expect(fetchJsonMock).toHaveBeenCalledWith('/api/v1/device/authorize', { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'X-CSRF-TOKEN': 'csrf-test', + }, + body: JSON.stringify({ userCode: 'AB12-CD34' }), + }) + }) + expect(getCsrfHeadersMock).toHaveBeenCalledWith({ 'Content-Type': 'application/json' }) + expect(await screen.findByText('device.success')).toBeTruthy() + }) + + it('renders the authorization error when submission fails', async () => { + fetchJsonMock.mockRejectedValueOnce(new Error('Invalid device code')) + + render(createElement(DeviceAuthPage)) + + const [part1Input, part2Input] = screen.getAllByPlaceholderText('XXXX') + fireEvent.change(part1Input, { target: { value: 'WXYZ' } }) + fireEvent.change(part2Input, { target: { value: '1234' } }) + fireEvent.click(screen.getByRole('button', { name: 'device.submit' })) + + expect(await screen.findByText('Invalid device code')).toBeTruthy() + }) })