Merge pull request #10625 from growilabs/fix/codeql-errors

fix: CodeQL errors

apps/app/src/server/routes/apiv3/forgot-password.js

@@ -148,7 +148,7 @@ module.exports = (crowi) => {
       const appUrl = growiInfoService.getSiteUrl();
 
       try {
-        const user = await User.findOne({ email });
+        const user = await User.findOne({ email: { $eq: email } });
 
         // when the user is not found or active
         if (user == null || user.status !== 2) {

apps/app/src/server/routes/apiv3/g2g-transfer.ts

@@ -1,11 +1,11 @@
+import { createReadStream } from 'node:fs';
 import { SCOPE } from '@growi/core/dist/interfaces';
 import { ErrorV3 } from '@growi/core/dist/models';
 import type { NextFunction, Request, Router } from 'express';
 import express from 'express';
 import { body } from 'express-validator';
-import { createReadStream } from 'fs';
 import multer from 'multer';
-import path from 'path';
+import path from 'pathe';
 
 import type { GrowiArchiveImportOption } from '~/models/admin/growi-archive-import-option';
 import { accessTokenParser } from '~/server/middlewares/access-token-parser';
@@ -22,6 +22,7 @@ import { TransferKey } from '~/utils/vo/transfer-key';
 import type Crowi from '../../crowi';
 import { apiV3FormValidator } from '../../middlewares/apiv3-form-validator';
 import { Attachment } from '../../models/attachment';
+import { isPathWithinBase } from '../../util/safe-path-utils';
 import type { ApiV3Response } from './interfaces/apiv3-response';
 
 interface AuthorizedRequest extends Request {
@@ -511,6 +512,28 @@ module.exports = (crowi: Crowi): Router => {
         );
       }
 
+      // Validate file path to prevent path traversal attack
+      const importService = getImportService();
+      if (importService == null) {
+        return res.apiv3Err(
+          new ErrorV3(
+            'Import service is not available.',
+            'service_unavailable',
+          ),
+          500,
+        );
+      }
+      if (!isPathWithinBase(file.path, importService.baseDir)) {
+        logger.error('Path traversal attack detected', {
+          filePath: file.path,
+          baseDir: importService.baseDir,
+        });
+        return res.apiv3Err(
+          new ErrorV3('Invalid file path.', 'invalid_path'),
+          400,
+        );
+      }
+
       const fileStream = createReadStream(file.path, {
         flags: 'r',
         mode: 0o666,

apps/app/src/server/routes/attachment/api.js

@@ -318,7 +318,7 @@ export const routesFactory = (crowi) => {
   api.remove = async (req, res) => {
     const id = req.body.attachment_id;
 
-    const attachment = await Attachment.findById(id);
+    const attachment = await Attachment.findOne({ _id: { $eq: id } });
 
     if (attachment == null) {
       return res.json(ApiResponse.error('attachment not found'));

apps/app/src/server/routes/comment.js

@@ -279,7 +279,7 @@ module.exports = (crowi, app) => {
     }
     // update page
     const page = await Page.findOneAndUpdate(
-      { _id: pageId },
+      { _id: { $eq: pageId } },
       {
         lastUpdateUser: req.user,
         updatedAt: new Date(),
@@ -422,7 +422,7 @@ module.exports = (crowi, app) => {
 
     let updatedComment;
     try {
-      const comment = await Comment.findById(commentId).exec();
+      const comment = await Comment.findOne({ _id: { $eq: commentId } }).exec();
 
       if (comment == null) {
         throw new Error('This comment does not exist.');
@@ -442,7 +442,7 @@ module.exports = (crowi, app) => {
       }
 
       updatedComment = await Comment.findOneAndUpdate(
-        { _id: commentId },
+        { _id: { $eq: commentId } },
         { $set: { comment: commentStr, revision } },
       );
       commentEvent.emit(CommentEvent.UPDATE, updatedComment);
@@ -506,7 +506,7 @@ module.exports = (crowi, app) => {
 
     try {
       /** @type {import('mongoose').HydratedDocument<import('~/interfaces/comment').IComment>} */
-      const comment = await Comment.findById(commentId).exec();
+      const comment = await Comment.findOne({ _id: { $eq: commentId } }).exec();
 
       if (comment == null) {
         throw new Error('This comment does not exist.');

apps/app/src/server/routes/tag.js

@@ -128,7 +128,7 @@ module.exports = (crowi, app) => {
     const result = {};
     try {
       // TODO GC-1921 consider permission
-      const page = await Page.findById(pageId);
+      const page = await Page.findOne({ _id: { $eq: pageId } });
       const user = await User.findById(userId);
 
       if (!(await Page.isAccessiblePageByViewer(page._id, user))) {
@@ -137,7 +137,9 @@ module.exports = (crowi, app) => {
         );
       }
 
-      const previousRevision = await Revision.findById(revisionId);
+      const previousRevision = await Revision.findOne({
+        _id: { $eq: revisionId },
+      });
       result.savedPage = await crowi.pageService.updatePage(
         page,
         previousRevision.body,

apps/app/src/server/service/growi-bridge/index.spec.ts

@@ -0,0 +1,123 @@
+import fs from 'node:fs';
+import path from 'pathe';
+import { mock } from 'vitest-mock-extended';
+
+import type Crowi from '~/server/crowi';
+
+import { GrowiBridgeService } from './index';
+
+vi.mock('fs');
+
+describe('GrowiBridgeService', () => {
+  let growiBridgeService: GrowiBridgeService;
+
+  beforeEach(() => {
+    vi.clearAllMocks();
+    const crowiMock = mock<Crowi>();
+    growiBridgeService = new GrowiBridgeService(crowiMock);
+  });
+
+  describe('getFile', () => {
+    const baseDir = '/tmp/growi-export';
+
+    beforeEach(() => {
+      // Mock fs.accessSync to not throw (file exists)
+      vi.mocked(fs.accessSync).mockImplementation(() => undefined);
+    });
+
+    describe('valid file paths', () => {
+      test('should return resolved path for a simple filename', () => {
+        const fileName = 'test.json';
+        const result = growiBridgeService.getFile(fileName, baseDir);
+
+        expect(result).toBe(path.resolve(baseDir, fileName));
+        expect(fs.accessSync).toHaveBeenCalledWith(
+          path.resolve(baseDir, fileName),
+        );
+      });
+
+      test('should return resolved path for a filename in subdirectory', () => {
+        const fileName = 'subdir/test.json';
+        const result = growiBridgeService.getFile(fileName, baseDir);
+
+        expect(result).toBe(path.resolve(baseDir, fileName));
+      });
+
+      test('should handle baseDir with trailing slash', () => {
+        const fileName = 'test.json';
+        const baseDirWithSlash = '/tmp/growi-export/';
+        const result = growiBridgeService.getFile(fileName, baseDirWithSlash);
+
+        expect(result).toBe(path.resolve(baseDirWithSlash, fileName));
+      });
+    });
+
+    describe('path traversal attack prevention', () => {
+      test('should throw error for path traversal with ../', () => {
+        const fileName = '../etc/passwd';
+
+        expect(() => {
+          growiBridgeService.getFile(fileName, baseDir);
+        }).toThrow('Invalid file path: path traversal detected');
+      });
+
+      test('should throw error for path traversal with multiple ../', () => {
+        const fileName = '../../etc/passwd';
+
+        expect(() => {
+          growiBridgeService.getFile(fileName, baseDir);
+        }).toThrow('Invalid file path: path traversal detected');
+      });
+
+      test('should throw error for path traversal in middle of path', () => {
+        const fileName = 'subdir/../../../etc/passwd';
+
+        expect(() => {
+          growiBridgeService.getFile(fileName, baseDir);
+        }).toThrow('Invalid file path: path traversal detected');
+      });
+
+      test('should throw error for absolute path outside baseDir', () => {
+        const fileName = '/etc/passwd';
+
+        expect(() => {
+          growiBridgeService.getFile(fileName, baseDir);
+        }).toThrow('Invalid file path: path traversal detected');
+      });
+
+      test('should throw error for path traversal at the end of path', () => {
+        // e.g., trying to access sibling directory
+        const fileName = 'subdir/../../other-dir/file.json';
+
+        expect(() => {
+          growiBridgeService.getFile(fileName, baseDir);
+        }).toThrow('Invalid file path: path traversal detected');
+      });
+    });
+
+    describe('file access check', () => {
+      test('should throw error if file does not exist', () => {
+        const fileName = 'nonexistent.json';
+        vi.mocked(fs.accessSync).mockImplementation(() => {
+          throw new Error('ENOENT: no such file or directory');
+        });
+
+        expect(() => {
+          growiBridgeService.getFile(fileName, baseDir);
+        }).toThrow('ENOENT: no such file or directory');
+      });
+    });
+  });
+
+  describe('getEncoding', () => {
+    test('should return utf-8', () => {
+      expect(growiBridgeService.getEncoding()).toBe('utf-8');
+    });
+  });
+
+  describe('getMetaFileName', () => {
+    test('should return meta.json', () => {
+      expect(growiBridgeService.getMetaFileName()).toBe('meta.json');
+    });
+  });
+});

apps/app/src/server/service/growi-bridge/index.ts

@@ -1,12 +1,13 @@
-import fs from 'fs';
-import path from 'path';
-import { pipeline } from 'stream';
-import { finished } from 'stream/promises';
+import fs from 'node:fs';
+import { pipeline } from 'node:stream';
+import { finished } from 'node:stream/promises';
+import path from 'pathe';
 import unzipStream, { type Entry } from 'unzip-stream';
 
 import type Crowi from '~/server/crowi';
 import loggerFactory from '~/utils/logger';
 
+import { assertFileNameSafeForBaseDir } from '../../util/safe-path-utils';
 import type { ZipFileStat } from '../interfaces/export';
 import { tapStreamDataByPromise } from './unzip-stream-utils';
 
@@ -55,7 +56,10 @@ export class GrowiBridgeService {
    * @memberOf GrowiBridgeService
    */
   getFile(fileName: string, baseDir: string): string {
-    const jsonFile = path.join(baseDir, fileName);
+    // Prevent path traversal attack
+    assertFileNameSafeForBaseDir(fileName, baseDir);
+
+    const jsonFile = path.resolve(baseDir, fileName);
 
     // throws err if the file does not exist
     fs.accessSync(jsonFile);

apps/app/src/server/util/safe-path-utils.spec.ts

@@ -0,0 +1,169 @@
+import path from 'pathe';
+
+import {
+  assertFileNameSafeForBaseDir,
+  isFileNameSafeForBaseDir,
+  isPathWithinBase,
+} from './safe-path-utils';
+
+describe('path-utils', () => {
+  describe('isPathWithinBase', () => {
+    const baseDir = '/tmp/growi-export';
+
+    describe('valid paths', () => {
+      test('should return true for file directly in baseDir', () => {
+        const filePath = '/tmp/growi-export/test.json';
+        expect(isPathWithinBase(filePath, baseDir)).toBe(true);
+      });
+
+      test('should return true for file in subdirectory', () => {
+        const filePath = '/tmp/growi-export/subdir/test.json';
+        expect(isPathWithinBase(filePath, baseDir)).toBe(true);
+      });
+
+      test('should return true for baseDir itself', () => {
+        expect(isPathWithinBase(baseDir, baseDir)).toBe(true);
+      });
+
+      test('should handle relative paths correctly', () => {
+        const filePath = path.join(baseDir, 'test.json');
+        expect(isPathWithinBase(filePath, baseDir)).toBe(true);
+      });
+    });
+
+    describe('invalid paths (path traversal attacks)', () => {
+      test('should return false for path outside baseDir', () => {
+        const filePath = '/etc/passwd';
+        expect(isPathWithinBase(filePath, baseDir)).toBe(false);
+      });
+
+      test('should return false for path traversal with ../', () => {
+        const filePath = '/tmp/growi-export/../etc/passwd';
+        expect(isPathWithinBase(filePath, baseDir)).toBe(false);
+      });
+
+      test('should return false for sibling directory', () => {
+        const filePath = '/tmp/other-dir/test.json';
+        expect(isPathWithinBase(filePath, baseDir)).toBe(false);
+      });
+
+      test('should return false for directory with similar prefix', () => {
+        // /tmp/growi-export-evil should not match /tmp/growi-export
+        const filePath = '/tmp/growi-export-evil/test.json';
+        expect(isPathWithinBase(filePath, baseDir)).toBe(false);
+      });
+    });
+  });
+
+  describe('isFileNameSafeForBaseDir', () => {
+    const baseDir = '/tmp/growi-export';
+
+    describe('valid file names', () => {
+      test('should return true for simple filename', () => {
+        expect(isFileNameSafeForBaseDir('test.json', baseDir)).toBe(true);
+      });
+
+      test('should return true for filename in subdirectory', () => {
+        expect(isFileNameSafeForBaseDir('subdir/test.json', baseDir)).toBe(
+          true,
+        );
+      });
+
+      test('should return true for deeply nested file', () => {
+        expect(isFileNameSafeForBaseDir('a/b/c/d/test.json', baseDir)).toBe(
+          true,
+        );
+      });
+    });
+
+    describe('path traversal attacks', () => {
+      test('should return false for ../etc/passwd', () => {
+        expect(isFileNameSafeForBaseDir('../etc/passwd', baseDir)).toBe(false);
+      });
+
+      test('should return false for ../../etc/passwd', () => {
+        expect(isFileNameSafeForBaseDir('../../etc/passwd', baseDir)).toBe(
+          false,
+        );
+      });
+
+      test('should return false for subdir/../../../etc/passwd', () => {
+        expect(
+          isFileNameSafeForBaseDir('subdir/../../../etc/passwd', baseDir),
+        ).toBe(false);
+      });
+
+      test('should return false for absolute path outside baseDir', () => {
+        expect(isFileNameSafeForBaseDir('/etc/passwd', baseDir)).toBe(false);
+      });
+
+      test('should return false for path escaping to sibling directory', () => {
+        expect(
+          isFileNameSafeForBaseDir('subdir/../../other-dir/file.json', baseDir),
+        ).toBe(false);
+      });
+    });
+
+    describe('edge cases', () => {
+      test('should handle empty filename', () => {
+        // Empty filename resolves to baseDir itself, which is valid
+        expect(isFileNameSafeForBaseDir('', baseDir)).toBe(true);
+      });
+
+      test('should handle . (current directory)', () => {
+        expect(isFileNameSafeForBaseDir('.', baseDir)).toBe(true);
+      });
+
+      test('should handle ./filename', () => {
+        expect(isFileNameSafeForBaseDir('./test.json', baseDir)).toBe(true);
+      });
+    });
+  });
+
+  describe('assertFileNameSafeForBaseDir', () => {
+    const baseDir = '/tmp/growi-export';
+
+    describe('valid file names (should not throw)', () => {
+      test('should not throw for simple filename', () => {
+        expect(() => {
+          assertFileNameSafeForBaseDir('test.json', baseDir);
+        }).not.toThrow();
+      });
+
+      test('should not throw for filename in subdirectory', () => {
+        expect(() => {
+          assertFileNameSafeForBaseDir('subdir/test.json', baseDir);
+        }).not.toThrow();
+      });
+    });
+
+    describe('path traversal attacks (should throw)', () => {
+      test('should throw for ../etc/passwd', () => {
+        expect(() => {
+          assertFileNameSafeForBaseDir('../etc/passwd', baseDir);
+        }).toThrow('Invalid file path: path traversal detected');
+      });
+
+      test('should throw for ../../etc/passwd', () => {
+        expect(() => {
+          assertFileNameSafeForBaseDir('../../etc/passwd', baseDir);
+        }).toThrow('Invalid file path: path traversal detected');
+      });
+
+      test('should throw for absolute path outside baseDir', () => {
+        expect(() => {
+          assertFileNameSafeForBaseDir('/etc/passwd', baseDir);
+        }).toThrow('Invalid file path: path traversal detected');
+      });
+
+      test('should throw for path escaping to sibling directory', () => {
+        expect(() => {
+          assertFileNameSafeForBaseDir(
+            'subdir/../../other-dir/file.json',
+            baseDir,
+          );
+        }).toThrow('Invalid file path: path traversal detected');
+      });
+    });
+  });
+});

apps/app/src/server/util/safe-path-utils.ts

@@ -0,0 +1,69 @@
+import path from 'pathe';
+
+/**
+ * Validates that the given file path is within the base directory.
+ * This prevents path traversal attacks where an attacker could use sequences
+ * like '../' to access files outside the intended directory.
+ *
+ * @param filePath - The file path to validate
+ * @param baseDir - The base directory that the file path should be within
+ * @returns true if the path is valid, false otherwise
+ */
+export function isPathWithinBase(filePath: string, baseDir: string): boolean {
+  const resolvedBaseDir = path.resolve(baseDir);
+  const resolvedFilePath = path.resolve(filePath);
+
+  // Check if the resolved path starts with the base directory
+  // We add path.sep to ensure we're checking a directory boundary
+  // (e.g., /tmp/foo should not match /tmp/foobar)
+  return (
+    resolvedFilePath.startsWith(resolvedBaseDir + path.sep) ||
+    resolvedFilePath === resolvedBaseDir
+  );
+}
+
+/**
+ * Validates that joining baseDir with fileName results in a path within baseDir.
+ * This is useful for validating user-provided file names before using them.
+ *
+ * @param fileName - The file name to validate
+ * @param baseDir - The base directory
+ * @returns true if the resulting path is valid, false otherwise
+ * @throws Error if path traversal is detected
+ */
+export function assertFileNameSafeForBaseDir(
+  fileName: string,
+  baseDir: string,
+): void {
+  const resolvedBaseDir = path.resolve(baseDir);
+  const resolvedFilePath = path.resolve(baseDir, fileName);
+
+  const isValid =
+    resolvedFilePath.startsWith(resolvedBaseDir + path.sep) ||
+    resolvedFilePath === resolvedBaseDir;
+
+  if (!isValid) {
+    throw new Error('Invalid file path: path traversal detected');
+  }
+}
+
+/**
+ * Validates that joining baseDir with fileName results in a path within baseDir.
+ * This is useful for validating user-provided file names before using them.
+ *
+ * @param fileName - The file name to validate
+ * @param baseDir - The base directory
+ * @returns true if the resulting path is valid, false otherwise
+ */
+export function isFileNameSafeForBaseDir(
+  fileName: string,
+  baseDir: string,
+): boolean {
+  const resolvedBaseDir = path.resolve(baseDir);
+  const resolvedFilePath = path.resolve(baseDir, fileName);
+
+  return (
+    resolvedFilePath.startsWith(resolvedBaseDir + path.sep) ||
+    resolvedFilePath === resolvedBaseDir
+  );
+}