Skip to content

fix(coverage): reject file: URIs outside known roots in Resolver - #2534

Merged
liamappelbe merged 4 commits into
dart-lang:mainfrom
herdiyana256:fix-coverage-file-uri-arbitrary-read
Aug 24, 2026
Merged

liamappelbe merged 4 commits into
dart-lang:mainfrom
herdiyana256:fix-coverage-file-uri-arbitrary-read

Conversation

@herdiyana256

Copy link
Copy Markdown
Contributor

Resolver.resolve handled the file: scheme by resolving the URI straight to a path and returning it, with no check that the path was anywhere near the project being covered:

if (uri.scheme == 'file') {
  return resolveSymbolicLinks(p.fromUri(uri));
}

The source field in coverage data is supplied by whatever produced the coverage JSON, and format_coverage --pretty-print (formatter.dart) reads the resolved path's content into its output:

final source = resolver.resolve(entry.key);
...
final lines = await loader.load(source);
...
buf.writeln('$prefix|${lines[line - 1]}');

So a coverage.json containing "source": "file:///etc/passwd" gets that file's content read and printed straight into the generated coverage report. I verified this by running the real format_coverage CLI against a crafted coverage.json pointing at a file outside the project. The file's content appeared in the pretty-printed report, unmodified.

The package: and dart: branches aren't affected. They resolve relative to a known package or SDK root, and a package: URI can't traverse out either, since Uri.parse already collapses .. segments in the path before Resolver ever sees them (Uri.parse('package:foo/../../bar.dart').pathSegments is [bar.dart], not [.., .., bar.dart]).

The fix adds a containment check for the file: branch, allowing a resolved path only if it falls under packagePath, sdkRoot, one of the known package roots, or the current working directory:

if (uri.scheme == 'file') {
  final resolved = resolveSymbolicLinks(p.fromUri(uri));
  if (resolved == null || !_isWithinKnownRoots(resolved)) return null;
  return resolved;
}

null is already an existing, handled outcome elsewhere in this method for URIs it can't resolve, so callers don't need any changes.

Added a test that constructs a file outside every one of those roots and confirms it no longer resolves, and a companion test confirming a file: URI inside packagePath still resolves normally. I confirmed the new rejection test fails without the fix and passes with it, by temporarily reverting just the resolve method and rerunning.

dart analyze and dart format --set-exit-if-changed are clean on both touched files, and the full pkgs/coverage test suite (139 tests) passes.

Resolver.resolve handled the file: scheme by resolving the URI straight
to a path and returning it, with no check that the path was anywhere
near the project being covered. The source field in coverage data is
supplied by whatever produced the coverage JSON; format_coverage
--pretty-print and filterIgnored both read the resolved path's content
into their output, so a coverage.json containing
"source": "file:///etc/passwd" gets that file's content read and
printed straight into the generated coverage report.

Verified by running the real format_coverage CLI against a crafted
coverage.json pointing at a file outside the project: the file's
content appeared in the pretty-printed report. The package: and dart:
branches aren't affected, since they resolve relative to a known
package or SDK root; a package: URI can't traverse out either, since
Uri.parse already collapses .. segments in the path before Resolver
ever sees them.

The fix adds a containment check for the file: branch, allowing a
resolved path only if it falls under packagePath, sdkRoot, one of the
known package roots, or the current working directory. Falls back to
null (already an existing, handled outcome for unresolvable URIs)
otherwise. Added a test that constructs a file outside every one of
those roots and confirms it no longer resolves, and a companion test
confirming a file: URI inside packagePath still resolves normally.
Confirmed the new rejection test fails without the fix and passes with
it.

dart analyze and dart format --set-exit-if-changed are clean on both
touched files, and the full pkgs/coverage test suite (139 tests) passes.
@herdiyana256
herdiyana256 requested review from a team and liamappelbe as code owners August 18, 2026 17:37

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request enhances security in the coverage resolver by restricting resolved file: URIs to known roots (such as the package path, SDK root, package dependencies, or current directory) to prevent arbitrary file read vulnerabilities. It also adds corresponding unit tests. Feedback includes fixing a compilation error in Dart caused by invalid syntax (?packagePath and ?sdkRoot) by using conditional collection if statements, and simplifying the test setup by utilizing d.sandbox for temporary file creation instead of manually managing a system temp directory.

Comment thread pkgs/coverage/lib/src/resolver.dart Resolved
Comment thread pkgs/coverage/test/resolver_test.dart Resolved
Use if (x != null) x instead of the null-aware ?x collection element
syntax, matching the reviewer's suggested style. Build the outside-root
test file under d.sandbox instead of a manually created and torn down
system temp directory, since d.sandbox is already managed and cleaned
up by the test framework.
@herdiyana256

Copy link
Copy Markdown
Contributor Author

Pushed 64d1382 addressing both comments: the null-aware ?x elements are now plain if (x != null) x checks, and the outside-root test file is created under d.sandbox instead of a manually managed temp directory. dart analyze, dart format --set-exit-if-changed, and the full pkgs/coverage suite (139 tests) are all clean on the updated branch.

@liamappelbe liamappelbe left a comment

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.

Looks good. Can you add a short changelog entry?

Comment thread pkgs/coverage/lib/src/resolver.dart Resolved
expect(resolver.resolve('thing:foo/foo.dart'), null);
});

test('resolves file URIs within a known root', () async {

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.

Can you add tests covering the other _knownRoots?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added, pushed in 0a2c07a. Three new tests: sdkRoot, a known package root from package_config.json, and the current directory.

Also pushed a changelog entry, and reverted the null-aware ?x syntax back from the if (x != null) x form. CI runs dart analyze --fatal-infos, and this package's own lints flag the if form as info-level (use_null_aware_elements), which was failing that check even though it type-checks fine.

Adds a changelog entry for the file: containment fix.

Adds tests covering the other roots _isWithinKnownRoots checks: sdkRoot,
a known package root from package_config.json, and the current directory.
Only packagePath had a positive test before.

Reverts the null-aware ?packagePath/?sdkRoot elements back from the
if (x != null) x form requested in an earlier review comment. CI runs
dart analyze --fatal-infos, and this package's own lints recommend the
null-aware form (use_null_aware_elements), so the if form was failing
analyze in CI even though it type-checks fine locally without the flag.
@herdiyana256

Copy link
Copy Markdown
Contributor Author

Pushed 0a2c07a covering both review comments plus the CI failures:

  • Changelog entry added under 1.16.0-wip.
  • Three new tests for sdkRoot, a known package root, and the current directory (only packagePath had a positive test before).
  • Reverted ?packagePath/?sdkRoot from the if (x != null) x form back to the null-aware elements. CI's dart analyze --fatal-infos treats this package's own use_null_aware_elements lint as fatal, so the if form was the actual cause of the analyze (dev) failure.

dart analyze --fatal-infos, dart format --set-exit-if-changed, and the full pkgs/coverage suite (142 tests) are all clean.

@liamappelbe

Copy link
Copy Markdown
Contributor

Oops, forgot to hit merge.

@liamappelbe
liamappelbe merged commit 3de16a5 into dart-lang:main Aug 24, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants