From f5f8952df9df52ee9a4d6c2a559380e75c9039f3 Mon Sep 17 00:00:00 2001 From: Jesse Chan Date: Tue, 29 Sep 2020 17:32:11 +0800 Subject: [PATCH] moveTorrents: get sourceBasePath and baseFileName in server With these two properties, it is hard for third party to utilize API to move torrents without retrieving torrent details first. Plus, client-supplied paths and filenames can lead to arbitrary file system access which is a security issue. In conclusion, it doesn't make sense to let client to provide these two properties. --- .../src/javascript/actions/TorrentActions.ts | 5 +++- .../move-torrents-modal/MoveTorrentsModal.tsx | 10 +++----- server/routes/client.ts | 4 +-- server/services/clientGatewayService.ts | 25 ++++++++----------- shared/types/Action.ts | 5 ---- 5 files changed, 19 insertions(+), 30 deletions(-) diff --git a/client/src/javascript/actions/TorrentActions.ts b/client/src/javascript/actions/TorrentActions.ts index 749ec508..00653830 100644 --- a/client/src/javascript/actions/TorrentActions.ts +++ b/client/src/javascript/actions/TorrentActions.ts @@ -160,7 +160,10 @@ const TorrentActions = { (error) => { AppDispatcher.dispatchServerAction({ type: 'CLIENT_MOVE_TORRENTS_ERROR', - error, + error: { + error, + count: options.hashes.length, + }, }); }, ); diff --git a/client/src/javascript/components/modals/move-torrents-modal/MoveTorrentsModal.tsx b/client/src/javascript/components/modals/move-torrents-modal/MoveTorrentsModal.tsx index c6477ad8..cd448fbb 100644 --- a/client/src/javascript/components/modals/move-torrents-modal/MoveTorrentsModal.tsx +++ b/client/src/javascript/components/modals/move-torrents-modal/MoveTorrentsModal.tsx @@ -92,18 +92,14 @@ class MoveTorrents extends React.Component { - const filenames = TorrentStore.getSelectedTorrentsFilename(); - const sourcePaths = TorrentStore.getSelectedTorrentsDownloadLocations(); - - if (sourcePaths.length) { + const hashes = TorrentStore.getSelectedTorrents(); + if (hashes.length > 0) { this.setState({isSettingDownloadPath: true}); TorrentActions.moveTorrents({ - hashes: TorrentStore.getSelectedTorrents(), + hashes, destination: formData.destination, isBasePath: formData.isBasePath, - filenames, moveFiles: formData.moveFiles, - sourcePaths, isCheckHash: formData.isCheckHash, }).then(() => { this.setState({isSettingDownloadPath: false}); diff --git a/server/routes/client.ts b/server/routes/client.ts index fefccc6c..6619c393 100644 --- a/server/routes/client.ts +++ b/server/routes/client.ts @@ -160,11 +160,11 @@ router.post('/torrents/check-hash', (req * @return {Error} 500 - failure response - application/json */ router.post('/torrents/move', (req, res) => { - const {hashes, filenames, sourcePaths, destination, moveFiles, isBasePath, isCheckHash} = req.body; + const {hashes, destination, moveFiles, isBasePath, isCheckHash} = req.body; const callback = ajaxUtil.getResponseFn(res); req.services?.clientGatewayService - .moveTorrents({hashes, filenames, sourcePaths, destination, moveFiles, isBasePath, isCheckHash}) + .moveTorrents({hashes, destination, moveFiles, isBasePath, isCheckHash}) .then((response) => { req.services?.torrentService.fetchTorrentList(); return response; diff --git a/server/services/clientGatewayService.ts b/server/services/clientGatewayService.ts index 25d47d81..16bfc6e5 100644 --- a/server/services/clientGatewayService.ts +++ b/server/services/clientGatewayService.ts @@ -107,15 +107,7 @@ class ClientGatewayService extends BaseService { * @param {MoveTorrentsOptions} options - An object of options... * @return {Promise} - Resolves with the processed client response or rejects with the processed client error. */ - async moveTorrents({ - hashes, - filenames, - sourcePaths, - destination, - moveFiles, - isBasePath, - isCheckHash, - }: MoveTorrentsOptions) { + async moveTorrents({hashes, destination, moveFiles, isBasePath, isCheckHash}: MoveTorrentsOptions) { if (this.services == null || this.services.clientRequestManager == null || this.services.torrentService == null) { return Promise.reject(); } @@ -152,15 +144,18 @@ class ClientGatewayService extends BaseService { }); if (moveFiles) { - sourcePaths.forEach((source, index) => { - const destinationFilePath = fileUtil.sanitizePath(path.join(resolvedPath, filenames[index])); - if (!fileUtil.isAllowedPath(destinationFilePath)) { - throw fileUtil.accessDeniedError(); + hashes.forEach((hash) => { + const sourceBasePath = this.services?.torrentService.getTorrent(hash).basePath; + const baseFileName = this.services?.torrentService.getTorrent(hash).baseFilename; + + if (sourceBasePath == null || baseFileName == null) { + return; } - if (source !== destinationFilePath) { + const destinationFilePath = fileUtil.sanitizePath(path.join(resolvedPath, baseFileName)); + if (sourceBasePath !== destinationFilePath) { try { - moveSync(source, destinationFilePath, {overwrite: true}); + moveSync(sourceBasePath, destinationFilePath, {overwrite: true}); } catch (err) { console.error(`Failed to move files to ${resolvedPath}.`); console.error(err); diff --git a/shared/types/Action.ts b/shared/types/Action.ts index 654196d7..cd4359fe 100644 --- a/shared/types/Action.ts +++ b/shared/types/Action.ts @@ -23,17 +23,12 @@ export interface DeleteTorrentsOptions { deleteData?: boolean; } -// TODO: filenames and sourcePaths should not be supplied by the client. // POST /api/client/torrents/move export interface MoveTorrentsOptions { // Hashes of torrents to be moved hashes: Array; // Path of destination destination: string; - // Filenames of data of torrents - filenames: Array; - // Source paths of data of torrents - sourcePaths: Array; // Whether to move data of torrents moveFiles: boolean; // Whether destination is the base path