fix: improve FTPClient #304
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ | |
|
|
||
| /** A FTP Client that supports FTP & FTPS (FTP over TLS) connections. */ | ||
| export class FTPClient extends FTPClientBase { | ||
| private client: SafeFTPClient | ||
|
|
||
| private initializing: Promise<void> | null = null | ||
| public destroyed = false | ||
|
|
@@ -23,49 +23,8 @@ | |
|
|
||
| this.client = this.createFTPClient() | ||
| } | ||
| private createFTPClient(): SafeFTPClient { | ||
| return new SafeFTPClient() | ||
| } | ||
|
|
||
| async init(): Promise<void> { | ||
|
|
@@ -92,8 +51,10 @@ | |
| rejectUnauthorized: !this.options.allowAnyCertificate, // Allow self-signed certificates | ||
| }, | ||
| } | ||
| await this.client.batch(async (ftpClient) => { | ||
| await ftpClient.access(options) | ||
| await ftpClient.cd('/') | ||
| }) | ||
|
|
||
| this.initializing = null // Reset initializing promise after successful initialization | ||
| }) | ||
|
|
@@ -173,61 +134,68 @@ | |
| packageExists: false, | ||
| } | ||
|
|
||
| return this.client.batch(async (ftpClient) => { | ||
| const size = await ftpClient.size(fullPath) | ||
|
|
||
| let modDate: Date | undefined = undefined | ||
| try { | ||
| modDate = await ftpClient.lastMod(fullPath) | ||
| } catch { | ||
| // This is not supported by every FTP server, ignore any error | ||
| } | ||
| return { | ||
| success: true, | ||
| fileInfo: { | ||
| size, | ||
| modified: modDate ? modDate.getTime() : 0, | ||
| }, | ||
| } | ||
| }) | ||
| } | ||
| async upload(sourceStream: NodeJS.ReadableStream, fullPath: string): Promise<void> { | ||
| await this.init() // Ensure the client is connected | ||
|
|
||
| this.logger.silly(`Uploading file to: ${fullPath}`) | ||
|
|
||
| await this.client.batch(async (ftpClient) => { | ||
| // Ensure the directory exists: | ||
| await ftpClient.ensureDir(path.dirname(fullPath)) | ||
| await ftpClient.cd('/') // Revert to root after ensureDir | ||
|
|
||
| // Remove the file if it already exists: | ||
| await ftpClient.remove(fullPath, true) | ||
|
Comment on lines
+165
to
+166
There was a problem hiding this comment. 🧩 Analysis chain🌐 Web query:
💡 Result: The basic-ftp Client's uploadFrom method overwrites existing files on the remote FTP server by default. If a file with the specified remotePath already exists, it will be overwritten with the data from the provided readable stream or local file.[1][2][3] Citations:
🏁 Script executed: cat -n shared/packages/worker/src/worker/accessorHandlers/lib/FTPClient/FTPClient.ts | sed -n '155,200p'Repository: Sofie-Automation/sofie-package-manager Length of output: 1998 Inconsistency: The 🤖 Prompt for AI Agents |
||
|
|
||
| // Upload the file: | ||
| const response = await ftpClient.uploadFrom(Readable.from(sourceStream), fullPath) | ||
|
|
||
| if (response.code !== 226) { | ||
| // 226 means "Transfer complete" | ||
| throw new Error(`Upload failed: [${response.code}]: ${response.message}`) | ||
| } | ||
| }) | ||
| } | ||
| async uploadContent(fullPath: string, content: Buffer | string): Promise<void> { | ||
| await this.init() // Ensure the client is connected | ||
|
|
||
| this.logger.silly(`Uploading content to: ${fullPath}`) | ||
|
|
||
| await this.client.batch(async (ftpClient) => { | ||
| // Ensure the directory exists: | ||
| await ftpClient.ensureDir(path.dirname(fullPath)) | ||
| await ftpClient.cd('/') // Revert to root after ensureDir | ||
|
|
||
| const buffer = Buffer.isBuffer(content) ? content : Buffer.from(content, 'utf-8') | ||
|
|
||
| // Feed the buffer into a readable stream: | ||
| const readableStream = Readable.from(buffer) | ||
| // Upload the readable stream: | ||
| const response = await ftpClient.uploadFrom(readableStream, fullPath) | ||
|
|
||
| if (response.code !== 226) { | ||
| // 226 means "Transfer complete" | ||
| throw new Error(`Upload failed [${response.code}]: ${response.message}`) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| async download(fullPath: string): Promise<FileDownloadReturnType> { | ||
|
|
@@ -318,12 +286,94 @@ | |
|
|
||
| const response = await this._fileExists(fullPath) | ||
| if (response.exists) { | ||
| await this.client.batch(async (ftpClient) => { | ||
| await ftpClient.removeDir(fullPath) | ||
| await ftpClient.cd('/') // Revert to root after removeDir | ||
| }) | ||
|
|
||
| return true | ||
| } else { | ||
| return false | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Wraps FTP.Client. Ensures that only one method of FTP.Client is called at a time, by putting all calls in a queue. | ||
| * This is necessary because FTP.Client does not support concurrent calls, and will throw an error if multiple methods are called at the same time. | ||
| */ | ||
| class SafeFTPClient { | ||
| private ftpClient: FTP.Client | ||
|
Check warning on line 306 in shared/packages/worker/src/worker/accessorHandlers/lib/FTPClient/FTPClient.ts
|
||
| private queue: PQueue | ||
|
Check warning on line 307 in shared/packages/worker/src/worker/accessorHandlers/lib/FTPClient/FTPClient.ts
|
||
|
|
||
| constructor() { | ||
| this.ftpClient = new FTP.Client() | ||
| this.queue = new PQueue({ concurrency: 1 }) | ||
| } | ||
|
|
||
| /** execute multiple ftp-commands in batch */ | ||
| async batch<T>(cb: (ftp: FTP.Client) => Promise<T>): Promise<T> { | ||
| return this.putInQueue('batch', [], async () => { | ||
| const result = await cb(this.ftpClient) | ||
| return result | ||
| }) | ||
| } | ||
|
|
||
| get closed(): boolean { | ||
| return this.ftpClient.closed | ||
| } | ||
| close(): void { | ||
| return this.ftpClient.close() | ||
| } | ||
|
|
||
| async access(...args: Parameters<FTPInstance['access']>): ReturnType<FTPInstance['access']> { | ||
| return this.putInQueue('access', args, async () => this.ftpClient.access(...args)) | ||
| } | ||
|
Comment on lines
+329
to
+331
There was a problem hiding this comment. Avoid serializing arbitrary args into error messages — risk of leaking credentials.
Consider either dropping args from the error string, redacting them, or only including the method name: 🛡️ Proposed hardening- const orgError = new Error(`Error executing ${methodName}: ${JSON.stringify(args)}`) // Used later
+ const orgError = new Error(`Error executing ${methodName}`) // Used later (args omitted to avoid leaking credentials/secrets)Also applies to: 360-377 🤖 Prompt for AI Agents |
||
| async downloadTo(...args: Parameters<FTPInstance['downloadTo']>): ReturnType<FTPInstance['downloadTo']> { | ||
| return this.putInQueue('downloadTo', args, async () => this.ftpClient.downloadTo(...args)) | ||
| } | ||
| async ensureDir(...args: Parameters<FTPInstance['ensureDir']>): ReturnType<FTPInstance['ensureDir']> { | ||
| return this.putInQueue('ensureDir', args, async () => this.ftpClient.ensureDir(...args)) | ||
| } | ||
| async lastMod(...args: Parameters<FTPInstance['lastMod']>): ReturnType<FTPInstance['lastMod']> { | ||
| return this.putInQueue('lastMod', args, async () => this.ftpClient.lastMod(...args)) | ||
| } | ||
| async list(...args: Parameters<FTPInstance['list']>): ReturnType<FTPInstance['list']> { | ||
| return this.putInQueue('list', args, async () => this.ftpClient.list(...args)) | ||
| } | ||
| async remove(...args: Parameters<FTPInstance['remove']>): ReturnType<FTPInstance['remove']> { | ||
| return this.putInQueue('remove', args, async () => this.ftpClient.remove(...args)) | ||
| } | ||
| async removeDir(...args: Parameters<FTPInstance['removeDir']>): ReturnType<FTPInstance['removeDir']> { | ||
| return this.putInQueue('removeDir', args, async () => this.ftpClient.removeDir(...args)) | ||
| } | ||
| async rename(...args: Parameters<FTPInstance['rename']>): ReturnType<FTPInstance['rename']> { | ||
| return this.putInQueue('rename', args, async () => this.ftpClient.rename(...args)) | ||
| } | ||
| async size(...args: Parameters<FTPInstance['size']>): ReturnType<FTPInstance['size']> { | ||
| return this.putInQueue('size', args, async () => this.ftpClient.size(...args)) | ||
| } | ||
| async uploadFrom(...args: Parameters<FTPInstance['uploadFrom']>): ReturnType<FTPInstance['uploadFrom']> { | ||
| return this.putInQueue('uploadFrom', args, async () => this.ftpClient.uploadFrom(...args)) | ||
| } | ||
|
|
||
| private async putInQueue<T, Args extends unknown[]>( | ||
| methodName: string, | ||
| args: Args, | ||
| cb: () => Promise<T> | ||
| ): Promise<T> { | ||
| return this.queue.add(async () => { | ||
| const orgError = new Error(`Error executing ${methodName}: ${JSON.stringify(args)}`) // Used later | ||
| try { | ||
| const result = await cb() | ||
| return result | ||
| } catch (e) { | ||
| if (typeof e === 'object' && e !== null && 'stack' in e) { | ||
| e.stack += `\nOriginal stack: ${orgError.stack}` | ||
| } | ||
| throw e | ||
| } | ||
| }) | ||
| } | ||
| } | ||
| type FTPInstance = InstanceType<typeof FTP.Client> // Short type for FTP.Client instance | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: Sofie-Automation/sofie-package-manager
Length of output: 16656
Remove the guard or apply consistently across all file handlers.
The
clearPackageRemoval()guard at lines 201-204 and 272-275 creates a behavioral inconsistency. All other handlers (s3, localFolder, httpProxy, fileShare) callclearPackageRemoval()unconditionally in equivalent methods. When FTP skips this due toremoveDelay < 0, any stale delayed-removal entry persists in thepackagesToRemovelist (see GenericFileOperations.ts:41-45) and can still execute later viaremoveDuePackages(). If a package is scheduled for removal, then re-written withremoveDelay = undefined, the stale removal entry will not be canceled. Either remove the guard to match other handlers or apply it to all handlers consistently.🤖 Prompt for AI Agents