From abd63700a52a233d19e74630be77878130458ee3 Mon Sep 17 00:00:00 2001 From: Pranav C Date: Fri, 28 Jul 2023 17:04:03 +0530 Subject: [PATCH 1/5] chore: update sdk path Signed-off-by: Pranav C --- packages/nc-gui/package-lock.json | 52 +++++++++++---------------- packages/nc-gui/package.json | 2 +- packages/nocodb-sdk/package-lock.json | 4 +-- packages/nocodb/package-lock.json | 30 +++++++++------- packages/nocodb/package.json | 4 +-- 5 files changed, 42 insertions(+), 50 deletions(-) diff --git a/packages/nc-gui/package-lock.json b/packages/nc-gui/package-lock.json index ea8ccadd64..7ccbf09e52 100644 --- a/packages/nc-gui/package-lock.json +++ b/packages/nc-gui/package-lock.json @@ -30,7 +30,7 @@ "leaflet.markercluster": "^1.5.3", "locale-codes": "^1.3.1", "monaco-editor": "^0.33.0", - "nocodb-sdk": "0.109.5", + "nocodb-sdk": "file:../nocodb-sdk", "papaparse": "^5.3.2", "pinia": "^2.0.33", "qrcode": "^1.5.1", @@ -111,7 +111,6 @@ }, "../nocodb-sdk": { "version": "0.109.5", - "extraneous": true, "license": "AGPL-3.0-or-later", "dependencies": { "axios": "^0.21.1", @@ -8720,6 +8719,7 @@ "version": "1.15.1", "resolved": "https://registry.npmjs.org/follow-redirects/-/follow-redirects-1.15.1.tgz", "integrity": "sha512-yLAMQs+k0b2m7cVxpS1VKJVvoz7SS9Td1zss3XRwXj+ZDH00RJgnuLx7E44wx02kQLrdM3aOOy+FpzS7+8OizA==", + "devOptional": true, "funding": [ { "type": "individual", @@ -12238,21 +12238,8 @@ } }, "node_modules/nocodb-sdk": { - "version": "0.109.5", - "resolved": "https://registry.npmjs.org/nocodb-sdk/-/nocodb-sdk-0.109.5.tgz", - "integrity": "sha512-CGrTADus9bfUJ5qHcMqRN3OYwOmkRm7iRml54lXmE36WoxACQs/ACWi7OQkzDSsSpduPitHis0a0n83AmlBVYw==", - "dependencies": { - "axios": "^0.21.1", - "jsep": "^1.3.6" - } - }, - "node_modules/nocodb-sdk/node_modules/axios": { - "version": "0.21.4", - "resolved": "https://registry.npmjs.org/axios/-/axios-0.21.4.tgz", - "integrity": "sha512-ut5vewkiu8jjGBdqpM44XxjuCjq9LAKeHVmoVfHVzy8eHgxxq8SbAVQNovDA8mVi05kP0Ea/n/UzcSHcTJQfNg==", - "dependencies": { - "follow-redirects": "^1.14.0" - } + "resolved": "../nocodb-sdk", + "link": true }, "node_modules/node-abi": { "version": "3.23.0", @@ -24729,7 +24716,8 @@ "follow-redirects": { "version": "1.15.1", "resolved": "https://registry.npmjs.org/follow-redirects/-/follow-redirects-1.15.1.tgz", - "integrity": "sha512-yLAMQs+k0b2m7cVxpS1VKJVvoz7SS9Td1zss3XRwXj+ZDH00RJgnuLx7E44wx02kQLrdM3aOOy+FpzS7+8OizA==" + "integrity": "sha512-yLAMQs+k0b2m7cVxpS1VKJVvoz7SS9Td1zss3XRwXj+ZDH00RJgnuLx7E44wx02kQLrdM3aOOy+FpzS7+8OizA==", + "devOptional": true }, "form-data": { "version": "4.0.0", @@ -27279,22 +27267,22 @@ } }, "nocodb-sdk": { - "version": "0.109.5", - "resolved": "https://registry.npmjs.org/nocodb-sdk/-/nocodb-sdk-0.109.5.tgz", - "integrity": "sha512-CGrTADus9bfUJ5qHcMqRN3OYwOmkRm7iRml54lXmE36WoxACQs/ACWi7OQkzDSsSpduPitHis0a0n83AmlBVYw==", + "version": "file:../nocodb-sdk", "requires": { + "@typescript-eslint/eslint-plugin": "^4.0.1", + "@typescript-eslint/parser": "^4.0.1", "axios": "^0.21.1", - "jsep": "^1.3.6" - }, - "dependencies": { - "axios": { - "version": "0.21.4", - "resolved": "https://registry.npmjs.org/axios/-/axios-0.21.4.tgz", - "integrity": "sha512-ut5vewkiu8jjGBdqpM44XxjuCjq9LAKeHVmoVfHVzy8eHgxxq8SbAVQNovDA8mVi05kP0Ea/n/UzcSHcTJQfNg==", - "requires": { - "follow-redirects": "^1.14.0" - } - } + "cspell": "^4.1.0", + "eslint": "^7.8.0", + "eslint-config-prettier": "^6.11.0", + "eslint-plugin-eslint-comments": "^3.2.0", + "eslint-plugin-functional": "^3.0.2", + "eslint-plugin-import": "^2.22.0", + "eslint-plugin-prettier": "^4.0.0", + "jsep": "^1.3.6", + "npm-run-all": "^4.1.5", + "prettier": "^2.1.1", + "typescript": "^4.0.2" } }, "node-abi": { diff --git a/packages/nc-gui/package.json b/packages/nc-gui/package.json index b1ba777e4a..44f0905daf 100644 --- a/packages/nc-gui/package.json +++ b/packages/nc-gui/package.json @@ -54,7 +54,7 @@ "leaflet.markercluster": "^1.5.3", "locale-codes": "^1.3.1", "monaco-editor": "^0.33.0", - "nocodb-sdk": "0.109.5", + "nocodb-sdk": "file:../nocodb-sdk", "papaparse": "^5.3.2", "pinia": "^2.0.33", "qrcode": "^1.5.1", diff --git a/packages/nocodb-sdk/package-lock.json b/packages/nocodb-sdk/package-lock.json index 0aa5aabb3e..dda8da70f5 100644 --- a/packages/nocodb-sdk/package-lock.json +++ b/packages/nocodb-sdk/package-lock.json @@ -1,12 +1,12 @@ { "name": "nocodb-sdk", - "version": "0.109.4", + "version": "0.109.5", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "nocodb-sdk", - "version": "0.109.4", + "version": "0.109.5", "license": "AGPL-3.0-or-later", "dependencies": { "axios": "^0.21.1", diff --git a/packages/nocodb/package-lock.json b/packages/nocodb/package-lock.json index a12cfdb960..b8da539dec 100644 --- a/packages/nocodb/package-lock.json +++ b/packages/nocodb/package-lock.json @@ -83,7 +83,7 @@ "nc-lib-gui": "0.109.5", "nc-plugin": "^0.1.3", "ncp": "^2.0.0", - "nocodb-sdk": "0.109.5", + "nocodb-sdk": "file:../nocodb-sdk", "nodemailer": "^6.4.10", "object-hash": "^3.0.0", "object-sizeof": "^2.6.1", @@ -192,7 +192,6 @@ }, "../nocodb-sdk": { "version": "0.109.5", - "extraneous": true, "license": "AGPL-3.0-or-later", "dependencies": { "axios": "^0.21.1", @@ -13208,13 +13207,8 @@ } }, "node_modules/nocodb-sdk": { - "version": "0.109.5", - "resolved": "https://registry.npmjs.org/nocodb-sdk/-/nocodb-sdk-0.109.5.tgz", - "integrity": "sha512-CGrTADus9bfUJ5qHcMqRN3OYwOmkRm7iRml54lXmE36WoxACQs/ACWi7OQkzDSsSpduPitHis0a0n83AmlBVYw==", - "dependencies": { - "axios": "^0.21.1", - "jsep": "^1.3.6" - } + "resolved": "../nocodb-sdk", + "link": true }, "node_modules/node-abort-controller": { "version": "3.1.1", @@ -28517,12 +28511,22 @@ "integrity": "sha512-dBpDMdxv9Irdq66304OLfEmQ9tbNRFnFTuZiLo+bD+r332bBmMJ8GBLXklIXXgxd3+v9+KUnZaUR5PJMa75Gsg==" }, "nocodb-sdk": { - "version": "0.109.5", - "resolved": "https://registry.npmjs.org/nocodb-sdk/-/nocodb-sdk-0.109.5.tgz", - "integrity": "sha512-CGrTADus9bfUJ5qHcMqRN3OYwOmkRm7iRml54lXmE36WoxACQs/ACWi7OQkzDSsSpduPitHis0a0n83AmlBVYw==", + "version": "file:../nocodb-sdk", "requires": { + "@typescript-eslint/eslint-plugin": "^4.0.1", + "@typescript-eslint/parser": "^4.0.1", "axios": "^0.21.1", - "jsep": "^1.3.6" + "cspell": "^4.1.0", + "eslint": "^7.8.0", + "eslint-config-prettier": "^6.11.0", + "eslint-plugin-eslint-comments": "^3.2.0", + "eslint-plugin-functional": "^3.0.2", + "eslint-plugin-import": "^2.22.0", + "eslint-plugin-prettier": "^4.0.0", + "jsep": "^1.3.6", + "npm-run-all": "^4.1.5", + "prettier": "^2.1.1", + "typescript": "^4.0.2" } }, "node-abort-controller": { diff --git a/packages/nocodb/package.json b/packages/nocodb/package.json index 0819d4922e..2a1c30f8ca 100644 --- a/packages/nocodb/package.json +++ b/packages/nocodb/package.json @@ -116,7 +116,7 @@ "nc-lib-gui": "0.109.5", "nc-plugin": "^0.1.3", "ncp": "^2.0.0", - "nocodb-sdk": "0.109.5", + "nocodb-sdk": "file:../nocodb-sdk", "nodemailer": "^6.4.10", "object-hash": "^3.0.0", "object-sizeof": "^2.6.1", @@ -204,4 +204,4 @@ "coverageDirectory": "../coverage", "testEnvironment": "node" } -} \ No newline at end of file +} From 6e90e2258e537b8e19fb532c79028025629263eb Mon Sep 17 00:00:00 2001 From: Pranav C Date: Mon, 31 Jul 2023 18:11:12 +0530 Subject: [PATCH 2/5] fix: file read - allow only accessing files from intended folder Signed-off-by: Pranav C --- packages/nocodb/src/plugins/storage/Local.ts | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/packages/nocodb/src/plugins/storage/Local.ts b/packages/nocodb/src/plugins/storage/Local.ts index f0dbebf2a4..a6232ed32f 100644 --- a/packages/nocodb/src/plugins/storage/Local.ts +++ b/packages/nocodb/src/plugins/storage/Local.ts @@ -3,6 +3,7 @@ import path from 'path'; import { promisify } from 'util'; import mkdirp from 'mkdirp'; import axios from 'axios'; +import { NcError } from '../../helpers/catchError'; import { getToolDir } from '../../utils/nc-config'; import type { IStorageAdapterV2, XcFile } from 'nc-plugin'; import type { Readable } from 'stream'; @@ -102,9 +103,20 @@ export default class Local implements IStorageAdapterV2 { public async fileRead(filePath: string): Promise { try { - const fileData = await fs.promises.readFile( + // Get the absolute path to the base directory + const absoluteBasePath = path.resolve(getToolDir()); + + // Get the absolute path to the file + const absolutePath = path.resolve( path.join(getToolDir(), ...filePath.split('/')), ); + + // Check if the resolved path is within the intended directory + if (!absolutePath.startsWith(absoluteBasePath)) { + NcError.notFound('Invalid path'); + } + + const fileData = await fs.promises.readFile(absolutePath); return fileData; } catch (e) { throw e; From 1e303a5cbc7c7bf533a15528ec4bf73b32bcf387 Mon Sep 17 00:00:00 2001 From: Pranav C Date: Mon, 31 Jul 2023 22:35:30 +0530 Subject: [PATCH 3/5] fix: add validation and normalisation for all methods since write/list operations also vulnerable Signed-off-by: Pranav C --- packages/nocodb/src/plugins/storage/Local.ts | 50 +++++++++++++------- 1 file changed, 32 insertions(+), 18 deletions(-) diff --git a/packages/nocodb/src/plugins/storage/Local.ts b/packages/nocodb/src/plugins/storage/Local.ts index a6232ed32f..e7bff7fdce 100644 --- a/packages/nocodb/src/plugins/storage/Local.ts +++ b/packages/nocodb/src/plugins/storage/Local.ts @@ -12,7 +12,7 @@ export default class Local implements IStorageAdapterV2 { constructor() {} public async fileCreate(key: string, file: XcFile): Promise { - const destPath = path.join(getToolDir(), ...key.split('/')); + const destPath = this.validateAndNormalisePath(key); try { await mkdirp(path.dirname(destPath)); const data = await promisify(fs.readFile)(file.path); @@ -25,7 +25,7 @@ export default class Local implements IStorageAdapterV2 { } async fileCreateByUrl(key: string, url: string): Promise { - const destPath = path.join(getToolDir(), ...key.split('/')); + const destPath = this.validateAndNormalisePath(key); return new Promise((resolve, reject) => { axios .get(url, { @@ -72,7 +72,7 @@ export default class Local implements IStorageAdapterV2 { stream: Readable, ): Promise { return new Promise((resolve, reject) => { - const destPath = path.join(getToolDir(), ...key.split('/')); + const destPath = this.validateAndNormalisePath(key); try { mkdirp(path.dirname(destPath)).then(() => { const writableStream = fs.createWriteStream(destPath); @@ -87,12 +87,12 @@ export default class Local implements IStorageAdapterV2 { } public async fileReadByStream(key: string): Promise { - const srcPath = path.join(getToolDir(), ...key.split('/')); + const srcPath = this.validateAndNormalisePath(key); return fs.createReadStream(srcPath, { encoding: 'utf8' }); } public async getDirectoryList(key: string): Promise { - const destDir = path.join(getToolDir(), ...key.split('/')); + const destDir = this.validateAndNormalisePath(key); return fs.promises.readdir(destDir); } @@ -103,20 +103,9 @@ export default class Local implements IStorageAdapterV2 { public async fileRead(filePath: string): Promise { try { - // Get the absolute path to the base directory - const absoluteBasePath = path.resolve(getToolDir()); - - // Get the absolute path to the file - const absolutePath = path.resolve( - path.join(getToolDir(), ...filePath.split('/')), + const fileData = await fs.promises.readFile( + this.validateAndNormalisePath(filePath, true), ); - - // Check if the resolved path is within the intended directory - if (!absolutePath.startsWith(absoluteBasePath)) { - NcError.notFound('Invalid path'); - } - - const fileData = await fs.promises.readFile(absolutePath); return fileData; } catch (e) { throw e; @@ -130,4 +119,29 @@ export default class Local implements IStorageAdapterV2 { test(): Promise { return Promise.resolve(false); } + + // method for validate/normalise the path for avoid path traversal attack + protected validateAndNormalisePath( + fileOrFolderPath: string, + throw404 = false, + ): string { + // Get the absolute path to the base directory + const absoluteBasePath = path.resolve(getToolDir()); + + // Get the absolute path to the file + const absolutePath = path.resolve( + path.join(getToolDir(), ...fileOrFolderPath.split('/')), + ); + + // Check if the resolved path is within the intended directory + if (!absolutePath.startsWith(absoluteBasePath)) { + if (throw404) { + NcError.notFound(); + } else { + NcError.badRequest('Invalid path'); + } + } + + return absolutePath; + } } From 5fcb7c88acc661fed282dff06c51396e454a93ec Mon Sep 17 00:00:00 2001 From: Pranav C Date: Tue, 1 Aug 2023 12:36:24 +0530 Subject: [PATCH 4/5] fix: include `nc` along with base path since we are always uploading under that folder - base path contain sqlite file as well Signed-off-by: Pranav C --- packages/nocodb/src/plugins/storage/Local.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nocodb/src/plugins/storage/Local.ts b/packages/nocodb/src/plugins/storage/Local.ts index e7bff7fdce..7d49e54f70 100644 --- a/packages/nocodb/src/plugins/storage/Local.ts +++ b/packages/nocodb/src/plugins/storage/Local.ts @@ -126,7 +126,7 @@ export default class Local implements IStorageAdapterV2 { throw404 = false, ): string { // Get the absolute path to the base directory - const absoluteBasePath = path.resolve(getToolDir()); + const absoluteBasePath = path.resolve(getToolDir(), 'nc'); // Get the absolute path to the file const absolutePath = path.resolve( From 19e6075f54096940c77d3e4a1c7c55d9f410c8be Mon Sep 17 00:00:00 2001 From: Anbarasu Date: Wed, 2 Aug 2023 14:57:36 +0530 Subject: [PATCH 5/5] fix: test webhook on condition --- packages/nocodb/src/helpers/webhookHelpers.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nocodb/src/helpers/webhookHelpers.ts b/packages/nocodb/src/helpers/webhookHelpers.ts index c0fc56a448..400db86b71 100644 --- a/packages/nocodb/src/helpers/webhookHelpers.ts +++ b/packages/nocodb/src/helpers/webhookHelpers.ts @@ -271,7 +271,7 @@ export async function invokeWebhook( return; } - if (hook.condition) { + if (hook.condition && !testHook) { if (isBulkOperation) { const filteredData = []; for (const data of newData) {