From 40d7de846289b61a31c305914760329402a04b1d Mon Sep 17 00:00:00 2001 From: dongmucat <1127093059@qq.com> Date: Thu, 11 Jun 2026 18:42:31 +0800 Subject: [PATCH 1/3] fix(security): harden review findings Signed-off-by: dongmucat <1127093059@qq.com> --- .env.release.draft | 3 + .env.release.example | 3 + .github/workflows/pr-scripts.yml | 8 +- .github/workflows/security.yml | 83 +++++++++ Makefile | 7 +- cli/src/platform/archive.ts | 84 ++++++++- cli/src/platform/download.ts | 56 ++++++ cli/src/services/install-service.ts | 3 +- cli/test/unit/platform/archive.test.ts | 28 +++ .../unit/services/install-service.test.ts | 26 ++- compose.release.yml | 3 + docker-compose.staging.yml | 6 + scanner/docs/configuration.md | 22 +-- scripts/smoke-test.sh | 13 +- scripts/tests/dev-web-host-test.sh | 25 +++ scripts/tests/validate-release-config-test.sh | 83 +++++++++ scripts/tests/workflow-security-test.sh | 33 ++++ scripts/validate-release-config.sh | 16 ++ .../config/DownloadRateLimitProperties.java | 2 +- .../config/SkillScannerProperties.java | 2 +- .../portal/NamespaceController.java | 9 +- .../AnonymousDownloadIdentityService.java | 23 +++ .../NamespacePortalQueryAppService.java | 13 +- .../src/main/resources/application-local.yml | 7 +- .../src/main/resources/application.yml | 4 +- .../src/main/resources/messages.properties | 2 + .../src/main/resources/messages_zh.properties | 2 + .../DownloadRateLimitPropertiesTest.java | 16 ++ .../SkillScannerPropertiesBindingTest.java | 18 +- .../controller/DeviceAuthControllerTest.java | 58 ++++++ .../controller/LocalAuthControllerTest.java | 56 ++++++ .../NamespacePortalControllerTest.java | 35 ++++ .../NamespaceWorkflowContractTest.java | 2 +- .../controller/SkillStarControllerTest.java | 5 +- .../controller/cli/CliDryRunValidateTest.java | 4 + .../cli/CliSkillControllerTest.java | 1 + .../SkillSubscriptionControllerTest.java | 6 +- .../AnonymousDownloadIdentityServiceTest.java | 20 +++ .../NamespacePortalQueryAppServiceTest.java | 18 +- .../src/test/resources/application-test.yml | 3 + .../skillhub/auth/config/SecurityConfig.java | 20 ++- .../policy/RouteSecurityPolicyRegistry.java | 14 +- .../auth/config/SecurityConfigTest.java | 43 +++++ .../RouteSecurityPolicyRegistryTest.java | 14 +- .../skill/service/SkillPublishService.java | 10 ++ .../service/SkillPublishServiceTest.java | 166 +++++++++++++++++- web/e2e/helpers/csrf.ts | 27 +++ web/e2e/helpers/session.ts | 20 ++- web/e2e/helpers/test-data-builder.ts | 22 ++- web/e2e/public-skill-detail-anonymous.spec.ts | 2 + web/e2e/skill-subscription.spec.ts | 2 + web/e2e/skill-version-compare.spec.ts | 4 + web/src/api/generated/schema.d.ts | 4 + 53 files changed, 1094 insertions(+), 62 deletions(-) create mode 100644 .github/workflows/security.yml create mode 100644 cli/src/platform/download.ts create mode 100755 scripts/tests/dev-web-host-test.sh create mode 100755 scripts/tests/validate-release-config-test.sh create mode 100755 scripts/tests/workflow-security-test.sh create mode 100644 server/skillhub-app/src/test/java/com/iflytek/skillhub/config/DownloadRateLimitPropertiesTest.java create mode 100644 server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/DeviceAuthControllerTest.java create mode 100644 server/skillhub-app/src/test/java/com/iflytek/skillhub/ratelimit/AnonymousDownloadIdentityServiceTest.java create mode 100644 server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/config/SecurityConfigTest.java create mode 100644 web/e2e/helpers/csrf.ts diff --git a/.env.release.draft b/.env.release.draft index 400582667..c8f0872bc 100644 --- a/.env.release.draft +++ b/.env.release.draft @@ -93,3 +93,6 @@ SPRING_MAIL_PROPERTIES_MAIL_SMTP_SSL_TRUST= SKILLHUB_AUTH_PASSWORD_RESET_CODE_EXPIRY=PT10M SKILLHUB_AUTH_PASSWORD_RESET_FROM_ADDRESS=noreply@example.com SKILLHUB_AUTH_PASSWORD_RESET_FROM_NAME=SkillHub + +# Required for signing anonymous download rate-limit cookies. Use a unique random value per deployment. +SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET=replace-with-random-download-secret-32-bytes diff --git a/.env.release.example b/.env.release.example index cdbddf061..40e51ac10 100644 --- a/.env.release.example +++ b/.env.release.example @@ -104,6 +104,9 @@ SKILLHUB_AUTH_PASSWORD_RESET_FROM_NAME=SkillHub # Security scanner is enabled by default. Set to false to disable scanning. SKILLHUB_SECURITY_SCANNER_ENABLED=true +# Required for signing anonymous download rate-limit cookies. Use a unique random value per deployment. +SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET=replace-with-random-download-secret-32-bytes + # Scanner LLM configuration (optional, for AI-powered scanning features) SKILL_SCANNER_LLM_API_KEY= SKILL_SCANNER_LLM_BASE_URL= diff --git a/.github/workflows/pr-scripts.yml b/.github/workflows/pr-scripts.yml index eb7809d11..2f8cc80d8 100644 --- a/.github/workflows/pr-scripts.yml +++ b/.github/workflows/pr-scripts.yml @@ -4,10 +4,13 @@ on: pull_request: paths: - 'scripts/**' + - 'Makefile' + - '.github/workflows/security.yml' - '.github/workflows/pr-scripts.yml' jobs: - publish-cli-test: + scripts-tests: + name: Script Regression Tests runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 @@ -17,3 +20,6 @@ jobs: with: node-version: '21' - run: bash scripts/tests/publish-cli-test.sh + - run: bash scripts/tests/validate-release-config-test.sh + - run: bash scripts/tests/dev-web-host-test.sh + - run: bash scripts/tests/workflow-security-test.sh diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml new file mode 100644 index 000000000..9b476ca96 --- /dev/null +++ b/.github/workflows/security.yml @@ -0,0 +1,83 @@ +name: Security + +on: + pull_request: + types: + - opened + - synchronize + - reopened + - ready_for_review + push: + branches: + - main + schedule: + - cron: '23 3 * * 1' + workflow_dispatch: + +permissions: + contents: read + +jobs: + dependency-review: + name: Dependency Review + if: ${{ github.event_name == 'pull_request' && !github.event.pull_request.draft }} + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: read + steps: + - name: Check out repository + uses: actions/checkout@v4 + + - name: Review dependency changes + uses: actions/dependency-review-action@v4 + + codeql: + name: CodeQL (${{ matrix.language }}) + if: ${{ github.event_name != 'pull_request' }} + runs-on: ubuntu-latest + permissions: + contents: read + security-events: write + + strategy: + fail-fast: false + matrix: + include: + - language: java-kotlin + build-mode: manual + - language: javascript-typescript + build-mode: none + - language: python + build-mode: none + + steps: + - name: Check out repository + uses: actions/checkout@v4 + + - name: Set up Java + if: matrix.language == 'java-kotlin' + uses: actions/setup-java@v4 + with: + distribution: temurin + java-version: 21 + cache: maven + + - name: Ensure Maven wrapper is executable + if: matrix.language == 'java-kotlin' + run: chmod +x server/mvnw + + - name: Initialize CodeQL + uses: github/codeql-action/init@v3 + with: + languages: ${{ matrix.language }} + build-mode: ${{ matrix.build-mode }} + + - name: Build Java for CodeQL + if: matrix.language == 'java-kotlin' + run: cd server && ./mvnw -q -DskipTests package + + - name: Analyze + uses: github/codeql-action/analyze@v3 + with: + category: /language:${{ matrix.language }} diff --git a/Makefile b/Makefile index 039f213d2..bf3e74391 100644 --- a/Makefile +++ b/Makefile @@ -6,6 +6,7 @@ DEV_WEB_PID := $(DEV_DIR)/web.pid DEV_SERVER_LOG := $(DEV_DIR)/server.log DEV_WEB_LOG := $(DEV_DIR)/web.log DEV_WEB_URL := http://localhost:3000 +DEV_WEB_HOST ?= 127.0.0.1 DEV_API_URL := http://localhost:8080 DEV_SCANNER_URL := http://localhost:8000 STAGING_API_URL := http://localhost:8080 @@ -48,7 +49,7 @@ dev-all: ## 一键启动本地开发环境（依赖 + scanner + 后端 + 前端 echo "Frontend already running with PID $$(cat $(DEV_WEB_PID))"; \ else \ echo "Starting frontend..."; \ - $(DEV_PROCESS) start --pid-file $(DEV_WEB_PID) --log-file $(DEV_WEB_LOG) --cwd web -- pnpm exec vite --host 0.0.0.0 --strictPort >/dev/null; \ + $(DEV_PROCESS) start --pid-file $(DEV_WEB_PID) --log-file $(DEV_WEB_LOG) --cwd web -- pnpm exec vite --host $(DEV_WEB_HOST) --strictPort >/dev/null; \ fi @echo "Waiting for backend on $(DEV_API_URL) ..." @backend_ready=0; \ @@ -126,7 +127,7 @@ dev-all: ## 一键启动本地开发环境（依赖 + scanner + 后端 + 前端 @echo " Frontend: $(DEV_WEB_LOG)" dev-server: ## 启动后端开发服务器 - cd server && /bin/sh -lc '$(DEV_SERVER_PREPARE) && exec $(DEV_SERVER_CMD)' + cd server && /bin/sh -lc '$(DEV_SERVER_PREPARE) && exec env $(DEV_SERVER_SCANNER_ENV) $(DEV_SERVER_CMD)' dev-server-restart: ## 重启后端开发服务器 @mkdir -p $(DEV_DIR) @@ -237,7 +238,7 @@ web-install-ci: ## 以 CI 方式安装前端依赖 cd web && CI=true pnpm install --frozen-lockfile dev-web: ## 启动前端开发服务器 - cd web && pnpm run dev + cd web && pnpm exec vite --host $(DEV_WEB_HOST) build-frontend: web-deps ## 构建前端 cd web && pnpm run build diff --git a/cli/src/platform/archive.ts b/cli/src/platform/archive.ts index 1e4205674..c13067118 100644 --- a/cli/src/platform/archive.ts +++ b/cli/src/platform/archive.ts @@ -1,6 +1,14 @@ import { mkdir, readdir, readFile, writeFile } from 'node:fs/promises' import { dirname, isAbsolute, join, relative, resolve } from 'node:path' import { zipSync, unzipSync } from 'fflate' +import { MAX_PACKAGE_BYTES } from './download' + +const MAX_ZIP_ENTRIES = 500 +const MAX_SINGLE_FILE_BYTES = 10 * 1024 * 1024 +const EOCD_SIGNATURE = 0x06054b50 +const CENTRAL_DIRECTORY_SIGNATURE = 0x02014b50 +const ZIP64_MARKER_16 = 0xffff +const ZIP64_MARKER_32 = 0xffffffff /** * Extract a zip archive buffer into the target directory. @@ -8,7 +16,9 @@ import { zipSync, unzipSync } from 'fflate' */ export async function extractZip(buffer: ArrayBuffer, targetDir: string): Promise { await mkdir(targetDir, { recursive: true }) - const files = unzipSync(new Uint8Array(buffer)) + const archive = new Uint8Array(buffer) + validateZipCentralDirectory(archive) + const files = unzipSync(archive) for (const [name, data] of Object.entries(files)) { const filePath = safeJoin(targetDir, name) if (name.endsWith('/')) { @@ -20,6 +30,78 @@ export async function extractZip(buffer: ArrayBuffer, targetDir: string): Promis } } +function validateZipCentralDirectory(archive: Uint8Array): void { + const view = new DataView(archive.buffer, archive.byteOffset, archive.byteLength) + const eocdOffset = findEndOfCentralDirectory(view) + if (eocdOffset < 0) { + throw new Error('invalid zip central directory') + } + + const diskNumber = view.getUint16(eocdOffset + 4, true) + const centralDirectoryDisk = view.getUint16(eocdOffset + 6, true) + const entriesOnDisk = view.getUint16(eocdOffset + 8, true) + const totalEntries = view.getUint16(eocdOffset + 10, true) + const centralDirectorySize = view.getUint32(eocdOffset + 12, true) + const centralDirectoryOffset = view.getUint32(eocdOffset + 16, true) + + if ( + entriesOnDisk === ZIP64_MARKER_16 || + totalEntries === ZIP64_MARKER_16 || + centralDirectorySize === ZIP64_MARKER_32 || + centralDirectoryOffset === ZIP64_MARKER_32 + ) { + throw new Error('zip64 archives are not supported') + } + if (diskNumber !== 0 || centralDirectoryDisk !== 0 || entriesOnDisk !== totalEntries) { + throw new Error('multi-disk zip archives are not supported') + } + if (totalEntries > MAX_ZIP_ENTRIES) { + throw new Error('zip entry count exceeds limit') + } + if (centralDirectoryOffset + centralDirectorySize > archive.byteLength) { + throw new Error('invalid zip central directory') + } + + let offset = centralDirectoryOffset + let totalUncompressedSize = 0 + const decoder = new TextDecoder() + for (let i = 0; i < totalEntries; i++) { + if (offset + 46 > archive.byteLength || view.getUint32(offset, true) !== CENTRAL_DIRECTORY_SIGNATURE) { + throw new Error('invalid zip central directory') + } + const uncompressedSize = view.getUint32(offset + 24, true) + const nameLength = view.getUint16(offset + 28, true) + const extraLength = view.getUint16(offset + 30, true) + const commentLength = view.getUint16(offset + 32, true) + const nameStart = offset + 46 + const nameEnd = nameStart + nameLength + const nextOffset = nameEnd + extraLength + commentLength + if (nameEnd > archive.byteLength || nextOffset > archive.byteLength) { + throw new Error('invalid zip central directory') + } + + const entryName = decoder.decode(archive.subarray(nameStart, nameEnd)) + if (!entryName.endsWith('/') && uncompressedSize > MAX_SINGLE_FILE_BYTES) { + throw new Error('zip entry size exceeds limit') + } + totalUncompressedSize += uncompressedSize + if (totalUncompressedSize > MAX_PACKAGE_BYTES) { + throw new Error('zip total uncompressed size exceeds limit') + } + offset = nextOffset + } +} + +function findEndOfCentralDirectory(view: DataView): number { + const minOffset = Math.max(0, view.byteLength - 0xffff - 22) + for (let offset = view.byteLength - 22; offset >= minOffset; offset--) { + if (view.getUint32(offset, true) === EOCD_SIGNATURE) { + return offset + } + } + return -1 +} + /** * Create a zip archive from a directory. * Returns the archive as a Blob. diff --git a/cli/src/platform/download.ts b/cli/src/platform/download.ts new file mode 100644 index 000000000..30490731a --- /dev/null +++ b/cli/src/platform/download.ts @@ -0,0 +1,56 @@ +import { EXIT } from '../shared/constants' +import { CliError } from '../shared/errors' + +export const MAX_PACKAGE_BYTES = 100 * 1024 * 1024 + +export async function readBoundedResponseBody(response: Response, maxBytes = MAX_PACKAGE_BYTES): Promise { + const contentLength = response.headers.get('content-length') + if (contentLength !== null) { + const declaredLength = Number(contentLength) + if (Number.isFinite(declaredLength) && declaredLength > maxBytes) { + throw new CliError('download exceeds maximum package size', EXIT.network, { + contentLength: declaredLength, + maxBytes + }) + } + } + + if (!response.body) { + const buffer = await response.arrayBuffer() + if (buffer.byteLength > maxBytes) { + throw new CliError('download exceeds maximum package size', EXIT.network, { + receivedBytes: buffer.byteLength, + maxBytes + }) + } + return buffer + } + + const reader = response.body.getReader() + const chunks: Uint8Array[] = [] + let receivedBytes = 0 + + for (;;) { + const { done, value } = await reader.read() + if (done) { + break + } + receivedBytes += value.byteLength + if (receivedBytes > maxBytes) { + await reader.cancel() + throw new CliError('download exceeds maximum package size', EXIT.network, { + receivedBytes, + maxBytes + }) + } + chunks.push(value) + } + + const result = new Uint8Array(receivedBytes) + let offset = 0 + for (const chunk of chunks) { + result.set(chunk, offset) + offset += chunk.byteLength + } + return result.buffer +} diff --git a/cli/src/services/install-service.ts b/cli/src/services/install-service.ts index b989ce581..ed09ffd69 100644 --- a/cli/src/services/install-service.ts +++ b/cli/src/services/install-service.ts @@ -5,6 +5,7 @@ import { InventoryStore } from '../stores/inventory-store' import { CliError } from '../shared/errors' import { EXIT } from '../shared/constants' import { extractZip } from '../platform/archive' +import { readBoundedResponseBody } from '../platform/download' import { pathExists } from '../platform/paths' import type { AgentCandidate } from '../agents/types' @@ -23,7 +24,7 @@ export async function installSkill(options: InstallOptions): Promise<{ installed const client = new SkillHubClient(options.registry, options.token) const resolved = await client.resolve(options.namespace, options.slug, options.version) const response = await client.download(options.namespace, options.slug, resolved.version) - const buffer = await response.arrayBuffer() + const buffer = await readBoundedResponseBody(response) const installed: Array<{ agent: string; dir: string }> = [] const store = new InventoryStore(options.home) diff --git a/cli/test/unit/platform/archive.test.ts b/cli/test/unit/platform/archive.test.ts index a39c3c03c..98972134e 100644 --- a/cli/test/unit/platform/archive.test.ts +++ b/cli/test/unit/platform/archive.test.ts @@ -56,4 +56,32 @@ describe('archive helpers', () => { const entries = await readdir(target) expect(entries).toEqual([]) }) + + test('rejects zip archives with more than 500 entries before extraction', async () => { + const target = await mkdtemp(join(tmpdir(), 'skillhub-archive-many-')) + const manyEntries = Object.fromEntries( + Array.from({ length: 501 }, (_, index) => [`file-${index}.txt`, new Uint8Array(0)]) + ) + const archive = zipSync(manyEntries) + + await expect(extractZip(archive.buffer as ArrayBuffer, target)).rejects.toThrow('zip entry count exceeds limit') + }) + + test('rejects zip entries larger than 10 MiB before extraction', async () => { + const target = await mkdtemp(join(tmpdir(), 'skillhub-archive-large-file-')) + const archive = zipSync({ 'large.bin': new Uint8Array(10 * 1024 * 1024 + 1) }) + + await expect(extractZip(archive.buffer as ArrayBuffer, target)).rejects.toThrow('zip entry size exceeds limit') + }) + + test('rejects zip archives larger than 100 MiB after decompression before extraction', async () => { + const target = await mkdtemp(join(tmpdir(), 'skillhub-archive-large-total-')) + const oneMiB = new Uint8Array(1024 * 1024) + const entries = Object.fromEntries( + Array.from({ length: 101 }, (_, index) => [`file-${index}.bin`, oneMiB]) + ) + const archive = zipSync(entries) + + await expect(extractZip(archive.buffer as ArrayBuffer, target)).rejects.toThrow('zip total uncompressed size exceeds limit') + }) }) diff --git a/cli/test/unit/services/install-service.test.ts b/cli/test/unit/services/install-service.test.ts index 2ea2735d6..784f4394f 100644 --- a/cli/test/unit/services/install-service.test.ts +++ b/cli/test/unit/services/install-service.test.ts @@ -21,6 +21,13 @@ function installFetch(zipEntries: Record): typeof fetch { Object.entries(zipEntries).map(([name, content]) => [name, new TextEncoder().encode(content)]) )) + return installFetchWithDownloadResponse(new Response( + archive.buffer.slice(archive.byteOffset, archive.byteOffset + archive.byteLength) as ArrayBuffer, + { status: 200 } + )) +} + +function installFetchWithDownloadResponse(downloadResponse: Response): typeof fetch { const fakeFetch = async (input: URL | RequestInfo) => { const path = new URL(String(input)).pathname if (path.endsWith('/resolve')) { @@ -37,8 +44,7 @@ function installFetch(zipEntries: Record): typeof fetch { }) } if (path.endsWith('/download')) { - const body = archive.buffer.slice(archive.byteOffset, archive.byteOffset + archive.byteLength) as ArrayBuffer - return new Response(body, { status: 200 }) + return downloadResponse.clone() } return Response.json({ code: 404 }, { status: 404 }) } @@ -125,4 +131,20 @@ describe('installSkill', () => { expect(inventory.items[0].targets).toHaveLength(1) expect(inventory.items[0].targets[0].installDir).toBe(skillDir) }) + + test('rejects downloads whose content-length exceeds the package limit', async () => { + globalThis.fetch = installFetchWithDownloadResponse(new Response(new Uint8Array(0), { + status: 200, + headers: { 'Content-Length': String(100 * 1024 * 1024 + 1) } + })) + const rootDir = await mkdtemp(join(tmpdir(), 'skillhub-install-root-')) + + await expect(installSkill({ + registry: 'http://registry.test', + namespace: 'global', + slug: 'demo', + targets: [{ agent: 'codex', rootDir, scope: 'project', source: 'explicit' }], + force: false + })).rejects.toThrow('download exceeds maximum package size') + }) }) diff --git a/compose.release.yml b/compose.release.yml index 1b8787317..5ed7086e9 100644 --- a/compose.release.yml +++ b/compose.release.yml @@ -58,6 +58,7 @@ services: SESSION_COOKIE_SECURE: ${SESSION_COOKIE_SECURE:-false} SKILLHUB_PUBLIC_BASE_URL: ${SKILLHUB_PUBLIC_BASE_URL:-} DEVICE_AUTH_VERIFICATION_URI: ${DEVICE_AUTH_VERIFICATION_URI:-} + SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET: ${SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET:?required} SKILLHUB_STORAGE_PROVIDER: ${SKILLHUB_STORAGE_PROVIDER:-s3} STORAGE_BASE_PATH: /var/lib/skillhub/storage SKILLHUB_STORAGE_S3_ENDPOINT: ${SKILLHUB_STORAGE_S3_ENDPOINT:-} @@ -99,6 +100,8 @@ services: condition: service_healthy redis: condition: service_healthy + skill-scanner: + condition: service_healthy healthcheck: test: ["CMD", "wget", "-qO-", "http://localhost:8080/actuator/health"] interval: 10s diff --git a/docker-compose.staging.yml b/docker-compose.staging.yml index a6c5e2ff6..48d1fd1ad 100644 --- a/docker-compose.staging.yml +++ b/docker-compose.staging.yml @@ -42,11 +42,17 @@ services: BOOTSTRAP_ADMIN_EMAIL: admin@skillhub.local OAUTH2_GITHUB_CLIENT_ID: local-placeholder OAUTH2_GITHUB_CLIENT_SECRET: local-placeholder + SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET: staging-download-secret-32-bytes + SKILLHUB_SECURITY_SCANNER_ENABLED: "true" + SKILLHUB_SECURITY_SCANNER_URL: http://skill-scanner:8000 + SKILLHUB_SECURITY_SCANNER_MODE: upload depends_on: postgres: condition: service_healthy redis: condition: service_healthy + skill-scanner: + condition: service_healthy healthcheck: test: ["CMD", "wget", "-qO-", "http://localhost:8080/actuator/health"] interval: 10s diff --git a/scanner/docs/configuration.md b/scanner/docs/configuration.md index 1de8606a4..2544aed2e 100644 --- a/scanner/docs/configuration.md +++ b/scanner/docs/configuration.md @@ -18,7 +18,7 @@ skillhub: security: scanner: # 基础配置 - enabled: ${SKILLHUB_SECURITY_SCANNER_ENABLED:false} + enabled: ${SKILLHUB_SECURITY_SCANNER_ENABLED:true} base-url: ${SKILLHUB_SECURITY_SCANNER_URL:http://localhost:8000} mode: ${SKILLHUB_SECURITY_SCANNER_MODE:local} @@ -48,20 +48,20 @@ skillhub: #### `enabled` - **类型**：Boolean -- **默认值**：`false` +- **默认值**：`true` - **环境变量**：`SKILLHUB_SECURITY_SCANNER_ENABLED` - **说明**：是否启用安全扫描功能 - **影响**： - `true`：技能包发布时会触发安全扫描 - - `false`：跳过安全扫描，直接进入审核流程 + - `false`：仅允许 `PRIVATE` 技能跳过扫描；`PUBLIC` / `NAMESPACE_ONLY` 发布会失败并提示必须启用 Scanner **示例**： ```yaml -# 开发环境：禁用扫描 +# 仅本地私有技能调试：禁用扫描 enabled: false -# 生产环境：启用扫描 +# 公共或命名空间可见发布：启用扫描 enabled: true ``` @@ -103,8 +103,8 @@ base-url: https://scanner.example.com **示例**： ```yaml -# Docker Compose 环境（共享卷） -mode: local +# Docker Compose 环境（Scanner 独立容器，无共享发布目录） +mode: upload # Kubernetes 环境（独立 Pod） mode: upload @@ -298,7 +298,7 @@ services: environment: - SKILLHUB_SECURITY_SCANNER_ENABLED=true - SKILLHUB_SECURITY_SCANNER_URL=http://skill-scanner:8000 - - SKILLHUB_SECURITY_SCANNER_MODE=local + - SKILLHUB_SECURITY_SCANNER_MODE=upload - SKILLHUB_SCANNER_USE_BEHAVIORAL=false - SKILLHUB_SCANNER_USE_LLM=false - SKILLHUB_SCANNER_USE_META=true @@ -348,9 +348,9 @@ stringData: skillhub: security: scanner: - enabled: false # 开发时禁用扫描，加快迭代速度 + enabled: true # 默认启用；PUBLIC/NAMESPACE_ONLY 发布依赖扫描 base-url: http://localhost:8000 - mode: local + mode: upload analyzers: meta: true # 只启用元数据分析 policy: @@ -366,7 +366,7 @@ skillhub: scanner: enabled: true # 测试环境启用扫描 base-url: http://skill-scanner:8000 - mode: local + mode: upload analyzers: behavioral: true meta: true diff --git a/scripts/smoke-test.sh b/scripts/smoke-test.sh index 35389ece9..e1e462894 100755 --- a/scripts/smoke-test.sh +++ b/scripts/smoke-test.sh @@ -36,8 +36,8 @@ echo "Target: $BASE_URL" echo check "Health endpoint" "$BASE_URL/actuator/health" "200" -check "Prometheus metrics" "$BASE_URL/actuator/prometheus" "200" -check "Namespaces API" "$BASE_URL/api/v1/namespaces" "200" +check "Prometheus metrics requires auth" "$BASE_URL/actuator/prometheus" "401" +check "Namespaces API requires auth" "$BASE_URL/api/v1/namespaces" "401" check "Auth required" "$BASE_URL/api/v1/auth/me" "401" curl -s -c "$COOKIE_JAR" "$BASE_URL/api/v1/auth/me" >/dev/null @@ -67,6 +67,15 @@ else FAIL=$((FAIL + 1)) fi +NAMESPACES_AUTH_STATUS="$(curl --max-time 10 -s -o /dev/null -w "%{http_code}" -b "$COOKIE_JAR" "$BASE_URL/api/v1/namespaces" || true)" +if [[ "$NAMESPACES_AUTH_STATUS" == "200" ]]; then + echo "PASS: Namespaces API with session (HTTP $NAMESPACES_AUTH_STATUS)" + PASS=$((PASS + 1)) +else + echo "FAIL: Namespaces API with session (got $NAMESPACES_AUTH_STATUS)" + FAIL=$((FAIL + 1)) +fi + CHANGE_PASSWORD_STATUS="$(curl --max-time 10 -s -o /dev/null -w "%{http_code}" \ -X POST "$BASE_URL/api/v1/auth/local/change-password" \ -b "$COOKIE_JAR" \ diff --git a/scripts/tests/dev-web-host-test.sh b/scripts/tests/dev-web-host-test.sh new file mode 100755 index 000000000..10c22a076 --- /dev/null +++ b/scripts/tests/dev-web-host-test.sh @@ -0,0 +1,25 @@ +#!/usr/bin/env bash +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +MAKEFILE="$REPO_ROOT/Makefile" + +fail() { + echo "FAIL: $*" >&2 + exit 1 +} + +grep -Eq '^DEV_WEB_HOST[[:space:]]*\?=[[:space:]]*127\.0\.0\.1$' "$MAKEFILE" \ + || fail "Makefile must default DEV_WEB_HOST to 127.0.0.1" + +grep -Fq 'pnpm exec vite --host $(DEV_WEB_HOST)' "$MAKEFILE" \ + || fail "dev web startup must pass DEV_WEB_HOST to vite" + +grep -Fq "cd server && /bin/sh -lc '\$(DEV_SERVER_PREPARE) && exec env \$(DEV_SERVER_SCANNER_ENV) \$(DEV_SERVER_CMD)'" "$MAKEFILE" \ + || fail "dev-server must inject scanner upload environment" + +if grep -Fq 'pnpm exec vite --host 0.0.0.0' "$MAKEFILE"; then + fail "dev web startup must not bind Vite to 0.0.0.0 by default" +fi + +echo "dev-web-host-test passed" diff --git a/scripts/tests/validate-release-config-test.sh b/scripts/tests/validate-release-config-test.sh new file mode 100755 index 000000000..3a52d7a3f --- /dev/null +++ b/scripts/tests/validate-release-config-test.sh @@ -0,0 +1,83 @@ +#!/usr/bin/env bash +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +SCRIPT="$REPO_ROOT/scripts/validate-release-config.sh" + +TMP_DIRS=() +cleanup() { + local d + for d in "${TMP_DIRS[@]+"${TMP_DIRS[@]}"}"; do + rm -rf "$d" + done +} +trap cleanup EXIT + +new_tmp() { + local d + d="$(mktemp -d)" + TMP_DIRS+=("$d") + echo "$d" +} + +fail() { + echo "FAIL: $*" >&2 + exit 1 +} + +write_env() { + local file="$1" + local secret="${2:-}" + local include_secret="${3:-yes}" + cat >"$file" <>"$file" + fi +} + +expect_fail() { + local file="$1" + local expected="$2" + local output + if output="$("$SCRIPT" "$file" 2>&1)"; then + fail "expected validation to fail for $file" + fi + if [[ "$output" != *"$expected"* ]]; then + fail "expected output to contain '$expected', got: $output" + fi +} + +tmp="$(new_tmp)" + +valid_env="$tmp/valid.env" +write_env "$valid_env" "release-download-secret-32-bytes-minimum" +"$SCRIPT" "$valid_env" >/dev/null + +missing_env="$tmp/missing.env" +write_env "$missing_env" "" no +expect_fail "$missing_env" "SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET is required" + +placeholder_env="$tmp/placeholder.env" +write_env "$placeholder_env" "change-me-in-production" +expect_fail "$placeholder_env" "SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET still uses placeholder/default value" + +short_env="$tmp/short.env" +write_env "$short_env" "too-short" +expect_fail "$short_env" "SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET must be at least 32 characters" + +echo "validate-release-config-test passed" diff --git a/scripts/tests/workflow-security-test.sh b/scripts/tests/workflow-security-test.sh new file mode 100755 index 000000000..8219af51d --- /dev/null +++ b/scripts/tests/workflow-security-test.sh @@ -0,0 +1,33 @@ +#!/usr/bin/env bash +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +SECURITY_WORKFLOW="$REPO_ROOT/.github/workflows/security.yml" +PR_SCRIPTS_WORKFLOW="$REPO_ROOT/.github/workflows/pr-scripts.yml" + +fail() { + echo "FAIL: $*" >&2 + exit 1 +} + +[[ -f "$SECURITY_WORKFLOW" ]] || fail ".github/workflows/security.yml is required" + +grep -Fq 'actions/dependency-review-action' "$SECURITY_WORKFLOW" \ + || fail "security workflow must run dependency review" +grep -Fq 'github/codeql-action/init' "$SECURITY_WORKFLOW" \ + || fail "security workflow must initialize CodeQL" +grep -Fq 'cd server && ./mvnw -q -DskipTests package' "$SECURITY_WORKFLOW" \ + || fail "security workflow must build Java with the server Maven wrapper" +grep -Fq 'security-events: write' "$SECURITY_WORKFLOW" \ + || fail "security workflow must grant SARIF upload permission" + +grep -Fq '.github/workflows/security.yml' "$PR_SCRIPTS_WORKFLOW" \ + || fail "pr-scripts must run when security workflow changes" +grep -Fq 'bash scripts/tests/validate-release-config-test.sh' "$PR_SCRIPTS_WORKFLOW" \ + || fail "pr-scripts must run validate-release-config-test" +grep -Fq 'bash scripts/tests/dev-web-host-test.sh' "$PR_SCRIPTS_WORKFLOW" \ + || fail "pr-scripts must run dev-web-host-test" +grep -Fq 'bash scripts/tests/workflow-security-test.sh' "$PR_SCRIPTS_WORKFLOW" \ + || fail "pr-scripts must run workflow-security-test" + +echo "workflow-security-test passed" diff --git a/scripts/validate-release-config.sh b/scripts/validate-release-config.sh index 1d4f9285b..758b168ff 100755 --- a/scripts/validate-release-config.sh +++ b/scripts/validate-release-config.sh @@ -97,10 +97,26 @@ validate_port() { esac } +validate_min_length() { + var_name="$1" + min_length="$2" + eval "var_value=\${$var_name:-}" + if [ -z "$var_value" ]; then + return 0 + fi + if [ "${#var_value}" -lt "$min_length" ]; then + error "$var_name must be at least $min_length characters" + fi +} + require_non_empty SKILLHUB_PUBLIC_BASE_URL validate_url SKILLHUB_PUBLIC_BASE_URL validate_no_trailing_slash SKILLHUB_PUBLIC_BASE_URL +require_non_empty SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET +reject_values SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET "change-me-in-production" "replace-me" "replace-with-random-download-secret-32-bytes" +validate_min_length SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET 32 + reject_values POSTGRES_PASSWORD "change-this-postgres-password" "skillhub_demo" "skillhub_dev" reject_values BOOTSTRAP_ADMIN_PASSWORD "replace-this-admin-password" "ChangeMe!2026" "Admin@2026" if [ "${BOOTSTRAP_ADMIN_ENABLED:-false}" = "true" ]; then diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/DownloadRateLimitProperties.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/DownloadRateLimitProperties.java index 3d922e903..2b1930fa7 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/DownloadRateLimitProperties.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/DownloadRateLimitProperties.java @@ -10,7 +10,7 @@ public class DownloadRateLimitProperties { private String anonymousCookieName = "skillhub_anon_dl"; private Duration anonymousCookieMaxAge = Duration.ofDays(30); - private String anonymousCookieSecret = "change-me-in-production"; + private String anonymousCookieSecret; public String getAnonymousCookieName() { return anonymousCookieName; diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/SkillScannerProperties.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/SkillScannerProperties.java index 7ab3b55ad..bb1f0f879 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/SkillScannerProperties.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/SkillScannerProperties.java @@ -7,7 +7,7 @@ @ConfigurationProperties(prefix = "skillhub.security.scanner") public class SkillScannerProperties { - private boolean enabled = false; + private boolean enabled = true; private String baseUrl = "http://localhost:8000"; private String healthPath = "/health"; private String scanPath = "/scan-upload"; diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/NamespaceController.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/NamespaceController.java index 69be5fa3e..c0c345678 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/NamespaceController.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/NamespaceController.java @@ -31,6 +31,7 @@ import java.util.List; import java.util.Map; +import java.util.Set; /** * Namespace portal endpoints for discovery, membership management, and @@ -155,9 +156,13 @@ public ApiResponse restoreNamespace(@PathVariable String slug @GetMapping("/namespaces/{slug}/members") public ApiResponse> listMembers(@PathVariable String slug, Pageable pageable, - @RequestAttribute("userId") String userId) { + @RequestAttribute("userId") String userId, + @AuthenticationPrincipal PlatformPrincipal principal) { + Set platformRoles = principal != null && principal.platformRoles() != null + ? principal.platformRoles() + : Set.of(); return ok("response.success.read", - namespacePortalQueryAppService.listMembers(slug, pageable, userId)); + namespacePortalQueryAppService.listMembers(slug, pageable, userId, platformRoles)); } @GetMapping("/namespaces/{slug}/member-candidates") diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/ratelimit/AnonymousDownloadIdentityService.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/ratelimit/AnonymousDownloadIdentityService.java index f6ff6dffe..a166c6660 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/ratelimit/AnonymousDownloadIdentityService.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/ratelimit/AnonymousDownloadIdentityService.java @@ -1,6 +1,7 @@ package com.iflytek.skillhub.ratelimit; import com.iflytek.skillhub.config.DownloadRateLimitProperties; +import jakarta.annotation.PostConstruct; import jakarta.servlet.http.Cookie; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; @@ -11,6 +12,7 @@ import java.time.Duration; import java.util.Arrays; import java.util.Base64; +import java.util.Set; import javax.crypto.Mac; import javax.crypto.spec.SecretKeySpec; import org.springframework.http.ResponseCookie; @@ -24,6 +26,12 @@ public class AnonymousDownloadIdentityService { private static final String COOKIE_VERSION = "v1"; + private static final int MIN_SECRET_LENGTH = 32; + private static final Set DISALLOWED_SECRET_VALUES = Set.of( + "change-me-in-production", + "replace-me", + "replace-with-random-download-secret-32-bytes" + ); private static final SecureRandom RANDOM = new SecureRandom(); private final DownloadRateLimitProperties properties; @@ -35,6 +43,21 @@ public AnonymousDownloadIdentityService(DownloadRateLimitProperties properties, this.clientIpResolver = clientIpResolver; } + @PostConstruct + void validateAnonymousCookieSecret() { + String secret = properties.getAnonymousCookieSecret(); + if (secret == null || secret.isBlank()) { + throw new IllegalStateException("SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET is required"); + } + String trimmedSecret = secret.trim(); + if (DISALLOWED_SECRET_VALUES.contains(trimmedSecret)) { + throw new IllegalStateException("SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET must not use the default placeholder"); + } + if (trimmedSecret.length() < MIN_SECRET_LENGTH) { + throw new IllegalStateException("SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET must be at least 32 characters"); + } + } + public AnonymousDownloadIdentity resolve(HttpServletRequest request, HttpServletResponse response) { String ip = clientIpResolver.resolve(request); String cookieId = extractValidCookieId(request); diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/NamespacePortalQueryAppService.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/NamespacePortalQueryAppService.java index 82b38610f..e8df0ad94 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/NamespacePortalQueryAppService.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/NamespacePortalQueryAppService.java @@ -8,6 +8,7 @@ import com.iflytek.skillhub.domain.namespace.NamespaceRole; import com.iflytek.skillhub.domain.namespace.NamespaceService; import com.iflytek.skillhub.domain.namespace.NamespaceStatus; +import com.iflytek.skillhub.domain.namespace.NamespaceType; import com.iflytek.skillhub.domain.shared.exception.DomainForbiddenException; import com.iflytek.skillhub.domain.user.UserAccount; import com.iflytek.skillhub.domain.user.UserAccountRepository; @@ -18,6 +19,7 @@ import java.util.Comparator; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; import org.springframework.data.domain.Page; @@ -111,9 +113,16 @@ public NamespaceResponse getNamespace(String slug, String userId, Map listMembers(String slug, Pageable pageable, String userId) { + public PageResponse listMembers(String slug, Pageable pageable, String userId, Set platformRoles) { Namespace namespace = namespaceService.getNamespaceBySlug(slug); - namespaceService.assertMember(namespace.getId(), userId); + if (namespace.getType() == NamespaceType.GLOBAL) { + Set roles = platformRoles != null ? platformRoles : Set.of(); + if (!roles.contains("SUPER_ADMIN") && !roles.contains("USER_ADMIN")) { + throw new DomainForbiddenException("error.namespace.global.members.platformAdmin.required"); + } + } else { + namespaceService.assertMember(namespace.getId(), userId); + } Page members = namespaceMemberService.listMembers(namespace.getId(), pageable); List memberUserIds = members.getContent().stream() diff --git a/server/skillhub-app/src/main/resources/application-local.yml b/server/skillhub-app/src/main/resources/application-local.yml index 87dd24194..0e390aa55 100644 --- a/server/skillhub-app/src/main/resources/application-local.yml +++ b/server/skillhub-app/src/main/resources/application-local.yml @@ -27,13 +27,18 @@ skillhub: auth: mock: enabled: true + ratelimit: + download: + anonymous-cookie-secret: ${SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET:local-dev-download-secret-32-bytes} notification: cleanup: read-retention-days: 30 unread-retention-days: 90 security: scanner: - enabled: ${SKILLHUB_SECURITY_SCANNER_ENABLED:false} + enabled: ${SKILLHUB_SECURITY_SCANNER_ENABLED:true} + base-url: ${SKILLHUB_SECURITY_SCANNER_URL:http://localhost:8000} + mode: ${SKILLHUB_SECURITY_SCANNER_MODE:upload} stream: reclaim-enabled: ${SKILLHUB_SCAN_STREAM_RECLAIM_ENABLED:true} reclaim-min-idle: ${SKILLHUB_SCAN_STREAM_RECLAIM_MIN_IDLE:PT2M} diff --git a/server/skillhub-app/src/main/resources/application.yml b/server/skillhub-app/src/main/resources/application.yml index 7d9a1cbc9..286ff05f9 100644 --- a/server/skillhub-app/src/main/resources/application.yml +++ b/server/skillhub-app/src/main/resources/application.yml @@ -144,7 +144,7 @@ skillhub: download: anonymous-cookie-name: ${SKILLHUB_DOWNLOAD_ANON_COOKIE_NAME:skillhub_anon_dl} anonymous-cookie-max-age: ${SKILLHUB_DOWNLOAD_ANON_COOKIE_MAX_AGE:P30D} - anonymous-cookie-secret: ${SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET:change-me-in-production} + anonymous-cookie-secret: ${SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET:} publish: max-file-count: 100 max-single-file-size: 10485760 # 10MB @@ -166,7 +166,7 @@ skillhub: verification-uri: ${DEVICE_AUTH_VERIFICATION_URI:${skillhub.public.base-url:}/cli/auth} security: scanner: - enabled: ${SKILLHUB_SECURITY_SCANNER_ENABLED:false} + enabled: ${SKILLHUB_SECURITY_SCANNER_ENABLED:true} base-url: ${SKILLHUB_SECURITY_SCANNER_URL:http://localhost:8000} health-path: /health scan-path: /scan-upload diff --git a/server/skillhub-app/src/main/resources/messages.properties b/server/skillhub-app/src/main/resources/messages.properties index 2301e0d5c..8f7cf1dfc 100644 --- a/server/skillhub-app/src/main/resources/messages.properties +++ b/server/skillhub-app/src/main/resources/messages.properties @@ -68,6 +68,7 @@ error.namespace.slug.exists=Namespace slug ''{0}'' already exists error.namespace.id.notFound=Namespace not found: {0} error.namespace.slug.notFound=Namespace not found: {0} error.namespace.membership.required=Namespace membership required +error.namespace.global.members.platformAdmin.required=Only platform user administrators can list global namespace members error.namespace.admin.required=Namespace owner or admin role required error.namespace.owner.required=Namespace owner role required error.namespace.create.platformAdminRequired=Only SKILL_ADMIN or SUPER_ADMIN can create namespaces @@ -93,6 +94,7 @@ error.skill.publish.package.invalid=Package validation failed: {0} error.skill.publish.skillMd.notFound=SKILL.md not found error.skill.publish.precheck.confirmRequired=Pre-publish warnings require confirmation before publishing:\n{0} error.skill.publish.precheck.failed=Pre-publish validation failed: {0} +error.security.scanner.required=Security scanner must be enabled before publishing public or namespace-visible skills error.skill.publish.archived=Archived skill must be restored before publishing: {0} review.withdraw.not_pending=Only pending review submissions can be withdrawn: {0} review.withdraw.not_submitter=Only the submitter can withdraw this review diff --git a/server/skillhub-app/src/main/resources/messages_zh.properties b/server/skillhub-app/src/main/resources/messages_zh.properties index a968cc77e..e608d2475 100644 --- a/server/skillhub-app/src/main/resources/messages_zh.properties +++ b/server/skillhub-app/src/main/resources/messages_zh.properties @@ -68,6 +68,7 @@ error.namespace.slug.exists=命名空间 slug ''{0}'' 已存在 error.namespace.id.notFound=未找到命名空间：{0} error.namespace.slug.notFound=未找到命名空间：{0} error.namespace.membership.required=需要先加入该命名空间 +error.namespace.global.members.platformAdmin.required=只有平台用户管理员可以查看 global 命名空间成员 error.namespace.admin.required=需要命名空间管理员或所有者权限 error.namespace.owner.required=需要命名空间所有者权限 error.namespace.create.platformAdminRequired=只有 SKILL_ADMIN 或 SUPER_ADMIN 可以创建命名空间 @@ -93,6 +94,7 @@ error.skill.publish.package.invalid=技能包校验失败：{0} error.skill.publish.skillMd.notFound=未找到 SKILL.md error.skill.publish.precheck.confirmRequired=预发布发现以下风险提醒，确认后仍可继续发布：\n{0} error.skill.publish.precheck.failed=预发布校验失败：{0} +error.security.scanner.required=发布公开或命名空间可见技能前必须启用安全扫描器 error.skill.publish.archived=该技能已归档，请先恢复后再发布：{0} review.withdraw.not_pending=只有待审核版本才能撤销审核：{0} review.withdraw.not_submitter=只有提交人本人可以撤销此次审核 diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/config/DownloadRateLimitPropertiesTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/config/DownloadRateLimitPropertiesTest.java new file mode 100644 index 000000000..56f8994dd --- /dev/null +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/config/DownloadRateLimitPropertiesTest.java @@ -0,0 +1,16 @@ +package com.iflytek.skillhub.config; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.jupiter.api.Test; + +class DownloadRateLimitPropertiesTest { + + @Test + void anonymousCookieSecretDoesNotDefaultToProductionPlaceholder() { + DownloadRateLimitProperties properties = new DownloadRateLimitProperties(); + + assertThat(properties.getAnonymousCookieSecret()) + .isNull(); + } +} diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/config/SkillScannerPropertiesBindingTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/config/SkillScannerPropertiesBindingTest.java index 63efeb8be..42e252941 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/config/SkillScannerPropertiesBindingTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/config/SkillScannerPropertiesBindingTest.java @@ -14,22 +14,34 @@ import java.util.Map; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; class SkillScannerPropertiesBindingTest { @Test - void defaultConfig_disablesScannerByDefault() throws IOException { + void defaultConfig_enablesScannerByDefault() throws IOException { SkillScannerProperties properties = bindProperties( List.of("application.yml"), Map.of() ); - assertFalse(properties.isEnabled()); + assertTrue(properties.isEnabled()); assertEquals("local", properties.getMode()); assertEquals("http://localhost:8000", properties.getBaseUrl()); } + @Test + void localConfig_usesUploadScannerModeByDefault() throws IOException { + SkillScannerProperties properties = bindProperties( + List.of("application-local.yml", "application.yml"), + Map.of() + ); + + assertTrue(properties.isEnabled()); + assertEquals("upload", properties.getMode()); + assertEquals("http://localhost:8000", properties.getBaseUrl()); + } + @Test void environmentVariables_overrideScannerDefaults() throws IOException { SkillScannerProperties properties = bindProperties( diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/DeviceAuthControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/DeviceAuthControllerTest.java new file mode 100644 index 000000000..b2c8a4607 --- /dev/null +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/DeviceAuthControllerTest.java @@ -0,0 +1,58 @@ +package com.iflytek.skillhub.controller; + +import static org.mockito.BDDMockito.given; +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; + +import com.iflytek.skillhub.auth.device.DeviceAuthService; +import com.iflytek.skillhub.auth.device.DeviceCodeResponse; +import com.iflytek.skillhub.auth.device.DeviceTokenResponse; +import com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.http.MediaType; +import org.springframework.test.context.ActiveProfiles; +import org.springframework.test.web.servlet.MockMvc; + +@SpringBootTest +@AutoConfigureMockMvc +@ActiveProfiles("test") +class DeviceAuthControllerTest { + + @Autowired + private MockMvc mockMvc; + + @MockBean + private DeviceAuthService deviceAuthService; + + @MockBean + private NamespaceMemberRepository namespaceMemberRepository; + + @Test + void requestDeviceCode_withoutCsrfIsAllowedForCliFlow() throws Exception { + given(deviceAuthService.generateDeviceCode()) + .willReturn(new DeviceCodeResponse("device-1", "USER-CODE", "http://localhost/device", 600, 5)); + + mockMvc.perform(post("/api/v1/auth/device/code")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.code").value(0)) + .andExpect(jsonPath("$.data.deviceCode").value("device-1")); + } + + @Test + void pollToken_withoutCsrfIsAllowedForCliFlow() throws Exception { + given(deviceAuthService.pollToken("device-1")) + .willReturn(DeviceTokenResponse.success("token-1")); + + mockMvc.perform(post("/api/v1/auth/device/token") + .contentType(MediaType.APPLICATION_JSON) + .content("{\"deviceCode\":\"device-1\"}")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.code").value(0)) + .andExpect(jsonPath("$.data.accessToken").value("token-1")); + } +} diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/LocalAuthControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/LocalAuthControllerTest.java index 7158cdfd9..440fe4bf9 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/LocalAuthControllerTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/LocalAuthControllerTest.java @@ -17,6 +17,7 @@ import com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository; import com.iflytek.skillhub.metrics.SkillHubMetrics; import com.iflytek.skillhub.security.AuthFailureThrottleService; +import jakarta.servlet.http.Cookie; import java.util.List; import java.util.Set; import org.junit.jupiter.api.Test; @@ -197,6 +198,61 @@ void changePassword_withAuthentication_returnsUpdated() throws Exception { .andExpect(jsonPath("$.code").value(0)); } + @Test + void changePassword_withAuthentication_withInvalidCsrf_returnsForbidden() throws Exception { + PlatformPrincipal principal = new PlatformPrincipal( + "usr_3", + "carol", + "carol@example.com", + "", + "local", + Set.of("SUPER_ADMIN") + ); + var auth = new UsernamePasswordAuthenticationToken( + principal, + null, + List.of(new SimpleGrantedAuthority("ROLE_SUPER_ADMIN")) + ); + + mockMvc.perform(post("/api/v1/auth/local/change-password") + .with(authentication(auth)) + .with(csrf().useInvalidToken()) + .contentType(MediaType.APPLICATION_JSON) + .content(""" + {"currentPassword":"old","newPassword":"Newpass123!"} + """)) + .andExpect(status().isForbidden()); + } + + @Test + void changePassword_withSessionCookieAndBearerHeaderWithoutCsrf_isRejected() throws Exception { + PlatformPrincipal principal = new PlatformPrincipal( + "usr_3", + "carol", + "carol@example.com", + "", + "local", + Set.of("SUPER_ADMIN") + ); + var auth = new UsernamePasswordAuthenticationToken( + principal, + null, + List.of(new SimpleGrantedAuthority("ROLE_SUPER_ADMIN")) + ); + + mockMvc.perform(post("/api/v1/auth/local/change-password") + .with(authentication(auth)) + .header("Authorization", "Bearer invalid-token") + .cookie(new Cookie("SESSION", "browser-session")) + .contentType(MediaType.APPLICATION_JSON) + .content(""" + {"currentPassword":"old","newPassword":"Newpass123!"} + """)) + .andExpect(status().isUnauthorized()); + + verify(localAuthService, never()).changePassword("usr_3", "old", "Newpass123!"); + } + @Test void requestPasswordReset_returnsGenericSuccessEnvelope() throws Exception { mockMvc.perform(post("/api/v1/auth/local/password-reset/request") diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/NamespacePortalControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/NamespacePortalControllerTest.java index 699cb740a..566d3ef02 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/NamespacePortalControllerTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/NamespacePortalControllerTest.java @@ -174,6 +174,41 @@ void listMembers_forNonMember_returns403() throws Exception { .andExpect(jsonPath("$.code").value(403)); } + @Test + void listMembers_globalNamespaceRejectsRegularUsers() throws Exception { + Namespace namespace = namespace(1L, "global", NamespaceStatus.ACTIVE, NamespaceType.GLOBAL); + given(namespaceService.getNamespaceBySlug("global")).willReturn(namespace); + given(namespaceMemberService.listMembers(eq(1L), any())) + .willReturn(new org.springframework.data.domain.PageImpl<>(List.of(), org.springframework.data.domain.PageRequest.of(0, 20), 0)); + + mockMvc.perform(get("/api/v1/namespaces/global/members") + .with(auth("regular-1")) + .requestAttr("userId", "regular-1")) + .andExpect(status().isForbidden()) + .andExpect(jsonPath("$.code").value(403)); + } + + @Test + void listMembers_globalNamespaceAllowsUserAdminWithoutMembership() throws Exception { + Namespace namespace = namespace(1L, "global", NamespaceStatus.ACTIVE, NamespaceType.GLOBAL); + NamespaceMember member = new NamespaceMember(1L, "user-2", NamespaceRole.MEMBER); + UserAccount user = new UserAccount("user-2", "Alice", "alice@example.com", null); + given(namespaceService.getNamespaceBySlug("global")).willReturn(namespace); + doThrow(new DomainForbiddenException("error.namespace.membership.required")) + .when(namespaceService).assertMember(1L, "user-admin-1"); + given(namespaceMemberService.listMembers(eq(1L), any())) + .willReturn(new org.springframework.data.domain.PageImpl<>(List.of(member), org.springframework.data.domain.PageRequest.of(0, 20), 1)); + given(userAccountRepository.findByIdIn(List.of("user-2"))).willReturn(List.of(user)); + + mockMvc.perform(get("/api/v1/namespaces/global/members") + .with(auth("user-admin-1", Set.of("USER_ADMIN"))) + .requestAttr("userId", "user-admin-1")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.code").value(0)) + .andExpect(jsonPath("$.data.items[0].userId").value("user-2")) + .andExpect(jsonPath("$.data.items[0].email").value("alice@example.com")); + } + @Test void searchMemberCandidates_returnsCandidates() throws Exception { Namespace namespace = namespace(1L, "team-a", NamespaceStatus.ACTIVE, NamespaceType.TEAM); diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/NamespaceWorkflowContractTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/NamespaceWorkflowContractTest.java index 72b5d26dd..b72551ed6 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/NamespaceWorkflowContractTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/NamespaceWorkflowContractTest.java @@ -95,7 +95,7 @@ void namespaceWorkflowEndpoints_shareExpectedEnvelopeShapes() throws Exception { .willReturn(List.of(new NamespaceCandidateUserResponse("user-admin", "Admin", "admin@example.com", "ACTIVE"))); given(namespacePortalCommandAppService.addMember("team-flow", "user-admin", NamespaceRole.ADMIN, "owner-1")) .willReturn(adminMemberResponse); - given(namespacePortalQueryAppService.listMembers(eq("team-flow"), any(org.springframework.data.domain.Pageable.class), eq("owner-1"))) + given(namespacePortalQueryAppService.listMembers(eq("team-flow"), any(org.springframework.data.domain.Pageable.class), eq("owner-1"), eq(Set.of()))) .willReturn(new PageResponse<>(List.of(adminMemberResponse), 1, 0, 20)); given(namespacePortalCommandAppService.updateMemberRole(eq("team-flow"), eq("user-admin"), any(), eq("owner-1"))) .willReturn(adminMemberResponse); diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/SkillStarControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/SkillStarControllerTest.java index 111d11739..a605b7cc8 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/SkillStarControllerTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/SkillStarControllerTest.java @@ -136,7 +136,7 @@ void check_starred_unauthenticated_returns_401() throws Exception { } @Test - void apiWebStarSkillWithoutCsrfShouldAllowSessionAuth() throws Exception { + void apiWebStarSkillWithCsrfShouldAllowSessionAuth() throws Exception { PlatformPrincipal principal = new PlatformPrincipal( "user-42", "tester", @@ -152,7 +152,8 @@ void apiWebStarSkillWithoutCsrfShouldAllowSessionAuth() throws Exception { ); mockMvc.perform(put("/api/web/skills/10/star") - .with(authentication(auth))) + .with(authentication(auth)) + .with(csrf())) .andExpect(status().isOk()) .andExpect(jsonPath("$.code").value(0)); diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/cli/CliDryRunValidateTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/cli/CliDryRunValidateTest.java index f2224de08..d82aa32dd 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/cli/CliDryRunValidateTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/cli/CliDryRunValidateTest.java @@ -55,6 +55,7 @@ void validatePublish_returnsValidResult() throws Exception { mockMvc.perform(multipart("/api/cli/v1/skills/global/publish/validate") .file(file) + .header("Authorization", "Bearer test-token") .with(authentication(auth()))) .andExpect(status().isOk()) .andExpect(jsonPath("$.data.valid").value(true)) @@ -75,6 +76,7 @@ void validatePublish_returnsInvalidResult() throws Exception { mockMvc.perform(multipart("/api/cli/v1/skills/global/publish/validate") .file(file) + .header("Authorization", "Bearer test-token") .with(authentication(auth()))) .andExpect(status().isOk()) .andExpect(jsonPath("$.data.valid").value(false)) @@ -95,6 +97,7 @@ void validatePublish_acceptsCustomVisibility() throws Exception { mockMvc.perform(multipart("/api/cli/v1/skills/global/publish/validate") .file(file) .file(new MockMultipartFile("visibility", "", "text/plain", "PRIVATE".getBytes())) + .header("Authorization", "Bearer test-token") .with(authentication(auth()))) .andExpect(status().isOk()) .andExpect(jsonPath("$.data.valid").value(true)); @@ -108,6 +111,7 @@ void validatePublish_rejectsInvalidVisibility() throws Exception { mockMvc.perform(multipart("/api/cli/v1/skills/global/publish/validate") .file(file) .file(new MockMultipartFile("visibility", "", "text/plain", "BOGUS".getBytes())) + .header("Authorization", "Bearer test-token") .with(authentication(auth()))) .andExpect(status().isBadRequest()); } diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/cli/CliSkillControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/cli/CliSkillControllerTest.java index d7edf81d6..b2421c891 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/cli/CliSkillControllerTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/cli/CliSkillControllerTest.java @@ -118,6 +118,7 @@ void deleteReturnsCliDeleteResponse() throws Exception { mockMvc.perform(org.springframework.test.web.servlet.request.MockMvcRequestBuilders .delete("/api/cli/v1/skills/global/demo") + .header("Authorization", "Bearer test-token") .with(authentication(auth))) .andExpect(status().isOk()) .andExpect(jsonPath("$.data.ok").value(true)) diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/portal/SkillSubscriptionControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/portal/SkillSubscriptionControllerTest.java index 809c22a97..4ebf94243 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/portal/SkillSubscriptionControllerTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/portal/SkillSubscriptionControllerTest.java @@ -59,7 +59,8 @@ private static UsernamePasswordAuthenticationToken authenticatedUser() { @Test void subscribe_skill_returns_envelope() throws Exception { mockMvc.perform(put("/api/web/skills/10/subscription") - .with(authentication(authenticatedUser()))) + .with(authentication(authenticatedUser())) + .with(csrf())) .andExpect(status().isOk()) .andExpect(jsonPath("$.code").value(0)) .andExpect(jsonPath("$.timestamp").isNotEmpty()) @@ -80,7 +81,8 @@ void subscribe_skill_unauthenticated_returns_401() throws Exception { @Test void unsubscribe_skill_returns_envelope() throws Exception { mockMvc.perform(delete("/api/web/skills/10/subscription") - .with(authentication(authenticatedUser()))) + .with(authentication(authenticatedUser())) + .with(csrf())) .andExpect(status().isOk()) .andExpect(jsonPath("$.code").value(0)) .andExpect(jsonPath("$.timestamp").isNotEmpty()) diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/ratelimit/AnonymousDownloadIdentityServiceTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/ratelimit/AnonymousDownloadIdentityServiceTest.java new file mode 100644 index 000000000..39603ebd6 --- /dev/null +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/ratelimit/AnonymousDownloadIdentityServiceTest.java @@ -0,0 +1,20 @@ +package com.iflytek.skillhub.ratelimit; + +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import com.iflytek.skillhub.config.DownloadRateLimitProperties; +import org.junit.jupiter.api.Test; + +class AnonymousDownloadIdentityServiceTest { + + @Test + void validateAnonymousCookieSecretRejectsReleaseExamplePlaceholder() { + DownloadRateLimitProperties properties = new DownloadRateLimitProperties(); + properties.setAnonymousCookieSecret("replace-with-random-download-secret-32-bytes"); + AnonymousDownloadIdentityService service = new AnonymousDownloadIdentityService(properties, new ClientIpResolver()); + + assertThatThrownBy(service::validateAnonymousCookieSecret) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("must not use the default placeholder"); + } +} diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/NamespacePortalQueryAppServiceTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/NamespacePortalQueryAppServiceTest.java index 10e58718c..05a9fd3c6 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/NamespacePortalQueryAppServiceTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/NamespacePortalQueryAppServiceTest.java @@ -29,6 +29,7 @@ import java.util.List; import java.util.Map; +import java.util.Set; class NamespacePortalQueryAppServiceTest { @@ -131,7 +132,7 @@ void listMembers_withUserAccount_returnsDisplayNameAndEmail() { when(userAccountRepository.findByIdIn(List.of("user-2"))) .thenReturn(List.of(user)); - PageResponse result = service.listMembers("team-a", PageRequest.of(0, 20), "owner-1"); + PageResponse result = service.listMembers("team-a", PageRequest.of(0, 20), "owner-1", Set.of()); assertThat(result.items()).hasSize(1); MemberResponse mr = result.items().get(0); @@ -153,7 +154,7 @@ void listMembers_withoutUserAccount_returnsNullFields() { when(userAccountRepository.findByIdIn(List.of("ghost-user"))) .thenReturn(List.of()); - PageResponse result = service.listMembers("team-a", PageRequest.of(0, 20), "owner-1"); + PageResponse result = service.listMembers("team-a", PageRequest.of(0, 20), "owner-1", Set.of()); assertThat(result.items()).hasSize(1); MemberResponse mr = result.items().get(0); @@ -161,4 +162,17 @@ void listMembers_withoutUserAccount_returnsNullFields() { assertThat(mr.displayName()).isNull(); assertThat(mr.email()).isNull(); } + + @Test + void listMembers_globalNamespaceRejectsRegularUsersEvenWhenTheyAreGlobalMembers() { + Namespace ns = namespace(1L, "global"); + ns.setType(NamespaceType.GLOBAL); + when(namespaceService.getNamespaceBySlug("global")).thenReturn(ns); + when(namespaceMemberService.listMembers(eq(1L), any(PageRequest.class))) + .thenReturn(new PageImpl<>(List.of(), PageRequest.of(0, 20), 0)); + + assertThatThrownBy(() -> service.listMembers("global", PageRequest.of(0, 20), "user-1", Set.of())) + .isInstanceOf(DomainForbiddenException.class) + .hasMessageContaining("error.namespace.global.members.platformAdmin.required"); + } } diff --git a/server/skillhub-app/src/test/resources/application-test.yml b/server/skillhub-app/src/test/resources/application-test.yml index e7a6ea698..8749a3557 100644 --- a/server/skillhub-app/src/test/resources/application-test.yml +++ b/server/skillhub-app/src/test/resources/application-test.yml @@ -41,6 +41,9 @@ skillhub: enforce-active-user-check: false access-policy: mode: OPEN + ratelimit: + download: + anonymous-cookie-secret: test-download-secret-32-bytes-long security: scanner: enabled: false diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/SecurityConfig.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/SecurityConfig.java index 05b870252..8c2ff2dca 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/SecurityConfig.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/SecurityConfig.java @@ -9,6 +9,8 @@ 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.Cookie; +import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; import org.springframework.beans.factory.ObjectProvider; import org.springframework.context.annotation.Bean; @@ -103,7 +105,7 @@ public SecurityFilterChain filterChain(HttpSecurity http) throws Exception { RequestMatcher csrfIgnoreMatcher = request -> { String path = request.getRequestURI(); String authorization = request.getHeader("Authorization"); - return routeSecurityPolicyRegistry.shouldIgnoreCsrf(path, authorization); + return routeSecurityPolicyRegistry.shouldIgnoreCsrf(request.getMethod(), path, authorization, hasSessionCookie(request)); }; http @@ -183,4 +185,20 @@ private void configureRoutePolicies(AuthorizeHttpRequestsConfigurer existingSkills = skillRepository.findByNamespaceIdAndSlug(namespace.getId(), skillSlug); @@ -588,6 +594,10 @@ private void deleteReplaceableVersionArtifacts(Skill skill, SkillVersion version skillVersionRepository.flush(); } + private boolean requiresSecurityScanner(SkillVisibility visibility) { + return visibility == SkillVisibility.PUBLIC || visibility == SkillVisibility.NAMESPACE_ONLY; + } + private String resolveNamespaceSlug(Long namespaceId) { return namespaceRepository.findById(namespaceId) .orElseThrow(() -> new DomainBadRequestException("error.namespace.notFound", namespaceId)) diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java index e40fa545b..73f118fd0 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java @@ -101,7 +101,7 @@ void setUp() { eventPublisher, CLOCK ); - lenient().when(securityScanService.isEnabled()).thenReturn(false); + lenient().when(securityScanService.isEnabled()).thenReturn(true); lenient().when(skillVersionRepository.findBySkillIdAndStatus(anyLong(), eq(SkillVersionStatus.PENDING_REVIEW))) .thenReturn(List.of()); lenient().when(reviewTaskRepository.save(any(ReviewTask.class))).thenAnswer(invocation -> invocation.getArgument(0)); @@ -1284,6 +1284,120 @@ void testSkillVersionStatus_ShouldSupportScanningLifecycleStates() { assertEquals(SkillVersionStatus.SCAN_FAILED, SkillVersionStatus.valueOf("SCAN_FAILED")); } + @Test + void testPublishFromEntries_PublicWhenScannerDisabled_ShouldRejectBeforeSideEffects() throws Exception { + String namespaceSlug = "test-ns"; + String publisherId = "user-100"; + PublishFixture fixture = stubValidPublishInputs(namespaceSlug, publisherId, "test-skill", "test-skill", "1.0.0", true); + when(securityScanService.isEnabled()).thenReturn(false); + + DomainBadRequestException exception = assertThrows(DomainBadRequestException.class, () -> service.publishFromEntries( + namespaceSlug, + fixture.entries(), + publisherId, + SkillVisibility.PUBLIC, + Set.of() + )); + + assertEquals("error.security.scanner.required", exception.messageCode()); + verify(skillRepository, never()).findByNamespaceIdAndSlug(anyLong(), anyString()); + verify(skillRepository, never()).save(any(Skill.class)); + verify(skillVersionRepository, never()).save(any(SkillVersion.class)); + verify(objectStorageService, never()).putObject(anyString(), any(), anyLong(), anyString()); + verify(reviewTaskRepository, never()).save(any(ReviewTask.class)); + } + + @Test + void testPublishFromEntries_NamespaceOnlyWhenScannerDisabled_ShouldRejectBeforeSideEffects() throws Exception { + String namespaceSlug = "test-ns"; + String publisherId = "user-100"; + PublishFixture fixture = stubValidPublishInputs(namespaceSlug, publisherId, "team-skill", "team-skill", "1.0.0", true); + when(securityScanService.isEnabled()).thenReturn(false); + + DomainBadRequestException exception = assertThrows(DomainBadRequestException.class, () -> service.publishFromEntries( + namespaceSlug, + fixture.entries(), + publisherId, + SkillVisibility.NAMESPACE_ONLY, + Set.of() + )); + + assertEquals("error.security.scanner.required", exception.messageCode()); + verify(skillRepository, never()).findByNamespaceIdAndSlug(anyLong(), anyString()); + verify(skillRepository, never()).save(any(Skill.class)); + verify(skillVersionRepository, never()).save(any(SkillVersion.class)); + verify(objectStorageService, never()).putObject(anyString(), any(), anyLong(), anyString()); + verify(reviewTaskRepository, never()).save(any(ReviewTask.class)); + } + + @Test + void testPublishFromEntries_PrivateWhenScannerDisabled_ShouldAllowUploadWithoutScan() throws Exception { + String namespaceSlug = "test-ns"; + String publisherId = "user-100"; + PublishFixture fixture = stubValidPublishInputs(namespaceSlug, publisherId, "private-skill", "private-skill", "1.0.0", true); + when(securityScanService.isEnabled()).thenReturn(false); + + SkillPublishService.PublishResult result = service.publishFromEntries( + namespaceSlug, + fixture.entries(), + publisherId, + SkillVisibility.PRIVATE, + Set.of() + ); + + assertEquals(SkillVersionStatus.UPLOADED, result.version().getStatus()); + verify(securityScanService, never()).triggerScan(anyLong(), anyList(), anyString()); + verify(reviewTaskRepository, never()).save(any(ReviewTask.class)); + } + + @Test + void testPublishFromEntries_SuperAdminPublicWhenScannerDisabled_ShouldStillRejectBeforeAutoPublish() throws Exception { + String namespaceSlug = "test-ns"; + String publisherId = "admin-user"; + PublishFixture fixture = stubValidPublishInputs(namespaceSlug, publisherId, "admin-skill", "admin-skill", "1.0.0", false); + when(securityScanService.isEnabled()).thenReturn(false); + + DomainBadRequestException exception = assertThrows(DomainBadRequestException.class, () -> service.publishFromEntries( + namespaceSlug, + fixture.entries(), + publisherId, + SkillVisibility.PUBLIC, + Set.of("SUPER_ADMIN") + )); + + assertEquals("error.security.scanner.required", exception.messageCode()); + verify(skillVersionRepository, never()).save(any(SkillVersion.class)); + verify(eventPublisher, never()).publishEvent(any(SkillPublishedEvent.class)); + } + + @Test + void testValidateOnly_PublicWhenScannerDisabled_ShouldReturnScannerRequiredError() throws Exception { + String namespaceSlug = "test-ns"; + String publisherId = "user-100"; + List entries = skillEntries("test-skill", "1.0.0"); + Namespace namespace = new Namespace(namespaceSlug, "Test NS", "user-1"); + setId(namespace, 1L); + NamespaceMember member = mock(NamespaceMember.class); + SkillMetadata metadata = new SkillMetadata("test-skill", "Test", "1.0.0", "Body", Map.of()); + when(securityScanService.isEnabled()).thenReturn(false); + when(namespaceRepository.findBySlug(namespaceSlug)).thenReturn(Optional.of(namespace)); + when(namespaceMemberRepository.findByNamespaceIdAndUserId(any(), eq(publisherId))).thenReturn(Optional.of(member)); + when(skillPackageValidator.validate(entries)).thenReturn(ValidationResult.pass()); + when(skillMetadataParser.parse(anyString())).thenReturn(metadata); + when(prePublishValidator.validate(any())).thenReturn(ValidationResult.pass()); + + SkillPublishService.DryRunResult result = service.validateOnly( + namespaceSlug, + entries, + publisherId, + SkillVisibility.PUBLIC, + Set.of() + ); + + assertFalse(result.valid()); + assertTrue(result.errors().contains("error.security.scanner.required")); + } + @Test void testPublishFromEntries_WhenScannerEnabled_ShouldCreateReviewTaskAndTriggerScan() throws Exception { String namespaceSlug = "test-ns"; @@ -1377,6 +1491,56 @@ void testPublishFromEntries_SuperAdmin_WhenScannerEnabled_ShouldTriggerScan() th verify(reviewTaskRepository, never()).save(any(ReviewTask.class)); } + private record PublishFixture(List entries) { + } + + private PublishFixture stubValidPublishInputs( + String namespaceSlug, + String publisherId, + String skillName, + String skillSlug, + String version, + boolean stubMembership) throws Exception { + List entries = skillEntries(skillName, version); + Namespace namespace = new Namespace(namespaceSlug, "Test NS", "user-1"); + setId(namespace, 1L); + SkillMetadata metadata = new SkillMetadata(skillName, "Test", version, "Body", Map.of()); + Skill skill = new Skill(namespace.getId(), skillSlug, publisherId, SkillVisibility.PUBLIC); + setId(skill, 1L); + + when(namespaceRepository.findBySlug(namespaceSlug)).thenReturn(Optional.of(namespace)); + if (stubMembership) { + NamespaceMember member = mock(NamespaceMember.class); + when(namespaceMemberRepository.findByNamespaceIdAndUserId(any(), eq(publisherId))).thenReturn(Optional.of(member)); + } + when(skillPackageValidator.validate(entries)).thenReturn(ValidationResult.pass()); + when(skillMetadataParser.parse(anyString())).thenReturn(metadata); + when(prePublishValidator.validate(any())).thenReturn(ValidationResult.pass()); + lenient().when(skillRepository.findByNamespaceIdAndSlug(namespace.getId(), skillSlug)).thenReturn(List.of(skill)); + lenient().when(skillRepository.findByNamespaceIdAndSlugAndOwnerId(namespace.getId(), skillSlug, publisherId)).thenReturn(Optional.of(skill)); + lenient().when(skillVersionRepository.findBySkillIdAndVersion(skill.getId(), version)).thenReturn(Optional.empty()); + lenient().when(skillVersionRepository.save(any(SkillVersion.class))).thenAnswer(invocation -> { + SkillVersion saved = invocation.getArgument(0); + if (saved.getId() == null) { + setId(saved, 10L); + } + return saved; + }); + lenient().when(skillRepository.save(any(Skill.class))).thenReturn(skill); + return new PublishFixture(entries); + } + + private List skillEntries(String skillName, String version) { + String skillMdContent = "---\nname: " + skillName + "\ndescription: Test\nversion: " + version + "\n---\nBody"; + PackageEntry skillMd = new PackageEntry( + "SKILL.md", + skillMdContent.getBytes(StandardCharsets.UTF_8), + skillMdContent.getBytes(StandardCharsets.UTF_8).length, + "text/markdown"); + PackageEntry readme = new PackageEntry("README.md", "content".getBytes(StandardCharsets.UTF_8), 7, "text/markdown"); + return List.of(skillMd, readme); + } + private void setId(Object entity, Long id) throws Exception { Field idField = entity.getClass().getDeclaredField("id"); idField.setAccessible(true); diff --git a/web/e2e/helpers/csrf.ts b/web/e2e/helpers/csrf.ts new file mode 100644 index 000000000..410b08375 --- /dev/null +++ b/web/e2e/helpers/csrf.ts @@ -0,0 +1,27 @@ +import type { Page } from '@playwright/test' + +const csrfCookieName = 'XSRF-TOKEN' +const csrfHeaderName = 'X-XSRF-TOKEN' +const requestTimeoutMs = process.env.CI ? 12_000 : 8_000 + +async function readCsrfToken(page: Page): Promise { + const cookie = (await page.context().cookies()).find((item) => item.name === csrfCookieName) + return cookie?.value?.trim() || null +} + +export async function csrfHeaders(page: Page, headers?: Record): Promise> { + let token = await readCsrfToken(page) + if (!token) { + await page.context().request.get('/api/v1/auth/providers', { timeout: requestTimeoutMs }) + token = await readCsrfToken(page) + } + + if (!token) { + throw new Error('Missing XSRF-TOKEN cookie after auth provider warm-up') + } + + return { + ...headers, + [csrfHeaderName]: token, + } +} diff --git a/web/e2e/helpers/session.ts b/web/e2e/helpers/session.ts index 763ef1c37..91554c626 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 { csrfHeaders } from './csrf' const password = 'Passw0rd!123' const cachedUserByWorker = new Map() @@ -50,15 +51,17 @@ function isRetryableStatus(status: number): boolean { } async function loginWithRetry( - request: Page['request'], + page: Page, username: string, currentPassword = password, retries = process.env.CI ? 10 : 6, ): Promise { + const request = page.context().request for (let i = 0; i < retries; i += 1) { try { const login = await request.post('/api/v1/auth/local/login', { data: { username, password: currentPassword }, + headers: await csrfHeaders(page), timeout: requestTimeoutMs, }) @@ -187,13 +190,13 @@ async function registerSessionOnce(page: Page, testInfo?: TestInfo, options?: Re } // Prefer the known-good cached account to avoid repeated failed-logins on a fixed username. - if (cached && await loginWithRetry(request, cached)) { + if (cached && await loginWithRetry(page, cached)) { await cacheSession(page, worker, cached) return { username: cached, password } } // Support environments where a deterministic worker account already exists. - if (!cached && await loginWithRetry(request, username, password, process.env.CI ? 4 : 3)) { + if (!cached && await loginWithRetry(page, username, password, process.env.CI ? 4 : 3)) { cachedUserByWorker.set(worker, username) await cacheSession(page, worker, username) return { username, password } @@ -206,6 +209,7 @@ async function registerSessionOnce(page: Page, testInfo?: TestInfo, options?: Re password, email: `${username}@example.test`, }, + headers: await csrfHeaders(page), timeout: requestTimeoutMs, }) @@ -215,7 +219,7 @@ async function registerSessionOnce(page: Page, testInfo?: TestInfo, options?: Re return { username, password } } - if (register.status() === 409 && await loginWithRetry(request, username, password, process.env.CI ? 8 : 6)) { + if (register.status() === 409 && await loginWithRetry(page, username, password, process.env.CI ? 8 : 6)) { cachedUserByWorker.set(worker, username) await cacheSession(page, worker, username) return { username, password } @@ -236,6 +240,7 @@ async function registerSessionOnce(page: Page, testInfo?: TestInfo, options?: Re password, email: `${uniqueUsername}@example.test`, }, + headers: await csrfHeaders(page), timeout: requestTimeoutMs, }) @@ -269,7 +274,7 @@ async function registerSessionOnce(page: Page, testInfo?: TestInfo, options?: Re // Final fallback for environments where registration is temporarily unavailable. const fallbackCandidates = [cached, username].filter((candidate): candidate is string => Boolean(candidate)) for (const candidate of fallbackCandidates) { - if (await loginWithRetry(request, candidate, password, process.env.CI ? 12 : 8)) { + if (await loginWithRetry(page, candidate, password, process.env.CI ? 12 : 8)) { cachedUserByWorker.set(worker, candidate) await cacheSession(page, worker, candidate) return { username: candidate, password } @@ -295,6 +300,7 @@ async function createFreshSessionOnce(page: Page, testInfo?: TestInfo) { password, email: `${uniqueUsername}@example.test`, }, + headers: await csrfHeaders(page), timeout: requestTimeoutMs, }) @@ -358,8 +364,6 @@ export async function createFreshSession(page: Page, testInfo?: TestInfo) { } export async function loginWithCredentials(page: Page, credentials: TestCredentials, _testInfo?: TestInfo) { - const request = page.context().request - await primeAuthProviders(page) const restored = await restoreCachedSessionForAccount(page, credentials.username) @@ -368,7 +372,7 @@ export async function loginWithCredentials(page: Page, credentials: TestCredenti } const loggedIn = await loginWithRetry( - request, + page, credentials.username, credentials.password, process.env.CI ? 12 : 8, diff --git a/web/e2e/helpers/test-data-builder.ts b/web/e2e/helpers/test-data-builder.ts index a6b77adce..f255ea4a6 100644 --- a/web/e2e/helpers/test-data-builder.ts +++ b/web/e2e/helpers/test-data-builder.ts @@ -3,6 +3,7 @@ import { tmpdir } from 'node:os' import { execFileSync } from 'node:child_process' import path from 'node:path' import type { APIRequestContext, Page, TestInfo } from '@playwright/test' +import { csrfHeaders } from './csrf' type CleanupTask = () => Promise @@ -237,12 +238,14 @@ export class E2eTestDataBuilder { displayName, description: `E2E namespace ${slug}`, }, + headers: await csrfHeaders(this.page), }), ) this.cleanupTasks.push(async () => { await this.request.post(`/api/web/namespaces/${encodeURIComponent(created.slug)}/archive`, { data: { reason: 'e2e cleanup' }, + headers: await csrfHeaders(this.page), }) }) @@ -270,13 +273,17 @@ export class E2eTestDataBuilder { if (namespace.status === 'FROZEN' && namespace.canUnfreeze) { return parseEnvelope( - await this.request.post(`/api/web/namespaces/${encodeURIComponent(namespace.slug)}/unfreeze`), + await this.request.post(`/api/web/namespaces/${encodeURIComponent(namespace.slug)}/unfreeze`, { + headers: await csrfHeaders(this.page), + }), ) } if (namespace.status === 'ARCHIVED' && namespace.canRestore) { return parseEnvelope( - await this.request.post(`/api/web/namespaces/${encodeURIComponent(namespace.slug)}/restore`), + await this.request.post(`/api/web/namespaces/${encodeURIComponent(namespace.slug)}/restore`, { + headers: await csrfHeaders(this.page), + }), ) } @@ -483,11 +490,12 @@ export class E2eTestDataBuilder { async approveReview(reviewTaskId: number, comment = 'Approved by Playwright E2E'): Promise { let lastError: unknown - for (let attempt = 0; attempt < 30; attempt += 1) { + for (let attempt = 0; attempt < 60; attempt += 1) { try { await parseEnvelope( await this.request.post(`/api/web/reviews/${reviewTaskId}/approve`, { data: { comment }, + headers: await csrfHeaders(this.page), }), ) return @@ -498,7 +506,7 @@ export class E2eTestDataBuilder { if (!isScanInProgress) { throw error } - await new Promise((resolve) => setTimeout(resolve, 500)) + await new Promise((resolve) => setTimeout(resolve, 1_000)) } } throw lastError instanceof Error ? lastError : new Error('approveReview timed out') @@ -515,6 +523,7 @@ export class E2eTestDataBuilder { await parseEnvelope<{ userId: string; role: string }>( await this.request.post(`/api/web/namespaces/${encodeURIComponent(slug)}/members`, { data: { userId, role }, + headers: await csrfHeaders(this.page), }), ) } @@ -533,11 +542,14 @@ export class E2eTestDataBuilder { }, visibility: 'PUBLIC', }, + headers: await csrfHeaders(this.page), }), ) this.cleanupTasks.push(async () => { - await this.request.delete(`/api/web/skills/${encodeURIComponent(result.namespace)}/${encodeURIComponent(result.slug)}`) + await this.request.delete(`/api/web/skills/${encodeURIComponent(result.namespace)}/${encodeURIComponent(result.slug)}`, { + headers: await csrfHeaders(this.page), + }) }) return result diff --git a/web/e2e/public-skill-detail-anonymous.spec.ts b/web/e2e/public-skill-detail-anonymous.spec.ts index 720c27967..f58348121 100644 --- a/web/e2e/public-skill-detail-anonymous.spec.ts +++ b/web/e2e/public-skill-detail-anonymous.spec.ts @@ -18,6 +18,8 @@ function escapeRegExp(value: string) { let seeded: PreparedSearchSeed | undefined test.describe('Public Skill Detail Anonymous Access (Real API)', () => { + test.describe.configure({ timeout: 150_000 }) + test.beforeAll(async ({ browser }, testInfo) => { seeded = await prepareSearchSeed(browser, testInfo, { count: 1 }) }) diff --git a/web/e2e/skill-subscription.spec.ts b/web/e2e/skill-subscription.spec.ts index c6d74d24e..220032010 100644 --- a/web/e2e/skill-subscription.spec.ts +++ b/web/e2e/skill-subscription.spec.ts @@ -16,6 +16,8 @@ function adminCredentials() { } test.describe('Skill Subscription (Real API)', () => { + test.describe.configure({ timeout: 150_000 }) + test.beforeEach(async ({ page }, testInfo) => { await setEnglishLocale(page) await registerSession(page, testInfo) diff --git a/web/e2e/skill-version-compare.spec.ts b/web/e2e/skill-version-compare.spec.ts index c24da0a7c..274197ac1 100644 --- a/web/e2e/skill-version-compare.spec.ts +++ b/web/e2e/skill-version-compare.spec.ts @@ -1,5 +1,6 @@ import { expect, test } from '@playwright/test' import { setEnglishLocale } from './helpers/auth-fixtures' +import { csrfHeaders } from './helpers/csrf' import { loginWithCredentials, registerSession } from './helpers/session' import { E2eTestDataBuilder } from './helpers/test-data-builder' @@ -16,6 +17,8 @@ function adminCredentials() { } test.describe('Skill Version Compare (Real API)', () => { + test.describe.configure({ timeout: 150_000 }) + test.beforeEach(async ({ page }, testInfo) => { await setEnglishLocale(page) await registerSession(page, testInfo) @@ -50,6 +53,7 @@ test.describe('Skill Version Compare (Real API)', () => { targetVersion: '1.1.0', confirmWarnings: true, }, + headers: await csrfHeaders(page), } ) expect(rereleaseResponse.ok()).toBe(true) diff --git a/web/src/api/generated/schema.d.ts b/web/src/api/generated/schema.d.ts index 9e056dff4..07d8c65b7 100644 --- a/web/src/api/generated/schema.d.ts +++ b/web/src/api/generated/schema.d.ts @@ -9938,6 +9938,8 @@ export interface operations { page?: number; size?: number; filter?: string; + q?: string; + namespace?: string; }; header?: never; path?: never; @@ -9962,6 +9964,8 @@ export interface operations { page?: number; size?: number; filter?: string; + q?: string; + namespace?: string; }; header?: never; path?: never; From 7d0402e93783dc9ccd2efb7dba726dda981296ee Mon Sep 17 00:00:00 2001 From: dongmucat <1127093059@qq.com> Date: Tue, 16 Jun 2026 10:17:50 +0800 Subject: [PATCH 2/3] fix(security): address review blockers Signed-off-by: dongmucat <1127093059@qq.com> --- .env.release.example | 1 + .github/workflows/pr-scripts.yml | 5 + cli/src/platform/archive.ts | 8 +- cli/test/unit/platform/archive.test.ts | 11 +++ scripts/runtime.sh | 38 +++++++ scripts/tests/runtime-secret-test.sh | 99 +++++++++++++++++++ scripts/tests/validate-release-config-test.sh | 13 +++ scripts/tests/workflow-security-test.sh | 9 ++ scripts/validate-release-config.sh | 24 +++++ 9 files changed, 206 insertions(+), 2 deletions(-) create mode 100755 scripts/tests/runtime-secret-test.sh diff --git a/.env.release.example b/.env.release.example index 40e51ac10..2d30c7bc9 100644 --- a/.env.release.example +++ b/.env.release.example @@ -105,6 +105,7 @@ SKILLHUB_AUTH_PASSWORD_RESET_FROM_NAME=SkillHub SKILLHUB_SECURITY_SCANNER_ENABLED=true # Required for signing anonymous download rate-limit cookies. Use a unique random value per deployment. +# runtime.sh generates and persists one automatically when this placeholder is still present. SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET=replace-with-random-download-secret-32-bytes # Scanner LLM configuration (optional, for AI-powered scanning features) diff --git a/.github/workflows/pr-scripts.yml b/.github/workflows/pr-scripts.yml index 2f8cc80d8..d15ee4bdd 100644 --- a/.github/workflows/pr-scripts.yml +++ b/.github/workflows/pr-scripts.yml @@ -8,6 +8,9 @@ on: - '.github/workflows/security.yml' - '.github/workflows/pr-scripts.yml' +permissions: + contents: read + jobs: scripts-tests: name: Script Regression Tests @@ -16,10 +19,12 @@ jobs: - uses: actions/checkout@v4 with: fetch-depth: 0 + persist-credentials: false - uses: actions/setup-node@v4 with: node-version: '21' - run: bash scripts/tests/publish-cli-test.sh + - run: bash scripts/tests/runtime-secret-test.sh - run: bash scripts/tests/validate-release-config-test.sh - run: bash scripts/tests/dev-web-host-test.sh - run: bash scripts/tests/workflow-security-test.sh diff --git a/cli/src/platform/archive.ts b/cli/src/platform/archive.ts index c13067118..f549185df 100644 --- a/cli/src/platform/archive.ts +++ b/cli/src/platform/archive.ts @@ -19,8 +19,12 @@ export async function extractZip(buffer: ArrayBuffer, targetDir: string): Promis const archive = new Uint8Array(buffer) validateZipCentralDirectory(archive) const files = unzipSync(archive) - for (const [name, data] of Object.entries(files)) { - const filePath = safeJoin(targetDir, name) + const entries = Object.entries(files).map(([name, data]) => ({ + name, + data, + filePath: safeJoin(targetDir, name), + })) + for (const { name, data, filePath } of entries) { if (name.endsWith('/')) { await mkdir(filePath, { recursive: true }) continue diff --git a/cli/test/unit/platform/archive.test.ts b/cli/test/unit/platform/archive.test.ts index 98972134e..b175e279a 100644 --- a/cli/test/unit/platform/archive.test.ts +++ b/cli/test/unit/platform/archive.test.ts @@ -32,6 +32,17 @@ describe('archive helpers', () => { await expect(extractZip(unsafe.buffer as ArrayBuffer, target)).rejects.toThrow('unsafe zip entry path') }) + test('rejects unsafe zip before writing earlier safe entries', async () => { + const target = await mkdtemp(join(tmpdir(), 'skillhub-archive-partial-')) + const unsafe = zipSync({ + 'SKILL.md': new TextEncoder().encode('# Partial'), + '../escape.txt': new TextEncoder().encode('bad'), + }) + + await expect(extractZip(unsafe.buffer as ArrayBuffer, target)).rejects.toThrow('unsafe zip entry path') + await expect(readFile(join(target, 'SKILL.md'), 'utf-8')).rejects.toThrow() + }) + test('rejects zip entries with absolute paths', async () => { const target = await mkdtemp(join(tmpdir(), 'skillhub-archive-abs-')) const unsafe = zipSync({ '/etc/passwd': new TextEncoder().encode('bad') }) diff --git a/scripts/runtime.sh b/scripts/runtime.sh index f5b895506..f46e7c3e6 100755 --- a/scripts/runtime.sh +++ b/scripts/runtime.sh @@ -180,6 +180,42 @@ get_env_value() { fi } +generate_secret() { + if command -v openssl >/dev/null 2>&1; then + openssl rand -hex 32 + return 0 + fi + + if [ -r /dev/urandom ] && command -v od >/dev/null 2>&1; then + dd if=/dev/urandom bs=32 count=1 2>/dev/null | od -An -tx1 | tr -d ' \n' + return 0 + fi + + echo "Unable to generate SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET. Install openssl or configure it manually." >&2 + exit 1 +} + +is_placeholder_secret() { + case "$1" in + ""|change-me-in-production|replace-me|replace-with-random-download-secret-32-bytes|TODO*|todo*|replace*) + return 0 + ;; + *) + return 1 + ;; + esac +} + +ensure_anonymous_download_secret() { + secret="$(get_env_value "SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET" "")" + if ! is_placeholder_secret "$secret" && [ "${#secret}" -ge 32 ]; then + return 0 + fi + + set_env_value "SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET" "$(generate_secret)" + echo "Generated SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET in $ENV_FILE" +} + wait_for_postgres_ready() { postgres_user="$1" postgres_db="$2" @@ -280,6 +316,8 @@ prepare_runtime_files() { if [ -n "$SKILLHUB_PUBLIC_BASE_URL_VALUE" ]; then set_env_value "SKILLHUB_PUBLIC_BASE_URL" "$SKILLHUB_PUBLIC_BASE_URL_VALUE" fi + + ensure_anonymous_download_secret } run_compose() { diff --git a/scripts/tests/runtime-secret-test.sh b/scripts/tests/runtime-secret-test.sh new file mode 100755 index 000000000..34dd4fab5 --- /dev/null +++ b/scripts/tests/runtime-secret-test.sh @@ -0,0 +1,99 @@ +#!/usr/bin/env bash +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +SCRIPT="$REPO_ROOT/scripts/runtime.sh" + +TMP_DIRS=() +cleanup() { + local d + for d in "${TMP_DIRS[@]+"${TMP_DIRS[@]}"}"; do + rm -rf "$d" + done +} +trap cleanup EXIT + +new_tmp() { + local d + d="$(mktemp -d)" + TMP_DIRS+=("$d") + echo "$d" +} + +fail() { + echo "FAIL: $*" >&2 + exit 1 +} + +install_fake_tools() { + local bin_dir="$1" + mkdir -p "$bin_dir" + cat >"$bin_dir/docker" <<'EOF' +#!/usr/bin/env bash +set -euo pipefail +printf '%s\n' "$*" >> "${DOCKER_LOG:?DOCKER_LOG is required}" +if [[ "${1:-}" == "compose" && "${2:-}" == "version" ]]; then + exit 0 +fi +exit 0 +EOF + chmod +x "$bin_dir/docker" + + cat >"$bin_dir/openssl" <<'EOF' +#!/usr/bin/env bash +set -euo pipefail +if [[ "${1:-}" == "rand" && "${2:-}" == "-hex" && "${3:-}" == "32" ]]; then + printf '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef\n' + exit 0 +fi +echo "unsupported openssl args: $*" >&2 +exit 1 +EOF + chmod +x "$bin_dir/openssl" +} + +run_runtime() { + local home="$1" + local bin_dir="$2" + local stdout="$3" + DOCKER_LOG="$home/docker.log" \ + SKILLHUB_HOME="$home" \ + SKILLHUB_RAW_BASE="file://$REPO_ROOT" \ + PATH="$bin_dir:$PATH" \ + sh "$SCRIPT" up --version sha-test --public-url http://localhost >"$stdout" +} + +tmp="$(new_tmp)" +bin_dir="$tmp/bin" +install_fake_tools "$bin_dir" + +home_generated="$tmp/generated" +stdout_generated="$tmp/generated.out" +mkdir -p "$home_generated" +run_runtime "$home_generated" "$bin_dir" "$stdout_generated" + +generated_secret="$(grep '^SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET=' "$home_generated/.env.release" | cut -d= -f2-)" +[[ "$generated_secret" == "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" ]] \ + || fail "runtime should generate a persisted anonymous download secret" +grep -Fq "Generated SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET" "$stdout_generated" \ + || fail "runtime should explain that it generated the secret" +if grep -Fq "$generated_secret" "$stdout_generated"; then + fail "runtime must not print the generated secret value" +fi + +home_preserved="$tmp/preserved" +stdout_preserved="$tmp/preserved.out" +mkdir -p "$home_preserved" +cat >"$home_preserved/.env.release" <<'EOF' +SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET=already-valid-runtime-secret-32-bytes +EOF +run_runtime "$home_preserved" "$bin_dir" "$stdout_preserved" + +preserved_secret="$(grep '^SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET=' "$home_preserved/.env.release" | cut -d= -f2-)" +[[ "$preserved_secret" == "already-valid-runtime-secret-32-bytes" ]] \ + || fail "runtime must preserve an existing valid anonymous download secret" +if grep -Fq "Generated SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET" "$stdout_preserved"; then + fail "runtime must not regenerate an existing valid secret" +fi + +echo "runtime-secret-test passed" diff --git a/scripts/tests/validate-release-config-test.sh b/scripts/tests/validate-release-config-test.sh index 3a52d7a3f..fd76ddfcd 100755 --- a/scripts/tests/validate-release-config-test.sh +++ b/scripts/tests/validate-release-config-test.sh @@ -80,4 +80,17 @@ short_env="$tmp/short.env" write_env "$short_env" "too-short" expect_fail "$short_env" "SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET must be at least 32 characters" +draft_env="$tmp/draft.env" +while IFS= read -r line || [[ -n "$line" ]]; do + case "$line" in + SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET=*) + printf '%s\n' "SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET=release-download-secret-32-bytes-minimum" + ;; + *) + printf '%s\n' "$line" + ;; + esac +done <"$REPO_ROOT/.env.release.draft" >"$draft_env" +expect_fail "$draft_env" "POSTGRES_PASSWORD" + echo "validate-release-config-test passed" diff --git a/scripts/tests/workflow-security-test.sh b/scripts/tests/workflow-security-test.sh index 8219af51d..94cb25564 100755 --- a/scripts/tests/workflow-security-test.sh +++ b/scripts/tests/workflow-security-test.sh @@ -12,6 +12,13 @@ fail() { [[ -f "$SECURITY_WORKFLOW" ]] || fail ".github/workflows/security.yml is required" +grep -Eq '^permissions:[[:space:]]*$' "$PR_SCRIPTS_WORKFLOW" \ + || fail "pr-scripts must declare top-level permissions" +grep -Eq '^[[:space:]]+contents:[[:space:]]+read[[:space:]]*$' "$PR_SCRIPTS_WORKFLOW" \ + || fail "pr-scripts GITHUB_TOKEN permissions must be read-only" +grep -Fq 'persist-credentials: false' "$PR_SCRIPTS_WORKFLOW" \ + || fail "pr-scripts checkout must not persist credentials" + grep -Fq 'actions/dependency-review-action' "$SECURITY_WORKFLOW" \ || fail "security workflow must run dependency review" grep -Fq 'github/codeql-action/init' "$SECURITY_WORKFLOW" \ @@ -25,6 +32,8 @@ grep -Fq '.github/workflows/security.yml' "$PR_SCRIPTS_WORKFLOW" \ || fail "pr-scripts must run when security workflow changes" grep -Fq 'bash scripts/tests/validate-release-config-test.sh' "$PR_SCRIPTS_WORKFLOW" \ || fail "pr-scripts must run validate-release-config-test" +grep -Fq 'bash scripts/tests/runtime-secret-test.sh' "$PR_SCRIPTS_WORKFLOW" \ + || fail "pr-scripts must run runtime-secret-test" grep -Fq 'bash scripts/tests/dev-web-host-test.sh' "$PR_SCRIPTS_WORKFLOW" \ || fail "pr-scripts must run dev-web-host-test" grep -Fq 'bash scripts/tests/workflow-security-test.sh' "$PR_SCRIPTS_WORKFLOW" \ diff --git a/scripts/validate-release-config.sh b/scripts/validate-release-config.sh index 758b168ff..03e42bd8c 100755 --- a/scripts/validate-release-config.sh +++ b/scripts/validate-release-config.sh @@ -52,6 +52,23 @@ reject_values() { done } +reject_patterns() { + var_name="$1" + shift + eval "var_value=\${$var_name:-}" + if [ -z "$var_value" ]; then + return 0 + fi + for pattern in "$@"; do + case "$var_value" in + $pattern) + error "$var_name still uses placeholder/default pattern: $var_value" + return 0 + ;; + esac + done +} + validate_url() { var_name="$1" eval "var_value=\${$var_name:-}" @@ -115,15 +132,22 @@ validate_no_trailing_slash SKILLHUB_PUBLIC_BASE_URL require_non_empty SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET reject_values SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET "change-me-in-production" "replace-me" "replace-with-random-download-secret-32-bytes" +reject_patterns SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET "TODO_*" "todo_*" "replace*" validate_min_length SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET 32 reject_values POSTGRES_PASSWORD "change-this-postgres-password" "skillhub_demo" "skillhub_dev" +reject_patterns POSTGRES_PASSWORD "TODO_*" "todo_*" reject_values BOOTSTRAP_ADMIN_PASSWORD "replace-this-admin-password" "ChangeMe!2026" "Admin@2026" +reject_patterns BOOTSTRAP_ADMIN_PASSWORD "TODO_*" "todo_*" "replace*" if [ "${BOOTSTRAP_ADMIN_ENABLED:-false}" = "true" ]; then require_non_empty BOOTSTRAP_ADMIN_PASSWORD fi reject_values SKILLHUB_STORAGE_S3_ACCESS_KEY "replace-me" reject_values SKILLHUB_STORAGE_S3_SECRET_KEY "replace-me" +reject_patterns SKILLHUB_STORAGE_S3_ACCESS_KEY "TODO_*" "todo_*" "replace*" +reject_patterns SKILLHUB_STORAGE_S3_SECRET_KEY "TODO_*" "todo_*" "replace*" +reject_patterns SPRING_MAIL_USERNAME "TODO_*" "todo_*" "replace*" +reject_patterns SPRING_MAIL_PASSWORD "TODO_*" "todo_*" "replace*" validate_boolean SESSION_COOKIE_SECURE validate_boolean BOOTSTRAP_ADMIN_ENABLED From e50140272b6f18fca41865f8f80672c2ab3b502d Mon Sep 17 00:00:00 2001 From: dongmucat <1127093059@qq.com> Date: Tue, 16 Jun 2026 17:48:15 +0800 Subject: [PATCH 3/3] fix(security): close review hardening gaps Signed-off-by: dongmucat <1127093059@qq.com> --- .github/workflows/pr-cli.yml | 5 ++ .github/workflows/pr-e2e.yml | 2 + .github/workflows/pr-scripts.yml | 6 ++ .github/workflows/pr-tests.yml | 6 ++ .github/workflows/security.yml | 4 + cli/src/services/install-service.ts | 79 +++++++++++++------ .../unit/services/install-service.test.ts | 40 ++++++++++ scripts/runtime.sh | 62 +++++++++++++-- scripts/tests/runtime-secret-test.sh | 26 +++++- scripts/tests/workflow-security-test.sh | 33 ++++++-- .../skill/service/SkillDownloadService.java | 36 ++++++--- .../service/SkillDownloadServiceTest.java | 28 +++++++ 12 files changed, 278 insertions(+), 49 deletions(-) diff --git a/.github/workflows/pr-cli.yml b/.github/workflows/pr-cli.yml index 7de2a31e6..65c0ca6a4 100644 --- a/.github/workflows/pr-cli.yml +++ b/.github/workflows/pr-cli.yml @@ -7,6 +7,9 @@ on: - 'Makefile' - '.github/workflows/pr-cli.yml' +permissions: + contents: read + jobs: cli: strategy: @@ -16,6 +19,8 @@ jobs: runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - uses: oven-sh/setup-bun@v2 with: bun-version: 1.3.13 diff --git a/.github/workflows/pr-e2e.yml b/.github/workflows/pr-e2e.yml index f25d80728..f94ffdc29 100644 --- a/.github/workflows/pr-e2e.yml +++ b/.github/workflows/pr-e2e.yml @@ -34,6 +34,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v4 + with: + persist-credentials: false - name: Set up pnpm uses: pnpm/action-setup@v4 diff --git a/.github/workflows/pr-scripts.yml b/.github/workflows/pr-scripts.yml index d15ee4bdd..1e31dccfb 100644 --- a/.github/workflows/pr-scripts.yml +++ b/.github/workflows/pr-scripts.yml @@ -4,7 +4,13 @@ on: pull_request: paths: - 'scripts/**' + - '.env.release.example' + - '.env.release.draft' + - 'compose.release.yml' - 'Makefile' + - '.github/workflows/pr-cli.yml' + - '.github/workflows/pr-e2e.yml' + - '.github/workflows/pr-tests.yml' - '.github/workflows/security.yml' - '.github/workflows/pr-scripts.yml' diff --git a/.github/workflows/pr-tests.yml b/.github/workflows/pr-tests.yml index da4d7622a..4e957ec3f 100644 --- a/.github/workflows/pr-tests.yml +++ b/.github/workflows/pr-tests.yml @@ -25,6 +25,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v4 + with: + persist-credentials: false - name: Set up pnpm uses: pnpm/action-setup@v4 @@ -52,6 +54,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v4 + with: + persist-credentials: false - name: Set up Java uses: actions/setup-java@v4 @@ -74,6 +78,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v4 + with: + persist-credentials: false - name: Detect docs changes id: changed diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 9b476ca96..3329a267f 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -28,6 +28,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v4 + with: + persist-credentials: false - name: Review dependency changes uses: actions/dependency-review-action@v4 @@ -54,6 +56,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v4 + with: + persist-credentials: false - name: Set up Java if: matrix.language == 'java-kotlin' diff --git a/cli/src/services/install-service.ts b/cli/src/services/install-service.ts index ed09ffd69..4293ff6e0 100644 --- a/cli/src/services/install-service.ts +++ b/cli/src/services/install-service.ts @@ -1,4 +1,4 @@ -import { mkdir, rm, writeFile } from 'node:fs/promises' +import { mkdir, mkdtemp, rename, rm, writeFile } from 'node:fs/promises' import { join } from 'node:path' import { SkillHubClient } from '../clients/skillhub-client' import { InventoryStore } from '../stores/inventory-store' @@ -39,34 +39,61 @@ export async function installSkill(options: InstallOptions): Promise<{ installed }) } - if (await pathExists(skillDir) && options.force) { - await store.removeTargetsByInstallDir(skillDir) - await rm(skillDir, { recursive: true, force: true }) - } + await mkdir(target.rootDir, { recursive: true }) + const tempDir = await mkdtemp(join(target.rootDir, `.${options.slug}.install-`)) + let movedIntoPlace = false + + try { + await extractZip(buffer, tempDir) + + const installedAt = new Date().toISOString() + const metaDir = join(tempDir, '.skillhub') + await mkdir(metaDir, { recursive: true }) + await writeFile(join(metaDir, 'metadata.json'), JSON.stringify({ + registry: options.registry, + namespace: options.namespace, + slug: options.slug, + version: resolved.version, + agent: target.agent, + installedAt + }, null, 2)) - // Create skill directory and extract into a clean skill-specific directory. - await mkdir(skillDir, { recursive: true }) - await extractZip(buffer, skillDir) + if (await pathExists(skillDir) && !options.force) { + throw new CliError(`skill already installed at ${skillDir}`, EXIT.filesystem, { + path: skillDir, + next: 'pass --force to overwrite' + }) + } - // Write .skillhub/metadata.json - const metaDir = join(skillDir, '.skillhub') - await mkdir(metaDir, { recursive: true }) - await writeFile(join(metaDir, 'metadata.json'), JSON.stringify({ - registry: options.registry, - namespace: options.namespace, - slug: options.slug, - version: resolved.version, - agent: target.agent, - installedAt: new Date().toISOString() - }, null, 2)) + if (await pathExists(skillDir) && options.force) { + await store.removeTargetsByInstallDir(skillDir) + await rm(skillDir, { recursive: true, force: true }) + } - // Update inventory - await store.upsertTarget(options.registry, options.namespace, options.slug, resolved.version, { - agent: target.agent, - rootDir: target.rootDir, - installDir: skillDir, - installedAt: new Date().toISOString() - }) + try { + await rename(tempDir, skillDir) + } catch (error) { + if (!options.force && await pathExists(skillDir)) { + throw new CliError(`skill already installed at ${skillDir}`, EXIT.filesystem, { + path: skillDir, + next: 'pass --force to overwrite' + }) + } + throw error + } + movedIntoPlace = true + + await store.upsertTarget(options.registry, options.namespace, options.slug, resolved.version, { + agent: target.agent, + rootDir: target.rootDir, + installDir: skillDir, + installedAt + }) + } finally { + if (!movedIntoPlace) { + await rm(tempDir, { recursive: true, force: true }).catch(() => {}) + } + } installed.push({ agent: target.agent, dir: skillDir }) } diff --git a/cli/test/unit/services/install-service.test.ts b/cli/test/unit/services/install-service.test.ts index 784f4394f..fd08e1b65 100644 --- a/cli/test/unit/services/install-service.test.ts +++ b/cli/test/unit/services/install-service.test.ts @@ -132,6 +132,46 @@ describe('installSkill', () => { expect(inventory.items[0].targets[0].installDir).toBe(skillDir) }) + test('force keeps old installation and inventory when replacement extraction fails', async () => { + globalThis.fetch = installFetchWithDownloadResponse(new Response(new TextEncoder().encode('not a zip'), { status: 200 })) + const home = await mkdtemp(join(tmpdir(), 'skillhub-install-home-')) + const rootDir = await mkdtemp(join(tmpdir(), 'skillhub-install-root-')) + const skillDir = join(rootDir, 'demo') + await mkdir(skillDir, { recursive: true }) + await writeFile(join(skillDir, 'SKILL.md'), '# Old') + const inventoryPath = join(home, '.skillhub', 'inventory.json') + await mkdir(join(home, '.skillhub'), { recursive: true }) + await writeFile(inventoryPath, JSON.stringify({ + items: [{ + registry: 'http://registry.test', + namespace: 'global', + slug: 'demo', + version: '0.1.0', + targets: [{ + agent: 'codex', + rootDir, + installDir: skillDir, + installedAt: '2026-04-20T00:00:00.000Z' + }] + }] + }, null, 2)) + + await expect(installSkill({ + registry: 'http://registry.test', + namespace: 'global', + slug: 'demo', + targets: [{ agent: 'codex', rootDir, scope: 'project', source: 'explicit' }], + force: true, + home + })).rejects.toThrow('invalid zip central directory') + + expect(await readFile(join(skillDir, 'SKILL.md'), 'utf-8')).toBe('# Old') + const inventory = JSON.parse(await readFile(inventoryPath, 'utf-8')) + expect(inventory.items).toHaveLength(1) + expect(inventory.items[0]).toMatchObject({ namespace: 'global', slug: 'demo', version: '0.1.0' }) + expect(inventory.items[0].targets[0].installDir).toBe(skillDir) + }) + test('rejects downloads whose content-length exceeds the package limit', async () => { globalThis.fetch = installFetchWithDownloadResponse(new Response(new Uint8Array(0), { status: 200, diff --git a/scripts/runtime.sh b/scripts/runtime.sh index f46e7c3e6..fec6375e7 100755 --- a/scripts/runtime.sh +++ b/scripts/runtime.sh @@ -159,13 +159,34 @@ set_env_value() { fi tmp="$ENV_FILE.tmp" - if grep -q "^$key=" "$ENV_FILE"; then - sed "s|^$key=.*|$key=$value|" "$ENV_FILE" >"$tmp" - else - cat "$ENV_FILE" >"$tmp" - printf '%s=%s\n' "$key" "$value" >>"$tmp" - fi + found=false + old_umask="$(umask)" + umask 077 + { + while IFS= read -r line || [ -n "$line" ]; do + case "$line" in + "$key="*) + printf '%s=%s\n' "$key" "$value" + found=true + ;; + *) + printf '%s\n' "$line" + ;; + esac + done <"$ENV_FILE" + if [ "$found" = "false" ]; then + printf '%s=%s\n' "$key" "$value" + fi + } >"$tmp" + umask "$old_umask" mv "$tmp" "$ENV_FILE" + secure_env_file +} + +secure_env_file() { + if [ -f "$ENV_FILE" ]; then + chmod 600 "$ENV_FILE" + fi } get_env_value() { @@ -235,6 +256,23 @@ wait_for_postgres_ready() { exit 1 } +wait_for_redis_ready() { + attempt=1 + + while [ "$attempt" -le 60 ]; do + if run_compose exec -T redis redis-cli ping >/dev/null 2>&1; then + return 0 + fi + + attempt=$((attempt + 1)) + sleep 2 + done + + echo "Redis did not become ready in time." >&2 + run_compose logs redis >&2 || true + exit 1 +} + ensure_postgres_password_matches_env() { postgres_user="$(get_env_value "POSTGRES_USER" "skillhub")" postgres_db="$(get_env_value "POSTGRES_DB" "skillhub")" @@ -267,8 +305,12 @@ prepare_runtime_files() { download_file "$SKILLHUB_RAW_BASE/.env.release.example" "$ENV_EXAMPLE_FILE" if [ ! -f "$ENV_FILE" ]; then + old_umask="$(umask)" + umask 077 cp "$ENV_EXAMPLE_FILE" "$ENV_FILE" + umask "$old_umask" fi + secure_env_file if [ -n "$SKILLHUB_MIRROR_REGISTRY_VALUE" ]; then mirror_registry="${SKILLHUB_MIRROR_REGISTRY_VALUE%/}" @@ -317,6 +359,10 @@ prepare_runtime_files() { set_env_value "SKILLHUB_PUBLIC_BASE_URL" "$SKILLHUB_PUBLIC_BASE_URL_VALUE" fi + if [ "$DISABLE_SCANNER" = "true" ]; then + set_env_value "SKILLHUB_SECURITY_SCANNER_ENABLED" "false" + fi + ensure_anonymous_download_secret } @@ -333,7 +379,9 @@ case "$COMMAND" in run_compose up -d postgres ensure_postgres_password_matches_env if [ "$DISABLE_SCANNER" = "true" ]; then - SKILLHUB_SECURITY_SCANNER_ENABLED=false run_compose up -d --scale skill-scanner=0 + run_compose up -d redis + wait_for_redis_ready + SKILLHUB_SECURITY_SCANNER_ENABLED=false run_compose up -d --no-deps --scale skill-scanner=0 server web else run_compose up -d fi diff --git a/scripts/tests/runtime-secret-test.sh b/scripts/tests/runtime-secret-test.sh index 34dd4fab5..506b58fd8 100755 --- a/scripts/tests/runtime-secret-test.sh +++ b/scripts/tests/runtime-secret-test.sh @@ -56,11 +56,21 @@ run_runtime() { local home="$1" local bin_dir="$2" local stdout="$3" + shift 3 DOCKER_LOG="$home/docker.log" \ SKILLHUB_HOME="$home" \ SKILLHUB_RAW_BASE="file://$REPO_ROOT" \ PATH="$bin_dir:$PATH" \ - sh "$SCRIPT" up --version sha-test --public-url http://localhost >"$stdout" + sh "$SCRIPT" up --version sha-test --public-url http://localhost "$@" >"$stdout" +} + +file_mode() { + local file="$1" + if stat -c %a "$file" >/dev/null 2>&1; then + stat -c %a "$file" + else + stat -f %Lp "$file" + fi } tmp="$(new_tmp)" @@ -75,6 +85,8 @@ run_runtime "$home_generated" "$bin_dir" "$stdout_generated" generated_secret="$(grep '^SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET=' "$home_generated/.env.release" | cut -d= -f2-)" [[ "$generated_secret" == "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" ]] \ || fail "runtime should generate a persisted anonymous download secret" +[[ "$(file_mode "$home_generated/.env.release")" == "600" ]] \ + || fail "runtime env file must be readable only by the owner" grep -Fq "Generated SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET" "$stdout_generated" \ || fail "runtime should explain that it generated the secret" if grep -Fq "$generated_secret" "$stdout_generated"; then @@ -92,8 +104,20 @@ run_runtime "$home_preserved" "$bin_dir" "$stdout_preserved" preserved_secret="$(grep '^SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET=' "$home_preserved/.env.release" | cut -d= -f2-)" [[ "$preserved_secret" == "already-valid-runtime-secret-32-bytes" ]] \ || fail "runtime must preserve an existing valid anonymous download secret" +[[ "$(file_mode "$home_preserved/.env.release")" == "600" ]] \ + || fail "runtime env file must remain owner-readable only when an existing secret is preserved" if grep -Fq "Generated SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET" "$stdout_preserved"; then fail "runtime must not regenerate an existing valid secret" fi +home_no_scanner="$tmp/no-scanner" +stdout_no_scanner="$tmp/no-scanner.out" +mkdir -p "$home_no_scanner" +run_runtime "$home_no_scanner" "$bin_dir" "$stdout_no_scanner" --no-scanner + +grep -Fq "SKILLHUB_SECURITY_SCANNER_ENABLED=false" "$home_no_scanner/.env.release" \ + || fail "runtime should persist scanner disabled state for --no-scanner" +grep -Fq -- "up -d --no-deps --scale skill-scanner=0 server web" "$home_no_scanner/docker.log" \ + || fail "runtime --no-scanner should start server/web without waiting on scanner dependencies" + echo "runtime-secret-test passed" diff --git a/scripts/tests/workflow-security-test.sh b/scripts/tests/workflow-security-test.sh index 94cb25564..2d6deb43d 100755 --- a/scripts/tests/workflow-security-test.sh +++ b/scripts/tests/workflow-security-test.sh @@ -10,14 +10,23 @@ fail() { exit 1 } +assert_pr_workflow_hardened() { + local workflow="$1" + grep -Eq '^permissions:[[:space:]]*$' "$workflow" \ + || fail "$workflow must declare top-level permissions" + grep -Eq '^[[:space:]]+contents:[[:space:]]+read[[:space:]]*$' "$workflow" \ + || fail "$workflow GITHUB_TOKEN permissions must include contents: read" + grep -Fq 'persist-credentials: false' "$workflow" \ + || fail "$workflow checkout steps must not persist credentials" +} + [[ -f "$SECURITY_WORKFLOW" ]] || fail ".github/workflows/security.yml is required" -grep -Eq '^permissions:[[:space:]]*$' "$PR_SCRIPTS_WORKFLOW" \ - || fail "pr-scripts must declare top-level permissions" -grep -Eq '^[[:space:]]+contents:[[:space:]]+read[[:space:]]*$' "$PR_SCRIPTS_WORKFLOW" \ - || fail "pr-scripts GITHUB_TOKEN permissions must be read-only" -grep -Fq 'persist-credentials: false' "$PR_SCRIPTS_WORKFLOW" \ - || fail "pr-scripts checkout must not persist credentials" +assert_pr_workflow_hardened "$REPO_ROOT/.github/workflows/pr-cli.yml" +assert_pr_workflow_hardened "$REPO_ROOT/.github/workflows/pr-e2e.yml" +assert_pr_workflow_hardened "$REPO_ROOT/.github/workflows/pr-tests.yml" +assert_pr_workflow_hardened "$PR_SCRIPTS_WORKFLOW" +assert_pr_workflow_hardened "$SECURITY_WORKFLOW" grep -Fq 'actions/dependency-review-action' "$SECURITY_WORKFLOW" \ || fail "security workflow must run dependency review" @@ -30,6 +39,18 @@ grep -Fq 'security-events: write' "$SECURITY_WORKFLOW" \ grep -Fq '.github/workflows/security.yml' "$PR_SCRIPTS_WORKFLOW" \ || fail "pr-scripts must run when security workflow changes" +grep -Fq '.github/workflows/pr-cli.yml' "$PR_SCRIPTS_WORKFLOW" \ + || fail "pr-scripts must run when PR CLI workflow changes" +grep -Fq '.github/workflows/pr-e2e.yml' "$PR_SCRIPTS_WORKFLOW" \ + || fail "pr-scripts must run when PR E2E workflow changes" +grep -Fq '.github/workflows/pr-tests.yml' "$PR_SCRIPTS_WORKFLOW" \ + || fail "pr-scripts must run when PR Tests workflow changes" +grep -Fq '.env.release.example' "$PR_SCRIPTS_WORKFLOW" \ + || fail "pr-scripts must run when release env example changes" +grep -Fq '.env.release.draft' "$PR_SCRIPTS_WORKFLOW" \ + || fail "pr-scripts must run when release env draft changes" +grep -Fq 'compose.release.yml' "$PR_SCRIPTS_WORKFLOW" \ + || fail "pr-scripts must run when release compose changes" grep -Fq 'bash scripts/tests/validate-release-config-test.sh' "$PR_SCRIPTS_WORKFLOW" \ || fail "pr-scripts must run validate-release-config-test" grep -Fq 'bash scripts/tests/runtime-secret-test.sh' "$PR_SCRIPTS_WORKFLOW" \ diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillDownloadService.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillDownloadService.java index b53a6c63b..56b69dddf 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillDownloadService.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillDownloadService.java @@ -102,7 +102,7 @@ public DownloadResult downloadLatest( SkillVersion version = skillVersionRepository.findById(skill.getLatestVersionId()) .orElseThrow(() -> new DomainBadRequestException("error.skill.version.latest.notFound")); - return downloadVersion(skill, version); + return downloadVersion(skill, version, currentUserId, userNsRoles); } /** @@ -123,7 +123,7 @@ public DownloadResult downloadVersion( SkillVersion version = skillVersionRepository.findBySkillIdAndVersion(skill.getId(), versionStr) .orElseThrow(() -> new DomainBadRequestException("error.skill.version.notFound", versionStr)); - return downloadVersion(skill, version); + return downloadVersion(skill, version, currentUserId, userNsRoles); } /** @@ -150,7 +150,7 @@ public DownloadResult downloadByTag( SkillVersion version = skillVersionRepository.findById(tag.getVersionId()) .orElseThrow(() -> new DomainBadRequestException("error.skill.tag.version.notFound", tagName)); - return downloadVersion(skill, version); + return downloadVersion(skill, version, currentUserId, userNsRoles); } /** @@ -161,9 +161,12 @@ public DownloadResult downloadReviewVersion(Skill skill, SkillVersion version) { return buildDownloadResult(skill, version); } - private DownloadResult downloadVersion(Skill skill, SkillVersion version) { + private DownloadResult downloadVersion(Skill skill, + SkillVersion version, + String currentUserId, + Map userNsRoles) { assertPublishedAccessible(skill); - assertDownloadableVersion(skill, version); + assertDownloadableVersion(skill, version, currentUserId, userNsRoles); DownloadResult result = buildDownloadResult(skill, version); // Only increment download count for PUBLISHED versions @@ -304,18 +307,33 @@ private void assertPublishedAccessible(Skill skill) { /** * Asserts that the version can be downloaded. * - PUBLISHED: anyone with skill access can download - * - UPLOADED/PENDING_REVIEW: only skill owner can download + * - UPLOADED/PENDING_REVIEW: only skill owner or namespace admin can download */ - private void assertDownloadableVersion(Skill skill, SkillVersion version) { + private void assertDownloadableVersion(Skill skill, + SkillVersion version, + String currentUserId, + Map userNsRoles) { switch (version.getStatus()) { case PUBLISHED -> { // Anyone with skill access can download published versions } case UPLOADED, PENDING_REVIEW -> { - // Only owner can download UPLOADED/PENDING_REVIEW versions - // Note: This check is already done in assertCanDownload via visibilityChecker + if (!canManageSkillDraft(skill, currentUserId, userNsRoles)) { + throw new DomainForbiddenException("error.skill.access.denied", skill.getSlug()); + } } default -> throw new DomainBadRequestException("error.skill.version.notDownloadable", version.getVersion()); } } + + private boolean canManageSkillDraft(Skill skill, String currentUserId, Map userNsRoles) { + if (currentUserId == null) { + return false; + } + if (skill.getOwnerId().equals(currentUserId)) { + return true; + } + NamespaceRole role = userNsRoles == null ? null : userNsRoles.get(skill.getNamespaceId()); + return role == NamespaceRole.OWNER || role == NamespaceRole.ADMIN; + } } diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillDownloadServiceTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillDownloadServiceTest.java index 4bc20d344..ce84509df 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillDownloadServiceTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillDownloadServiceTest.java @@ -6,6 +6,7 @@ import com.iflytek.skillhub.domain.namespace.NamespaceRole; import com.iflytek.skillhub.domain.namespace.NamespaceType; import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException; +import com.iflytek.skillhub.domain.shared.exception.DomainForbiddenException; import com.iflytek.skillhub.domain.skill.*; import com.iflytek.skillhub.storage.ObjectMetadata; import com.iflytek.skillhub.storage.ObjectStorageService; @@ -351,6 +352,33 @@ void testDownloadVersion_AllowsAnonymousForTeamNamespacePublicSkill() throws Exc verify(eventPublisher).publishEvent(any(SkillDownloadedEvent.class)); } + @Test + void testDownloadVersion_RejectsAnonymousPendingReviewPublicSkill() throws Exception { + Namespace namespace = new Namespace("global", "Global", "system"); + setId(namespace, 1L); + namespace.setType(NamespaceType.GLOBAL); + + Skill skill = new Skill(1L, "demo-skill", "owner-1", SkillVisibility.PUBLIC); + setId(skill, 1L); + skill.setStatus(SkillStatus.ACTIVE); + skill.setLatestVersionId(10L); + + SkillVersion version = new SkillVersion(1L, "1.1.0", "owner-1"); + setId(version, 11L); + version.setStatus(SkillVersionStatus.PENDING_REVIEW); + + when(namespaceRepository.findBySlug("global")).thenReturn(Optional.of(namespace)); + when(skillRepository.findByNamespaceIdAndSlug(1L, "demo-skill")).thenReturn(List.of(skill)); + when(visibilityChecker.canAccess(skill, null, Map.of())).thenReturn(true); + when(skillVersionRepository.findBySkillIdAndVersion(1L, "1.1.0")).thenReturn(Optional.of(version)); + + assertThrows(DomainForbiddenException.class, () -> + service.downloadVersion("global", "demo-skill", "1.1.0", null, Map.of())); + verify(skillRepository, never()).incrementDownloadCount(anyLong()); + verify(skillVersionStatsRepository, never()).incrementDownloadCount(anyLong(), anyLong()); + verify(eventPublisher, never()).publishEvent(any(SkillDownloadedEvent.class)); + } + private void setId(Object entity, Long id) throws Exception { Field idField = entity.getClass().getDeclaredField("id"); idField.setAccessible(true);