DYN-10745: Block non-built-in Autodesk Assistant / DynamoMCP instances - #17278
DYN-10745: Block non-built-in Autodesk Assistant / DynamoMCP instances#17278jasonstratton wants to merge 3 commits into
Conversation
Dynamo 4.2 ships Autodesk Assistant and DynamoMCP as built-in packages for the first time. Older copies from the alpha program can still be present on disk (as a manually-installed DynamoMCP package, or as an orphaned Autodesk-Assistant view extension left behind by a Dynamo-for- Revit uninstall) and can silently displace the shipped built-in copy. Add LegacyAssistantExtensionGuard, a shared path-based check (the only reliable signal, since package names and signing do not discriminate alpha copies from built-in ones) used by two gates: PackageLoader refuses to load either package from outside Built-In Packages, and ViewExtensionLoader refuses to load either view extension from outside Built-In Packages before its assembly is ever loaded. Blocked items are reported via a notification plus a consolidated modal listing the exact files to delete. The guard is disabled in test mode and is throwaway code intended to be removed once DYN-10739 lands the permanent fix.
IsOutsideBuiltInPackages used a plain StartsWith check, so a sibling directory like "Built-In PackagesOld" could be misclassified as inside Built-In Packages. Normalize both paths with Path.GetFullPath and require an exact match or a directory-separator boundary right after the prefix. The view-extension gate only checked the manifest path. A manifest placed under Built-In Packages could still declare an AssemblyPath that climbs out via ".." segments, loading a legacy assembly despite the manifest passing the check. Normalize the resolved AssemblyPath and check it independently of the manifest path.
![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-10745
There was a problem hiding this comment.
Pull request overview
Adds a temporary “legacy assistant” guard to prevent outdated, non-built-in copies of Autodesk Assistant / DynamoMCP (packages or view extensions) from being loaded in Dynamo 4.2, and surfaces actionable user messaging (notifications + a consolidated modal) describing what to delete.
Changes:
- Introduces
LegacyAssistantExtensionGuardto identify restricted AA/DynamoMCP packages/view extensions and track blocked paths for reporting. - Enforces the guard in
PackageLoader.ScanPackageDirectory(...)andViewExtensionLoader.Load(string), including resolving full paths to prevent..escapes. - Adds unit/WPF tests plus new localized resource strings and public API declarations for the new user-facing messages.
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/Libraries/PackageManagerTests/PackageLoaderTests.cs | Adds package-layer regression tests ensuring legacy DynamoMCP/DynamoAssistant installs outside built-ins are rejected and don’t affect resolution paths. |
| test/DynamoCoreWpf2Tests/ViewExtensions/ViewExtensionTests.cs | Adds view-extension-layer tests for blocking legacy Autodesk Assistant manifests outside built-ins and for blocking AssemblyPath escapes. |
| test/DynamoCoreWpf2Tests/ViewExtensions/Sample Manifests/LegacyAutodeskAssistant_ViewExtensionDefinition.xml | New manifest fixture representing a legacy AA extension. |
| test/DynamoCoreWpf2Tests/ViewExtensions/Sample Manifests/LegacyAutodeskAssistant_EscapingAssemblyPath_ViewExtensionDefinition.xml | New manifest fixture where AssemblyPath resolves outside the trusted built-in directory. |
| test/DynamoCoreTests/LegacyAssistantExtensionGuardTests.cs | New unit tests covering restricted-name/type lookups and robust built-in path boundary/normalization behavior. |
| src/DynamoPackages/PackageLoader.cs | Blocks restricted packages when the guard is enabled, raising a LibraryLoadFailedException with a targeted message. |
| src/DynamoCoreWpf/Views/Core/DynamoView.xaml.cs | Raises per-extension notifications and a consolidated modal listing all blocked files/folders once the main window exists. |
| src/DynamoCoreWpf/PublicAPI.Unshipped.txt | Declares new public resource properties for the WPF notification/modal strings. |
| src/DynamoCoreWpf/Properties/Resources.resx | Adds WPF strings for extension-block notifications and consolidated modal messaging. |
| src/DynamoCoreWpf/Properties/Resources.en-US.resx | Adds en-US equivalents of the new WPF strings. |
| src/DynamoCoreWpf/Properties/Resources.Designer.cs | Updates generated resource accessors for the added WPF strings. |
| src/DynamoCoreWpf/Extensions/ViewExtensionLoader.cs | Resolves manifest/assembly paths to full paths and blocks restricted extensions outside built-ins before assembly load. |
| src/DynamoCore/Utilities/LegacyAssistantExtensionGuard.cs | New guard implementation: restricted-name/type lookup + robust “outside built-ins” check + blocked-path tracking/reset. |
| src/DynamoCore/PublicAPI.Unshipped.txt | Declares new public core resource property for the package-block message. |
| src/DynamoCore/Properties/Resources.resx | Adds core string used when blocking restricted packages. |
| src/DynamoCore/Properties/Resources.en-US.resx | Adds en-US equivalent of the new core string. |
| src/DynamoCore/Properties/Resources.Designer.cs | Updates generated resource accessor for the added core string. |
| src/DynamoCore/Models/DynamoModel.cs | Enables/disables the guard based on IsTestMode and resets state per model instance. |
Files not reviewed (2)
- src/DynamoCore/Properties/Resources.Designer.cs: Generated file
- src/DynamoCoreWpf/Properties/Resources.Designer.cs: Generated file
| if (LegacyAssistantExtensionGuard.IsEnabled && | ||
| LegacyAssistantExtensionGuard.TryGetRestrictedPackageDisplayName(discoveredPackage.Name, out var restrictedDisplayName) && | ||
| !discoveredPackage.BuiltInPackage) | ||
| { |
There was a problem hiding this comment.
Fixed in b97f429 — switched to LegacyAssistantExtensionGuard.IsOutsideBuiltInPackages(discoveredPackage.RootDirectory), which normalizes both paths with Path.GetFullPath and requires an exact match or a directory-separator boundary right after the prefix, instead of Package.BuiltInPackage's unbounded StartsWith. Added a regression test (LegacyDynamoMcpPackageInSiblingDirectoryWithSimilarNameIsNotLoaded) that plants a legacy DynamoMCP package under a sibling directory sharing the built-in root's name as a string prefix (e.g. ...\Packages vs ...\PackagesOld) and confirms it's now rejected. Thanks for catching this.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| if (item.Name == "AssemblyPath") | ||
| { | ||
| path = Path.Combine(path, item.InnerText); | ||
| path = Path.GetFullPath(Path.Combine(path, item.InnerText)); |
There was a problem hiding this comment.
This one's a bit different from the others below: item.InnerText here comes from an untrusted manifest file, so it could in principle be a rooted path and make Path.Combine discard the manifest's directory. That's fine in this case though — the very next line wraps the result in Path.GetFullPath, and the resolved AssemblyPath is then independently checked against LegacyAssistantExtensionGuard.IsOutsideBuiltInPackages before the assembly is loaded. So whether the manifest's AssemblyPath is relative (joined against the manifest's folder) or rooted (used as-is), the final resolved path is validated the same way. No behavior change needed here.
| public void WhenPathIsUnderBuiltinPackagesDirectoryThenIsNotOutside() | ||
| { | ||
| PathManager.BuiltinPackagesDirectory = @"C:\Dynamo\Built-In Packages\Packages"; | ||
| var path = Path.Combine(PathManager.BuiltinPackagesDirectory, "AutodeskAssistant"); |
There was a problem hiding this comment.
False positive here — the second argument is a compile-time string literal with no leading path separator (a plain relative segment or filename), so it can never be treated as rooted and Path.Combine can't drop the earlier argument. Leaving as-is.
| var manifestPath = Path.Combine(GetTestDirectory(ExecutingDirectory), | ||
| @"DynamoCoreWpf2Tests\ViewExtensions\Sample Manifests\LegacyAutodeskAssistant_ViewExtensionDefinition.xml"); |
There was a problem hiding this comment.
False positive here — the second argument is a compile-time string literal with no leading path separator (a plain relative segment or filename), so it can never be treated as rooted and Path.Combine can't drop the earlier argument. Leaving as-is.
| var manifestDirectory = Path.Combine(GetTestDirectory(ExecutingDirectory), | ||
| @"DynamoCoreWpf2Tests\ViewExtensions\Sample Manifests"); |
There was a problem hiding this comment.
False positive here — the second argument is a compile-time string literal with no leading path separator (a plain relative segment or filename), so it can never be treated as rooted and Path.Combine can't drop the earlier argument. Leaving as-is.
|
|
||
| var extensionManager = View.viewExtensionManager; | ||
| var loader = extensionManager.ExtensionLoader; | ||
| var manifestPath = Path.Combine(manifestDirectory, "LegacyAutodeskAssistant_ViewExtensionDefinition.xml"); |
There was a problem hiding this comment.
False positive here — the second argument is a compile-time string literal with no leading path separator (a plain relative segment or filename), so it can never be treated as rooted and Path.Combine can't drop the earlier argument. Leaving as-is.
| var manifestDirectory = Path.Combine(GetTestDirectory(ExecutingDirectory), | ||
| @"DynamoCoreWpf2Tests\ViewExtensions\Sample Manifests"); |
There was a problem hiding this comment.
False positive here — the second argument is a compile-time string literal with no leading path separator (a plain relative segment or filename), so it can never be treated as rooted and Path.Combine can't drop the earlier argument. Leaving as-is.
| var manifestPath = Path.Combine(manifestDirectory, | ||
| "LegacyAutodeskAssistant_EscapingAssemblyPath_ViewExtensionDefinition.xml"); |
There was a problem hiding this comment.
False positive here — the second argument is a compile-time string literal with no leading path separator (a plain relative segment or filename), so it can never be treated as rooted and Path.Combine can't drop the earlier argument. Leaving as-is.
| // DYN-10739 lands the permanent fix. | ||
| private string WriteLegacyDynamoMcpPackage(string packagesRoot) | ||
| { | ||
| var packageDir = Path.Combine(packagesRoot, "DynamoMCP"); |
There was a problem hiding this comment.
False positive here — the second argument is a compile-time string literal with no leading path separator (a plain relative segment or filename), so it can never be treated as rooted and Path.Combine can't drop the earlier argument. Leaving as-is.
| private string WriteLegacyDynamoMcpPackage(string packagesRoot) | ||
| { | ||
| var packageDir = Path.Combine(packagesRoot, "DynamoMCP"); | ||
| Directory.CreateDirectory(Path.Combine(packageDir, "bin")); |
There was a problem hiding this comment.
False positive here — the second argument is a compile-time string literal with no leading path separator (a plain relative segment or filename), so it can never be treated as rooted and Path.Combine can't drop the earlier argument. Leaving as-is.
| { | ||
| var packageDir = Path.Combine(packagesRoot, "DynamoMCP"); | ||
| Directory.CreateDirectory(Path.Combine(packageDir, "bin")); | ||
| File.WriteAllText(Path.Combine(packageDir, "pkg.json"), |
There was a problem hiding this comment.
False positive here — the second argument is a compile-time string literal with no leading path separator (a plain relative segment or filename), so it can never be treated as rooted and Path.Combine can't drop the earlier argument. Leaving as-is.
PackageLoader's gate relied on Package.BuiltInPackage, which is a plain StartsWith against PathManager.BuiltinPackagesDirectory with no directory-separator boundary. A sibling directory like "Built-In PackagesOld" would be misclassified as built-in, letting a legacy package slip past the guard the same way the view-extension gate's manifest path did before the previous fix. Use LegacyAssistantExtensionGuard.IsOutsideBuiltInPackages, which already normalizes paths and enforces the boundary.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 18 changed files in this pull request and generated no new comments.
Files not reviewed (2)
- src/DynamoCore/Properties/Resources.Designer.cs: Generated file
- src/DynamoCoreWpf/Properties/Resources.Designer.cs: Generated file
Suppressed comments (4)
src/DynamoCore/Utilities/LegacyAssistantExtensionGuard.cs:155
- AllBlockedPaths de-duplicates blocked paths with OrdinalIgnoreCase, which can collapse distinct paths on case-sensitive file systems. If you introduce an OS-appropriate path comparer (see blockedPackageDirectories), use it here too.
.Distinct(StringComparer.OrdinalIgnoreCase)
src/DynamoCore/Utilities/LegacyAssistantExtensionGuard.cs:127
- IsOutsideBuiltInPackages uses OrdinalIgnoreCase for the path prefix/equality checks. On case-sensitive file systems (Linux and some macOS/Windows configurations), this can misclassify paths as inside/outside Built-In Packages. Use OS-appropriate comparison (case-insensitive on Windows, ordinal elsewhere).
// A plain StartsWith would let a sibling like "Built-In PackagesOld" pass as if it
// were under "Built-In Packages" -- require an exact match or a directory-separator
// boundary right after the prefix.
return !fullPath.Equals(fullBuiltInDirectory, StringComparison.OrdinalIgnoreCase) &&
!fullPath.StartsWith(fullBuiltInDirectory + Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase);
src/DynamoCore/Utilities/LegacyAssistantExtensionGuard.cs:64
- blockedPackageDirectories is created with StringComparer.OrdinalIgnoreCase, which can incorrectly treat distinct paths as equal on case-sensitive file systems. Prefer a path comparer that matches the current OS (ignore case on Windows only).
This issue also appears in the following locations of the same file:
- line 123
- line 155
// Package blocks record the package's ROOT DIRECTORY: a Dynamo package is a
// self-contained folder, so "delete this folder" is safe and correct advice.
private static readonly HashSet<string> blockedPackageDirectories =
new HashSet<string>(StringComparer.OrdinalIgnoreCase);
src/DynamoCore/Utilities/LegacyAssistantExtensionGuard.cs:145
- BlockedViewExtensions returns the mutable List instance typed as IReadOnlyList; internal callers can still cast it back to List and mutate shared guard state. Return a read-only wrapper to prevent unintended mutation.
internal static IReadOnlyList<BlockedLegacyViewExtension> BlockedViewExtensions => blockedViewExtensions;
edwin-vasquez-ucaldas
left a comment
edwin-vasquez-ucaldas
left a comment
There was a problem hiding this comment.
You already addressed the Copilot comments.
Just to fix the merge conflicts.
Purpose
Dynamo 4.2 ships Autodesk Assistant and DynamoMCP as built-in packages for the first time. Alpha-era copies of either can still be present on disk — a manually-installed
DynamoMCPpackage under the default packages folder, or an orphaned Autodesk Assistant view extension left behind by a Dynamo-for-Revit uninstall — and can silently displace the shipped built-in copy (see DYN-10739).This is an explicit 4.2 band-aid, not the permanent fix. Path is the only reliable signal (package names have churned and alpha builds are signed the same as release builds), so
LegacyAssistantExtensionGuardrefuses any AA/DynamoMCP package or view extension found outsideBuilt-In Packages, and reports exactly which files to delete via a notification plus a consolidated modal. It is disabled in test mode and every new symbol is marked for removal once DYN-10739 lands the architectural fix in 4.3.Target:
master, then cherry-pick toRC4.2.0_master.Supersedes #17277, which was opened from a branch whose merge-base predated the DYN-10716 squash-merge and so pulled in that PR's already-merged commits/diff again. This PR is cut from current
masterand contains only the DYN-10745 changes.Declarations
Check these if you believe they are true
Release Notes
Dynamo now detects and refuses to load outdated, non-built-in copies of Autodesk Assistant or DynamoMCP left over from earlier alpha testing, and tells you which files to delete.
Reviewers
(Assign reviewer)
Note for reviewers: this changes the local-dev workflow for AA/DynamoMCP maintainers — local builds must now be deployed directly into
Built-In Packages\packages\<Name>\rather than aCustomPackageFoldersoverride, since any copy outside Built-In Packages is now refused when the guard is enabled (non-test builds).FYIs