[ Tool ] Throw ToolExit when asset entries use absolute paths
#174230
Conversation
Our documentation states that "the value of an asset is a relative path from the pubspec.yaml file", but this was never actually verified. On systems with POSIX semantics, this would simply result in invalid asset paths being built, but on Windows this could cause an exception to be thrown as the built URI would not be a valid `file://` URI. This change adds checks to ensure that asset paths are relative and that they are valid file paths. Fixes #173405
There was a problem hiding this comment.
Code Review
This pull request introduces validation to ensure that asset paths in pubspec.yaml are relative, as documented. A new function, ensureAssetPathIsValid, is added to check for absolute paths and unsupported URI schemes, providing clearer error messages and preventing potential tool crashes, particularly on Windows. The URI parsing for assets has been made more robust by switching to Uri.parse. The changes are well-supported by a comprehensive set of new and updated tests that cover various scenarios across different platforms. The implementation looks solid and effectively addresses the issue.
| windows: _platform.isWindows, | ||
| ).resolveUri(assetUri).toFilePath(windows: _platform.isWindows) == | ||
| assetUri.toFilePath(windows: _platform.isWindows)) { | ||
| throwToolExit( |
There was a problem hiding this comment.
I'm supportive of this change, but I do wonder if it ends up being a breaking change in practice and we could consider relaxing this (making it a warning) for a single release before making it a hard error.
On the other hand if it's very corner-casey, and we don't expect it was used much (or much intentionally), no problem doing this IMO.
There was a problem hiding this comment.
I don't think this ever actually worked, it was just surfaced as a different error.
We basically take the base directory (the directory that contains the pubspec.yaml) and then do a path join with the asset path from the pubspec. Absolute asset paths on Windows would have never worked due to the presence of a drive letter in the path, and would result in an asset not found error on POSIX file systems. There may be a couple of edge cases where this did happen to work, but I'd consider that a bug.
The exception message gives actionable feedback on how to fix this issue, and the fix is "relative"-ly simple, so I think it's worth just landing this as-is.
|
autosubmit label was removed for flutter/flutter/174230, because - The status or check suite Windows tool_tests_general has failed. Please fix the issues identified (or deflake) before re-applying this label.
|
…ter#174230) Our documentation states that "the value of an asset is a relative path from the pubspec.yaml file", but this was never actually verified by the tool. On systems with POSIX semantics, this would simply result in invalid asset paths being built, but on Windows this could cause an exception to be thrown as the built URI would not be a valid `file://` URI. This change adds checks to ensure that asset paths are relative and that they are valid file paths. Fixes flutter#173405
…ter#174230) Our documentation states that "the value of an asset is a relative path from the pubspec.yaml file", but this was never actually verified by the tool. On systems with POSIX semantics, this would simply result in invalid asset paths being built, but on Windows this could cause an exception to be thrown as the built URI would not be a valid `file://` URI. This change adds checks to ensure that asset paths are relative and that they are valid file paths. Fixes flutter#173405
flutter/flutter@d2ac021...26bb33b 2025-08-22 34871572+gmackall@users.noreply.github.com [HCPP] Clean up overlay layer when last frame had overlay content and current doesn't (flutter/flutter#173881) 2025-08-22 32538273+ValentinVignal@users.noreply.github.com Migrate more files to use WidgetStateProperty (flutter/flutter#174176) 2025-08-22 engine-flutter-autoroll@skia.org Roll Skia from 75fef9fb3ed7 to cb15e1452399 (1 revision) (flutter/flutter#174255) 2025-08-22 engine-flutter-autoroll@skia.org Roll Skia from 006241a7fbe1 to 75fef9fb3ed7 (1 revision) (flutter/flutter#174254) 2025-08-22 biggs0125@gmail.com Skip wasm build when dry run is disabled and --wasm is not specified. (flutter/flutter#174184) 2025-08-22 engine-flutter-autoroll@skia.org Roll Dart SDK from c153c5259e62 to 4f9623f024ab (2 revisions) (flutter/flutter#174250) 2025-08-22 engine-flutter-autoroll@skia.org Roll Skia from d70087007490 to 006241a7fbe1 (2 revisions) (flutter/flutter#174252) 2025-08-22 engine-flutter-autoroll@skia.org Roll Skia from c09589f7ca69 to d70087007490 (22 revisions) (flutter/flutter#174245) 2025-08-21 bkonyi@google.com [ Widget Preview ] Add regression test for issue 173895 (flutter/flutter#174037) 2025-08-21 okorohelijah@google.com Improve xcresult comment and naming (flutter/flutter#173129) 2025-08-21 matanlurey@users.noreply.github.com Update `.gemini/styleguide.md` to encourage `master`-only (flutter/flutter#174065) 2025-08-21 bkonyi@google.com [ Widget Preview ] Fix crash when attempting to provide non-const params to a `Preview` (flutter/flutter#174242) 2025-08-21 engine-flutter-autoroll@skia.org Roll Skia from 721e68fe652a to c09589f7ca69 (12 revisions) (flutter/flutter#174162) 2025-08-21 engine-flutter-autoroll@skia.org Roll Dart SDK from 0d0a0c394381 to c153c5259e62 (7 revisions) (flutter/flutter#174235) 2025-08-21 bkonyi@google.com [ Tool ] Throw `ToolExit` when asset entries use absolute paths (flutter/flutter#174230) 2025-08-21 engine-flutter-autoroll@skia.org Roll Dart SDK from 0d0a0c394381 to c153c5259e62 (7 revisions) (flutter/flutter#174227) 2025-08-21 bkonyi@google.com [ Tool ] Cleanup widget preview and frontend server shutdown (flutter/flutter#173863) 2025-08-21 matanlurey@users.noreply.github.com Use an alternative to `git describe` for `master` version resolution (flutter/flutter#174088) 2025-08-21 matt.kosarek@canonical.com Report a correct display ID in the window metrics event on win32 (flutter/flutter#174156) 2025-08-21 47866232+chunhtai@users.noreply.github.com Revert "Update the AccessibilityPlugin::Announce method to account fo… (flutter/flutter#174223) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Our documentation states that "the value of an asset is a relative path from the pubspec.yaml file", but this was never actually verified by the tool. On systems with POSIX semantics, this would simply result in invalid asset paths being built, but on Windows this could cause an exception to be thrown as the built URI would not be a valid
file://URI.This change adds checks to ensure that asset paths are relative and that they are valid file paths.
Fixes #173405