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
16 changes: 13 additions & 3 deletions packages/devtools_shared/lib/src/server/file_system.dart
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,24 @@ extension LocalFileSystem on Never {
///
/// Only files within ~/.flutter-devtools/ can be accessed.
static File? devToolsFileFromPath(String pathFromDevToolsDir) {
if (pathFromDevToolsDir.contains('..')) {
if (pathFromDevToolsDir.contains('..') ||
path.isAbsolute(pathFromDevToolsDir)) {
Comment on lines +48 to +49
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

[MUST-FIX] Please add automated tests to verify these security-critical path validation checks.\n\nSince this change fixes a security vulnerability (arbitrary file read via path traversal/absolute paths), it is crucial to have automated tests to prevent regressions.\n\nThe tests should cover:\n- Valid relative paths (should be allowed and return the file if it exists).\n- Absolute paths (should be rejected).\n- Path traversal attempts using .. (should be rejected).\n- Root-relative paths (e.g., starting with / or \\ on Windows, which might bypass path.isAbsolute but should be caught by path.isWithin).

References
  1. All changes should include automated tests to ensure correctness and prevent regressions. (link)

// The passed in path should not be able to walk up the directory tree
// outside of the ~/.flutter-devtools/ directory.
// outside of the ~/.flutter-devtools/ directory. It must also not be an
// absolute path: path.join() discards the base directory when its second
// argument is absolute, which would otherwise allow reading an arbitrary
// file on disk (e.g. an absolute path to a credentials .json file).
return null;
}

ensureDevToolsDirectory();
final file = File(path.join(devToolsDir(), pathFromDevToolsDir));
final devToolsDirPath = devToolsDir();
final file = File(path.join(devToolsDirPath, pathFromDevToolsDir));
// Defense in depth: ensure the resolved path is actually contained within
// the DevTools directory.
if (!path.isWithin(devToolsDirPath, file.path)) {
return null;
}
if (!file.existsSync()) {
return null;
}
Expand Down
38 changes: 38 additions & 0 deletions packages/devtools_shared/test/server/file_system_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Copyright 2026 The Flutter Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd.

import 'package:devtools_shared/src/server/file_system.dart';
import 'package:test/test.dart';

void main() {
group('LocalFileSystem.devToolsFileFromPath path validation', () {
// These inputs must be rejected before any filesystem access so that reads
// stay confined to the ~/.flutter-devtools/ directory.

test('rejects absolute paths', () {
// path.join() discards the base directory when its second argument is
// absolute, so an absolute path would otherwise escape the DevTools
// directory and read an arbitrary file on disk.
expect(LocalFileSystem.devToolsFileFromPath('/etc/passwd'), isNull);
expect(
LocalFileSystem.devToolsFileFromPath(
'/home/user/.config/gcloud/application_default_credentials.json',
),
isNull,
);
});

test('rejects paths containing ".."', () {
expect(LocalFileSystem.devToolsFileFromPath('..'), isNull);
expect(
LocalFileSystem.devToolsFileFromPath('../../../etc/passwd'),
isNull,
);
expect(
LocalFileSystem.devToolsFileFromPath('subdir/../../escape.json'),
isNull,
);
});
});
}