VPR-59 [1/4] CMS migration: files, photos, import, rate limiting (backend)#251
VPR-59 [1/4] CMS migration: files, photos, import, rate limiting (backend)#251rlorenzo wants to merge 14 commits into
Conversation
Bundle ReportBundle size has no change ✅ |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Implements the first slice of the CMS migration backend for the files domain: new CMS file-management APIs (list/upload/edit/delete/restore/audit/import/bulk-encrypt), hardened legacy-parity download handling (nosniff + attachment for inline-unsafe types), CMS download rate limiting, a conditional-cached user-photo API, and a gated scheduled trash-purge job—plus supporting validators/constants and comprehensive unit tests.
Changes:
- Add CMS file management services/controllers (storage, encryption/crypto, audit, import, trash purge job) and DTO/mapping infrastructure.
- Harden and optimize the existing
/CMS/Filesdownload surface (rate limiting, access auditing parity, nosniff + attachment rules, permission lookup efficiency). - Add CMS user photo endpoints + service with Last-Modified/304 support and caching headers, plus shared validation/date-range helpers and tests.
Reviewed changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| web/Viper.csproj | Adds htmldiff dependency for diff rendering. |
| web/Services/IHtmlSanitizerService.cs | Adds SanitizeDiff API contract. |
| web/Services/HtmlSanitizerService.cs | Implements diff-aware sanitizer policy. |
| web/Program.cs | Registers CMS download rate limiting + middleware. |
| web/Classes/Utilities/DateRangeHelper.cs | Shared inclusive “To date” handling. |
| web/Areas/CMS/Validation/SafeUrlAttribute.cs | URL validation with optional relative support. |
| web/Areas/CMS/Validation/MaxLengthEachAttribute.cs | Per-element max-length validation for collections. |
| web/Areas/CMS/Services/CmsUserPhotoService.cs | User photo resolution + alt photo support. |
| web/Areas/CMS/Services/CmsFileStorageService.cs | Safe file storage + path containment + collisions. |
| web/Areas/CMS/Services/CmsFileService.cs | Core file CRUD, encryption toggle, purge. |
| web/Areas/CMS/Services/CmsFileResponse.cs | Download response hardening helpers. |
| web/Areas/CMS/Services/CmsFileImportService.cs | Legacy-webroot import + bulk encrypt tools. |
| web/Areas/CMS/Services/CmsFileEncryptionService.cs | Master-key wrapper + in-place transforms. |
| web/Areas/CMS/Services/CmsFileCrypto.cs | CF-compatible AES-ECB + UU encoding primitives. |
| web/Areas/CMS/Services/CmsFileAuditService.cs | File audit row creation + paging/filtering. |
| web/Areas/CMS/Services/CmsDownloadRateLimiting.cs | Rate limiter policy + partition keying. |
| web/Areas/CMS/Services/CmsConcurrencyException.cs | Shared 409 stale-edit exception type. |
| web/Areas/CMS/Models/DTOs/CreateLinkDto.cs | Fixes SafeUrlAttribute namespace reference. |
| web/Areas/CMS/Models/DTOs/CmsFileRequests.cs | Request DTOs for create/update endpoints. |
| web/Areas/CMS/Models/DTOs/CmsFileImportDtos.cs | Import + preview + bulk-encrypt DTOs. |
| web/Areas/CMS/Models/DTOs/CmsFileDto.cs | File management DTOs and name-check result. |
| web/Areas/CMS/Models/CmsFileMapper.cs | Mapperly-based entity→DTO mapping. |
| web/Areas/CMS/Models/CMSFile.cs | Aligns URL generation with new handler shape. |
| web/Areas/CMS/Jobs/CmsTrashPurgeScheduledJob.cs | Gated daily purge job for trashed files. |
| web/Areas/CMS/Data/CMS.cs | Download hardening, auditing parity, perf tweaks. |
| web/Areas/CMS/Controllers/CMSUserPhotoController.cs | /api/cms/photos endpoints + 304 behavior. |
| web/Areas/CMS/Controllers/CMSOptionsController.cs | CMS option lists for permissions/people. |
| web/Areas/CMS/Controllers/CMSFilesController.cs | /api/cms/files management API surface. |
| web/Areas/CMS/Controllers/CMSController.cs | Enables rate limiting on /CMS/Files. |
| web/Areas/CMS/Constants/CmsTrash.cs | Trash retention + config key constants. |
| web/Areas/CMS/Constants/CmsPermissions.cs | Central CMS permission strings. |
| web/Areas/CMS/Constants/CmsFileTypes.cs | File allowlist + inline-unsafe extension set. |
| web/Areas/CMS/Constants/CmsFileNaming.cs | Friendly-name + canonical folder key rules. |
| web/appsettings.json | Adds default-off trash purge config. |
| test/Services/HtmlSanitizerServiceTests.cs | Tests for SanitizeDiff XSS guarantees. |
| test/CMS/SafeUrlAttributeTests.cs | SafeUrlAttribute absolute/relative-mode tests. |
| test/CMS/MaxLengthEachAttributeTests.cs | MaxLengthEachAttribute behavior tests. |
| test/CMS/CmsUserPhotoServiceTests.cs | Photo service resolution/traversal/Last-Modified tests. |
| test/CMS/CMSUserPhotoControllerTests.cs | Controller wiring + cache/304 behavior tests. |
| test/CMS/CmsTrashPurgeScheduledJobTests.cs | Ensures purge gate stays off by default. |
| test/CMS/CmsFileStorageServiceTests.cs | Storage containment, collisions, temp writes tests. |
| test/CMS/CMSFilesControllerTests.cs | Controller status mapping + admin-only hard delete tests. |
| test/CMS/CmsFileResponseTests.cs | nosniff + attachment/inline disposition tests. |
| test/CMS/CmsFileImportServiceTests.cs | Import preview/execute + bulk-encrypt tests. |
| test/CMS/CmsFileCryptoTests.cs | Crypto format round-trip compatibility tests. |
| test/CMS/CmsDownloadRateLimitingTests.cs | ZIP vs file partitioning key tests. |
| test/CMS/CMSContentPermissionTests.cs | Content permission gate tests (perf change coverage). |
| test/Classes/Utilities/DateRangeHelperTests.cs | Exclusive upper-bound helper tests. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds CMS file-management APIs, storage, encryption, import, purge, rate limiting, and photo serving, plus a diff-aware HTML sanitizer path and tests. ChangesCMS File Management System
HTML Diff Sanitization
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/Areas/CMS/Data/CMS.cs (1)
398-424: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPer-file synchronous
SaveChanges()in theDownloadZiploop.
AuditFileAccessclears the change tracker and callsSaveChanges()for every guid inguids(both allowed and now denied files). A zip request with many files causes that many individual round-trips to the audit table instead of one batched write.Consider collecting
FileAuditrows in the loop and callingAddRange+ a singleSaveChanges()after the loop completes.🤖 Prompt for 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. In `@web/Areas/CMS/Data/CMS.cs` around lines 398 - 424, The DownloadZip audit path is doing a synchronous SaveChanges for every file through AuditFileAccess, causing one database round-trip per guid. Update the CMS.DownloadZip flow and the AuditFileAccess usage so the loop only collects FileAudit entries for both allowed and denied files, then persist them once after the loop with AddRange and a single SaveChanges call. Keep the existing audit action names and use the current _viperContext, CheckFilePermission, and AuditFileAccess flow to locate the batching change.
🤖 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/Services/HtmlSanitizerServiceTests.cs`:
- Around line 249-269: Add an idempotency regression test for SanitizeDiff in
HtmlSanitizerServiceTests, mirroring the existing Sanitization_is_idempotent
coverage for Sanitize. Use SanitizeDiff as the target and verify that running it
twice on already-sanitized diff HTML (for example content with <ins> and <del>
markers) produces the same output both times. This will protect the SanitizeDiff
method and its diff-specific allowlist from double-sanitization issues when
re-rendered.
In `@web/Areas/CMS/Constants/CmsFileTypes.cs`:
- Around line 31-37: The CmsFileTypes allow-list currently includes potentially
dangerous binary/archive types, which creates a malware-distribution risk.
Review the CmsFileTypes mapping and remove unsupported executable/archive
entries such as exe, dmg, 7z, and zip unless there is a confirmed legacy
requirement; if they must remain, ensure the upload flow that uses this
allow-list also routes those types through malware scanning before they are
accepted into the CMS file store. Refer to the CmsFileTypes constant so the
allow-list and any related upload validation stay in sync.
In `@web/Areas/CMS/Controllers/CMSFilesController.cs`:
- Around line 259-317: Add an upper bound check for batch sizes in
CMSFilesController so ImportFiles, PreviewImport, and BulkEncrypt reject
oversized requests instead of only checking for empty lists. Use the existing
request collections (CmsFileImportRequest.FilePaths and the BulkEncrypt
fileGuids parameter) to enforce a reasonable max, and return BadRequest when
exceeded. Keep the validation close to the guards in ImportFiles, PreviewImport,
and BulkEncrypt so the limit is enforced before calling _importService.
In `@web/Areas/CMS/Controllers/CMSOptionsController.cs`:
- Around line 47-84: Add coverage for CMSOptionsController.SearchPeople because
it has non-trivial search filtering and result-shaping logic. Create a
controller test class for CMSOptionsController that verifies the
min-length/whitespace guard returns an empty list, the multi-field OR search
matches DisplayLastName/DisplayFirstName/LoginId/MailId, and the query respects
ordering plus the Take(25) limit. Use the SearchPeople action and
CmsPersonOption type to locate the behavior.
- Around line 47-74: The SearchPeople action in CMSOptionsController is using
Contains() on concatenated name fields and LoginId/MailId, which translates to
leading-wildcard scans and won’t benefit from indexes. Update the filtering
logic to use an index-friendly search strategy for AaudUsers, such as a
persisted/computed display-name field or full-text search, and adjust the query
in SearchPeople so it targets those indexed/searchable fields instead of
concatenated expressions and broad Contains() predicates.
In `@web/Areas/CMS/Data/CMS.cs`:
- Around line 642-647: `DecryptFile` is re-reading the master key from disk on
every invocation, which is expensive when `DownloadZip` calls it repeatedly
inside the archive loop. Update the `DecryptFile` flow to reuse a cached master
key instead of calling
`CmsFileCrypto.ReadMasterKey(CmsFileCrypto.GetDefaultKeyFilePath())` each time,
either by confirming `ReadMasterKey` caches internally or by caching the key
once per `DownloadZip`/`ProvideFile` execution and passing it through to
`DecryptFile` or delegating to `CmsFileEncryptionService`.
- Around line 60-86: The CMS permission check is using a hardcoded security
string instead of the shared constant. Update the access-resolution logic in
CMS.cs where isCmsAdmin is assigned to use CmsPermissions.ManageContentBlocks
rather than "SVMSecure.CMS.ManageContentBlocks", so the check stays aligned with
the rest of the CMS permission code and avoids drift.
- Around line 611-619: The kiosk IP filter in AuditFileAccess only checks the
raw RemoteIpAddress string, so IPv4-mapped IPv6 values like ::ffff:192.168.x.x
slip through. Update the RemoteIpAddress handling in CMS.AuditFileAccess to
normalize mapped IPv4 addresses before evaluating the kiosk range, then perform
the 192.168. prefix check using StartsWith with StringComparison.Ordinal.
In `@web/Areas/CMS/Services/CmsFileAuditService.cs`:
- Around line 87-88: The audit history pagination in CmsFileAuditService has the
same unbounded perPage issue as CmsFileService.GetFilesAsync: only the lower
bound is enforced. Update the paging logic around the page/perPage normalization
to also cap perPage at a safe maximum, using the existing CmsFileAuditService
method that fetches audit history so callers cannot request arbitrarily large
page sizes.
In `@web/Areas/CMS/Services/CmsFileEncryptionService.cs`:
- Around line 25-31: `CmsFileEncryptionService` is still scoped, so the
`_masterKey` Lazy<string> is recreated per request and repeatedly hits the key
file on first use. Update the DI registration or introduce a singleton key-cache
so the master key is loaded once and reused across scopes, and keep the caching
behind `CmsFileEncryptionService`/`CmsFileCrypto.ReadMasterKey` rather than per
scoped instance.
In `@web/Areas/CMS/Services/CmsFileImportService.cs`:
- Around line 331-389: `EncryptSingleFileAsync` is missing some expected
file/key failure handling, so malformed key data or invalid paths can still
escape and abort `BulkEncryptAsync`. Update this method to catch the same
anticipated exceptions as `ImportSingleFileAsync`, especially `FormatException`
alongside the existing file access cases, and convert them into `result.Message`
so the bulk loop can continue collecting results. Keep the fix localized to
`EncryptSingleFileAsync` and preserve the existing rollback behavior for save
failures.
In `@web/Areas/CMS/Services/CmsFileService.cs`:
- Line 390: The current encryption toggle flow in CmsFileService unnecessarily
rewrites the existing file on disk when a replacement upload is present. Update
the logic around ApplyEncryptionToggle, WriteReplacementUploadAsync, and the
replacement path so that when file != null, only metadata changes (entity.Key
and entity.Encrypted) are applied before the new bytes are written. Keep the
in-place encrypt/decrypt work only for the no-replacement case, and ensure the
replacement upload already handles writing the file in the requested encryption
state.
- Around line 76-77: Clamp both bounds in CmsFileService pagination: the current
page/perPage normalization only enforces a minimum in the paging logic, so
perPage can still grow without limit. Update the relevant method in
CmsFileService to cap perPage at a safe maximum before it is used for Skip/Take,
while keeping the existing lower-bound guard and preserving the page
normalization behavior.
- Around line 235-297: The overwrite path in CreateFileAsync does not clean up
when ReplaceInPlace throws after BackupManagedFile succeeds, leaving a stale
backup and inconsistent disk state. Update the overwrite branch in
CmsFileService.CreateFileAsync to wrap ReplaceInPlace in error handling, and on
failure call ReconcileStoreAfterFailedCreate(targetPath, overwriteBackup) before
rethrowing so the backup/original is restored or removed consistently.
In `@web/Areas/CMS/Services/CmsFileStorageService.cs`:
- Around line 136-154: FileNameInUse is being called repeatedly from
GetUniqueFileName, causing up to 1000 synchronous, non-sargable database checks
per upload. Refactor the uniqueness check so GetUniqueFileName loads the
existing file names for the target folder once, combines that with the disk
listing, and then checks candidates in memory across the suffix loop. Keep
FileNameInUse/CmsFileStorageService as the place to locate the current logic,
but avoid per-candidate _context.Files.Any calls and the EndsWith-based DB scan.
---
Outside diff comments:
In `@web/Areas/CMS/Data/CMS.cs`:
- Around line 398-424: The DownloadZip audit path is doing a synchronous
SaveChanges for every file through AuditFileAccess, causing one database
round-trip per guid. Update the CMS.DownloadZip flow and the AuditFileAccess
usage so the loop only collects FileAudit entries for both allowed and denied
files, then persist them once after the loop with AddRange and a single
SaveChanges call. Keep the existing audit action names and use the current
_viperContext, CheckFilePermission, and AuditFileAccess flow to locate the
batching change.
🪄 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: 5fbfb8bc-5434-4e95-94af-4ed1a1343881
📒 Files selected for processing (49)
test/CMS/CMSContentPermissionTests.cstest/CMS/CMSFilesControllerTests.cstest/CMS/CMSUserPhotoControllerTests.cstest/CMS/CmsDownloadRateLimitingTests.cstest/CMS/CmsFileCryptoTests.cstest/CMS/CmsFileImportServiceTests.cstest/CMS/CmsFileResponseTests.cstest/CMS/CmsFileServiceTests.cstest/CMS/CmsFileStorageServiceTests.cstest/CMS/CmsTrashPurgeScheduledJobTests.cstest/CMS/CmsUserPhotoServiceTests.cstest/CMS/MaxLengthEachAttributeTests.cstest/CMS/SafeUrlAttributeTests.cstest/Classes/Utilities/DateRangeHelperTests.cstest/Services/HtmlSanitizerServiceTests.csweb/Areas/CMS/Constants/CmsFileNaming.csweb/Areas/CMS/Constants/CmsFileTypes.csweb/Areas/CMS/Constants/CmsPermissions.csweb/Areas/CMS/Constants/CmsTrash.csweb/Areas/CMS/Controllers/CMSController.csweb/Areas/CMS/Controllers/CMSFilesController.csweb/Areas/CMS/Controllers/CMSOptionsController.csweb/Areas/CMS/Controllers/CMSUserPhotoController.csweb/Areas/CMS/Data/CMS.csweb/Areas/CMS/Jobs/CmsTrashPurgeScheduledJob.csweb/Areas/CMS/Models/CMSFile.csweb/Areas/CMS/Models/CmsFileMapper.csweb/Areas/CMS/Models/DTOs/CmsFileDto.csweb/Areas/CMS/Models/DTOs/CmsFileImportDtos.csweb/Areas/CMS/Models/DTOs/CmsFileRequests.csweb/Areas/CMS/Models/DTOs/CreateLinkDto.csweb/Areas/CMS/Services/CmsConcurrencyException.csweb/Areas/CMS/Services/CmsDownloadRateLimiting.csweb/Areas/CMS/Services/CmsFileAuditService.csweb/Areas/CMS/Services/CmsFileCrypto.csweb/Areas/CMS/Services/CmsFileEncryptionService.csweb/Areas/CMS/Services/CmsFileImportService.csweb/Areas/CMS/Services/CmsFileResponse.csweb/Areas/CMS/Services/CmsFileService.csweb/Areas/CMS/Services/CmsFileStorageService.csweb/Areas/CMS/Services/CmsUserPhotoService.csweb/Areas/CMS/Validation/MaxLengthEachAttribute.csweb/Areas/CMS/Validation/SafeUrlAttribute.csweb/Classes/Utilities/DateRangeHelper.csweb/Program.csweb/Services/HtmlSanitizerService.csweb/Services/IHtmlSanitizerService.csweb/Viper.csprojweb/appsettings.json
|
@coderabbitai review |
✅ Action performedReview finished.
|
Slice 1/3 of the restacked CMS migration (built from the verified branch tip, so every slice carries the OS-independent path handling and the review fixes). Adds the files-domain backend: storage, CF-compatible crypto, audit, import, trash purge job, user photos, options lookups, the rebuilt /CMS/Files download path, and download rate limiting, with their unit tests. The content/nav backend and the management SPA follow in slices 2-3.
Its strict JSON parser cannot read appsettings*.json (ASP.NET parses those as JSON-with-comments by design) and warned on every review touching them; CI already covers its lint surface.
- CMS:LegacyWebrootPath now ships in appsettings.{Environment}.json
(dev: C:\Sites\https\VIPER; TEST/PROD: S:\webroot\https\viper, matching
the legacy deploy configs) instead of a hardcoded dev-only fallback
- unset in any environment fails fast with the existing not-configured error
The legacy import tool stripped a leading "viper" segment from pasted paths (case/separator insensitive). ResolveSource now does the same so those lines resolve, and the recorded OldUrl stays webroot-relative.
… overwrite - The overwrite-blocking check only matched an existing record by exact current-root path or freshly-derived friendly name, so a record created under another environment's storage root with a since-renamed friendly name could slip through and have its bytes overwritten - Extract HasFileRecord (path, path-suffix, or friendly-name match) so CreateFileAsync's overwrite guard uses the same detection FileNameInUse already relied on to enter that code path
- Permissions isn't [Required], so a client can post it as an explicit null; the inline Where/Select/ToList chain would then throw a NullReferenceException instead of the intended empty-list default. Reuse the shared CleanList helper other CMS services already use for this exact null-safety concern.
- MoveIntoPlace only validated the extension; a caller-supplied name (e.g. via the FileName override) with characters like ? : < > | would pass that check and then make File.Move throw an unhandled exception instead of the intended 400. Reuse the same character set the folder validator already rejects.
- OwnerRestriction fails closed for a missing user, but a non-admin user with a null LoginId fell through the ?: to return null, which the trash query treats as the unrestricted admin view - leaking the full trash list to a user it shouldn't.
- Map file-store (IO/UnauthorizedAccess) and database (DbUpdate/Sql) failures to descriptive 500s instead of the generic reference-ID error - Surface the inner SqlException message for database failures
- Add disk-space-cms-writable: probes the store root and every top-level VIPER app folder for create+delete access, naming any that regress - Resolve the store root from CMS:FileStorageRoot, matching CmsFileStorageService
Wrap the temp write, store move, and in-place replace so IO/permission failures carry the resolved path; the raw exception omits it and admins can't read server logs.
fc729b1 to
c6d7d3a
Compare
Slice 1 of 4 stacked PRs for the CMS migration (VPR-59), restacked from the CI-verified branch tip. Replaces the earlier 6-PR stack (#245-#250): those slices were historical cut points, so code predating the branch's later OS-independent path handling and ReSharper/Fallow cleanups failed CI on Linux runners. Each new slice is built from the final file states, so every slice carries all fixes and passes the full gate set (verified locally per slice: lint, 2,400+ backend tests, 1,026 frontend tests, build).
Scope — files-domain backend
/api/cms/files: paged list with filters and folder counts, upload with name-conflict handling, edit with permission/person deltas and a 409 stale-edit guard, soft delete/restore with owner scoping, admin-gated permanent delete.CmsFileStorageService(containment-checked paths, collision handling),CmsFileCrypto+CmsFileEncryptionService(CF-compatible AES so the legacy ColdFusion app can still read encrypted files),CmsFileAuditService, import-from-legacy-webroot, 30-day trash purge job (Cms:TrashPurgeEnabled, default off)./api/cms/photos, by mail/login/iam/mothra) with Last-Modified/304 conditional caching./CMS/Fileshandler (token bucket for ZIPs, sliding window for files) and the rebuiltData/CMS.csdownload path with legacy-parity access auditing.CMSOptionsControllerfor permission/person typeahead; unit tests for all of the above.All 27 review comments from the previous stack are addressed in this restack (fixes) or answered on the old PRs (declines with rationale).
Stack: this PR → content blocks/left nav backend → management SPA. Merging this slice alone ships inert-but-tested APIs.