Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 0 additions & 104 deletions src/storage/backend/file.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,110 +160,6 @@ describe('FileBackend xattr metadata', () => {
})
})

describe('FileBackend resolveSecurePath unit', () => {
let tmpDir: string
let backend: FileBackend
let originalStoragePath: string | undefined
let originalFilePath: string | undefined

const resolveSecurePath = (relativePath: string) =>
(
backend as unknown as {
resolveSecurePath: (relativePath: string) => string
}
).resolveSecurePath(relativePath)

beforeEach(async () => {
tmpDir = await fsp.mkdtemp(path.join(os.tmpdir(), 'storage-file-backend-'))
originalStoragePath = process.env.STORAGE_FILE_BACKEND_PATH
originalFilePath = process.env.FILE_STORAGE_BACKEND_PATH
process.env.STORAGE_FILE_BACKEND_PATH = tmpDir
process.env.FILE_STORAGE_BACKEND_PATH = tmpDir
getConfig({ reload: true })
backend = new FileBackend()
})

afterEach(async () => {
if (originalStoragePath === undefined) {
delete process.env.STORAGE_FILE_BACKEND_PATH
} else {
process.env.STORAGE_FILE_BACKEND_PATH = originalStoragePath
}
if (originalFilePath === undefined) {
delete process.env.FILE_STORAGE_BACKEND_PATH
} else {
process.env.FILE_STORAGE_BACKEND_PATH = originalFilePath
}

await removePath(tmpDir)
})

it('resolves safe paths under storage root', () => {
expect(resolveSecurePath('bucket/folder/file.txt')).toBe(
path.join(tmpDir, 'bucket', 'folder', 'file.txt')
)
})

it('rejects parent-directory segments even if they normalize within storage root', () => {
expect(() => resolveSecurePath('bucket/dir/../file.txt')).toThrow(
expect.objectContaining({
code: 'InvalidKey',
})
)
})

it('allows double-dot in file names when not a path segment', () => {
expect(resolveSecurePath('bucket/file..name.txt')).toBe(
path.join(tmpDir, 'bucket', 'file..name.txt')
)
})

it('rejects current-directory segments', () => {
expect(() => resolveSecurePath('.')).toThrow(
expect.objectContaining({
code: 'InvalidKey',
})
)
})

it('rejects absolute paths', () => {
expect(() => resolveSecurePath('/tmp/escape.txt')).toThrow(
expect.objectContaining({
code: 'InvalidKey',
})
)
})

it('rejects Windows absolute path formats', () => {
expect(() => resolveSecurePath('C:\\temp\\escape.txt')).toThrow(
expect.objectContaining({
code: 'InvalidKey',
})
)
expect(() => resolveSecurePath('\\\\server\\share\\escape.txt')).toThrow(
expect.objectContaining({
code: 'InvalidKey',
})
)
})

it('rejects null bytes', () => {
expect(() => resolveSecurePath('bucket/\0escape.txt')).toThrow(
expect.objectContaining({
code: 'InvalidKey',
})
)
})

it('rejects traversal outside storage root', () => {
expect(() => resolveSecurePath('../escape.txt')).toThrow(
expect.objectContaining({
code: 'InvalidKey',
})
)
})
})

describe('FileBackend traversal protection', () => {
let tmpDir: string
let backend: FileBackend
Expand Down
36 changes: 2 additions & 34 deletions src/storage/backend/file.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
UploadPart,
withOptionalVersion,
} from './adapter'
import { resolveSecureFilesystemPath } from './secure-path'

const pipeline = promisify(stream.pipeline)

Expand Down Expand Up @@ -657,40 +658,7 @@ export class FileBackend implements StorageBackendAdapter {
* @throws {StorageBackendError} If the resolved path escapes the storage directory
*/
private resolveSecurePath(relativePath: string): string {
if (relativePath.includes('\0')) {
throw ERRORS.InvalidKey(`Invalid key: ${relativePath} contains null byte`)
}

if (path.isAbsolute(relativePath)) {
throw ERRORS.InvalidKey(`Invalid key: ${relativePath} must be a relative path`)
}

const isWindowsDriveAbsolutePath = /^[a-zA-Z]:[\\/]/.test(relativePath)
const isWindowsUncPath = /^\\\\[^\\/]+[\\/][^\\/]+/.test(relativePath)
if (isWindowsDriveAbsolutePath || isWindowsUncPath) {
throw ERRORS.InvalidKey(`Invalid key: ${relativePath} must not be an absolute Windows path`)
}

const hasDotTraversalSegment = relativePath
.split(/[\\/]+/)
.filter(Boolean)
.some((segment) => segment === '.' || segment === '..')

if (hasDotTraversalSegment) {
throw ERRORS.InvalidKey(`Path traversal detected: ${relativePath} contains dot path segment`)
}

const resolvedPath = path.resolve(this.filePath, relativePath)
const normalizedPath = path.normalize(resolvedPath)

// Ensure the resolved path is within the storage directory
if (!normalizedPath.startsWith(this.filePath + path.sep) && normalizedPath !== this.filePath) {
throw ERRORS.InvalidKey(
`Path traversal detected: ${relativePath} resolves outside storage directory`
)
}

return normalizedPath
return resolveSecureFilesystemPath(this.filePath, relativePath)
}

private async etag(file: string, stats: Stats): Promise<string> {
Expand Down
1 change: 1 addition & 0 deletions src/storage/backend/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { S3Backend, S3ClientOptions } from './s3/adapter'
export * from './adapter'
export * from './file'
export * from './s3'
export * from './secure-path'

const { storageS3Region, storageS3Endpoint, storageS3ForcePathStyle, storageS3ClientTimeout } =
getConfig()
Expand Down
102 changes: 102 additions & 0 deletions src/storage/backend/secure-path.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
import path from 'node:path'
import { resolveSecureFilesystemPath } from './secure-path'

describe('resolveSecureFilesystemPath', () => {
const storageRoot = path.resolve('tmp', 'storage-root')

it('resolves safe paths under the storage root', () => {
expect(resolveSecureFilesystemPath(storageRoot, 'bucket/folder/file.txt')).toBe(
path.join(storageRoot, 'bucket', 'folder', 'file.txt')
)
})

it('normalizes the configured root path before resolving children', () => {
const rootWithParentSegment = path.join(storageRoot, 'tenant', '..')

expect(resolveSecureFilesystemPath(rootWithParentSegment, 'bucket/file.txt')).toBe(
path.join(storageRoot, 'bucket', 'file.txt')
)
})

it('allows child paths when the configured root is the filesystem root', () => {
const filesystemRoot = path.parse(storageRoot).root

expect(resolveSecureFilesystemPath(filesystemRoot, 'bucket/file.txt')).toBe(
path.join(filesystemRoot, 'bucket', 'file.txt')
)
})

it('allows double-dot in file names when not used as a path segment', () => {
expect(resolveSecureFilesystemPath(storageRoot, 'bucket/file..name.txt')).toBe(
path.join(storageRoot, 'bucket', 'file..name.txt')
)
})

it.each([
'bucket/dir/../file.txt',
'.',
'../escape.txt',
])('rejects dot path segments in %s', (relativePath) => {
expect(() => resolveSecureFilesystemPath(storageRoot, relativePath)).toThrow(
expect.objectContaining({
code: 'InvalidKey',
})
)
})

it('rejects absolute posix paths', () => {
expect(() => resolveSecureFilesystemPath(storageRoot, '/tmp/escape.txt')).toThrow(
expect.objectContaining({
code: 'InvalidKey',
})
)
})

it.each([
'C:\\temp\\escape.txt',
'\\\\server\\share\\escape.txt',
])('rejects absolute windows paths in %s format', (relativePath) => {
expect(() => resolveSecureFilesystemPath(storageRoot, relativePath)).toThrow(
expect.objectContaining({
code: 'InvalidKey',
})
)
})

it('rejects null bytes', () => {
expect(() => resolveSecureFilesystemPath(storageRoot, 'bucket/\0escape.txt')).toThrow(
expect.objectContaining({
code: 'InvalidKey',
})
)
})

it.each([
{
relativePath: 'bucket/\0escape.txt',
expectedMessage: 'Invalid key: bucket/\0escape.txt contains null byte',
},
{
relativePath: '/tmp/escape.txt',
expectedMessage: 'Invalid key: /tmp/escape.txt must be a relative path',
},
{
relativePath: '../escape.txt',
expectedMessage: 'Invalid key: ../escape.txt contains dot path segment',
},
])('keeps InvalidKey resource and message aligned for %s', ({
relativePath,
expectedMessage,
}) => {
try {
resolveSecureFilesystemPath(storageRoot, relativePath)
throw new Error('expected resolveSecureFilesystemPath to throw')
} catch (error) {
expect(error).toMatchObject({
code: 'InvalidKey',
message: expectedMessage,
resource: relativePath,
})
}
})
})
53 changes: 53 additions & 0 deletions src/storage/backend/secure-path.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import { ErrorCode, StorageBackendError } from '@internal/errors'
import path from 'path'

/**
* Resolve a user-controlled relative path under a fixed filesystem root.
* Rejects null bytes, absolute paths, and `.` / `..` path segments before resolution.
*/
export function resolveSecureFilesystemPath(rootPath: string, relativePath: string): string {
if (relativePath.includes('\0')) {
throwInvalidKey(relativePath, 'contains null byte')
}

if (path.isAbsolute(relativePath)) {
throwInvalidKey(relativePath, 'must be a relative path')
}

const isWindowsDriveAbsolutePath = /^[a-zA-Z]:[\\/]/.test(relativePath)
const isWindowsUncPath = /^\\\\[^\\/]+[\\/][^\\/]+/.test(relativePath)
if (isWindowsDriveAbsolutePath || isWindowsUncPath) {
throwInvalidKey(relativePath, 'must not be an absolute Windows path')
}
Comment thread
ferhatelmas marked this conversation as resolved.

const hasDotTraversalSegment = relativePath
.split(/[\\/]+/)
.filter(Boolean)
.some((segment) => segment === '.' || segment === '..')

if (hasDotTraversalSegment) {
throwInvalidKey(relativePath, 'contains dot path segment')
}

const normalizedRootPath = path.normalize(path.resolve(rootPath))
const resolvedPath = path.resolve(normalizedRootPath, relativePath)
const normalizedPath = path.normalize(resolvedPath)
const normalizedRootPrefix = normalizedRootPath.endsWith(path.sep)
? normalizedRootPath
: normalizedRootPath + path.sep

if (!normalizedPath.startsWith(normalizedRootPrefix) && normalizedPath !== normalizedRootPath) {
throwInvalidKey(relativePath, 'resolves outside storage directory')
}

return normalizedPath
}

function throwInvalidKey(relativePath: string, reason: string): never {
throw new StorageBackendError({
code: ErrorCode.InvalidKey,
resource: relativePath,
httpStatusCode: 400,
message: `Invalid key: ${relativePath} ${reason}`,
})
}
Loading