mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-11-14 10:39:32 +01:00
fix(files): the review's fixes for the Files panel delete
The fixes #529's review asked for, each with a test. - No inline handler. The button quoted the path inside onclick="...decodeURIComponent('<path>')", and encodeURIComponent leaves ' alone, so a file named x'-alert(1)-'.txt ran script in Codeman's origin and it's.txt was a syntax error. It is now bound in the existing .file-tree-item loops from the row's escaped data-path. - Symbolic links are refused, never followed. validateSessionFilePath() returns the realpath, so deleting AGENTS.md -> CLAUDE.md deleted CLAUDE.md and left the link dangling. The path as named is lstat'ed, a link answers 400, and the inode is compared again before the unlink. - The edit route's confinement whole: validateLocalSessionFilePath() behind withGuardedOutsideWorktree(), so an absolute path in a followed outside worktree obeys the same rules, and the sensitive, blocked and .git checks run on both the realpath and the path as named. - Adopted sessions are refused like remote ones (the /tmp exception of the older file surfaces is not copied to a new one); Docker workspaces are allowed, as on the edit route. The workspace is probed bounded first (#516). Errors are thrown with their status. - Deleting from search results re-runs the search, the row buttons stay visible on touch screens, the confirm and toasts have zh-CN text, and no button is drawn for remote or adopted sessions. - Docs: CLAUDE.md, architecture-invariants, api-reference and the Working With Files wiki page. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
dfe1af2e5f
commit
078bd74bd1
12 files changed
+776
-37
No files matched your search
@@ -1688,6 +1688,11 @@
|
||||
// The file name is user text: it passes through untranslated.
|
||||
[/^Downloaded (.+) to Downloads$/, (_m, name) => `已将 ${name} 下载到“下载”文件夹`],
|
||||
[/^Download of (.+) failed$/, (_m, name) => `下载 ${name} 失败`],
|
||||
// The Files panel's delete button (panels-ui.js): the path is user text, and the server's
|
||||
// reason passes through untranslated.
|
||||
[/^Delete the file ([\s\S]+)\? This cannot be undone\.$/, (_m, path) => `删除文件 ${path}?此操作无法撤销。`],
|
||||
[/^File deleted: ([\s\S]+)$/, (_m, path) => `已删除文件:${path}`],
|
||||
[/^Delete failed: ([\s\S]+)$/, (_m, why) => `删除失败:${why}`],
|
||||
// Group names are user text: they pass through untranslated.
|
||||
[/^Move to "(.+)"$/, (_m, group) => `移到“${group}”`],
|
||||
// Session names are user text too.
|
||||
|
||||
+42
-10
@@ -3777,6 +3777,7 @@ Object.assign(CodemanApp.prototype, {
|
||||
}
|
||||
|
||||
const html = [];
|
||||
const canDelete = this._fileBrowserCanDelete(owner);
|
||||
|
||||
const renderNode = (node, depth) => {
|
||||
const isDir = node.type === 'directory';
|
||||
@@ -3799,8 +3800,12 @@ Object.assign(CodemanApp.prototype, {
|
||||
const downloadBtn = !isDir
|
||||
? `<a class="file-tree-download" href="${escapeHtml(CodemanBase.url(`/api/sessions/${encodeURIComponent(owner)}/file-raw?path=${encodeURIComponent(node.path)}&download=true`))}" title="Download" onclick="event.stopPropagation()">⬇</a>`
|
||||
: '';
|
||||
const deleteBtn = !isDir
|
||||
? `<button class="file-tree-delete" title="Delete" aria-label="Delete ${escapeHtml(node.name)}" onclick="event.stopPropagation(); app.deleteFileFromBrowser(decodeURIComponent('${encodeURIComponent(owner)}'), decodeURIComponent('${encodeURIComponent(node.path)}'))">×</button>`
|
||||
// No inline handler: the path is a workspace file name, which an agent or a cloned repo
|
||||
// chose, and `encodeURIComponent` leaves `'` alone, so a quoted string in an onclick ran
|
||||
// whatever a file name like `x'-alert(1)-'.txt` held (#529's review). The click is bound
|
||||
// below from the row's already-escaped `data-path`.
|
||||
const deleteBtn = !isDir && canDelete
|
||||
? `<button type="button" class="file-tree-delete" title="Delete" aria-label="Delete ${escapeHtml(node.name)}">×</button>`
|
||||
: '';
|
||||
|
||||
html.push(`
|
||||
@@ -3846,9 +3851,21 @@ Object.assign(CodemanApp.prototype, {
|
||||
this.openFilePreview(path, owner);
|
||||
}
|
||||
});
|
||||
item.querySelector('.file-tree-delete')?.addEventListener('click', (e) => {
|
||||
e.stopPropagation();
|
||||
this.deleteFileFromBrowser(owner, item.dataset.path);
|
||||
});
|
||||
});
|
||||
},
|
||||
|
||||
// The delete button is offered only where the route can act: never for a remote (SSH) session,
|
||||
// whose files live on another host, nor for an adopted one, whose folder is not a workspace of
|
||||
// ours (`session.remote` is undefined on an adopted session, so both are tested).
|
||||
_fileBrowserCanDelete(sessionId) {
|
||||
const session = this.sessions?.get?.(sessionId);
|
||||
return !session?.remote && !session?.adopt;
|
||||
},
|
||||
|
||||
toggleFileBrowserFolder(path) {
|
||||
if (this.fileBrowserExpandedDirs.has(path)) {
|
||||
this.fileBrowserExpandedDirs.delete(path);
|
||||
@@ -3974,6 +3991,7 @@ Object.assign(CodemanApp.prototype, {
|
||||
treeEl.innerHTML = `<div class="file-browser-empty">${escapeHtml('No matches')}</div>`;
|
||||
} else {
|
||||
const ownerPath = encodeURIComponent(ownerSessionId);
|
||||
const canDelete = this._fileBrowserCanDelete(ownerSessionId);
|
||||
treeEl.innerHTML = matches
|
||||
.map(match => {
|
||||
const isDir = match.type === 'directory';
|
||||
@@ -3985,8 +4003,9 @@ Object.assign(CodemanApp.prototype, {
|
||||
const downloadBtn = !isDir
|
||||
? `<a class="file-tree-download" href="${escapeHtml(CodemanBase.url(`/api/sessions/${ownerPath}/file-raw?path=${encodeURIComponent(match.path)}&download=true`))}" title="Download" onclick="event.stopPropagation()">⬇</a>`
|
||||
: '';
|
||||
const deleteBtn = !isDir
|
||||
? `<button class="file-tree-delete" title="Delete" aria-label="Delete ${escapeHtml(match.name)}" onclick="event.stopPropagation(); app.deleteFileFromBrowser(decodeURIComponent('${encodeURIComponent(ownerSessionId)}'), decodeURIComponent('${encodeURIComponent(match.path)}'))">×</button>`
|
||||
// Bound below from `data-path`, never an inline handler (see renderFileBrowserTree).
|
||||
const deleteBtn = !isDir && canDelete
|
||||
? `<button type="button" class="file-tree-delete" title="Delete" aria-label="Delete ${escapeHtml(match.name)}">×</button>`
|
||||
: '';
|
||||
return `
|
||||
<div class="file-tree-item" data-path="${escapeHtml(match.path)}" data-type="${escapeHtml(match.type)}" data-owner="${escapeHtml(ownerSessionId)}">
|
||||
@@ -4011,6 +4030,10 @@ Object.assign(CodemanApp.prototype, {
|
||||
this.openFilePreview(path, ownerSessionId);
|
||||
}
|
||||
});
|
||||
item.querySelector('.file-tree-delete')?.addEventListener('click', (e) => {
|
||||
e.stopPropagation();
|
||||
this.deleteFileFromBrowser(ownerSessionId, item.dataset.path);
|
||||
});
|
||||
});
|
||||
|
||||
const statusEl = this.$('fileBrowserStatus');
|
||||
@@ -4020,22 +4043,31 @@ Object.assign(CodemanApp.prototype, {
|
||||
}
|
||||
},
|
||||
|
||||
// The Files panel's delete button (DELETE .../files?path=): asks first, in the same native
|
||||
// dialog the panel's other destructive actions use, then reloads the listing. With a search in
|
||||
// the box the reload re-runs it, so the deleted row leaves the results too; a plain reload
|
||||
// would refresh the tree behind them and leave the row on screen.
|
||||
async deleteFileFromBrowser(sessionId, filePath) {
|
||||
if (!sessionId || !filePath) return;
|
||||
if (!window.confirm(`Delete ${filePath}?`)) return;
|
||||
if (!confirm(`Delete the file ${filePath}? This cannot be undone.`)) return;
|
||||
let result = null;
|
||||
try {
|
||||
const response = await fetch(
|
||||
`/api/sessions/${encodeURIComponent(sessionId)}/files?path=${encodeURIComponent(filePath)}`,
|
||||
{ method: 'DELETE' },
|
||||
);
|
||||
const result = await response.json().catch(() => null);
|
||||
if (!response.ok || !result?.success) throw new Error(result?.error || 'Delete failed');
|
||||
this.showToast(`Deleted: ${filePath}`, 'success');
|
||||
await this.loadFileBrowser(sessionId, { force: true });
|
||||
result = await response.json().catch(() => null);
|
||||
if (!response.ok || result?.success !== true) {
|
||||
throw new Error(result?.error || `HTTP ${response.status}`);
|
||||
}
|
||||
} catch (error) {
|
||||
console.error('File browser delete failed:', error);
|
||||
this.showToast(error?.message || 'Delete failed', 'error');
|
||||
this.showToast(`Delete failed: ${error?.message || 'network error'}`, 'error');
|
||||
return;
|
||||
}
|
||||
this.showToast(`File deleted: ${filePath}`, 'success');
|
||||
const state = this._ensureFileBrowserState();
|
||||
if (state.ownerSessionId === sessionId) await this._reloadFileBrowserKeepingQuery();
|
||||
},
|
||||
|
||||
_findFileBrowserDirectory(nodes, targetPath, ancestors = []) {
|
||||
|
||||
@@ -12404,6 +12404,14 @@ html:is([data-skin="paper-gray"], [data-skin="solarized-light"], [data-skin="cat
|
||||
.file-tree-delete:hover {
|
||||
color: #ef5350;
|
||||
}
|
||||
/* A touch screen has no hover, so the row's buttons stay visible there: otherwise the first tap
|
||||
on a row opens the preview and the buttons never show. */
|
||||
@media (hover: none) {
|
||||
.file-tree-download,
|
||||
.file-tree-delete {
|
||||
display: inline;
|
||||
}
|
||||
}
|
||||
|
||||
/* File tree indent levels */
|
||||
.file-tree-item[data-depth="0"] { padding-left: 0.5rem; }
|
||||
|
||||
@@ -1,9 +1,11 @@
|
||||
/**
|
||||
* @fileoverview File browser and streaming routes.
|
||||
* Provides directory listing, file content preview, raw file serving, tail
|
||||
* streaming, and the File Viewer edit-mode write path (edit=1 read +
|
||||
* streaming, the File Viewer edit-mode write path (edit=1 read +
|
||||
* PUT /api/sessions/:id/file-content; policy in src/config/file-editing.ts,
|
||||
* design in docs/file-viewer-edit-plan.md).
|
||||
* design in docs/file-viewer-edit-plan.md), and the Files panel's delete
|
||||
* (DELETE /api/sessions/:id/files?path=: one regular file, the edit route's
|
||||
* confinement and guards, symbolic links refused).
|
||||
*/
|
||||
|
||||
import { FastifyInstance, type FastifyReply } from 'fastify';
|
||||
@@ -19,6 +21,7 @@ import type {
|
||||
FilesystemBrowseEntry,
|
||||
FilesystemBrowseRoot,
|
||||
FilesystemPreviewKind,
|
||||
FileDeleteData,
|
||||
FileWriteData,
|
||||
} from '../../types.js';
|
||||
import { ApiErrorCode, createErrorResponse, getErrorMessage } from '../../types.js';
|
||||
@@ -2000,45 +2003,100 @@ export function registerFileRoutes(app: FastifyInstance, ctx: SessionPort & Even
|
||||
};
|
||||
});
|
||||
|
||||
// Delete a regular file from the current session workspace. The same
|
||||
// confinement and sensitive-path checks used by previews apply here.
|
||||
app.delete('/api/sessions/:id/files', async (req, reply) => {
|
||||
// The Files panel's delete button: remove ONE regular file from the session's workspace (or from
|
||||
// its outside worktree, for an absolute path inside it). A write surface like the edit route
|
||||
// above, so it takes that route's confinement and guards whole (ownership, realpath + workspace boundary, sensitive and blocked trees, `.git`), and adds
|
||||
// its own rule for links: a symbolic link is REFUSED, never followed. The confinement check
|
||||
// returns the link's realpath, which is its TARGET, so an `unlink` of that path deletes the file
|
||||
// the link points at and leaves the link dangling (#529's review: `AGENTS.md -> CLAUDE.md`
|
||||
// deleted CLAUDE.md). The final component is therefore read with `lstat` on the path as the
|
||||
// client named it, and only a path whose last part is a plain file goes on; `unlink` never
|
||||
// follows a final symlink, so a link swapped in after that check is removed rather than its
|
||||
// target, and the inode is compared again right before the unlink.
|
||||
app.delete('/api/sessions/:id/files', async (req): Promise<ApiResponse<FileDeleteData>> => {
|
||||
const { id } = req.params as { id: string };
|
||||
const { path: filePath } = req.query as { path?: string };
|
||||
const session = findSessionOrFail(ctx, id, req);
|
||||
// Ahead of any path check, as on the edit route: the validation below resolves against the
|
||||
// LOCAL filesystem, so a same-named local directory would take the delete.
|
||||
if (session.remote) {
|
||||
reply.code(400);
|
||||
return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Deleting is not supported for remote (SSH) sessions');
|
||||
throwRouteError(400, ApiErrorCode.INVALID_INPUT, 'Deleting is not supported for remote (SSH) sessions');
|
||||
}
|
||||
if (!filePath) {
|
||||
reply.code(400);
|
||||
return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Missing path parameter');
|
||||
// An adopted session has no workspace of ours: its folder is the neutral `/tmp`. The older
|
||||
// file surfaces keep serving it as a standing exception (root CLAUDE.md, Adopted tmux
|
||||
// sessions), which is not copied to a new surface, least of all one that deletes.
|
||||
if (session.isAdopted) {
|
||||
throwRouteError(400, ApiErrorCode.INVALID_INPUT, 'Deleting is not supported for adopted sessions');
|
||||
}
|
||||
const validated = validateSessionFilePath(session.workingDir, filePath);
|
||||
// A repeated `?path=` arrives as an array.
|
||||
if (typeof filePath !== 'string' || !filePath) {
|
||||
throwRouteError(400, ApiErrorCode.INVALID_INPUT, 'Missing path parameter');
|
||||
}
|
||||
// #516: the confinement check below runs realpathSync, which freezes the server on a mount
|
||||
// that stopped answering. Probe the folders it will touch first, bounded.
|
||||
const scope = await withGuardedOutsideWorktree({
|
||||
workingDir: session.workingDir,
|
||||
liveWorktree: session.liveWorktree,
|
||||
});
|
||||
const worktreeRoot = scope.liveWorktree?.outside && isAbsolute(filePath) ? scope.liveWorktree.path : null;
|
||||
for (const folder of worktreeRoot ? [session.workingDir, worktreeRoot] : [session.workingDir]) {
|
||||
if ((await probePathKind(folder, { pastCap: true })) === 'unknown') {
|
||||
throwRouteError(
|
||||
422,
|
||||
ApiErrorCode.OPERATION_FAILED,
|
||||
describeUnknownPath('Workspace', folder, { pastCap: true })
|
||||
);
|
||||
}
|
||||
}
|
||||
const validated = validateLocalSessionFilePath(scope, filePath);
|
||||
if (!validated) {
|
||||
reply.code(404);
|
||||
return createErrorResponse(ApiErrorCode.NOT_FOUND, 'File not found');
|
||||
// Missing files, traversal and symlink escapes alike, as on every other file route.
|
||||
throwRouteError(404, ApiErrorCode.NOT_FOUND, 'File not found');
|
||||
}
|
||||
const { resolvedPath, relativePath } = validated;
|
||||
// The path as the client named it: `resolve` keeps an absolute (outside worktree) path as is.
|
||||
const namedPath = resolve(session.workingDir, filePath);
|
||||
const guard = await loadAttachmentGuardConfig();
|
||||
if (
|
||||
isSensitivePath(resolvedPath) ||
|
||||
isSensitivePath(namedPath) ||
|
||||
isBlockedAttachmentPath(resolvedPath, guard.blockedTrees) ||
|
||||
isDeniedEditRelativePath(relativePath)
|
||||
isBlockedAttachmentPath(namedPath, guard.blockedTrees)
|
||||
) {
|
||||
reply.code(403);
|
||||
return createErrorResponse(ApiErrorCode.FORBIDDEN, 'Deleting this file is blocked');
|
||||
throwRouteError(403, ApiErrorCode.FORBIDDEN, 'Deleting this file is blocked');
|
||||
}
|
||||
const stat = await fs.lstat(resolvedPath).catch(() => undefined);
|
||||
if (!stat) {
|
||||
reply.code(404);
|
||||
return createErrorResponse(ApiErrorCode.NOT_FOUND, 'File not found');
|
||||
// Both spellings: a `.git` reached through a linked folder, and a linked `.git`.
|
||||
if (isDeniedEditRelativePath(relativePath) || isDeniedEditRelativePath(relative(session.workingDir, namedPath))) {
|
||||
throwRouteError(403, ApiErrorCode.FORBIDDEN, 'Files under .git cannot be deleted');
|
||||
}
|
||||
if (!stat.isFile()) {
|
||||
reply.code(400);
|
||||
return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Only regular files can be deleted');
|
||||
const named = await fs.lstat(namedPath).catch(() => undefined);
|
||||
if (!named) {
|
||||
throwRouteError(404, ApiErrorCode.NOT_FOUND, 'File not found');
|
||||
}
|
||||
if (named.isSymbolicLink()) {
|
||||
throwRouteError(
|
||||
400,
|
||||
ApiErrorCode.INVALID_INPUT,
|
||||
'Symbolic links are not deleted from the Files panel (deleting one here would remove the file it points to)'
|
||||
);
|
||||
}
|
||||
if (!named.isFile()) {
|
||||
throwRouteError(400, ApiErrorCode.INVALID_INPUT, 'Only regular files can be deleted');
|
||||
}
|
||||
// The same file the checks were made on: realpath of a path whose last part is no link names
|
||||
// the same inode. A change in between (the file replaced) refuses rather than guesses.
|
||||
const real = await fs.lstat(resolvedPath).catch(() => undefined);
|
||||
if (!real || !real.isFile() || real.ino !== named.ino || real.dev !== named.dev) {
|
||||
throwRouteError(409, ApiErrorCode.CONFLICT, 'The file changed while it was being deleted; reload and try again');
|
||||
}
|
||||
try {
|
||||
await fs.unlink(resolvedPath);
|
||||
} catch (err) {
|
||||
const code = (err as NodeJS.ErrnoException).code;
|
||||
if (code === 'ENOENT') throwRouteError(404, ApiErrorCode.NOT_FOUND, 'File not found');
|
||||
// The errno text names the resolved path, so only its code goes back.
|
||||
throwRouteError(500, ApiErrorCode.OPERATION_FAILED, `Failed to delete the file (${code ?? 'error'})`);
|
||||
}
|
||||
await fs.unlink(resolvedPath);
|
||||
return { success: true, data: { path: filePath } };
|
||||
});
|
||||
|
||||
|
||||
Reference in new issue
Block a user