Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Even examples shouldn't have massive security holes. #14

Open
Danack opened this issue Sep 7, 2017 · 5 comments
Open

Even examples shouldn't have massive security holes. #14

Danack opened this issue Sep 7, 2017 · 5 comments

Comments

@Danack
Copy link

Danack commented Sep 7, 2017

The example code given for the PHP documentation has a massive security hole.

$response = FroalaEditor_File::delete($_POST['src']);


public static function delete($src) {

    $filePath = $_SERVER['DOCUMENT_ROOT'] . $src;
    // Check if file exists.
    if (file_exists($filePath)) {
      // Delete file.
      return unlink($filePath);
    }

    return true;
  }

That code allows anyone who knows what the delete URL is, to delete any file off the server, that PHP has permissions to delete.

This is sub-optimal.

Even though it is just example code, there should be some example lines that check that the file being deleted is under the appropriate directory, and probably also a note that there should be a permissions check to ensure the user is allowed to delete images.

@NunoLopesPT
Copy link
Contributor

Hi @Danack ,
I don't think the verifications should happen on the DiskManagement Class, so I updated the examples as you can see:

<?php

require __DIR__ . '/vendor/froala/wysiwyg-editor-php-sdk/lib/FroalaEditor.php';

try {
	$src = $_POST['src'];
	$upload_folder = "/uploads/";

	// If the file is inside the uploads folder
	if (substr($src, 0, strlen($upload_folder)) === $upload_folder)
	{
		$response = FroalaEditor_File::delete($src);
		echo stripslashes(json_encode('Success'));
	}
	else
	{
		echo stripslashes(json_encode('Error'));
	}
} catch (Exception $e) {
	echo $e->getMessage();
	http_response_code(404);
}

Regarding the permissions any idea to approach this? It may depend on the developers choice to do this

@shreypasari-accolite
Copy link
Contributor

Hi @stefanneculai
This is an example and this security hole can be taken care by a programmer because it depends where he wants to upload his files/images, so can he only put a check on it that the user is not trying to delete anything from somewhere else.
Thank You

@stefanneculai
Copy link
Contributor

@shreypasari-accolite can we update the examples with some code to avoid that? Thanks in advance.

@shreypasari-accolite
Copy link
Contributor

@stefanneculai
Raised PR for the mentioned thing in the examples.
Thank You

@stefanneculai
Copy link
Contributor

It's updated now on https://github.com/froala/editor-php-sdk-example, we'll update shortly on the website too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants