fix: address code review - fallback to homedir, validate baseDir, use afterAll
Address all 3 points from @op7418's review: 1. No baseDir fallback (severe): Both /api/files and /api/files/preview now fall back to os.homedir() when no baseDir is provided, preventing access to arbitrary system directories/files. 2. baseDir bypass via baseDir=/ (medium): Both APIs now validate that baseDir itself is within the user's home directory. Setting baseDir=/ will be rejected with 403. 3. Test cleanup (minor): Replaced 'it(cleanup...)' with proper after() hook. Added 6 new tests for baseDir validation (18 total, all pass).
This commit is contained in:
1 parent
84de2d1de8
commit
0f05e4fd0f
3 files changed
+74
-8
No files matched your search
@@ -10,7 +10,7 @@
|
||||
* 4. Edge cases (root, same path, trailing separators) are handled
|
||||
*/
|
||||
|
||||
import { describe, it } from 'node:test';
|
||||
import { describe, it, after } from 'node:test';
|
||||
import assert from 'node:assert/strict';
|
||||
import path from 'path';
|
||||
import os from 'os';
|
||||
@@ -112,8 +112,46 @@ describe('File API path traversal scenarios', () => {
|
||||
});
|
||||
}
|
||||
|
||||
// Cleanup
|
||||
it('cleanup test fixtures', () => {
|
||||
// Cleanup test fixtures
|
||||
after(() => {
|
||||
fs.rmSync(tmpDir, { recursive: true, force: true });
|
||||
});
|
||||
});
|
||||
|
||||
describe('baseDir validation', () => {
|
||||
it('should reject baseDir set to root (bypass attempt)', () => {
|
||||
// If baseDir=/, every path would pass isPathSafe — must be blocked
|
||||
const homeDir = os.homedir();
|
||||
assert.equal(isPathSafe(homeDir, '/'), false);
|
||||
});
|
||||
|
||||
it('should reject baseDir outside home directory', () => {
|
||||
const homeDir = os.homedir();
|
||||
assert.equal(isPathSafe(homeDir, '/tmp'), false);
|
||||
assert.equal(isPathSafe(homeDir, '/etc'), false);
|
||||
assert.equal(isPathSafe(homeDir, '/var/log'), false);
|
||||
});
|
||||
|
||||
it('should allow baseDir inside home directory', () => {
|
||||
const homeDir = os.homedir();
|
||||
const projectDir = path.join(homeDir, 'projects', 'myapp');
|
||||
assert.equal(isPathSafe(homeDir, projectDir), true);
|
||||
});
|
||||
|
||||
it('should allow baseDir equal to home directory', () => {
|
||||
const homeDir = os.homedir();
|
||||
assert.equal(isPathSafe(homeDir, homeDir), true);
|
||||
});
|
||||
|
||||
it('should block files outside home when no baseDir provided (fallback)', () => {
|
||||
const homeDir = os.homedir();
|
||||
assert.equal(isPathSafe(homeDir, '/etc/passwd'), false);
|
||||
assert.equal(isPathSafe(homeDir, '/tmp/malicious'), false);
|
||||
});
|
||||
|
||||
it('should allow files inside home when no baseDir provided (fallback)', () => {
|
||||
const homeDir = os.homedir();
|
||||
const filePath = path.join(homeDir, 'documents', 'file.txt');
|
||||
assert.equal(isPathSafe(homeDir, filePath), true);
|
||||
});
|
||||
});
|
||||
@@ -15,14 +15,25 @@ export async function GET(request: NextRequest) {
|
||||
}
|
||||
|
||||
const path = require('path');
|
||||
const os = require('os');
|
||||
const resolvedPath = path.resolve(filePath);
|
||||
const homeDir = os.homedir();
|
||||
|
||||
// Validate that the file is within the session's working directory.
|
||||
// The baseDir parameter should be the session's working directory,
|
||||
// which acts as the trust boundary for file access.
|
||||
// The baseDir itself must be under the user's home directory to prevent
|
||||
// attackers from setting baseDir=/ to bypass all restrictions.
|
||||
const baseDir = searchParams.get('baseDir');
|
||||
if (baseDir) {
|
||||
const resolvedBase = path.resolve(baseDir);
|
||||
// Ensure baseDir is within the home directory (prevent baseDir=/ bypass)
|
||||
if (!isPathSafe(homeDir, resolvedBase)) {
|
||||
return NextResponse.json<ErrorResponse>(
|
||||
{ error: 'Base directory is outside the allowed scope' },
|
||||
{ status: 403 }
|
||||
);
|
||||
}
|
||||
if (!isPathSafe(resolvedBase, resolvedPath)) {
|
||||
return NextResponse.json<ErrorResponse>(
|
||||
{ error: 'File is outside the project scope' },
|
||||
@@ -32,8 +43,6 @@ export async function GET(request: NextRequest) {
|
||||
} else {
|
||||
// Fallback: without a baseDir, restrict to the user's home directory
|
||||
// to prevent reading arbitrary system files like /etc/passwd
|
||||
const os = require('os');
|
||||
const homeDir = os.homedir();
|
||||
if (!isPathSafe(homeDir, resolvedPath)) {
|
||||
return NextResponse.json<ErrorResponse>(
|
||||
{ error: 'File is outside the allowed scope' },
|
||||
|
||||
@@ -15,21 +15,40 @@ export async function GET(request: NextRequest) {
|
||||
}
|
||||
|
||||
const path = require('path');
|
||||
const os = require('os');
|
||||
const resolvedDir = path.resolve(dir);
|
||||
const homeDir = os.homedir();
|
||||
|
||||
// Use baseDir (the session's working directory) as the trust boundary.
|
||||
// If no baseDir is provided, fall back to the requested directory itself
|
||||
// (preserves backward compatibility while still preventing traversal
|
||||
// when the frontend supplies the session working directory).
|
||||
// The baseDir itself must be under the user's home directory to prevent
|
||||
// attackers from setting baseDir=/ to bypass all restrictions.
|
||||
// If no baseDir is provided, fall back to the user's home directory
|
||||
// to prevent scanning arbitrary system directories.
|
||||
const baseDir = searchParams.get('baseDir');
|
||||
if (baseDir) {
|
||||
const resolvedBase = path.resolve(baseDir);
|
||||
// Ensure baseDir is within the home directory (prevent baseDir=/ bypass)
|
||||
if (!isPathSafe(homeDir, resolvedBase)) {
|
||||
return NextResponse.json<ErrorResponse>(
|
||||
{ error: 'Base directory is outside the allowed scope' },
|
||||
{ status: 403 }
|
||||
);
|
||||
}
|
||||
if (!isPathSafe(resolvedBase, resolvedDir)) {
|
||||
return NextResponse.json<ErrorResponse>(
|
||||
{ error: 'Directory is outside the project scope' },
|
||||
{ status: 403 }
|
||||
);
|
||||
}
|
||||
} else {
|
||||
// Fallback: without a baseDir, restrict to the user's home directory
|
||||
// to prevent scanning arbitrary system directories like /etc
|
||||
if (!isPathSafe(homeDir, resolvedDir)) {
|
||||
return NextResponse.json<ErrorResponse>(
|
||||
{ error: 'Directory is outside the allowed scope' },
|
||||
{ status: 403 }
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
try {
|
||||
|
||||
Reference in new issue
Block a user