Skip to content

Commit 11f3a35

Browse files
zerob13zerob13
committed
fix(skill): harden draft handles
1 parent a3f09e2 commit 11f3a35

File tree

6 files changed

+154
-45
lines changed

6 files changed

+154
-45
lines changed

‎src/main/presenter/skillPresenter/index.ts‎

Lines changed: 90 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { app, shell } from 'electron'
22
import path from 'path'
33
import fs from 'fs'
4+
import { randomUUID } from 'node:crypto'
45
import { FSWatcher, watch } from 'chokidar'
56
import matter from 'gray-matter'
67
import { unzipSync } from 'fflate'
@@ -101,6 +102,7 @@ const BINARY_LIKE_EXTENSIONS = new Set([
101102
])
102103
const DRAFT_ALLOWED_TOP_LEVEL_DIRS = new Set(['references', 'templates', 'scripts', 'assets'])
103104
const DRAFT_CONVERSATION_ID_PATTERN = /^[A-Za-z0-9._-]+$/
105+
const DRAFT_ID_PATTERN = /^[A-Za-z0-9._-]+$/
104106
const DRAFT_INJECTION_PATTERNS = [
105107
/ignore\s+previous\s+instructions/i,
106108
/disregard\s+all\s+prior/i,
@@ -280,7 +282,11 @@ export class SkillPresenter implements ISkillPresenter {
280282
return []
281283
}
282284

283-
for (const skillPath of this.collectSkillManifestPaths(this.skillsDir)) {
285+
const skillManifestPaths = [...this.collectSkillManifestPaths(this.skillsDir)].sort(
286+
(left, right) => left.localeCompare(right)
287+
)
288+
289+
for (const skillPath of skillManifestPaths) {
284290
const dirName = path.basename(path.dirname(skillPath))
285291
try {
286292
const metadata = await this.parseSkillMetadata(skillPath, dirName)
@@ -578,36 +584,52 @@ export class SkillPresenter implements ISkillPresenter {
578584
if (!parsed.success) {
579585
return { success: false, action, error: parsed.error }
580586
}
581-
const draftPath = this.createDraftDirectory(conversationId, parsed.skillName)
587+
const { draftId, draftPath } = this.createDraftHandle(conversationId)
582588
this.atomicWriteFile(path.join(draftPath, 'SKILL.md'), request.content!)
583-
return { success: true, action, draftPath, skillName: parsed.skillName }
589+
return { success: true, action, draftId, skillName: parsed.skillName }
584590
}
585591
case 'edit': {
586592
const parsed = this.validateDraftSkillDocument(request.content)
587593
if (!parsed.success) {
588594
return { success: false, action, error: parsed.error }
589595
}
590-
const draftPath = this.resolveDraftPath(conversationId, request.draftPath)
596+
const draftId = this.validateDraftId(request.draftId)
597+
if (!draftId) {
598+
return {
599+
success: false,
600+
action,
601+
error: 'Draft handle is invalid for this conversation'
602+
}
603+
}
604+
const draftPath = this.getDraftPathForId(conversationId, draftId)
591605
if (!draftPath) {
592606
return {
593607
success: false,
594608
action,
595-
error: 'Draft path is invalid or outside the draft root'
609+
error: 'Draft handle is invalid for this conversation'
596610
}
597611
}
598612
if (!fs.existsSync(draftPath)) {
599-
return { success: false, action, error: 'Draft path not found' }
613+
return { success: false, action, error: 'Draft not found' }
600614
}
601615
this.atomicWriteFile(path.join(draftPath, 'SKILL.md'), request.content!)
602-
return { success: true, action, draftPath, skillName: parsed.skillName }
616+
return { success: true, action, draftId, skillName: parsed.skillName }
603617
}
604618
case 'write_file': {
605-
const draftPath = this.resolveDraftPath(conversationId, request.draftPath)
619+
const draftId = this.validateDraftId(request.draftId)
620+
if (!draftId) {
621+
return {
622+
success: false,
623+
action,
624+
error: 'Draft handle is invalid for this conversation'
625+
}
626+
}
627+
const draftPath = this.getDraftPathForId(conversationId, draftId)
606628
if (!draftPath) {
607629
return {
608630
success: false,
609631
action,
610-
error: 'Draft path is invalid or outside the draft root'
632+
error: 'Draft handle is invalid for this conversation'
611633
}
612634
}
613635
if (!request.filePath?.trim()) {
@@ -637,17 +659,25 @@ export class SkillPresenter implements ISkillPresenter {
637659
return {
638660
success: true,
639661
action,
640-
draftPath,
662+
draftId,
641663
filePath: path.relative(draftPath, resolvedFilePath)
642664
}
643665
}
644666
case 'remove_file': {
645-
const draftPath = this.resolveDraftPath(conversationId, request.draftPath)
667+
const draftId = this.validateDraftId(request.draftId)
668+
if (!draftId) {
669+
return {
670+
success: false,
671+
action,
672+
error: 'Draft handle is invalid for this conversation'
673+
}
674+
}
675+
const draftPath = this.getDraftPathForId(conversationId, draftId)
646676
if (!draftPath) {
647677
return {
648678
success: false,
649679
action,
650-
error: 'Draft path is invalid or outside the draft root'
680+
error: 'Draft handle is invalid for this conversation'
651681
}
652682
}
653683
if (!request.filePath?.trim()) {
@@ -668,24 +698,32 @@ export class SkillPresenter implements ISkillPresenter {
668698
return {
669699
success: true,
670700
action,
671-
draftPath,
701+
draftId,
672702
filePath: path.relative(draftPath, resolvedFilePath)
673703
}
674704
}
675705
case 'delete': {
676-
const draftPath = this.resolveDraftPath(conversationId, request.draftPath)
706+
const draftId = this.validateDraftId(request.draftId)
707+
if (!draftId) {
708+
return {
709+
success: false,
710+
action,
711+
error: 'Draft handle is invalid for this conversation'
712+
}
713+
}
714+
const draftPath = this.getDraftPathForId(conversationId, draftId)
677715
if (!draftPath) {
678716
return {
679717
success: false,
680718
action,
681-
error: 'Draft path is invalid or outside the draft root'
719+
error: 'Draft handle is invalid for this conversation'
682720
}
683721
}
684722
if (!fs.existsSync(draftPath)) {
685-
return { success: false, action, error: 'Draft path not found' }
723+
return { success: false, action, error: 'Draft not found' }
686724
}
687725
fs.rmSync(draftPath, { recursive: true, force: true })
688-
return { success: true, action, draftPath }
726+
return { success: true, action, draftId }
689727
}
690728
default:
691729
return { success: false, action, error: `Unsupported draft action: ${action}` }
@@ -1870,32 +1908,58 @@ export class SkillPresenter implements ISkillPresenter {
18701908
return normalizedConversationId
18711909
}
18721910

1873-
private createDraftDirectory(conversationId: string, skillName: string): string {
1911+
private validateDraftId(draftId: string | undefined): string | null {
1912+
const normalizedDraftId = draftId?.trim()
1913+
if (!normalizedDraftId) {
1914+
return null
1915+
}
1916+
if (path.isAbsolute(normalizedDraftId)) {
1917+
return null
1918+
}
1919+
if (normalizedDraftId !== path.basename(normalizedDraftId)) {
1920+
return null
1921+
}
1922+
if (
1923+
normalizedDraftId.includes('..') ||
1924+
normalizedDraftId.includes('/') ||
1925+
normalizedDraftId.includes('\\') ||
1926+
normalizedDraftId.includes(path.sep)
1927+
) {
1928+
return null
1929+
}
1930+
if (!DRAFT_ID_PATTERN.test(normalizedDraftId)) {
1931+
return null
1932+
}
1933+
return normalizedDraftId
1934+
}
1935+
1936+
private createDraftHandle(conversationId: string): { draftId: string; draftPath: string } {
18741937
const safeConversationId = this.validateDraftConversationId(conversationId)
18751938
if (!safeConversationId) {
1876-
throw new Error('Invalid conversationId for draft path')
1939+
throw new Error('Invalid conversationId for draft access')
18771940
}
18781941
this.ensureDraftRoot()
18791942
const conversationRoot = path.join(this.draftsRoot, safeConversationId)
18801943
fs.mkdirSync(conversationRoot, { recursive: true })
1881-
const safeSlug = skillName.replace(/[^a-z0-9._-]/g, '-')
1882-
const draftPath = path.join(conversationRoot, `${Date.now()}-${safeSlug}`)
1944+
const draftId = `draft-${randomUUID()}`
1945+
const draftPath = path.join(conversationRoot, draftId)
18831946
fs.mkdirSync(draftPath, { recursive: true })
1884-
return draftPath
1947+
return { draftId, draftPath }
18851948
}
18861949

1887-
private resolveDraftPath(conversationId: string, draftPath: string | undefined): string | null {
1888-
if (!draftPath?.trim()) {
1950+
private getDraftPathForId(conversationId: string, draftId: string): string | null {
1951+
const safeDraftId = this.validateDraftId(draftId)
1952+
if (!safeDraftId) {
18891953
return null
18901954
}
18911955
const safeConversationId = this.validateDraftConversationId(conversationId)
18921956
if (!safeConversationId) {
18931957
return null
18941958
}
18951959
const conversationRoot = path.resolve(this.draftsRoot, safeConversationId)
1896-
const resolvedDraftPath = path.resolve(draftPath)
1960+
const resolvedDraftPath = path.resolve(conversationRoot, safeDraftId)
18971961
const relativePath = path.relative(conversationRoot, resolvedDraftPath)
1898-
if (relativePath === '' || relativePath.startsWith('..') || path.isAbsolute(relativePath)) {
1962+
if (!relativePath || relativePath.startsWith('..') || path.isAbsolute(relativePath)) {
18991963
return null
19001964
}
19011965
return resolvedDraftPath

‎src/main/presenter/toolPresenter/agentTools/agentToolManager.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -266,27 +266,27 @@ export class AgentToolManager {
266266
}),
267267
z.object({
268268
action: z.literal('edit').describe('Draft-only skill management action'),
269-
draftPath: z.string().describe('Existing draft root path'),
269+
draftId: z.string().describe('Opaque draft ID returned by skill_manage create'),
270270
content: z.string().describe('Complete SKILL.md document including frontmatter and body')
271271
}),
272272
z.object({
273273
action: z.literal('write_file').describe('Draft-only skill management action'),
274-
draftPath: z.string().describe('Existing draft root path'),
274+
draftId: z.string().describe('Opaque draft ID returned by skill_manage create'),
275275
filePath: z
276276
.string()
277277
.describe('Relative file path under references/, templates/, scripts/, or assets/'),
278278
fileContent: z.string().describe('Text content for write_file')
279279
}),
280280
z.object({
281281
action: z.literal('remove_file').describe('Draft-only skill management action'),
282-
draftPath: z.string().describe('Existing draft root path'),
282+
draftId: z.string().describe('Opaque draft ID returned by skill_manage create'),
283283
filePath: z
284284
.string()
285285
.describe('Relative file path under references/, templates/, scripts/, or assets/')
286286
}),
287287
z.object({
288288
action: z.literal('delete').describe('Draft-only skill management action'),
289-
draftPath: z.string().describe('Existing draft root path')
289+
draftId: z.string().describe('Opaque draft ID returned by skill_manage create')
290290
})
291291
])
292292
}
@@ -1530,7 +1530,7 @@ export class AgentToolManager {
15301530
function: {
15311531
name: 'skill_manage',
15321532
description:
1533-
'Create or edit temporary draft skills in the conversation draft area. This cannot modify installed skills.',
1533+
'Create or edit temporary draft skills in the conversation draft area. Use the returned draftId for follow-up draft operations. This cannot modify installed skills.',
15341534
parameters: zodToJsonSchema(schemas.skill_manage) as {
15351535
type: string
15361536
properties: Record<string, unknown>

‎src/shared/types/skill.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ export type SkillManageAction = 'create' | 'edit' | 'write_file' | 'remove_file'
145145

146146
export interface SkillManageRequest {
147147
action: SkillManageAction
148-
draftPath?: string
148+
draftId?: string
149149
content?: string
150150
filePath?: string
151151
fileContent?: string
@@ -154,7 +154,7 @@ export interface SkillManageRequest {
154154
export interface SkillManageResult {
155155
success: boolean
156156
action: SkillManageAction
157-
draftPath?: string
157+
draftId?: string
158158
filePath?: string
159159
skillName?: string
160160
error?: string

0 commit comments

Comments
 (0)