VPR-138 fix(cms): harden DownloadZip and unblock controller route#184
Open
rlorenzo wants to merge 2 commits into
Open
VPR-138 fix(cms): harden DownloadZip and unblock controller route#184rlorenzo wants to merge 2 commits into
rlorenzo wants to merge 2 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughFixes directory-traversal and temp-file safety in CMS ZIP downloads: add CmsFilePathSafety helpers, refactor DownloadZip to use validated temp paths and sanitized entry/download names, update DI registration, and add comprehensive security tests including a VPR-138 regression. ChangesCMS DownloadZip Security Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 42.96% 42.99% +0.03%
==========================================
Files 877 878 +1
Lines 51468 51542 +74
Branches 4802 4812 +10
==========================================
+ Hits 22113 22163 +50
- Misses 28831 28853 +22
- Partials 524 526 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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
- Replace user-influenced temp file path with a server-generated GUID
under a dedicated temp folder; use FileMode.Create +
ZipArchiveMode.Create and delete in a finally block.
- Add CmsFilePathSafety helper (static + DI variant) for download-name
sanitization, ZIP-entry sanitization, and per-request temp-archive
path generation. Register the new Viper.Areas.CMS.Services namespace
with the existing Scrutor scan.
- SanitizeDownloadName normalizes backslash to forward slash before
Path.GetFileName so Windows-style traversal payloads ("..\evil") are
stripped on Linux runners too. Allow-list, reserved Windows device
names, and forced .zip suffix come along.
- SanitizeZipEntryName strips path components from CMSFile.FriendlyName
before passing it to archive.CreateEntry/CreateEntryFromFile -
defense in depth against ZIP-slip when stored names contain
separators or traversal sequences.
- BuildTempArchivePath trims trailing separators on the resolved root
so the StartsWith containment check survives a "C:\Temp\\"-style
input.
- DownloadZip writes encrypted bytes directly to the ZipArchiveEntry
stream instead of wrapping a StreamWriter (the text encoder was
unused). Return 400 on empty fileGUIDs and 404 when no files resolve,
both before any filesystem I/O.
- Add 42 unit tests covering the sanitizer (separators, traversal,
reserved names, extension forcing), ZIP entry helper, temp-path
builder (incl. trailing separator), and a parameterized traversal
regression guard.
…efixes
Vue app prefixes like /CMS, /Effort, etc. were claimed in two places that
ran before MVC routing: a dev-only Vite proxy and the URL rewriter
("(?i)^{appName}" → "/2/vue/src/{app}/index.html"). Any controller route
under those prefixes - notably /CMS/Files (CMSController.Files, the
modernized download surface VPR-138 hardens) - was returned the SPA shell
instead of executing.
Restructure so SPA shell serving (Vite proxy + rewriter + /2/vue static
files) runs inside a UseWhen branch gated on ctx.GetEndpoint() == null.
MVC controller routes claim their endpoints during UseRouting() and the
branch is skipped; everything else (Vue SPA roots, deep client-side
routes, Vite asset paths) still hits the branch and gets the SPA shell.
Smoke-tested locally:
- /CMS/Files?ids=<bogus> now returns 404 from the controller
(was 200 SPA shell).
- /CMS still serves the Vue SPA shell.
- /Effort and /favicon.ico unaffected.
- Vite /2/vue/@vite/client and HMR assets unaffected.
rlorenzo
force-pushed
the
VPR-138-cms-download-cleanup
branch
from
05876a4 to
d85d125
Compare
| DefaultFileNames = new List<string> { "index.html" }, | ||
| FileProvider = new PhysicalFileProvider( | ||
| Path.Combine(builder.Environment.ContentRootPath, "wwwroot/vue")), | ||
| Path.Combine(builder.Environment.ContentRootPath, "wwwroot", "vue")), |
| branch.UseStaticFiles(new StaticFileOptions | ||
| { | ||
| FileProvider = new PhysicalFileProvider( | ||
| Path.Combine(builder.Environment.ContentRootPath, "wwwroot", "vue")), |
| FileProvider = new PhysicalFileProvider( | ||
| Path.Combine(builder.Environment.ContentRootPath, "wwwroot/vue")), | ||
| RequestPath = "/2/vue" | ||
| }); |
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
Security hardening of
CMS.DownloadZip(commit6e67564):%TEMP%/Viper-CMS/folder;FileMode.Create+ZipArchiveMode.Create+ finally-cleanup.CmsFilePathSafety(static + DIICmsFilePathSafetyfor the PLAN-CMS migration consumer); Scrutor registration forViper.Areas.CMS.Services.SanitizeDownloadName: cross-platform separator handling (\→/beforePath.GetFileName), allow-list, reserved Windows device names, dot/space-only rejection, forced.zipsuffix.SanitizeZipEntryName: defense-in-depth ZIP-slip — strip path components from eachCMSFile.FriendlyNamebefore writing to the archive.BuildTempArchivePath: trim trailing separator so the containment check survives"C:\Temp\"-style roots.ZipArchiveEntry.Open()instead of wrapping aStreamWriter(text encoder was unused).400on emptyfileGUIDs,404on no resolvable files — both before any filesystem I/O.Dev-environment routing fix (commit
05876a4):/CMS/*path before MVC routing ran, soCMSController.Filesnever executed and the Vue SPA shell came back instead. Wrap the rewriter + Vite proxy + SPA static files inUseWhen(ctx => ctx.GetEndpoint() is null, …)so MVC controllers claim their endpoints first. Vue SPA roots, deep client-side routes, and Vite/HMR assets unaffected.Test plan
test/CMS/DownloadZipSecurityTests.cs— 51 tests across sanitizer, ZIP-entry helper, temp-path builder, traversal regression, trailing-separator handling./CMS/Files?ids=<bogus>→ 404 from controller (was 200 SPA shell pre-routing-fix);/CMSstill serves SPA shell;/2/vue/@vite/client+ HMR unaffected;/Effortand/favicon.icounaffected.TEST verification is not possible pre-merge — this PR's routing fix is what makes
/2/CMS/Filesreachable on TEST in the first place. Post-deploy manual smoke (after merge → Development → TEST):/2/CMS/Files?ids=<real-guid>&fileName=Report.zip→ ZIP downloads withContent-Disposition: attachment; filename=Report.zip./2/CMS/Files?ids=<real-guid>&fileName=..%5C..%5Cevil.zip→ ZIP still downloads, response filename sanitized toevil.zip(no.., no separators).%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 two commits in this PR are load-bearing on each other: shipping the routing fix without the hardening would expose the path-traversal vulnerability; shipping the hardening alone leaves it gated behind a still-broken route. Merging both at once unlocks the endpoint and secures it in the same step.