fix(skills): harden freshness integrity checks

This commit is contained in:
Brennan Benson
2026-07-14 19:50:43 -07:00
parent b8877dda6c
commit aaba669a30
13 changed files with 201 additions and 49 deletions
@@ -18,6 +18,10 @@ function sha256(bytes) {
return createHash('sha256').update(bytes).digest('hex')
}
function compareCodeUnits(left, right) {
return left === right ? 0 : left < right ? -1 : 1
}
function gitObjectSha(kind, bytes) {
return createHash('sha1').update(`${kind} ${bytes.length}\0`).update(bytes).digest()
}
@@ -116,7 +120,9 @@ async function collectPackageFiles(packageRoot) {
async function visit(directory) {
const entries = await readdir(directory, { withFileTypes: true })
entries.sort((left, right) => left.name.localeCompare(right.name, 'en'))
// Why: build-time Node and packaged Electron may ship different ICU data;
// package identity order must use the same locale-independent comparison.
entries.sort((left, right) => compareCodeUnits(left.name, right.name))
for (const entry of entries) {
const absolutePath = path.join(directory, entry.name)
const relativePath = path.relative(packageRoot, absolutePath)
@@ -243,7 +249,7 @@ function compareManifestPaths(left, right) {
const rightParts = right.split('/')
const shared = Math.min(leftParts.length, rightParts.length)
for (let index = 0; index < shared; index += 1) {
const order = leftParts[index].localeCompare(rightParts[index], 'en')
const order = compareCodeUnits(leftParts[index], rightParts[index])
if (order !== 0) {
return order
}
@@ -331,9 +337,7 @@ function buildReleasedHistory() {
if (!packages) {
throw new Error(`Missing released skill tree ${skillsTreeSha} at ${tag}`)
}
for (const name of [...packages.keys()].sort((left, right) =>
left.localeCompare(right, 'en')
)) {
for (const name of [...packages.keys()].sort(compareCodeUnits)) {
const entries = packages.get(name)
const filesWithGitHashes = collectGitPackageFiles(skillsTreeSha, name, entries, blobs)
if (!filesWithGitHashes.some((file) => file.path === 'SKILL.md')) {
@@ -369,7 +373,7 @@ async function buildArtifacts(appVersion) {
const skillDirectories = (await readdir(SKILLS_ROOT, { withFileTypes: true }))
.filter((entry) => entry.isDirectory())
.map((entry) => entry.name)
.sort((left, right) => left.localeCompare(right, 'en'))
.sort(compareCodeUnits)
const currentSkills = []
for (const name of skillDirectories) {
const filesWithGitHashes = await collectPackageFiles(path.join(SKILLS_ROOT, name))
@@ -44,10 +44,11 @@ describe('skill bundle manifest generator', () => {
it('uses normalized text identity but exact executable identity', () => {
const skillFile = describeFile('SKILL.md', Buffer.from('line one\r\nline two\r\n'), false)
const executable = describeFile('run.sh', Buffer.from('#!/bin/sh\necho ok\n'), true)
const executable = describeFile('run.sh', Buffer.from('#!/bin/sh\r\necho ok\r\n'), true)
expect(skillFile.identitySha256).toBe(skillFile.textNormalizedSha256)
expect(skillFile.identitySha256).not.toBe(skillFile.exactSha256)
expect(executable.exactSha256).not.toBe(executable.textNormalizedSha256)
expect(executable.identitySha256).toBe(executable.exactSha256)
expect(packageDigest([skillFile, executable])).toMatch(/^[a-f0-9]{64}$/)
})
@@ -69,10 +70,10 @@ describe('skill bundle manifest generator', () => {
expect(sortManifestFiles(gitOrdered)).toEqual(walked)
expect(packageDigest(sortManifestFiles(gitOrdered))).toBe(packageDigest(walked))
expect(walked.map((file) => file.path)).toEqual([
'Zebra.md',
'apple.md',
'sub/inner.txt',
'sub.md',
'Zebra.md'
'sub.md'
])
})
@@ -210,6 +210,13 @@ try {
execSkills(['update', targetName, '--global'])
await assertCurrentCanonical(targetName)
const targetProviderAfter = await packageDigestAt(await realpath(targetProvider))
const targetProviderStat = await lstat(targetProvider)
if (shape === 'symlink' && !targetProviderStat.isSymbolicLink()) {
throw new Error(`${targetName} provider alias was replaced with an independent copy`)
}
if (shape === 'copy' && targetProviderStat.isSymbolicLink()) {
throw new Error(`${targetName} independent provider copy was replaced with an alias`)
}
if (shape === 'symlink' && targetProviderAfter !== currentSkill(targetName).packageDigest) {
throw new Error(`${targetName} provider alias did not converge with the canonical update`)
}
@@ -0,0 +1,37 @@
import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'
import { tmpdir } from 'node:os'
import { join, resolve } from 'node:path'
import { afterEach, describe, expect, it } from 'vitest'
import { loadSkillBundleArtifacts } from './skill-bundle-artifacts'
const temporaryDirectories: string[] = []
afterEach(async () => {
await Promise.all(temporaryDirectories.splice(0).map((root) => rm(root, { recursive: true })))
})
describe('skill bundle artifacts', () => {
it('rejects malformed nested release entries before building provenance', async () => {
const resourceRoot = await mkdtemp(join(tmpdir(), 'orca-skill-artifacts-'))
temporaryDirectories.push(resourceRoot)
const target = join(resourceRoot, 'skills')
const source = resolve('resources', 'skills')
await mkdir(target, { recursive: true })
const [manifest, registry, releaseMapping] = await Promise.all(
['current-manifest.json', 'snapshot-registry.json', 'release-mapping.json'].map((name) =>
readFile(join(source, name), 'utf8')
)
)
const malformedMapping = JSON.parse(releaseMapping)
malformedMapping.releases[0] = { appVersion: 'invalid' }
await Promise.all([
writeFile(join(target, 'current-manifest.json'), manifest),
writeFile(join(target, 'snapshot-registry.json'), registry),
writeFile(join(target, 'release-mapping.json'), JSON.stringify(malformedMapping))
])
await expect(loadSkillBundleArtifacts(resourceRoot)).rejects.toThrow(
'Invalid skill release mapping'
)
})
})
+100 -32
View File
@@ -1,6 +1,7 @@
import { app } from 'electron'
import { readFile } from 'node:fs/promises'
import { join, resolve } from 'node:path'
import { z, type ZodType } from 'zod'
import type {
SkillBundleManifest,
SkillKnownSnapshot,
@@ -16,18 +17,70 @@ export type SkillBundleArtifacts = {
releasedAppVersions: Record<string, Record<number, string>>
}
function assertSupportedSchema(
value: unknown,
label: string
): asserts value is { schemaVersion: 1 } {
if (
typeof value !== 'object' ||
value === null ||
!('schemaVersion' in value) ||
value.schemaVersion !== 1
) {
throw new Error(`Unsupported ${label} schema`)
const sha256Schema = z.string().regex(/^[a-f0-9]{64}$/)
const snapshotShape = {
releaseRevision: z.number().int().positive(),
packageDigest: sha256Schema,
gitTreeSha: z.string().regex(/^[a-f0-9]{40}$/),
files: z
.array(
z
.object({
path: z.string().min(1),
size: z.number().int().nonnegative(),
executable: z.boolean(),
classification: z.enum(['text', 'binary']),
exactSha256: sha256Schema,
textNormalizedSha256: sha256Schema.nullable(),
identitySha256: sha256Schema
})
.strict()
)
.min(1)
}
const knownSnapshotSchema = z.object(snapshotShape).strict()
const manifestSchema = z
.object({
schemaVersion: z.literal(1),
appVersion: z.string().min(1),
skills: z.array(
z
.object({
name: z.string().regex(/^[a-z0-9][a-z0-9._-]*$/),
sourcePath: z.string().min(1),
appVersion: z.string().min(1),
...snapshotShape
})
.strict()
)
})
.strict()
const registrySchema = z
.object({
schemaVersion: z.literal(1),
skills: z.record(z.string().min(1), z.array(knownSnapshotSchema).min(1))
})
.strict()
const releaseMappingSchema = z
.object({
schemaVersion: z.literal(1),
releases: z.array(
z
.object({
appVersion: z.string().min(1),
skills: z.record(z.string().min(1), z.number().int().positive())
})
.strict()
)
})
.strict()
function parseArtifact<T>(schema: ZodType<T>, value: unknown, label: string): T {
const result = schema.safeParse(value)
if (!result.success) {
throw new Error(`Invalid ${label}: ${result.error.issues[0]?.message ?? 'schema mismatch'}`)
}
return result.data
}
const artifactsByResourceRoot = new Map<string, Promise<SkillBundleArtifacts>>()
@@ -51,46 +104,61 @@ export function loadSkillBundleArtifacts(
async function readSkillBundleArtifacts(resourceRoot: string): Promise<SkillBundleArtifacts> {
const bundleRoot = join(resourceRoot, 'skills')
const [manifest, registry, releaseMapping] = await Promise.all([
const [manifestValue, registryValue, releaseMappingValue] = await Promise.all([
readFile(join(bundleRoot, 'current-manifest.json'), 'utf8').then(JSON.parse),
readFile(join(bundleRoot, 'snapshot-registry.json'), 'utf8').then(JSON.parse),
readFile(join(bundleRoot, 'release-mapping.json'), 'utf8').then(JSON.parse)
])
assertSupportedSchema(manifest, 'skill bundle manifest')
assertSupportedSchema(registry, 'skill snapshot registry')
assertSupportedSchema(releaseMapping, 'skill release mapping')
if (!('skills' in manifest) || !Array.isArray(manifest.skills)) {
throw new Error('Invalid skill bundle manifest')
}
if (!('appVersion' in manifest) || typeof manifest.appVersion !== 'string') {
throw new Error('Invalid skill bundle manifest')
}
if (!('skills' in registry) || typeof registry.skills !== 'object' || registry.skills === null) {
throw new Error('Invalid skill snapshot registry')
}
if (!('releases' in releaseMapping) || !Array.isArray(releaseMapping.releases)) {
throw new Error('Invalid skill release mapping')
const manifest: SkillBundleManifest = parseArtifact(
manifestSchema,
manifestValue,
'skill bundle manifest'
)
const registry: SkillSnapshotRegistry = parseArtifact(
registrySchema,
registryValue,
'skill snapshot registry'
)
const releaseMapping: SkillReleaseMapping = parseArtifact(
releaseMappingSchema,
releaseMappingValue,
'skill release mapping'
)
for (const current of manifest.skills) {
if (
current.appVersion !== manifest.appVersion ||
!registry.skills[current.name]?.some(
(snapshot) =>
snapshot.releaseRevision === current.releaseRevision &&
snapshot.packageDigest === current.packageDigest
)
) {
throw new Error(`Inconsistent current skill snapshot: ${current.name}`)
}
}
const releasedAppVersions: Record<string, Record<number, string>> = {}
for (const release of (releaseMapping as SkillReleaseMapping).releases) {
for (const release of releaseMapping.releases) {
for (const [name, revision] of Object.entries(release.skills)) {
if (!registry.skills[name]?.some((snapshot) => snapshot.releaseRevision === revision)) {
throw new Error(`Unknown released skill revision: ${name}@${revision}`)
}
releasedAppVersions[name] ??= {}
releasedAppVersions[name][revision] ??= release.appVersion
}
}
for (const current of (manifest as SkillBundleManifest).skills) {
for (const current of manifest.skills) {
releasedAppVersions[current.name] ??= {}
releasedAppVersions[current.name][current.releaseRevision] = current.appVersion
}
return {
manifest: manifest as SkillBundleManifest,
registry: registry as SkillSnapshotRegistry,
releaseMapping: releaseMapping as SkillReleaseMapping,
manifest,
registry,
releaseMapping,
// Why: newer-known classification needs every identity packaged with this
// build, while release mapping remains the provenance record for shipped revisions.
knownSnapshots: (registry as SkillSnapshotRegistry).skills,
knownSnapshots: registry.skills,
releasedAppVersions
}
}
@@ -30,7 +30,7 @@ function snapshot(releaseRevision: number, markdown: string): SkillKnownSnapshot
return {
releaseRevision,
packageDigest: skillPackageDigest([file]),
gitTreeSha: `tree-${releaseRevision}`,
gitTreeSha: releaseRevision.toString(16).padStart(40, '0'),
files: [file]
}
}
@@ -48,6 +48,16 @@ describe('skill package identity', () => {
expect(binary.classification).toBe('binary')
})
it('orders package files by locale-independent code units', async () => {
const root = await temporarySkill()
await writeFile(join(root, 'apple.md'), 'apple')
await writeFile(join(root, 'Zebra.md'), 'zebra')
const observed = await observeSkillPackage(root)
expect(observed.files.map((file) => file.path)).toEqual(['Zebra.md', 'apple.md'])
})
it('rejects links and bounded-observation overflows', async () => {
const root = await temporarySkill()
await writeFile(join(root, 'SKILL.md'), 'skill')
+7 -1
View File
@@ -63,6 +63,10 @@ function sha256(bytes: Buffer): string {
return createHash('sha256').update(bytes).digest('hex')
}
function compareCodeUnits(left: string, right: string): number {
return left === right ? 0 : left < right ? -1 : 1
}
function normalizedText(bytes: Buffer): Buffer {
const text = new TextDecoder('utf-8', { fatal: true }).decode(bytes)
return Buffer.from(text.replace(/\r\n/g, '\n').replace(/\r/g, '\n'), 'utf8')
@@ -154,7 +158,9 @@ export async function observeSkillPackage(
} finally {
await directoryHandle.close().catch(() => undefined)
}
entries.sort((left, right) => left.name.localeCompare(right.name, 'en'))
// Why: runtime Electron and the build's Node may carry different ICU data;
// identity order must match the generator without locale-sensitive collation.
entries.sort((left, right) => compareCodeUnits(left.name, right.name))
for (const entry of entries) {
const absolutePath = join(directory, entry.name)
const relativePath = relative(packageRoot, absolutePath)
@@ -23,4 +23,18 @@ describe('plugin skill candidate scan', () => {
expect(result.candidates).toHaveLength(1)
expect(result.incompletePaths).toEqual([root])
})
it('marks depth-truncated subtrees incomplete so hidden skills poison eligibility', async () => {
const root = await mkdtemp(join(tmpdir(), 'orca-plugin-skill-depth-'))
temporaryDirectories.push(root)
const segments = Array.from({ length: 11 }, (_, index) => `level-${index}`)
const hiddenSkill = join(root, ...segments, 'orca-cli')
await mkdir(hiddenSkill, { recursive: true })
const result = await scanKnownPluginSkillCandidates(root, new Set(['orca-cli']))
expect(result.candidates).toEqual([])
expect(result.incompletePaths).toHaveLength(1)
expect(hiddenSkill.startsWith(result.incompletePaths[0] ?? '')).toBe(true)
})
})
+6 -2
View File
@@ -50,7 +50,11 @@ export async function scanKnownPluginSkillCandidates(
}
async function visit(directory: string, depth: number): Promise<void> {
if (limitReached || depth > MAXIMUM_PLUGIN_SCAN_DEPTH) {
if (limitReached) {
return
}
if (depth > MAXIMUM_PLUGIN_SCAN_DEPTH) {
recordIncomplete(directory)
return
}
let resolved: string
@@ -95,7 +99,7 @@ export async function scanKnownPluginSkillCandidates(
await handle.close().catch(() => undefined)
}
entries.sort((left, right) => left.name.localeCompare(right.name, 'en'))
entries.sort((left, right) => (left.name === right.name ? 0 : left.name < right.name ? -1 : 1))
for (const entry of entries) {
if (limitReached) {
return
@@ -0,0 +1 @@
export const INSTALLED_AGENT_SKILLS_CHANGED_EVENT = 'orca:installed-agent-skills-changed'
@@ -7,9 +7,9 @@ import type {
} from '../../../shared/skills'
import { ORCHESTRATION_SKILL_NAME } from '@/lib/agent-feature-install-commands'
import { markOrchestrationSetupComplete } from '@/lib/orchestration-setup-state'
import { INSTALLED_AGENT_SKILLS_CHANGED_EVENT } from './installed-agent-skills-change-event'
import { useMountedRef } from './useMountedRef'
const INSTALLED_AGENT_SKILLS_CHANGED_EVENT = 'orca:installed-agent-skills-changed'
export const GLOBAL_AGENT_SKILL_SOURCE_KINDS = [
'home'
] as const satisfies readonly SkillSourceKind[]
+3 -3
View File
@@ -1,8 +1,8 @@
import { useCallback, useEffect, useState } from 'react'
import type { SkillFreshnessInventory } from '../../../shared/skill-freshness'
import { INSTALLED_AGENT_SKILLS_CHANGED_EVENT } from './installed-agent-skills-change-event'
import { useMountedRef } from './useMountedRef'
const INSTALLED_SKILLS_CHANGED_EVENT = 'orca:installed-agent-skills-changed'
// Why: window focus fires on every alt-tab, and each scan re-reads and re-hashes
// every installed package; a just-completed scan stays authoritative briefly.
const FOCUS_RESCAN_COOLDOWN_MS = 15_000
@@ -100,10 +100,10 @@ export function useSkillFreshness(): SkillFreshnessState {
const onFocus = (): void => invalidate(true)
const onInstalledSkillsChanged = (): void => invalidate(false)
window.addEventListener('focus', onFocus)
window.addEventListener(INSTALLED_SKILLS_CHANGED_EVENT, onInstalledSkillsChanged)
window.addEventListener(INSTALLED_AGENT_SKILLS_CHANGED_EVENT, onInstalledSkillsChanged)
return () => {
window.removeEventListener('focus', onFocus)
window.removeEventListener(INSTALLED_SKILLS_CHANGED_EVENT, onInstalledSkillsChanged)
window.removeEventListener(INSTALLED_AGENT_SKILLS_CHANGED_EVENT, onInstalledSkillsChanged)
}
}, [refresh])