From c00df4f45bf080dc55566b180b84ee6d34f28950 Mon Sep 17 00:00:00 2001
From: valadaptive <valadaptive@protonmail.com>
Date: Tue, 5 Dec 2023 08:10:31 -0500
Subject: [PATCH] Clean up file name sanitization

- Checking for null bytes is unnecessary because the check for illegal
characters directly below it will catch them.
- We can use the path.extname method to get the file extension more
cleanly. It returns the *last* extension (e.g.
path.extname('file.foo.js') === '.js'), so behavior is preserved.
- Normalizing the path is unnecessary. We don't allow any path
separators in the file name, so it does nothing.
- Stripping '..', path separators, and '$' is unnecessary because of the
earlier illegal character check.
---
 src/endpoints/assets.js | 21 ++++++++-------------
 src/endpoints/files.js  |  4 ++--
 2 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/src/endpoints/assets.js b/src/endpoints/assets.js
index 55a939816..442e59fe3 100644
--- a/src/endpoints/assets.js
+++ b/src/endpoints/assets.js
@@ -15,19 +15,14 @@ const VALID_CATEGORIES = ['bgm', 'ambient', 'blip', 'live2d'];
  * @param {string} inputFilename Input filename
  * @returns {string} Normalized or empty path if invalid
  */
-function checkAssetFileName(inputFilename) {
-    // Sanitize filename
-    if (inputFilename.indexOf('\0') !== -1) {
-        console.debug('Bad request: poisong null bytes in filename.');
-        return '';
-    }
-
+function sanitizeAssetFileName(inputFilename) {
     if (!/^[a-zA-Z0-9_\-.]+$/.test(inputFilename)) {
         console.debug('Bad request: illegal character in filename, only alphanumeric, \'_\', \'-\' are accepted.');
         return '';
     }
 
-    if (UNSAFE_EXTENSIONS.some(ext => inputFilename.toLowerCase().endsWith(ext))) {
+    const inputExtension = path.extname(inputFilename).toLowerCase();
+    if (UNSAFE_EXTENSIONS.some(ext => ext === inputExtension)) {
         console.debug('Bad request: forbidden file extension.');
         return '';
     }
@@ -37,7 +32,7 @@ function checkAssetFileName(inputFilename) {
         return '';
     }
 
-    return path.normalize(inputFilename).replace(/^(\.\.(\/|\\|$))+/, '');
+    return inputFilename;
 }
 
 // Recursive function to get files
@@ -144,7 +139,7 @@ router.post('/download', jsonParser, async (request, response) => {
     }
 
     // Sanitize filename
-    const safe_input = checkAssetFileName(inputFilename);
+    const safe_input = sanitizeAssetFileName(inputFilename);
     if (safe_input == '')
         return response.sendStatus(400);
 
@@ -203,7 +198,7 @@ router.post('/delete', jsonParser, async (request, response) => {
     }
 
     // Sanitize filename
-    const safe_input = checkAssetFileName(inputFilename);
+    const safe_input = sanitizeAssetFileName(inputFilename);
     if (safe_input == '')
         return response.sendStatus(400);
 
@@ -305,7 +300,7 @@ router.post('/upload', jsonParser, async (request, response) => {
             return response.status(400).send('No upload data specified');
         }
 
-        const safeInput = checkAssetFileName(request.body.name);
+        const safeInput = sanitizeAssetFileName(request.body.name);
 
         if (!safeInput) {
             return response.status(400).send('Invalid upload name');
@@ -321,4 +316,4 @@ router.post('/upload', jsonParser, async (request, response) => {
     }
 });
 
-module.exports = { router, checkAssetFileName };
+module.exports = { router, sanitizeAssetFileName };
diff --git a/src/endpoints/files.js b/src/endpoints/files.js
index 191b0f081..cbbe8d202 100644
--- a/src/endpoints/files.js
+++ b/src/endpoints/files.js
@@ -2,7 +2,7 @@ const path = require('path');
 const writeFileSyncAtomic = require('write-file-atomic').sync;
 const express = require('express');
 const router = express.Router();
-const { checkAssetFileName } = require('./assets');
+const { sanitizeAssetFileName } = require('./assets');
 const { jsonParser } = require('../express-common');
 const { DIRECTORIES } = require('../constants');
 
@@ -16,7 +16,7 @@ router.post('/upload', jsonParser, async (request, response) => {
             return response.status(400).send('No upload data specified');
         }
 
-        const safeInput = checkAssetFileName(request.body.name);
+        const safeInput = sanitizeAssetFileName(request.body.name);
 
         if (!safeInput) {
             return response.status(400).send('Invalid upload name');