WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#9452 closed defect (bug) (fixed)

Plugin Editor lists all files in plugin - should limit to text only

Reported by: jbsil Owned by: jbsil
Milestone: 2.8 Priority: normal
Severity: normal Version: 2.8
Component: Administration Keywords: has-patch tested commit
Focuses: Cc:

Description

The plugin editor lists and allows users to edit files that are not text only, including images. Simplest solution would be to limit to certain file extensions that are always text only.

Attachments (11)

9452.patch (919 bytes) - added by jbsil 6 years ago.
Limits file extensions to php, txt, js, css, html, xml, inc and include
9452.regex.patch (1.1 KB) - added by jbsil 6 years ago.
plugin-editor.php (8.0 KB) - added by bingorabbit 6 years ago.
Just a fix for calling files that don't need to be editted using a GET request in the URL
9452.url.patch (1.2 KB) - added by bingorabbit 6 years ago.
URL Fix Patch
9452.1.patch (1.9 KB) - added by jbsil 6 years ago.
Includes regex to find extensions, apply_filter call for 'editable_extensions', and check for current file as well as the list
9452.2.patch (2.1 KB) - added by jbsil 6 years ago.
Only allows filter to add to existing list. All other changes also in this patch.
width.patch (581 bytes) - added by bingorabbit 6 years ago.
Error layout CSS Fix
9452.errormessage.patch (3.1 KB) - added by hakre 6 years ago.
9452.3.patch (5.2 KB) - added by hakre 6 years ago.
direct wp_die() vs. later $error / fix of filter
9452.4.patch (5.7 KB) - added by hakre 6 years ago.
removing overseen if ( !$error) condition
9452.5.patch (5.8 KB) - added by hakre 6 years ago.
Patch fixed for current trunk

Download all attachments as: .zip

Change History (45)

comment:1 @jbsil6 years ago

  • Owner changed from anonymous to jbsil

comment:2 @jbsil6 years ago

  • Keywords plugin_editor added; plugin editor removed

@jbsil6 years ago

Limits file extensions to php, txt, js, css, html, xml, inc and include

comment:3 @jbsil6 years ago

  • Cc jbsil added
  • Keywords has_patch added
  • Resolution set to worksforme
  • Status changed from new to closed

comment:4 @jbsil6 years ago

  • Keywords plugin-editor has-patch needs-testing added; plugin_editor has_patch removed

comment:5 follow-up: @jbsil6 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

comment:6 in reply to: ↑ 5 @jbsil6 years ago

Replying to jbsil:
Yeah... I didn't understand what worksforme meant when I added it. Whoops?

comment:7 @Denis-de-Bernardy6 years ago

  • Keywords needs-patch added; plugin-editor has-patch needs-testing removed

Maybe add .text and .htm to the list, and lowercase the extension before checking?

Also, it won't work with a weird plugin name like foo.bar/foo.bar.php

Aside, and for what it's worth... testing reveals that using an anchored regex such as /\.([.]+)$/ is much faster than using php's pathinfo().

comment:8 @Denis-de-Bernardy6 years ago

silly wiki formatting. the regex was:

/\.([^.]+)$/

@jbsil6 years ago

comment:9 @jbsil6 years ago

  • Keywords has-patch added; needs-patch removed

Excellent suggestions. Thanks! New patch file reflects the additional extensions, strtolower before check, and regex to find the extension.

comment:10 @jhodgdon6 years ago

Can I make a suggestion? Maybe allow a plugin to modify the list of allowed editable extensions. Something like

$include = apply_filters( 'editable_extensions', array("php", "txt", "js", "css", "html", "xml", "inc", "include"));

Just a thought...

@bingorabbit6 years ago

Just a fix for calling files that don't need to be editted using a GET request in the URL

@bingorabbit6 years ago

URL Fix Patch

comment:11 @bingorabbit6 years ago

  • Cc bingorabbit added

I have added a URL Fix Patch as anyone can access such files in the editors using a GET Request in the URL..

@jbsil6 years ago

Includes regex to find extensions, apply_filter call for 'editable_extensions', and check for current file as well as the list

comment:12 follow-up: @jbsil6 years ago

allow a plugin to modify the list of allowed editable extensions.

Included in 9452.1.patch

I have added a URL Fix Patch as anyone can access such files in the editors using a GET Request in the URL.

Good catch! Your updated code used the first patch shown here, so I changed it to use the regex, the filter, and to exit before the <form> is displayed, with clean html and a moderately useful error message.

comment:13 in reply to: ↑ 12 @bingorabbit6 years ago

Replying to jbsil:

allow a plugin to modify the list of allowed editable extensions.

Included in 9452.1.patch

I have added a URL Fix Patch as anyone can access such files in the editors using a GET Request in the URL.

Good catch! Your updated code used the first patch shown here, so I changed it to use the regex, the filter, and to exit before the <form> is displayed, with clean html and a moderately useful error message.

Excellent, this works greatly here.

comment:14 follow-up: @strider726 years ago

If putting in a hook to allow plugins to modify the list, you should probably just allow them to *add* to the list, rather than overwrite it completely.

Something like I did in this patch: http://trac.wordpress.org/ticket/8964

@jbsil6 years ago

Only allows filter to add to existing list. All other changes also in this patch.

comment:15 in reply to: ↑ 14 @jbsil6 years ago

Replying to strider72:

If putting in a hook to allow plugins to modify the list, you should probably just allow them to *add* to the list, rather than overwrite it completely.

Something like I did in this patch: http://trac.wordpress.org/ticket/8964

That's probably safer, yes. I took a different approach to it than the patch you linked to.

comment:16 @ryan6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [10879]) Don't allow editing of binary files. Props jbsil. fixes #9452

comment:17 @hakre6 years ago

Just FYI: generated links still lack of using &amp; instead of &. see #9432 and patch (patch is in conflict with [10879])

@bingorabbit6 years ago

Error layout CSS Fix

comment:18 @bingorabbit6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

The layout of the error was nasty, the red box expanded to the end of the page, so I added a simple CSS inline fix. I think it may considered reopened till a code developer close.

I really dunnu in such "tiny" situations what should be the action done as I'm a bit new to Trac.

@hakre6 years ago

comment:19 @hakre6 years ago

  • Keywords developer-feedback added

The last attachment should fix the output problems (you're right bingorabbit, that did not work out). I have chosen a wp_die() to display the two new messages. This does not need any css modifications.

If you want an error message box, then I suggest to place it on top by adding a callback instead:

add_action('admin_notices', create_function('', sprintf('print \'<div class="%s"><p>%s</p></div>\';', 'error', $error)));

comment:20 @Denis-de-Bernardy6 years ago

  • Keywords tested dev-feedback added; developer-feedback removed

+1 to the latest patch

@hakre: the error class, applied to a div, places the thingy to the top automatically.

comment:21 follow-up: @Denis-de-Bernardy6 years ago

  • Keywords 2nd-opinion added; tested dev-feedback removed

err... nm that. the point raised by bingorabbit is invalid -- that it extends the full width is actually expected, no?

comment:22 in reply to: ↑ 21 @bingorabbit6 years ago

Replying to Denis-de-Bernardy:

err... nm that. the point raised by bingorabbit is invalid -- that it extends the full width is actually expected, no?

Actually I couldn't get this one, can you please explain?!

comment:23 @hakre6 years ago

  • Keywords 2nd-opinion removed

@denis: 100%/.error/top: well, not with that editor page.

@bingorabbit: denis assumed that 100% is ok here. but it is not this time because of the wrong placed error message.

I still think what wp_die() is best here. the error message now pops mostyl because of better file sanitization and the new check for editable textfiles. because the action is invalid, wp_die() is perfectly fitting here.

comment:24 @Denis-de-Bernardy6 years ago

oh, in that case, might it be missing a div class=wrap, rather?

comment:25 @Denis-de-Bernardy6 years ago

  • Keywords dev-feedback added

(I agree the die call is better though, let's get dev-feedback before updating anything.

comment:26 @Denis-de-Bernardy6 years ago

  • Keywords tested added

comment:27 @ryan6 years ago

wp_die() sounds fine it me. It will allow us to remove some if error blocks and cleanup the code.

comment:28 @Denis-de-Bernardy6 years ago

  • Keywords needs-patch added; has-patch tested dev-feedback removed
  1. patch needs a small refresh to remove the unneeded wrapper on if no error then

@hakre6 years ago

direct wp_die() vs. later $error / fix of filter

comment:29 @hakre6 years ago

  • Keywords has-patch added; needs-patch removed

unneeded wrapper if no error then has been removed, wp_die() direct.

while goind through I saw that you are not able to actually filter the list of extensions, you can only add new ones. this has been simplyfied to filter the actual list.

the i asked myself if it makes sense to use a regex to get the file extension. isn't instrrev or similar faster? but this is backend and one call only so maybe not a need to put the finger on.

comment:30 @Denis-de-Bernardy6 years ago

  • Keywords tested commit added

there is still an unneeded if ( !$error ) call in the default case.

else, tested and good to go imo. codepress makes all of this look really neat. :-)

@hakre6 years ago

removing overseen if ( !$error) condition

comment:31 @hakre6 years ago

denis, thanks for having a look. that is now removed, should be fine now.

comment:32 @ryan6 years ago

Patch isn't applying cleanly for me.

@hakre6 years ago

Patch fixed for current trunk

comment:33 @hakre6 years ago

had conflicts, could fix it. please try again.

comment:34 @ryan6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [11214]) Die on file error. Cleanup. Props hakre. fixes #9452

Note: See TracTickets for help on using tickets.