WordPress.org

Make WordPress Core

Opened 12 years ago

Closed 12 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 12 years ago.
Limits file extensions to php, txt, js, css, html, xml, inc and include
9452.regex.patch (1.1 KB) - added by jbsil 12 years ago.
plugin-editor.php (8.0 KB) - added by bingorabbit 12 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 12 years ago.
URL Fix Patch
9452.1.patch (1.9 KB) - added by jbsil 12 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 12 years ago.
Only allows filter to add to existing list. All other changes also in this patch.
width.patch (581 bytes) - added by bingorabbit 12 years ago.
Error layout CSS Fix
9452.errormessage.patch (3.1 KB) - added by hakre 12 years ago.
9452.3.patch (5.2 KB) - added by hakre 12 years ago.
direct wp_die() vs. later $error / fix of filter
9452.4.patch (5.7 KB) - added by hakre 12 years ago.
removing overseen if ( !$error) condition
9452.5.patch (5.8 KB) - added by hakre 12 years ago.
Patch fixed for current trunk

Download all attachments as: .zip

Change History (45)

#1 @jbsil
12 years ago

  • Owner changed from anonymous to jbsil

#2 @jbsil
12 years ago

  • Keywords plugin_editor added; plugin editor removed

@jbsil
12 years ago

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

#3 @jbsil
12 years ago

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

#4 @jbsil
12 years ago

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

#5 follow-up: @jbsil
12 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

#6 in reply to: ↑ 5 @jbsil
12 years ago

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

#7 @Denis-de-Bernardy
12 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().

#8 @Denis-de-Bernardy
12 years ago

silly wiki formatting. the regex was:

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

@jbsil
12 years ago

#9 @jbsil
12 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.

#10 @jhodgdon
12 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...

@bingorabbit
12 years ago

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

@bingorabbit
12 years ago

URL Fix Patch

#11 @bingorabbit
12 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..

@jbsil
12 years ago

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

#12 follow-up: @jbsil
12 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.

#13 in reply to: ↑ 12 @bingorabbit
12 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.

#14 follow-up: @strider72
12 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

@jbsil
12 years ago

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

#15 in reply to: ↑ 14 @jbsil
12 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.

#16 @ryan
12 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

#17 @hakre
12 years ago

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

@bingorabbit
12 years ago

Error layout CSS Fix

#18 @bingorabbit
12 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.

#19 @hakre
12 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)));

#20 @Denis-de-Bernardy
12 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.

#21 follow-up: @Denis-de-Bernardy
12 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?

#22 in reply to: ↑ 21 @bingorabbit
12 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?!

#23 @hakre
12 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.

#24 @Denis-de-Bernardy
12 years ago

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

#25 @Denis-de-Bernardy
12 years ago

  • Keywords dev-feedback added

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

#26 @Denis-de-Bernardy
12 years ago

  • Keywords tested added

#27 @ryan
12 years ago

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

#28 @Denis-de-Bernardy
12 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

@hakre
12 years ago

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

#29 @hakre
12 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.

#30 @Denis-de-Bernardy
12 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. :-)

@hakre
12 years ago

removing overseen if ( !$error) condition

#31 @hakre
12 years ago

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

#32 @ryan
12 years ago

Patch isn't applying cleanly for me.

@hakre
12 years ago

Patch fixed for current trunk

#33 @hakre
12 years ago

had conflicts, could fix it. please try again.

#34 @ryan
12 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.