Bugfix/add getSafeFilePath utility for secure file path resolution (#6549)

* add getSafeFilePath utility for secure file path resolution * Enhance getSafeFilePath: add null byte detection and improve path traversal checks. Extend test cases for edge scenarios including encoded paths and leading/trailing dots.
2026-10-12 02:02:17 +00:00 · 2026-06-23 17:53:07 +01:00
parent d071868442
commit 571b5d6218
4 changed files with 164 additions and 48 deletions
@@ -1,6 +1,7 @@
 import { ICommonObject, INode, INodeData, INodeOptionsValue, INodeOutputsValue, INodeParams } from '../../../src/Interface'
 import { handleDocumentLoaderDocuments, handleDocumentLoaderMetadata, handleDocumentLoaderOutput } from '../../../src/utils'
 import { getAWSCredentialConfig } from '../../../src/awsToolsUtils'
+import { getSafeFilePath } from '../../../src/validator'
 import { S3Client, GetObjectCommand, S3ClientConfig, ListObjectsV2Command, ListObjectsV2Output } from '@aws-sdk/client-s3'
 import { getRegions, MODEL_TYPE } from '../../../src/modelLoader'
 import { Readable } from 'node:stream'
@@ -188,7 +189,7 @@ class S3_DocumentLoaders implements INode {

            await Promise.all(
                keys.map(async (key) => {
-                    const filePath = path.join(tempDir, key)
+                    const filePath = getSafeFilePath(tempDir, key)
                    try {
                        const response = await s3Client.send(
                            new GetObjectCommand({
@@ -9,6 +9,7 @@ import {
 } from '@langchain/community/document_loaders/fs/unstructured'
 import { handleDocumentLoaderDocuments, handleDocumentLoaderMetadata, handleDocumentLoaderOutput } from '../../../src/utils'
 import { getAWSCredentialConfig } from '../../../src/awsToolsUtils'
+import { getSafeFilePath } from '../../../src/validator'
 import { S3Client, GetObjectCommand, HeadObjectCommand, S3ClientConfig } from '@aws-sdk/client-s3'
 import { getRegions, MODEL_TYPE } from '../../../src/modelLoader'
 import { Readable } from 'node:stream'
@@ -753,68 +754,70 @@ class S3_DocumentLoaders implements INode {
        loader.load = async () => {
            const tempDir = fsDefault.mkdtempSync(path.join(os.tmpdir(), 's3fileloader-'))

-            const filePath = path.join(tempDir, keyName)
-
            try {
-                const s3Client = new S3Client(s3Config)
+                const filePath = getSafeFilePath(tempDir, keyName)

-                const getObjectCommand = new GetObjectCommand({
-                    Bucket: bucketName,
-                    Key: keyName
-                })
+                try {
+                    const s3Client = new S3Client(s3Config)

-                const response = await s3Client.send(getObjectCommand)
+                    const getObjectCommand = new GetObjectCommand({
+                        Bucket: bucketName,
+                        Key: keyName
+                    })

-                const objectData = await new Promise<Buffer>((resolve, reject) => {
-                    const chunks: Buffer[] = []
+                    const response = await s3Client.send(getObjectCommand)

-                    if (response.Body instanceof Readable) {
-                        response.Body.on('data', (chunk: Buffer) => chunks.push(chunk))
-                        response.Body.on('end', () => resolve(Buffer.concat(chunks)))
-                        response.Body.on('error', reject)
-                    } else {
-                        reject(new Error('Response body is not a readable stream.'))
-                    }
-                })
+                    const objectData = await new Promise<Buffer>((resolve, reject) => {
+                        const chunks: Buffer[] = []

-                fsDefault.mkdirSync(path.dirname(filePath), { recursive: true })
+                        if (response.Body instanceof Readable) {
+                            response.Body.on('data', (chunk: Buffer) => chunks.push(chunk))
+                            response.Body.on('end', () => resolve(Buffer.concat(chunks)))
+                            response.Body.on('error', reject)
+                        } else {
+                            reject(new Error('Response body is not a readable stream.'))
+                        }
+                    })

-                fsDefault.writeFileSync(filePath, objectData)
-            } catch (e: any) {
-                throw new Error(`Failed to download file ${keyName} from S3 bucket ${bucketName}: ${e.message}`)
-            }
+                    fsDefault.mkdirSync(path.dirname(filePath), { recursive: true })

-            try {
-                const obj: UnstructuredLoaderOptions = {
-                    apiUrl: unstructuredAPIUrl,
-                    strategy,
-                    encoding,
-                    coordinates,
-                    skipInferTableTypes,
-                    hiResModelName,
-                    includePageBreaks,
-                    chunkingStrategy,
-                    ocrLanguages,
-                    xmlKeepTags,
-                    multiPageSections,
-                    combineUnderNChars,
-                    newAfterNChars,
-                    maxCharacters
+                    fsDefault.writeFileSync(filePath, objectData)
+                } catch (e: any) {
+                    throw new Error(`Failed to download file ${keyName} from S3 bucket ${bucketName}: ${e.message}`)
                }

-                if (unstructuredAPIKey) obj.apiKey = unstructuredAPIKey
+                try {
+                    const obj: UnstructuredLoaderOptions = {
+                        apiUrl: unstructuredAPIUrl,
+                        strategy,
+                        encoding,
+                        coordinates,
+                        skipInferTableTypes,
+                        hiResModelName,
+                        includePageBreaks,
+                        chunkingStrategy,
+                        ocrLanguages,
+                        xmlKeepTags,
+                        multiPageSections,
+                        combineUnderNChars,
+                        newAfterNChars,
+                        maxCharacters
+                    }

-                const unstructuredLoader = new UnstructuredLoader(filePath, obj)
+                    if (unstructuredAPIKey) obj.apiKey = unstructuredAPIKey

-                let docs = await handleDocumentLoaderDocuments(unstructuredLoader)
+                    const unstructuredLoader = new UnstructuredLoader(filePath, obj)

-                docs = handleDocumentLoaderMetadata(docs, _omitMetadataKeys, metadata, sourceIdKey)
+                    let docs = await handleDocumentLoaderDocuments(unstructuredLoader)

-                return handleDocumentLoaderOutput(docs, output)
-            } catch {
-                throw new Error(`Failed to load file ${filePath} using unstructured loader.`)
+                    docs = handleDocumentLoaderMetadata(docs, _omitMetadataKeys, metadata, sourceIdKey)
+
+                    return handleDocumentLoaderOutput(docs, output)
+                } catch {
+                    throw new Error(`Failed to load file ${filePath} using unstructured loader.`)
+                }
            } finally {
-                fsDefault.rmSync(path.dirname(filePath), { recursive: true })
+                fsDefault.rmSync(tempDir, { recursive: true, force: true })
            }
        }

@@ -1,4 +1,5 @@
 import {
+    getSafeFilePath,
    isPathTraversal,
    isUnsafeFilePath,
    isValidURL,
@@ -724,3 +725,74 @@ describe('isValidURL', () => {
        })
    })
 })
+
+describe('getSafeFilePath', () => {
+    const base = path.resolve('/tmp/s3fileloader-abc123')
+
+    describe('returns a contained absolute path for safe keys', () => {
+        it.each([
+            ['simple filename', 'report.pdf'],
+            ['nested key', 'documents/2024/report.pdf'],
+            ['internal dot-dot that stays inside', 'a/b/../c.txt'],
+            ['leading ./', './notes.txt'],
+            ['name starting with dot-dot (not traversal)', '..foo.txt'],
+            ['triple dot name', '...'],
+            ['dot-dot prefixed dir', '..hidden/file.txt'],
+            ['encoded space in legitimate key', 'my%20report.pdf']
+        ])('should resolve %s within base', (_desc, key) => {
+            const resolved = getSafeFilePath(base, key)
+            const relative = path.relative(base, resolved)
+            expect(path.isAbsolute(resolved)).toBe(true)
+            expect(relative === '..' || relative.startsWith('..' + path.sep)).toBe(false)
+            expect(path.isAbsolute(relative)).toBe(false)
+        })
+    })
+
+    describe('throws on path traversal / absolute keys', () => {
+        it.each([
+            ['parent traversal', '../escape.txt'],
+            ['deep traversal', '../../../../tmp/flowise-poc.txt'],
+            ['traversal mid-path', 'a/../../escape.txt'],
+            ['bare dot-dot', '..'],
+            ['absolute unix path', '/etc/cron.d/evil'],
+            ['key resolving to base itself', '.'],
+            ['url-encoded traversal', '%2e%2e%2fescape.txt'],
+            ['url-encoded traversal mixed', '%2e%2e/escape.txt'],
+            ['partially-encoded slash traversal', '..%2f..%2fescape.txt']
+        ])('should reject %s', (_desc, key) => {
+            expect(() => getSafeFilePath(base, key)).toThrow(/path traversal attempt detected/)
+        })
+    })
+
+    describe('throws on null bytes', () => {
+        it.each([
+            ['literal null byte', 'evil\0.txt'],
+            ['encoded null byte', 'evil%00.txt']
+        ])('should reject %s', (_desc, key) => {
+            expect(() => getSafeFilePath(base, key)).toThrow(/null byte detected/)
+        })
+    })
+
+    describe('throws on missing/invalid keys', () => {
+        it.each([
+            ['empty string', ''],
+            ['undefined', undefined],
+            ['null', null]
+        ])('should reject %s', (_desc, key) => {
+            expect(() => getSafeFilePath(base, key as any)).toThrow(/key is required/)
+        })
+    })
+
+    describe('PATH_TRAVERSAL_SAFETY=false bypasses the containment check', () => {
+        beforeEach(() => {
+            process.env.PATH_TRAVERSAL_SAFETY = 'false'
+        })
+        afterEach(() => {
+            delete process.env.PATH_TRAVERSAL_SAFETY
+        })
+
+        it('returns the resolved (escaping) path without throwing', () => {
+            expect(() => getSafeFilePath(base, '../escape.txt')).not.toThrow()
+        })
+    })
+})
@@ -391,3 +391,43 @@ export const sanitizeFileName = (name: string): string => {
    }
    return baseName
 }
+
+/**
+ * Safely resolve an untrusted relative key/filename to an absolute path inside a
+ * trusted base directory, guaranteeing the result cannot escape that directory.
+ *
+ * @param {string} baseDir The trusted base directory (e.g. a freshly created temp dir)
+ * @param {string} key The untrusted relative key or filename
+ * @returns {string} A validated absolute path guaranteed to be within baseDir
+ * @throws {Error} If key is missing/invalid or the resolved path escapes baseDir
+ */
+export const getSafeFilePath = (baseDir: string, key: string): string => {
+    if (!key || typeof key !== 'string') {
+        throw new Error('Invalid file path: key is required and must be a string')
+    }
+
+    let decodedKey = key
+    try {
+        decodedKey = decodeURIComponent(key)
+    } catch {
+        // malformed percent-encoding — keep the raw key; resolve/relative handle it safely
+    }
+
+    if (decodedKey.includes('\0')) {
+        throw new Error(`Invalid file path: null byte detected in "${key}"`)
+    }
+
+    const resolvedBase = path.resolve(baseDir)
+    const resolvedPath = path.resolve(resolvedBase, decodedKey)
+
+    if (process.env.PATH_TRAVERSAL_SAFETY === 'false') {
+        return resolvedPath
+    }
+
+    const relative = path.relative(resolvedBase, resolvedPath)
+    if (relative === '' || relative === '..' || relative.startsWith('..' + path.sep) || path.isAbsolute(relative)) {
+        throw new Error(`Invalid file path: path traversal attempt detected in "${key}"`)
+    }
+
+    return resolvedPath
+}