mirror of
https://github.com/openclaw/openclaw.git
synced 2026-07-21 10:16:44 +00:00
fix: reject unsafe Mattermost API path segments (#98390)
* Harden Mattermost API paths * fix(mattermost): guard direct media file paths * refactor(mattermost): streamline API path validation --------- Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
co-authored by
Peter Steinberger
parent
a83ed13204
commit
9769642e7f
@@ -368,6 +368,45 @@ describe("createMattermostClient", () => {
|
||||
);
|
||||
});
|
||||
|
||||
it("rejects relative API path segments before fetch", async () => {
|
||||
const { client, calls } = createTestClient();
|
||||
|
||||
await expect(client.request("/posts/../users/me")).rejects.toThrow(
|
||||
"Mattermost API path must not contain unsafe path segments",
|
||||
);
|
||||
|
||||
expect(calls).toEqual([]);
|
||||
});
|
||||
|
||||
it("rejects encoded relative API path segments before fetch", async () => {
|
||||
const { client, calls } = createTestClient();
|
||||
|
||||
await expect(client.request("/posts/%2e%2e/users/me")).rejects.toThrow(
|
||||
"Mattermost API path must not contain unsafe path segments",
|
||||
);
|
||||
|
||||
expect(calls).toEqual([]);
|
||||
});
|
||||
|
||||
it("rejects URL-normalized relative API path bypasses before fetch", async () => {
|
||||
const { client, calls } = createTestClient();
|
||||
|
||||
for (const path of [
|
||||
"/posts/..?x=1",
|
||||
"/posts/%2e%2e?x=1",
|
||||
"/posts\\..\\users/me",
|
||||
"/posts/.\n./users/me",
|
||||
"/posts/.%0a./users/me",
|
||||
"/posts/%2e%2e%2fusers%80%2f..%2fme",
|
||||
]) {
|
||||
await expect(client.request(path)).rejects.toThrow(
|
||||
"Mattermost API path must not contain unsafe path segments",
|
||||
);
|
||||
}
|
||||
|
||||
expect(calls).toEqual([]);
|
||||
});
|
||||
|
||||
it("sends Authorization header with Bearer token", async () => {
|
||||
const { mockFetch, calls } = createMockFetch({ body: { id: "u1" } });
|
||||
const client = createMattermostClient({
|
||||
|
||||
@@ -91,11 +91,23 @@ export function normalizeMattermostBaseUrl(raw?: string | null): string | undefi
|
||||
return withoutTrailing.replace(/\/api\/v4$/i, "");
|
||||
}
|
||||
|
||||
function buildMattermostApiUrl(baseUrl: string, path: string): string {
|
||||
export function buildMattermostApiUrl(baseUrl: string, path: string): string {
|
||||
const normalized = normalizeMattermostBaseUrl(baseUrl);
|
||||
if (!normalized) {
|
||||
throw new Error("Mattermost baseUrl is required");
|
||||
}
|
||||
const pathname = (path.split(/[?#]/, 1)[0] ?? "").replace(/[\t\r\n]/g, "").replace(/\\/g, "/");
|
||||
for (const segment of pathname.split("/")) {
|
||||
let decoded: string;
|
||||
try {
|
||||
decoded = decodeURIComponent(segment).replace(/[\t\r\n]/g, "");
|
||||
} catch {
|
||||
throw new Error("Mattermost API path must not contain unsafe path segments");
|
||||
}
|
||||
if (decoded.split(/[\\/]/).some((part) => part === "." || part === "..")) {
|
||||
throw new Error("Mattermost API path must not contain unsafe path segments");
|
||||
}
|
||||
}
|
||||
const suffix = path.startsWith("/") ? path : `/${path}`;
|
||||
return `${normalized}/api/v4${suffix}`;
|
||||
}
|
||||
|
||||
@@ -7,12 +7,16 @@ const sendMattermostTyping = vi.hoisted(() => vi.fn());
|
||||
const updateMattermostPost = vi.hoisted(() => vi.fn());
|
||||
const buildButtonProps = vi.hoisted(() => vi.fn());
|
||||
|
||||
vi.mock("./client.js", () => ({
|
||||
fetchMattermostChannel,
|
||||
fetchMattermostUser,
|
||||
sendMattermostTyping,
|
||||
updateMattermostPost,
|
||||
}));
|
||||
vi.mock("./client.js", async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import("./client.js")>();
|
||||
return {
|
||||
...actual,
|
||||
fetchMattermostChannel,
|
||||
fetchMattermostUser,
|
||||
sendMattermostTyping,
|
||||
updateMattermostPost,
|
||||
};
|
||||
});
|
||||
|
||||
vi.mock("./interactions.js", () => ({
|
||||
buildButtonProps,
|
||||
@@ -104,6 +108,35 @@ describe("mattermost monitor resources", () => {
|
||||
});
|
||||
});
|
||||
|
||||
it("rejects unsafe file paths before media download", async () => {
|
||||
const saveRemoteMedia = vi.fn();
|
||||
const resources = createMattermostMonitorResources({
|
||||
accountId: "default",
|
||||
callbackUrl: "https://openclaw.test/callback",
|
||||
client: {
|
||||
apiBaseUrl: "https://chat.example.com/api/v4",
|
||||
baseUrl: "https://chat.example.com",
|
||||
token: "test-token",
|
||||
} as never,
|
||||
logger: {},
|
||||
mediaMaxBytes: 1024,
|
||||
saveRemoteMedia,
|
||||
mediaKindFromMime: () => "document",
|
||||
});
|
||||
|
||||
await expect(
|
||||
resources.resolveMattermostMedia([
|
||||
"../users/me",
|
||||
"%2e%2e/users/me",
|
||||
"..\\users\\me",
|
||||
".%0a./users/me",
|
||||
"%",
|
||||
]),
|
||||
).resolves.toEqual([]);
|
||||
|
||||
expect(saveRemoteMedia).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("times out inbound media downloads when response headers never arrive", async () => {
|
||||
const { createServer } = await import("node:http");
|
||||
const { saveRemoteMedia } = await import("openclaw/plugin-sdk/media-runtime");
|
||||
|
||||
@@ -7,6 +7,7 @@ import {
|
||||
} from "openclaw/plugin-sdk/number-runtime";
|
||||
import { normalizeStringEntries } from "openclaw/plugin-sdk/string-coerce-runtime";
|
||||
import {
|
||||
buildMattermostApiUrl,
|
||||
fetchMattermostChannel,
|
||||
fetchMattermostUser,
|
||||
sendMattermostTyping,
|
||||
@@ -124,7 +125,7 @@ export function createMattermostMonitorResources(params: {
|
||||
for (const fileId of ids) {
|
||||
try {
|
||||
const saved = await saveRemoteMedia({
|
||||
url: `${client.apiBaseUrl}/files/${fileId}`,
|
||||
url: buildMattermostApiUrl(client.baseUrl, `/files/${fileId}`),
|
||||
requestInit: {
|
||||
headers: {
|
||||
Authorization: `Bearer ${client.token}`,
|
||||
|
||||
Reference in New Issue
Block a user