fix: validate path traversal (#1463)

* fix: validate path traversal

* chore: add more test cases

* fix: directly copy path validator code over

* remove dependency

* simplify

Author: dcshzj

src/services/db/GitFileSystemService.ts

@@ -8,7 +8,6 @@ import {
   SimpleGit,
   DefaultLogFields,
   LogResult,
-  ListLogLine,
 } from "simple-git"
 
 import logger from "@logger/logger"
@@ -43,6 +42,7 @@ import type {
 import type { IsomerCommitMessage } from "@root/types/github"
 import { ALLOWED_FILE_EXTENSIONS } from "@root/utils/file-upload-utils"
 import { getPaginatedDirectoryContents } from "@root/utils/files"
+import { isSafePath } from "@root/validators/validators"
 
 // methods that do not need to be wrapped for instrumentation
 const METHOD_INSTRUMENTATION_BLACKLIST = [
@@ -366,22 +366,31 @@ export default class GitFileSystemService {
     const efsVolPath = isStaging
       ? EFS_VOL_PATH_STAGING
       : EFS_VOL_PATH_STAGING_LITE
-    return ResultAsync.fromPromise(
-      fs.promises.stat(`${efsVolPath}/${repoName}/${filePath}`),
-      (error) => {
-        if (error instanceof Error && error.message.includes("ENOENT")) {
-          return new NotFoundError("File/Directory does not exist")
-        }
 
-        logger.error(`Error when reading ${filePath}: ${error}`)
+    // Validate that the filePath is a valid relative path to prevent directory
+    // traversal attacks
+    const repoBaseDirectory = `${efsVolPath}/${repoName}`
+    const fullFilePath = path.join(repoBaseDirectory, filePath)
+    const isSafe = isSafePath(fullFilePath, repoBaseDirectory)
 
-        if (error instanceof Error) {
-          return new GitFileSystemError("Unable to read file/directory")
-        }
+    if (!isSafe) {
+      logger.error(`Invalid file path: ${filePath} for repo: ${repoName}`)
+      return errAsync(new BadRequestError("Invalid file path"))
+    }
 
-        return new GitFileSystemError("An unknown error occurred")
+    return ResultAsync.fromPromise(fs.promises.stat(fullFilePath), (error) => {
+      if (error instanceof Error && error.message.includes("ENOENT")) {
+        return new NotFoundError("File/Directory does not exist")
       }
-    )
+
+      logger.error(`Error when reading ${filePath}: ${error}`)
+
+      if (error instanceof Error) {
+        return new GitFileSystemError("Unable to read file/directory")
+      }
+
+      return new GitFileSystemError("An unknown error occurred")
+    })
   }
 
   /**

src/services/db/__tests__/GitFileSystemService.spec.ts

@@ -1,7 +1,7 @@
 import fs, { Stats } from "fs"
 
 import mockFs from "mock-fs"
-import { errAsync, okAsync } from "neverthrow"
+import { okAsync } from "neverthrow"
 import { GitError, SimpleGit } from "simple-git"
 
 import config from "@config/config"
@@ -720,6 +720,46 @@ describe("GitFileSystemService", () => {
       expect(result._unsafeUnwrap().isDirectory()).toBeTrue()
     })
 
+    it("should return a BadRequestError if the path goes above the repo root", async () => {
+      const result = await GitFileSystemService.getFilePathStats(
+        "fake-repo",
+        "../some-file",
+        DEFAULT_BRANCH === "staging"
+      )
+
+      expect(result._unsafeUnwrapErr()).toBeInstanceOf(BadRequestError)
+    })
+
+    it("should return a BadRequestError if the path contains glob expressions", async () => {
+      const result = await GitFileSystemService.getFilePathStats(
+        "fake-repo",
+        "{../,a}/some-file",
+        DEFAULT_BRANCH === "staging"
+      )
+
+      expect(result._unsafeUnwrapErr()).toBeInstanceOf(BadRequestError)
+    })
+
+    it('should return a BadRequestError if path contains "%2f"', async () => {
+      const result = await GitFileSystemService.getFilePathStats(
+        "fake-repo",
+        "..%2ffake-file",
+        DEFAULT_BRANCH === "staging"
+      )
+
+      expect(result._unsafeUnwrapErr()).toBeInstanceOf(BadRequestError)
+    })
+
+    it("should return a BadRequestError if path contains backslashes", async () => {
+      const result = await GitFileSystemService.getFilePathStats(
+        "fake-repo",
+        "..\\fake-file",
+        DEFAULT_BRANCH === "staging"
+      )
+
+      expect(result._unsafeUnwrapErr()).toBeInstanceOf(BadRequestError)
+    })
+
     it("should return a NotFoundError for a non-existent path", async () => {
       const result = await GitFileSystemService.getFilePathStats(
         "fake-repo",

src/validators/validators.js

@@ -1,3 +1,5 @@
+const path = require("path")
+
 const specialCharactersRegexTest = /[~%^*_+\-./\\`;~{}[\]"<>]/
 const jekyllFirstCharacterRegexTest = /^[._#~]/
 const dateRegexTest = /^[0-9]{4}-[0-9]{2}-[0-9]{2}$/
@@ -33,9 +35,35 @@ const isMediaPathValid = ({ path, isFile = false }) => {
 
 const isPasswordValid = (password) => passwordRegexTest.test(password)
 
+const isSafePath = (absPath, basePath) => {
+  // check for poison null bytes
+  if (absPath.indexOf("\0") !== -1) {
+    return false
+  }
+  // check for backslashes
+  if (absPath.indexOf("\\") !== -1) {
+    return false
+  }
+
+  // check for dot segments, even if they don't normalize to anything
+  if (absPath.includes("..")) {
+    return false
+  }
+
+  // check if the normalized path is within the provided 'safe' base path
+  if (path.resolve(basePath, path.relative(basePath, absPath)) !== absPath) {
+    return false
+  }
+  if (absPath.indexOf(basePath) !== 0) {
+    return false
+  }
+  return true
+}
+
 module.exports = {
   hasSpecialCharInTitle,
   isDateValid,
   isMediaPathValid,
   isPasswordValid,
+  isSafePath,
 }