From ebfa5e541c873dcbdaff3a82bbdc86c1997036b1 Mon Sep 17 00:00:00 2001 From: Matt Strapp Date: Tue, 15 Feb 2022 15:38:38 -0600 Subject: Fix bug involving missing a return --- package.json | 1 - src/routes/api.ts | 162 +++++++++++++++++++++++++++--------------------------- yarn.lock | 5 -- 3 files changed, 81 insertions(+), 87 deletions(-) diff --git a/package.json b/package.json index 9739358..57b1c43 100644 --- a/package.json +++ b/package.json @@ -7,7 +7,6 @@ "express-fileupload": "^1.3.1", "express-rate-limit": "^6.2.1", "helmet": "^5.0.2", - "loglevel": "^1.8.0", "shell-quote": "^1.7.3" }, "devDependencies": { diff --git a/src/routes/api.ts b/src/routes/api.ts index c3518d6..77b3c79 100644 --- a/src/routes/api.ts +++ b/src/routes/api.ts @@ -17,12 +17,12 @@ const csrf = csurf({ cookie: true }); // For file uploads api.use(fileUpload({ - preserveExtension: true, // Preserve file extension on upload - safeFileNames: true, // Only allow alphanumeric characters in file names - limits: { fileSize: 1 * 1024 * 1024 }, // Limit file size to 1MB - useTempFiles: true, // Store files in temp instead of memory - tempFileDir: '/tmp/', // Store files in /tmp/ - debug: false, // Log debug information + preserveExtension: true, // Preserve file extension on upload + safeFileNames: true, // Only allow alphanumeric characters in file names + limits: { fileSize: 1 * 1024 * 1024 }, // Limit file size to 1MB + useTempFiles: true, // Store files in temp instead of memory + tempFileDir: '/tmp/', // Store files in /tmp/ + debug: false, // Log debug information })); @@ -46,33 +46,33 @@ api.use(fileUpload({ 500 for any other errors */ api.route('/upload') - .post(csrf, (req: Request, res: Response) => { - try { - // Check if anything was actually uploaded - if (!req.files || Object.keys(req.files).length === 0) - return res.status(400).json({ error: 'No file uploaded.' }); - - const file: UploadedFile = req.files.file as UploadedFile; // Kludge to prevent a compiler error, only one file gets uploaded so this should be fine - - // Check if the file is too large (see fileUpload.limits for the limit) - if (file.truncated) - return res.status(413).json({ error: 'File uploaded was too large.' }); - - // Check if the file is a python file - if (file.mimetype !== 'text/x-python') - return res.status(415).json({ error: 'File uploaded was not a Python file.' }); - - res.status(201).json({ file: file.name, path: file.tempFilePath, msg: 'File uploaded successfully.', csrf: req.csrfToken() }); - } catch (err) { - // Generic error handler - res.status(500).json({ error: 'An unknown error occurred while uploading the file.', error_msg: err }); - } - }) - // Fallback - .all(csrf, (req: Request, res: Response) => { - res.set('Allow', 'POST'); - res.status(405).json({ error: 'Method not allowed.' }); - }); + .post(csrf, (req: Request, res: Response) => { + try { + // Check if anything was actually uploaded + if (!req.files || Object.keys(req.files).length === 0) + return res.status(400).json({ error: 'No file uploaded.' }); + + const file: UploadedFile = req.files.file as UploadedFile; // Kludge to prevent a compiler error, only one file gets uploaded so this should be fine + + // Check if the file is too large (see fileUpload.limits for the limit) + if (file.truncated) + return res.status(413).json({ error: 'File uploaded was too large.' }); + + // Check if the file is a python file + if (file.mimetype !== 'text/x-python') + return res.status(415).json({ error: 'File uploaded was not a Python file.' }); + + res.status(201).json({ file: file.name, path: file.tempFilePath, msg: 'File uploaded successfully.', csrf: req.csrfToken() }); + } catch (err) { + // Generic error handler + res.status(500).json({ error: 'An unknown error occurred while uploading the file.', error_msg: err }); + } + }) + // Fallback + .all(csrf, (req: Request, res: Response) => { + res.set('Allow', 'POST'); + res.status(405).json({ error: 'Method not allowed.' }); + }); /* Actuate the pendulum @@ -99,54 +99,54 @@ api.route('/upload') */ api.route('/actuate') - // Snyk error mitigation, should be fine since the rate limiting is already in place - // file deepcode ignore NoRateLimitingForExpensiveWebOperation: This is already rate limited by the website, so we don't need to do it again - .post(csrf, async (req: Request, res: Response) => { - // Make sure the file being requested to run exists - try { - await access(req.body.path); - } catch (err) { - return res.status(403).json({ error: 'File is not accessible or does not exist.' }); - } - - - const stats: Stats = await stat(req.body.path); - // Make sure the file being requested to run is a regular file - if (!stats.isFile()) - return res.status(400).json({ error: 'File is not a regular file.' }); - // Make sure the file being requested to run is not a directory - if (stats.isDirectory()) - return res.status(400).json({ error: 'File is a directory.' }); - - const escaped = quote( [ req.body.path ] ); - // Run the code - /* - TODO: - - Potentially add the limiter to one-per-person here - - Add a timeout - - Communicate to the machine to give up (the user as well), maybe just kill the process? - - Make this more secure - - HOW? - */ - let output = ''; - const actuation = spawn('python', escaped.split(' ')); - actuation.stdout.on('data', (data: Buffer) => { - output += data.toString(); - }); - actuation.stderr.on('data', (data: Buffer) => { - output += `STDERR: ${data.toString()}`; - }); - actuation.on('close', (code: number) => { - if (code !== 0) - res.status(500).json({ error: `Program exited with exit code ${code}`, error_msg: output }); - res.status(200).json({ stdout: output }); - }); - }) - // Fallback - .all(csrf, (req: Request, res: Response) => { - res.set('Allow', 'POST'); - res.status(405).json({ error: 'Method not allowed.' }); + // Snyk error mitigation, should be fine since the rate limiting is already in place + // file deepcode ignore NoRateLimitingForExpensiveWebOperation: This is already rate limited by the website, so we don't need to do it again + .post(csrf, async (req: Request, res: Response) => { + // Make sure the file being requested to run exists + try { + await access(req.body.path); + } catch (err) { + return res.status(403).json({ error: 'File is not accessible or does not exist.' }); + } + + + const stats: Stats = await stat(req.body.path); + // Make sure the file being requested to run is a regular file + if (!stats.isFile()) + return res.status(400).json({ error: 'File is not a regular file.' }); + // Make sure the file being requested to run is not a directory + if (stats.isDirectory()) + return res.status(400).json({ error: 'File is a directory.' }); + + const escaped = quote([req.body.path]); + // Run the code + /* + TODO: + - Potentially add the limiter to one-per-person here + - Add a timeout + - Communicate to the machine to give up (the user as well), maybe just kill the process? + - Make this more secure + - HOW? + */ + let output = ''; + const actuation = spawn('python', escaped.split(' ')); + actuation.stdout.on('data', (data: Buffer) => { + output += data.toString(); + }); + actuation.stderr.on('data', (data: Buffer) => { + output += `STDERR: ${data.toString()}`; + }); + actuation.on('close', (code: number) => { + if (code !== 0) + return res.status(500).json({ error: `Program exited with exit code ${code}`, error_msg: output }); + return res.status(200).json({ stdout: output }); }); + }) + // Fallback + .all(csrf, (req: Request, res: Response) => { + res.set('Allow', 'POST'); + res.status(405).json({ error: 'Method not allowed.' }); + }); -export default api; \ No newline at end of file +export default api; diff --git a/yarn.lock b/yarn.lock index f3742cc..f784039 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1663,11 +1663,6 @@ lodash.merge@^4.6.2: resolved "https://registry.yarnpkg.com/lodash.merge/-/lodash.merge-4.6.2.tgz#558aa53b43b661e1925a0afdfa36a9a1085fe57a" integrity sha512-0KpjqXRVvrYyCsX1swR/XTK0va6VQkQM6MNo7PqW77ByjAhoARA8EfrP1N4+KlKj8YS0ZUCtRT/YUuhyYDujIQ== -loglevel@^1.8.0: - version "1.8.0" - resolved "https://registry.yarnpkg.com/loglevel/-/loglevel-1.8.0.tgz#e7ec73a57e1e7b419cb6c6ac06bf050b67356114" - integrity sha512-G6A/nJLRgWOuuwdNuA6koovfEV1YpqqAG4pRUlFaz3jj2QNZ8M4vBqnVA+HBTmU/AMNUtlOsMmSpF6NyOjztbA== - lowercase-keys@^1.0.0, lowercase-keys@^1.0.1: version "1.0.1" resolved "https://registry.yarnpkg.com/lowercase-keys/-/lowercase-keys-1.0.1.tgz#6f9e30b47084d971a7c820ff15a6c5167b74c26f" -- cgit v1.2.3