PersistentStorage/PSLocal/PSServer/PSFixed Plugins [was: Update the ExtendedFileManager to use new dialogs...]

[douglas]

The ExtendedFileManager, which is the replacement for the deprecated ImageManager, is very heavily tied to the UI, and the popup system. It needs to be cleaned up / refactored /rewritten to use the new dialogs, and ideally updated to clean up the server protocol.

[douglas]

While going through the code to EFM in an attempt to pull out the UI, I got frustrated by how mixed the PHP, HTML, and javascript were. I've created a new series of plugins designed to handle file storage. I say series, because the plugins have been split into two categories: the first, which provides the UI independant of the storage, and then the backend plugins for providing storage. This makes it easier to keep a clean seperation of code, and allows us to do some things that weren't pssible with EFM. The initial changeset is r1121. It's not complete, it still has some bugs which I will continue to work on (especially with regards to the image insertion UI).

[douglas]

Take a look at the Newbie example to see what we have. (Or Extended demo with PSServer, PersistentStorage?, and PSLocal)

I should note that there is extended functionality if jQuery UI is loaded on the page (dag and drop UI elements)

Some problems still to be addressed:

# Handle NO plugins better... # Open Document needs it's own set of details (different from the image details # Copying a file in PSServer creates an 'undefined' entry # Pass a URL to a plugin to see if it's located there (for selecting entry) # Use above to read the selected image for editing it's properties # Double-click to change folders # Go up a directory button doesn't do anything # Make copy/paste work standalone

[nicholasbs]

Another thing to add: Setting the default width/height for the dialogs (should be different for the open/save and insert image dialogs)

[mokhet]

extended functionality with jquery ? Oh come one, this API is crap and should (must) never be used with Xinha or in any Xinha plugins (with no code in fact).

Please, dont make Xinha *and* no plugin rely on JQuery, I dont wanna create once again my own fork version because Xinha core is going in another bad direction. I love Xinha, please don't force me to give up on Xinha.

[douglas]

mokhet, relative opinions of jQuery aside, the plugin does not rely in any way on jQuery. For anyone who is using jQuery already on their page with Xinha, however, they get drag and drop for files. And as of right now, that is a surprisingly large number of web sites.

[ray]

I agree with mokhet that we must not rely on any external library at all (though I like jQuery)

[douglas]

ray, mokhet, with regards to external libraries, frankly, I'd personally like to see Xinha embed a library so that we can concentrate on the WYSIWYG, leaving the navigation and other non-core functionality to the library. I happen to like jQuery, partly because it's only 50k, and thus smaller than many of our images. I'm not interested in it enough, however, to want to fork it to do so, and I have no intentions of trying to shove that down someone's throat. I'm not pushing for that, and that's not how the new plugins work.

I just looked at the amount of code necessary to do drag and drop (it's a lot of ugly code, with all sorts of edge cases) and decided it wasn't worth it to add that functionality to Xinha. Especially not in the time I've had available. However, if you have jQuery already on your page (and there are a lot of sites that do), it was a difference of less than 30 lines of code to enable drag and drop, giving a richer UI for those that want it.

In any case, the drag and drop stuff is a tiny drop of code compared to the rest of the plugins. Things that I've added:

• Local document storage using Google Gears
  • Anyone with Gears gets the ability to save drafts, and templates to their hard drive. Anywhere on the web that they use Xinha, they have access to these files.
  • User configuration of Xinha. Save a file called config.js to your local storage, and it will be loaded wherever you use Xinha. If, as a user, there is a plugin you like, you can configure it to load on all of your Xinhas.
• Separation of backend and UI
  • People using Xinha with a server that already has some sort of storage can write an interface to that, and still get all of the benefits of Xinha's UI.
  • I hope to write a WEBDAV based backend (for 0.97) so that Xinha will work with any WEBDAV server. Combine this with support for WEBDAV+SVN, and we'll be able to use Xinha with subversion. Someone else here is working on Trac, if we do a good enough job, we can get a Trac plugin to use Xinha for repository editing.

[gogo]

I see some immediate problems with this, I don't want to sound like I'm just a "not invented here" moaner as I'm not around enough for that really, just bug reporting and what ifs...

1. PSServer uses json_encode, this function is not part of PHP until recent versions. A backcompat implementation should be supplied so PSServer has some chance to work out of the box. The nightly demo does not work because of this.

2. Loading PersistantStorage? kills the insert image button, even when also loading ImageManager (masks it?).

3. Gears is not available for all platforms, including even my own (64bit linux). It's pointless to load (and mask useful stuff like ImageManager) if it's not supported, isn't it?

4. There are a some functions defined in the global scope of these plugins... "addClass", "removeClass", "treeRecurse" etc, this is a bad idea, put them as part of your object, or if they are really useful then put them inside the Xinha namespace/scope. Polluting the global namespace with such generic sounding functions is asking for trouble. (Don't we already have something like Xinha.addClass anyway?). treeRecurse is also defined separately twice that I noticed, again, trouble lurks in doing that.

5. None of the plugins worked for me (I have no gears, no json_encode, and PSFixed did nothing out of the box) so I can't comment on functionality at the moment.

6. Is PSServer configured in the "normal" way as in ImageManager, Linker, ExtendedFileManager? I didn't see anything like that in the js files, we *certainly* do not want people *under any circumstance* to have to alter the distributed files (plugins/PSServer/config.inc.php specifically) because it makes it much harder to maintain, all configuration should use the established means (in lieu of a better one).

7. I think it is quite unpleasant to have to manually load the required plugins, you should just have to load (say) PSServer and it drags in the requirement of PersistentStorage? (perhaps already does, I can't test).

8. The security patch I applied to ImageManager/ExtendedFileManager? should be applied where applicable here also (PSServer?).

9. The naming of the plugins isn't very good in my opinion, "PSLocal" what does that mean, what does it do etc... let's not have arcane names for things.

1. Can we get some "interface specification" for what a backend must do. I had last year given ImageManager the ability to select images (and videos) from multiple sources such as Flickr and YouTube? (with some after-the-fact help from Javascript to convert a placeholding image into a video, which is why it wasn't committed because I wasn't especially pleased with having to run a javascript on your resultant page to get your videos to display). I think it would be easy to write a "backend" to do the same, but the files are not well commented to see exactly what is required. Some function header comments would not go amiss!

1. I don't see filenames being santised in PSServer/backend.php (although I didn't look too close), it is important that filenames are fairly sanitised, not only for security purposes (never underestimate how determined people can be) but also because having unusual characters in filenames can be a pain in the ass (never underestimate how stupid people can be). At a bare minumum / and \ and all the initial ascii control codes should be stripped, I'd also strip |, &, >, <, and `. Probably ", ' and ~ should also go. Naturally should be configurable, but by default be "locked down".

I know it's a long list of gripey sounding things, but don't get me wrong, I think that this is a long overdue refactorisation of ExtendedFileManager and ImageManager (I've long wanted to see these plugins recombined) and splitting out the "interface" from the "backend" in different plugins is exactly the right way to do it.

[douglas]

Hey James, thanks for the feedback. Don't worry about "gripey"; detailed notes are always very helpful. I've been really busy here at work, but I intend to address all of these before the final release of 0.96.

1. As to json_encode, I noticed that on the demo server, and I've been meaning to include a default implementation.
2. Ok, I'll look at this. It should be just acting like the standard insert image if there are no working backends
3. PersistantStorage? is just the interface. It's supposed to work regardless of the installed backends. If Gears is installed, it is presented for the user, if there is a server backend, that will be presented, if not, it should provide the standard external link entry form...
4. The functions you refer to are not polluting the global namespace. I have the module wrapped inside an anonymous function in order to prevent that. As such, all of the non-namespaced functions are file-local. In any case, after having written this code, I noticed the Xinha.addClass and removeClass. I'll update the code to use them.
5. The fact that none of the plugins worked out of the box is a problem that I'll address. I agree that out of the box experience is very important.
6. With regards to the "normal" way of configuration, I think you're referring to javescript based configuration. In the previous system, there were two sets of configuration files, the javascript, and the php. Users who disabled functionality in the javascript thought they were protected, even though attackers could still get at their server. In PSServer, loading config.inc.php in the browser generates the JSON version of the same config, allowing the frontend and the backend to share the same config. The reason for storing the config in PHP that will than be converted to JSON (and not the opposite way around) is because certain configuration variables should not be exposed to the end user for security reasons.
7. I'll look at having the backend load the frontend if necessary, I could see how this would be annoying.
8. I'll make sure it is.
9. Got any suggestions? PSLocal is PersistentStorage?-Local (Local storage being the established name for client local storage on the web) I could just call it LocalStorage?, especially if I make sure the UI is autoloaded by the backend.
10. Great idea! That's exactly one of the things I was thinking about. I'll create some docs.
11. You're absolutely right that we need to be careful with regards to file naming. At source:/tags/0.96beta/plugins/PSServer/backend.php#L137 there are regexes to deal with invalid file characters as well as reserved file names (See http://en.wikipedia.org/wiki/Filename ). Rather than just stripping the characters and saving the file as something other than what the user asked for, we return an error so that the user can be notified.

[gogo]

Replying to douglas:

1. With regards to the "normal" way of configuration, I think you're referring to javescript based configuration. In the previous system, there were two sets of configuration files, the javascript, and the php. Users who disabled functionality in the javascript thought they were protected, even though attackers could still get at their server. In PSServer, loading config.inc.php in the browser generates the JSON version of the same config, allowing the frontend and the backend to share the same config. The reason for storing the config in PHP that will than be converted to JSON (and not the opposite way around) is because certain configuration variables should not be exposed to the end user for security reasons.

I refer to the ability to (tamper-proofedly) configure the backend in a combination of JS and PHP from your normal Xinha configuration..

 with(xinha_config.ImageManager)
 {
  <?php
     require_once('/path/to/xinha/contrib/php-xinha.php');
     xinha_pass_to_php_backend(
       array(
         'images_dir' => '/path/to/images', 
         'images_url' => '/images',
         'allow_upload' => true
       )
     ); 
   ?>
 }

Without this ability in your plugins, we have a serious problem on two counts as I see it...

1. People must modify your distributed /plugins/PSServer/config.inc.php to set the configuration. Which means that updating Xinha for them is no longer a case of unpack and copy, because now their Xinha is polluted with configured data. Xinha has always stressed that people should never modify the files distributed directly to make configuration, it should always be done in their own code outside of Xinha's folder so that upgrades are easy.
2. It makes it impossible to easily have different backend configurations for different times they use their Xinha, case in point they may want User-1 to not be able to upload files, and User-2 to get this ability - if PSServer only looks at config.inc.php to know if uploading files is permitted, how can both cases be handled with this single static configuration (without serious modification!) In the "normal" way this is a trivial thing to do in their own configuration emitting code.

Not following the "normal" way here is a big detractor (and makes your plugins unusable to me).

[douglas]

Ahhh, okay, I'm sorry I misunderstood... I can see the value in a system like the one you mentioned, I hadn't seen any documentation of xinha_pass_to_php_backend before. My only question is as to why the config data is passed to the frontend at all? Since we're already depending on server side sessions, why not store the config data directly in the session? That way anyone writing custom xinha configuration on the server side will have an easier time of it and there's less chance of a security breach. (For instance, xinha_read_passed_data takes the key location from the client. In xinha code, this is not a problem, because there's only one key in the session. Embedded into another php app, however, and we have an attack vector if the client is able to figure out or pollute the value of a session variable...)

[gogo]

Replying to douglas:

My only question is as to why the config data is passed to the frontend at all? Since we're already depending on server side sessions, why not store the config data directly in the session?

Preservation of resources would be the main (only?) reason I guess :-)

Consider that every time Xinha is loaded, even multiple Xinha's on the same page, it could be using different backend configuration (and infact, quite likely), that means that each and every instance of Xinha realistically needs it's very own backend configuration to be passed to it.

Passing those configurations via SESSION means we wouldn't know when we can garbage collect so we could potentially accumulate hundreds of configurations in $_SESSION which are all useless to us in future.

Currently instead all we have to store in session is one "key" (salt), then everything is passed in through the client, hashed with the salt, and checked for tampering that way.

Your view on the passing of "best kept quiet" data in the open air is valid, we could always encrypt it that passed data, with that random encryption key stored in SESSION. Although any such encryption should be optional (but on by default if supported by the server) to assist in debugging.

Embedded into another php app

I think, if another PHP app on the server is already compromised enough to allow that, then nothing we do will help, they can probably already do whatever they want.

[douglas]

Replying to gogo:

I think, if another PHP app on the server is already compromised enough to allow that, then nothing we do will help, they can probably already do whatever they want.

A quick google search on drupal and $_SESSION turns up that $_SESSIONusername? is used, meaning that drupal and xinha have security risks. With wordpress, a number of plugins use $_SESSION (though the vanilla package does not) so that's a possibility. Gallery uses $_SESSIONlanguage?, so that's a key (though I don't see a use case for xinha in gallery).

My point is that for the three most popular php applications I know of, there is an easy way to use xinha to read and write any data that the web server can write, including script files.

Would you consider the switch to session storage? I could understand the desire not to load the server, but this is one case where I think the benefits outweigh the drawbacks. Especially when we consider that the amount of extra data storage amounts to something around 100-200 bytes, and we'd be saving the extra two hashes, lowering CPU usage as well...

[gogo]

Replying to douglas:

A quick google search on drupal and $_SESSION turns up that $_SESSION['username'] is used, meaning that drupal and xinha have security risks. With wordpress, a

I think what you are saying is that it is possible a third party application (drupal) can reveal our data stored in $_SESSION - that is, there was some way to convince that third party application to output $_SESSION['Xinha:BackendKey']?

In which case, yes I can see there would be a problem.

I guess storing the data itself in the $_SESSION would alleviate that somewhat (as long as they can't convince drupal to WRITE to $_SESSION['Xinha:Config']).

I just don't like leaving all those used trash configs lying in $_SESSION though, mainly because it's untidy, but also because every one extra degrades the performance of PHP a little bit (lots of little bits for lots of users adds up to one big bit).

We could store one instance of each UNIQUE config per session (where fingerprint = md5(serialize($Config)) ), maybe a good compromise.

Hmmmm... here is an untested rewriting of the appropriate functions in contrib/php-xinha.php, I think they should just drop-in-replace the existing ones.

  function xinha_pass_to_php_backend($Data, $ConfigLocation = 'Xinha:Config', $ReturnPHP = FALSE)
  {        
    @session_start();
    if(!isset($_SESSION[$ConfigLocation]))
    {
      $_SESSION[$ConfigLocation] = array();
    }
    
    $bk['session_name']    = session_name();      
    $bk['config_location'] = $ConfigLocation;      
    $bk['fingerprint']     = function_exists('sha1') ? sha1(serialize($Data)) : md5(serialize($Data));
    
    $_SESSION[$ConfigLocation][$bk['fingerprint']] = $Data;
    
    // The data will be passed via a postback to the 
    // backend, we want to make sure these are going to come
    // out from the PHP as an array like $bk above, so 
    // we need to adjust the keys.
    $backend_data = array();
    foreach($bk as $k => $v)
    {
      $backend_data["backend_data[$k]"] = $v; 
    }
    
    // The session_start() above may have been after data was sent, so cookies
    // wouldn't have worked.
    $backend_data[session_name()] = session_id();
    
    if($ReturnPHP)
    {      
      return array('backend_data' => $backend_data);      
    }
    else
    {      
      echo 'backend_data = ' . xinha_to_js($backend_data) . "; \n";  
    }                
  }  

function xinha_read_passed_data()
  {
   if(isset($_REQUEST['backend_data']) && is_array($_REQUEST['backend_data']))
   {
     $bk = $_REQUEST['backend_data'];
     session_name($bk['session_name']);
     @session_start();
     if(!isset($_SESSION[$bk['config_location']])) return NULL;
     if(!isset($_SESSION[$bk['fingerprint']]))     return NULL;
     return $_SESSION[$bk['fingerprint']];     
   }
   
   return NULL;
  }

PS: I also noticed you are using escape() I think you should be using encodeURIComponent(), from memory escape() isn't totally unicode safe?

[gogo]

See Ticket #1350

I think we probably will need to go to putting backend data in $_SESSION.

Douglas, where are your 3 plugins at. Is it abandoned? I don't see any changes to it since we discussed this all above and as far as I know it's still non-functional? I liked your idea, it just needed some work (to make it work) is all.