Address PR review feedback (#1466)

- routes.ts: use isAbsolute() before resolve() for proper path validation
- routes.ts: wrap mkdir() in try/catch to map EEXIST/ENOENT/ENOTDIR to 4xx
- DirectoryPicker.tsx: use !browser.currentPath guard to prevent re-fetch loop
- DirectoryPicker.tsx: disable Create button during in-flight requests
- DirectoryPicker.tsx: add fetchEntries to useEffect dependency array
- DirectoryPicker.tsx: client-side validation for folder names
- Add changeset for the new create-folder feature
This commit is contained in:
gsxdsm
2026-06-06 06:31:44 -07:00
parent 020a1cf7b9
commit ee5f5e84ac
3 changed files with 57 additions and 16 deletions

View File

@@ -74,13 +74,12 @@ export function DirectoryPicker({ value, onChange, placeholder, onInputKeyDown,
});
}, []);
// Fetch when browser opens
// Fetch when browser opens (only for the initial open before any path has been fetched)
useEffect(() => {
if (browser.isOpen && !browser.loading && browser.entries.length === 0 && !browser.error) {
// Use browser.currentPath if available (user has navigated), otherwise fall back to value prop
fetchEntries(browser.currentPath || value || undefined, browser.showHidden);
if (browser.isOpen && !browser.loading && !browser.currentPath && !browser.error) {
fetchEntries(value || undefined, browser.showHidden);
}
}, [browser.isOpen, browser.loading, browser.entries.length, browser.error, value, browser.showHidden, fetchEntries, nodeId, localNodeId]);
}, [browser.isOpen, browser.loading, browser.currentPath, browser.error, value, browser.showHidden, fetchEntries, nodeId, localNodeId]);
const handleNavigate = useCallback(
(path: string) => {
@@ -106,7 +105,8 @@ export function DirectoryPicker({ value, onChange, placeholder, onInputKeyDown,
if (browser.isOpen && browser.currentPath) {
fetchEntries(browser.currentPath, browser.showHidden);
}
}, [browser.showHidden]);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [browser.showHidden, fetchEntries]);
const handleToggleCreateFolder = useCallback(() => {
setBrowser((prev) => ({
@@ -120,12 +120,22 @@ export function DirectoryPicker({ value, onChange, placeholder, onInputKeyDown,
const handleCreateFolder = useCallback(async () => {
if (!newFolderName.trim() || !browser.currentPath) return;
// Validate folder name doesn't contain path separators or traversal
const trimmedName = newFolderName.trim();
if (trimmedName.includes("/") || trimmedName.includes("\\") || trimmedName.includes("..")) {
setBrowser((prev) => ({
...prev,
createFolderError: "Folder name cannot contain path separators or '..'",
}));
return;
}
// Normalize path separator for the current platform by using the same
// separator already present in currentPath
const sep = browser.currentPath.includes("\\") ? "\\" : "/";
const folderPath = browser.currentPath.endsWith(sep)
? browser.currentPath + newFolderName.trim()
: browser.currentPath + sep + newFolderName.trim();
? browser.currentPath + trimmedName
: browser.currentPath + sep + trimmedName;
setBrowser((prev) => ({ ...prev, loading: true, createFolderError: null }));
try {
@@ -287,7 +297,7 @@ export function DirectoryPicker({ value, onChange, placeholder, onInputKeyDown,
type="button"
className="btn btn-sm btn-primary"
onClick={handleCreateFolder}
disabled={!newFolderName.trim()}
disabled={!newFolderName.trim() || browser.loading}
>
{t("dirPicker.createFolderConfirm", "Create")}
</button>

View File

@@ -4463,22 +4463,22 @@ export function createApiRoutes(store: TaskStore, options?: ServerOptions): Rout
*/
router.post("/create-directory", async (req, res) => {
try {
const { resolve } = await import("node:path");
const { resolve, isAbsolute } = await import("node:path");
const { mkdir, stat } = await import("node:fs/promises");
const rawPath = req.body?.path as string | undefined;
const rawPath = typeof req.body?.path === "string" ? req.body.path.trim() : "";
if (!rawPath) {
throw badRequest("Path is required");
}
// Validate: must be absolute, no .. traversal
const resolvedPath = resolve(rawPath);
if (!isAbsolute(rawPath)) {
throw badRequest("Path must be absolute");
}
if (rawPath.includes("..")) {
throw badRequest("Path must not contain '..' traversal");
}
if (resolvedPath !== resolve(resolvedPath)) {
throw badRequest("Path must be absolute");
}
const resolvedPath = resolve(rawPath);
// Check if path already exists
try {
@@ -4512,7 +4512,21 @@ export function createApiRoutes(store: TaskStore, options?: ServerOptions): Rout
}
// Create the directory
await mkdir(resolvedPath);
try {
await mkdir(resolvedPath);
} catch (err: unknown) {
const e = err as NodeJS.ErrnoException;
if (e.code === "EEXIST") {
throw badRequest("Directory already exists");
}
if (e.code === "ENOENT") {
throw badRequest("Parent directory does not exist");
}
if (e.code === "ENOTDIR") {
throw badRequest("Parent path is not a directory");
}
throw err;
}
res.json({ success: true, path: resolvedPath });
} catch (err: unknown) {