restricting file upload types #49

kkarpieszuk · 2022-06-23T09:34:42Z

Fixes #35

adds file types natively supported by WP to the Settings > File upload types table so user can now disable them if needed

Test steps:

activate fut plugin
go to wp-admin > settings > file upload types and check if file types with description "WordPress natively registered type" are added and automatically enabled
disable some poplar native file type (for example .jpeg) and save
install wpforms and create form with file upload field
preview form and try to upload .jpeg file -> you should see error file extension is not supported
go to wp-admin > settings > file upload types and re-enable .jpeg file extension
go to form and try to upload .jpeg file -> you should be able to do this

…-file-upload-types

…ion or version migration

cheh

@kkarpieszuk sorry for being silent for so long time 🙏 and thanks for handling it by yourself!

I've made a first round of reviews and here is my feedback:

There is an issue once I've checked out to PR branch without additional actions - *.jpg file type is not allowed, see. The same with other natively registered types, e.g. https://a.supportally.com/4NWVC2, https://a.supportally.com/TUH50E. Env.: WP 5.2 / PHP 5.6.
See comment below.

src/Plugin.php

src/Allowed.php

src/Migrations/Dispatcher.php

… allowed

kkarpieszuk · 2022-06-27T11:56:34Z

@cheh I have fixed issue with missing option to upload native type even though it was checked as available

additionally, I have applied all your suggested changes. In a few comments you asked why I decided for some approach and my general answer is: it was because I changed strategy during the work on this and sometimes I forgot to clean the older apporach (like injecting Plugin class into Allowed). I cleared all such cases and now code should be simpler.

cheh · 2022-06-27T21:08:09Z

Unfortunately, there is PHP Parse error: syntax error, unexpected '->' (T_OBJECT_OPERATOR) in /plugins/file-upload-types/src/functions.php on line 31 on PHP 5.6 / WP 5.2.

I guess the root of the problem is using Plugin::get_instance() - we don't need to wrap it in brackets, e.g. Plugin::get_instance()->get_native_types();

kkarpieszuk · 2022-06-28T06:05:31Z

I moved it to separate line for better clarity (also in one more place where I wrapped it in brackets in the same way). @cheh please continue

cheh

There is an interesting case when the user adds a fake MIME Type with an extension that already exists - https://a.supportally.com/iTIwIg. Personally, I expected that I wasn't able to upload *.numbers file.

cheh · 2022-06-27T21:27:18Z

src/Restrict/Native/Admin.php

+		add_filter( 'fileuploadtypes_settings_transform_native_type', [ $this, 'get_upload_type' ] );
+	}
+
+	// phpcs:disable Generic.Metrics.CyclomaticComplexity.TooHigh


Looks like this phpcs:disable is unnecessary.

it is needed in fact: without this, phpcs says the complexity of this method is too high.

cheh · 2022-06-27T21:33:15Z

src/Restrict/Native/Admin.php

+
+		$stored_types  = get_option( 'file_upload_types', [] );
+		$native_types  = isset( $stored_types['native'] ) ? (array) $stored_types['native'] : [];
+		$enabled_types = isset( $stored_types['enabled'] ) ? (array) $stored_types['enabled'] : [];


Since it is used in several places, perhaps it could be moved into a separate method (maybe introduce a new Helper class) or global function. So, using that we always receive an array with needed keys (enabled, native, custom).

introduced new StoredTypes class with magic __get method. It cries for __set method as well but adding it would be completely out of scope of this ticket: if I did it, I would have to rewrite tons of code in this plugin and it will be not related to native types anymore.

So for the time being I didn't touch any code related to setting/saving data to wp_options

cheh · 2022-06-27T21:48:53Z

src/Restrict/Native/Admin.php

+
+			remove_filter( 'upload_mimes', [ $this->allowed, 'allowed_types' ] );
+
+			self::$unfiltered_types = get_allowed_mime_types();


A good point is to skip types allowed by the FUT plugin.
But what about other third-party plugins that can allow custom/own MIME types?
In that case, we cannot assure that all retrieved types are allowed natively by WordPress.

Does WP have any other ways here?

"native" name is a bit misinforming here, but I didn't find anything better. In fact the logic here is to add support for any file types which are not coming from our json file.

So it doesn't matter if the file type is natively added by WP plugin or some plugin, other than File Upload Types plugin.

Therefore, the logic here is correct: we just want to get all file types which are not affected by FUT plugin. We should keep this piece of code as is.

cheh · 2022-06-28T06:42:44Z

src/Restrict/Native/Admin.php

+		if ( ! empty( $native_types ) ) :
+
+		?>
+		<tr class="section">


Let's create a new abstraction/class for passing data to templates and move HTML markup to a template file.

I can completely do this, but I vote to create an GH issue for this and handle separately as this is not 100% related to the task. I mean this piece of code is, but what do you think about introducing templates for all html we already have? This code to echo pieces of <table> is here and there: it would be great to convert them to templates as a separate task, what to you say?

cheh · 2022-06-28T06:51:09Z

src/Migrations/Dispatcher.php

+	 *
+	 * @since {VERSION}
+	 *
+	 * @return void


IIRC, we don't have an agreement to add return void for method/functions if they don't return anything. Could you please remove it here and in other places?

cheh · 2022-06-28T07:00:30Z

src/Migrations/Dispatcher.php

+		$option_changed = false;
+
+		foreach ( $all_migrations as $name => $callback ) {
+			if ( ! isset( $already_run[ $name ] ) && is_callable( $callback ) && $callback() ) {


Please, add a blank line before if.

cheh · 2022-06-28T07:01:14Z

src/Migrations/Dispatcher.php

+		foreach ( $all_migrations as $name => $callback ) {
+			if ( ! isset( $already_run[ $name ] ) && is_callable( $callback ) && $callback() ) {
+					$already_run[ $name ] = 1;
+					$option_changed       = true;


Please, remove one extra tab-indent here and above.

cheh · 2022-06-28T07:03:38Z

src/Migrations/Dispatcher.php

+		$all_migrations = $this->get_migrations_list();
+		$option_changed = false;
+
+		foreach ( $all_migrations as $name => $callback ) {


$this->get_migrations_list() could be used instead of $all_migrations.

cheh · 2022-06-28T07:14:19Z

src/Settings.php

@@ -290,6 +292,10 @@ public function display_types_table() { // phpcs:ignore Generic.Metrics.NestingL
 					}

 				endif;
+
+					// phpcs:disable WPForms.Comments.PHPDocHooks.RequiredHookDocumentation
+					do_action( 'fileuploadtypes_settings_display_types_table_after_enabled_types' );


awesomemotive/wpforms-phpcs package has an issue and notify about an invalid hook name incorrectly. For better readability the FileUploadTypes namespace should be converted to file_upload_types.

Could you please change nabes for all new hooks?
Obviously, you will have to use phpcs:disable / phpcs:enable for reducing a noise.

cheh · 2022-06-28T07:17:46Z

src/Allowed.php

+	 *
+	 * @var array
+	 */
+	private $enabled_types;


It could be initialized as an empty array.

kkarpieszuk · 2022-06-29T07:42:53Z

@cheh I've applied most of your suggestions and if I didn't I provided my reasoning comments why not.

about this:

There is an interesting case when the user adds a fake MIME Type with an extension that already exists - https://a.supportally.com/iTIwIg. Personally, I expected that I wasn't able to upload *.numbers file.

Yup, but the same is in develop branch so it is not related to the task.

In general I found - but I didn't explore it deeply yet, I am still quite new to FUT plugin and mime types handling in WP - WordPress probably in fact ignores mime types when restricting and looks only on file extension. Weird and I might be wrong, but it requires deeper investigation and that's why I suggest a separate ticket.

So to sum up again:

I've applied suggested changes when neccessary
I've provided comments why I can't change some parts
I suggest two separate tickets: one for html templates and one for the mime issue

kkarpieszuk added 16 commits March 23, 2022 21:42

[wip] Restricting file upload types

d6cb9c7

Merge remote-tracking branch 'origin/develop' into fut/35-restricting…

be912e3

…-file-upload-types

[fixup] works but it needs to enable native types during registration

1a009b7

[fixup] auto enable native file formats

a4d04b7

[fixup] rephrazed description over table

408cbd7

Merge remote-tracking branch 'origin/develop' into fut/35-restricting…

3d44042

…-file-upload-types

[fixup] added working version of the front end filtering

b89ff68

[fixup] moved inits to Plugin class

4912193

[fixup] phpdoc

748c7a7

[fixup] decoupled Allowed and Plugin class

e3ee0cf

[fixup] performance fix for getting native types

e1ab53f

[fixup] phpcs

cedfe75

[fixup] removed todo

5f566dc

[fixup] decoupled dispatcher

4c14e87

[fixup] decoupled Settings and Admin

7ea8dbd

[fixup] added immediate native file type registration after installat…

9c68ffb

…ion or version migration

kkarpieszuk requested a review from cheh June 23, 2022 09:34

kkarpieszuk mentioned this pull request Jun 23, 2022

Restricting file upload types #35

Open

cheh requested changes Jun 27, 2022

View reviewed changes

cheh assigned kkarpieszuk Jun 27, 2022

kkarpieszuk added 2 commits June 27, 2022 13:43

[fixup] fix native file types are disallowed by default and cannot be…

d27e268

… allowed

[fixup] applied suggested changes

05e4b57

kkarpieszuk assigned cheh and unassigned kkarpieszuk Jun 27, 2022

[fixup] plugin instantiation fix

d599f47

cheh requested changes Jun 28, 2022

View reviewed changes

cheh assigned kkarpieszuk Jun 28, 2022

cheh removed their assignment Jun 28, 2022

kkarpieszuk added 4 commits June 29, 2022 09:06

[fixup] new StoredTypes helper class

c03506e

[fixup] cleaned Dispatcher

f635358

[fixup] renamed hooks

de17721

[fixup] parameter default value as array

dd68c81

kkarpieszuk assigned cheh and unassigned kkarpieszuk Jun 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

restricting file upload types #49

restricting file upload types #49

kkarpieszuk commented Jun 23, 2022

cheh left a comment

kkarpieszuk commented Jun 27, 2022

cheh commented Jun 27, 2022 •

edited

Loading

kkarpieszuk commented Jun 28, 2022

cheh left a comment

cheh Jun 27, 2022

kkarpieszuk Jun 29, 2022

cheh Jun 27, 2022

kkarpieszuk Jun 29, 2022

cheh Jun 27, 2022

kkarpieszuk Jun 29, 2022

cheh Jun 28, 2022

kkarpieszuk Jun 29, 2022

cheh Jun 28, 2022

kkarpieszuk Jun 29, 2022

cheh Jun 28, 2022

cheh Jun 28, 2022

cheh Jun 28, 2022

cheh Jun 28, 2022

kkarpieszuk Jun 29, 2022

cheh Jun 28, 2022

kkarpieszuk commented Jun 29, 2022


		remove_filter( 'upload_mimes', [ $this->allowed, 'allowed_types' ] );

		self::$unfiltered_types = get_allowed_mime_types();

restricting file upload types #49

Are you sure you want to change the base?

restricting file upload types #49

Conversation

kkarpieszuk commented Jun 23, 2022

cheh left a comment

Choose a reason for hiding this comment

kkarpieszuk commented Jun 27, 2022

cheh commented Jun 27, 2022 • edited Loading

kkarpieszuk commented Jun 28, 2022

cheh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkarpieszuk commented Jun 29, 2022

cheh commented Jun 27, 2022 •

edited

Loading