Skip to content

fix(security): validate path characters in skill removal request (fixes #53) #55

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ThankNIXlater wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix(security): validate path characters in skill removal request (fixes #53) #55

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2170674
fix(security): validate path characters in skill removal request (fix…
ThankNIXlater Mar 23, 2026
b2f313e
fix: address review feedback on path validation
earntoshi Mar 25, 2026
59ec7eb
Merge branch 'main' into fix/issue-53-skill-remove-path-validation
iamlukethedev Mar 28, 2026
97122d4
Merge branch 'main' into fix/issue-53-skill-remove-path-validation
iamlukethedev Apr 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions src/app/api/gateway/skills/remove/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ const REMOVABLE_SOURCES = new Set<RemovableSkillSource>([
"openclaw-workspace",
]);

const SAFE_PATH_RE = /^[a-zA-Z0-9_.~\x2F:\\-]+$/;

const normalizeRequired = (value: unknown, field: string): string => {
if (typeof value !== "string") {
throw new Error(`${field} is required.`);
Expand All @@ -28,6 +30,14 @@ const normalizeRequired = (value: unknown, field: string): string => {
return trimmed;
};

const normalizeRequiredPath = (value: unknown, field: string): string => {
const trimmed = normalizeRequired(value, field);
if (!SAFE_PATH_RE.test(trimmed)) {
throw new Error(`${field} contains invalid characters.`);

@iamlukethedev iamlukethedev Mar 24, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This introduces a new validation failure mode ("contains invalid characters"), but the catch block below does not classify that message as client input. As written, malformed paths will now return 500 instead of 400. Please either include this message in the 400 mapping or switch to a structured validation error check.

}
return trimmed;
};

const resolveSkillRemovalSshTarget = (): string | null => {
const configured = resolveConfiguredSshTarget(process.env);
if (configured) return configured;
Expand All @@ -51,9 +61,9 @@ const normalizeRemoveRequest = (body: unknown): SkillRemoveRequest => {
return {
skillKey: normalizeRequired(record.skillKey, "skillKey"),
source: sourceRaw as RemovableSkillSource,
baseDir: normalizeRequired(record.baseDir, "baseDir"),
workspaceDir: normalizeRequired(record.workspaceDir, "workspaceDir"),
managedSkillsDir: normalizeRequired(record.managedSkillsDir, "managedSkillsDir"),
baseDir: normalizeRequiredPath(record.baseDir, "baseDir"),

@cursor cursor Bot Mar 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skillKey SSH argument not validated for unsafe characters

High Severity

skillKey is passed as an SSH positional argument in the same argv array as baseDir, workspaceDir, and managedSkillsDir, but it is only validated with normalizeRequired (non-empty check), not with SAFE_PATH_RE or equivalent character validation. Since SSH concatenates remote command arguments into a shell-interpreted string, a skillKey containing shell metacharacters could break the argument boundary on the remote side — the same class of injection this PR aims to prevent.

Fix in Cursor Fix in Web

workspaceDir: normalizeRequiredPath(record.workspaceDir, "workspaceDir"),
managedSkillsDir: normalizeRequiredPath(record.managedSkillsDir, "managedSkillsDir"),
};
};

Expand All @@ -79,7 +89,8 @@ export async function POST(request: Request) {
message.includes("Remote workspace skill removal is not supported over SSH") ||
message.includes("Gateway URL is missing") ||
message.includes("Invalid gateway URL") ||
message.includes("require OPENCLAW_GATEWAY_SSH_TARGET")
message.includes("require OPENCLAW_GATEWAY_SSH_TARGET") ||
message.includes("invalid characters")
? 400
: 500;
if (status >= 500) {
Expand Down