From 5c06e5bf84c850e8dba08e5044e0b9b8eb20c8a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20K=C3=B6ssler?= Date: Tue, 24 Sep 2024 12:17:39 +0200 Subject: [PATCH 01/13] Add new install script --- Makefile | 11 +-- package-lock.json | 1 + package.json | 4 ++ scripts/copyPackageJSON.js | 24 ------- scripts/install.js | 133 ++++++++++++++++++++++++------------- 5 files changed, 94 insertions(+), 79 deletions(-) delete mode 100644 scripts/copyPackageJSON.js diff --git a/Makefile b/Makefile index 57d501d5a..74785b78b 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,6 @@ .PHONY: containers containers: - cd sample-apps && docker-compose up -d --remove-orphans + npm run containers .PHONY: express-mongodb express-mongodb: @@ -64,14 +64,7 @@ nestjs-sentry: .PHONY: install install: - mkdir -p build - node scripts/copyPackageJSON.js - touch build/index.js - cd build && npm link - npm install - cd library && npm install - cd end2end && npm install - node scripts/install.js + npm run install .PHONY: build build: diff --git a/package-lock.json b/package-lock.json index 329b9f92f..3bc390470 100644 --- a/package-lock.json +++ b/package-lock.json @@ -5,6 +5,7 @@ "packages": { "": { "name": "firewall-node", + "hasInstallScript": true, "devDependencies": { "prettier": "^3.2.4" } diff --git a/package.json b/package.json index 2604a985d..601bb05fd 100644 --- a/package.json +++ b/package.json @@ -2,5 +2,9 @@ "name": "firewall-node", "devDependencies": { "prettier": "^3.2.4" + }, + "scripts": { + "install": "node scripts/install.js", + "containers": "cd sample-apps && docker-compose up -d --remove-orphans" } } diff --git a/scripts/copyPackageJSON.js b/scripts/copyPackageJSON.js deleted file mode 100644 index 352c2c4bc..000000000 --- a/scripts/copyPackageJSON.js +++ /dev/null @@ -1,24 +0,0 @@ -const { writeFile } = require("fs/promises"); -const { join } = require("path"); - -async function main() { - const pkg = require(join(__dirname, "../library/package.json")); - - // We're going to remove the devDependencies from the package.json - // Otherwise they will show up in every lock file - // whenever we add a new dev dependency to the library - delete pkg.devDependencies; - - await writeFile( - join(__dirname, "../build/package.json"), - JSON.stringify(pkg, null, 2) - ); -} - -(async () => { - try { - await main(); - } catch (e) { - console.error(e); - } -})(); diff --git a/scripts/install.js b/scripts/install.js index a32041355..eb702ff42 100644 --- a/scripts/install.js +++ b/scripts/install.js @@ -1,80 +1,121 @@ -const { readdir, stat, access, constants } = require("fs/promises"); +const { + readdir, + stat, + access, + mkdir, + writeFile, + constants, +} = require("fs/promises"); const { join } = require("path"); const { exec } = require("child_process"); const { promisify } = require("util"); const execAsync = promisify(exec); +const projectRoot = join(__dirname, ".."); + async function main() { - const sampleAppsDir = join(__dirname, "../sample-apps"); - const sampleApps = await readdir(sampleAppsDir); - - await Promise.all( - sampleApps.map(async (file) => { - const stats = await stat(join(sampleAppsDir, file)); - - if ( - !stats.isFile() && - (await fileExists(join(sampleAppsDir, file, "package.json"))) - ) { - await installSampleAppDeps(file); - } - }) - ); + await prepareBuildDir(); - const benchmarksDir = join(__dirname, "../benchmarks"); - const benchmarks = await readdir(benchmarksDir); + const installDirs = [".", "library", "end2end"]; + const scanForSubDirs = ["sample-apps", "benchmarks"]; - await Promise.all( - benchmarks.map(async (file) => { - const stats = await stat(join(benchmarksDir, file)); + for (const dir of scanForSubDirs) { + const subDirs = await scanForSubDirsWithPackageJson(dir); + installDirs.push(...subDirs); + } - if ( - !stats.isFile() && - (await fileExists(join(benchmarksDir, file, "package.json"))) - ) { - await installBenchmarkDeps(file); - } - }) - ); + // Install dependencies for all directories + await Promise.all(installDirs.map(installDeps)); + + console.log("Successfully installed all dependencies"); + process.exit(0); } -async function installSampleAppDeps(sampleApp) { - console.log(`Installing dependencies for ${sampleApp}`); +/** + * Install dependencies for a given folder + */ +async function installDeps(folder) { + console.log(`Installing dependencies for ${folder}`); try { await execAsync(`npm install`, { - cwd: join(__dirname, "../sample-apps", sampleApp), + cwd: join(projectRoot, folder), }); - console.log(`Dependencies installed for ${sampleApp}`); + console.log(`Installed dependencies for ${folder}`); } catch (error) { - console.error(`Failed to install dependencies for ${sampleApp}`); + console.error(`Failed to install dependencies for ${folder}`); console.error(error); process.exit(1); } } -async function installBenchmarkDeps(benchmark) { - console.log(`Installing dependencies for ${benchmark}`); +async function fileExists(path) { + try { + await access(path, constants.F_OK); + return true; + } catch { + return false; + } +} +/** + * Prepare the build directory + */ +async function prepareBuildDir() { try { - await execAsync(`npm install`, { - cwd: join(__dirname, "../benchmarks", benchmark), + const pkg = require(join(__dirname, "../library/package.json")); + + // We're going to remove the devDependencies from the package.json + // Otherwise they will show up in every lock file + // whenever we add a new dev dependency to the library + delete pkg.devDependencies; + + // If the build folder doesn't exist, create it + const buildDirPath = join(__dirname, "../build"); + if (!(await fileExists(buildDirPath))) { + await mkdir(buildDirPath); + } + + await writeFile( + join(buildDirPath, "package.json"), + JSON.stringify(pkg, null, 2) + ); + + // Create empty index.js file if it doesn't exist + if (!(await fileExists(join(buildDirPath, "index.js")))) { + await writeFile(join(buildDirPath, "index.js"), ""); + } + + // Link the library build folder + await execAsync(`npm link`, { + cwd: buildDirPath, }); - console.log(`Dependencies installed for ${benchmark}`); } catch (error) { - console.error(`Failed to install dependencies for ${benchmark}`); + console.error(`Failed to prepare build directory`); console.error(error); process.exit(1); } } -async function fileExists(path) { - try { - await access(path, constants.F_OK); - return true; - } catch { - return false; +/** + * Check for subdirectories with a package.json file in the given directory + */ +async function scanForSubDirsWithPackageJson(dir) { + const dirPath = join(projectRoot, dir); + const files = await readdir(dirPath, { withFileTypes: true }); + + const results = []; + + for (const file of files) { + if (file.isDirectory()) { + const packageJsonPath = join(dirPath, file.name, "package.json"); + if (await fileExists(packageJsonPath)) { + results.push(join(dir, file.name)); + } + } } + + return results; } (async () => { From 3320fa93dc2a1dc3d7af6c42e7c7aa6a7d555910 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20K=C3=B6ssler?= Date: Tue, 24 Sep 2024 12:34:55 +0200 Subject: [PATCH 02/13] Fix install script, migrate build --- Makefile | 9 ++------- package.json | 3 ++- scripts/build.js | 41 +++++++++++++++++++++++++++++++++++++++++ scripts/install.js | 15 +++++---------- 4 files changed, 50 insertions(+), 18 deletions(-) create mode 100644 scripts/build.js diff --git a/Makefile b/Makefile index 74785b78b..22f732947 100644 --- a/Makefile +++ b/Makefile @@ -64,16 +64,11 @@ nestjs-sentry: .PHONY: install install: - npm run install + npm install .PHONY: build build: - mkdir -p build - rm -r build - cd library && npm run build - cp README.md build/README.md - cp LICENSE build/LICENSE - cp library/package.json build/package.json + npm run build .PHONY: watch watch: build diff --git a/package.json b/package.json index 601bb05fd..01ff21688 100644 --- a/package.json +++ b/package.json @@ -5,6 +5,7 @@ }, "scripts": { "install": "node scripts/install.js", - "containers": "cd sample-apps && docker-compose up -d --remove-orphans" + "containers": "cd sample-apps && docker-compose up -d --remove-orphans", + "build": "node scripts/build.js" } } diff --git a/scripts/build.js b/scripts/build.js new file mode 100644 index 000000000..45ab15fe3 --- /dev/null +++ b/scripts/build.js @@ -0,0 +1,41 @@ +const { existsSync } = require("fs"); +const { rm, copyFile } = require("fs/promises"); +const { join } = require("path"); +const { exec } = require("child_process"); +const { promisify } = require("util"); +const execAsync = promisify(exec); + +async function main() { + const rootDir = join(__dirname, ".."); + const buildDir = join(rootDir, "build"); + const libDir = join(rootDir, "library"); + + // Delete build directory if it exists + if (existsSync(buildDir)) { + await rm(buildDir, { recursive: true }); + } + + await execAsync(`npm run build`, { + cwd: libDir, + }); + + // Copy additional files to build directory + await copyFile( + join(rootDir, "library", "package.json"), + join(buildDir, "package.json") + ); + await copyFile(join(rootDir, "README.md"), join(buildDir, "README.md")); + await copyFile(join(rootDir, "LICENSE"), join(buildDir, "LICENSE")); + + console.log("Build successful"); + process.exit(0); +} + +(async () => { + try { + await main(); + } catch (error) { + console.error(error); + process.exit(1); + } +})(); diff --git a/scripts/install.js b/scripts/install.js index eb702ff42..5ee570538 100644 --- a/scripts/install.js +++ b/scripts/install.js @@ -1,11 +1,4 @@ -const { - readdir, - stat, - access, - mkdir, - writeFile, - constants, -} = require("fs/promises"); +const { readdir, access, mkdir, writeFile, constants } = require("fs/promises"); const { join } = require("path"); const { exec } = require("child_process"); const { promisify } = require("util"); @@ -16,7 +9,7 @@ const projectRoot = join(__dirname, ".."); async function main() { await prepareBuildDir(); - const installDirs = [".", "library", "end2end"]; + const installDirs = ["library", "end2end"]; const scanForSubDirs = ["sample-apps", "benchmarks"]; for (const dir of scanForSubDirs) { @@ -25,7 +18,9 @@ async function main() { } // Install dependencies for all directories - await Promise.all(installDirs.map(installDeps)); + for (const dir of installDirs) { + await installDeps(dir); + } console.log("Successfully installed all dependencies"); process.exit(0); From f01db3683e1258daab5e35dce22375f9dc7c38ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20K=C3=B6ssler?= Date: Tue, 24 Sep 2024 13:34:47 +0200 Subject: [PATCH 03/13] Migrate benchmarks, update workflows and docs --- .github/CONTRIBUTING.md | 12 ++++---- .github/workflows/benchmark.yml | 4 +-- .github/workflows/build-and-release.yml | 6 ++-- .github/workflows/end-to-end-tests.yml | 6 ++-- .github/workflows/lint-code.yml | 4 +-- .github/workflows/unit-test.yml | 4 +-- Makefile | 16 ++++------- package.json | 11 ++++++-- scripts/benchmark.js | 31 +++++++++++++++++++++ scripts/helpers/fs.js | 37 +++++++++++++++++++++++++ scripts/install.js | 33 ++-------------------- 11 files changed, 103 insertions(+), 61 deletions(-) create mode 100644 scripts/benchmark.js create mode 100644 scripts/helpers/fs.js diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index 67814cdd9..64a5fb41a 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -96,12 +96,12 @@ Enhancement suggestions are tracked as [GitHub issues](https://github.com/Aikido ### Your First Code Contribution - clone the repository to your local machine -- run `$ make install` to install dependencies -- run `$ make build` to build the library -- run `$ make watch` to watch for changes and rebuild the library -- run `$ make test` to run tests using tap -- run `$ make end2end` to run end-to-end tests using tap -- run `$ make lint` to run ESLint +- run `$ npm install` to install dependencies +- run `$ npm run build` to build the library +- run `$ npm run watch` to watch for changes and rebuild the library +- run `$ npm run test` to run tests using tap +- run `$ npm run end2end` to run end-to-end tests using tap +- run `$ npm run lint` to run ESLint ## Styleguides diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index d8c803cd5..053ba40e6 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -33,8 +33,8 @@ jobs: node-version: ${{ matrix.node-version }} - name: Install K6 uses: grafana/setup-k6-action@v1 - - run: make install - - run: make build + - run: npm install + - run: npm run build - name: Run NoSQL Injection Benchmark run: cd benchmarks/nosql-injection && AIKIDO_CI=true node --preserve-symlinks benchmark.js - name: Run SQL Injection Benchmark diff --git a/.github/workflows/build-and-release.yml b/.github/workflows/build-and-release.yml index 83ea8fcae..b7bd00fa0 100644 --- a/.github/workflows/build-and-release.yml +++ b/.github/workflows/build-and-release.yml @@ -26,13 +26,13 @@ jobs: node-version: "18.x" registry-url: "https://registry.npmjs.org" scope: "@aikidosec" - - run: make install + - run: npm install - name: Get the version id: get_version run: echo ::set-output name=tag::${GITHUB_REF/refs\/tags\//} - run: cd library && npm --no-git-tag-version version ${{ steps.get_version.outputs.tag }} - - run: make build - - run: make lint + - run: npm run build + - run: npm run lint - run: cd build && npm publish --provenance --access public env: NODE_AUTH_TOKEN: ${{ secrets.NPM_PUBLISH_TOKEN }} diff --git a/.github/workflows/end-to-end-tests.yml b/.github/workflows/end-to-end-tests.yml index 77a296461..f95cda9c1 100644 --- a/.github/workflows/end-to-end-tests.yml +++ b/.github/workflows/end-to-end-tests.yml @@ -45,6 +45,6 @@ jobs: - name: Build and run server run: | cd end2end/server && docker build -t server . && docker run -d -p 5874:3000 server - - run: make install - - run: make build - - run: make end2end + - run: npm install + - run: npm run build + - run: npm run end2end diff --git a/.github/workflows/lint-code.yml b/.github/workflows/lint-code.yml index 7130f2497..eec4984ad 100644 --- a/.github/workflows/lint-code.yml +++ b/.github/workflows/lint-code.yml @@ -12,5 +12,5 @@ jobs: uses: actions/setup-node@v2 with: node-version: ${{ matrix.node-version }} - - run: make install - - run: make lint + - run: npm install + - run: npm run lint diff --git a/.github/workflows/unit-test.yml b/.github/workflows/unit-test.yml index 9c4da4a44..e737af6db 100644 --- a/.github/workflows/unit-test.yml +++ b/.github/workflows/unit-test.yml @@ -48,8 +48,8 @@ jobs: - name: Add local.aikido.io to /etc/hosts run: | sudo echo "127.0.0.1 local.aikido.io" | sudo tee -a /etc/hosts - - run: make install - - run: make test-ci + - run: npm install + - run: npm run test:ci - name: "Upload coverage" uses: codecov/codecov-action@v4.0.1 with: diff --git a/Makefile b/Makefile index 22f732947..7c0f2f70e 100644 --- a/Makefile +++ b/Makefile @@ -72,27 +72,23 @@ build: .PHONY: watch watch: build - cd library && npm run build:watch + npm run watch .PHONY: test test: - cd library && npm run test + npm run test .PHONY: test-ci test-ci: - cd library && npm run test:ci + npm run test:ci .PHONY: lint lint: - cd library && npm run lint + npm run lint .PHONY: end2end end2end: - cd end2end && npm run test + npm run end2end benchmark: build - cd benchmarks/nosql-injection && AIKIDO_CI=true node --preserve-symlinks benchmark.js - cd benchmarks/shell-injection && node --preserve-symlinks benchmark.js - cd benchmarks/sql-injection && node --preserve-symlinks benchmark.js - cd benchmarks/hono-pg && node --preserve-symlinks benchmark.js - cd benchmarks/api-discovery && node --preserve-symlinks benchmark.js + npm run benchmark diff --git a/package.json b/package.json index 01ff21688..84724efce 100644 --- a/package.json +++ b/package.json @@ -5,7 +5,14 @@ }, "scripts": { "install": "node scripts/install.js", - "containers": "cd sample-apps && docker-compose up -d --remove-orphans", - "build": "node scripts/build.js" + "containers": "cd sample-apps && docker compose up -d --remove-orphans", + "build": "node scripts/build.js", + "watch": "cd library && npm run build:watch", + "test": "cd library && npm run test", + "test:ci": "cd library && npm run test:ci", + "lint": "cd library && npm run lint", + "end2end": "cd end2end && npm run test", + "format": "prettier --write .", + "benchmark": "node scripts/benchmark.js" } } diff --git a/scripts/benchmark.js b/scripts/benchmark.js new file mode 100644 index 000000000..211d4b055 --- /dev/null +++ b/scripts/benchmark.js @@ -0,0 +1,31 @@ +const { exec } = require("child_process"); +const { promisify } = require("util"); +const execAsync = promisify(exec); +const { scanForSubDirsWithPackageJson } = require("./helpers/fs"); +const { join } = require("path"); + +async function main() { + const benchmarks = await scanForSubDirsWithPackageJson("benchmarks"); + + for (const benchmark of benchmarks) { + console.log(`Running ${benchmark}`); + const output = await execAsync("node --preserve-symlinks benchmark.js", { + cwd: join(__dirname, "..", benchmark), + env: { + ...process.env, + AIKIDO_CI: "true", + }, + }); + console.log(output.stdout); + console.error(output.stderr); + console.log(`Finished running ${benchmark}`); + } +} + +(async () => { + try { + await main(); + } catch (error) { + console.error(error); + } +})(); diff --git a/scripts/helpers/fs.js b/scripts/helpers/fs.js new file mode 100644 index 000000000..310f3b864 --- /dev/null +++ b/scripts/helpers/fs.js @@ -0,0 +1,37 @@ +const { readdir, access, constants } = require("fs/promises"); +const { join } = require("path"); + +async function fileExists(path) { + try { + await access(path, constants.F_OK); + return true; + } catch { + return false; + } +} + +/** + * Check for subdirectories with a package.json file in the given directory + */ +async function scanForSubDirsWithPackageJson(dir) { + const dirPath = join(__dirname, "../..", dir); + const files = await readdir(dirPath, { withFileTypes: true }); + + const results = []; + + for (const file of files) { + if (file.isDirectory()) { + const packageJsonPath = join(dirPath, file.name, "package.json"); + if (await fileExists(packageJsonPath)) { + results.push(join(dir, file.name)); + } + } + } + + return results; +} + +module.exports = { + fileExists, + scanForSubDirsWithPackageJson, +}; diff --git a/scripts/install.js b/scripts/install.js index 5ee570538..ea8483bf5 100644 --- a/scripts/install.js +++ b/scripts/install.js @@ -1,8 +1,9 @@ -const { readdir, access, mkdir, writeFile, constants } = require("fs/promises"); +const { readdir, mkdir, writeFile } = require("fs/promises"); const { join } = require("path"); const { exec } = require("child_process"); const { promisify } = require("util"); const execAsync = promisify(exec); +const { fileExists, scanForSubDirsWithPackageJson } = require("./helpers/fs"); const projectRoot = join(__dirname, ".."); @@ -44,15 +45,6 @@ async function installDeps(folder) { } } -async function fileExists(path) { - try { - await access(path, constants.F_OK); - return true; - } catch { - return false; - } -} - /** * Prepare the build directory */ @@ -92,27 +84,6 @@ async function prepareBuildDir() { } } -/** - * Check for subdirectories with a package.json file in the given directory - */ -async function scanForSubDirsWithPackageJson(dir) { - const dirPath = join(projectRoot, dir); - const files = await readdir(dirPath, { withFileTypes: true }); - - const results = []; - - for (const file of files) { - if (file.isDirectory()) { - const packageJsonPath = join(dirPath, file.name, "package.json"); - if (await fileExists(packageJsonPath)) { - results.push(join(dir, file.name)); - } - } - } - - return results; -} - (async () => { try { await main(); From 4e12015abdfcda6ad9cf99b60b11a1f8e6684886 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20K=C3=B6ssler?= Date: Tue, 24 Sep 2024 15:52:24 +0200 Subject: [PATCH 04/13] Fix first failing tests on windows --- library/helpers/isWindows.ts | 1 + library/sinks/ChildProcess.test.ts | 37 ++++++++++++++++++++---------- library/sinks/Path.test.ts | 7 +++++- 3 files changed, 32 insertions(+), 13 deletions(-) create mode 100644 library/helpers/isWindows.ts diff --git a/library/helpers/isWindows.ts b/library/helpers/isWindows.ts new file mode 100644 index 000000000..1ebc44b8a --- /dev/null +++ b/library/helpers/isWindows.ts @@ -0,0 +1 @@ +export const isWindows = process.platform === "win32"; diff --git a/library/sinks/ChildProcess.test.ts b/library/sinks/ChildProcess.test.ts index b57e72b1a..cde45b113 100644 --- a/library/sinks/ChildProcess.test.ts +++ b/library/sinks/ChildProcess.test.ts @@ -5,6 +5,8 @@ import { Context, runWithContext } from "../agent/Context"; import { LoggerNoop } from "../agent/logger/LoggerNoop"; import { ChildProcess } from "./ChildProcess"; import { execFile, execFileSync, fork } from "child_process"; +import { join } from "path"; +import { isWindows } from "../helpers/isWindows"; const unsafeContext: Context = { remoteAddress: "::1", @@ -72,19 +74,30 @@ t.test("it works", async (t) => { }); const runSafeCommands = () => { - exec("ls", (err, stdout, stderr) => {}).unref(); - execSync("ls", (err, stdout, stderr) => {}); - - spawn("ls", ["-la"], {}, (err, stdout, stderr) => {}).unref(); - spawnSync("ls", ["-la"], {}, (err, stdout, stderr) => {}); - - spawn("ls", ["-la"], { shell: false }, (err, stdout, stderr) => {}).unref(); - spawnSync("ls", ["-la"], { shell: false }, (err, stdout, stderr) => {}); - - execFile("ls", ["-la"], {}, (err, stdout, stderr) => {}).unref(); - execFileSync("ls", ["-la"], {}); + if (!isWindows) { + exec("ls", (err, stdout, stderr) => {}).unref(); + execSync("ls", (err, stdout, stderr) => {}); + + spawn("ls", ["-la"], {}, (err, stdout, stderr) => {}).unref(); + spawnSync("ls", ["-la"], {}, (err, stdout, stderr) => {}); + + spawn( + "ls", + ["-la"], + { shell: false }, + (err, stdout, stderr) => {} + ).unref(); + spawnSync("ls", ["-la"], { shell: false }, (err, stdout, stderr) => {}); + + execFile("ls", ["-la"], {}, (err, stdout, stderr) => {}).unref(); + execFileSync("ls", ["-la"], {}); + } else { + // Is Windows + exec("dir", (err, stdout, stderr) => {}).unref(); + execSync("dir", (err, stdout, stderr) => {}); + } - fork("./fixtures/helloWorld.js").unref(); + fork(join(__dirname, "./fixtures/helloWorld.js")).unref(); }; runSafeCommands(); diff --git a/library/sinks/Path.test.ts b/library/sinks/Path.test.ts index a104df230..fdd3f2c7a 100644 --- a/library/sinks/Path.test.ts +++ b/library/sinks/Path.test.ts @@ -4,6 +4,7 @@ import { ReportingAPIForTesting } from "../agent/api/ReportingAPIForTesting"; import { Context, runWithContext } from "../agent/Context"; import { LoggerNoop } from "../agent/logger/LoggerNoop"; import { Path } from "./Path"; +import { isWindows } from "../helpers/isWindows"; const unsafeContext: Context = { remoteAddress: "::1", @@ -43,7 +44,11 @@ t.test("it works", async (t) => { function safeCalls() { t.same(join("test.txt"), "test.txt"); t.same(resolve(__dirname, "./test.txt"), join(__dirname, "./test.txt")); - t.same(join("/app", "/etc/data"), resolve("/app/etc/data")); + if (!isWindows) { + t.same(join("/app", "/etc/data"), resolve("/app/etc/data")); + } else { + t.same(join("c:/app", "/etc/data"), resolve("c:/app/etc/data")); + } } safeCalls(); From e1075aabbf3a7c0f37c93d4de0a3597eddc2e263 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20K=C3=B6ssler?= Date: Wed, 25 Sep 2024 10:49:31 +0200 Subject: [PATCH 05/13] Fix file hooks --- .prettierignore | 5 +++++ library/agent/applyHooks.ts | 5 +++-- 2 files changed, 8 insertions(+), 2 deletions(-) create mode 100644 .prettierignore diff --git a/.prettierignore b/.prettierignore new file mode 100644 index 000000000..2c6239355 --- /dev/null +++ b/.prettierignore @@ -0,0 +1,5 @@ +# Ignore generated files +.tap +.next +build +dist \ No newline at end of file diff --git a/library/agent/applyHooks.ts b/library/agent/applyHooks.ts index a3fb1cd54..cd097bc50 100644 --- a/library/agent/applyHooks.ts +++ b/library/agent/applyHooks.ts @@ -1,5 +1,6 @@ /* eslint-disable max-lines-per-function */ -import { join, resolve } from "path"; +import { resolve } from "path"; +import { join as posixJoin } from "path/posix"; import { cleanupStackTrace } from "../helpers/cleanupStackTrace"; import { wrap } from "../helpers/wrap"; import { getPackageVersion } from "../helpers/getPackageVersion"; @@ -111,7 +112,7 @@ export function applyHooks(hooks: Hooks, agent: Agent) { function wrapFiles(pkg: Package, files: WrappableFile[], agent: Agent) { files.forEach((file) => { try { - const exports = require(join(pkg.getName(), file.getRelativePath())); + const exports = require(posixJoin(pkg.getName(), file.getRelativePath())); file .getSubjects() From 13af45ee29c983d211bad390bd084cb966bb8677 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20K=C3=B6ssler?= Date: Wed, 25 Sep 2024 12:27:01 +0200 Subject: [PATCH 06/13] Fix path traversal, migrate e2e tests --- end2end/package.json | 2 +- end2end/run.js | 9 +++ library/sinks/FileSystem.test.ts | 81 +++++++++++++++---- .../checkContextForPathTraversal.test.ts | 52 ++++++++---- .../detectPathTraversal.test.ts | 1 - .../path-traversal/detectPathTraversal.ts | 24 ++++-- 6 files changed, 128 insertions(+), 41 deletions(-) create mode 100644 end2end/run.js diff --git a/end2end/package.json b/end2end/package.json index 53387d8bc..d737556f0 100644 --- a/end2end/package.json +++ b/end2end/package.json @@ -6,6 +6,6 @@ "tap": "^18.7.0" }, "scripts": { - "test": "AIKIDO_CI=true tap tests/*.js --allow-empty-coverage -j 1" + "test": "node run.js" } } diff --git a/end2end/run.js b/end2end/run.js new file mode 100644 index 000000000..9bd000c5e --- /dev/null +++ b/end2end/run.js @@ -0,0 +1,9 @@ +const { execSync } = require("child_process"); + +// Command to run +const command = "tap tests/*.js --allow-empty-coverage -j 1"; + +execSync(command, { + stdio: "inherit", + env: { ...process.env, AIKIDO_CI: "true" }, +}); diff --git a/library/sinks/FileSystem.test.ts b/library/sinks/FileSystem.test.ts index 3ad5d678d..1486bad33 100644 --- a/library/sinks/FileSystem.test.ts +++ b/library/sinks/FileSystem.test.ts @@ -4,6 +4,7 @@ import { ReportingAPIForTesting } from "../agent/api/ReportingAPIForTesting"; import { Context, runWithContext } from "../agent/Context"; import { LoggerNoop } from "../agent/logger/LoggerNoop"; import { FileSystem } from "./FileSystem"; +import { isWindows } from "../helpers/isWindows"; const unsafeContext: Context = { remoteAddress: "::1", @@ -96,7 +97,11 @@ t.test("it works", async (t) => { { encoding: "utf-8" } ); rename("./test.txt", "./test2.txt", (err) => {}); - rename(new URL("file:///test123.txt"), "test2.txt", (err) => {}); + if (!isWindows) { + rename(new URL("file:///test123.txt"), "test2.txt", (err) => {}); + } else { + rename(new URL("file:///c:/test123.txt"), "test2.txt", (err) => {}); + } rename(Buffer.from("./test123.txt"), "test2.txt", (err) => {}); }; @@ -153,23 +158,59 @@ t.test("it works", async (t) => { "Zen has blocked a path traversal attack: fs.rename(...) originating from body.file.matches" ); - throws( - () => rename(new URL("file:///../test.txt"), "../test2.txt", (err) => {}), - "Zen has blocked a path traversal attack: fs.rename(...) originating from body.file.matches" - ); + if (!isWindows) { + throws( + () => + rename(new URL("file:///../test.txt"), "../test2.txt", (err) => {}), + "Zen has blocked a path traversal attack: fs.rename(...) originating from body.file.matches" + ); - throws( - () => - rename(new URL("file:///./../test.txt"), "../test2.txt", (err) => {}), - "Zen has blocked a path traversal attack: fs.rename(...) originating from body.file.matches" - ); + throws( + () => + rename(new URL("file:///./../test.txt"), "../test2.txt", (err) => {}), + "Zen has blocked a path traversal attack: fs.rename(...) originating from body.file.matches" + ); - throws( - () => - rename(new URL("file:///../../test.txt"), "../test2.txt", (err) => {}), - "Zen has blocked a path traversal attack: fs.rename(...) originating from body.file.matches" - ); + throws( + () => + rename( + new URL("file:///../../test.txt"), + "../test2.txt", + (err) => {} + ), + "Zen has blocked a path traversal attack: fs.rename(...) originating from body.file.matches" + ); + } else { + throws( + () => + rename( + new URL("file:///C:/../test.txt"), + "../test2.txt", + (err) => {} + ), + "Zen has blocked a path traversal attack: fs.rename(...) originating from body.file.matches" + ); + throws( + () => + rename( + new URL("file:///C:/./../test.txt"), + "../test2.txt", + (err) => {} + ), + "Zen has blocked a path traversal attack: fs.rename(...) originating from body.file.matches" + ); + + throws( + () => + rename( + new URL("file:///C:/../../test.txt"), + "../test2.txt", + (err) => {} + ), + "Zen has blocked a path traversal attack: fs.rename(...) originating from body.file.matches" + ); + } throws( () => rename(Buffer.from("../test.txt"), "../test2.txt", (err) => {}), "Zen has blocked a path traversal attack: fs.rename(...) originating from body.file.matches" @@ -193,7 +234,15 @@ t.test("it works", async (t) => { runWithContext( { ...unsafeContext, body: { file: { matches: "../%" } } }, () => { - rename(new URL("file:///../../test.txt"), "../test2.txt", (err) => {}); + if (!isWindows) { + rename(new URL("file:///../../test.txt"), "../test2.txt", (err) => {}); + } else { + rename( + new URL("file:/C://../../test.txt"), + "../test2.txt", + (err) => {} + ); + } } ); }); diff --git a/library/vulnerabilities/path-traversal/checkContextForPathTraversal.test.ts b/library/vulnerabilities/path-traversal/checkContextForPathTraversal.test.ts index 7788bc7db..fc4317d2d 100644 --- a/library/vulnerabilities/path-traversal/checkContextForPathTraversal.test.ts +++ b/library/vulnerabilities/path-traversal/checkContextForPathTraversal.test.ts @@ -1,5 +1,6 @@ import * as t from "tap"; import { checkContextForPathTraversal } from "./checkContextForPathTraversal"; +import { isWindows } from "../../helpers/isWindows"; const unsafeContext = { filename: "../file/test.txt", @@ -81,22 +82,41 @@ t.test("it does not flag safe operation", async () => { }); t.test("it detects path traversal with URL", async () => { - t.same( - checkContextForPathTraversal({ - ...unsafeContext, - filename: new URL("file:///../file/test.txt"), - }), - { - operation: "operation", - kind: "path_traversal", - source: "routeParams", - pathToPayload: ".path", - metadata: { - filename: "/file/test.txt", - }, - payload: "../file", - } - ); + if (!isWindows) { + t.same( + checkContextForPathTraversal({ + ...unsafeContext, + filename: new URL("file:///../file/test.txt"), + }), + { + operation: "operation", + kind: "path_traversal", + source: "routeParams", + pathToPayload: ".path", + metadata: { + filename: "/file/test.txt", + }, + payload: "../file", + } + ); + } else { + t.same( + checkContextForPathTraversal({ + ...unsafeContext, + filename: new URL("file:///C:/../file/test.txt"), + }), + { + operation: "operation", + kind: "path_traversal", + source: "routeParams", + pathToPayload: ".path", + metadata: { + filename: "/C:/file/test.txt", + }, + payload: "../file", + } + ); + } }); t.test("it detects path traversal with Buffer", async () => { diff --git a/library/vulnerabilities/path-traversal/detectPathTraversal.test.ts b/library/vulnerabilities/path-traversal/detectPathTraversal.test.ts index 1fe0acfff..bcae388bf 100644 --- a/library/vulnerabilities/path-traversal/detectPathTraversal.test.ts +++ b/library/vulnerabilities/path-traversal/detectPathTraversal.test.ts @@ -1,6 +1,5 @@ import * as t from "tap"; import { detectPathTraversal } from "./detectPathTraversal"; -import { join, resolve } from "path"; t.test("empty user input", async () => { t.same(detectPathTraversal("test.txt", ""), false); diff --git a/library/vulnerabilities/path-traversal/detectPathTraversal.ts b/library/vulnerabilities/path-traversal/detectPathTraversal.ts index 1a15934b9..ebc7df2ec 100644 --- a/library/vulnerabilities/path-traversal/detectPathTraversal.ts +++ b/library/vulnerabilities/path-traversal/detectPathTraversal.ts @@ -1,6 +1,7 @@ +import { normalize } from "path"; +import { isWindows } from "../../helpers/isWindows"; import { containsUnsafePathParts } from "./containsUnsafePathParts"; import { startsWithUnsafePath } from "./unsafePathStart"; -import { fileURLToPath } from "url"; export function detectPathTraversal( filePath: string, @@ -58,14 +59,23 @@ export function detectPathTraversal( */ function parseAsFileUrl(path: string) { let url = path; - if (!url.startsWith("file:")) { - if (!url.startsWith("/")) { - url = `/${url}`; - } - url = `file://${url}`; + + // Remove leading file: + if (url.startsWith("file:")) { + url = url.slice(5); + } + + if (!url.startsWith("/")) { + url = "/" + url; } + try { - return fileURLToPath(url); + const normalizePath = normalize(url); + if (isWindows) { + // Replace backslashes with forward slashes + return normalizePath.replace(/\\/g, "/"); + } + return normalizePath; } catch (e) { // } From b590a84e604e72d8d84d6310a7bd9d7dc2635423 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20K=C3=B6ssler?= Date: Wed, 25 Sep 2024 13:03:11 +0200 Subject: [PATCH 07/13] Run unit tests on Windows in CI --- .github/workflows/unit-test.yml | 22 ++++++++++++++++- library/sinks/BetterSQLite3.test.ts | 37 ++++++++++++++++------------- 2 files changed, 41 insertions(+), 18 deletions(-) diff --git a/.github/workflows/unit-test.yml b/.github/workflows/unit-test.yml index e737af6db..299a689d3 100644 --- a/.github/workflows/unit-test.yml +++ b/.github/workflows/unit-test.yml @@ -3,7 +3,7 @@ on: push: {} workflow_call: {} jobs: - build: + test: runs-on: ubuntu-latest services: s3: @@ -57,3 +57,23 @@ jobs: env: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} slug: AikidoSec/firewall-node + test-windows: + runs-on: windows-latest + steps: + - uses: actions/checkout@v4 + - name: Use Node.js 20.x + uses: actions/setup-node@v2 + with: + node-version: 20.x + - name: Add local.aikido.io to hosts file + run: | + Add-Content -Path "C:\Windows\System32\drivers\etc\hosts" -Value "127.0.0.1 local.aikido.io" + - run: npm install + - run: npm run test:ci + - name: "Upload coverage" + uses: codecov/codecov-action@v4.0.1 + with: + file: ./library/.tap/report/lcov.info + env: + CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} + slug: AikidoSec/firewall-node diff --git a/library/sinks/BetterSQLite3.test.ts b/library/sinks/BetterSQLite3.test.ts index 3770a7be9..300069014 100644 --- a/library/sinks/BetterSQLite3.test.ts +++ b/library/sinks/BetterSQLite3.test.ts @@ -4,6 +4,7 @@ import { ReportingAPIForTesting } from "../agent/api/ReportingAPIForTesting"; import { runWithContext, type Context } from "../agent/Context"; import { LoggerNoop } from "../agent/logger/LoggerNoop"; import { BetterSQLite3 } from "./BetterSQLite3"; +import { isWindows } from "../helpers/isWindows"; const dangerousContext: Context = { remoteAddress: "::1", @@ -117,25 +118,27 @@ t.test("it detects SQL injections", async (t) => { } }); - await runWithContext(dangerousPathContext, async () => { - const error = t.throws(() => db.backup("/tmp/insecure")); - t.ok(error instanceof Error); - if (error instanceof Error) { - t.same( - error.message, - "Zen has blocked a path traversal attack: better-sqlite3.backup(...) originating from body.myTitle" - ); - } - await db.backup("/tmp/sqlite-test-secure"); - }); + if (!isWindows) { + await runWithContext(dangerousPathContext, async () => { + const error = t.throws(() => db.backup("/tmp/insecure")); + t.ok(error instanceof Error); + if (error instanceof Error) { + t.same( + error.message, + "Zen has blocked a path traversal attack: better-sqlite3.backup(...) originating from body.myTitle" + ); + } + await db.backup("/tmp/sqlite-test-secure"); + }); - await db.backup("/tmp/sqlite-test-secure-2"); + await db.backup("/tmp/sqlite-test-secure-2"); - try { - await db.backup(); - t.fail("Expected an error"); - } catch (error: any) { - t.same(error.message, "Expected first argument to be a string"); + try { + await db.backup(); + t.fail("Expected an error"); + } catch (error: any) { + t.same(error.message, "Expected first argument to be a string"); + } } } catch (error: any) { t.fail(error); From 4b2dcdf0b82b1ba53bc5b4b98fcfcdeef056c5bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20K=C3=B6ssler?= Date: Wed, 25 Sep 2024 13:13:15 +0200 Subject: [PATCH 08/13] Fix CI workflows --- .github/workflows/benchmark.yml | 2 +- .github/workflows/end-to-end-tests.yml | 2 +- .github/workflows/lint-code.yml | 2 +- .github/workflows/unit-test-windows.yml | 55 +++++++++++++++++++++++++ .github/workflows/unit-test.yml | 22 +--------- library/package.json | 2 +- scripts/run-tap.js | 5 +++ 7 files changed, 65 insertions(+), 25 deletions(-) create mode 100644 .github/workflows/unit-test-windows.yml diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index 053ba40e6..8f99157fc 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -3,7 +3,7 @@ on: push: {} workflow_call: {} jobs: - build: + benchmark: runs-on: ubuntu-latest services: mongodb: diff --git a/.github/workflows/end-to-end-tests.yml b/.github/workflows/end-to-end-tests.yml index f95cda9c1..ce7c82217 100644 --- a/.github/workflows/end-to-end-tests.yml +++ b/.github/workflows/end-to-end-tests.yml @@ -3,7 +3,7 @@ on: push: {} workflow_call: {} jobs: - build: + e2etest: runs-on: ubuntu-latest services: mongodb: diff --git a/.github/workflows/lint-code.yml b/.github/workflows/lint-code.yml index eec4984ad..8e39bc79e 100644 --- a/.github/workflows/lint-code.yml +++ b/.github/workflows/lint-code.yml @@ -1,7 +1,7 @@ name: Lint code on: push jobs: - build: + lint: runs-on: ubuntu-latest strategy: matrix: diff --git a/.github/workflows/unit-test-windows.yml b/.github/workflows/unit-test-windows.yml new file mode 100644 index 000000000..01eb4d2cf --- /dev/null +++ b/.github/workflows/unit-test-windows.yml @@ -0,0 +1,55 @@ +name: Unit tests Windows +on: + push: {} + workflow_call: {} +jobs: + test-windows: + runs-on: windows-latest + services: + s3: + image: adobe/s3mock + env: + "initialBuckets": "bucket" + ports: + - "9090:9090" + mongodb: + image: mongo:5 + env: + "MONGO_INITDB_ROOT_USERNAME": "root" + "MONGO_INITDB_ROOT_PASSWORD": "password" + ports: + - 27017:27017 + postgres: + image: postgres:14-alpine + env: + "POSTGRES_PASSWORD": "password" + "POSTGRES_USER": "root" + "POSTGRES_DB": "main_db" + ports: + - "27016:5432" + mysql: + image: mysql:8.0 + # NOTE: use of "mysql_native_password" is not recommended: https://dev.mysql.com/doc/refman/8.0/en/upgrading-from-previous-series.html#upgrade-caching-sha2-password + # We need to use this long command in order to execute the last part : mysql_native_password + # https://stackoverflow.com/questions/60902904/how-to-pass-mysql-native-password-to-mysql-service-in-github-actions + options: --health-cmd="mysqladmin ping" --health-interval=10s --health-timeout=5s --health-retries=5 -e MYSQL_ROOT_PASSWORD=mypassword -e MYSQL_DATABASE=catsdb --entrypoint sh mysql:8.0 -c "exec docker-entrypoint.sh mysqld --default-authentication-plugin=mysql_native_password" + ports: + - "27015:3306" + steps: + - uses: actions/checkout@v4 + - name: Use Node.js 20.x + uses: actions/setup-node@v4 + with: + node-version: 20.x + - name: Add local.aikido.io to hosts file + run: | + Add-Content -Path "C:\Windows\System32\drivers\etc\hosts" -Value "127.0.0.1 local.aikido.io" + - run: npm install + - run: npm run test:ci + - name: "Upload coverage" + uses: codecov/codecov-action@v4.0.1 + with: + file: ./library/.tap/report/lcov.info + env: + CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} + slug: AikidoSec/firewall-node diff --git a/.github/workflows/unit-test.yml b/.github/workflows/unit-test.yml index 299a689d3..1eb589d3d 100644 --- a/.github/workflows/unit-test.yml +++ b/.github/workflows/unit-test.yml @@ -42,7 +42,7 @@ jobs: steps: - uses: actions/checkout@v4 - name: Use Node.js ${{ matrix.node-version }} - uses: actions/setup-node@v2 + uses: actions/setup-node@v4 with: node-version: ${{ matrix.node-version }} - name: Add local.aikido.io to /etc/hosts @@ -57,23 +57,3 @@ jobs: env: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} slug: AikidoSec/firewall-node - test-windows: - runs-on: windows-latest - steps: - - uses: actions/checkout@v4 - - name: Use Node.js 20.x - uses: actions/setup-node@v2 - with: - node-version: 20.x - - name: Add local.aikido.io to hosts file - run: | - Add-Content -Path "C:\Windows\System32\drivers\etc\hosts" -Value "127.0.0.1 local.aikido.io" - - run: npm install - - run: npm run test:ci - - name: "Upload coverage" - uses: codecov/codecov-action@v4.0.1 - with: - file: ./library/.tap/report/lcov.info - env: - CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} - slug: AikidoSec/firewall-node diff --git a/library/package.json b/library/package.json index 3e2696de0..c422e7c35 100644 --- a/library/package.json +++ b/library/package.json @@ -85,7 +85,7 @@ }, "scripts": { "test": "node ../scripts/run-tap.js", - "test:ci": "CI=true node ../scripts/run-tap.js", + "test:ci": "node ../scripts/run-tap.js --ci", "build": "tsc", "build:watch": "tsc --watch", "lint": "npm run lint-eslint && npm run lint-tsc", diff --git a/scripts/run-tap.js b/scripts/run-tap.js index cee877b1d..ec6e6442f 100644 --- a/scripts/run-tap.js +++ b/scripts/run-tap.js @@ -4,6 +4,11 @@ const version = process.versions.node.split("."); const major = parseInt(version[0], 10); const minor = parseInt(version[1], 10); +// If script is called with arg --ci, set env CI to true +if (process.argv.includes("--ci")) { + process.env.CI = "true"; +} + let args = "--allow-incomplete-coverage"; if (process.env.CI) { From fb18ee79f717cb9e7e044c868e2a8f7c01f95540 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20K=C3=B6ssler?= Date: Wed, 25 Sep 2024 14:08:42 +0200 Subject: [PATCH 09/13] Disable tests with containers in Windows CI --- .github/workflows/unit-test-windows.yml | 30 --- library/agent/applyHooks.test.ts | 102 ++++---- library/sinks/AwsSDKVersion2.test.ts | 154 +++++------ library/sinks/MongoDB.test.ts | 328 ++++++++++++------------ library/sinks/MySQL.test.ts | 205 ++++++++------- library/sinks/MySQL2.test.ts | 150 ++++++----- library/sinks/Postgres.pool.test.ts | 138 +++++----- library/sinks/Postgres.test.ts | 220 ++++++++-------- 8 files changed, 685 insertions(+), 642 deletions(-) diff --git a/.github/workflows/unit-test-windows.yml b/.github/workflows/unit-test-windows.yml index 01eb4d2cf..9963d0bd3 100644 --- a/.github/workflows/unit-test-windows.yml +++ b/.github/workflows/unit-test-windows.yml @@ -5,36 +5,6 @@ on: jobs: test-windows: runs-on: windows-latest - services: - s3: - image: adobe/s3mock - env: - "initialBuckets": "bucket" - ports: - - "9090:9090" - mongodb: - image: mongo:5 - env: - "MONGO_INITDB_ROOT_USERNAME": "root" - "MONGO_INITDB_ROOT_PASSWORD": "password" - ports: - - 27017:27017 - postgres: - image: postgres:14-alpine - env: - "POSTGRES_PASSWORD": "password" - "POSTGRES_USER": "root" - "POSTGRES_DB": "main_db" - ports: - - "27016:5432" - mysql: - image: mysql:8.0 - # NOTE: use of "mysql_native_password" is not recommended: https://dev.mysql.com/doc/refman/8.0/en/upgrading-from-previous-series.html#upgrade-caching-sha2-password - # We need to use this long command in order to execute the last part : mysql_native_password - # https://stackoverflow.com/questions/60902904/how-to-pass-mysql-native-password-to-mysql-service-in-github-actions - options: --health-cmd="mysqladmin ping" --health-interval=10s --health-timeout=5s --health-retries=5 -e MYSQL_ROOT_PASSWORD=mypassword -e MYSQL_DATABASE=catsdb --entrypoint sh mysql:8.0 -c "exec docker-entrypoint.sh mysqld --default-authentication-plugin=mysql_native_password" - ports: - - "27015:3306" steps: - uses: actions/checkout@v4 - name: Use Node.js 20.x diff --git a/library/agent/applyHooks.test.ts b/library/agent/applyHooks.test.ts index b2b133c5a..c356ee8f2 100644 --- a/library/agent/applyHooks.test.ts +++ b/library/agent/applyHooks.test.ts @@ -6,6 +6,7 @@ import { applyHooks } from "./applyHooks"; import { Context, runWithContext } from "./Context"; import { Hooks } from "./hooks/Hooks"; import { LoggerForTesting } from "./logger/LoggerForTesting"; +import { isWindows } from "../helpers/isWindows"; const context: Context = { remoteAddress: "::1", @@ -123,58 +124,67 @@ function removeStackTraceErrorMessage(error: string) { return msg; } -t.test("it adds try/catch around the wrapped method", async (t) => { - const hooks = new Hooks(); - const connection = hooks - .addPackage("mysql2") - .withVersion("^3.0.0") - .addSubject((exports) => exports.Connection.prototype); - connection.inspect("query", () => { - throw new Error("THIS SHOULD BE CATCHED"); - }); - connection.modifyArguments("execute", () => { - throw new Error("THIS SHOULD BE CATCHED"); - }); - connection.inspectResult("execute", () => { - throw new Error("THIS SHOULD BE CATCHED"); - }); +t.test( + "it adds try/catch around the wrapped method", + { + skip: + isWindows && process.env.CI + ? "CI on Windows does not support containers" + : false, + }, + async (t) => { + const hooks = new Hooks(); + const connection = hooks + .addPackage("mysql2") + .withVersion("^3.0.0") + .addSubject((exports) => exports.Connection.prototype); + connection.inspect("query", () => { + throw new Error("THIS SHOULD BE CATCHED"); + }); + connection.modifyArguments("execute", () => { + throw new Error("THIS SHOULD BE CATCHED"); + }); + connection.inspectResult("execute", () => { + throw new Error("THIS SHOULD BE CATCHED"); + }); - const { agent, logger } = createAgent(); - t.same(applyHooks(hooks, agent), { - mysql2: { - version: "3.11.0", - supported: true, - }, - }); + const { agent, logger } = createAgent(); + t.same(applyHooks(hooks, agent), { + mysql2: { + version: "3.11.0", + supported: true, + }, + }); - const mysql = require("mysql2/promise"); - const actualConnection = await mysql.createConnection({ - host: "localhost", - user: "root", - password: "mypassword", - database: "catsdb", - port: 27015, - multipleStatements: true, - }); + const mysql = require("mysql2/promise"); + const actualConnection = await mysql.createConnection({ + host: "localhost", + user: "root", + password: "mypassword", + database: "catsdb", + port: 27015, + multipleStatements: true, + }); - const [queryRows] = await runWithContext(context, () => - actualConnection.query("SELECT 1 as number") - ); - t.same(queryRows, [{ number: 1 }]); + const [queryRows] = await runWithContext(context, () => + actualConnection.query("SELECT 1 as number") + ); + t.same(queryRows, [{ number: 1 }]); - const [executeRows] = await runWithContext(context, () => - actualConnection.execute("SELECT 1 as number") - ); - t.same(executeRows, [{ number: 1 }]); + const [executeRows] = await runWithContext(context, () => + actualConnection.execute("SELECT 1 as number") + ); + t.same(executeRows, [{ number: 1 }]); - t.same(logger.getMessages().map(removeStackTraceErrorMessage), [ - 'Internal error in module "mysql2" in method "query"', - 'Internal error in module "mysql2" in method "execute"', - 'Internal error in module "mysql2" in method "execute"', - ]); + t.same(logger.getMessages().map(removeStackTraceErrorMessage), [ + 'Internal error in module "mysql2" in method "query"', + 'Internal error in module "mysql2" in method "execute"', + 'Internal error in module "mysql2" in method "execute"', + ]); - await actualConnection.end(); -}); + await actualConnection.end(); + } +); t.test("it hooks into dns module", async (t) => { const seenDomains: string[] = []; diff --git a/library/sinks/AwsSDKVersion2.test.ts b/library/sinks/AwsSDKVersion2.test.ts index 2a9f97cff..cf236cc8a 100644 --- a/library/sinks/AwsSDKVersion2.test.ts +++ b/library/sinks/AwsSDKVersion2.test.ts @@ -4,6 +4,7 @@ import { ReportingAPIForTesting } from "../agent/api/ReportingAPIForTesting"; import { Context, runWithContext } from "../agent/Context"; import { LoggerForTesting } from "../agent/logger/LoggerForTesting"; import { AwsSDKVersion2 } from "./AwsSDKVersion2"; +import { isWindows } from "../helpers/isWindows"; // Suppress upgrade to SDK v3 notice require("aws-sdk/lib/maintenance_mode_message").suppress = true; @@ -25,89 +26,98 @@ const unsafeContext: Context = { route: "/posts/:id", }; -t.test("it works", async (t) => { - const logger = new LoggerForTesting(); - const agent = new Agent( - true, - logger, - new ReportingAPIForTesting(), - undefined, - undefined - ); +t.test( + "it works", + { + skip: + isWindows && process.env.CI + ? "CI on Windows does not support containers" + : false, + }, + async (t) => { + const logger = new LoggerForTesting(); + const agent = new Agent( + true, + logger, + new ReportingAPIForTesting(), + undefined, + undefined + ); - agent.start([new AwsSDKVersion2()]); + agent.start([new AwsSDKVersion2()]); - const AWS = require("aws-sdk"); + const AWS = require("aws-sdk"); - const s3 = new AWS.S3({ - region: "us-east-1", - endpoint: new AWS.Endpoint("http://localhost:9090"), - credentials: { - accessKeyId: "test", - secretAccessKey: "test", - }, - s3ForcePathStyle: true, - }); + const s3 = new AWS.S3({ + region: "us-east-1", + endpoint: new AWS.Endpoint("http://localhost:9090"), + credentials: { + accessKeyId: "test", + secretAccessKey: "test", + }, + s3ForcePathStyle: true, + }); + + async function safeOperation() { + const put = await s3 + .putObject({ + Bucket: "bucket", + Key: "test.txt", + }) + .promise(); - async function safeOperation() { - const put = await s3 - .putObject({ + t.same(put.$response.httpResponse.statusCode, 200); + } + + function safeSignedURL() { + const url = s3.getSignedUrl("getObject", { Bucket: "bucket", Key: "test.txt", - }) - .promise(); + }); - t.same(put.$response.httpResponse.statusCode, 200); - } - - function safeSignedURL() { - const url = s3.getSignedUrl("getObject", { - Bucket: "bucket", - Key: "test.txt", - }); + t.same(url.startsWith("http://localhost:9090/bucket/test.txt"), true); + } - t.same(url.startsWith("http://localhost:9090/bucket/test.txt"), true); - } + async function unsafeOperation() { + await s3 + .putObject({ + Bucket: "bucket", + Key: "test/../test.txt", + }) + .promise(); + } - async function unsafeOperation() { - await s3 - .putObject({ + function unsafeSignedURL() { + s3.getSignedUrl("getObject", { Bucket: "bucket", Key: "test/../test.txt", - }) - .promise(); - } - - function unsafeSignedURL() { - s3.getSignedUrl("getObject", { - Bucket: "bucket", - Key: "test/../test.txt", - }); - } - - // No context - await safeOperation(); - await unsafeOperation(); - safeSignedURL(); - unsafeSignedURL(); - - await runWithContext(unsafeContext, async () => { - await safeOperation(); - const error = await t.rejects(() => unsafeOperation()); - if (error instanceof Error) { - t.same( - error.message, - "Zen has blocked a path traversal attack: S3.putObject(...) originating from body.file.matches" - ); + }); } + // No context + await safeOperation(); + await unsafeOperation(); safeSignedURL(); - const signedURLError = t.throws(() => unsafeSignedURL()); - if (signedURLError instanceof Error) { - t.same( - signedURLError.message, - "Zen has blocked a path traversal attack: S3.getSignedUrl(...) originating from body.file.matches" - ); - } - }); -}); + unsafeSignedURL(); + + await runWithContext(unsafeContext, async () => { + await safeOperation(); + const error = await t.rejects(() => unsafeOperation()); + if (error instanceof Error) { + t.same( + error.message, + "Zen has blocked a path traversal attack: S3.putObject(...) originating from body.file.matches" + ); + } + + safeSignedURL(); + const signedURLError = t.throws(() => unsafeSignedURL()); + if (signedURLError instanceof Error) { + t.same( + signedURLError.message, + "Zen has blocked a path traversal attack: S3.getSignedUrl(...) originating from body.file.matches" + ); + } + }); + } +); diff --git a/library/sinks/MongoDB.test.ts b/library/sinks/MongoDB.test.ts index 77b55ab78..c8151ecfc 100644 --- a/library/sinks/MongoDB.test.ts +++ b/library/sinks/MongoDB.test.ts @@ -4,6 +4,7 @@ import { ReportingAPIForTesting } from "../agent/api/ReportingAPIForTesting"; import { Context, runWithContext } from "../agent/Context"; import { LoggerNoop } from "../agent/logger/LoggerNoop"; import { MongoDB } from "./MongoDB"; +import { isWindows } from "../helpers/isWindows"; const unsafeContext: Context = { remoteAddress: "::1", @@ -35,197 +36,206 @@ const safeContext: Context = { route: "/posts/:id", }; -t.test("it inspects method calls and blocks if needed", async (t) => { - const agent = new Agent( - true, - new LoggerNoop(), - new ReportingAPIForTesting(), - undefined, - "lambda" - ); - agent.start([new MongoDB()]); - - const { MongoClient } = require("mongodb"); - const client = new MongoClient("mongodb://root:password@127.0.0.1:27017"); - await client.connect(); - - try { - const db = client.db("test"); - const collections: { name: string }[] = await db - .listCollections({ name: "test" }) - .toArray(); - if (collections.find((collection) => collection.name === "test")) { - await db.dropCollection("test"); - } +t.test( + "it inspects method calls and blocks if needed", + { + skip: + isWindows && process.env.CI + ? "CI on Windows does not support containers" + : false, + }, + async (t) => { + const agent = new Agent( + true, + new LoggerNoop(), + new ReportingAPIForTesting(), + undefined, + "lambda" + ); + agent.start([new MongoDB()]); + + const { MongoClient } = require("mongodb"); + const client = new MongoClient("mongodb://root:password@127.0.0.1:27017"); + await client.connect(); + + try { + const db = client.db("test"); + const collections: { name: string }[] = await db + .listCollections({ name: "test" }) + .toArray(); + if (collections.find((collection) => collection.name === "test")) { + await db.dropCollection("test"); + } - const collection = db.collection("test"); + const collection = db.collection("test"); - t.same(await collection.count({ title: "Title" }), 0); + t.same(await collection.count({ title: "Title" }), 0); - await collection.insertOne({ - title: "Title", - }); + await collection.insertOne({ + title: "Title", + }); - t.same(await collection.count({ title: "Title" }), 1); + t.same(await collection.count({ title: "Title" }), 1); - const cursor = collection.aggregate([ - { - $group: { - _id: "$title", - count: { $sum: 1 }, + const cursor = collection.aggregate([ + { + $group: { + _id: "$title", + count: { $sum: 1 }, + }, }, - }, - ]); + ]); - t.same(await cursor.toArray(), [{ _id: "Title", count: 1 }]); + t.same(await cursor.toArray(), [{ _id: "Title", count: 1 }]); - t.match( - await collection.findOne({ - title: "Title", - }), - { title: "Title" } - ); + t.match( + await collection.findOne({ + title: "Title", + }), + { title: "Title" } + ); - await collection.updateOne( - { title: "Title" }, - { $set: { title: "New Title" } } - ); + await collection.updateOne( + { title: "Title" }, + { $set: { title: "New Title" } } + ); - await collection.updateMany( - { title: "New Title" }, - { $set: { title: "Another Title" } } - ); + await collection.updateMany( + { title: "New Title" }, + { $set: { title: "Another Title" } } + ); - await collection.replaceOne( - { title: "Another Title" }, - { title: "Yet Another Title" } - ); + await collection.replaceOne( + { title: "Another Title" }, + { title: "Yet Another Title" } + ); - t.same(await collection.count({ title: "Yet Another Title" }), 1); + t.same(await collection.count({ title: "Yet Another Title" }), 1); - await collection.deleteOne({ title: "Yet Another Title" }); + await collection.deleteOne({ title: "Yet Another Title" }); - t.same(await collection.count({ title: "Yet Another Title" }), 0); + t.same(await collection.count({ title: "Yet Another Title" }), 0); - // Bulk write without context - await collection.bulkWrite([ - { - updateMany: { - filter: { someField: "value" }, - update: { $set: { someField: "New Title" } }, + // Bulk write without context + await collection.bulkWrite([ + { + updateMany: { + filter: { someField: "value" }, + update: { $set: { someField: "New Title" } }, + }, }, - }, - ]); + ]); - const bulkError = await t.rejects(async () => { - await runWithContext(unsafeContext, () => { - return collection.bulkWrite([ - { - updateMany: { - filter: { title: { $ne: null } }, - update: { $set: { title: "New Title" } }, + const bulkError = await t.rejects(async () => { + await runWithContext(unsafeContext, () => { + return collection.bulkWrite([ + { + updateMany: { + filter: { title: { $ne: null } }, + update: { $set: { title: "New Title" } }, + }, }, - }, - ]); + ]); + }); }); - }); - if (bulkError instanceof Error) { - t.same( - bulkError.message, - "Zen has blocked a NoSQL injection: MongoDB.Collection.bulkWrite(...) originating from body.myTitle" - ); - } + if (bulkError instanceof Error) { + t.same( + bulkError.message, + "Zen has blocked a NoSQL injection: MongoDB.Collection.bulkWrite(...) originating from body.myTitle" + ); + } - const error = await t.rejects(async () => { - await runWithContext(unsafeContext, () => { - return collection.find({ title: { $ne: null } }).toArray(); + const error = await t.rejects(async () => { + await runWithContext(unsafeContext, () => { + return collection.find({ title: { $ne: null } }).toArray(); + }); }); - }); - if (error instanceof Error) { - t.same( - error.message, - "Zen has blocked a NoSQL injection: MongoDB.Collection.find(...) originating from body.myTitle" - ); - } + if (error instanceof Error) { + t.same( + error.message, + "Zen has blocked a NoSQL injection: MongoDB.Collection.find(...) originating from body.myTitle" + ); + } - const aggregateError = await t.rejects(async () => { - await runWithContext( - { - ...unsafeContext, - body: [ - { - $group: { - _id: "$title", - count: { $sum: 1 }, - }, - }, - ], - }, - () => { - return collection - .aggregate([ + const aggregateError = await t.rejects(async () => { + await runWithContext( + { + ...unsafeContext, + body: [ { $group: { _id: "$title", count: { $sum: 1 }, }, }, - ]) - .toArray(); - } - ); - }); + ], + }, + () => { + return collection + .aggregate([ + { + $group: { + _id: "$title", + count: { $sum: 1 }, + }, + }, + ]) + .toArray(); + } + ); + }); - if (aggregateError instanceof Error) { - t.same( - aggregateError.message, - "Zen has blocked a NoSQL injection: MongoDB.Collection.aggregate(...) originating from body.[0]" - ); - } + if (aggregateError instanceof Error) { + t.same( + aggregateError.message, + "Zen has blocked a NoSQL injection: MongoDB.Collection.aggregate(...) originating from body.[0]" + ); + } - // Test if it checks arguments - await runWithContext(safeContext, async () => { - await t.rejects(async () => collection.bulkWrite()); - await t.rejects(async () => collection.bulkWrite(1)); - await t.rejects(async () => collection.bulkWrite([1])); - await t.rejects(async () => collection.bulkWrite([])); - await t.rejects(async () => collection.bulkWrite([{}])); - await t.rejects(async () => - collection.bulkWrite([ - { - updateOne: { - // Structure similar to what MongoDB expects, but without 'filter' - update: { $set: { a: 1 } }, + // Test if it checks arguments + await runWithContext(safeContext, async () => { + await t.rejects(async () => collection.bulkWrite()); + await t.rejects(async () => collection.bulkWrite(1)); + await t.rejects(async () => collection.bulkWrite([1])); + await t.rejects(async () => collection.bulkWrite([])); + await t.rejects(async () => collection.bulkWrite([{}])); + await t.rejects(async () => + collection.bulkWrite([ + { + updateOne: { + // Structure similar to what MongoDB expects, but without 'filter' + update: { $set: { a: 1 } }, + }, }, - }, - ]) + ]) + ); + await t.rejects(() => collection.updateOne()); + await t.rejects(() => collection.updateOne(1)); + }); + + await runWithContext( + { + remoteAddress: "::1", + method: "POST", + url: "http://localhost:4000", + query: {}, + headers: {}, + body: {}, + cookies: {}, + source: "express", + route: "/posts/:id", + routeParams: {}, + }, + () => { + return collection.find({ title: { $ne: null } }).toArray(); + } ); - await t.rejects(() => collection.updateOne()); - await t.rejects(() => collection.updateOne(1)); - }); - - await runWithContext( - { - remoteAddress: "::1", - method: "POST", - url: "http://localhost:4000", - query: {}, - headers: {}, - body: {}, - cookies: {}, - source: "express", - route: "/posts/:id", - routeParams: {}, - }, - () => { - return collection.find({ title: { $ne: null } }).toArray(); - } - ); - } catch (error: any) { - t.fail(error.message); - } finally { - await client.close(); + } catch (error: any) { + t.fail(error.message); + } finally { + await client.close(); + } } -}); +); diff --git a/library/sinks/MySQL.test.ts b/library/sinks/MySQL.test.ts index 654a4d4ef..c96b8de33 100644 --- a/library/sinks/MySQL.test.ts +++ b/library/sinks/MySQL.test.ts @@ -5,6 +5,7 @@ import { getContext, runWithContext, type Context } from "../agent/Context"; import { LoggerNoop } from "../agent/logger/LoggerNoop"; import { MySQL } from "./MySQL"; import type { Connection } from "mysql"; +import { isWindows } from "../helpers/isWindows"; function query(sql: string, connection: Connection) { return new Promise((resolve, reject) => { @@ -45,119 +46,131 @@ const context: Context = { route: "/posts/:id", }; -t.test("it detects SQL injections", async () => { - const agent = new Agent( - true, - new LoggerNoop(), - new ReportingAPIForTesting(), - undefined, - "lambda" - ); - agent.start([new MySQL()]); - - const mysql = require("mysql"); - const connection = await mysql.createConnection({ - host: "localhost", - user: "root", - password: "mypassword", - database: "catsdb", - port: 27015, - multipleStatements: true, - }); +t.test( + "it detects SQL injections", + { + skip: + isWindows && process.env.CI + ? "CI on Windows does not support containers" + : false, + }, + async () => { + const agent = new Agent( + true, + new LoggerNoop(), + new ReportingAPIForTesting(), + undefined, + "lambda" + ); + agent.start([new MySQL()]); + + const mysql = require("mysql"); + const connection = await mysql.createConnection({ + host: "localhost", + user: "root", + password: "mypassword", + database: "catsdb", + port: 27015, + multipleStatements: true, + }); - try { - await query( - ` + try { + await query( + ` CREATE TABLE IF NOT EXISTS cats ( petname varchar(255) ); `, - connection - ); - await query("TRUNCATE cats", connection); - t.same(await query("SELECT petname FROM `cats`;", connection), []); - t.same( - await queryViaOptions({ sql: "SELECT petname FROM `cats`;" }, connection), - [] - ); - t.same( - await runWithContext(context, () => { - return query("SELECT petname FROM `cats`;", connection); - }), - [] - ); - t.same( - await runWithContext(context, () => { - return queryViaOptions( + connection + ); + await query("TRUNCATE cats", connection); + t.same(await query("SELECT petname FROM `cats`;", connection), []); + t.same( + await queryViaOptions( { sql: "SELECT petname FROM `cats`;" }, connection - ); - }), - [] - ); - - const error = await t.rejects(async () => { - await runWithContext(context, () => { - return connection.query("-- should be blocked"); - }); - }); - - if (error instanceof Error) { + ), + [] + ); t.same( - error.message, - "Zen has blocked an SQL injection: MySQL.query(...) originating from body.myTitle" + await runWithContext(context, () => { + return query("SELECT petname FROM `cats`;", connection); + }), + [] + ); + t.same( + await runWithContext(context, () => { + return queryViaOptions( + { sql: "SELECT petname FROM `cats`;" }, + connection + ); + }), + [] ); - } - const error2 = await t.rejects(async () => { - await runWithContext(context, () => { - return connection.query({ sql: "-- should be blocked" }); + const error = await t.rejects(async () => { + await runWithContext(context, () => { + return connection.query("-- should be blocked"); + }); }); - }); - if (error2 instanceof Error) { - t.same( - error2.message, - "Zen has blocked an SQL injection: MySQL.query(...) originating from body.myTitle" - ); - } + if (error instanceof Error) { + t.same( + error.message, + "Zen has blocked an SQL injection: MySQL.query(...) originating from body.myTitle" + ); + } - const undefinedQueryError = await t.rejects(async () => { - await runWithContext(context, () => { - return query(undefined, connection); + const error2 = await t.rejects(async () => { + await runWithContext(context, () => { + return connection.query({ sql: "-- should be blocked" }); + }); }); - }); - if (undefinedQueryError instanceof Error) { - t.same(undefinedQueryError.message, "ER_EMPTY_QUERY: Query was empty"); - } + if (error2 instanceof Error) { + t.same( + error2.message, + "Zen has blocked an SQL injection: MySQL.query(...) originating from body.myTitle" + ); + } - await runWithContext( - { - remoteAddress: "::1", - method: "POST", - url: "http://localhost:4000/", - query: {}, - headers: {}, - body: {}, - cookies: {}, - source: "express", - route: "/posts/:id", - routeParams: {}, - }, - () => { - return connection.query("-- This is a comment"); + const undefinedQueryError = await t.rejects(async () => { + await runWithContext(context, () => { + return query(undefined, connection); + }); + }); + + if (undefinedQueryError instanceof Error) { + t.same(undefinedQueryError.message, "ER_EMPTY_QUERY: Query was empty"); } - ); - runWithContext(context, () => { - connection.query("SELECT petname FROM `cats`;", (error, results) => { - t.same(getContext(), context); + await runWithContext( + { + remoteAddress: "::1", + method: "POST", + url: "http://localhost:4000/", + query: {}, + headers: {}, + body: {}, + cookies: {}, + source: "express", + route: "/posts/:id", + routeParams: {}, + }, + () => { + return connection.query("-- This is a comment"); + } + ); + + runWithContext(context, () => { + connection.query("SELECT petname FROM `cats`;", (error, results) => { + t.same(getContext(), context); + }); }); - }); - } catch (error: any) { - t.fail(error); - } finally { - await connection.end(); + } catch (error: any) { + t.fail(error); + } finally { + await connection.end(); + } } -}); +); diff --git a/library/sinks/MySQL2.test.ts b/library/sinks/MySQL2.test.ts index a2604aa8a..413609da1 100644 --- a/library/sinks/MySQL2.test.ts +++ b/library/sinks/MySQL2.test.ts @@ -4,6 +4,7 @@ import { ReportingAPIForTesting } from "../agent/api/ReportingAPIForTesting"; import { runWithContext, type Context } from "../agent/Context"; import { LoggerNoop } from "../agent/logger/LoggerNoop"; import { MySQL2 } from "./MySQL2"; +import { isWindows } from "../helpers/isWindows"; const dangerousContext: Context = { remoteAddress: "::1", @@ -33,90 +34,99 @@ const safeContext: Context = { route: "/posts/:id", }; -t.test("it detects SQL injections", async () => { - const agent = new Agent( - true, - new LoggerNoop(), - new ReportingAPIForTesting(), - undefined, - "lambda" - ); - agent.start([new MySQL2()]); +t.test( + "it detects SQL injections", + { + skip: + isWindows && process.env.CI + ? "CI on Windows does not support containers" + : false, + }, + async () => { + const agent = new Agent( + true, + new LoggerNoop(), + new ReportingAPIForTesting(), + undefined, + "lambda" + ); + agent.start([new MySQL2()]); - const mysql = require("mysql2/promise"); + const mysql = require("mysql2/promise"); - const connection = await mysql.createConnection({ - host: "localhost", - user: "root", - password: "mypassword", - database: "catsdb", - port: 27015, - multipleStatements: true, - }); + const connection = await mysql.createConnection({ + host: "localhost", + user: "root", + password: "mypassword", + database: "catsdb", + port: 27015, + multipleStatements: true, + }); - try { - await connection.query( - ` + try { + await connection.query( + ` CREATE TABLE IF NOT EXISTS cats ( petname varchar(255) ); ` - ); - await connection.execute("TRUNCATE cats"); - const [rows] = await connection.query("SELECT petname FROM `cats`;"); - t.same(rows, []); - const [moreRows] = await connection.query({ - sql: "SELECT petname FROM `cats`", - }); - t.same(moreRows, []); + ); + await connection.execute("TRUNCATE cats"); + const [rows] = await connection.query("SELECT petname FROM `cats`;"); + t.same(rows, []); + const [moreRows] = await connection.query({ + sql: "SELECT petname FROM `cats`", + }); + t.same(moreRows, []); - const error = await t.rejects(async () => { - await runWithContext(dangerousContext, () => { - return connection.query("-- should be blocked"); + const error = await t.rejects(async () => { + await runWithContext(dangerousContext, () => { + return connection.query("-- should be blocked"); + }); }); - }); - if (error instanceof Error) { - t.same( - error.message, - "Zen has blocked an SQL injection: mysql2.query(...) originating from body.myTitle" - ); - } + if (error instanceof Error) { + t.same( + error.message, + "Zen has blocked an SQL injection: mysql2.query(...) originating from body.myTitle" + ); + } - const error2 = await t.rejects(async () => { - await runWithContext(dangerousContext, () => { - return connection.query({ sql: "-- should be blocked" }); + const error2 = await t.rejects(async () => { + await runWithContext(dangerousContext, () => { + return connection.query({ sql: "-- should be blocked" }); + }); }); - }); - if (error2 instanceof Error) { - t.same( - error2.message, - "Zen has blocked an SQL injection: mysql2.query(...) originating from body.myTitle" - ); - } + if (error2 instanceof Error) { + t.same( + error2.message, + "Zen has blocked an SQL injection: mysql2.query(...) originating from body.myTitle" + ); + } - const undefinedQueryError = await t.rejects(async () => { - await runWithContext(dangerousContext, () => { - return connection.query(undefined); + const undefinedQueryError = await t.rejects(async () => { + await runWithContext(dangerousContext, () => { + return connection.query(undefined); + }); }); - }); - if (undefinedQueryError instanceof Error) { - t.same( - undefinedQueryError.message, - "Cannot read properties of undefined (reading 'constructor')" - ); - } + if (undefinedQueryError instanceof Error) { + t.same( + undefinedQueryError.message, + "Cannot read properties of undefined (reading 'constructor')" + ); + } - await runWithContext(safeContext, () => { - return connection.query("-- This is a comment"); - }); + await runWithContext(safeContext, () => { + return connection.query("-- This is a comment"); + }); - await runWithContext(safeContext, () => { - return connection.execute("SELECT 1"); - }); - } catch (error: any) { - t.fail(error); - } finally { - await connection.end(); + await runWithContext(safeContext, () => { + return connection.execute("SELECT 1"); + }); + } catch (error: any) { + t.fail(error); + } finally { + await connection.end(); + } } -}); +); diff --git a/library/sinks/Postgres.pool.test.ts b/library/sinks/Postgres.pool.test.ts index f5e9310f7..072b723f0 100644 --- a/library/sinks/Postgres.pool.test.ts +++ b/library/sinks/Postgres.pool.test.ts @@ -4,6 +4,7 @@ import { ReportingAPIForTesting } from "../agent/api/ReportingAPIForTesting"; import { runWithContext, type Context } from "../agent/Context"; import { LoggerNoop } from "../agent/logger/LoggerNoop"; import { Postgres } from "./Postgres"; +import { isWindows } from "../helpers/isWindows"; const context: Context = { remoteAddress: "::1", @@ -20,81 +21,90 @@ const context: Context = { route: "/posts/:id", }; -t.test("it detects SQL injections", async () => { - const agent = new Agent( - true, - new LoggerNoop(), - new ReportingAPIForTesting(), - undefined, - "lambda" - ); - agent.start([new Postgres()]); +t.test( + "it detects SQL injections", + { + skip: + isWindows && process.env.CI + ? "CI on Windows does not support containers" + : false, + }, + async () => { + const agent = new Agent( + true, + new LoggerNoop(), + new ReportingAPIForTesting(), + undefined, + "lambda" + ); + agent.start([new Postgres()]); - const { Pool } = require("pg"); - const pool = new Pool({ - user: "root", - host: "127.0.0.1", - database: "main_db", - password: "password", - port: 27016, - }); + const { Pool } = require("pg"); + const pool = new Pool({ + user: "root", + host: "127.0.0.1", + database: "main_db", + password: "password", + port: 27016, + }); - const client = await pool.connect(); + const client = await pool.connect(); - try { - await client.query(` + try { + await client.query(` CREATE TABLE IF NOT EXISTS cats ( petname varchar(255) ); `); - await client.query("TRUNCATE cats"); - t.same((await client.query("SELECT petname FROM cats;")).rows, []); + await client.query("TRUNCATE cats"); + t.same((await client.query("SELECT petname FROM cats;")).rows, []); - const error = await t.rejects(async () => { - await runWithContext(context, () => { - return client.query("-- should be blocked"); + const error = await t.rejects(async () => { + await runWithContext(context, () => { + return client.query("-- should be blocked"); + }); }); - }); - if (error instanceof Error) { - t.same( - error.message, - "Zen has blocked an SQL injection: pg.query(...) originating from body.myTitle" - ); - } + if (error instanceof Error) { + t.same( + error.message, + "Zen has blocked an SQL injection: pg.query(...) originating from body.myTitle" + ); + } - const undefinedQueryError = await t.rejects(async () => { - await runWithContext(context, () => { - return client.query(null); + const undefinedQueryError = await t.rejects(async () => { + await runWithContext(context, () => { + return client.query(null); + }); }); - }); - if (undefinedQueryError instanceof Error) { - t.same( - undefinedQueryError.message, - "Client was passed a null or undefined query" + if (undefinedQueryError instanceof Error) { + t.same( + undefinedQueryError.message, + "Client was passed a null or undefined query" + ); + } + + await runWithContext( + { + remoteAddress: "::1", + method: "POST", + url: "http://localhost:4000/", + query: {}, + headers: {}, + body: {}, + cookies: {}, + source: "express", + route: "/posts/:id", + routeParams: {}, + }, + () => { + return client.query("-- This is a comment"); + } ); + } catch (error: any) { + t.fail(error); + } finally { + client.release(); + await pool.end(); } - - await runWithContext( - { - remoteAddress: "::1", - method: "POST", - url: "http://localhost:4000/", - query: {}, - headers: {}, - body: {}, - cookies: {}, - source: "express", - route: "/posts/:id", - routeParams: {}, - }, - () => { - return client.query("-- This is a comment"); - } - ); - } catch (error: any) { - t.fail(error); - } finally { - client.release(); - await pool.end(); } -}); +); diff --git a/library/sinks/Postgres.test.ts b/library/sinks/Postgres.test.ts index cf269d66f..9c6133f38 100644 --- a/library/sinks/Postgres.test.ts +++ b/library/sinks/Postgres.test.ts @@ -4,6 +4,7 @@ import { ReportingAPIForTesting } from "../agent/api/ReportingAPIForTesting"; import { getContext, runWithContext, type Context } from "../agent/Context"; import { LoggerNoop } from "../agent/logger/LoggerNoop"; import { Postgres } from "./Postgres"; +import { isWindows } from "../helpers/isWindows"; const context: Context = { remoteAddress: "::1", @@ -20,128 +21,137 @@ const context: Context = { route: "/posts/:id", }; -t.test("it inspects query method calls and blocks if needed", async (t) => { - const agent = new Agent( - true, - new LoggerNoop(), - new ReportingAPIForTesting(), - undefined, - "lambda" - ); - agent.start([new Postgres()]); +t.test( + "it inspects query method calls and blocks if needed", + { + skip: + isWindows && process.env.CI + ? "CI on Windows does not support containers" + : false, + }, + async (t) => { + const agent = new Agent( + true, + new LoggerNoop(), + new ReportingAPIForTesting(), + undefined, + "lambda" + ); + agent.start([new Postgres()]); - const { Client } = require("pg"); - const client = new Client({ - user: "root", - host: "127.0.0.1", - database: "main_db", - password: "password", - port: 27016, - }); - await client.connect(); + const { Client } = require("pg"); + const client = new Client({ + user: "root", + host: "127.0.0.1", + database: "main_db", + password: "password", + port: 27016, + }); + await client.connect(); - try { - await client.query(` + try { + await client.query(` CREATE TABLE IF NOT EXISTS cats ( petname varchar(255) ); `); - await client.query("TRUNCATE cats"); - - t.same((await client.query("SELECT petname FROM cats;")).rows, []); - t.same( - (await client.query({ text: "SELECT petname FROM cats;" })).rows, - [] - ); - t.same( - ( - await runWithContext(context, () => { - return client.query("SELECT petname FROM cats;"); - }) - ).rows, - [] - ); - t.same( - ( - await runWithContext(context, () => { - return client.query({ text: "SELECT petname FROM cats;" }); - }) - ).rows, - [] - ); + await client.query("TRUNCATE cats"); - const error = await t.rejects(async () => { - await runWithContext(context, () => { - return client.query("-- should be blocked"); - }); - }); - if (error instanceof Error) { + t.same((await client.query("SELECT petname FROM cats;")).rows, []); t.same( - error.message, - "Zen has blocked an SQL injection: pg.query(...) originating from body.myTitle" + (await client.query({ text: "SELECT petname FROM cats;" })).rows, + [] ); - } - - const error2 = await t.rejects(async () => { - await runWithContext(context, () => { - return client.query({ text: "-- should be blocked" }); - }); - }); - if (error2 instanceof Error) { t.same( - error2.message, - "Zen has blocked an SQL injection: pg.query(...) originating from body.myTitle" + ( + await runWithContext(context, () => { + return client.query("SELECT petname FROM cats;"); + }) + ).rows, + [] ); - } - - const undefinedQueryError = await t.rejects(async () => { - await runWithContext(context, () => { - return client.query(null); - }); - }); - if (undefinedQueryError instanceof Error) { t.same( - undefinedQueryError.message, - "Client was passed a null or undefined query" + ( + await runWithContext(context, () => { + return client.query({ text: "SELECT petname FROM cats;" }); + }) + ).rows, + [] ); - } - await runWithContext( - { - remoteAddress: "::1", - method: "POST", - url: "http://localhost:4000/", - query: {}, - headers: {}, - body: {}, - cookies: {}, - source: "express", - route: "/posts/:id", - routeParams: {}, - }, - () => { - return client.query("-- This is a comment"); + const error = await t.rejects(async () => { + await runWithContext(context, () => { + return client.query("-- should be blocked"); + }); + }); + if (error instanceof Error) { + t.same( + error.message, + "Zen has blocked an SQL injection: pg.query(...) originating from body.myTitle" + ); } - ); - // Check if context is available in the callback - runWithContext(context, () => { - client.query("SELECT petname FROM cats;", (error, result) => { - t.match(getContext(), context); + const error2 = await t.rejects(async () => { + await runWithContext(context, () => { + return client.query({ text: "-- should be blocked" }); + }); + }); + if (error2 instanceof Error) { + t.same( + error2.message, + "Zen has blocked an SQL injection: pg.query(...) originating from body.myTitle" + ); + } - try { - client.query("-- should be blocked", () => {}); - } catch (error: any) { - t.match( - error.message, - /Zen has blocked an SQL injection: pg.query\(\.\.\.\) originating from body\.myTitle/ - ); + const undefinedQueryError = await t.rejects(async () => { + await runWithContext(context, () => { + return client.query(null); + }); + }); + if (undefinedQueryError instanceof Error) { + t.same( + undefinedQueryError.message, + "Client was passed a null or undefined query" + ); + } + + await runWithContext( + { + remoteAddress: "::1", + method: "POST", + url: "http://localhost:4000/", + query: {}, + headers: {}, + body: {}, + cookies: {}, + source: "express", + route: "/posts/:id", + routeParams: {}, + }, + () => { + return client.query("-- This is a comment"); } + ); + + // Check if context is available in the callback + runWithContext(context, () => { + client.query("SELECT petname FROM cats;", (error, result) => { + t.match(getContext(), context); + + try { + client.query("-- should be blocked", () => {}); + } catch (error: any) { + t.match( + error.message, + /Zen has blocked an SQL injection: pg.query\(\.\.\.\) originating from body\.myTitle/ + ); + } + }); }); - }); - } catch (error: any) { - t.fail(error); - } finally { - await client.end(); + } catch (error: any) { + t.fail(error); + } finally { + await client.end(); + } } -}); +); From 0a755f0d3da39df0859d8708a56c1375641521b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20K=C3=B6ssler?= Date: Wed, 25 Sep 2024 15:35:20 +0200 Subject: [PATCH 10/13] Fix additional tests with paths --- library/sinks/FileSystem.test.ts | 30 +++++++++++++++++------------ library/sinks/Path.test.ts | 25 +++++++++++++++--------- library/sinks/SQLite3.test.ts | 21 +++++++++++--------- library/sources/HTTP2Server.test.ts | 5 ++++- 4 files changed, 50 insertions(+), 31 deletions(-) diff --git a/library/sinks/FileSystem.test.ts b/library/sinks/FileSystem.test.ts index 1486bad33..1c939b110 100644 --- a/library/sinks/FileSystem.test.ts +++ b/library/sinks/FileSystem.test.ts @@ -217,18 +217,24 @@ t.test("it works", async (t) => { ); }); - runWithContext(unsafeContextAbsolute, () => { - throws( - () => - rename(new URL("file:///etc/passwd"), "../test123.txt", (err) => {}), - "Zen has blocked a path traversal attack: fs.rename(...) originating from body.file.matches" - ); - throws( - () => - rename(new URL("file:///../etc/passwd"), "../test123.txt", (err) => {}), - "Zen has blocked a path traversal attack: fs.rename(...) originating from body.file.matches" - ); - }); + if (!isWindows) { + runWithContext(unsafeContextAbsolute, () => { + throws( + () => + rename(new URL("file:///etc/passwd"), "../test123.txt", (err) => {}), + "Zen has blocked a path traversal attack: fs.rename(...) originating from body.file.matches" + ); + throws( + () => + rename( + new URL("file:///../etc/passwd"), + "../test123.txt", + (err) => {} + ), + "Zen has blocked a path traversal attack: fs.rename(...) originating from body.file.matches" + ); + }); + } // Ignores malformed URLs runWithContext( diff --git a/library/sinks/Path.test.ts b/library/sinks/Path.test.ts index fdd3f2c7a..953f8fb3b 100644 --- a/library/sinks/Path.test.ts +++ b/library/sinks/Path.test.ts @@ -99,14 +99,21 @@ t.test("it works", async (t) => { }); runWithContext(unsafeAbsoluteContext, () => { - t.throws( - () => join("/etc/", "test.txt"), - "Zen has blocked a Path traversal: fs.join(...) originating from body.file.matches" - ); - - t.throws( - () => resolve("/etc/some_directory", "test.txt"), - "Zen has blocked a Path traversal: fs.resolve(...) originating from body.file.matches" - ); + if (!isWindows) { + t.throws( + () => join("/etc/", "test.txt"), + "Zen has blocked a Path traversal: fs.join(...) originating from body.file.matches" + ); + + t.throws( + () => resolve("/etc/some_directory", "test.txt"), + "Zen has blocked a Path traversal: fs.resolve(...) originating from body.file.matches" + ); + } else { + t.throws( + () => join("C:/etc/", "test.txt"), + "Zen has blocked a Path traversal: fs.join(...) originating from body.file.matches" + ); + } }); }); diff --git a/library/sinks/SQLite3.test.ts b/library/sinks/SQLite3.test.ts index 79b53685e..152a8cf2e 100644 --- a/library/sinks/SQLite3.test.ts +++ b/library/sinks/SQLite3.test.ts @@ -5,6 +5,7 @@ import { runWithContext, type Context } from "../agent/Context"; import { LoggerNoop } from "../agent/logger/LoggerNoop"; import { SQLite3 } from "./SQLite3"; import { promisify } from "util"; +import { isWindows } from "../helpers/isWindows"; const dangerousContext: Context = { remoteAddress: "::1", @@ -102,16 +103,18 @@ t.test("it detects SQL injections", async () => { return all("SELECT 1"); }); - const error3 = await t.rejects(async () => { - await runWithContext(dangerousPathContext, () => { - return backup("/tmp/insecure"); + if (!isWindows) { + const error3 = await t.rejects(async () => { + await runWithContext(dangerousPathContext, () => { + return backup("/tmp/insecure"); + }); }); - }); - if (error3 instanceof Error) { - t.same( - error3.message, - "Zen has blocked a path traversal attack: sqlite3.backup(...) originating from body.myTitle" - ); + if (error3 instanceof Error) { + t.same( + error3.message, + "Zen has blocked a path traversal attack: sqlite3.backup(...) originating from body.myTitle" + ); + } } } catch (error: any) { t.fail(error); diff --git a/library/sources/HTTP2Server.test.ts b/library/sources/HTTP2Server.test.ts index 202eb82ae..0060a7353 100644 --- a/library/sources/HTTP2Server.test.ts +++ b/library/sources/HTTP2Server.test.ts @@ -12,6 +12,7 @@ import * as pkg from "../helpers/isPackageInstalled"; import { readFileSync } from "fs"; import { resolve } from "path"; import { FileSystem } from "../sinks/FileSystem"; +import { isWindows } from "../helpers/isWindows"; const originalIsPackageInstalled = pkg.isPackageInstalled; wrap(pkg, "isPackageInstalled", function wrap() { @@ -651,10 +652,12 @@ t.test("real injection test", async (t) => { } }); + const path = isWindows ? "C:\\Windows\\win.ini" : "/etc/passwd"; + await new Promise((resolve) => { server.listen(3431, () => { http2Request( - new URL("http://localhost:3431?path=/etc/passwd"), + new URL(`http://localhost:3431?path=${path}`), "GET", {} ).then(({ body }) => { From 84ea85bf9db19f28c8b5dab3a552fa65ffe503d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20K=C3=B6ssler?= Date: Thu, 26 Sep 2024 10:34:18 +0200 Subject: [PATCH 11/13] Fix unsafe path start on Windows --- library/sinks/FileSystem.test.ts | 10 +++---- library/sinks/Path.test.ts | 30 ++++++++++++------- .../detectPathTraversal.test.ts | 2 +- .../path-traversal/unsafePathStart.ts | 30 ++++++++++++++----- 4 files changed, 48 insertions(+), 24 deletions(-) diff --git a/library/sinks/FileSystem.test.ts b/library/sinks/FileSystem.test.ts index 1c939b110..187c913e5 100644 --- a/library/sinks/FileSystem.test.ts +++ b/library/sinks/FileSystem.test.ts @@ -100,7 +100,7 @@ t.test("it works", async (t) => { if (!isWindows) { rename(new URL("file:///test123.txt"), "test2.txt", (err) => {}); } else { - rename(new URL("file:///c:/test123.txt"), "test2.txt", (err) => {}); + rename(new URL("file:///x:/test123.txt"), "test2.txt", (err) => {}); } rename(Buffer.from("./test123.txt"), "test2.txt", (err) => {}); }; @@ -184,7 +184,7 @@ t.test("it works", async (t) => { throws( () => rename( - new URL("file:///C:/../test.txt"), + new URL("file:///x:/../test.txt"), "../test2.txt", (err) => {} ), @@ -194,7 +194,7 @@ t.test("it works", async (t) => { throws( () => rename( - new URL("file:///C:/./../test.txt"), + new URL("file:///x:/./../test.txt"), "../test2.txt", (err) => {} ), @@ -204,7 +204,7 @@ t.test("it works", async (t) => { throws( () => rename( - new URL("file:///C:/../../test.txt"), + new URL("file:///X:/../../test.txt"), "../test2.txt", (err) => {} ), @@ -244,7 +244,7 @@ t.test("it works", async (t) => { rename(new URL("file:///../../test.txt"), "../test2.txt", (err) => {}); } else { rename( - new URL("file:/C://../../test.txt"), + new URL("file:/x://../../test.txt"), "../test2.txt", (err) => {} ); diff --git a/library/sinks/Path.test.ts b/library/sinks/Path.test.ts index 953f8fb3b..b028d7ff3 100644 --- a/library/sinks/Path.test.ts +++ b/library/sinks/Path.test.ts @@ -47,7 +47,7 @@ t.test("it works", async (t) => { if (!isWindows) { t.same(join("/app", "/etc/data"), resolve("/app/etc/data")); } else { - t.same(join("c:/app", "/etc/data"), resolve("c:/app/etc/data")); + t.same(join("x:/app", "/etc/data"), resolve("x:/app/etc/data")); } } @@ -98,8 +98,8 @@ t.test("it works", async (t) => { safeCalls(); }); - runWithContext(unsafeAbsoluteContext, () => { - if (!isWindows) { + if (!isWindows) { + runWithContext(unsafeAbsoluteContext, () => { t.throws( () => join("/etc/", "test.txt"), "Zen has blocked a Path traversal: fs.join(...) originating from body.file.matches" @@ -109,11 +109,21 @@ t.test("it works", async (t) => { () => resolve("/etc/some_directory", "test.txt"), "Zen has blocked a Path traversal: fs.resolve(...) originating from body.file.matches" ); - } else { - t.throws( - () => join("C:/etc/", "test.txt"), - "Zen has blocked a Path traversal: fs.join(...) originating from body.file.matches" - ); - } - }); + }); + } else { + runWithContext( + { + ...unsafeAbsoluteContext, + ...{ + body: { file: { matches: "X:/etc/" } }, + }, + }, + () => { + t.throws( + () => resolve("X:/etc/", "test.txt"), + "Zen has blocked a Path traversal: fs.join(...) originating from body.file.matches" + ); + } + ); + } }); diff --git a/library/vulnerabilities/path-traversal/detectPathTraversal.test.ts b/library/vulnerabilities/path-traversal/detectPathTraversal.test.ts index bcae388bf..46ccac58c 100644 --- a/library/vulnerabilities/path-traversal/detectPathTraversal.test.ts +++ b/library/vulnerabilities/path-traversal/detectPathTraversal.test.ts @@ -126,6 +126,6 @@ t.test( "windows drive letter", { skip: process.platform !== "win32" ? "Windows only" : false }, async () => { - t.same(detectPathTraversal("C:\\file.txt", "C:\\"), true); + t.same(detectPathTraversal("X:\\file.txt", "X:\\"), true); } ); diff --git a/library/vulnerabilities/path-traversal/unsafePathStart.ts b/library/vulnerabilities/path-traversal/unsafePathStart.ts index da0ac68a6..ffc7e28d6 100644 --- a/library/vulnerabilities/path-traversal/unsafePathStart.ts +++ b/library/vulnerabilities/path-traversal/unsafePathStart.ts @@ -22,7 +22,6 @@ const linuxRootFolders = [ "/usr/", "/var/", ]; -const dangerousPathStarts = [...linuxRootFolders, "c:/", "c:\\"]; export function startsWithUnsafePath(filePath: string, userInput: string) { // Check if path is relative (not absolute or drive letter path) @@ -38,13 +37,28 @@ export function startsWithUnsafePath(filePath: string, userInput: string) { const normalizedPath = origResolve(filePath).toLowerCase(); const normalizedUserInput = origResolve(userInput).toLowerCase(); - for (const dangerousStart of dangerousPathStarts) { - if ( - normalizedPath.startsWith(dangerousStart) && - normalizedPath.startsWith(normalizedUserInput) - ) { - return true; - } + + return isDangerous(normalizedPath, normalizedUserInput); +} + +function isDangerous(normalizedPath: string, normalizedUserInput: string) { + // Check if normalizedPath starts with normalizedUserInput + if (!normalizedPath.startsWith(normalizedUserInput)) { + return false; + } + + const foundLinuxRoot = linuxRootFolders.find((folder) => + normalizedPath.startsWith(folder) + ); + + if (foundLinuxRoot) { + return true; } + + // Check for windows drive letter + if (/^[a-z]:(\\|\/)/i.test(normalizedPath)) { + return true; + } + return false; } From 663a92cae4b39342ff801ceed9fa754ab7f61fbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20K=C3=B6ssler?= Date: Thu, 26 Sep 2024 11:27:09 +0200 Subject: [PATCH 12/13] Fix file: bypass --- library/sinks/FileSystem.test.ts | 18 ++++++++++++++++++ .../path-traversal/detectPathTraversal.ts | 10 ++++++---- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/library/sinks/FileSystem.test.ts b/library/sinks/FileSystem.test.ts index 187c913e5..d1d4b4812 100644 --- a/library/sinks/FileSystem.test.ts +++ b/library/sinks/FileSystem.test.ts @@ -234,6 +234,24 @@ t.test("it works", async (t) => { "Zen has blocked a path traversal attack: fs.rename(...) originating from body.file.matches" ); }); + } else { + runWithContext( + { + ...unsafeContextAbsolute, + body: { file: { matches: "file:///X:/Windows/System32/" } }, + }, + () => { + throws( + () => + rename( + new URL("file:///X:/Windows/System32/"), + "../test", + (err) => {} + ), + "Zen has blocked a path traversal attack: fs.rename(...) originating from body.file.matches" + ); + } + ); } // Ignores malformed URLs diff --git a/library/vulnerabilities/path-traversal/detectPathTraversal.ts b/library/vulnerabilities/path-traversal/detectPathTraversal.ts index ebc7df2ec..c27269a57 100644 --- a/library/vulnerabilities/path-traversal/detectPathTraversal.ts +++ b/library/vulnerabilities/path-traversal/detectPathTraversal.ts @@ -19,10 +19,12 @@ export function detectPathTraversal( // The normal check for relative path traversal will fail in this case, because transformed path does not contain ../. // For absolute path traversal, we dont need to check the transformed path, because it will always start with /. // Also /./ is checked by normal absolute path traversal check (if #219 is merged) - if (isUrl && containsUnsafePathParts(userInput)) { - const filePathFromUrl = parseAsFileUrl(userInput); - if (filePathFromUrl && filePath.includes(filePathFromUrl)) { - return true; + if (isUrl) { + if (containsUnsafePathParts(userInput) || userInput.startsWith("file:")) { + const filePathFromUrl = parseAsFileUrl(userInput); + if (filePathFromUrl && filePath.includes(filePathFromUrl)) { + return true; + } } } From 43c3cafbd9a6e72de0f43bd4993cd33282cd7330 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20K=C3=B6ssler?= Date: Thu, 26 Sep 2024 11:50:20 +0200 Subject: [PATCH 13/13] Fix unit test after merge --- library/sinks/Shelljs.test.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/library/sinks/Shelljs.test.ts b/library/sinks/Shelljs.test.ts index e8902924f..b43d011b2 100644 --- a/library/sinks/Shelljs.test.ts +++ b/library/sinks/Shelljs.test.ts @@ -6,6 +6,7 @@ import { LoggerNoop } from "../agent/logger/LoggerNoop"; import { Shelljs } from "./Shelljs"; import { ChildProcess } from "./ChildProcess"; import { FileSystem } from "./FileSystem"; +import { isWindows } from "../helpers/isWindows"; const dangerousContext: Context = { remoteAddress: "::1", @@ -187,9 +188,15 @@ t.test("it prevents path injections using ls", async () => { const shelljs = require("shelljs"); const error = await t.rejects(async () => { - runWithContext(dangerousPathContext, () => { - return shelljs.ls("/etc/ssh"); - }); + runWithContext( + { + ...dangerousPathContext, + body: { myTitle: "../../" }, + }, + () => { + return shelljs.ls("../../"); + } + ); }); t.same(