fix(security): harden control-ui static path resolution
This commit is contained in:
Peter Steinberger
@@ -125,4 +125,87 @@ describe("handleControlUiHttpRequest", () => {
|
||||
},
|
||||
});
|
||||
});
|
||||
|
||||
it("rejects symlinked assets that resolve outside control-ui root", async () => {
|
||||
await withControlUiRoot({
|
||||
fn: async (tmp) => {
|
||||
const assetsDir = path.join(tmp, "assets");
|
||||
const outsideDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-ui-outside-"));
|
||||
try {
|
||||
const outsideFile = path.join(outsideDir, "secret.txt");
|
||||
await fs.mkdir(assetsDir, { recursive: true });
|
||||
await fs.writeFile(outsideFile, "outside-secret\n");
|
||||
await fs.symlink(outsideFile, path.join(assetsDir, "leak.txt"));
|
||||
|
||||
const { res, end } = makeMockHttpResponse();
|
||||
const handled = handleControlUiHttpRequest(
|
||||
{ url: "/assets/leak.txt", method: "GET" } as IncomingMessage,
|
||||
res,
|
||||
{
|
||||
root: { kind: "resolved", path: tmp },
|
||||
},
|
||||
);
|
||||
|
||||
expect(handled).toBe(true);
|
||||
expect(res.statusCode).toBe(404);
|
||||
expect(end).toHaveBeenCalledWith("Not Found");
|
||||
} finally {
|
||||
await fs.rm(outsideDir, { recursive: true, force: true });
|
||||
}
|
||||
},
|
||||
});
|
||||
});
|
||||
|
||||
it("allows symlinked assets that resolve inside control-ui root", async () => {
|
||||
await withControlUiRoot({
|
||||
fn: async (tmp) => {
|
||||
const assetsDir = path.join(tmp, "assets");
|
||||
await fs.mkdir(assetsDir, { recursive: true });
|
||||
await fs.writeFile(path.join(assetsDir, "actual.txt"), "inside-ok\n");
|
||||
await fs.symlink(path.join(assetsDir, "actual.txt"), path.join(assetsDir, "linked.txt"));
|
||||
|
||||
const { res, end } = makeMockHttpResponse();
|
||||
const handled = handleControlUiHttpRequest(
|
||||
{ url: "/assets/linked.txt", method: "GET" } as IncomingMessage,
|
||||
res,
|
||||
{
|
||||
root: { kind: "resolved", path: tmp },
|
||||
},
|
||||
);
|
||||
|
||||
expect(handled).toBe(true);
|
||||
expect(res.statusCode).toBe(200);
|
||||
expect(String(end.mock.calls[0]?.[0] ?? "")).toBe("inside-ok\n");
|
||||
},
|
||||
});
|
||||
});
|
||||
|
||||
it("rejects symlinked SPA fallback index.html outside control-ui root", async () => {
|
||||
await withControlUiRoot({
|
||||
fn: async (tmp) => {
|
||||
const outsideDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-ui-index-outside-"));
|
||||
try {
|
||||
const outsideIndex = path.join(outsideDir, "index.html");
|
||||
await fs.writeFile(outsideIndex, "<html>outside</html>\n");
|
||||
await fs.rm(path.join(tmp, "index.html"));
|
||||
await fs.symlink(outsideIndex, path.join(tmp, "index.html"));
|
||||
|
||||
const { res, end } = makeMockHttpResponse();
|
||||
const handled = handleControlUiHttpRequest(
|
||||
{ url: "/app/route", method: "GET" } as IncomingMessage,
|
||||
res,
|
||||
{
|
||||
root: { kind: "resolved", path: tmp },
|
||||
},
|
||||
);
|
||||
|
||||
expect(handled).toBe(true);
|
||||
expect(res.statusCode).toBe(404);
|
||||
expect(end).toHaveBeenCalledWith("Not Found");
|
||||
} finally {
|
||||
await fs.rm(outsideDir, { recursive: true, force: true });
|
||||
}
|
||||
},
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -188,10 +188,72 @@ function serveFile(res: ServerResponse, filePath: string) {
|
||||
res.end(fs.readFileSync(filePath));
|
||||
}
|
||||
|
||||
function serveIndexHtml(res: ServerResponse, indexPath: string) {
|
||||
function serveResolvedFile(res: ServerResponse, filePath: string, body: Buffer) {
|
||||
const ext = path.extname(filePath).toLowerCase();
|
||||
res.setHeader("Content-Type", contentTypeForExt(ext));
|
||||
res.setHeader("Cache-Control", "no-cache");
|
||||
res.end(body);
|
||||
}
|
||||
|
||||
function serveResolvedIndexHtml(res: ServerResponse, body: string) {
|
||||
res.setHeader("Content-Type", "text/html; charset=utf-8");
|
||||
res.setHeader("Cache-Control", "no-cache");
|
||||
res.end(fs.readFileSync(indexPath, "utf8"));
|
||||
res.end(body);
|
||||
}
|
||||
|
||||
function isContainedPath(baseDir: string, targetPath: string): boolean {
|
||||
const relative = path.relative(baseDir, targetPath);
|
||||
return relative !== ".." && !relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative);
|
||||
}
|
||||
|
||||
function isExpectedSafePathError(error: unknown): boolean {
|
||||
const code =
|
||||
typeof error === "object" && error !== null && "code" in error ? String(error.code) : "";
|
||||
return code === "ENOENT" || code === "ENOTDIR" || code === "ELOOP";
|
||||
}
|
||||
|
||||
function areSameFileIdentity(preOpen: fs.Stats, opened: fs.Stats): boolean {
|
||||
return preOpen.dev === opened.dev && preOpen.ino === opened.ino;
|
||||
}
|
||||
|
||||
function resolveSafeControlUiFile(
|
||||
root: string,
|
||||
filePath: string,
|
||||
): { path: string; body: Buffer } | null {
|
||||
let fd: number | null = null;
|
||||
try {
|
||||
const rootReal = fs.realpathSync(root);
|
||||
const fileReal = fs.realpathSync(filePath);
|
||||
if (!isContainedPath(rootReal, fileReal)) {
|
||||
return null;
|
||||
}
|
||||
|
||||
const preOpenStat = fs.lstatSync(fileReal);
|
||||
if (!preOpenStat.isFile()) {
|
||||
return null;
|
||||
}
|
||||
|
||||
const openFlags =
|
||||
fs.constants.O_RDONLY |
|
||||
(typeof fs.constants.O_NOFOLLOW === "number" ? fs.constants.O_NOFOLLOW : 0);
|
||||
fd = fs.openSync(fileReal, openFlags);
|
||||
const openedStat = fs.fstatSync(fd);
|
||||
// Compare inode identity so swaps between validation and open are rejected.
|
||||
if (!openedStat.isFile() || !areSameFileIdentity(preOpenStat, openedStat)) {
|
||||
return null;
|
||||
}
|
||||
|
||||
return { path: fileReal, body: fs.readFileSync(fd) };
|
||||
} catch (error) {
|
||||
if (isExpectedSafePathError(error)) {
|
||||
return null;
|
||||
}
|
||||
throw error;
|
||||
} finally {
|
||||
if (fd !== null) {
|
||||
fs.closeSync(fd);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
function isSafeRelativePath(relPath: string) {
|
||||
@@ -340,12 +402,13 @@ export function handleControlUiHttpRequest(
|
||||
return true;
|
||||
}
|
||||
|
||||
if (fs.existsSync(filePath) && fs.statSync(filePath).isFile()) {
|
||||
if (path.basename(filePath) === "index.html") {
|
||||
serveIndexHtml(res, filePath);
|
||||
const safeFile = resolveSafeControlUiFile(root, filePath);
|
||||
if (safeFile) {
|
||||
if (path.basename(safeFile.path) === "index.html") {
|
||||
serveResolvedIndexHtml(res, safeFile.body.toString("utf8"));
|
||||
return true;
|
||||
}
|
||||
serveFile(res, filePath);
|
||||
serveResolvedFile(res, safeFile.path, safeFile.body);
|
||||
return true;
|
||||
}
|
||||
|
||||
@@ -361,8 +424,9 @@ export function handleControlUiHttpRequest(
|
||||
|
||||
// SPA fallback (client-side router): serve index.html for unknown paths.
|
||||
const indexPath = path.join(root, "index.html");
|
||||
if (fs.existsSync(indexPath)) {
|
||||
serveIndexHtml(res, indexPath);
|
||||
const safeIndex = resolveSafeControlUiFile(root, indexPath);
|
||||
if (safeIndex) {
|
||||
serveResolvedIndexHtml(res, safeIndex.body.toString("utf8"));
|
||||
return true;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user