-
Notifications
You must be signed in to change notification settings - Fork 8.7k
Gently rework icon and background image validation #19044
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
DHowett
merged 2 commits into
main
from
dev/duhowett/icons-are-less-bad-but-still-pretty-bad-if-i-am-honest
Jun 16, 2025
Merged
Gently rework icon and background image validation #19044
DHowett
merged 2 commits into
main
from
dev/duhowett/icons-are-less-bad-but-still-pretty-bad-if-i-am-honest
Jun 16, 2025
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Right now, image validation accepts web-sourced icons (boo) and rejects images whose paths begin with `\\?\`. In addition, it will warn the user for things out of their control like images set by fragments. This pull request adds a filesystem path validator (which accepts images with fully-qualified paths and UNC paths), makes the URI validator reject any web-origin URIs (only `file` and `ms-*` are allowable), and suppresses warnings for any images that were not _directly_ set by the user. Since we want to avoid using fragment images that fail validation, we no longer `Clear` each image property but rather set it to the blank or fallback value. This does **not** actually add support for images at absolute paths beginning with `\\?\`. Such images are still rejected by `Image` and the other XAML fixtures we use for images. It's better than a warning, though. Closes #18703
lhecker
reviewed
Jun 16, 2025
const auto scheme{ resourceUri.SchemeName() }; | ||
std::wstring_view schemeSv{ scheme }; | ||
// Only file: URIs and ms-* URIs are permissible. http, https, ftp, gopher, etc. are not. | ||
return til::equals_insensitive_ascii(scheme, L"file") || (schemeSv.size() > 3 && til::equals_insensitive_ascii(schemeSv.substr(0, 3), L"ms-")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOL. apparently yes.
lhecker
approved these changes
Jun 16, 2025
DHowett
added a commit
that referenced
this pull request
Jun 17, 2025
Right now, image validation accepts web-sourced icons (boo) and rejects images whose paths begin with `\\?\`. In addition, it will warn the user for things out of their control like images set by fragments. This pull request adds a filesystem path validator (which accepts images with fully-qualified paths and UNC paths), makes the URI validator reject any web-origin URIs (only `file` and `ms-*` are allowable), and suppresses warnings for any images that were not _directly_ set by the user. Since we want to avoid using fragment images that fail validation, we no longer `Clear` each image property but rather set it to the blank or fallback value. This does **not** actually add support for images at absolute paths beginning with `\\?\`. Such images are still rejected by `Image` and the other XAML fixtures we use for images. It's better than a warning, though. Closes #18703 Closes #14143 Refs #18710 Refs #5204 Related to #18922 (http-origin icons will be blank everywhere and not just the jump list ;)) (cherry picked from commit f28bb42) Service-Card-Id: PVTI_lADOAF3p4s4AmhmQzgbjZlQ Service-Version: 1.22
DHowett
added a commit
that referenced
this pull request
Jun 17, 2025
Right now, image validation accepts web-sourced icons (boo) and rejects images whose paths begin with `\\?\`. In addition, it will warn the user for things out of their control like images set by fragments. This pull request adds a filesystem path validator (which accepts images with fully-qualified paths and UNC paths), makes the URI validator reject any web-origin URIs (only `file` and `ms-*` are allowable), and suppresses warnings for any images that were not _directly_ set by the user. Since we want to avoid using fragment images that fail validation, we no longer `Clear` each image property but rather set it to the blank or fallback value. This does **not** actually add support for images at absolute paths beginning with `\\?\`. Such images are still rejected by `Image` and the other XAML fixtures we use for images. It's better than a warning, though. Closes #18703 Closes #14143 Refs #18710 Refs #5204 Related to #18922 (http-origin icons will be blank everywhere and not just the jump list ;)) (cherry picked from commit f28bb42) Service-Card-Id: PVTI_lADOAF3p4s4AxadtzgbjZlM Service-Version: 1.23
This was referenced Jun 28, 2025
dhruvinsh
added a commit
to dhruvinsh/dotfiles
that referenced
this pull request
Jul 4, 2025
Reference: microsoft/terminal#19044 Remove unused or unnecessary icon fields to clean up the settings template.
This was referenced Jul 16, 2025
DHowett
added a commit
that referenced
this pull request
Jul 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Right now, image validation accepts web-sourced icons (boo) and rejects images whose paths begin with
\\?\
. In addition, it will warn the user for things out of their control like images set by fragments.This pull request adds a filesystem path validator (which accepts images with fully-qualified paths and UNC paths), makes the URI validator reject any web-origin URIs (only
file
andms-*
are allowable), and suppresses warnings for any images that were not directly set by the user.Since we want to avoid using fragment images that fail validation, we no longer
Clear
each image property but rather set it to the blank or fallback value.This does not actually add support for images at absolute paths beginning with
\\?\
. Such images are still rejected byImage
and the other XAML fixtures we use for images. It's better than a warning, though.Closes #18703
Closes #14143
Refs #18710
Refs #5204
Related to #18922 (http-origin icons will be blank everywhere and not just the jump list 😉)