Skip to content

Commit e501402

Browse files
dongmucatdongmucat
committed
fix(security): close review hardening gaps
Signed-off-by: dongmucat <1127093059@qq.com>
1 parent 7d0402e commit e501402

12 files changed

Lines changed: 278 additions & 49 deletions

File tree

‎.github/workflows/pr-cli.yml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,9 @@ on:
77
- 'Makefile'
88
- '.github/workflows/pr-cli.yml'
99

10+
permissions:
11+
contents: read
12+
1013
jobs:
1114
cli:
1215
strategy:
@@ -16,6 +19,8 @@ jobs:
1619
runs-on: ${{ matrix.os }}
1720
steps:
1821
- uses: actions/checkout@v4
22+
with:
23+
persist-credentials: false
1924
- uses: oven-sh/setup-bun@v2
2025
with:
2126
bun-version: 1.3.13

‎.github/workflows/pr-e2e.yml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ jobs:
3434
steps:
3535
- name: Check out repository
3636
uses: actions/checkout@v4
37+
with:
38+
persist-credentials: false
3739

3840
- name: Set up pnpm
3941
uses: pnpm/action-setup@v4

‎.github/workflows/pr-scripts.yml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,13 @@ on:
44
pull_request:
55
paths:
66
- 'scripts/**'
7+
- '.env.release.example'
8+
- '.env.release.draft'
9+
- 'compose.release.yml'
710
- 'Makefile'
11+
- '.github/workflows/pr-cli.yml'
12+
- '.github/workflows/pr-e2e.yml'
13+
- '.github/workflows/pr-tests.yml'
814
- '.github/workflows/security.yml'
915
- '.github/workflows/pr-scripts.yml'
1016

‎.github/workflows/pr-tests.yml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ jobs:
2525
steps:
2626
- name: Check out repository
2727
uses: actions/checkout@v4
28+
with:
29+
persist-credentials: false
2830

2931
- name: Set up pnpm
3032
uses: pnpm/action-setup@v4
@@ -52,6 +54,8 @@ jobs:
5254
steps:
5355
- name: Check out repository
5456
uses: actions/checkout@v4
57+
with:
58+
persist-credentials: false
5559

5660
- name: Set up Java
5761
uses: actions/setup-java@v4
@@ -74,6 +78,8 @@ jobs:
7478
steps:
7579
- name: Check out repository
7680
uses: actions/checkout@v4
81+
with:
82+
persist-credentials: false
7783

7884
- name: Detect docs changes
7985
id: changed

‎.github/workflows/security.yml‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@ jobs:
2828
steps:
2929
- name: Check out repository
3030
uses: actions/checkout@v4
31+
with:
32+
persist-credentials: false
3133

3234
- name: Review dependency changes
3335
uses: actions/dependency-review-action@v4
@@ -54,6 +56,8 @@ jobs:
5456
steps:
5557
- name: Check out repository
5658
uses: actions/checkout@v4
59+
with:
60+
persist-credentials: false
5761

5862
- name: Set up Java
5963
if: matrix.language == 'java-kotlin'

‎cli/src/services/install-service.ts‎

Lines changed: 53 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { mkdir, rm, writeFile } from 'node:fs/promises'
1+
import { mkdir, mkdtemp, rename, rm, writeFile } from 'node:fs/promises'
22
import { join } from 'node:path'
33
import { SkillHubClient } from '../clients/skillhub-client'
44
import { InventoryStore } from '../stores/inventory-store'
@@ -39,34 +39,61 @@ export async function installSkill(options: InstallOptions): Promise<{ installed
3939
})
4040
}
4141

42-
if (await pathExists(skillDir) && options.force) {
43-
await store.removeTargetsByInstallDir(skillDir)
44-
await rm(skillDir, { recursive: true, force: true })
45-
}
42+
await mkdir(target.rootDir, { recursive: true })
43+
const tempDir = await mkdtemp(join(target.rootDir, `.${options.slug}.install-`))
44+
let movedIntoPlace = false
45+
46+
try {
47+
await extractZip(buffer, tempDir)
48+
49+
const installedAt = new Date().toISOString()
50+
const metaDir = join(tempDir, '.skillhub')
51+
await mkdir(metaDir, { recursive: true })
52+
await writeFile(join(metaDir, 'metadata.json'), JSON.stringify({
53+
registry: options.registry,
54+
namespace: options.namespace,
55+
slug: options.slug,
56+
version: resolved.version,
57+
agent: target.agent,
58+
installedAt
59+
}, null, 2))
4660

47-
// Create skill directory and extract into a clean skill-specific directory.
48-
await mkdir(skillDir, { recursive: true })
49-
await extractZip(buffer, skillDir)
61+
if (await pathExists(skillDir) && !options.force) {
62+
throw new CliError(`skill already installed at ${skillDir}`, EXIT.filesystem, {
63+
path: skillDir,
64+
next: 'pass --force to overwrite'
65+
})
66+
}
5067

51-
// Write .skillhub/metadata.json
52-
const metaDir = join(skillDir, '.skillhub')
53-
await mkdir(metaDir, { recursive: true })
54-
await writeFile(join(metaDir, 'metadata.json'), JSON.stringify({
55-
registry: options.registry,
56-
namespace: options.namespace,
57-
slug: options.slug,
58-
version: resolved.version,
59-
agent: target.agent,
60-
installedAt: new Date().toISOString()
61-
}, null, 2))
68+
if (await pathExists(skillDir) && options.force) {
69+
await store.removeTargetsByInstallDir(skillDir)
70+
await rm(skillDir, { recursive: true, force: true })
71+
}
6272

63-
// Update inventory
64-
await store.upsertTarget(options.registry, options.namespace, options.slug, resolved.version, {
65-
agent: target.agent,
66-
rootDir: target.rootDir,
67-
installDir: skillDir,
68-
installedAt: new Date().toISOString()
69-
})
73+
try {
74+
await rename(tempDir, skillDir)
75+
} catch (error) {
76+
if (!options.force && await pathExists(skillDir)) {
77+
throw new CliError(`skill already installed at ${skillDir}`, EXIT.filesystem, {
78+
path: skillDir,
79+
next: 'pass --force to overwrite'
80+
})
81+
}
82+
throw error
83+
}
84+
movedIntoPlace = true
85+
86+
await store.upsertTarget(options.registry, options.namespace, options.slug, resolved.version, {
87+
agent: target.agent,
88+
rootDir: target.rootDir,
89+
installDir: skillDir,
90+
installedAt
91+
})
92+
} finally {
93+
if (!movedIntoPlace) {
94+
await rm(tempDir, { recursive: true, force: true }).catch(() => {})
95+
}
96+
}
7097

7198
installed.push({ agent: target.agent, dir: skillDir })
7299
}

‎cli/test/unit/services/install-service.test.ts‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,46 @@ describe('installSkill', () => {
132132
expect(inventory.items[0].targets[0].installDir).toBe(skillDir)
133133
})
134134

135+
test('force keeps old installation and inventory when replacement extraction fails', async () => {
136+
globalThis.fetch = installFetchWithDownloadResponse(new Response(new TextEncoder().encode('not a zip'), { status: 200 }))
137+
const home = await mkdtemp(join(tmpdir(), 'skillhub-install-home-'))
138+
const rootDir = await mkdtemp(join(tmpdir(), 'skillhub-install-root-'))
139+
const skillDir = join(rootDir, 'demo')
140+
await mkdir(skillDir, { recursive: true })
141+
await writeFile(join(skillDir, 'SKILL.md'), '# Old')
142+
const inventoryPath = join(home, '.skillhub', 'inventory.json')
143+
await mkdir(join(home, '.skillhub'), { recursive: true })
144+
await writeFile(inventoryPath, JSON.stringify({
145+
items: [{
146+
registry: 'http://registry.test',
147+
namespace: 'global',
148+
slug: 'demo',
149+
version: '0.1.0',
150+
targets: [{
151+
agent: 'codex',
152+
rootDir,
153+
installDir: skillDir,
154+
installedAt: '2026-04-20T00:00:00.000Z'
155+
}]
156+
}]
157+
}, null, 2))
158+
159+
await expect(installSkill({
160+
registry: 'http://registry.test',
161+
namespace: 'global',
162+
slug: 'demo',
163+
targets: [{ agent: 'codex', rootDir, scope: 'project', source: 'explicit' }],
164+
force: true,
165+
home
166+
})).rejects.toThrow('invalid zip central directory')
167+
168+
expect(await readFile(join(skillDir, 'SKILL.md'), 'utf-8')).toBe('# Old')
169+
const inventory = JSON.parse(await readFile(inventoryPath, 'utf-8'))
170+
expect(inventory.items).toHaveLength(1)
171+
expect(inventory.items[0]).toMatchObject({ namespace: 'global', slug: 'demo', version: '0.1.0' })
172+
expect(inventory.items[0].targets[0].installDir).toBe(skillDir)
173+
})
174+
135175
test('rejects downloads whose content-length exceeds the package limit', async () => {
136176
globalThis.fetch = installFetchWithDownloadResponse(new Response(new Uint8Array(0), {
137177
status: 200,

‎scripts/runtime.sh‎

Lines changed: 55 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -159,13 +159,34 @@ set_env_value() {
159159
fi
160160

161161
tmp="$ENV_FILE.tmp"
162-
if grep -q "^$key=" "$ENV_FILE"; then
163-
sed "s|^$key=.*|$key=$value|" "$ENV_FILE" >"$tmp"
164-
else
165-
cat "$ENV_FILE" >"$tmp"
166-
printf '%s=%s\n' "$key" "$value" >>"$tmp"
167-
fi
162+
found=false
163+
old_umask="$(umask)"
164+
umask 077
165+
{
166+
while IFS= read -r line || [ -n "$line" ]; do
167+
case "$line" in
168+
"$key="*)
169+
printf '%s=%s\n' "$key" "$value"
170+
found=true
171+
;;
172+
*)
173+
printf '%s\n' "$line"
174+
;;
175+
esac
176+
done <"$ENV_FILE"
177+
if [ "$found" = "false" ]; then
178+
printf '%s=%s\n' "$key" "$value"
179+
fi
180+
} >"$tmp"
181+
umask "$old_umask"
168182
mv "$tmp" "$ENV_FILE"
183+
secure_env_file
184+
}
185+
186+
secure_env_file() {
187+
if [ -f "$ENV_FILE" ]; then
188+
chmod 600 "$ENV_FILE"
189+
fi
169190
}
170191

171192
get_env_value() {
@@ -235,6 +256,23 @@ wait_for_postgres_ready() {
235256
exit 1
236257
}
237258

259+
wait_for_redis_ready() {
260+
attempt=1
261+
262+
while [ "$attempt" -le 60 ]; do
263+
if run_compose exec -T redis redis-cli ping >/dev/null 2>&1; then
264+
return 0
265+
fi
266+
267+
attempt=$((attempt + 1))
268+
sleep 2
269+
done
270+
271+
echo "Redis did not become ready in time." >&2
272+
run_compose logs redis >&2 || true
273+
exit 1
274+
}
275+
238276
ensure_postgres_password_matches_env() {
239277
postgres_user="$(get_env_value "POSTGRES_USER" "skillhub")"
240278
postgres_db="$(get_env_value "POSTGRES_DB" "skillhub")"
@@ -267,8 +305,12 @@ prepare_runtime_files() {
267305
download_file "$SKILLHUB_RAW_BASE/.env.release.example" "$ENV_EXAMPLE_FILE"
268306

269307
if [ ! -f "$ENV_FILE" ]; then
308+
old_umask="$(umask)"
309+
umask 077
270310
cp "$ENV_EXAMPLE_FILE" "$ENV_FILE"
311+
umask "$old_umask"
271312
fi
313+
secure_env_file
272314

273315
if [ -n "$SKILLHUB_MIRROR_REGISTRY_VALUE" ]; then
274316
mirror_registry="${SKILLHUB_MIRROR_REGISTRY_VALUE%/}"
@@ -317,6 +359,10 @@ prepare_runtime_files() {
317359
set_env_value "SKILLHUB_PUBLIC_BASE_URL" "$SKILLHUB_PUBLIC_BASE_URL_VALUE"
318360
fi
319361

362+
if [ "$DISABLE_SCANNER" = "true" ]; then
363+
set_env_value "SKILLHUB_SECURITY_SCANNER_ENABLED" "false"
364+
fi
365+
320366
ensure_anonymous_download_secret
321367
}
322368

@@ -333,7 +379,9 @@ case "$COMMAND" in
333379
run_compose up -d postgres
334380
ensure_postgres_password_matches_env
335381
if [ "$DISABLE_SCANNER" = "true" ]; then
336-
SKILLHUB_SECURITY_SCANNER_ENABLED=false run_compose up -d --scale skill-scanner=0
382+
run_compose up -d redis
383+
wait_for_redis_ready
384+
SKILLHUB_SECURITY_SCANNER_ENABLED=false run_compose up -d --no-deps --scale skill-scanner=0 server web
337385
else
338386
run_compose up -d
339387
fi

‎scripts/tests/runtime-secret-test.sh‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,11 +56,21 @@ run_runtime() {
5656
local home="$1"
5757
local bin_dir="$2"
5858
local stdout="$3"
59+
shift 3
5960
DOCKER_LOG="$home/docker.log" \
6061
SKILLHUB_HOME="$home" \
6162
SKILLHUB_RAW_BASE="file://$REPO_ROOT" \
6263
PATH="$bin_dir:$PATH" \
63-
sh "$SCRIPT" up --version sha-test --public-url http://localhost >"$stdout"
64+
sh "$SCRIPT" up --version sha-test --public-url http://localhost "$@" >"$stdout"
65+
}
66+
67+
file_mode() {
68+
local file="$1"
69+
if stat -c %a "$file" >/dev/null 2>&1; then
70+
stat -c %a "$file"
71+
else
72+
stat -f %Lp "$file"
73+
fi
6474
}
6575

6676
tmp="$(new_tmp)"
@@ -75,6 +85,8 @@ run_runtime "$home_generated" "$bin_dir" "$stdout_generated"
7585
generated_secret="$(grep '^SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET=' "$home_generated/.env.release" | cut -d= -f2-)"
7686
[[ "$generated_secret" == "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" ]] \
7787
|| fail "runtime should generate a persisted anonymous download secret"
88+
[[ "$(file_mode "$home_generated/.env.release")" == "600" ]] \
89+
|| fail "runtime env file must be readable only by the owner"
7890
grep -Fq "Generated SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET" "$stdout_generated" \
7991
|| fail "runtime should explain that it generated the secret"
8092
if grep -Fq "$generated_secret" "$stdout_generated"; then
@@ -92,8 +104,20 @@ run_runtime "$home_preserved" "$bin_dir" "$stdout_preserved"
92104
preserved_secret="$(grep '^SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET=' "$home_preserved/.env.release" | cut -d= -f2-)"
93105
[[ "$preserved_secret" == "already-valid-runtime-secret-32-bytes" ]] \
94106
|| fail "runtime must preserve an existing valid anonymous download secret"
107+
[[ "$(file_mode "$home_preserved/.env.release")" == "600" ]] \
108+
|| fail "runtime env file must remain owner-readable only when an existing secret is preserved"
95109
if grep -Fq "Generated SKILLHUB_DOWNLOAD_ANON_COOKIE_SECRET" "$stdout_preserved"; then
96110
fail "runtime must not regenerate an existing valid secret"
97111
fi
98112

113+
home_no_scanner="$tmp/no-scanner"
114+
stdout_no_scanner="$tmp/no-scanner.out"
115+
mkdir -p "$home_no_scanner"
116+
run_runtime "$home_no_scanner" "$bin_dir" "$stdout_no_scanner" --no-scanner
117+
118+
grep -Fq "SKILLHUB_SECURITY_SCANNER_ENABLED=false" "$home_no_scanner/.env.release" \
119+
|| fail "runtime should persist scanner disabled state for --no-scanner"
120+
grep -Fq -- "up -d --no-deps --scale skill-scanner=0 server web" "$home_no_scanner/docker.log" \
121+
|| fail "runtime --no-scanner should start server/web without waiting on scanner dependencies"
122+
99123
echo "runtime-secret-test passed"

0 commit comments

Comments
 (0)