VPR-138 fix(cms): harden DownloadZip and unblock controller route#184
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughIssue: Download zip and temp-file handling allowed path traversal and unsafe filenames. Fix: add CmsFilePathSafety (sanitizers, traversal detector, temp-path builder), wire it into DownloadZip/ProvideFile, update DI scan, and add comprehensive security tests including a VPR-138 regression. ChangesCMS DownloadZip Security Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #184 +/- ##
==========================================
+ Coverage 43.15% 43.17% +0.02%
==========================================
Files 892 893 +1
Lines 51697 51810 +113
Branches 4824 4841 +17
==========================================
+ Hits 22310 22370 +60
- Misses 28843 28894 +51
- Partials 544 546 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/CMS/DownloadZipSecurityTests.cs`:
- Around line 5-9: Update the XML doc comment in the CMS.DownloadZip security
tests to use inline code formatting instead of an unresolved <see/> tag: replace
the <see cref="CMS.DownloadZip"/> reference with <c>CMS.DownloadZip</c> in the
comment above the tests (the summary for the tests covering CMS.DownloadZip,
filename sanitizer and temp-archive path builder) to remove the analyzer
warning.
In `@web/Areas/CMS/Data/CMS.cs`:
- Around line 491-505: Sanitize the ZIP entry names before creating entries:
instead of passing file.FriendlyName directly to archive.CreateEntry and
archive.CreateEntryFromFile, strip any path characters (e.g., via
Path.GetFileName or by calling the existing
CmsFilePathSafety.SanitizeDownloadName()) and use that sanitized name; update
the block that handles encrypted files (where DecryptFile is used and writer
writes to fileEntry) and the else branch (where CreateEntryFromFile is called)
to supply the sanitized filename variable.
In `@web/Areas/CMS/Services/CmsFilePathSafety.cs`:
- Around line 48-50: Normalize backslashes before extracting the filename and
ensure any remaining traversal dots are removed: in CmsFilePathSafety.cs, before
calling Path.GetFileName(userInput) replace backslashes with forward slashes
(e.g., userInput.Replace('\\','/')) so Path.GetFileName treats sequences like
"\..\evil.zip" as path segments on Unix, then after applying
SafeFileNameAllowList and Trim remove any remaining ".." sequences (e.g., strip
or collapse runs of "..") so
SanitizeDownloadName_StripsPathSeparatorsAndTraversal and VPR-138 tests no
longer fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 84f82321-14d3-46c2-b06b-32c07318742d
📒 Files selected for processing (4)
test/CMS/DownloadZipSecurityTests.csweb/Areas/CMS/Data/CMS.csweb/Areas/CMS/Services/CmsFilePathSafety.csweb/Program.cs
There was a problem hiding this comment.
Pull request overview
This PR hardens the legacy CMS ZIP download flow by separating user-provided download names (response header only) from server-generated on-disk temp archive paths, reducing the risk of path traversal and unsafe filesystem writes.
Changes:
- Introduces
CmsFilePathSafety(static + DI-friendly interface/service) for sanitizing download names, ZIP entry names, and generating per-request temp archive paths. - Updates
CMS.DownloadZipto validate inputs earlier (400/404), generate temp ZIP paths under a dedicated temp folder, create archives withFileMode.Create/ZipArchiveMode.Create, and always attempt cleanup infinally. - Adds a focused security test suite covering sanitization and temp-path generation regressions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| web/Program.cs | Adds Viper.Areas.CMS.Services to the Scrutor namespace scan for auto DI registration. |
| web/Areas/CMS/Services/CmsFilePathSafety.cs | Adds centralized path/filename sanitization and temp archive path generation helpers (+ DI wrapper). |
| web/Areas/CMS/Data/CMS.cs | Reworks DownloadZip to use the new safety helpers, improve early validation, and ensure temp file cleanup. |
| test/CMS/DownloadZipSecurityTests.cs | Adds security regression tests for download-name sanitization, temp-path containment, and ZIP entry naming. |
rlorenzo
force-pushed
the
VPR-138-cms-download-cleanup
branch
from
2531897 to
caaaceb
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/Areas/CMS/Services/CmsFilePathSafety.cs`:
- Around line 54-58: The allow-list filtering can leave names that consist only
of dots/spaces (e.g., "." or "..") which then become unsafe when suffixed (e.g.,
"..zip"); in the method using SafeFileNameAllowList with variable fileNamePart,
after computing filtered = SafeFileNameAllowList.Replace(fileNamePart,
string.Empty).Trim() treat any filtered value that consists only of dots and/or
whitespace as invalid and return DefaultDownloadName instead; update the
conditional that checks string.IsNullOrEmpty(filtered) (in CmsFilePathSafety.cs
/ the method handling download name normalization) to also detect and reject
dot-only strings (for example by checking filtered.Trim('.') == string.Empty or
equivalent) so such inputs fall back to FileDownload.zip.
- Around line 124-133: The extracted basename in SanitizeZipEntryName (the block
that computes entry = Path.GetFileName(friendlyName.Replace('\\','/')) and
returns it) must reject names that collapse to "." or ".." or consist only of
dots/spaces; update the logic to treat such values as invalid and fall back to
the sanitized fallback basename (i.e., return
Path.GetFileName(fallback.Replace('\\','/')) when entry is null/whitespace or
matches a pattern of only dots/spaces or exactly "." or ".."); also add a
regression unit test next to SanitizeZipEntryName_StripsPathComponents covering
inputs like "..", "dir/..", and @"nested\." to assert the fallback is used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 31654e00-bc72-46a1-8d47-7691475ace9a
📒 Files selected for processing (4)
test/CMS/DownloadZipSecurityTests.csweb/Areas/CMS/Data/CMS.csweb/Areas/CMS/Services/CmsFilePathSafety.csweb/Program.cs
rlorenzo
force-pushed
the
VPR-138-cms-download-cleanup
branch
from
caaaceb to
7ad1880
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
web/Areas/CMS/Data/CMS.cs:459
DownloadZiponly checksfileGUIDs.Length == 0, but the controller passesids.Split(','), which can include empty/whitespace entries (e.g.ids=,or trailing commas). That currently falls through and returns 404 after doing work. Consider trimming/filtering out empty GUID strings up front and returning 400 when no non-empty IDs remain to match the intended “empty fileGUIDs => 400” behavior.
if (fileGUIDs.Length == 0)
{
return controller.BadRequest("Missing fileGUIDs parameter.");
}
var safeDownloadName = CmsFilePathSafety.SanitizeDownloadName(fileName);
List<CMSFile> files = new();
AaudUser? currentUser = UserHelper.GetCurrentUser();
foreach (var guid in fileGUIDs)
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This was referenced May 13, 2026
rlorenzo
force-pushed
the
VPR-138-cms-download-cleanup
branch
from
7fac84f to
05876a4
Compare
rlorenzo
changed the title
rlorenzo
force-pushed
the
VPR-138-cms-download-cleanup
branch
from
05876a4 to
d85d125
Compare
rlorenzo
force-pushed
the
VPR-138-cms-download-cleanup
branch
2 times, most recently
from
89c369e to
500a91b
Compare
rlorenzo
force-pushed
the
VPR-138-cms-download-cleanup
branch
from
500a91b to
3f26527
Compare
rlorenzo
force-pushed
the
VPR-138-cms-download-cleanup
branch
from
4d5b84b to
64f6c9c
Compare
rlorenzo
force-pushed
the
VPR-138-cms-download-cleanup
branch
from
64f6c9c to
07dcb94
Compare
rlorenzo
force-pushed
the
VPR-138-cms-download-cleanup
branch
from
07dcb94 to
70ce8a4
Compare
rlorenzo
force-pushed
the
VPR-138-cms-download-cleanup
branch
from
70ce8a4 to
45e9763
Compare
- Generate the zip temp path from a server-side GUID under a dedicated %TEMP%/Viper-CMS folder, created on demand so a download survives the temp directory being swept mid-run. Sanitize the download name and each zip entry name to harden DownloadZip against path traversal and ZIP-slip. - Harden the single-file ProvideFile path the same way: derive the inline Content-Disposition name from the stored record via SetHttpFileName, and fall back to application/octet-stream for unmapped extensions instead of throwing a 500. - Log a [CMS-DOWNLOAD-NAME] warning when a requested fileName is traversal-shaped (detection only; the name never reaches the filesystem). - Expose the helpers via ICmsFilePathSafetyService for the PLAN-CMS migration, and cover the sanitizers and traversal cases in DownloadZipSecurityTests.
- Wrap the SPA rewriter, Vite proxy, and /2/vue static files in UseWhen(ctx => ctx.GetEndpoint() is null) so attribute-routed controllers like CMSController.Files claim their endpoint instead of being served the SPA shell. Run the block right after UseRouting (before auth/session) so built Vue assets are served without per-request auth/session overhead. - Register Viper.Areas.CMS.Services for Scrutor so the download safety helpers resolve via DI.
The StatusCode.cshtml error view passed Model.ToString() (a string) to Enum.GetName(Type, object), which throws ArgumentException, so every non-403 status page (404, 500, ...) rendered as a 500 instead. Render the HttpStatusCode model directly.
rlorenzo
force-pushed
the
VPR-138-cms-download-cleanup
branch
from
45e9763 to
eb33c78
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
Author
|
@bsedwards Ready for review. This is necessary to get file downloads working in the new CMS system. |
bsedwards
approved these changes
Jun 10, 2026
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
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.
Summary
Hardens and enables the modernized CMS file-download surface (
CMSController.Files→CMS.DownloadZip/CMS.ProvideFile) and fixes the routing that kept it unreachable. Three commits:d9b1955feat(cms): secure the CMS file download handlersCMS.DownloadZip,CMS.ProvideFile, the newCmsFilePathSafetyhelpers, and tests.%TEMP%/Viper-CMSfolder, created on demand so a download survives the OS or an admin sweeping the temp dir, and written withFileMode.Create+ZipArchiveMode.Createplus a finally-cleanup. User input never forms an on-disk path.SanitizeDownloadNamefor theContent-Dispositionname: strip path components (\normalized to/first), allow-list via theDisallowedFileNameCharsregex, reject reserved Windows device names and dot/space-only names, force a.zipsuffix.SanitizeZipEntryName: defense-in-depth ZIP-slip protection on each archive entry.BuildTempArchivePath: server-GUID path with a containment assertion that also handles filesystem-root temp roots (C:\,/) without doubling the separator.ProvideFile(single-file path) hardened the same way: the inlineContent-Dispositionname is derived from the stored record viaSetHttpFileName(never the rawfnquery param), and the content type uses aTryGetValue+application/octet-streamfallback (ToLowerInvariantlookup) instead of an indexer that 500s on an unmapped extension.DownloadZipfilters blank/whitespace segments up front and returns400when none remain (soids=,,is treated as a missing parameter), and skips blanks in the loop.[CMS-DOWNLOAD-NAME]warning (LooksLikeTraversalAttempt) when a requestedfileNameis traversal-shaped (a separator, or..as a distinct segment;report..final.zipis not flagged). Detection only: the name feedsContent-Disposition, never the filesystem.ICmsFilePathSafetyService(Scrutor-registered) for the PLAN-CMS migration. 68 security tests inDownloadZipSecurityTests.cs.89d8737fix(routing): let MVC controllers win over the Vue SPA shell/CMS/*path before MVC routing ran, soCMSController.Filesnever executed and the Vue SPA shell came back instead. Wrap the rewriter + Vite proxy +/2/vuestatic files inUseWhen(ctx => ctx.GetEndpoint() is null, …)so attribute-routed controllers claim their endpoint first.UseWhenblock runs immediately afterUseRouting, before auth/session, so built Vue assets and SPA shells are served without per-request auth/session overhead.Viper.Areas.CMS.Servicesfor Scrutor (DI for the safety service); static-file providers resolve viaWebRootPath.eb33c78fix(error): stop StatusCode view from throwing on renderViews/Home/StatusCode.cshtmlpassedModel.ToString()(a string) toEnum.GetName(Type, object), which throwsArgumentException, so every non-403 status page (404, 500, ...) rendered as a 500. Render theHttpStatusCodemodel directly. Surfaced while verifying the download route on TEST (it was masking the route's 404s as 500s); a pre-existing error-view bug bundled here since it gated this PR's smoke tests.Test plan
test/CMS/DownloadZipSecurityTests.cs— 68 tests: sanitizer, ZIP-entry helper, temp-path builder (incl. trailing-separator and filesystem-root), traversal regression, and theLooksLikeTraversalAttemptpredicate (path-shaped flagged; plain names incl.report..final.zipnot)./CMS/Files?ids=<bogus>→ 404 from the controller;/CMSserves the SPA shell;/Effortand Vite/HMR assets unaffected.secure-test,/2PathBase) after deploy:/2/CMS/Files?ids=<bogus-guid>→ 404 (clean; was 500 before the StatusCode fix)./2/CMS/Files?ids=,→ 400./2/CMSand/2/EffortSPA shells serve; login / SPA auth flow intact.Authenticated happy path still to smoke with a logged-in session:
/2/CMS/Files?ids=<real-guid>&fileName=Report.zip→ ZIP downloads asReport.zip./2/CMS/Files?ids=<real-guid>&fileName=..%5C..%5Cevil.zip→ downloads, response filename sanitized toevil.zip, and a[CMS-DOWNLOAD-NAME]warning is logged.%TEMP%/Viper-CMS/empty after each request (finally-cleanup intact).Notes
CMSController.Filescarries no production traffic today; file downloads go through the legacy ColdFusion CMS at the root domain (/cms/files/?id=...). VIPER2's modernized handler exists as the migration target for PLAN-CMS.md. The routing fix and theDownloadZiphardening are load-bearing on each other: the routing fix without the hardening would expose path traversal; the hardening alone leaves it gated behind a still-broken route. The traversal-name logging,ProvideFilehardening, andStatusCodeerror-view fix are additive on the same surface.