Fix data loss: copying a file onto itself via two sessions truncated it to 0
Two panels on the same server hold two SSH sessions by design, and every self-copy guard compared sessionId — so same-server same-path copies slipped through all of them, and createWriteStream truncated the destination before the read began: the file (or every file in a folder) became 0 bytes, with the overwrite dialog making it look like a legitimate replace. Fix, defense in depth, keyed on FILESYSTEM identity instead of session: - fsKey on every provider: 'local', or resolvedIP:port for SFTP (host is DNS-resolved at connect so hostname-vs-IP aliases compare equal; username excluded — same host:port is the same filesystem regardless of login) - engine guard blocks equal/nested paths when fsKeys match; per-file belt-and-suspenders check right before streams open - probe layer for hidden aliases (different addresses, same machine): when fsKeys differ but paths collide, a token file written via dst and read via src decides; visible => same machine => blocked. SFTP exposes no dev/inode (what cp/rsync use), so probing is the only deterministic test; fails open and cleans up after itself - UI: same-folder toast now fires across two sessions to one server, and the overwrite dialog auto-skips (never offers Replace) when the colliding item IS the source file Known residual, documented in README 'Same-file protection': one filesystem mounted at different paths on two connections is undetectable client-side. Regression suite against the real box: file-onto-itself and folder-onto-itself blocked with contents byte-intact, folder-into-descendant blocked, legit same-server different-dir copies still work, local same-path still blocked, simulated alias caught by the probe with cleanup verified. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -142,6 +142,31 @@ Saved connections live in a plain JSON file in the OS app-data directory
|
|||||||
|
|
||||||
(On Macs, function keys may need `fn` — every action also has a toolbar button.)
|
(On Macs, function keys may need `fn` — every action also has a toolbar button.)
|
||||||
|
|
||||||
|
## Same-file protection
|
||||||
|
|
||||||
|
Copying a file or folder onto itself would truncate it to zero bytes (the
|
||||||
|
destination is opened for writing before the source is read). BunSCP blocks
|
||||||
|
this at several layers:
|
||||||
|
|
||||||
|
- **Same session / same host**: every connection carries a filesystem identity
|
||||||
|
(`local`, or `resolvedIP:port` — the hostname is DNS-resolved, so
|
||||||
|
`myserver.lan` and its IP compare equal even across two panels/tabs holding
|
||||||
|
separate SSH sessions to the same server). Equal identity + equal or nested
|
||||||
|
paths ⇒ the transfer is refused before anything is opened.
|
||||||
|
- **Hidden aliases** (two *different* addresses that reach the same machine —
|
||||||
|
NAT, split DNS): when identities differ but source and destination paths
|
||||||
|
collide anyway, a probe file is written through one connection and looked up
|
||||||
|
through the other; if it's visible on both, they're the same machine and the
|
||||||
|
copy is refused. (SFTP offers no device/inode identity — the mechanism
|
||||||
|
`cp`/`rsync` use locally — so probing is the only deterministic test.)
|
||||||
|
- The overwrite dialog never offers to "Replace" a file with itself — such
|
||||||
|
items are auto-skipped with a notice.
|
||||||
|
|
||||||
|
**Known residual limitation**: the same filesystem mounted at *different
|
||||||
|
paths* on two connections (bind mounts, NFS exported twice, shared container
|
||||||
|
volumes) cannot be detected — the paths differ, so no guard or probe triggers.
|
||||||
|
Be careful when two panels reach the same data through different mount points.
|
||||||
|
|
||||||
## Portability notes
|
## Portability notes
|
||||||
|
|
||||||
The codebase is platform-neutral except for two seams, both macOS-only today:
|
The codebase is platform-neutral except for two seams, both macOS-only today:
|
||||||
|
|||||||
@@ -24,6 +24,7 @@ function toEntry(name: string, full: string, st: Stats): FileEntry {
|
|||||||
|
|
||||||
export class LocalProvider implements FileSystemProvider {
|
export class LocalProvider implements FileSystemProvider {
|
||||||
readonly kind = "local" as const;
|
readonly kind = "local" as const;
|
||||||
|
readonly fsKey = "local";
|
||||||
|
|
||||||
join = (...parts: string[]) => path.join(...parts);
|
join = (...parts: string[]) => path.join(...parts);
|
||||||
dirname = (p: string) => path.dirname(p);
|
dirname = (p: string) => path.dirname(p);
|
||||||
|
|||||||
@@ -12,6 +12,15 @@ import type { FileEntry } from "../../shared/types";
|
|||||||
*/
|
*/
|
||||||
export interface FileSystemProvider {
|
export interface FileSystemProvider {
|
||||||
readonly kind: "local" | "sftp";
|
readonly kind: "local" | "sftp";
|
||||||
|
/**
|
||||||
|
* Identity of the FILESYSTEM this provider reaches, independent of the
|
||||||
|
* session: "local", or "<resolvedIp>:<port>" for SFTP. Two panels connected
|
||||||
|
* to the same server hold two SSH sessions but share one fsKey — self-copy
|
||||||
|
* guards key on this, never on sessionId (a sessionId comparison caused a
|
||||||
|
* truncate-to-zero data-loss bug). Username is deliberately excluded: the
|
||||||
|
* same path on the same host:port is the same file regardless of login.
|
||||||
|
*/
|
||||||
|
readonly fsKey: string;
|
||||||
join(...parts: string[]): string;
|
join(...parts: string[]): string;
|
||||||
dirname(p: string): string;
|
dirname(p: string): string;
|
||||||
basename(p: string): string;
|
basename(p: string): string;
|
||||||
|
|||||||
+17
-2
@@ -1,3 +1,4 @@
|
|||||||
|
import dns from "node:dns/promises";
|
||||||
import { readFileSync } from "node:fs";
|
import { readFileSync } from "node:fs";
|
||||||
import os from "node:os";
|
import os from "node:os";
|
||||||
import path from "node:path";
|
import path from "node:path";
|
||||||
@@ -24,12 +25,16 @@ function expandTilde(p: string): string {
|
|||||||
|
|
||||||
export class SftpProvider implements FileSystemProvider {
|
export class SftpProvider implements FileSystemProvider {
|
||||||
readonly kind = "sftp" as const;
|
readonly kind = "sftp" as const;
|
||||||
|
readonly fsKey: string;
|
||||||
private closedByUs = false;
|
private closedByUs = false;
|
||||||
|
|
||||||
private constructor(
|
private constructor(
|
||||||
private client: Client,
|
private client: Client,
|
||||||
private sftp: SFTPWrapper,
|
private sftp: SFTPWrapper,
|
||||||
) {}
|
fsKey: string,
|
||||||
|
) {
|
||||||
|
this.fsKey = fsKey;
|
||||||
|
}
|
||||||
|
|
||||||
/** Resolves once the SSH connection is ready and the SFTP subsystem is open. */
|
/** Resolves once the SSH connection is ready and the SFTP subsystem is open. */
|
||||||
static async connect(
|
static async connect(
|
||||||
@@ -51,6 +56,16 @@ export class SftpProvider implements FileSystemProvider {
|
|||||||
if (config.auth.passphrase) connectConfig.passphrase = config.auth.passphrase;
|
if (config.auth.passphrase) connectConfig.passphrase = config.auth.passphrase;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Filesystem identity: resolve the host so a hostname and its IP compare
|
||||||
|
// equal (self-copy guards). Falls back to the raw host string offline.
|
||||||
|
let resolvedHost = config.host.toLowerCase();
|
||||||
|
try {
|
||||||
|
resolvedHost = (await dns.lookup(config.host)).address;
|
||||||
|
} catch {
|
||||||
|
// unresolvable (e.g. already an IP on some systems) — keep the string
|
||||||
|
}
|
||||||
|
const fsKey = `${resolvedHost}:${config.port}`;
|
||||||
|
|
||||||
const client = new Client();
|
const client = new Client();
|
||||||
const provider = await new Promise<SftpProvider>((resolve, reject) => {
|
const provider = await new Promise<SftpProvider>((resolve, reject) => {
|
||||||
client
|
client
|
||||||
@@ -60,7 +75,7 @@ export class SftpProvider implements FileSystemProvider {
|
|||||||
client.end();
|
client.end();
|
||||||
reject(err);
|
reject(err);
|
||||||
} else {
|
} else {
|
||||||
resolve(new SftpProvider(client, sftp));
|
resolve(new SftpProvider(client, sftp, fsKey));
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -53,7 +53,7 @@ export function createAppRPC({ registry, engine, store, settings, edits, events
|
|||||||
sessionId = registry.add(provider);
|
sessionId = registry.add(provider);
|
||||||
const homeDir = await provider.homeDir();
|
const homeDir = await provider.homeDir();
|
||||||
console.log(`[rpc] connected ${sessionId} -> ${config.username}@${config.host}`);
|
console.log(`[rpc] connected ${sessionId} -> ${config.username}@${config.host}`);
|
||||||
return { sessionId, homeDir };
|
return { sessionId, homeDir, fsKey: provider.fsKey };
|
||||||
}),
|
}),
|
||||||
|
|
||||||
disconnect: ({ sessionId }) =>
|
disconnect: ({ sessionId }) =>
|
||||||
|
|||||||
@@ -181,6 +181,9 @@ export class TransferEngine {
|
|||||||
try {
|
try {
|
||||||
const src = this.registry.get(snap.srcSessionId);
|
const src = this.registry.get(snap.srcSessionId);
|
||||||
const dst = this.registry.get(snap.dstSessionId);
|
const dst = this.registry.get(snap.dstSessionId);
|
||||||
|
// same FILESYSTEM (never compare sessionIds — two panels on one server
|
||||||
|
// hold two sessions, and that comparison once truncated files to zero)
|
||||||
|
const sameFs = src.fsKey === dst.fsKey;
|
||||||
|
|
||||||
snap.state = "scanning";
|
snap.state = "scanning";
|
||||||
this.emit(job, true);
|
this.emit(job, true);
|
||||||
@@ -189,7 +192,7 @@ export class TransferEngine {
|
|||||||
let files: { srcPath: string; dstPath: string; size: number; name: string }[];
|
let files: { srcPath: string; dstPath: string; size: number; name: string }[];
|
||||||
if (job.type === "dir") {
|
if (job.type === "dir") {
|
||||||
const dstRoot = dst.join(snap.dstDir, snap.name);
|
const dstRoot = dst.join(snap.dstDir, snap.name);
|
||||||
this.guardAgainstSelfCopy(snap, dstRoot);
|
await this.guardAgainstSelfCopy(snap, dstRoot, sameFs, src, dst, true);
|
||||||
const tree = await walkTree(src, snap.srcPath);
|
const tree = await walkTree(src, snap.srcPath);
|
||||||
for (const segments of tree.dirs) {
|
for (const segments of tree.dirs) {
|
||||||
if (job.cancelled) throw new Error("transfer cancelled");
|
if (job.cancelled) throw new Error("transfer cancelled");
|
||||||
@@ -203,11 +206,21 @@ export class TransferEngine {
|
|||||||
}));
|
}));
|
||||||
} else {
|
} else {
|
||||||
const dstPath = dst.join(snap.dstDir, snap.name);
|
const dstPath = dst.join(snap.dstDir, snap.name);
|
||||||
this.guardAgainstSelfCopy(snap, dstPath);
|
await this.guardAgainstSelfCopy(snap, dstPath, sameFs, src, dst, false);
|
||||||
const st = await src.stat(snap.srcPath);
|
const st = await src.stat(snap.srcPath);
|
||||||
files = [{ srcPath: snap.srcPath, dstPath, size: st.size, name: snap.name }];
|
files = [{ srcPath: snap.srcPath, dstPath, size: st.size, name: snap.name }];
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// belt & suspenders: an identical path must never reach the write
|
||||||
|
// stream (createWriteStream truncates the destination immediately)
|
||||||
|
if (sameFs) {
|
||||||
|
for (const file of files) {
|
||||||
|
if (file.srcPath === file.dstPath) {
|
||||||
|
throw new Error(`"${file.name}": source and destination are the same file`);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
snap.totalFiles = files.length;
|
snap.totalFiles = files.length;
|
||||||
snap.totalBytes = files.reduce((sum, f) => sum + f.size, 0);
|
snap.totalBytes = files.reduce((sum, f) => sum + f.size, 0);
|
||||||
snap.state = "running";
|
snap.state = "running";
|
||||||
@@ -281,15 +294,79 @@ export class TransferEngine {
|
|||||||
job.activeStreams.clear();
|
job.activeStreams.clear();
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Copying onto itself truncates the source; into a descendant never ends. */
|
/**
|
||||||
private guardAgainstSelfCopy(snap: TransferSnapshot, dstPath: string): void {
|
* Copying onto itself truncates the source; into a descendant never ends.
|
||||||
if (snap.srcSessionId !== snap.dstSessionId) return;
|
* Keyed on filesystem identity (fsKey), not sessionId. When the keys differ
|
||||||
if (dstPath === snap.srcPath) {
|
* but the paths collide anyway, a probe file decides whether the two
|
||||||
throw new Error("Source and destination are the same");
|
* connections secretly reach the same machine (NAT / alias hosts).
|
||||||
|
*/
|
||||||
|
private async guardAgainstSelfCopy(
|
||||||
|
snap: TransferSnapshot,
|
||||||
|
dstPath: string,
|
||||||
|
sameFs: boolean,
|
||||||
|
src: FileSystemProvider,
|
||||||
|
dst: FileSystemProvider,
|
||||||
|
isDir: boolean,
|
||||||
|
): Promise<void> {
|
||||||
|
const samePath = dstPath === snap.srcPath;
|
||||||
|
const nestedPath = dstPath.startsWith(
|
||||||
|
snap.srcPath.endsWith("/") ? snap.srcPath : `${snap.srcPath}/`,
|
||||||
|
);
|
||||||
|
if (!samePath && !nestedPath) return;
|
||||||
|
|
||||||
|
// Paths collide. Same fsKey -> definitely the same file/folder. Different
|
||||||
|
// fsKey -> either a coincidence (two servers both have /root/x.conf) or a
|
||||||
|
// hidden alias of one machine; only a probe can tell (SFTP exposes no
|
||||||
|
// device/inode identity — the technique cp/rsync use locally).
|
||||||
|
const definitelySame =
|
||||||
|
sameFs || (await this.probeSameFilesystem(src, dst, dst.dirname(dstPath)));
|
||||||
|
if (!definitelySame) return;
|
||||||
|
|
||||||
|
if (samePath) {
|
||||||
|
throw new Error(
|
||||||
|
isDir ? "Cannot copy a folder onto itself" : "Source and destination are the same file",
|
||||||
|
);
|
||||||
}
|
}
|
||||||
if (dstPath.startsWith(snap.srcPath.endsWith("/") ? snap.srcPath : `${snap.srcPath}/`)) {
|
|
||||||
throw new Error("Cannot copy a folder into itself");
|
throw new Error("Cannot copy a folder into itself");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Deterministic same-filesystem test across two connections: write a token
|
||||||
|
* file via dst, look for it via src. Visible + matching content = one
|
||||||
|
* machine behind two addresses. Fails open (returns false) when the probe
|
||||||
|
* itself cannot run (e.g. destination not writable) — the copy then
|
||||||
|
* proceeds and permissions produce their own error.
|
||||||
|
*/
|
||||||
|
private async probeSameFilesystem(
|
||||||
|
src: FileSystemProvider,
|
||||||
|
dst: FileSystemProvider,
|
||||||
|
dir: string,
|
||||||
|
): Promise<boolean> {
|
||||||
|
const token = crypto.randomUUID();
|
||||||
|
const probePath = dst.join(dir, `.bunscp-probe-${token.slice(0, 8)}`);
|
||||||
|
try {
|
||||||
|
await new Promise<void>((resolve, reject) => {
|
||||||
|
const stream = dst.createWriteStream(probePath);
|
||||||
|
stream.on("error", reject);
|
||||||
|
stream.end(token, () => resolve());
|
||||||
|
});
|
||||||
|
try {
|
||||||
|
const chunks: Buffer[] = [];
|
||||||
|
for await (const chunk of src.createReadStream(src.join(dir, `.bunscp-probe-${token.slice(0, 8)}`))) {
|
||||||
|
chunks.push(typeof chunk === "string" ? Buffer.from(chunk) : chunk);
|
||||||
|
}
|
||||||
|
const seen = Buffer.concat(chunks).toString("utf8") === token;
|
||||||
|
if (seen) console.log(`[engine] probe: ${src.fsKey} and ${dst.fsKey} are the same machine`);
|
||||||
|
return seen;
|
||||||
|
} catch {
|
||||||
|
return false; // src cannot see the token -> different filesystems
|
||||||
|
}
|
||||||
|
} catch (error) {
|
||||||
|
console.log(`[engine] same-fs probe skipped: ${error instanceof Error ? error.message : error}`);
|
||||||
|
return false;
|
||||||
|
} finally {
|
||||||
|
await dst.deleteFile(probePath).catch(() => {});
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
private finish(job: Job, state: TransferState): void {
|
private finish(job: Job, state: TransferState): void {
|
||||||
|
|||||||
+1
-1
@@ -22,7 +22,7 @@ export type AppRPC = {
|
|||||||
// sessions
|
// sessions
|
||||||
connect: {
|
connect: {
|
||||||
params: { config: ConnectionConfig };
|
params: { config: ConnectionConfig };
|
||||||
response: Result<{ sessionId: string; homeDir: string }>;
|
response: Result<{ sessionId: string; homeDir: string; fsKey: string }>;
|
||||||
};
|
};
|
||||||
disconnect: { params: { sessionId: string }; response: Result };
|
disconnect: { params: { sessionId: string }; response: Result };
|
||||||
// filesystem (uniform across "local" and sftp sessions)
|
// filesystem (uniform across "local" and sftp sessions)
|
||||||
|
|||||||
@@ -13,6 +13,8 @@ interface PanelDeps {
|
|||||||
|
|
||||||
export class PanelState {
|
export class PanelState {
|
||||||
readonly sessionId = signal('local');
|
readonly sessionId = signal('local');
|
||||||
|
/** Filesystem identity ("local" or resolvedIp:port) — self-copy guards use this. */
|
||||||
|
readonly fsKey = signal('local');
|
||||||
readonly title = signal('Local');
|
readonly title = signal('Local');
|
||||||
readonly connecting = signal(false);
|
readonly connecting = signal(false);
|
||||||
readonly cwd = signal('');
|
readonly cwd = signal('');
|
||||||
@@ -339,6 +341,7 @@ export class PanelsService {
|
|||||||
void this.rpc.request.disconnect({ sessionId: previous });
|
void this.rpc.request.disconnect({ sessionId: previous });
|
||||||
}
|
}
|
||||||
panel.sessionId.set(res.sessionId);
|
panel.sessionId.set(res.sessionId);
|
||||||
|
panel.fsKey.set(res.fsKey);
|
||||||
panel.title.set(title);
|
panel.title.set(title);
|
||||||
await panel.load(config.initialPath ?? res.homeDir);
|
await panel.load(config.initialPath ?? res.homeDir);
|
||||||
return true;
|
return true;
|
||||||
@@ -357,6 +360,7 @@ export class PanelsService {
|
|||||||
|
|
||||||
private resetToLocal(panel: PanelState): void {
|
private resetToLocal(panel: PanelState): void {
|
||||||
panel.sessionId.set('local');
|
panel.sessionId.set('local');
|
||||||
|
panel.fsKey.set('local');
|
||||||
panel.title.set('Local');
|
panel.title.set('Local');
|
||||||
void panel.load(null);
|
void panel.load(null);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -56,12 +56,13 @@ export class TransfersService {
|
|||||||
this.toast.info('Nothing to copy (symlinks are skipped)');
|
this.toast.info('Nothing to copy (symlinks are skipped)');
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
if (src.sessionId() === dst.sessionId() && src.cwd() === dst.cwd()) {
|
const sameFs = src.fsKey() === dst.fsKey();
|
||||||
|
if (sameFs && src.cwd() === dst.cwd()) {
|
||||||
this.toast.info('Both panels show the same folder');
|
this.toast.info('Both panels show the same folder');
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
const finalTargets = await this.resolveCollisions(targets, dst.entries());
|
const finalTargets = await this.resolveCollisions(targets, dst.entries(), sameFs);
|
||||||
if (finalTargets === null) return; // user cancelled
|
if (finalTargets === null) return; // user cancelled
|
||||||
if (!finalTargets.length) {
|
if (!finalTargets.length) {
|
||||||
this.toast.info('Nothing to copy — all conflicting items were skipped');
|
this.toast.info('Nothing to copy — all conflicting items were skipped');
|
||||||
@@ -84,6 +85,7 @@ export class TransfersService {
|
|||||||
private async resolveCollisions(
|
private async resolveCollisions(
|
||||||
targets: FileEntry[],
|
targets: FileEntry[],
|
||||||
dstEntries: FileEntry[],
|
dstEntries: FileEntry[],
|
||||||
|
sameFs: boolean,
|
||||||
): Promise<FileEntry[] | null> {
|
): Promise<FileEntry[] | null> {
|
||||||
const dstByName = new Map(dstEntries.map((e) => [e.name, e]));
|
const dstByName = new Map(dstEntries.map((e) => [e.name, e]));
|
||||||
const collisions = targets.filter((t) => {
|
const collisions = targets.filter((t) => {
|
||||||
@@ -101,6 +103,11 @@ export class TransfersService {
|
|||||||
finalTargets.push(target);
|
finalTargets.push(target);
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
if (sameFs && dstEntry.path === target.path) {
|
||||||
|
// replacing a file with ITSELF would only destroy it — never offer that
|
||||||
|
this.toast.info(`Skipped "${target.name}" — it is the same file`);
|
||||||
|
continue;
|
||||||
|
}
|
||||||
if ((dstEntry.type === 'dir') !== (target.type === 'dir')) {
|
if ((dstEntry.type === 'dir') !== (target.type === 'dir')) {
|
||||||
this.toast.info(`Skipped "${target.name}" — a different kind of item is in the way`);
|
this.toast.info(`Skipped "${target.name}" — a different kind of item is in the way`);
|
||||||
continue;
|
continue;
|
||||||
|
|||||||
Reference in New Issue
Block a user