Drupal File Upload Renames Square Bracket Characters

Information technology looks like the work done on #2139287: Renaming 'File' uploads with tokens is not yet available for D8.

A short description of the effect is that information technology would exist overnice to be able to rename files uploaded via a webform by using tokens.

Comment	File	Size	Writer
#28	Screen Shot 2018-12-xix at 4.45.45 PM.png	24.12 KB	jrockowitz
#17	interdiff-2941286-fourteen-16.txt	477 bytes	bserem
#17	2941286-file-renaming-16.patch	22.74 KB	bserem
	viii.x-v.x: PHP v.6 & MySQL 5.5, D8.4 695 pass
#fourteen	interdiff-2941286-thirteen-14.txt	862 bytes	bserem
#14	2941286-file-renaming-14.patch	22.74 KB	bserem
	8.10-5.x: PHP five.6 & MySQL five.5, D8.four 695 laissez passer
#13	interdiff-eleven-13.txt	4.27 KB	bucefal91
#thirteen	2941286-file-renaming-xiii.patch	22.68 KB	bucefal91
	viii.x-five.x: PHP five.vi & MySQL five.five, D8.4 694 pass
#11	2941032-filename-pattern-11.patch	22.54 KB	bucefal91
	eight.x-5.x: PHP 5.6 & MySQL v.5, D8.four 690 laissez passer
#xi	interdiff-9-11.txt	ii.88 KB	bucefal91
#9	2941032-filename-pattern-9.patch	21.98 KB	bucefal91
	8.x-5.x: PHP 5.6 & MySQL 5.5, D8.4 689 pass
#ix	interdiff-8-nine.txt	7.one KB	bucefal91
#eight	interdiff-6-8.txt	3.17 KB	bucefal91
#8	2941032-filename-pattern-8.patch	14.88 KB	bucefal91
	viii.x-v.ten: PHP 5.6 & MySQL 5.5, D8.four 688 pass
#half dozen	2941032-filename-design-half dozen.patch	14.8 KB	bucefal91
	8.ten-5.ten: PHP 5.6 & MySQL 5.5, D8.4 668 laissez passer, 32 neglect
#6	interdiff.txt	10.42 KB	bucefal91
#4	2941032-filename-pattern-iv.patch	7.45 KB	bucefal91
	8.ten-v.x: PHP v.6 & MySQL v.5, D8.4 688 pass PHP 5.5 & MySQL 5.5, D8.4 688 pass

Support from Acquia helps fund testing for Drupal

Comments

• Log in or register to mail service comments

Jacob, if y'all similar, I can take this ane. As far as I can run into the following should exist done:

• Innovate necessary config UI for WebformManagedFileBase (and all its children) to specify a destination
• Enrich the back up of tokens (the available token data should be: current-user, webform_submission, and global ones)
• Actually lawmaking it so the file is saved under the specified name.
• Spice it all with tests :)

Just one dubiety: I would like to propose to unify the concept of "destination binder" and the "destination proper noun" to keep it simpler. Then the config that controls which folder and nether which name the file is saved is a single one. One example would look: webforms/[webform_submission:webform:id]/[webform_submission:series]/[webform_submission:first_name]. Then it would contain all but the scheme (public vs private, which is already controlled separately) and without the extension (since nobody would always want to change a file extension).

Does it audio ok?

• Log in or register to postal service comments

@bucefal91 Feel costless to take on this job and reassign this ticket to yourself.

It is very important that we don't let people customize the webforms/[webform_submission:webform:id]/[webform_submission:serial]/ path because it is used by the CSV export to include uploaded files. This path design also prevents whatever filename collisions.

We merely want to add together the ability to enter a custom filename with token support.

What do you think the new property should be called?

Probably #file_name is fine.

How do we handle multiple file uploads?

We might accept to autoincrement or disable file renaming.

• Log in or register to postal service comments

Here's the business concern logic hammered. I all the same wanna practise a few things:

• Practise proper dependency injection in WebformManagedFileBase.
• Introduce necessary extra test cases

I've basically introduced a new protected method which determines the destination URI for a merely uploaded file and placed all the related logic into information technology (thus offloading a little bit the ::postSave()).

Tested manually:

1. I file without sanitization and without renaming.
2. One file with sanitization and without renaming.
3. One file without sanitization and with renaming.
4. I file with sanitization and with renaming.
5. Multiple file with renaming. The files are saved as somefile.txt, somefile_0.txt, and so forth

I am throwing this patch generally to see if anyone wanna try it and to see what test bot says.

• Log in or annals to post comments

• Log in or register to post comments

Dependency injection is added in this patch throughout the WebformManagedFileBase class. Over again I wanna confirm testbot likes it.

• Log in or register to post comments

I didn't account for the fact that file-based elements have soft dependency on file module in my concluding patch.

• Log in or annals to mail comments

This patch is close to the final. It includes the new functionality and the new feature is also covered past Simpletests. Namely, if you go to edit any file-based webform element you will see a checkbox "Rename file" and if you enable it, you volition see a text field where you tin specify tokenized filename.

There is just 1 question I wanna bring up: is information technology OK to give capricious (anonymous) users ability to literally command filename? The user cannot jailbreak the scheme (public/individual) nor he can change the extension. Merely he can inject whatsoever symbol into filename. By itself from PHP point of view there is probably no injure, just information technology may be hurtful on the filesystem level or any other post-processing which might be there on height of a webform submission (emails, exporters, etc). Well, frankly, I am kind of scared to requite such level of control. So I vote in favor of allowing this filename only if the sanitization checkbox is enabled. (Note: this enforcement is not even so enabled in the patch I am attaching).

• Log in or annals to post comments

Below is some general code related feedback and compliments. I will have time to fully test the patch next week.

1.                     +++ b/src/Plugin/WebformElement/WebformManagedFileBase.php @@ -32,xi +32,91 @@ abstract course WebformManagedFileBase extends WebformElementBase { +   * @param assortment $configuration +   * @param string $plugin_id +   * @param mixed $plugin_definition +   * @param \Psr\Log\LoggerInterface $logger +   * @param \Drupal\Cadre\Config\ConfigFactoryInterface $config_factory +   * @param \Drupal\Core\Session\AccountInterface $current_user +   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager +   * @param \Drupal\Cadre\Return\ElementInfoManagerInterface $element_info +   * @param \Drupal\webform\Plugin\WebformElementManagerInterface $element_manager +   * @param \Drupal\webform\WebformTokenManagerInterface $token_manager +   * @param \Drupal\webform\WebformLibrariesManagerInterface $libraries_manager +   * @param \Drupal\Core\File\FileSystemInterface $file_system +   * @param \Drupal\file\FileUsage\FileUsageInterface|NULL $file_usage +   * @param \Drupal\Component\Transliteration\TransliterationInterface $transliteration +   * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager                                      

   Delight add descriptions since Drupal's mode checker is going to be asking for them.

2.                     +++ b/src/Plugin/WebformElement/WebformManagedFileBase.php @@ -32,11 +32,91 @@ abstract class WebformManagedFileBase extends WebformElementBase { +    $max_filesize = $this->configFactory->get('webform.settings')->get('file.default_max_filesize') ?: file_upload_max_size();                                      

   This change is going to conflict with #2941471: Setting a Managed file element'southward file size to the aforementioned as the default does not save the setting to config, results in other environments having incorrect setting. We should probably commit that much simpler patch outset.

3.                     +++ b/src/Tests/Chemical element/WebformElementManagedFileTest.php @@ -48,6 +50,47 @@ class WebformElementManagedFileTest extends WebformElementManagedFileTestBase { diff --git a/tests/modules/webform_test/config/install/webform.webform.test_element_managed_file_name.yml b/tests/modules/webform_test/config/install/webform.webform.test_element_managed_file_name.yml                                      

   Thanks for calculation the exam form it will make it really easy for people to review this patch

• Log in or register to post comments

Hey! :)

your #one is addressed in this new patch.

As far as #2 - yeah, allow'south commit the other patch first and and so I will just post a re-ringlet hither resolving the disharmonize.

I shifted the pending question into the main ticket description so we do not overlook it.

• Log in or annals to mail service comments

Since every form element tin can be edited via YAML it is very important to go along the backdrop equally unproblematic equally possible.

So instead of storing...

                file:   '#type': managed_file   '#title': File   '#file_name': true   '#file_name_pattern': 'file_[token]'                              

We should just store...

                file:   '#type': managed_file   '#title': File   '#file_name': 'file_[token]'                              

1.                     +++ b/src/Plugin/WebformElement/WebformManagedFileBase.php @@ -44,6 +139,eight @@ abstruse class WebformManagedFileBase extends WebformElementBase { +      'file_name' => FALSE,                                      

   Can we merge the #file_name_pattern property into the #file_name property.

2.                     +++ b/src/Plugin/WebformElement/WebformManagedFileBase.php @@ -637,half-dozen +695,21 @@ abstruse grade WebformManagedFileBase extends WebformElementBase { +    $form['file']['file_name_pattern'] = [ ... +    ];                                      

   Can you try using the new webform_checkbox_value element which I just committed via #2941740: Provide a checkbox value input.

   The form element would get...

                       $course['file']['file_name'] = [   '#type' => 'webform_checkbox_value'   '#championship' => $this->t('Rename files'),   '#clarification' => $this->t('Rename uploaded files to this tokenized pattern. Do not include the extension here. The bodily file extension will be automatically appended to this pattern.'),   '#chemical element' => [     '#type' => 'textfield',     '#title' => $this->t('File name pattern'),   ] ];                                      

   Permit's use 'Rename files' every bit the characterization since multiple files volition be renamed.

3.                     +++ b/src/Plugin/WebformElement/WebformManagedFileBase.php @@ -686,six +759,58 @@ abstruse class WebformManagedFileBase extends WebformElementBase { +    if (isset($element['#file_name']) && $element['#file_name'] && $element['#file_name_pattern']) {                                      

   This would need to but look for and use the #file_name holding.

I don't call up we should crave 'Sanitize file name' to be checked just we should conditionally display an inline warning when 'Rename files' is checked and not 'Sanitize file name'.

• Log in or register to mail comments

Jacob, your feedback is fully incorporated. Additionally I have rebased my local branch against latest viii.x-5.x as there was a disharmonize. I made the warning bulletin read: For security reasons we propose to use Rename files renaming along with the Sanitize file proper noun option. -- seems like a reasonable one to me :)

Equally far as I am concerned, I deem it commit set up :)

• Log in or register to postal service comments

First impressions are ok, I'd similar to say that allowing larger values on the rename pattern text field makes sense to me, since this is field where tokens will be used, and it is quite like shooting fish in a barrel to reach the limit if it is a pocket-sized ane.

I was expecting this to be really big because it was not limited, merely information technology wasn't, so I went this:

                diff --git a/src/Plugin/WebformElement/WebformManagedFileBase.php b/src/Plugin/WebformElement/WebformManagedFileBase.php index 033e6145..7d9602a1 100644 --- a/src/Plugin/WebformElement/WebformManagedFileBase.php +++ b/src/Plugin/WebformElement/WebformManagedFileBase.php @@ -703,6 +703,7 @@ abstract class WebformManagedFileBase extends WebformElementBase {        '#element' => [          '#blazon' => 'textfield',          '#championship' => $this->t('File proper name pattern'), +        '#maxlength' => 512,        ]      ];      $form['file']['sanitize'] = [              

Other than that I can verify that the patch works.

Attached is an improved patch file and the interdiff.

Thanks @bucefal91 for the work!

• Log in or annals to post comments

PS: the patch from 13 is RTBC by me.
The improvements in xiv are modest, merely lets expect for someone to cheque them before we change the issue status.

• Log in or annals to post comments

@bserem Your point almost the #maxlength is a valid and the #maxlength should be set to Nothing which volition let for an unlimited number of characters/tokens.

• Log in or register to post comments

• Log in or annals to postal service comments

Thank you for the tip! New patches on comment above

• Log in or register to mail service comments

I besides back up the improvement from #17. :)

• Log in or register to post comments

@bserem @bucefal91 I don't have fourth dimension to review the patch but it looks fine.

If you both feel this is RTBC then @bucefal91 please write a simple change notice and commit the patch.

• Log in or register to post comments

Status:

Needs review

» Stock-still

Yoohoo! :) My 1st commit into the module :) The patch #17 just landed.

• Log in or register to mail service comments

Well done @bucefal91!

Excellent work

• Log in or register to mail service comments

Status:

Fixed

» Airtight (stock-still)

Automatically closed - issue fixed for 2 weeks with no activity.

• Log in or register to post comments

Comment #26

GiorgosK

Chios, Greece 🇬🇷

Credit Attribution: GiorgosK commented 1 August 2018 at 09:16

very prissy work, very much needed

have one question how do you rename on multi value field ?

lets say I take a field attach and the files submitted are

file1.pdf
file2.doc

and the rename blueprint is set to [webform_submission:values:attach]-[current-engagement:raw]

then I will get 2 files names every bit such

file2-1533114159.pdf
file2-1533114159.md

only this renaming can create problem and should exist considered a issues
or am I using the filename token wrong ?
is there a token for each filename ?

• Log in or annals to post comments

In #12, jrockowitz writes:

conditionally display an inline warning when 'Rename files' is checked and non 'Sanitize file proper noun'

However, we are seeing this message in the contrary situation--when sanitize is checked and rename is unchecked. Is this intentional? Is it actually a security issue to sanitize without renaming? Is it better to not sanitize or rename at all? In the latter situation, we aren't seeing whatever warning.

We're training our customers to utilise this grade and the "security" message worries them. The rename option requires them to write a regex design. Training them to use regex is probably not possible.. nosotros might opt to write a patch to remove this message from the code instead...

• Log in or register to post comments

However, we are seeing this message in the opposite situation

Below is the warning that is displayed when 'Rename files' is checked and 'Sanitize file name' is unchecked.

The rename option requires them to write a regex design.

This is not truthful. The rename selection but optionally supports tokens. This is why the file name should be sanitized because tokens can incorporate any blazon of value.

• Log in or register to post comments

moorepura1937.blogspot.com

Source: https://www.drupal.org/project/webform/issues/2941286

Share this post