Enhance customALGoFiles feature - #2273
Ole Wunschmann (OleWunschmann) wants to merge 43 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR enhances the customALGoFiles behavior during “Update AL-Go System Files”, adding support for unconditional removals (filesToRemove) and improving custom-template behavior by merging template settings directly and resolving files from the original AL-Go template where applicable.
Changes:
- Add
customALGoFiles.filesToRemovesupport end-to-end (schema, defaults, resolution logic, docs, and release notes). - Update
CheckForUpdatesto read template repo settings viaReadSettingsand merge template settings during file resolution. - Expand automated coverage (unit + e2e) for include/exclude/remove resolution and custom-template propagation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| e2eTests/scenarios/CustomTemplate/runtest.ps1 | Extends e2e scenario to validate custom-template file include/exclude/remove propagation and workflow presence. |
| Tests/CheckForUpdates.Action.Test.ps1 | Adds unit tests for destination-folder resolution and expanded GetFilesToUpdate behaviors (including filesToRemove). |
| Actions/CheckForUpdates/CheckForUpdates.ps1 | Updates settings reading (incl. trigger) and wires template settings + filesToRemove into update/removal flow. |
| Actions/CheckForUpdates/CheckForUpdates.HelperFunctions.ps1 | Implements ResolveFilePathsInDestinationFolder and extends GetFilesToUpdate to produce include/exclude/remove lists. |
| Actions/.Modules/settings.schema.json | Extends settings schema with customALGoFiles.filesToRemove and clarifying descriptions. |
| Actions/.Modules/ReadSettings.psm1 | Adds default filesToRemove array under customALGoFiles. |
| Scenarios/settings.md | Documents customALGoFiles.filesToRemove in settings reference. |
| Scenarios/CustomizingALGoForGitHub.md | Adds conceptual docs + examples for original-template resolution and filesToRemove. |
| RELEASENOTES.md | Documents enhanced customALGoFiles behavior and new filesToRemove. |
|
Maria Zhelezova (@mazhelez) The failing PS5 tests should be fixed now. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
Actions/CheckForUpdates/CheckForUpdates.HelperFunctions.ps1:1205
- Removal destinations are not constrained to
$baseFolder. AfilesToRemoveentry with a matching template file anddestinationFolder = '..'produces adestinationFullPathoutside the repository;CheckForUpdates.ps1later converts it to a relative path and passes it toRemove-Item. Normalize every removal destination and reject paths that are not descendants of the repository root before returning this list.
$filesToRemove += @(ResolveFilePaths -sourceFolder $originalTemplateFolder -destinationFolder $baseFolder -files $filesToRemoveUnresolved -projects $projects)
}
$filesToRemove += @(ResolveFilePaths -sourceFolder $templateFolder -originalSourceFolder $originalTemplateFolder -destinationFolder $baseFolder -files $filesToRemoveUnresolved -projects $projects)
$filesToRemove += @(ResolveFilePathsInDestinationFolder -destinationFolder $baseFolder -files $filesToRemoveUnresolved -projects $projects)
|
Maria Zhelezova (@mazhelez) I’ve gone ahead and resolved the merge conflicts, since I suspect that Copilot struggled with the fact that the source branch comes from my fork. I saw that there were already approvals, but there are still two Copilot comments open:
|
Welcome back! I hope your vacation was splendid! On the comments: I resolved the first one. As for the second one, I'll need to take a proper look. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
The new containment logic still permits two symlink-based repository-boundary escapes.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 6/7 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Two symlink containment gaps can still permit reads or writes outside trusted template and repository roots.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 2
- Review effort level: Balanced
| if (Test-Path -LiteralPath $originalSourceFile -PathType Leaf) { | ||
| # If the file exists in the original template folder, use that file instead | ||
| $fullFilePath.originalSourceFullPath = Join-Path $originalSourceFolder $relativePath -Resolve | ||
| $fullFilePath.originalSourceFullPath = $originalSourceFile |
There was a problem hiding this comment.
Maria Zhelezova (@mazhelez) Alexander Holstrup (@aholstrup1) spetersenms
What do you think about this comment/issue?
Right now there is no escape check for the original-template files and I'm not sure what should happen with these cases.
Possible solutions:
- Ignore this
- Handle this cases by
- throwing an error for escaping original-template files
- ignoring the escaping original-template files
There was a problem hiding this comment.
A guard for going outside $originalSourceFolder sounds like a good solution here.
There was a problem hiding this comment.
I agree. But how should we handle violations: throw an error or ignore the escaping file?
There was a problem hiding this comment.
🟡 Changes recommended
Original-template matches still lack the physical source-containment validation applied to normal source paths.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Destination containment remains bypassable through dangling links and can become invalid after cloning.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 2
- Review effort level: Balanced
| if (-not (Test-Path -LiteralPath $realPath)) { | ||
| $resolveReparsePoints = $false | ||
| continue | ||
| } |
| # Check if the destination file is under the file destination folder (symlink/junction-aware) | ||
| if (-not (Test-PathPhysicallyContained -Path $fullFilePath.destinationFullPath -RootFolder $fileDestinationFolder)) { | ||
| OutputWarning "Skipping file '$srcFile': destination file '$($fullFilePath.destinationFullPath)' is outside the file destination folder '$fileDestinationFolder'." | ||
| continue |
❔What, Why & How
This pull request introduces significant improvements to the handling of custom template file inclusion and exclusion in AL-Go actions, focusing on robust path resolution, improved deduplication, and enhanced cross-platform compatibility. The changes also clarify and expand the documentation for settings, and add a new utility function for reading settings with custom templates.
Improvements to custom template file handling:
customALGoFilessettings from the custom template are now merged and used directly during updates, ensuring that the latest template configuration is always applied without waiting for a new update cycle.filesToIncludeandfilesToExcludeare now resolved against both the custom template and the original AL-Go template, ensuring that files added or removed upstream are properly propagated to consumer repositories.destinationNameproperty is added tofilesToInclude, allowing files to be renamed when copied to the destination. This is reflected in both the schema and implementation.**Path Handling and Cross-Platform Support
GetPathStringComparisonandGetPathStringComparerutility functions to ensure path comparisons and deduplication are case-insensitive on Windows/macOS and case-sensitive on Linux, improving cross-platform reliability.ResolveFilePathsto canonicalize all paths, enforce that destination folders/files are within allowed directories, and use the new path comparison utilities for accurate and secure file operations.Documentation and schema updates:
filesToIncludeandfilesToExcludehave been updated to clarify their new behavior and the newdestinationNameproperty.These changes make custom template management more robust, predictable, and safer for consumer repositories.
Related to discussion: #2227
✅ Checklist