Skip to content

Commit c4d3f05

Browse files
shakkernerdshakkernerd
committed
fix: validate workshop support symlink writes
1 parent 6b0525f commit c4d3f05

3 files changed

Lines changed: 112 additions & 14 deletions

File tree

‎src/skills/workshop/service.test.ts‎

Lines changed: 55 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,11 @@ import {
2525
resolvePendingSkillProposal,
2626
reviseSkillProposal,
2727
} from "./service.js";
28-
import { readSkillProposalManifest, resolveProposalDraftPath } from "./store.js";
28+
import {
29+
readSkillProposalManifest,
30+
resolveProposalDraftPath,
31+
updateSkillProposalRecord,
32+
} from "./store.js";
2933

3034
const tempDirs = createTrackedTempDirs();
3135
let testState: OpenClawTestState;
@@ -218,6 +222,56 @@ describe("skill workshop proposals", () => {
218222
},
219223
);
220224

225+
it.runIf(process.platform !== "win32")(
226+
"validates support file targets against trusted symlink write roots",
227+
async () => {
228+
const workspaceDir = await makeWorkspace();
229+
const targetSkillsDir = await tempDirs.make("openclaw-skill-workshop-support-trusted-");
230+
const untrustedSkillsDir = await tempDirs.make("openclaw-skill-workshop-support-untrusted-");
231+
await fs.symlink(targetSkillsDir, path.join(workspaceDir, "skills"), "dir");
232+
await fs.symlink(untrustedSkillsDir, path.join(workspaceDir, "other-skills"), "dir");
233+
const config = {
234+
skills: {
235+
load: { allowSymlinkTargets: [targetSkillsDir] },
236+
workshop: { allowSymlinkTargetWrites: true },
237+
},
238+
};
239+
const proposal = await proposeCreateSkill({
240+
workspaceDir,
241+
config,
242+
name: "Support Escape",
243+
description: "Must keep support writes in trusted roots",
244+
content: "# Support Escape\n\nDo not write through the wrong skill dir.\n",
245+
supportFiles: [
246+
{
247+
path: "references/details.md",
248+
content: "This support file must not be written outside the trusted target.\n",
249+
},
250+
],
251+
});
252+
253+
await updateSkillProposalRecord({
254+
record: {
255+
...proposal.record,
256+
target: {
257+
...proposal.record.target,
258+
skillDir: path.join(workspaceDir, "other-skills", "support-escape"),
259+
},
260+
},
261+
});
262+
263+
await expect(
264+
applySkillProposal({ workspaceDir, config, proposalId: proposal.record.id }),
265+
).rejects.toThrow("untrusted symlink target");
266+
await expect(
267+
fs.access(path.join(untrustedSkillsDir, "support-escape", "references", "details.md")),
268+
).rejects.toThrow();
269+
await expect(
270+
fs.access(path.join(targetSkillsDir, "support-escape", "SKILL.md")),
271+
).rejects.toThrow();
272+
},
273+
);
274+
221275
it.runIf(process.platform !== "win32")(
222276
"blocks untrusted workspace skills symlink targets before support files are written",
223277
async () => {

‎src/skills/workshop/service.ts‎

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -664,10 +664,12 @@ async function publishProposalTarget(params: {
664664
try {
665665
for (const file of params.supportFiles) {
666666
await writeWorkspaceSupportFile({
667+
workspaceDir: params.workspaceDir,
667668
skillDir: params.record.target.skillDir,
668669
relativePath: file.path,
669670
content: file.content,
670671
overwrite: params.record.kind === "update",
672+
allowedSymlinkTargetRealPaths: params.allowedSymlinkTargetRealPaths,
671673
});
672674
writtenSupportPaths.push(file.path);
673675
}
@@ -680,33 +682,49 @@ async function publishProposalTarget(params: {
680682
});
681683
} catch (error) {
682684
if (params.record.kind === "create") {
683-
await cleanupCreatedSupportFiles(params.record, writtenSupportPaths);
685+
await cleanupCreatedSupportFiles({
686+
workspaceDir: params.workspaceDir,
687+
record: params.record,
688+
writtenSupportPaths,
689+
allowedSymlinkTargetRealPaths: params.allowedSymlinkTargetRealPaths,
690+
});
684691
} else {
685692
await restoreUpdatedSupportFiles({
693+
workspaceDir: params.workspaceDir,
686694
record: params.record,
687695
writtenSupportPaths,
688696
previousSupportFiles: params.previousSupportFiles,
697+
allowedSymlinkTargetRealPaths: params.allowedSymlinkTargetRealPaths,
689698
});
690699
}
691700
throw error;
692701
}
693702
}
694703

695-
async function cleanupCreatedSupportFiles(
696-
record: SkillProposalRecord,
697-
writtenSupportPaths: readonly string[],
698-
): Promise<void> {
704+
async function cleanupCreatedSupportFiles(params: {
705+
workspaceDir: string;
706+
record: SkillProposalRecord;
707+
writtenSupportPaths: readonly string[];
708+
allowedSymlinkTargetRealPaths: readonly string[];
709+
}): Promise<void> {
699710
await Promise.allSettled(
700-
writtenSupportPaths.toReversed().map(async (relativePath) => {
701-
await removeWorkspaceSupportFile({ skillDir: record.target.skillDir, relativePath });
711+
params.writtenSupportPaths.toReversed().map(async (relativePath) => {
712+
await removeWorkspaceSupportFile({
713+
workspaceDir: params.workspaceDir,
714+
skillDir: params.record.target.skillDir,
715+
relativePath,
716+
allowedSymlinkTargetRealPaths: params.allowedSymlinkTargetRealPaths,
717+
});
702718
}),
703719
);
704720
}
705721

706722
async function restoreUpdatedSupportFiles(params: {
723+
workspaceDir: string;
707724
record: SkillProposalRecord;
708725
writtenSupportPaths: readonly string[];
709726
previousSupportFiles: NonNullable<SkillProposalRollback["supportFiles"]>;
727+
allowedSymlinkTargetRealPaths: readonly string[];
710728
}): Promise<void> {
711729
const previousByPath = new Map(params.previousSupportFiles.map((file) => [file.path, file]));
712730
await Promise.allSettled(
@@ -717,13 +735,20 @@ async function restoreUpdatedSupportFiles(params: {
717735
}
718736
if (previous.existed) {
719737
await writeWorkspaceSupportFile({
738+
workspaceDir: params.workspaceDir,
720739
skillDir: params.record.target.skillDir,
721740
relativePath,
722741
content: previous.previousContent ?? "",
723742
overwrite: true,
743+
allowedSymlinkTargetRealPaths: params.allowedSymlinkTargetRealPaths,
724744
});
725745
} else {
726-
await removeWorkspaceSupportFile({ skillDir: params.record.target.skillDir, relativePath });
746+
await removeWorkspaceSupportFile({
747+
workspaceDir: params.workspaceDir,
748+
skillDir: params.record.target.skillDir,
749+
relativePath,
750+
allowedSymlinkTargetRealPaths: params.allowedSymlinkTargetRealPaths,
751+
});
727752
}
728753
}),
729754
);

‎src/skills/workshop/store.ts‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -615,28 +615,47 @@ async function tryRealpath(filePath: string): Promise<string | null> {
615615
}
616616

617617
export async function writeWorkspaceSupportFile(params: {
618+
workspaceDir?: string;
618619
skillDir: string;
619620
relativePath: string;
620621
content: string;
621622
overwrite?: boolean;
623+
allowedSymlinkTargetRealPaths?: readonly string[];
622624
}): Promise<void> {
623625
const relativePath = normalizeSkillProposalSupportPath(params.relativePath);
624-
await fs.mkdir(params.skillDir, { recursive: true });
625-
const skillRoot = await root(params.skillDir);
626-
await skillRoot.write(relativePath, params.content, {
626+
const filePath = path.join(params.skillDir, ...relativePath.split("/"));
627+
const target = params.workspaceDir
628+
? await resolveWorkspaceSkillWriteTarget({
629+
workspaceDir: params.workspaceDir,
630+
filePath,
631+
allowedSymlinkTargetRealPaths: params.allowedSymlinkTargetRealPaths,
632+
})
633+
: { rootDir: params.skillDir, relativePath };
634+
const targetRoot = await root(target.rootDir);
635+
await targetRoot.write(target.relativePath, params.content, {
627636
encoding: "utf8",
628637
mkdir: true,
629638
...(params.overwrite === undefined ? {} : { overwrite: params.overwrite }),
630639
});
631640
}
632641

633642
export async function removeWorkspaceSupportFile(params: {
643+
workspaceDir?: string;
634644
skillDir: string;
635645
relativePath: string;
646+
allowedSymlinkTargetRealPaths?: readonly string[];
636647
}): Promise<void> {
637648
const relativePath = normalizeSkillProposalSupportPath(params.relativePath);
638-
const skillRoot = await root(params.skillDir);
639-
await skillRoot.remove(relativePath).catch((error: unknown) => {
649+
const filePath = path.join(params.skillDir, ...relativePath.split("/"));
650+
const target = params.workspaceDir
651+
? await resolveWorkspaceSkillWriteTarget({
652+
workspaceDir: params.workspaceDir,
653+
filePath,
654+
allowedSymlinkTargetRealPaths: params.allowedSymlinkTargetRealPaths,
655+
})
656+
: { rootDir: params.skillDir, relativePath };
657+
const targetRoot = await root(target.rootDir);
658+
await targetRoot.remove(target.relativePath).catch((error: unknown) => {
640659
if ((error as { code?: string })?.code !== "ENOENT") {
641660
throw error;
642661
}

0 commit comments

Comments
 (0)