VPR-59 [6/6] CMS migration: hardening, concurrency guards, review fixes#250
Closed
rlorenzo wants to merge 24 commits into
Closed
VPR-59 [6/6] CMS migration: hardening, concurrency guards, review fixes#250rlorenzo wants to merge 24 commits into
rlorenzo wants to merge 24 commits into
Conversation
- Snapshot a file's original bytes before an overwrite or replacement and restore them if the database save fails, so a failed create or edit can't orphan the new bytes or destroy the original. - If the restore itself fails, keep the backup and log its path for recovery instead of deleting the only remaining copy. - Import preview reserves each planned name so sources sharing a base name preview the unique names the import will actually assign.
- Treats "//host" as off-site and blocks it; backslash variants
("/\", "\\", "\/") normalize to the same bypass and are rejected
too. Ordinary relative paths still validate.
- The /CMS/Home route and the /CMS/ root redirect both accept any granular CMS permission (Files, Content Blocks, Left Nav, ...), not just base SVMSecure.CMS, reading one shared set so the guard and the canonicalization can't drift.
- Adopt the shared StatusBadge over raw q-badge and an ad-hoc palette, and replace hard-coded px/hex with rem and brand tokens. - Improve a11y (SR-hidden status icons, grouped/labelled left-nav rows) and keep button labels visible while loading. - ContentBlock watches only its name prop (drops a duplicate fetch), renders null-safe, and shares heading styles via the global sheet. - ContentBlockEdit skips the section-path/file APIs for create-only users so they don't fire requests that can only 403; the left-nav list now surfaces load errors.
- Add an inline uploader to the content-block editor: files stage client-side and only upload on Save, using the block's folder and permissions, with per-file conflict resolution (rename/overwrite/reuse) and rollback of newly created files if the save then fails - Make the block's VIPER section path required on create and read-only after, sourced from a new content-scoped /content/folders endpoint so create-only users can pick a folder without AllFiles - Restore legacy 30-day trash retention: a daily CmsTrashPurgeScheduledJob permanently deletes files (record + disk) past the cutoff, detaching content-block links first and tolerating per-file disk failures. Gated by a Cms:TrashPurgeEnabled flag (default off) so it ships disabled until the legacy VIPER 1 purge is retired and survives app restarts - Scope the trash to its owner for non-admins (restore and list match on ModifiedBy, failing closed) and open a Trash entry on the hub; surface the purge date on deleted files - Drop the System and Order columns from the content-blocks list; polish responsive list headers, stacked permission chips, and link a11y
- Treat both '/' and '\' as separators for legacy import lines and user-supplied file/folder names; traversal and invalid-name checks no longer rely on host-OS path APIs (Linux CI let "..\" and "<>" through) - Build test path expectations with Path.Join instead of hardcoded '\' - Clear ReSharper/CodeQL findings in the file services: specific catch filter on the bulk-encrypt save, disposed test streams, redundant qualifiers and suppressions
- Drop always-true null checks and dead coalesces flagged by NRT contracts; flatten EF search conditions CodeQL called too complex - [JsonRequired] on LeftNavItemEdit ints so under-posted JSON is a 400 instead of silently creating items or clearing headers - Test hygiene: dispose form-file streams, remove redundant suppressions and qualifiers, pass TestContext cancellation token
- New useServerTable/useUrlFilteredTable composables own the shared QTable pagination + URL-synced filter plumbing, removing the clone groups jscpd flagged vs main (delta now +0) - Split high-complexity save/upload functions into small helpers; fallow suppressions remain only for template-size synthetics
- GET content/fn/{friendlyName} projected raw ContentBlock entities:
attached files exposed their AES key and server path, plus full
unsanitized ContentHistory and permission rows, to anonymous callers
- Return ContentBlockDto (same consumer fields); regression test asserts
the payload carries no key/filePath/contentHistories
- X-Content-Type-Options: nosniff on all /CMS/Files responses - html/htm/xhtml/svg force Content-Disposition: attachment (keyed off the stored file path, not the user-supplied fn) so an uploaded page cannot render in the app origin; html stays on the allow-list for legacy files
- Align DTO MaxLengths to DB columns (overlong input was a 500 at SaveChanges); validate each permission string via MaxLengthEach - Reject unknown item ids on batch save (409) instead of silently re-creating concurrently deleted items; duplicate ids 400 from the service; friendly-name uniqueness enforced case-insensitively
- DateRangeHelper no longer overflows on DateTime.MaxValue.Date filters - CmsNavMenu takes IUserHelper via constructor like LeftNavMenu - EF.Parameter on the attached-file GUID Contains check - Document the legacy-parity CATS.Admin gate (verified in CF source)
- 75 new Vitest specs: content-block save/conflict flows, inline upload commit paths, file dialog conflicts, table composables, CmsHome personas, list page actions - Worst-covered files now 76-100% lines; patch coverage ~60% -> ~85%
- Mirrors the files list: ApiPagination envelope, whitelisted sort columns, count-before-page; list no longer loads every block client- side and the rows-per-page All option is gone - ContentBlocks.vue now uses useServerTable like its sibling lists; card-mode breakpoint standardized to lt.sm
- Link-collection edit dialog now guards unsaved tag edits on close, matching the link dialog and the DESIGN.md close-guard pattern - Filter bars unified to dense outlined across the five list pages - Tag colors moved off semantic status roles to a categorical palette; diff tints and focus ring tokenized; content-block headings get a descending scale; px to rem; deep-watcher and stale ?upload=1 query-sync race fixed
- loadBlocks sorted a default (alphabetical, 50-row) page client-side, so recent edits past the first page never appeared; request sortBy=modifiedOn perPage=5 server-side like loadFiles, with a spec pinning the params - Drop the bespoke field-error pill on the link form, residual px in InlineFileUpload, and the text-h6 that weakened the rail heading
- Hub boxes now match the left nav's three groups in its order, with Link Collections inside Content Blocks and the nav's Add File / Add Left-Nav Menu deep-links as card actions - Warn file managers when trashed files purge within 7 days (new deletedOn sort on the files list backs it); recently deleted files join the activity rail - Rail items link to their edit history / audit trail and open the latest change as an inline diff (live content vs newest saved version via the editor's POST diff contract); shared useContentDiffViewer replaces three parallel viewer implementations - Row actions fade in on hover/focus, stay visible on touch
- The rail's row markup (actions cluster, verb caption, hover reveal) pushed RecentActivity's template past the complexity threshold; the row is now its own component and the shared item types live in CMS/types
- Serve the anonymous content fn endpoint from a minimal public DTO: the full DTO disclosed editor login ids, permission names, and System/section placement to unauthenticated callers - Restore legacy download auditing: anonymous public hits are audited again (null login/iam), ZIP permission denies are audited at all, and internal 192.168.* requests are excluded like the legacy app - Resolve user permissions once per content-block render instead of per block-permission, mirroring CheckFilePermission - Guard the shared server-table against out-of-order responses so a slow earlier filter request cannot clobber a newer one, and stop query-only navigations (URL-synced filters) from yanking focus to the main landmark mid-keystroke - Guard link-collection loads against failed fetches that crashed the render; drop the dead always-empty GetAllFiles; AsNoTracking on the download-path file lookups - Make the activity row a single stretched title link so its action buttons are no longer interactive controls nested inside an anchor
- Mirror the content-block stale-edit guard on file updates: requests
carry the LastModifiedOn they loaded, a stale value gets 409 with
who saved and when, a missing value gets 400
- CmsConcurrencyException derives from InvalidOperationException, so
the controller catches it first or the 409 would surface as 400
- check-name echoes the conflicting record ModifiedOn so the
overwrite-by-upload path is guarded too: if the target changes
between the conflict prompt and the overwrite, the retry re-checks
instead of silently clobbering
- The edit dialog offers Reload (pulls the latest version into the
form) or Keep editing on conflict, matching the block editor
- Rides along in CmsFileService: save catches narrowed to specific
exception types and the nightly purge tolerates per-file SqlException
- FileFormDialog dirty tracking now sees a selected upload (a File
serializes to {} so choosing one left the form looking clean)
- Add by-mothra photo lookup: the legacy userPhoto.cfm accepted MothraID and the M6 milestone claimed support the code never had - Serve Last-Modified and honor If-Modified-Since with 304s, plus stale-while-revalidate, matching the legacy handler built for pages rendering hundreds of photos; ID-card/nopic photos use a stable per-process timestamp because the delegated Students photo pipeline exposes no per-file mtime
There was a problem hiding this comment.
Pull request overview
Completes the last slice of the CMS migration with security hardening, concurrency safeguards, OS-independent path handling, server-side paging, and UI/composable refactors (plus substantial test coverage) across the ASP.NET CMS API and the Vue/Quasar CMS SPA.
Changes:
- Hardened CMS endpoints and downloads (public DTO for anonymous content, URL validation hardening,
nosniff+ forced attachment for inline-unsafe types). - Added trash retention + optional scheduled purge job (config-gated) and exposed purge dates in API/UI.
- Introduced shared frontend table composables (server paging + URL-synced filters), focus-management fix for query-only navigation, and recent-activity rail improvements.
Reviewed changes
Copilot reviewed 99 out of 99 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| web/Services/HtmlSanitizerService.cs | Minor event handler cleanup |
| web/Classes/Utilities/DateRangeHelper.cs | Clamp MaxValue upper bound |
| web/Areas/CMS/Validation/SafeUrlAttribute.cs | Reject protocol-relative URLs |
| web/Areas/CMS/Validation/MaxLengthEachAttribute.cs | New per-element max-length validator |
| web/Areas/CMS/Services/CmsUserPhotoService.cs | Add MothraId + Last-Modified support |
| web/Areas/CMS/Services/CmsNavMenu.cs | Inject user helper for permissions |
| web/Areas/CMS/Services/CmsLeftNavService.cs | FriendlyName uniqueness + stale-id guards |
| web/Areas/CMS/Services/CmsFileStorageService.cs | Path hardening + batch naming + backups |
| web/Areas/CMS/Services/CmsFileResponse.cs | Download hardening helpers |
| web/Areas/CMS/Services/CmsFileImportService.cs | Cross-OS import parsing + preview naming |
| web/Areas/CMS/Services/CmsFileEncryptionService.cs | Minor IO API cleanup |
| web/Areas/CMS/Services/CmsFileCrypto.cs | Minor IO API cleanup |
| web/Areas/CMS/Services/CmsFileAuditService.cs | Simplify search predicate |
| web/Areas/CMS/Services/CmsContentBlockService.cs | Server paging/sorting + EF optimizations |
| web/Areas/CMS/Models/DTOs/LeftNavDtos.cs | DTO validation + JsonRequired safeguards |
| web/Areas/CMS/Models/DTOs/CreateLinkDto.cs | Update validation namespace import |
| web/Areas/CMS/Models/DTOs/ContentBlockDto.cs | Add minimal PublicContentBlockDto |
| web/Areas/CMS/Models/DTOs/CmsFileRequests.cs | Add LastModifiedOn concurrency field |
| web/Areas/CMS/Models/DTOs/CmsFileDto.cs | Add PurgeOn + overwrite timestamp echo |
| web/Areas/CMS/Models/CmsFileMapper.cs | Map PurgeOn via retention |
| web/Areas/CMS/Models/CmsContentBlockMapper.cs | Add mapping to public DTO |
| web/Areas/CMS/Jobs/CmsTrashPurgeScheduledJob.cs | New gated trash purge job |
| web/Areas/CMS/Controllers/CMSUserPhotoController.cs | Add Mothra endpoint + 304 caching |
| web/Areas/CMS/Controllers/CMSLeftNavController.cs | Map service exceptions to 400/409 |
| web/Areas/CMS/Controllers/CMSFilesController.cs | Trash owner scoping + 409 concurrency |
| web/Areas/CMS/Controllers/CMSContentController.cs | Paging + safe anonymous DTO + folders |
| web/Areas/CMS/Constants/CmsTrash.cs | Retention + config key constants |
| web/Areas/CMS/Constants/CmsPermissions.cs | Document admin permission usage |
| web/Areas/CMS/Constants/CmsFileTypes.cs | Inline-unsafe set + attachment decision |
| web/appsettings.json | Default TrashPurgeEnabled=false |
| VueApp/src/styles/colors.css | Add diff + focus-ring tokens |
| VueApp/src/styles/base.css | Mobile header layout + global content heading styles |
| VueApp/src/shared/tests/create-spa-router.test.ts | Strict equality assertions + focus test |
| VueApp/src/config/colors.ts | Treat arboretum as light background |
| VueApp/src/composables/use-route-focus.ts | Don’t steal focus on same-path nav |
| VueApp/src/components/SortableList.vue | Tokenize CSS values |
| VueApp/src/components/tests/sortable-list.test.ts | Add drag-commit coverage |
| VueApp/src/CMS/types/index.ts | Add purgeOn + activity rail types |
| VueApp/src/CMS/router/routes.ts | Centralize CMS home permissions |
| VueApp/src/CMS/router/index.ts | Canonicalize /CMS/ for granular users |
| VueApp/src/CMS/pages/ManageLinkCollections.vue | Unsaved-changes guard + fetch guards |
| VueApp/src/CMS/pages/LeftNavMenus.vue | Better empty state + error notify + UI tweaks |
| VueApp/src/CMS/pages/LeftNavEdit.vue | Reduce complexity + ARIA grouping |
| VueApp/src/CMS/pages/ImportFiles.vue | Button loading slot + label tweak |
| VueApp/src/CMS/pages/Files.vue | Switch to useServerTable + purge label |
| VueApp/src/CMS/pages/FileAuditLog.vue | Switch to useUrlFilteredTable |
| VueApp/src/CMS/pages/ContentBlockHistory.vue | Switch to shared diff/table composables |
| VueApp/src/CMS/pages/CmsHome.vue | Reorder hub + trash purge warning |
| VueApp/src/CMS/pages/BulkEncrypt.vue | Switch to useServerTable |
| VueApp/src/CMS/file-types.ts | Shared accepted-extension list |
| VueApp/src/CMS/composables/use-url-filtered-table.ts | New URL-synced table composable |
| VueApp/src/CMS/composables/use-server-table.ts | New server-paged QTable composable |
| VueApp/src/CMS/composables/use-content-diff-viewer.ts | New shared diff-view state helper |
| VueApp/src/CMS/components/StatusIcon.vue | Mark icon aria-hidden |
| VueApp/src/CMS/components/RecentActivity.vue | ActivityRow + diffs + deleted files |
| VueApp/src/CMS/components/PermissionChips.vue | Add stacked rendering option |
| VueApp/src/CMS/components/LinkCollections.vue | Guarded fetch + grouping perf improvement |
| VueApp/src/CMS/components/Link.vue | Use StatusBadge + non-semantic palette |
| VueApp/src/CMS/components/ContentDiffDialog.vue | Use CSS tokens for diff colors |
| VueApp/src/CMS/components/ContentBlock.vue | Fix duplicate fetch + narrow watcher |
| VueApp/src/CMS/components/ActivityRow.vue | New row component w/ stretched link overlay |
| VueApp/src/CMS/tests/use-url-filtered-table.test.ts | New composable tests |
| VueApp/src/CMS/tests/use-server-table.test.ts | New composable tests |
| VueApp/src/CMS/tests/test-utils.ts | Install Pinia + seed CMS permissions |
| VueApp/src/CMS/tests/router-canonicalization.test.ts | New canonicalization tests |
| VueApp/src/CMS/tests/person-selector.test.ts | Add out-of-order response test |
| VueApp/src/CMS/tests/link.test.ts | Update tag palette expectations |
| VueApp/src/CMS/tests/link-collections.test.ts | Grouping refactor + dialog guard tests |
| VueApp/src/CMS/tests/left-nav-menus.test.ts | Add failure/delete/navigation tests |
| VueApp/src/CMS/tests/file-form-dialog.test.ts | Add 409 conflict + unsaved guard tests |
| VueApp/src/CMS/tests/cms-home.test.ts | New hub permission + purge-warning tests |
| test/CMS/SafeUrlAttributeTests.cs | Add protocol-relative URL cases |
| test/CMS/MaxLengthEachAttributeTests.cs | New validator tests |
| test/CMS/CmsUserPhotoServiceTests.cs | Add mothra + Last-Modified tests |
| test/CMS/CMSUserPhotoControllerTests.cs | Add 304 + Last-Modified tests |
| test/CMS/CmsTrashPurgeScheduledJobTests.cs | New job gating tests |
| test/CMS/CMSLinkCollectionControllerTests.cs | Update duplicate-name test seed |
| test/CMS/CmsLeftNavServiceTests.cs | Add friendly/stale/duplicate guards tests |
| test/CMS/CMSLeftNavControllerTests.cs | Map exceptions to 400/409 tests |
| test/CMS/CMSFilesControllerTests.cs | Owner-scoped trash + 409 tests |
| test/CMS/CmsFileResponseTests.cs | New nosniff/disposition tests |
| test/CMS/CmsFileImportServiceTests.cs | Cross-OS path + preview rename tests |
| test/CMS/CMSContentPermissionTests.cs | Minor NSubstitute cleanup |
| test/CMS/CMSContentControllerTests.cs | Add folders + public DTO leakage test |
| test/CMS/CmsContentBlockServiceTests.cs | Update for paging/sorting + totals |
| test/Classes/Utilities/DateRangeHelperTests.cs | Add MaxValue clamp test |
Comment on lines
141
to
+145
| // List view: project without Content (it can be large) — the editor loads the full | ||
| // block. Two stages because GetFriendlyURL reads HttpContext and can't translate to SQL. | ||
| var blocks = await query | ||
| .OrderBy(b => b.Title) | ||
| .Skip((page - 1) * perPage) | ||
| .Take(perPage) |
Contributor
Author
- Lift Quasar 560px minimized-dialog cap for the shared md/lg dialog classes; the file dialog silently shrank from 600px when the inline widths moved onto the classes - Allow empty-text header items in left-nav saves: legacy menus use them as spacer rows and the display side still renders them, so a menu containing one could not be re-saved without destroying it. Links still require text (service + editor validation) - Show the folder-prefixed friendly name in the import preview column so it matches the name the Files list will show after import - Document why the Effort audit keeps its bulk Clear Filters (nine cascading filters), unlike the CMS audit trail
This was referenced Jul 3, 2026
Contributor
Author
|
Superseded by the restacked 3-PR stack: #251 (files/photos/import/rate-limit backend) -> #252 (content blocks/left nav/link collections backend) -> #253 (management SPA). These historical cut-point slices predated the branch's later OS-independent path handling and cleanup commits, so they failed CI on Linux runners; the new slices are built from the CI-verified branch tip, carry every review fix (see thread replies), and each passes the full gate set. All feedback on this PR has been answered inline. |
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.
Final slice, part 6 of 6 (stacks on #249 — the diff below shows only this slice). Replaces #242, whose branch predates a rebase onto main and holds the stale pre-rebase history. After this merges, the entire CMS migration is in.
Scope
Cms:TrashPurgeEnabled, default off until the VIPER 1 purge is retired).content/fnendpoint now returns a minimal public DTO (was disclosing editor login ids, permission names, and placement metadata); legacy-parity download auditing (anonymous hits audited, ZIP permission denies audited,192.168.*excluded); single-resolve permission set per content-block render; stale-response guard in the shared server table; query-only navigations no longer steal focus; link-collections fetch guards; activity rows are a single stretched title link (no controls nested in an anchor).ExistingModifiedOnon the name check.by-mothralookup andLast-Modified/If-Modified-Since304 conditional caching withstale-while-revalidate.Deploy notes (whole stack)
CMS:LegacyWebrootPath(import tool),viperfiles.txtkey file must exist (S:\Settings\), optionalCMS:FileStorageRoot/CMS:ProfilePhotoPath/CMS:EncryptionKeyFileoverrides,CMS:DownloadRateLimitlimits,Cms:TrashPurgeEnabledstays false until the legacy VIPER 1 purge is retired.Development→ TEST first, per repo convention.