Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AssetManager: Configurable upload name suffix for multi file uploads #5672

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

merlinschumacher
Copy link
Contributor

This PR adds the option multiUploadSuffix to the AssetManagerConfig. This makes it possible to replace the currently hard-coded value [] with a custom one or just an empty string.

To ensure the change causes no breakage for existing setups, the value of multiUploadSuffix defaults to []. Developers who would like to have another or no suffix at all can change it to their needs.

A related issue was raised already in #693, but closed with the argument, that the name property of a Content-Disposition line represents an array. I know the brackets are common practice in PHP, where they are processed as an indicator for an array of uploaded files, so there is a valid case for the brackets being there. The reasoning for this patch is, that in ASP.NET there is an integrated type (IFormFileCollection) that can receive multi-part/form-data-uploads. When using it, the file list is automatically mapped to the variable with a name that matches name property. This mechanism fails, because the variable name contains brackets.

There is a workaround for ASP.NET, but that requires maintaining a "manual" extraction of the files from the incoming request or creating a middle ware. Using a framework provided method is less work, more reliable and secure. Especially because we're dealing with binary file uploads here.

@artf artf merged commit 6c519ec into GrapesJS:dev Feb 12, 2024
2 checks passed
@artf
Copy link
Member

artf commented Feb 12, 2024

Thanks @merlinschumacher appreciate the detailed reasoning of the PR

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.

2 participants