DYN-9862 UnTrusted Paths Notifications #16752
Conversation
If TrustedLocations list contain paths pointing to ProgramData, we will be logging a warning notification (also in console but that functionality already existed). Also added resource strings for localization of messages shown in the notification.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-9862
There was a problem hiding this comment.
Pull request overview
This PR adds a security warning notification when trusted locations point to potentially unsafe paths in ProgramData. The implementation logs a warning when any trusted location starts with the CommonApplicationData path, helping users identify potential security risks in their Dynamo configuration.
Key Changes:
- Added localized resource strings for unsafe path detection notifications
- Implemented validation logic to detect and warn about trusted locations in ProgramData
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Notifications/Properties/Resources.resx | Added resource strings for "No notifications" UI text and unsafe path warning messages |
| src/Notifications/Properties/Resources.en-US.resx | Added English-specific resource strings for unsafe path detection warnings |
| src/Notifications/NotificationsViewExtension.cs | Implemented validation logic to check trusted locations against ProgramData path and display warnings |
Files not reviewed (1)
- src/Notifications/Properties/Resources.Designer.cs: Language not supported
| string programDataPath = Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData); | ||
| foreach (var location in viewModel.Model.PreferenceSettings.TrustedLocations) | ||
| { | ||
| if (location.StartsWith(programDataPath)) | ||
| { | ||
| Notifications.Add(new NotificationMessage("Preference Settings", Properties.Resources.UnsafePathDetectedTitle, Properties.Resources.UnsafePathDetectedDetail + "\n" + location)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The path comparison using StartsWith() may produce false positives on case-sensitive file systems or when paths have different separator formats. Use Path.GetFullPath() to normalize both paths before comparison, and consider using StringComparison.OrdinalIgnoreCase for the comparison to handle case differences on Windows.
| foreach (var location in viewModel.Model.PreferenceSettings.TrustedLocations) | ||
| { | ||
| if (location.StartsWith(programDataPath)) | ||
| { | ||
| Notifications.Add(new NotificationMessage("Preference Settings", Properties.Resources.UnsafePathDetectedTitle, Properties.Resources.UnsafePathDetectedDetail + "\n" + location)); | ||
| } | ||
| } | ||
| notificationsMenuItem.NotificationsChangeHandler.Invoke(this, null); |
There was a problem hiding this comment.
The NotificationsChangeHandler.Invoke() call is placed outside the loop, which means it will be called even when no unsafe paths are detected. This should be moved inside the conditional block (after line 122) or wrapped in a check to only invoke when notifications were actually added.
| foreach (var location in viewModel.Model.PreferenceSettings.TrustedLocations) | |
| { | |
| if (location.StartsWith(programDataPath)) | |
| { | |
| Notifications.Add(new NotificationMessage("Preference Settings", Properties.Resources.UnsafePathDetectedTitle, Properties.Resources.UnsafePathDetectedDetail + "\n" + location)); | |
| } | |
| } | |
| notificationsMenuItem.NotificationsChangeHandler.Invoke(this, null); | |
| bool notificationAdded = false; | |
| foreach (var location in viewModel.Model.PreferenceSettings.TrustedLocations) | |
| { | |
| if (location.StartsWith(programDataPath)) | |
| { | |
| Notifications.Add(new NotificationMessage("Preference Settings", Properties.Resources.UnsafePathDetectedTitle, Properties.Resources.UnsafePathDetectedDetail + "\n" + location)); | |
| notificationAdded = true; | |
| } | |
| } | |
| if (notificationAdded) | |
| { | |
| notificationsMenuItem.NotificationsChangeHandler.Invoke(this, null); | |
| } |
|
Adding GIF showing the expected behavior when having ProgramData in Trusted paths and when not having it. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Refactoring code for preventing false positives in the StartsWith call
re-trigger job
|
Hi team, do you have any comment about this changes? |
1 participant
Purpose
If TrustedLocations list contain paths pointing to ProgramData, we will be logging a warning notification (also in console but that functionality already existed).
Also added resource strings for localization of messages shown in the notification (and adding missing localization resources).
Declarations
Check these if you believe they are true
Release Notes
If TrustedLocations list contain paths pointing to ProgramData, we will be logging a warning notification (also in console but that functionality already existed).
Reviewers
@zeusongit @reddyashish
FYIs
@jnealb