Adds @webnet/ftp: FTP and implicit-FTPS client and server on the existing transport (RawDialer/RawListener) and VFS (AsyncVFS) abstractions, usable in both node and browsers (network via tsconnect in the browser; no node APIs in package source).
Server
FTPServer with an http-style listen(listener) accept loop; per-session state machine.
Serves any AsyncVFS; optional authenticate(user, pass) => Promise<AsyncVFS | null> for login validation and per-user roots.
Passive mode only: PASV + EPSV, data listeners come from a caller-injected dataListen(port) factory (passivePorts range or ephemeral); PORT/EPRT reply 502 (active mode is a follow-up, DataChannel is the seam).
Implicit FTPS: pass a TLS-terminating control listener (e.g. tsconnect listenTLS) and a TLS dataListen; PBSZ/PROT are handled; AUTH TLS replies 502 until the transport grows a mid-stream TLS upgrade primitive.
Command set aimed at FileZilla / KDE kio / classic ftp: USER/PASS, FEAT, OPTS UTF8, SYST, TYPE/MODE/STRU, PWD/CWD/CDUP, LIST/NLST (unix ls format), MLSD/MLST, STAT, SIZE/MDTM, REST/RETR (resume) /STOR, DELE/RMD/MKD, RNFR/RNTO, ABOR, QUIT. Transfers stream directly between data connections and VFS web streams; RETR opens the VFS resource before the 150 reply so missing files fail with a clean 550.
Client
FTPClient implements AsyncVFS (like DAVClient): stat, readdir, readFile(+Range via REST), writeFile, delete (recursive = client-side walk), mkdir, move (RNFR/RNTO); copy omitted (FTP has no server-side copy). rawCommand() escape hatch.
Prefers EPSV and MLSD/MLST, falls back to PASV and unix-ls LIST parsing (vsftpd has no MLSD); targets vsftpd/proftpd/own server.
security: "none" | "implicit" (string union so explicit AUTH TLS can be added non-breaking); implicit FTPS dials control and data connections with dialTls.
Operations serialize on one control connection; the mutex is held across each verb+data sequence and released when returned streams are consumed or cancelled.
Errors & tests
VFSError <-> reply-code translation tables in both directions (mirroring drive's status mapping).
125 tests: unit tests for codec/listing/addr/time/paths, loopback integration (AsyncVFS conformance, fallback paths via a disableFeatures server option, raw protocol-level assertions, data-timeout/ABOR/serialization edge cases).
Not yet verified against real FileZilla/kio/vsftpd installs; interop testing is a good follow-up before relying on it.
AI disclosure
Authored by Claude Code (Claude Fable 5; Claude Sonnet 5 subagents wrote the common protocol layer). See AI_CHANGES.md.
Adds `@webnet/ftp`: FTP and implicit-FTPS client and server on the existing transport (`RawDialer`/`RawListener`) and VFS (`AsyncVFS`) abstractions, usable in both node and browsers (network via tsconnect in the browser; no node APIs in package source).
## Server
- `FTPServer` with an http-style `listen(listener)` accept loop; per-session state machine.
- Serves any `AsyncVFS`; optional `authenticate(user, pass) => Promise<AsyncVFS | null>` for login validation and per-user roots.
- Passive mode only: PASV + EPSV, data listeners come from a caller-injected `dataListen(port)` factory (`passivePorts` range or ephemeral); PORT/EPRT reply 502 (active mode is a follow-up, `DataChannel` is the seam).
- Implicit FTPS: pass a TLS-terminating control listener (e.g. tsconnect `listenTLS`) and a TLS `dataListen`; PBSZ/PROT are handled; `AUTH TLS` replies 502 until the transport grows a mid-stream TLS upgrade primitive.
- Command set aimed at FileZilla / KDE kio / classic `ftp`: USER/PASS, FEAT, OPTS UTF8, SYST, TYPE/MODE/STRU, PWD/CWD/CDUP, LIST/NLST (unix `ls` format), MLSD/MLST, STAT, SIZE/MDTM, REST/RETR (resume) /STOR, DELE/RMD/MKD, RNFR/RNTO, ABOR, QUIT. Transfers stream directly between data connections and VFS web streams; RETR opens the VFS resource before the 150 reply so missing files fail with a clean 550.
## Client
- `FTPClient implements AsyncVFS` (like `DAVClient`): stat, readdir, readFile(+Range via REST), writeFile, delete (recursive = client-side walk), mkdir, move (RNFR/RNTO); `copy` omitted (FTP has no server-side copy). `rawCommand()` escape hatch.
- Prefers EPSV and MLSD/MLST, falls back to PASV and unix-`ls` LIST parsing (vsftpd has no MLSD); targets vsftpd/proftpd/own server.
- `security: "none" | "implicit"` (string union so explicit `AUTH TLS` can be added non-breaking); implicit FTPS dials control and data connections with `dialTls`.
- Operations serialize on one control connection; the mutex is held across each verb+data sequence and released when returned streams are consumed or cancelled.
## Errors & tests
- `VFSError <-> reply-code` translation tables in both directions (mirroring drive's status mapping).
- 125 tests: unit tests for codec/listing/addr/time/paths, loopback integration (AsyncVFS conformance, fallback paths via a `disableFeatures` server option, raw protocol-level assertions, data-timeout/ABOR/serialization edge cases).
Not yet verified against real FileZilla/kio/vsftpd installs; interop testing is a good follow-up before relying on it.
## AI disclosure
Authored by Claude Code (Claude Fable 5; Claude Sonnet 5 subagents wrote the common protocol layer). See `AI_CHANGES.md`.
Autonomous review of @webnet/ftp. Ran npm test (125/125 pass) and npm run typecheck (clean) before reporting. Findings below, ordered by severity; each verified by reading the code path (the injection one end-to-end with a scratch harness).
Major
1. CRLF command injection in the control codec (packages/ftp/src/common/codec.ts:55-58)
ControlWriter.command() writes ${verb} ${arg}\r\n without validating that arg (or verb) is free of CR/LF. Any client method that forwards a caller-supplied path as a command argument (MKD, DELE, CWD, STOR, RETR, RNFR/RNTO, SIZE, MDTM, MLST…) will smuggle additional commands if the path contains a newline. Verified end-to-end against the in-repo server:
client.mkdir("/injected\r\nMKD /pwned\r\nNOOP ")
// server dispatches BOTH commands -> creates /injected AND /pwned
The server side has the mirror gap: ControlWriter.reply() (same file, :60-70) emits filenames verbatim in LIST/MLSD/MLST output, so a VFS entry whose name contains CRLF would inject reply lines. Real filesystems can't produce such names, so the client argument path is the primary vector, but a defensive library should not allow argument smuggling at all. Suggest rejecting (or stripping) CR/LF in command()/reply() arguments. Practical precondition: the app must pass attacker-influenced path strings — still worth closing.
Minor
2. Error-mapping asymmetry: forbidden is not round-trippable (packages/ftp/src/common/replies.ts:22 vs :32-34)
vfsErrorToReply maps forbidden -> 550, but replyToVFSError maps 550/551 -> not-found (it only produces forbidden from 530/532, which the server emits solely for auth failures, never for a VFS forbidden). So a server-originated forbidden VFSError (e.g. VFS backends throw it for EACCES/EPERM and delete-root) surfaces on the client as not-found. Likewise precondition-failed (553) collapses to already-exists, and not-a-directory/is-a-directory/not-empty (550) collapse to not-found. Most of this is inherent to FTP's overloaded 550/553, but the forbidden -> 550 -> not-found case is a genuine information loss worth a comment or a tighter mapping if you care about it.
3. IAC-stripping comment is inaccurate (packages/ftp/src/common/codec.ts:37-40)
The comment claims telnet IAC bytes "decode as U+00Fx". They don't: invalid UTF-8 bytes (0xFF/0xFE/0xFB…) decode to U+FFFD (0xFFFD), confirmed via TextDecoder. The strip loop charCodeAt(start) >= 0xf0 happens to still catch U+FFFD (65533 >= 240), so it works by accident, but the rationale is wrong and the same guard would also strip a legitimately-decoded leading char in U+00F0–U+FFFF. Impact is negligible (verb position only), but the comment should be corrected or the guard made explicit (strip 0xFFFD).
Nit
4. Multiline-reply end detection can terminate early (packages/ftp/src/common/codec.ts:20-31)
readReply ends a multiline reply on the first continuation line matching ^<code> (.*)$. A server whose intermediate line legitimately begins with the same 3-digit code followed by a space would be truncated. This is a known FTP ambiguity (RFC 959 says such lines should be escaped) and the bundled server never produces it, so low risk — noting for completeness.
Everything else in the review scope checked out: the 150/226 data-connection sequencing (server replies 150 before accept, client dials only after seeing 1xx), the client mutex (every error/cancel path in #retrieve/writeFile/#transferText/#wrapDataStream releases via try/finally or explicit release-before-throw; the doc'd "hold across the returned stream" is intentional), the never-awaited dial promises (all handled with .then(t=>t.close()).catch() on the error branches), PassiveDataChannel.open() timeout (Promise.race, losing accept rejection is attached so not unhandled; listener closed in finally), bigint/number conversions, and UTF-8 line decoding (per-line stream decode, CR/LF never split a multibyte sequence). No functional bug found in those areas. Style matches packages/drive and packages/http conventions (#private fields, minimal comments).
Autonomous review of @webnet/ftp. Ran `npm test` (125/125 pass) and `npm run typecheck` (clean) before reporting. Findings below, ordered by severity; each verified by reading the code path (the injection one end-to-end with a scratch harness).
## Major
### 1. CRLF command injection in the control codec (`packages/ftp/src/common/codec.ts:55-58`)
`ControlWriter.command()` writes `${verb} ${arg}\r\n` without validating that `arg` (or `verb`) is free of CR/LF. Any client method that forwards a caller-supplied path as a command argument (MKD, DELE, CWD, STOR, RETR, RNFR/RNTO, SIZE, MDTM, MLST…) will smuggle additional commands if the path contains a newline. Verified end-to-end against the in-repo server:
```
client.mkdir("/injected\r\nMKD /pwned\r\nNOOP ")
// server dispatches BOTH commands -> creates /injected AND /pwned
```
The server side has the mirror gap: `ControlWriter.reply()` (same file, `:60-70`) emits filenames verbatim in LIST/MLSD/MLST output, so a VFS entry whose name contains CRLF would inject reply lines. Real filesystems can't produce such names, so the client argument path is the primary vector, but a defensive library should not allow argument smuggling at all. Suggest rejecting (or stripping) CR/LF in `command()`/`reply()` arguments. Practical precondition: the app must pass attacker-influenced path strings — still worth closing.
## Minor
### 2. Error-mapping asymmetry: `forbidden` is not round-trippable (`packages/ftp/src/common/replies.ts:22` vs `:32-34`)
`vfsErrorToReply` maps `forbidden` -> 550, but `replyToVFSError` maps 550/551 -> `not-found` (it only produces `forbidden` from 530/532, which the server emits solely for auth failures, never for a VFS `forbidden`). So a server-originated `forbidden` VFSError (e.g. VFS backends throw it for EACCES/EPERM and delete-root) surfaces on the client as `not-found`. Likewise `precondition-failed` (553) collapses to `already-exists`, and `not-a-directory`/`is-a-directory`/`not-empty` (550) collapse to `not-found`. Most of this is inherent to FTP's overloaded 550/553, but the `forbidden -> 550 -> not-found` case is a genuine information loss worth a comment or a tighter mapping if you care about it.
### 3. IAC-stripping comment is inaccurate (`packages/ftp/src/common/codec.ts:37-40`)
The comment claims telnet IAC bytes "decode as U+00Fx". They don't: invalid UTF-8 bytes (0xFF/0xFE/0xFB…) decode to U+FFFD (0xFFFD), confirmed via TextDecoder. The strip loop `charCodeAt(start) >= 0xf0` happens to still catch U+FFFD (65533 >= 240), so it works by accident, but the rationale is wrong and the same guard would also strip a legitimately-decoded leading char in U+00F0–U+FFFF. Impact is negligible (verb position only), but the comment should be corrected or the guard made explicit (strip 0xFFFD).
## Nit
### 4. Multiline-reply end detection can terminate early (`packages/ftp/src/common/codec.ts:20-31`)
`readReply` ends a multiline reply on the first continuation line matching `^<code> (.*)$`. A server whose intermediate line legitimately begins with the same 3-digit code followed by a space would be truncated. This is a known FTP ambiguity (RFC 959 says such lines should be escaped) and the bundled server never produces it, so low risk — noting for completeness.
---
Everything else in the review scope checked out: the 150/226 data-connection sequencing (server replies 150 before accept, client dials only after seeing 1xx), the client mutex (every error/cancel path in `#retrieve`/`writeFile`/`#transferText`/`#wrapDataStream` releases via try/finally or explicit release-before-throw; the doc'd "hold across the returned stream" is intentional), the never-awaited `dial` promises (all handled with `.then(t=>t.close()).catch()` on the error branches), `PassiveDataChannel.open()` timeout (Promise.race, losing accept rejection is attached so not unhandled; listener closed in finally), bigint/number conversions, and UTF-8 line decoding (per-line stream decode, CR/LF never split a multibyte sequence). No functional bug found in those areas. Style matches packages/drive and packages/http conventions (#private fields, minimal comments).
codinget
marked the pull request as ready for review 2026-07-06 09:09:57 +02:00
Autonomous review (Claude Fable 5). This is a well-put-together package: the control-channel mutex design (lock held across verb+data, released when the returned stream settles) is clean, CRLF injection is blocked in the codec, resolvePath prevents .. escapes on the server, and the 150-before-dial dance is correctly implemented on both sides. No blocking correctness bugs found; comments below are interop robustness and small hardening items.
One process note per prior feedback on wire-protocol packages: the test suite is loopback + unit only. Before finalizing, it would be worth one manual pass against a real peer — e.g. the client against vsftpd/proftpd, and the server against lftp/FileZilla — since that's where LIST-format and reply-code quirks show up.
Autonomous review (Claude Fable 5). This is a well-put-together package: the control-channel mutex design (lock held across verb+data, released when the returned stream settles) is clean, CRLF injection is blocked in the codec, `resolvePath` prevents `..` escapes on the server, and the 150-before-dial dance is correctly implemented on both sides. No blocking correctness bugs found; comments below are interop robustness and small hardening items.
One process note per prior feedback on wire-protocol packages: the test suite is loopback + unit only. Before finalizing, it would be worth one manual pass against a real peer — e.g. the client against vsftpd/proftpd, and the server against `lftp`/FileZilla — since that's where LIST-format and reply-code quirks show up.
Two small lifecycle nits: (1) if the connect promise rejected, await (await conn) makes close() reject too — worth a catch; (2) #control() only resets #conn on connect-time failure — if the established control connection later dies (server idle timeout is common in FTP), every subsequent call fails on the dead connection until the caller makes a new client. Resetting #conn when a command fails with a transport-level error would make long-lived clients self-heal.
Two small lifecycle nits: (1) if the connect promise rejected, `await (await conn)` makes `close()` reject too — worth a `catch`; (2) `#control()` only resets `#conn` on *connect-time* failure — if the established control connection later dies (server idle timeout is common in FTP), every subsequent call fails on the dead connection until the caller makes a new client. Resetting `#conn` when a command fails with a transport-level error would make long-lived clients self-heal.
REST is sent before EPSV/PASV. RFC 3659 §5.3 says REST must be the command immediately preceding the transfer command. vsftpd and proftpd happen to keep the restart offset across an intervening EPSV/PASV, so this works against the named targets (and your own server), but stricter servers are allowed to reset it. Moving the REST exchange to after openDataUnlocked() (right before RETR) is a one-line reorder that removes the interop risk.
**REST is sent before EPSV/PASV.** RFC 3659 §5.3 says REST must be the command immediately preceding the transfer command. vsftpd and proftpd happen to keep the restart offset across an intervening EPSV/PASV, so this works against the named targets (and your own server), but stricter servers are allowed to reset it. Moving the REST exchange to after `openDataUnlocked()` (right before RETR) is a one-line reorder that removes the interop risk.
550 is mapped unconditionally to not-found, but servers also use 550 for permission errors (vsftpd: "550 Permission denied." on DELE/STOR). A /(permission|denied|access)/i sniff before defaulting to not-found would keep forbidden from being misreported — same spirit as the existing RMD /empty/ special case.
550 is mapped unconditionally to `not-found`, but servers also use 550 for permission errors (vsftpd: "550 Permission denied." on DELE/STOR). A `/(permission|denied|access)/i` sniff before defaulting to `not-found` would keep `forbidden` from being misreported — same spirit as the existing RMD `/empty/` special case.
type (and prot) are tracked in session state but never read: TYPE A is accepted and then the transfer is sent as binary, and PROT C on an implicit-TLS server still serves TLS data connections. Both are fine behaviors in practice (modern clients use TYPE I / PROT P), but accepting-and-ignoring is the worst of both options for ASCII mode — either honor TYPE A (CRLF translation on RETR/STOR) or reply 504 so the client knows. If you keep them as accepted-but-ignored, dropping the dead state fields would at least make that explicit.
`type` (and `prot`) are tracked in session state but never read: TYPE A is accepted and then the transfer is sent as binary, and PROT C on an implicit-TLS server still serves TLS data connections. Both are fine behaviors in practice (modern clients use TYPE I / PROT P), but accepting-and-ignoring is the worst of both options for ASCII mode — either honor TYPE A (CRLF translation on RETR/STOR) or reply 504 so the client knows. If you keep them as accepted-but-ignored, dropping the dead state fields would at least make that explicit.
A non-VFSError thrown by a VFS implementation (or a handler bug) propagates out of run() and kills the whole control connection instead of producing a reply. Catching the generic case with a 451 Requested action aborted: local error keeps one bad operation from dropping the session; onError can still be notified.
A non-`VFSError` thrown by a VFS implementation (or a handler bug) propagates out of `run()` and kills the whole control connection instead of producing a reply. Catching the generic case with a `451 Requested action aborted: local error` keeps one bad operation from dropping the session; `onError` can still be notified.
Reviewed autonomously by GPT-5. I inspected FTP control serialization, passive data-channel lifecycle, transfer stream cancellation/cleanup, reply mapping, path handling, and server session state transitions. CI is passing, and I found no blocking correctness issues in this change.
Reviewed autonomously by GPT-5. I inspected FTP control serialization, passive data-channel lifecycle, transfer stream cancellation/cleanup, reply mapping, path handling, and server session state transitions. CI is passing, and I found no blocking correctness issues in this change.
Manual RW testing against classic ftp in anonymous mode worked fine (with some warnings from it, but these also were there with other servers), and RO testing with vsftpd also worked just fine. Didn't try kio, FileZilla or any other client or server manually.
Manual RW testing against classic `ftp` in anonymous mode worked fine (with some warnings from it, but these also were there with other servers), and RO testing with `vsftpd` also worked just fine. Didn't try kio, FileZilla or any other client or server manually.
Prevents command injection when an argument (e.g. a VFS path) contains
CRLF, which would otherwise be written verbatim onto the control channel
and let a caller smuggle additional FTP commands.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RMqjFbejK5h7MJamdfh3VY
- server: reply 451 (and notify onError) instead of dropping the control
connection when a VFS backend throws a non-VFSError
- server: drop dead TYPE/PROT session state (accept-and-ignore is
deliberate; binary transfers are correct for the compat targets)
- client: guard close() against a failed connect; reconnect transparently
when the cached control connection has dropped (FTP idle timeouts)
- client: send REST immediately before RETR, after PASV/EPSV (RFC 3659 5.3)
- replies: map 550 'permission denied' text to forbidden, not not-found
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RMqjFbejK5h7MJamdfh3VY
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
codinget
Adds
@webnet/ftp: FTP and implicit-FTPS client and server on the existing transport (RawDialer/RawListener) and VFS (AsyncVFS) abstractions, usable in both node and browsers (network via tsconnect in the browser; no node APIs in package source).Server
FTPServerwith an http-stylelisten(listener)accept loop; per-session state machine.AsyncVFS; optionalauthenticate(user, pass) => Promise<AsyncVFS | null>for login validation and per-user roots.dataListen(port)factory (passivePortsrange or ephemeral); PORT/EPRT reply 502 (active mode is a follow-up,DataChannelis the seam).listenTLS) and a TLSdataListen; PBSZ/PROT are handled;AUTH TLSreplies 502 until the transport grows a mid-stream TLS upgrade primitive.ftp: USER/PASS, FEAT, OPTS UTF8, SYST, TYPE/MODE/STRU, PWD/CWD/CDUP, LIST/NLST (unixlsformat), MLSD/MLST, STAT, SIZE/MDTM, REST/RETR (resume) /STOR, DELE/RMD/MKD, RNFR/RNTO, ABOR, QUIT. Transfers stream directly between data connections and VFS web streams; RETR opens the VFS resource before the 150 reply so missing files fail with a clean 550.Client
FTPClient implements AsyncVFS(likeDAVClient): stat, readdir, readFile(+Range via REST), writeFile, delete (recursive = client-side walk), mkdir, move (RNFR/RNTO);copyomitted (FTP has no server-side copy).rawCommand()escape hatch.lsLIST parsing (vsftpd has no MLSD); targets vsftpd/proftpd/own server.security: "none" | "implicit"(string union so explicitAUTH TLScan be added non-breaking); implicit FTPS dials control and data connections withdialTls.Errors & tests
VFSError <-> reply-codetranslation tables in both directions (mirroring drive's status mapping).disableFeaturesserver option, raw protocol-level assertions, data-timeout/ABOR/serialization edge cases).Not yet verified against real FileZilla/kio/vsftpd installs; interop testing is a good follow-up before relying on it.
AI disclosure
Authored by Claude Code (Claude Fable 5; Claude Sonnet 5 subagents wrote the common protocol layer). See
AI_CHANGES.md.Autonomous review of @webnet/ftp. Ran
npm test(125/125 pass) andnpm run typecheck(clean) before reporting. Findings below, ordered by severity; each verified by reading the code path (the injection one end-to-end with a scratch harness).Major
1. CRLF command injection in the control codec (
packages/ftp/src/common/codec.ts:55-58)ControlWriter.command()writes${verb} ${arg}\r\nwithout validating thatarg(orverb) is free of CR/LF. Any client method that forwards a caller-supplied path as a command argument (MKD, DELE, CWD, STOR, RETR, RNFR/RNTO, SIZE, MDTM, MLST…) will smuggle additional commands if the path contains a newline. Verified end-to-end against the in-repo server:The server side has the mirror gap:
ControlWriter.reply()(same file,:60-70) emits filenames verbatim in LIST/MLSD/MLST output, so a VFS entry whose name contains CRLF would inject reply lines. Real filesystems can't produce such names, so the client argument path is the primary vector, but a defensive library should not allow argument smuggling at all. Suggest rejecting (or stripping) CR/LF incommand()/reply()arguments. Practical precondition: the app must pass attacker-influenced path strings — still worth closing.Minor
2. Error-mapping asymmetry:
forbiddenis not round-trippable (packages/ftp/src/common/replies.ts:22vs:32-34)vfsErrorToReplymapsforbidden-> 550, butreplyToVFSErrormaps 550/551 ->not-found(it only producesforbiddenfrom 530/532, which the server emits solely for auth failures, never for a VFSforbidden). So a server-originatedforbiddenVFSError (e.g. VFS backends throw it for EACCES/EPERM and delete-root) surfaces on the client asnot-found. Likewiseprecondition-failed(553) collapses toalready-exists, andnot-a-directory/is-a-directory/not-empty(550) collapse tonot-found. Most of this is inherent to FTP's overloaded 550/553, but theforbidden -> 550 -> not-foundcase is a genuine information loss worth a comment or a tighter mapping if you care about it.3. IAC-stripping comment is inaccurate (
packages/ftp/src/common/codec.ts:37-40)The comment claims telnet IAC bytes "decode as U+00Fx". They don't: invalid UTF-8 bytes (0xFF/0xFE/0xFB…) decode to U+FFFD (0xFFFD), confirmed via TextDecoder. The strip loop
charCodeAt(start) >= 0xf0happens to still catch U+FFFD (65533 >= 240), so it works by accident, but the rationale is wrong and the same guard would also strip a legitimately-decoded leading char in U+00F0–U+FFFF. Impact is negligible (verb position only), but the comment should be corrected or the guard made explicit (strip 0xFFFD).Nit
4. Multiline-reply end detection can terminate early (
packages/ftp/src/common/codec.ts:20-31)readReplyends a multiline reply on the first continuation line matching^<code> (.*)$. A server whose intermediate line legitimately begins with the same 3-digit code followed by a space would be truncated. This is a known FTP ambiguity (RFC 959 says such lines should be escaped) and the bundled server never produces it, so low risk — noting for completeness.Everything else in the review scope checked out: the 150/226 data-connection sequencing (server replies 150 before accept, client dials only after seeing 1xx), the client mutex (every error/cancel path in
#retrieve/writeFile/#transferText/#wrapDataStreamreleases via try/finally or explicit release-before-throw; the doc'd "hold across the returned stream" is intentional), the never-awaiteddialpromises (all handled with.then(t=>t.close()).catch()on the error branches),PassiveDataChannel.open()timeout (Promise.race, losing accept rejection is attached so not unhandled; listener closed in finally), bigint/number conversions, and UTF-8 line decoding (per-line stream decode, CR/LF never split a multibyte sequence). No functional bug found in those areas. Style matches packages/drive and packages/http conventions (#private fields, minimal comments).Autonomous review (Claude Fable 5). This is a well-put-together package: the control-channel mutex design (lock held across verb+data, released when the returned stream settles) is clean, CRLF injection is blocked in the codec,
resolvePathprevents..escapes on the server, and the 150-before-dial dance is correctly implemented on both sides. No blocking correctness bugs found; comments below are interop robustness and small hardening items.One process note per prior feedback on wire-protocol packages: the test suite is loopback + unit only. Before finalizing, it would be worth one manual pass against a real peer — e.g. the client against vsftpd/proftpd, and the server against
lftp/FileZilla — since that's where LIST-format and reply-code quirks show up.@@ -0,0 +58,4 @@async close(): Promise<void> {const conn = this.#connthis.#conn = nullif (conn) await (await conn).close()Two small lifecycle nits: (1) if the connect promise rejected,
await (await conn)makesclose()reject too — worth acatch; (2)#control()only resets#connon connect-time failure — if the established control connection later dies (server idle timeout is common in FTP), every subsequent call fails on the dead connection until the caller makes a new client. Resetting#connwhen a command fails with a transport-level error would make long-lived clients self-heal.@@ -0,0 +201,4 @@let data: RawTransporttry {if (offset > 0n) {const rest = await conn.exchangeUnlocked("REST", String(offset))REST is sent before EPSV/PASV. RFC 3659 §5.3 says REST must be the command immediately preceding the transfer command. vsftpd and proftpd happen to keep the restart offset across an intervening EPSV/PASV, so this works against the named targets (and your own server), but stricter servers are allowed to reset it. Moving the REST exchange to after
openDataUnlocked()(right before RETR) is a one-line reorder that removes the interop risk.@@ -0,0 +29,4 @@export function replyToVFSError(reply: Reply, path: string, verb?: string): VFSError | FTPError {const { code, text } = replyif (code === 550 || code === 551) {550 is mapped unconditionally to
not-found, but servers also use 550 for permission errors (vsftpd: "550 Permission denied." on DELE/STOR). A/(permission|denied|access)/isniff before defaulting tonot-foundwould keepforbiddenfrom being misreported — same spirit as the existing RMD/empty/special case.@@ -0,0 +11,4 @@user: string | nullvfs: AsyncVFS | nullcwd: stringtype: "A" | "I"type(andprot) are tracked in session state but never read: TYPE A is accepted and then the transfer is sent as binary, and PROT C on an implicit-TLS server still serves TLS data connections. Both are fine behaviors in practice (modern clients use TYPE I / PROT P), but accepting-and-ignoring is the worst of both options for ASCII mode — either honor TYPE A (CRLF translation on RETR/STOR) or reply 504 so the client knows. If you keep them as accepted-but-ignored, dropping the dead state fields would at least make that explicit.@@ -0,0 +91,4 @@try {await handler(this, arg)} catch (e) {if (!(e instanceof VFSError)) throw eA non-
VFSErrorthrown by a VFS implementation (or a handler bug) propagates out ofrun()and kills the whole control connection instead of producing a reply. Catching the generic case with a451 Requested action aborted: local errorkeeps one bad operation from dropping the session;onErrorcan still be notified.Reviewed autonomously by GPT-5. I inspected FTP control serialization, passive data-channel lifecycle, transfer stream cancellation/cleanup, reply mapping, path handling, and server session state transitions. CI is passing, and I found no blocking correctness issues in this change.
codinget referenced this pull request2026-07-11 16:17:09 +02:00
Manual RW testing against classic
ftpin anonymous mode worked fine (with some warnings from it, but these also were there with other servers), and RO testing withvsftpdalso worked just fine. Didn't try kio, FileZilla or any other client or server manually.b108ec5e74tofee32aeb67