Duplicated configs #109
Comments
|
OK, I just re-read the wiki's config section. I saw So I think my concern is strictly for the duplicated options of the config file. There should only be one source for that info, in my opinion. Perhaps there could be a shared Thanks! |
Correct. There has been much debate on the subject. See related issues: #31 and #32 I would also prefer a way to have one config file, or get rid of duplicated options at least, but can't see a right way to do this taking into account the following:
If I got your suggestions right I can see that Option 1 does not meet some points from my list. Option 2 is more appropriate as for me with few remarks:
Despite of all problems I will highly encourage you to find the most optimal and convenient solution. |
|
@servocoder Thank you for the references to the other issues. I read through them. (For the record, though, I did not see the word "standalone", so I'm still unclear what is meant by that.) I strongly agree with your design goal of separating the client and server as much as possible. For example, always assume that someday we may also have a Java-based Android client, and a C++ desktop client, and a terminal-only shell script client, which also talk to the connector for file management. Even if those other clients do not exist, designing for the possibility results in a cleaner, more abstract design. For the configuration options, I see three groups:
As far as I can tell, all of the duplicate config options fall into the third set. The values in Is there any use case where it would make sense for a user to set So, my first attempt at resolving the duplicate options is this:
But that said, I think the duplicate options need more work than simply removing them from the I've put a lot of thought into these, and my evaluation is below; I am happy to work on these and submit PRs. Security Options
"security": {
"allowFolderDownload": true,
"allowChangeExtensions": true,
"allowNoExtension": false,
"normalizeFilename": true
},
/**
* Security section
*/
"security" => [
/**
* Default value "false".
* Allow users to download a Zip archive of a specific folder and contents (including subfolders).
*/
"allowFolderDownload" => false,
/**
* Default value "false".
* Allow users to change extension when renaming files.
*/
"allowChangeExtensions" => true,
/**
* Default value "false".
* If set to "true", allow users to upload file with no extension.
*/
"allowNoExtension" => false,
/**
* Default value "true".
* Sanitize file/folder name, replaces gaps and some other special chars.
*/
"normalizeFilename" => true,
/*** [snip] ***/
],security => allowFolderDownloadI think If I have read permission to individually download 100 files and zip them up on my workstation, then setting There can be a server-side config option to disallow Consider GitHub. If I have access to a repository, then it will let me download the file(s) as a .zip, or a git clone, or an individual raw file. What matters is my read access, not my "zip file access". I would move the option out of the security => allowChangeExtensionsI think If I have read and write permission to download a file and then upload the renamed file, setting I think this is a great option for the security => allowNoExtensionI think It is just a special case of a more general setting: whether or not an extension is allowable. The server will accept the user's desired extension, or not. Either the empty extension value Currently, the server restricts extensions using the options So I propose a similar, but different, security option on the server side. To support limited extensions being shown to the user, there should be a universal list of allowed (or disallowed) extensions. This would be a more global security option that limits all read and write actions on the server, regardless of what security => normalizeFilenameI believe This is functionality is currently implemented in both the client JS and the server-side PHP:
// Sanitize and transliterate file/folder name as server side (connector) way
var cleanString = function(string, allowed) {
if(config.security.normalizeFilename) {
// replace chars which are not related to any language
var replacements = {' ': '_', '\'': '_', '/': '', '\\': ''};
string = string.replace(/[\s\S]/g, function(c) {return replacements[c] || c});
}
// allow only latin alphabet
if(config.options.charsLatinOnly) {
if (typeof allowed == "undefined") {
allowed = [];
}
// transliterate string
string = getSlug(string, {
separator: '_',
maintainCase: true,
custom: allowed
});
// clean up all non-latin chars
string = string.replace(/[^_a-zA-Z0-9]/g, "");
}
// remove double underscore
string = string.replace(/[_]+/g, "_");
return string;
};
public function normalizeString($string, $allowed = [])
{
$allow = '';
if(!empty($allowed)) {
foreach ($allowed as $value) {
$allow .= "\\$value";
}
}
if($this->config['security']['normalizeFilename'] === true) {
// Remove path information and dots around the filename, to prevent uploading
// into different directories or replacing hidden system files.
// Also remove control characters and spaces (\x00..\x20) around the filename:
$string = trim(basename(stripslashes($string)), ".\x00..\x20");
// Replace chars which are not related to any language
$replacements = [' '=>'_', '\''=>'_', '/'=>'', '\\'=>''];
$string = strtr($string, $replacements);
}
if($this->config['options']['charsLatinOnly'] === true) {
// transliterate if extension is loaded
if(extension_loaded('intl') === true && function_exists('transliterator_transliterate')) {
$options = 'Any-Latin; Latin-ASCII; NFD; [:Nonspacing Mark:] Remove; NFC;';
$string = transliterator_transliterate($options, $string);
}
// clean up all non-latin chars
$string = preg_replace("/[^{$allow}_a-zA-Z0-9]/u", '', $string);
}
// remove double underscore
$string = preg_replace('/[_]+/', '_', $string);
return $string;
}The browser sanitizes it before passing it to the server, which then also sanitizes it. At no point is the user given a GUI option to view or interact with the new sanitized name, so why does the JS code need to sanitize it at all? The server will do that anyway. A server should never trust client input of any kind -- always assume the user has hand-crafted an AJAX request to try to break your server. Sanitizing input on the client side is pointless (unless it is somehow part of the user experience), because the client's sanitization can never be trusted. The client just needs to encode the user's input string to the AJAX json format, so the data transfer works correctly. So I would remove the Uploadupload => paramNameRemove. There is no use case for making this configurable. There is no config option to set the name of the request variables like upload => chunkSizeClient-side only, as this only affects user experience. It is meaningless on the server side; the server should only care whether or not an upload exceeds Also, oddly, this $this->options['readfile_chunk_size'] = $this->fm->config['upload']['chunkSize'];This limits memory consumption on the server, but is an invisible option to the client. So I recommend making upload => fileSizeLimitThis is strictly a server-side config option, enforced by the server. But this setting also affects user interface, because the GUI can warn the user their file is too large without having to wait for a doomed upload attempt to fail. So I think the GUI should query the server for this option. upload => policy (and upload => restrictions)This is a server-side option. However, the GUI will want to know what this value is, so it can show the user which file extensions are allowed. So, the client should query the server for this value. (Also, see my note above about having a global list of restricted extensions to replace this option.) download_via_phpI noticed in the discussion threads some comments about direct downloads (using the native web server, such as Apache, without PHP):
I agree that downloading via PHP is more CPU intensive that downloading directly via Apache. However, this "direct download" option is a huge security risk and not appropriate for a file manager. The PHP (or other server-side code) is necessary to enforce any kind of security. Allowing direct downloads bypasses the user's PHP session, including any user authentication or authorization that the PHP has done. It also bypasses all server-side settings such as It looks like this feature is implemented in Config group: optionscharsLatinOnlyThis should be server-side only. Its only purpose is to support backend filesystems that need Latin characters. This is only used by the Javascript in one place: in function cultureThis should be client-side only. If affects what words are displayed to users, and is purely a UI option. However, currently the server uses this option to translate AJAX error messages into human-readable text. I believe this is a design error. The error code from the server should be a well-defined set, not just "some random error text for display". I think the correct solution is to have the AJAX return raw error codes like 'DIRECTORY_NOT_EXIST', 'NOT_ALLOWED_SYSTEM', or 'FORBIDDEN_ACTION_DIR', as part of the well-defined protocol. Then the client can translate those into meaningful strings for humans. The JS code already supports localization with a language file. (And someday, the language 'culture' should be configurable on a per-user basis, so that different users can select different languages even though they are using the same server.) capabilitiesThis option restricts the client/server protocol to a subset of these "actions": $actions_list = ["select", "upload", "download", "rename", "copy", "move", "replace", "delete", "edit"];These are a list of User Interface restrictions, not server restrictions. If this option exists, it should be client-side only. But I believe this option should disappear, as it is a broken security model. For example, "Edit" is a User Interface action -- disabling it here just forces the user to use a local text editor instead of CodeMirror. It does not protect the server's data. And having "copy" does nothing to protect the server's data, if "upload" and "download" are still enabled. See issue #111 for more discussion. SummaryEach of these items above is a code change, and this list represents a considerable amount of work. I am willing to submit PRs for these, but would like more discussion and group buy-in before I start coding. I especially think there should be a design document for security, and major changes to the backend security model (re: issue #111). In my opinion, the security issues mentioned above make RichFilemanager unsuitable for production use in an organization -- at least, for certain combinations of configuration values. Thank You! |
"standalone" simply means the situation when the server-side is not aware of the client-side. Connectors serves to handle incoming requests from the client solely. No shared files. No other intersection points. For example, separation the server-side from the client-side (different servers, etc).
Exactly.
Don't think so. As far as I remember it was requested to make transition to the separated configuration more smoother. I remind you that there has been only one configuration file initially (json).
Excellent analysis! Most of those options has been implemented a long time ago and not all by me. I like your approach and your willingness to contribute. You think critically, but thoughtfully. I believe RichFilemanager needs a fresh perspective. You have got a green light from me to the modifications described above and the things described in the #111. Go ahead and do your best. Feel free to ask if you have any questions. |
Right, just as utilize common language files for server and client. I strongly believe that each connector should have own lang files. It will be hard to support, but one step closer to the full separation. I would appreciate if you will take care of that as well. |
|
Okay! I'll get started, thanks. What branch should I start from? I'd prefer to work from the markdown branch, as I use the Markdown feature, and want all these config changes to be compatible with that. But I'm happy to branch from dev as well. |
|
I still haven't accomplished to review of the markdown branch. Lack of time at the moment. But it's ok, you can create new branch based on the |
|
Hi @dereks, |
|
Hi! Sorry for the slow progress. I just started a new contract, so I've had to take a short break from RichFilemanager. I expect most (or all?) of these changes be done within the next 1 or 2 weeks. I'm using RichFilemanager for my business so it is important to me. (In other news, I have a version basically working using the "jstree" widget. I need to clean up some event handlers, use the small-font theme, and make it a config option, but then I will submit a PR for this too.) |
Nice to hear you are intent upon this issue, 1-2 weeks is ok
Interesting! But keep it at the separate branch, please, and don't mix with changes related to the config separation. Since config separation have higher priority I'm going to review and test it first, in isolation from other changes. |
|
Hi @dereks, how it's going? Let me know if you experience any difficulties with the implementation, please. I would like to know have you started to work on this topic? Thanks! |
|
@servocoder I am almost done with the changes described above. I need some clarification regarding options=>culture. Presently, the server-side connector uses this option to put human-readable text strings into Ajax error messages. Here are some examples: $this->error(sprintf($this->lang('ERROR_UPLOADING_FILE')));
$this->error(sprintf($this->lang('NOT_ALLOWED')));
$this->error(sprintf($this->lang('INVALID_DIRECTORY_OR_FILE')));Here is the error() method for the PHP connector, which returns the Ajax response with a human-readable error string: /**
* Echo error message and terminate the application
* @param string $title
*/
public function error($title)
{
Log::info('error message: "' . $title . '"');
if($this->isAjaxRequest()) {
$error_object = [
'id' => 'server',
'code' => '500',
'title' => $title
];
echo json_encode([
'errors' => [$error_object],
]);
} else {
echo "<h2>Server error: {$title}</h2>";
}
exit;
}In my original post, I said the server-side "culture" option (and with it, the $this->lang() method), should be removed from the server side completely. You replied:
I'm not sure what you meant in your reply. I believe that the server-side should not use any translation files at all. I think that the error field in the Ajax response should be the un-translated strings, 'ERROR_UPLOADING_FILE', 'NOT_ALLOWED', or 'INVALID_DIRECTORY_OR_FILE'. Then, on the client side Javascript, the translation would occur from 'ERROR_UPLOADING_FILE' to "Error uploading file.", or "Errore nell'invio del file.", or "Неудалось загрузить файл.". Is that what you had in mind? (Eventually, I'd like to see RFM allow each end-user to choose their own language, rather than have a language forced upon them by the server administrator.) Finally, as an aside, note that the server-side does not do any translation for its own internal error logging. For example, these messages are not (and should not be) translated: Server-side error logs must frequently be googled for troubleshooting. It is not good for the server admin if their error log has one of 30 different error messages, depending on their language choice. |
|
A quick update re:
because I saw this line in the 3rd-party $this->options['readfile_chunk_size'] = $this->fm->config['upload']['chunkSize'];However, I looked again and the PHP server code never even used this option. It is used for file downloads (not uploads), but the download function of The |
Hm, interesting idea. But I want to draw you attention to few points:
Despite of the notes above I believe your suggestion is much better option than server-side translation files. Go ahead with that.
It's ok. No need to translate logs. They are for server administrator solely.
Yeah, I have also noted that it is never used. I think we can remove Also you noted |
|
@servocoder I just submitted a Pull Request. It contains the changes listed above, minus two: allowNoExtension still exists. It will be replaced in a separate Pull Request that implements a new global blacklist/whitelist feature on the server side, as discussed in #111. culture still exists on the server side. I'd still like to remove this (as discussed above), but there are over 300 lines of code that use this (counting all of the separate connectors). This will be a bigger task that I thought it would be, so it will be in a separate PR. Aside from these two, all "duplicate" config options are now resolved and no longer duplicated. I updated all of the connectors, but have only tested the PHP connector. Note that Java and ASHX would offer some of the config options in their "initiate" methods, but did not implement the meaning of the config option. Some of the connectors (CFM and Python) are way behind and were not updated in this PR. |
|
Thank you! I am going to review your PR in the coming days.
Yeah, I know, they don't completely implement all of PHP connector capabilities, unfortunately.
All connectors except PHP, Java and ASHX are completely outdated, you can ignore them. |
|
@servocoder I just pushed a minor update to the PR. I had forgotten to remove the dead |
|
Could you create new PR for this update? I have already merged the PR and can't see a way to accept this update within the same PR. |
PR 141 (to the dev branch) submitted, thank you.
Sorry, I didn't realize the other PR had already been merged. If it had not been merged yet, then the latest commit would have been automatically included as part of the original PR. |
|
Released in 2.3.2 |
Presently, the user must edit a backend server file (a
config.phpor similar) to set config options, including security options, and also make a copy of matching options infilemanager.config.json. The.jsonfile sets Javascript UI behavior, such as hiding buttons or enabling features, and the (e.g.).phpsets backend server rules (so the user can't bypass Javascript restrictions by hand-crafting an AJAX request).I think there are several problems with having two copies of the config. A new admin of RichFilemanager could easily make a config change in one place, but not the other, leading unexpected behavior or unintended access. Also, having to maintain two different file formats (
.jsonand.php) means the user can't even copy'n'paste the options.I propose having a single config file for all common options, so there is just one copy of the config, in one format.
Option 1:
filemanager.config.jsonplusconfig.phpThe Javascript GUI would read
filemanager.config.jsonvia HTTP (as it does now), but the connector app fileFmApplication.phpwould replace this PHP require:...with code similar to this:
There would still need to be a
config.phpfile for options that are server-side only, such as database passwords and root file path info. Those options should be hidden from the Javascript GUI, which does not need access to the backend server configuration. This option still requires that the user maintain two different formats, with different sets of config options in each file.Option 2: Just
config.phpThere is no way to eliminate
config.php(and equivalents for other connectors), because there needs to be a place to configure the backend. But we can eliminatefilemanager.config.jsonby writing a simple function to return the JSON-formatted config with a call tojson_encode($base_config). Then the URL to grab the config.jsonwould change from:to:
I prefer option 2 because then the user only needs to know one config file format, not two.
I consider this a security issue, so I'm happy to submit a Pull Request. What other requirements exist for the config system?
Thanks!
The text was updated successfully, but these errors were encountered: