VPR-138 fix(cms): harden temp file path in DownloadZip#184
Open
VPR-138 fix(cms): harden temp file path in DownloadZip#184
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 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 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 42.95% 42.99% +0.03%
==========================================
Files 876 877 +1
Lines 51454 51525 +71
Branches 4802 4811 +9
==========================================
+ Hits 22101 22151 +50
- Misses 28829 28848 +19
- 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 |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 |
✅ Actions performedReview triggered.
|
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
- 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.
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)
Comment on lines
+41
to
+56
| public static string SanitizeDownloadName(string userInput) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(userInput)) | ||
| { | ||
| return DefaultDownloadName; | ||
| } | ||
|
|
||
| // Normalize backslash to forward slash so Path.GetFileName strips | ||
| // path components on every OS. Path.GetFileName only honors the | ||
| // host OS separator, so a Windows-style "\..\evil" leaks through | ||
| // unchanged on Linux runners. | ||
| var normalized = userInput.Replace('\\', '/'); | ||
| var fileNamePart = Path.GetFileName(normalized); | ||
| var filtered = SafeFileNameAllowList.Replace(fileNamePart, string.Empty).Trim(); | ||
|
|
||
| // Dots and spaces survive the allow-list. Names that collapse to |
Comment on lines
+387
to
+389
| #region Download helpers (moved to Viper.Areas.CMS.Services.CmsFilePathSafety) | ||
| #endregion | ||
|
|
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
CMS.DownloadZipwith a server-generated GUID under a dedicated temp folder; useFileMode.Create+ZipArchiveMode.Createand delete infinally.CmsFilePathSafety(static + DI-friendlyICmsFilePathSafetyfor new code); register the newViper.Areas.CMS.Servicesnamespace with the existing Scrutor scan.400on emptyfileGUIDsand404when no files resolve, both before any filesystem I/O.Test plan
test/CMS/DownloadZipSecurityTests.cs— 33 tests covering filename allow-list, reserved Windows device names, extension forcing, traversal regression payloads, and temp-path builder. All pass locally.Program.cscomplexity warnings remain)./CMS/Filesstill open with sanitized.zipfilename in TEST.fileName=..\..\evil) creates no file outside the temp root.Summary by CodeRabbit
New Features
Bug Fixes
Tests