Skip to content

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

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Jun 16, 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 😉)

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
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-"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL. apparently yes.

@github-project-automation github-project-automation bot moved this to To Cherry Pick in 1.23 Servicing Pipeline Jun 16, 2025
@github-project-automation github-project-automation bot moved this to To Cherry Pick in 1.22 Servicing Pipeline Jun 16, 2025
@DHowett DHowett enabled auto-merge (squash) June 16, 2025 22:47
@DHowett DHowett merged commit f28bb42 into main Jun 16, 2025
17 of 19 checks passed
@DHowett DHowett deleted the dev/duhowett/icons-are-less-bad-but-still-pretty-bad-if-i-am-honest branch June 16, 2025 23:01
@DHowett DHowett moved this from To Cherry Pick to Cherry Picked in 1.22 Servicing Pipeline Jun 17, 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 DHowett moved this from To Cherry Pick to Cherry Picked in 1.23 Servicing Pipeline Jun 17, 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_lADOAF3p4s4AxadtzgbjZlM
Service-Version: 1.23
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.
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
Labels
None yet
Projects
Status: Cherry Picked
Status: Cherry Picked
Development

Successfully merging this pull request may close these issues.

Found a profile with an invalid "icon" error We should block resources from web URLs
2 participants