Skip to content

fix(coverage): reject package: and dart: URIs outside known roots in Resolver - #2764

Merged
liamappelbe merged 1 commit into
dart-lang:mainfrom
j0xh-sec:restore-2640
Nov 1, 2026
Merged

fix(coverage): reject package: and dart: URIs outside known roots in Resolver#2764
liamappelbe merged 1 commit into
dart-lang:mainfrom
j0xh-sec:restore-2640

Conversation

@j0xh-sec

@j0xh-sec j0xh-sec commented Oct 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Completes the containment check added in #2534.

#2534 added _isWithinKnownRoots to the file: branch of Resolver.resolve(), on the basis — stated in its own _knownRoots doc comment — that coverage source entries are untrusted and their resolved contents are read into formatter output. The package: and dart: branches were left without the same check, so they can still resolve outside the known roots.

The gap

Plain .. does not traverse, because Uri.parse performs RFC 3986 dot-segment removal before the resolver sees the path. But Uri.pathSegments percent-decodes after that, so an encoded separator survives as a single segment:

package:foo/..%2f..%2foutside.txt  ->  pathSegments = [foo, ../../outside.txt]

packageName stays foo (the package lookup succeeds), and p.join(packagePath, ...) + resolveSymbolicLinks then walk out of the package root. The same applies to dart:. The resolved path's contents are read by format_coverage --pretty-print and filterIgnored, so wherever coverage JSON crosses a trust boundary this reads files outside the roots.

Change

  • Apply the existing _isWithinKnownRoots check to the package: and dart: branches, mirroring file:.
  • Canonicalize the known roots with resolveSymbolicLinks, since the paths they are compared against are canonicalized the same way. Without this, a root reached through a symlink (a symlinked pub cache, or /var -> /private/var on macOS) would never contain its own files and legitimate resolution would start returning null.

Tests

Three added: a package: encoded-separator escape returns null, a dart: escape returns null, and a package: URI through a symlinked root still resolves. dart analyze, dart format --set-exit-if-changed, and the full coverage suite pass.

Notes

@j0xh-sec
j0xh-sec requested review from a team and liamappelbe as code owners October 30, 2026 05:56
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Oct 30, 2026
View reviewed changes

@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 updates Resolver.resolve to apply out-of-roots rejection to package: and dart: URIs, and canonicalizes known roots to ensure paths reached through symlinks still resolve. Corresponding tests have been added to verify these behaviors. Feedback suggests wrapping calls to resolveSymbolicLinks (which performs synchronous I/O operations) in try-catch blocks within _resolveLinksWithinKnownRoots and _createKnownRoots to prevent potential FileSystemException crashes.

Comment thread pkgs/coverage/lib/src/resolver.dart Resolved
Comment thread pkgs/coverage/lib/src/resolver.dart Resolved
@j0xh-sec j0xh-sec mentioned this pull request Oct 30, 2026
Closed
@j0xh-sec

j0xh-sec commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Hi @liamappelbe, I've restored the original changes from #2640, resolved the merge conflicts with the latest main, and addressed the automated review feedback.

All changes have been squashed into a single commit, and the PR is now mergeable.

Thanks again for your earlier review! This should be ready for another look.

liamappelbe
liamappelbe reviewed Oct 30, 2026
View reviewed changes

@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.

Oops. After cleaning up the changelog merge cruft, I realized that v1.16.0 is already published. You'll need to move the changelog entry to v1.16.1-wip, and bump the version in the pubspec to that same version.

Other than that, LGTM.

@liamappelbe

liamappelbe commented Oct 30, 2026

Copy link
Copy Markdown
Contributor

Looks like there's a formatting error that needs fixing.

@bkonyi

bkonyi commented Oct 30, 2026

Copy link
Copy Markdown
Contributor

Still looks like there's some formatting errors.

@j0xh-sec

j0xh-sec commented Nov 1, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @bkonyi, @liamappelbe!

Formatting is fixed in the latest push.

The failing publish / validate check appears to be due to stale tooling rather than this PR. The run from 2026-10-30 activated firehose from the dart-lang/ecosystem tip and failed with Could not find bin/firehose.dart in package firehose, following ecosystem PR #454, which removed the CLI entrypoint. Ecosystem PR #479 restored it on 2026-10-31, so rerunning the job should now pass.

With both approvals in, this should be ready to merge once publish / validate is rerun—or merged as-is, since the failure is unrelated to this change.

Really appreciate the reviews!

@liamappelbe
liamappelbe merged commit 1579f8d into dart-lang:main Nov 1, 2026
28 of 29 checks passed
copybara-service Bot pushed a commit to dart-lang/sdk that referenced this pull request Nov 3, 2026
@natebosch dart-scoped@luci-project-accounts.iam.gserviceaccount.com
[deps] rev core, ecosystem, http, i18n, shelf, test, tools
c48ebb8 
Revisions updated by `dart tools/rev_sdk_deps.dart`.

core (https://github.com/dart-lang/core/compare/d7bd45c..24e09c4):
  24e09c4c  Sun Nov 1 21:12:19 2026 +0000  dependabot[bot]  Bump the github-actions group with 4 updates (dart-lang/core#1037)
  87ac1b4c  Fri Oct 30 16:02:43 2026 +0100  Michael Thomsen  [os_detect] Deprecate package:os_detect in favor of package:platform (dart-lang/core#1003)
  8debf335  Fri Oct 30 01:57:36 2026 +0530  Sahaja Pallapothula  Clarify Characters equality and Unicode normalization (dart-lang/core#1031)

ecosystem (https://github.com/dart-lang/ecosystem/compare/24b9ba8..f8faabf):
  f8faabf  Sun Nov 1 18:16:20 2026 +0000  dependabot[bot]  Bump the github-actions group with 2 updates (dart-lang/ecosystem#481)
  27c2fc8  Sat Oct 31 12:36:53 2026 +0100  Moritz  Fix publish workflow pins, add tag_prefix to health, and update docs (dart-lang/ecosystem#480)
  05641a9  Sat Oct 31 09:56:42 2026 +0100  Moritz  Restore deprecated publish.yaml workflow and firehose CLI entrypoint (dart-lang/ecosystem#479)
  78abf72  Fri Oct 30 08:24:07 2026 -0700  Kevin Moore  fix(repo_manage): use pub_semver for changelog -wip transitions (dart-lang/ecosystem#475)
  0bfa654  Fri Oct 30 09:33:57 2026 +0100  Moritz  Move publish check to health workflow and use individual package publishing workflows from `setup-dart` (dart-lang/ecosystem#454)
  adf7979  Thu Oct 29 23:46:49 2026 -0700  Kevin Moore  test(firehose): use hasLength, containsPair, and isFalse (dart-lang/ecosystem#477)

http (https://github.com/dart-lang/http/compare/c76dfb3..8c0d66a):
  8c0d66a  Fri Oct 30 09:54:41 2026 -0700  Brian Quinlan  chore(cupertino_http): check ffigen generation (dart-lang/http#2044)
  c8737d1  Fri Oct 30 09:11:46 2026 +0100  Moritz  chore(http): run Firefox tests with -headless instead of xvfb-run (dart-lang/http#2033)
  05d0b18  Fri Oct 30 07:08:19 2026 +0100  Moritz  fix(http2): make StreamException a StreamTransportException and forward ALPN errors to onError (dart-lang/http#2009)

i18n (https://github.com/dart-lang/i18n/compare/4c7b4ad..7475ce6):
  7475ce64  Sat Oct 31 02:49:28 2026 +0100  Moritz  [intl4x] Fix NumberFormat.percent ignoring digits (dart-lang/i18n#1115)

shelf (https://github.com/dart-lang/shelf/compare/62d3e4e..9946de5):
  9946de5  Sun Nov 1 12:27:59 2026 +0000  dependabot[bot]  build(deps): bump the github-actions group with 3 updates (dart-lang/shelf#554)

test (https://github.com/dart-lang/test/compare/6d74720..2a9e017):
  2a9e0175  Sun Nov 1 03:47:24 2026 +0000  dependabot[bot]  Bump the github-actions group with 6 updates (dart-lang/test#2813)
  55cd5cd5  Fri Oct 30 13:43:47 2026 -0700  Nate Bosch  Migrate publishing workflows from firehose to setup-dart (dart-lang/test#2812)
  10dc78c0  Fri Oct 30 11:21:53 2026 -0700  Nate Bosch  [checks] Describe what the stream did in failures (dart-lang/test#2811)
  c11ad06d  Fri Oct 30 08:59:57 2026 -0700  Nate Bosch  [checks] Add CheckFailure.describeNested (dart-lang/test#2793)
  0137b43e  Fri Oct 30 05:40:43 2026 +0530  Shriraj Jadhav  [checks] Use the Iterable.last member for the last expectation (dart-lang/test#2807)
  50c9f454  Thu Oct 29 15:11:39 2026 -0700  Nate Bosch  [checks] Collapse clauses for described conditions (dart-lang/test#2794)

tools (https://github.com/dart-lang/tools/compare/3980dca..1579f8d):
  1579f8d69  Mon Nov 2 02:44:58 2026 +0530  Joshua  fix(coverage): reject package: and dart: URIs outside known roots in Resolver (dart-lang/tools#2764)
  f869fdca4  Sun Nov 1 10:26:34 2026 +0100  dart-ecosystem-bot[bot]  [boring] Update BoringSSL to ed9209cf (dart-lang/tools#2774)
  d10e2b87f  Sun Nov 1 10:26:11 2026 +0100  dart-ecosystem-bot[bot]  [boring] Pin the prebuilt binaries of boring-binaries-20261101-33e9e69722af (dart-lang/tools#2777)
  33e9e6972  Sun Nov 1 05:56:30 2026 +0000  dependabot[bot]  build(deps): bump the github-actions group with 4 updates (dart-lang/tools#2775)
  f84d0d0d2  Sun Nov 1 05:36:12 2026 +0000  dependabot[bot]  build(deps): bump ffigen from 22.0.0 to 23.0.0 in /pkgs/boring (dart-lang/tools#2776)
  14ad5e515  Fri Oct 30 10:48:58 2026 -0700  Kevin Moore  [mime] Harden BoundMultipartStream error routing, headers, and epilogue (dart-lang/tools#2629)
  7a780d48c  Fri Oct 30 12:57:03 2026 +0100  Jonas Finnemann Jensen  [markdown] Avoid rescanning failed plain-text runs in InlineParser (dart-lang/tools#2757)
  3671d33a9  Fri Oct 30 12:56:48 2026 +0100  Jonas Finnemann Jensen  [markdown] Index footnote definitions and labels on Document for O(1) lookup (dart-lang/tools#2760)
  6f38e9fda  Fri Oct 30 12:56:09 2026 +0100  Jonas Finnemann Jensen  [markdown] Bound the domain of extended URL autolinks to DNS limits (dart-lang/tools#2759)
  aa0c38544  Fri Oct 30 12:20:40 2026 +0100  Jonas Finnemann Jensen  [markdown] Avoid quadratic removeRange in _combineAdjacentText (dart-lang/tools#2758)
  f425baf46  Fri Oct 30 11:16:28 2026 +0100  Jonas Finnemann Jensen  [markdown] Avoid rescanning failed space runs in LineBreakSyntax (dart-lang/tools#2756)
  c7eafbec3  Fri Oct 30 11:09:45 2026 +0100  Jonas Finnemann Jensen  [markdown] Reject '<' in AutolinkSyntax (dart-lang/tools#2755)
  86ed0f902  Fri Oct 30 11:08:25 2026 +0100  Jonas Finnemann Jensen  [markdown] Avoid quadratic backtracking in headerPattern (dart-lang/tools#2754)
  8ea07517f  Fri Oct 30 07:45:42 2026 +0100  Moritz  [boring] Regenerate the x509-limbo expected failures in the refresh job (dart-lang/tools#2712)
  40235c365  Thu Oct 29 14:46:17 2026 +0100  Jonas Finnemann Jensen  [markdown] Make HtmlBlockSyntax.parseChildLines iterative (dart-lang/tools#2749)
  0ff03f643  Thu Oct 29 14:41:33 2026 +0100  Jonas Finnemann Jensen  [markdown] Avoid re-escaping nested image alt text in ImageSyntax.createNode (dart-lang/tools#2752)
  784654d01  Thu Oct 29 14:37:11 2026 +0100  Jonas Finnemann Jensen  [markdown] Add test cases covering parser and renderer edge cases (dart-lang/tools#2753)
  4f2718ad8  Thu Oct 29 14:36:52 2026 +0100  Jonas Finnemann Jensen  [markdown] Make Document._parseInlineContent iterative (dart-lang/tools#2750)
  cac1421d5  Thu Oct 29 14:17:06 2026 +0100  Jonas Finnemann Jensen  [markdown] Avoid quadratic duplicate heading ID probing in HtmlRenderer (dart-lang/tools#2748)
  66880bcd3  Thu Oct 29 14:11:27 2026 +0100  Jonas Finnemann Jensen  [markdown] Check footnote label and empty map before scanning footnote keys (dart-lang/tools#2751)
  7f8696523  Thu Oct 29 14:07:06 2026 +0100  Jonas Finnemann Jensen  [markdown] Avoid quadratic blank-line lookahead in CodeBlockSyntax (dart-lang/tools#2747)
  9da6bb291  Thu Oct 29 14:03:04 2026 +0100  Jonas Finnemann Jensen  [markdown] Avoid exponential and quadratic backtracking in tablePattern (dart-lang/tools#2746)

dartdoc is held back at db94437, before dart-lang/dartdoc#4312, because its
mock SDK version changes conflict in the google3 3-way merge with the hotfix
in cl/1005963642.

native is held back at e0ca7c6, before dart-lang/native#3742, because
deprecating `MacOSCodeConfig(targetVersion: ...)` causes a fatal
`deprecated_member_use` diagnostic in `pkg/dartdev/lib/src/native_assets.dart`.

web is held back at 4e60e61, before dart-lang/web#596, because it changes
`package:web` signatures (e.g. `innerHTML=`, `parseFromString`, `Worker()`)
from `JSAny` to `String` in ways that break callers in google3.

Change-Id: I53d2cbde741c45b8fd33bca42742549819dd332b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/572780
Reviewed-by: Brian Quinlan <[email protected]>
Commit-Queue: Nate Bosch <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liamappelbe liamappelbe liamappelbe approved these changes
@bkonyi bkonyi bkonyi approved these changes
+1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

package:coverage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@j0xh-sec @liamappelbe @bkonyi