fix: harden permission denial responses

This commit is contained in:
Peter Steinberger
2026-05-11 15:50:14 +01:00
parent 6f619eabc0
commit bb6c6c1b38
9 changed files with 555 additions and 23 deletions
+2
View File
@@ -6,6 +6,8 @@
### Fixes
- API: return deterministic 403 responses for skill/package rescan and package transfer permission denials, with CI e2e coverage for protected write endpoints.
## 0.13.0 - 2026-05-11
### Changes
+135
View File
@@ -2863,6 +2863,28 @@ describe("httpApiV1 handlers", () => {
);
});
it("skill rescan maps ownership denials to 403", async () => {
vi.mocked(requireApiTokenUser).mockResolvedValue({
userId: "users:stranger",
user: { handle: "stranger" },
} as never);
const runMutation = vi.fn(async (_mutation: unknown, args: Record<string, unknown>) => {
if ("key" in args) return okRate();
throw new Error("Forbidden: You do not own this skill.");
});
const response = await __handlers.skillsPostRouterV1Handler(
makeCtx({ runMutation }),
new Request("https://example.com/api/v1/skills/demo/rescan", {
method: "POST",
headers: { Authorization: "Bearer clh_test" },
}),
);
expect(response.status).toBe(403);
expect(await response.text()).toBe("Forbidden: You do not own this skill.");
});
it("package rescan routes authenticated owners to the rescan mutation", async () => {
vi.mocked(requireApiTokenUser).mockResolvedValue({
userId: "users:1",
@@ -2902,6 +2924,28 @@ describe("httpApiV1 handlers", () => {
);
});
it("package rescan maps ownership denials to 403", async () => {
vi.mocked(requireApiTokenUser).mockResolvedValue({
userId: "users:stranger",
user: { handle: "stranger" },
} as never);
const runMutation = vi.fn(async (_mutation: unknown, args: Record<string, unknown>) => {
if ("key" in args) return okRate();
throw new Error("Forbidden: You do not own this package.");
});
const response = await __handlers.packagesPostRouterV1Handler(
makeCtx({ runMutation }),
new Request("https://example.com/api/v1/packages/%40scope%2Fdemo/rescan", {
method: "POST",
headers: { Authorization: "Bearer clh_test" },
}),
);
expect(response.status).toBe(403);
expect(await response.text()).toBe("Forbidden: You do not own this package.");
});
it("transfer request requires auth", async () => {
vi.mocked(requireApiTokenUser).mockRejectedValueOnce(new Error("Unauthorized"));
const runMutation = vi.fn().mockResolvedValue(okRate());
@@ -2955,6 +2999,34 @@ describe("httpApiV1 handlers", () => {
);
});
it("skill transfer maps ownership denials to 403", async () => {
vi.mocked(requireApiTokenUser).mockResolvedValue({
userId: "users:stranger",
user: { handle: "stranger" },
} as never);
const runQuery = vi.fn(async (_query: unknown, args: Record<string, unknown>) => {
if ("slug" in args) return { _id: "skills:1", slug: "demo" };
return null;
});
const runMutation = vi.fn(async (_mutation: unknown, args: Record<string, unknown>) => {
if ("key" in args) return okRate();
throw new Error("Forbidden: Only owners can transfer this skill.");
});
const response = await __handlers.skillsPostRouterV1Handler(
makeCtx({ runQuery, runMutation }),
new Request("https://example.com/api/v1/skills/demo/transfer", {
method: "POST",
headers: { Authorization: "Bearer clh_test", "content-type": "application/json" },
body: JSON.stringify({ toUserHandle: "alice" }),
}),
);
expect(response.status).toBe(403);
expect(await response.text()).toBe("Forbidden: Only owners can transfer this skill.");
});
it("transfers a skill directly to an org publisher when the target handle is an org", async () => {
vi.mocked(requireApiTokenUser).mockResolvedValue({
userId: "users:1",
@@ -6758,6 +6830,32 @@ describe("httpApiV1 handlers", () => {
);
});
it("package transfer maps ownership denials to 403", async () => {
vi.mocked(requireApiTokenUser).mockResolvedValue({
userId: "users:stranger",
user: { _id: "users:stranger", handle: "stranger" },
} as never);
const runMutation = vi.fn(async (_mutation: unknown, args: Record<string, unknown>) => {
if ("key" in args) return okRate();
throw new Error("Forbidden: Only owners can transfer this package.");
});
const response = await __handlers.packagesPostRouterV1Handler(
makeCtx({ runMutation }),
new Request("https://example.com/api/v1/packages/%40opik%2Fopik-openclaw/transfer", {
method: "POST",
headers: {
Authorization: "Bearer clh_test",
"content-type": "application/json",
},
body: JSON.stringify({ toOwner: "opik" }),
}),
);
expect(response.status).toBe(403);
expect(await response.text()).toBe("Forbidden: Only owners can transfer this package.");
});
it("sets trusted publisher config for a package without environment", async () => {
vi.mocked(requireApiTokenUser).mockResolvedValue({
userId: "users:1",
@@ -6886,6 +6984,43 @@ describe("httpApiV1 handlers", () => {
);
});
it("package delete and undelete map ownership denials to 403", async () => {
vi.mocked(requireApiTokenUser).mockResolvedValue({
userId: "users:stranger",
user: { _id: "users:stranger", handle: "stranger" },
} as never);
const runMutationForbidden = vi.fn(
async (_mutation: unknown, args: Record<string, unknown>) => {
if ("key" in args) return okRate();
throw new Error("Forbidden: This package belongs to another owner.");
},
);
const deleteResponse = await __handlers.packagesDeleteRouterV1Handler(
makeCtx({ runMutation: runMutationForbidden }),
new Request("https://example.com/api/v1/packages/%40openclaw%2Fdemo-plugin", {
method: "DELETE",
headers: { Authorization: "Bearer clh_test" },
}),
);
expect(deleteResponse.status).toBe(403);
expect(await deleteResponse.text()).toBe("Forbidden: This package belongs to another owner.");
vi.mocked(requireApiTokenUser).mockResolvedValue({
userId: "users:stranger",
user: { _id: "users:stranger", handle: "stranger" },
} as never);
const undeleteResponse = await __handlers.packagesPostRouterV1Handler(
makeCtx({ runMutation: runMutationForbidden }),
new Request("https://example.com/api/v1/packages/%40openclaw%2Fdemo-plugin/undelete", {
method: "POST",
headers: { Authorization: "Bearer clh_test" },
}),
);
expect(undeleteResponse.status).toBe(403);
expect(await undeleteResponse.text()).toBe("Forbidden: This package belongs to another owner.");
});
it("deletes trusted publisher config for a package", async () => {
vi.mocked(requireApiTokenUser).mockResolvedValue({
userId: "users:1",
+18 -10
View File
@@ -51,6 +51,7 @@ import {
requirePackagePublishAuthOrResponse,
safeTextFileResponse,
softDeleteErrorToResponse,
formatAuthzMessage,
text,
toOptionalNumber,
} from "./shared";
@@ -113,6 +114,21 @@ const internalRefs = internal as unknown as {
};
};
function packageOperationErrorToResponse(
error: unknown,
headers: HeadersInit,
fallback = "Package operation failed",
) {
const message = error instanceof Error ? error.message : fallback;
const lower = message.toLowerCase();
if (lower.includes("unauthorized"))
return text(formatAuthzMessage(error, "Unauthorized"), 401, headers);
if (lower.includes("forbidden"))
return text(formatAuthzMessage(error, "Forbidden"), 403, headers);
if (lower.includes("not found")) return text(message, 404, headers);
return text(message, 400, headers);
}
async function runQueryRef<T>(ctx: ActionCtx, ref: unknown, args: unknown): Promise<T> {
return (await ctx.runQuery(ref as never, args as never)) as T;
}
@@ -1721,11 +1737,7 @@ export async function packagesPostRouterV1Handler(ctx: ActionCtx, request: Reque
);
return json(result, 200, rate.headers);
} catch (error) {
return text(
error instanceof Error ? error.message : "Rescan request failed",
400,
rate.headers,
);
return packageOperationErrorToResponse(error, rate.headers, "Rescan request failed");
}
}
@@ -1770,11 +1782,7 @@ export async function packagesPostRouterV1Handler(ctx: ActionCtx, request: Reque
);
return json(result, 200, rate.headers);
} catch (error) {
return text(
error instanceof Error ? error.message : "Package transfer failed",
400,
rate.headers,
);
return packageOperationErrorToResponse(error, rate.headers, "Package transfer failed");
}
}
+1 -5
View File
@@ -1448,11 +1448,7 @@ export async function skillsPostRouterV1Handler(ctx: ActionCtx, request: Request
);
return json(result, 200, rate.headers);
} catch (error) {
return text(
error instanceof Error ? error.message : "Rescan request failed",
400,
rate.headers,
);
return ownershipErrorToResponse(error, rate.headers);
}
}
+385
View File
@@ -0,0 +1,385 @@
/* @vitest-environment node */
import { spawnSync } from "node:child_process";
import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises";
import { tmpdir } from "node:os";
import { join } from "node:path";
import { ApiRoutes } from "clawhub-schema";
import { describe, expect, it } from "vitest";
import { readGlobalConfig } from "../packages/clawhub/src/config";
import {
allowLiveMutations,
buildE2ESkillMarkdown,
fetchWithTimeout,
getRegistry,
getSite,
getUserToken,
makeTempConfig,
mustGetToken,
} from "./helpers/clawhubCli";
const itIfLiveMutationsAndUserToken = allowLiveMutations() && getUserToken() ? it : it.skip;
function commandOutput(result: ReturnType<typeof spawnSync>) {
return `${result.stdout ?? ""}\n${result.stderr ?? ""}`;
}
function expectForbiddenCli(result: ReturnType<typeof spawnSync>) {
expect(result.status).not.toBe(0);
expect(commandOutput(result)).toMatch(/Forbidden|not authorized|not allowed|owner/i);
}
async function writeE2ECodePluginFixture(packageDir: string, packageName: string) {
await mkdir(join(packageDir, "dist"), { recursive: true });
await writeFile(
join(packageDir, "package.json"),
`${JSON.stringify(
{
name: packageName,
displayName: `E2E ${packageName}`,
version: "1.0.0",
description: "Disposable ClawHub production permission e2e plugin fixture.",
type: "module",
main: "./dist/index.js",
openclaw: {
extensions: ["./dist/index.js"],
compat: { pluginApi: ">=2026.3.24-beta.2" },
build: { openclawVersion: "2026.3.24-beta.2" },
},
},
null,
2,
)}\n`,
"utf8",
);
await writeFile(
join(packageDir, "openclaw.plugin.json"),
`${JSON.stringify(
{
id: `${packageName}.plugin`,
name: `E2E ${packageName}`,
configSchema: { type: "object", properties: {}, additionalProperties: false },
},
null,
2,
)}\n`,
"utf8",
);
await writeFile(
join(packageDir, "dist", "index.js"),
"export default function activate() { return { ok: true }; }\n",
"utf8",
);
await writeFile(
join(packageDir, "README.md"),
`# ${packageName}\n\nDisposable production permission e2e package. It has no side effects and exists only to prove non-owner package actions are rejected.\n`,
"utf8",
);
}
describe("permission boundary e2e", () => {
it("rejects unauthenticated protected write endpoints before mutation", async () => {
const registry = getRegistry();
const cases = [
{ method: "DELETE", path: `${ApiRoutes.skills}/gifgrep` },
{ method: "POST", path: `${ApiRoutes.skills}/gifgrep/undelete`, body: {} },
{
method: "POST",
path: `${ApiRoutes.skills}/gifgrep/transfer`,
body: { toUserHandle: "openclaw" },
},
{ method: "POST", path: `${ApiRoutes.skills}/gifgrep/rescan`, body: {} },
{ method: "DELETE", path: `${ApiRoutes.packages}/e2e-nonexistent-permission` },
{ method: "POST", path: `${ApiRoutes.packages}/e2e-nonexistent-permission/undelete` },
{
method: "POST",
path: `${ApiRoutes.packages}/e2e-nonexistent-permission/transfer`,
body: { toOwner: "openclaw" },
},
{ method: "POST", path: `${ApiRoutes.packages}/e2e-nonexistent-permission/rescan` },
{
method: "POST",
path: `${ApiRoutes.packages}/e2e-nonexistent-permission/trusted-publisher`,
body: {
repository: "openclaw/clawhub",
workflowFilename: "release.yml",
},
},
{
method: "DELETE",
path: `${ApiRoutes.packages}/e2e-nonexistent-permission/trusted-publisher`,
},
{ method: "POST", path: `${ApiRoutes.users}/restore`, body: { handle: "nobody" } },
{ method: "POST", path: `${ApiRoutes.users}/reclaim`, body: { handle: "nobody" } },
{ method: "POST", path: `${ApiRoutes.users}/reserve`, body: { handle: "nobody" } },
{ method: "POST", path: `${ApiRoutes.users}/publisher`, body: { handle: "nobody" } },
] as const;
for (const testCase of cases) {
const response = await fetchWithTimeout(new URL(testCase.path, registry), {
method: testCase.method,
headers: { Accept: "application/json", "Content-Type": "application/json" },
body: "body" in testCase ? JSON.stringify(testCase.body) : undefined,
});
expect(response.status, `${testCase.method} ${testCase.path}`).toBe(401);
expect(await response.text()).toMatch(/Unauthorized/i);
}
});
itIfLiveMutationsAndUserToken(
"rejects non-owner skill lifecycle, transfer, and rescan actions",
async () => {
const registry = getRegistry();
const site = getSite();
const ownerToken = mustGetToken() ?? (await readGlobalConfig())?.token ?? null;
const strangerToken = getUserToken();
if (!ownerToken || !strangerToken) {
throw new Error("Missing owner token or CLAWHUB_E2E_USER_TOKEN");
}
const ownerCfg = await makeTempConfig(registry, ownerToken);
const strangerCfg = await makeTempConfig(registry, strangerToken);
const workdir = await mkdtemp(join(tmpdir(), "clawhub-e2e-permission-skill-"));
const slug = `e2e-perm-${Date.now()}`;
const skillDir = join(workdir, slug);
const metaUrl = new URL(`${ApiRoutes.skills}/${slug}`, registry);
try {
await mkdir(skillDir, { recursive: true });
await writeFile(join(skillDir, "SKILL.md"), buildE2ESkillMarkdown(slug), "utf8");
const publish = spawnSync(
"bun",
[
"clawhub",
"publish",
skillDir,
"--slug",
slug,
"--name",
`E2E ${slug}`,
"--version",
"1.0.0",
"--tags",
"latest",
"--site",
site,
"--registry",
registry,
"--workdir",
workdir,
],
{
cwd: process.cwd(),
env: {
...process.env,
CLAWHUB_CONFIG_PATH: ownerCfg.path,
CLAWHUB_DISABLE_TELEMETRY: "1",
},
encoding: "utf8",
},
);
expect(publish.status, commandOutput(publish)).toBe(0);
const strangerEnv = {
...process.env,
CLAWHUB_CONFIG_PATH: strangerCfg.path,
CLAWHUB_DISABLE_TELEMETRY: "1",
};
const baseArgs = ["--site", site, "--registry", registry, "--workdir", workdir];
expectForbiddenCli(
spawnSync("bun", ["clawhub", "delete", slug, "--yes", ...baseArgs], {
cwd: process.cwd(),
env: strangerEnv,
encoding: "utf8",
}),
);
expectForbiddenCli(
spawnSync(
"bun",
["clawhub", "transfer", "request", slug, "openclaw", "--yes", ...baseArgs],
{
cwd: process.cwd(),
env: strangerEnv,
encoding: "utf8",
},
),
);
expectForbiddenCli(
spawnSync("bun", ["clawhub", "skill", "rescan", slug, "--yes", ...baseArgs], {
cwd: process.cwd(),
env: strangerEnv,
encoding: "utf8",
}),
);
const metaAfterDeniedActions = await fetchWithTimeout(metaUrl.toString(), {
headers: { Accept: "application/json" },
});
expect(metaAfterDeniedActions.status).toBe(200);
} finally {
spawnSync(
"bun",
[
"clawhub",
"delete",
slug,
"--yes",
"--site",
site,
"--registry",
registry,
"--workdir",
workdir,
],
{
cwd: process.cwd(),
env: {
...process.env,
CLAWHUB_CONFIG_PATH: ownerCfg.path,
CLAWHUB_DISABLE_TELEMETRY: "1",
},
encoding: "utf8",
},
);
await rm(workdir, { recursive: true, force: true });
await rm(ownerCfg.dir, { recursive: true, force: true });
await rm(strangerCfg.dir, { recursive: true, force: true });
}
},
180_000,
);
itIfLiveMutationsAndUserToken(
"rejects non-owner package lifecycle, transfer, and rescan actions",
async () => {
const registry = getRegistry();
const site = getSite();
const ownerToken = mustGetToken() ?? (await readGlobalConfig())?.token ?? null;
const strangerToken = getUserToken();
if (!ownerToken || !strangerToken) {
throw new Error("Missing owner token or CLAWHUB_E2E_USER_TOKEN");
}
const ownerCfg = await makeTempConfig(registry, ownerToken);
const strangerCfg = await makeTempConfig(registry, strangerToken);
const workdir = await mkdtemp(join(tmpdir(), "clawhub-e2e-permission-package-"));
const packageName = `e2e-perm-plugin-${Date.now()}`;
const packageDir = join(workdir, packageName);
try {
await mkdir(packageDir, { recursive: true });
await writeE2ECodePluginFixture(packageDir, packageName);
const publish = spawnSync(
"bun",
[
"clawhub",
"package",
"publish",
packageDir,
"--source-repo",
"openclaw/clawhub",
"--source-commit",
"0000000000000000000000000000000000000000",
"--site",
site,
"--registry",
registry,
"--workdir",
workdir,
],
{
cwd: process.cwd(),
env: {
...process.env,
CLAWHUB_CONFIG_PATH: ownerCfg.path,
CLAWHUB_DISABLE_TELEMETRY: "1",
},
encoding: "utf8",
},
);
expect(publish.status, commandOutput(publish)).toBe(0);
const strangerEnv = {
...process.env,
CLAWHUB_CONFIG_PATH: strangerCfg.path,
CLAWHUB_DISABLE_TELEMETRY: "1",
};
const baseArgs = ["--site", site, "--registry", registry, "--workdir", workdir];
expectForbiddenCli(
spawnSync("bun", ["clawhub", "package", "delete", packageName, "--yes", ...baseArgs], {
cwd: process.cwd(),
env: strangerEnv,
encoding: "utf8",
}),
);
expectForbiddenCli(
spawnSync(
"bun",
["clawhub", "package", "transfer", packageName, "--to", "openclaw", ...baseArgs],
{
cwd: process.cwd(),
env: strangerEnv,
encoding: "utf8",
},
),
);
expectForbiddenCli(
spawnSync("bun", ["clawhub", "package", "rescan", packageName, "--yes", ...baseArgs], {
cwd: process.cwd(),
env: strangerEnv,
encoding: "utf8",
}),
);
const ownerInspect = spawnSync(
"bun",
["clawhub", "package", "inspect", packageName, ...baseArgs],
{
cwd: process.cwd(),
env: {
...process.env,
CLAWHUB_CONFIG_PATH: ownerCfg.path,
CLAWHUB_DISABLE_TELEMETRY: "1",
},
encoding: "utf8",
},
);
expect(ownerInspect.status, commandOutput(ownerInspect)).toBe(0);
} finally {
spawnSync(
"bun",
[
"clawhub",
"package",
"delete",
packageName,
"--yes",
"--site",
site,
"--registry",
registry,
"--workdir",
workdir,
],
{
cwd: process.cwd(),
env: {
...process.env,
CLAWHUB_CONFIG_PATH: ownerCfg.path,
CLAWHUB_DISABLE_TELEMETRY: "1",
},
encoding: "utf8",
},
);
await rm(workdir, { recursive: true, force: true });
await rm(ownerCfg.dir, { recursive: true, force: true });
await rm(strangerCfg.dir, { recursive: true, force: true });
}
},
180_000,
);
});
+1 -1
View File
@@ -10,7 +10,7 @@
"check": "bun run lint",
"check:peers": "bun scripts/check-peer-deps.ts",
"check:secrets": "bun scripts/check-staged-secrets.mjs",
"ci:e2e-http": "bun run test:e2e:prod-http && bunx vitest run -c vitest.e2e.config.ts e2e/clawhub.e2e.test.ts --testNamePattern \"prints CLI version|search endpoint returns a results array|cli search does not error|package publish --dry-run from a GitHub repo|package publish --dry-run --json|package publish help shows\"",
"ci:e2e-http": "bun run test:e2e:prod-http && bunx vitest run -c vitest.e2e.config.ts e2e/clawhub.e2e.test.ts --testNamePattern \"prints CLI version|search endpoint returns a results array|cli search does not error|package publish --dry-run from a GitHub repo|package publish --dry-run --json|package publish help shows\" && bunx vitest run -c vitest.e2e.config.ts e2e/permissions.e2e.test.ts",
"ci:packages": "bun run --cwd packages/schema build && bun run --cwd packages/clawhub verify && bun run --cwd packages/clawhub-mod verify",
"ci:playwright": "VITE_CONVEX_URL=https://wry-manatee-359.convex.cloud VITE_CONVEX_SITE_URL=https://wry-manatee-359.convex.site bun run build && VITE_CONVEX_URL=https://wry-manatee-359.convex.cloud VITE_CONVEX_SITE_URL=https://wry-manatee-359.convex.site bun run test:pw",
"ci:playwright-smoke": "VITE_CONVEX_URL=https://wry-manatee-359.convex.cloud VITE_CONVEX_SITE_URL=https://wry-manatee-359.convex.site bun run build && VITE_CONVEX_URL=https://wry-manatee-359.convex.cloud VITE_CONVEX_SITE_URL=https://wry-manatee-359.convex.site bun run test:pw -- --project=chromium e2e/ci-smoke.pw.test.ts",
+6 -3
View File
@@ -421,9 +421,12 @@ export declare const ApiV1SkillMergeResponseSchema: import("arktype/internal/var
}, {}>;
export declare const ApiV1TransferRequestResponseSchema: import("arktype/internal/variants/object.ts").ObjectType<{
ok: true;
transferId: string;
toUserHandle: string;
expiresAt: number;
transferId?: string | undefined;
toUserHandle?: string | undefined;
toPublisherHandle?: string | undefined;
skillSlug?: string | undefined;
expiresAt?: number | undefined;
transferred?: boolean | undefined;
}, {}>;
export declare const ApiV1TransferDecisionResponseSchema: import("arktype/internal/variants/object.ts").ObjectType<{
ok: true;
+6 -3
View File
@@ -374,9 +374,12 @@ export const ApiV1SkillMergeResponseSchema = type({
});
export const ApiV1TransferRequestResponseSchema = type({
ok: "true",
transferId: "string",
toUserHandle: "string",
expiresAt: "number",
transferId: "string?",
toUserHandle: "string?",
toPublisherHandle: "string?",
skillSlug: "string?",
expiresAt: "number?",
transferred: "boolean?",
});
export const ApiV1TransferDecisionResponseSchema = type({
ok: "true",
File diff suppressed because one or more lines are too long