Skip to content

Add chunked file upload support to asset manager PRESIDECMS-3260 - #1760

Open
alexskinner wants to merge 3 commits into
pixl8:stablefrom
alexskinner:Feature-Chunked-Uploader
Open

Add chunked file upload support to asset manager PRESIDECMS-3260#1760
alexskinner wants to merge 3 commits into
pixl8:stablefrom
alexskinner:Feature-Chunked-Uploader

Conversation

@alexskinner

@alexskinner alexskinner commented Apr 11, 2026

Copy link
Copy Markdown
Contributor

Splits large files into 10MB chunks client-side and reassembles them server-side, bypassing proxy/CDN request size limits (e.g. Cloudflare's 100MB limit) while still enforcing Preside's own file size restrictions. The chunking threshold is configurable via settings.assetManager.chunkingThreshold.

New files:

  • ChunkedUploadService.cfc — chunk storage, assembly, stale session cleanup
  • ChunkedUpload.cfc handler — uploadChunk, finalize, uploadNewVersionPage, finalizeNewVersion actions
  • uploadNewVersionPage.cfm — simplified single-file version upload UI
  • chunkedUploadInterface.js — client-side chunking, Dropzone integration, folder restriction re-validation, compiled Sticker bundle

AssetManager changes:

  • uploadAssets, assetPickerUploader, getUploadRestrictions now always include chunked upload URLs and pass the full system extension list to the client when no folder restriction is set
  • New getUploadRestrictions AJAX action for re-validating queued files when folder selection changes
  • Upload new version button now links to chunkedUpload.uploadNewVersionPage replacing the old hidden file input approach

Bugs discovered and fixed during development (unrelated to chunking):

  • uploadAssets: event.includeData was inside the folder.recordCount condition, so chunk URLs and maxFileSize were never sent when uploading to the root folder (no folder selected)
  • uploadAssets: prc.pageIcon was "picture" instead of "picture-o", causing a missing icon in the admin breadcrumb
  • uploadAssets / assetPickerUploader: unknown file types (e.g. .dmg) were accepted client-side when no folder restriction was set because allowedExtensions was never populated from the system type list, causing a server-side exception instead of a validation message

i18n: added chunked upload error and success keys; all user-facing messages use plain English with no technical terminology


Note

Medium Risk
Introduces a new upload path that streams and reassembles file chunks server-side, touching core asset upload/versioning flows and temporary file handling. Risk is mainly around incomplete chunk sessions, cleanup behavior, and parity with existing per-folder restriction validation.

Overview
Adds client/server chunked upload support to the admin Asset Manager so large files can be uploaded in multiple requests (configurable via settings.assetManager.chunkingThreshold, default 10MB) and then finalized into a single stored asset.

Updates the upload pages to always provide allowedExtensions (falling back to the full system type list), plus new chunk endpoints and an AJAX getUploadRestrictions call to re-validate queued files when the target folder changes.

Reworks “upload new version” to use a dedicated chunked-upload page and server finalize action, replacing the previous hidden file-input flow, and adds new i18n strings for chunked upload success/failure messages.

Reviewed by Cursor Bugbot for commit 076c5e1. Bugbot is set up for automated code reviews on this repo. Configure here.

Splits large files into 10MB chunks client-side and reassembles them
server-side, bypassing proxy/CDN request size limits (e.g. Cloudflare's
100MB limit) while still enforcing Preside's own file size restrictions.
The chunking threshold is configurable via settings.assetManager.chunkingThreshold.

New files:
- ChunkedUploadService.cfc — chunk storage, assembly, stale session cleanup
- ChunkedUpload.cfc handler — uploadChunk, finalize, uploadNewVersionPage,
  finalizeNewVersion actions
- uploadNewVersionPage.cfm — simplified single-file version upload UI
- chunkedUploadInterface.js — client-side chunking, Dropzone integration,
  folder restriction re-validation, compiled Sticker bundle

AssetManager changes:
- uploadAssets, assetPickerUploader, getUploadRestrictions now always
  include chunked upload URLs and pass the full system extension list to
  the client when no folder restriction is set
- New getUploadRestrictions AJAX action for re-validating queued files
  when folder selection changes
- Upload new version button now links to chunkedUpload.uploadNewVersionPage
  replacing the old hidden file input approach

Bugs discovered and fixed during development (unrelated to chunking):
- uploadAssets: event.includeData was inside the folder.recordCount
  condition, so chunk URLs and maxFileSize were never sent when uploading
  to the root folder (no folder selected)
- uploadAssets: prc.pageIcon was "picture" instead of "picture-o",
  causing a missing icon in the admin breadcrumb
- uploadAssets / assetPickerUploader: unknown file types (e.g. .dmg) were
  accepted client-side when no folder restriction was set because
  allowedExtensions was never populated from the system type list,
  causing a server-side exception instead of a validation message

i18n: added chunked upload error and success keys; all user-facing
messages use plain English with no technical terminology
@CLAassistant

CLAassistant commented Apr 11, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ alexskinner
❌ Test


Test seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 3 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 076c5e1. Configure here.

var canonicalTempBase = createObject( "java", "java.io.File" ).init( GetTempDirectory() ).getCanonicalPath();
if ( Right( canonicalTempBase, 1 ) != "/" ) { canonicalTempBase &= "/"; }
return canonicalTempBase & "preside_chunked_" & arguments.uuid & "/";
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path traversal via unsanitized client-supplied UUID

High Severity

The uuid parameter is generated client-side and received via rc.uuid with only a non-empty check. _getTempDir concatenates it directly into a filesystem path without validating it's a proper UUID format. An authenticated attacker could supply ../ sequences in the uuid value, causing saveChunk to write arbitrary binary data to directories outside the temp folder. The base path is canonicalized but the final path including the UUID is not, so .. traversal is resolved by the OS at write time.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 076c5e1. Configure here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ^^. Also the whole java getCanonicalPath should not be necessary. Just something like:

if ( !isValid( "uuid", arguments.uuid ) { // note, isValid( "uuid", uuid ) only returns true for cfml uuids, might need a helper function + some regex
    // throw...
}
return ListAppend( ReReplace( GetTempDirectory(), "/$", "" ), "preside_chunked_#arguments.uuid#", "/" );

, message = translateResource( "cms:assetmanager.chunked.upload.error.assemble" )
} );
return;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temp files leak when assembly fails in finalizeNewVersion

Medium Severity

When assembleTempFile returns success=false (e.g., a chunk is missing), finalizeNewVersion returns early at line 183, before reaching the try/finally block that performs temp directory cleanup. The uploaded chunk files are never deleted. The sibling method assembleAndSave handles this correctly by calling _cleanupTempFiles on failure, but finalizeNewVersion lacks equivalent cleanup on this early-return path.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 076c5e1. Configure here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid ^^

Comment thread system/handlers/admin/ChunkedUpload.cfc Outdated
var tempDir = GetTempDirectory() & "preside_chunked_" & uuid & "/";
if ( DirectoryExists( tempDir ) ) {
try { DirectoryDelete( tempDir, true ); } catch ( any ignored ) {}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temp dir cleanup path bypasses canonical resolution

Low Severity

The finally block in finalizeNewVersion constructs the temp directory path using raw GetTempDirectory(), while ChunkedUploadService._getTempDir() uses Java getCanonicalPath() to resolve symlinks. This inconsistency means the cleanup path may not match the actual directory on systems where the temp dir involves symlinks. The handler duplicates path-building logic instead of delegating to the service.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 076c5e1. Configure here.

@DominicWatson DominicWatson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great. Few bits of review. Particularly important is the tmp directory feedback.

Comment thread system/handlers/admin/ChunkedUpload.cfc Outdated
var saved = chunkedUploadService.saveChunk(
uuid = uuid
, chunkNumber = chunkNumber
, chunkData = FileReadBinary( chunkPath )

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anyway to avoid reading the file - can we just be copying file paths instead? (makes for leaner memory usage if we can).

, message = translateResource( "cms:assetmanager.chunked.upload.error.assemble" )
} );
return;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid ^^

Comment thread system/handlers/admin/ChunkedUpload.cfc Outdated
} );
} finally {
// Clean up the temp directory after addAssetVersion is done
var tempDir = GetTempDirectory() & "preside_chunked_" & uuid & "/";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be moved to a cleanupTmpDir() method (or similar name) in the chunkingService so that all the temp directory logic is in there.

public boolean function saveChunk(
required string uuid
, required numeric chunkNumber
, required any chunkData

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As previously stated, this could be passed as a filepath and we can then just move the file rather than reading the 10mb binary into memory and then writing it back to file.

var canonicalTempBase = createObject( "java", "java.io.File" ).init( GetTempDirectory() ).getCanonicalPath();
if ( Right( canonicalTempBase, 1 ) != "/" ) { canonicalTempBase &= "/"; }
return canonicalTempBase & "preside_chunked_" & arguments.uuid & "/";
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ^^. Also the whole java getCanonicalPath should not be necessary. Just something like:

if ( !isValid( "uuid", arguments.uuid ) { // note, isValid( "uuid", uuid ) only returns true for cfml uuids, might need a helper function + some regex
    // throw...
}
return ListAppend( ReReplace( GetTempDirectory(), "/$", "" ), "preside_chunked_#arguments.uuid#", "/" );

* Clean up abandoned upload directories older than 2 hours.
* Called before each assembly to avoid temp dir accumulation.
*/
public void function cleanupStaleSessions() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be necessary. Lucee does its own tmp directory cleanup

Security: Validate UUID format in handler and service to prevent path
traversal via crafted upload identifiers.

Performance: saveChunk now accepts a file path and uses FileCopy instead
of reading 10MB chunk binary into CFML heap via FileReadBinary.

Cleanup: Centralise temp directory management in ChunkedUploadService
with a public cleanupTempDir() method. Removes inline path construction
from the handler (which used raw GetTempDirectory() vs the service's
getCanonicalPath(), causing potential path mismatches). Remove
cleanupStaleSessions() as Lucee handles temp directory cleanup natively.

Fix temp file leak in finalizeNewVersion where assembly failure caused
an early return before the cleanup finally block.

Fix folder-change re-validation to include Dropzone.ERROR files (not
just ADDED), so files rejected at add time for size/type are properly
re-checked when the user selects a different folder. Save original
detail content on pre-upload errors so it can be restored. Update
Dropzone options on folder change for consistent validation of newly
added files.
@cursor

cursor Bot commented Apr 16, 2026

Copy link
Copy Markdown

You have used all of your free Bugbot PR reviews.

To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

Single source of truth — the handler now calls the service method
instead of maintaining its own private copy of the regex check.
@cursor

cursor Bot commented Apr 16, 2026

Copy link
Copy Markdown

You have used all of your free Bugbot PR reviews.

To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

@alexskinner
alexskinner requested a review from DominicWatson July 4, 2026 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants