fix(security): close review hardening gaps

Signed-off-by: dongmucat <1127093059@qq.com>

.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

.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

.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'
 

.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

.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'

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,35 +39,62 @@ 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))
+
+      if (await pathExists(skillDir) && !options.force) {
+        throw new CliError(`skill already installed at ${skillDir}`, EXIT.filesystem, {
+          path: skillDir,
+          next: 'pass --force to overwrite'
+        })
+      }
+
+      if (await pathExists(skillDir) && options.force) {
+        await store.removeTargetsByInstallDir(skillDir)
+        await rm(skillDir, { recursive: true, force: true })
+      }
+
+      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(() => {})
+      }
     }
 
-    // Create skill directory and extract into a clean skill-specific directory.
-    await mkdir(skillDir, { recursive: true })
-    await extractZip(buffer, skillDir)
-
-    // 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))
-
-    // 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()
-    })
-
     installed.push({ agent: target.agent, dir: skillDir })
   }
 

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,

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

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"

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" \

server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillDownloadService.java

@@ -102,7 +102,7 @@ public class SkillDownloadService {
         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 class SkillDownloadService {
         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 class SkillDownloadService {
         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 class SkillDownloadService {
         return buildDownloadResult(skill, version);
     }
 
-    private DownloadResult downloadVersion(Skill skill, SkillVersion version) {
+    private DownloadResult downloadVersion(Skill skill,
+                                           SkillVersion version,
+                                           String currentUserId,
+                                           Map<Long, NamespaceRole> 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 @@ public class SkillDownloadService {
     /**
      * 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<Long, NamespaceRole> 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<Long, NamespaceRole> 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;
+    }
 }

server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillDownloadServiceTest.java

@@ -6,6 +6,7 @@ import com.iflytek.skillhub.domain.namespace.NamespaceRepository;
 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 @@ class SkillDownloadServiceTest {
         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);