Repository navigation
Precompiled browser tests: stop serving files from outside the precompiled root - #2724
Conversation
…piled root PrecompiledSupport created its static file handler as createStaticHandler(_root, serveFilesOutsidePath: true), disabling shelf_static's path-containment check. The sibling dart2js and dart2wasm compilers use createStaticHandler(_root) (containment on), so precompiled mode was the only browser-test server that served files from outside its root in response to a `..`-bearing request. No browser-fetched resource needs out-of-root serving: the compiled bundles live under the precompiled root, /packages/ is served by packagesDirHandler, and source-map stack mapping is done server-side (the browser never fetches the .map or .dart sources). Removing the flag restores path containment and brings precompiled mode to parity with dart2js/dart2wasm. Includes a CHANGELOG entry.
|
This does look like it was intentionally added, although there wasn't any reason listed in the PR (#2170). I did mention package:build tests in that PR, so I can do a quick check to see if this will somehow break package:build. I do wonder if this is related to symlinks in particular. But, if it doesn't break that package or internal tests, we can land it. |
|
Thanks for looking, and good point on symlinks. That is a real gap in my "breaks nothing" claim: serveFilesOutsidePath also gates files reached through a symlink whose target resolves outside _root, not just literal Happy to wait for your package:build and internal-test check. If those pass, landing the removal is the simplest fix. If they show symlinks are load-bearing, I can rework this to keep following symlinks but validate that the resolved real path stays within an allow-list (the precompiled root plus the known package roots), which blocks the out-of-tree read while preserving legitimate in-package symlinks. Let me know which direction you prefer once you have the results. |
|
I am pretty sure this is safe to land, cc @natebosch do you have concerns? It appears internally we don't use this specific code path. |
|
Thanks! |
Revisions updated by `dart tools/rev_sdk_deps.dart test`. test (https://github.com/dart-lang/test/compare/55b186d..18ef597): 18ef5978 Fri Sep 4 10:13:39 2026 -0700 Nate Bosch Add support for collapsed error messages (dart-lang/test#2651) 6783445c Wed Sep 2 16:46:04 2026 -0700 Nate Bosch Prepare to publish (dart-lang/test#2737) 8de693e1 Tue Sep 1 03:54:15 2026 +0000 dependabot[bot] Bump the github-actions group with 2 updates (dart-lang/test#2744) 575e4dad Mon Aug 31 17:01:46 2026 -0700 Nate Bosch Fix deadlock waiting on serverSocket (dart-lang/test#2740) bd9acf32 Thu Aug 27 22:53:54 2026 -0700 Nate Bosch Unskip a test on windows (dart-lang/test#2741) b575eda9 Thu Aug 27 17:16:16 2026 -0700 Nate Bosch Remove redundant tag (dart-lang/test#2738) d21b06cb Thu Aug 27 16:38:17 2026 -0700 dependabot[bot] Bump the github-actions group across 1 directory with 5 updates (dart-lang/test#2709) 773bc56c Thu Aug 27 16:11:00 2026 -0700 Nate Bosch Use read permissions in test_descriptor and test_process workflows (dart-lang/test#2736) 73e85f1b Wed Aug 26 06:36:41 2026 +0700 herdiyanitdev Precompiled browser tests: stop serving files from outside the precompiled root (dart-lang/test#2724) bbe8f9bf Tue Aug 18 09:02:24 2026 -0700 Kevin Moore Export TestFailure from package:test_api/scaffolding.dart (dart-lang/test#2723) TAG=agy CONV=e6fd1c32-ded3-443f-83e5-2e15b4ffaccb [email protected] Change-Id: Iabc67d46c413470f0d2e5341e719d4edf702a94b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/546020 Commit-Queue: Nate Bosch <[email protected]> Auto-Submit: Nate Bosch <[email protected]> Reviewed-by: Kevin Moore <[email protected]> Commit-Queue: Kevin Moore <[email protected]>
Fixes #2766 #2724 removed `serveFilesOutsidePath: true` from the precompiled static handler to stop requests from reading files outside the precompiled directory. Without that flag `shelf_static` also refuses to serve any symlink whose target resolves outside the root. `build_runner test` defaults to `--symlink` on non-Windows platforms. That creates a merged output directory where every file is a symlink into `.dart_tool/build` or the package sources, so every request for a compiled test returned 404 and tests failed to load with "Failed to load script at ...". Check containment on the requested path instead of the symlink-resolved path. The new `createSymlinkFollowingStaticHandler` joins the request path segments onto the root the same way `shelf_static` does, rejects the request if the normalized result is outside the root, and otherwise delegates to a static handler with `serveFilesOutsidePath: true`. Symlinks placed inside the precompiled directory are followed. Traversal through the request path, such as `..%2F` segments or an encoded absolute path, is still rejected. Add unit tests that send raw HTTP requests to cover symlinked files and directories and several traversal forms. With the containment check disabled, the encoded-slash and absolute-path cases serve the outside file. Add a Chrome integration test that runs a precompiled test from a directory of symlinks; it fails with the previous handler.
The precompiled browser-test server serves files from outside its root because it disables shelf_static's path containment.
pkgs/test/lib/src/runner/browser/compilers/precompiled.dartbuilds its static handler as:The two sibling compilers in the same directory build the same handler without that flag:
With
serveFilesOutsidePath: true, shelf_static stops rejecting requests whose path resolves outside_root, so a..-bearing request to the precompiled server is served a file from anywhere on disk the runner can read. Precompiled is the only one of the three that does this.Removing the flag does not change what the browser can load. In precompiled mode
_rootis the precompiled output directory itself (platform.dartpassesroot: _config.suiteDefaults.precompiledPath), so the compiled.jsbundles and their bootstrap.htmlalready live under it./packages/requests are served by the separatepackagesDirHandleralready in the cascade. Source maps are never fetched by the browser:compileSuitereads the.js.mapserver-side withFile(mapPath).readAsStringSync()and builds aJSStackTraceMapperin the runner process. Nothing the browser requests needs a path outside_root, which is why dart2js and dart2wasm already serve an equivalent root with containment on.The server binds to loopback and nests every handler under a per-run secret URL segment, and the comment on that secret says its job is to keep other users on the same machine from snooping on what the server serves. The secret does not actually hide from a co-located user: the full server URL is passed to the browser on its command line, so another local user can read it from
/proc/<pid>/cmdline. With containment on that only lets them read within_root; withserveFilesOutsidePath: trueit lets them read any file the runner user can read, for example/<secret>/../../../home/<user>/.ssh/id_rsa. Removing the flag restores the boundary the secret is meant to enforce.The fix drops the flag so precompiled matches the other two compilers:
I was not able to run the toolchain in my environment, so I have not attached a live run or a formatter/analyzer pass. The change removes an over-permissive flag that no browser-fetched resource depends on and that dart2js and dart2wasm already omit. I am happy to add an integration test that asserts the precompiled server rejects a
..traversal if you would like one. Includes a CHANGELOG entry under1.32.0-wip.I've reviewed the contributor guide and applied the relevant portions to this PR.