Repository navigation
fix(coverage): reject file: URIs outside known roots in Resolver - #2534
liamappelbe merged 4 commits into
Conversation
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.
There was a problem hiding this comment.
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.
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.
|
Pushed 64d1382 addressing both comments: the null-aware |
liamappelbe
left a comment
There was a problem hiding this comment.
Looks good. Can you add a short changelog entry?
| expect(resolver.resolve('thing:foo/foo.dart'), null); | ||
| }); | ||
|
|
||
| test('resolves file URIs within a known root', () async { |
There was a problem hiding this comment.
Can you add tests covering the other _knownRoots?
There was a problem hiding this comment.
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.
|
Pushed 0a2c07a covering both review comments plus the CI failures:
|
|
Oops, forgot to hit merge. |
Resolver.resolvehandled thefile: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
sourcefield in coverage data is supplied by whatever produced the coverage JSON, andformat_coverage --pretty-print(formatter.dart) reads the resolved path's content into its output: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 realformat_coverageCLI 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:anddart:branches aren't affected. They resolve relative to a known package or SDK root, and apackage:URI can't traverse out either, sinceUri.parsealready collapses..segments in the path beforeResolverever sees them (Uri.parse('package:foo/../../bar.dart').pathSegmentsis[bar.dart], not[.., .., bar.dart]).The fix adds a containment check for the
file:branch, allowing a resolved path only if it falls underpackagePath,sdkRoot, one of the known package roots, or the current working directory:nullis 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 insidepackagePathstill resolves normally. I confirmed the new rejection test fails without the fix and passes with it, by temporarily reverting just theresolvemethod and rerunning.dart analyzeanddart format --set-exit-if-changedare clean on both touched files, and the fullpkgs/coveragetest suite (139 tests) passes.