[D8] N1ED - Bootstrap Editor for CKEditor; File Manager, Image Editor, Widgets, custom templates and much more

Thanks for your contribution !

Please fix below warning
Review of the 8.x-2.x branch (commit 7fb92fe):
DrupalPractice has found some issues with your code, but could be false positives.

FILE: ...rupal/pareviewsh/pareview_temp/flmngr/lib/composer/autoload_real.php
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------
9 | WARNING | Class name must be prefixed with the project name
| | "N1ed.info"
--------------------------------------------------------------------------

Time: 4.48 secs; Memory: 8Mb

Please also add in info file to support d9 core_version_requirement: ^8 || ^9
https://www.drupal.org/docs/8/creating-custom-modules/let-drupal-8-know-...

The module includes code that is licensed under GPLv3: https://packagist.org/packages/edsdk/flmngr-server-php and https://packagist.org/packages/edsdk/file-uploader-server-php. Those libraries should be added as Composer dependencies. Including them in drupal.org repositories, you are releasing them as GPLv2+.

Dependencies that are available on external sites should not be committed in the drupal.org repositories.

I will review the code in the next 12 hours.

• What follows is a quick review of the project; it doesn't mean to be complete
• For every point, I didn't make a complete list of where the code should be fixed, but an example of what is wrong in the code
• Not all the points are application stoppers; some of them describe changes that would be preferable to make

/**
 * {@inheritdoc}
 */
function n1ed_help($path, $arg) {
  switch ($path) {
    case "help.page.n1ed":
      return "

                <h3>About N1ED</h3>
            
                <p><a href=\"https://n1ed.com\">N1ED add-on for CKEditor</a> adds a lot of features to your editor. N1ED is a multi-add-on meaning it will connect different plugins from N1ED Ecosystem which you specify in the preferences.</p>


                <h3>Installation</h3>
                
                <p>The installation process is typical for Drupal 8 - just install N1ED module, all dependencies will be linked automatically. Also this module will attach to those Text Formats which have CKEditor and are fine to be used by article editors. For example it will attach to \"Full format\" and will not to \"Plain text\" or \"Basic HTML\".</p>
                
                
                <h3>Configuration</h3>
                
                <p>Acting as standard CKEditor Drupal 8 submodule N1ED will be enabled in those Text Formats which have CKEditor and where N1ED is not disabled. Go to the <a href=\"/admin/config/content/formats\">Text Formats page</a> and you can see the badge \"N1ED\" near formats N1ED is marked enabled in. Go into text format to configure N1ED there by clicking \"Configure\" button.</p>
                
                <p>When you are on some text format page, first you need to set you N1ED API key once. N1ED configuration widget will lead you through this simple process and attach existing N1ED account or register a new one for free. Your default API key is demo key, and it is also workable but does not have access to some online services, so we recommend you to change it first of all.</p>
                
                <p>Why do you need to link an account? It's easy: because your API key is a reference to your configuration. N1ED will be auto updated using CDN, also cloud of N1ED provides some services like getting screenshots of custom blocks you define, storing configurations and sharing them between your different websites if required, fast switching configurations, and more services in future versions.</p>
                
                <p>Then you can enable or disable N1ED for each text format independently. It is recommended to use N1ED in text formats available for administrator/articles editor users and disable for restricted formats which used in comments form or somewhere like it.</p>
                
                
                
                <h3> Editing an articles</h3>
                
                <p>You will go to the article page as before and edit your content with CKEditor powered with N1ED, Bootstrap Editor, File Manager, Image Editor and other currently available and available in future plugins for CKEditor which are published inside <a href=\"https://n1ed.com/plugins\">N1ED Ecosystem</a>.</p>
                
                
                
                <h3>Troubleshooting</h3>
                
                <p>If you do not see these features on your CKEditor, please be sure you chose appripriate Text Format (which has N1ED attached). In some cases your default text format can be \"Basic HTML\" which may require to switch fo \"Full HMTL\" in the Drupal combobox right under CKEditor area.</p>
                
                <p>In case of any problems please check <a href=\"https://n1ed.com/docs\">documentation</a> or <a href=\"mailto:support@n1ed.zendesk.com\">ask support via e-mail</a>.</p>
            
            ";
  }
}

{@inheritdoc} is not used for hook implementations, which are documented, for example, as Implements hook_help().

Any string shown to the users needs to be translatable. The full output of the hook_help() implementation and other functions isn't translatable.

flmngr:
  path: '/flmngr'
  defaults:
    _controller: '\Drupal\n1ed\Controller\FlmngrController::flmngr'
    _title: 'Flmngr file manager'
  requirements:
    _permission: 'edit own content'

edit own content isn't a permission Drupal core nor this module defines. It's also not clear in which way that page is related to content (i.e. nodes).

n1edSetApiKey:
  path: '/n1ed/setApiKey'
  defaults:
    _controller: '\Drupal\n1ed\Controller\N1EDController::setApiKey'
    _title: 'Set API key request'
  requirements:
    _permission: 'administer site configuration'

With a path like /n1ed/setApiKey, the settings page will not be visible from any page. Settings pages normally have path starting with /admin/config.

class N1EDController extends ControllerBase {

  /**
   * Drupal\Core\Config\ConfigFactoryInterface definition.
   *
   * @var Drupal\Core\Config\ConfigFactoryInterface
   */
  protected $configFactory;

  /**
   * {@inheritdoc}
   */
  public function __construct(ConfigFactoryInterface $config_factory) {
    $this->configFactory = $config_factory;
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('config.factory')
    );
  }

  /**
   * Sets API key into Drupal config.
   */
  public function setApiKey() {
    if (!isset($_POST["apiKey"])) {
      throw new AccessDeniedHttpException();
    }

    $config = $this->configFactory->getEditable('n1ed.settings');
    $config->set('apikey', $_POST["apiKey"]);
    $config->save(TRUE);

    return new Response();
  }

}

Settings are changed using Drupal forms. How are administrator users supposed to send a POST request to set the API key?
Drupal code should not directly access $_POST.

include $this->fileSystem->realpath(drupal_get_path('module', 'n1ed') . '/flmngr/flmngr.php');

Now that Drupal 8 support PSR4, code isn't included that way anymore. Eventually, if a PHP file needs to be included, there is the module_handler service.

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('file_system'),
      $container->get('request_stack')
    );
  }

  /**
   * Calls file manager server side and returns a result to client.
   */
  public function flmngr() {

    FlmngrServer::flmngrRequest(
      [
        'dirFiles' => realpath(".") . '/sites/default/files/flmngr',
        'dirTmp' => realpath(".") . '/sites/default/files/flmngr-tmp',
        'dirCache' => realpath(".") . '/sites/default/files/flmngr-cache',
      ],
      $this->requestStack
    );

    return new Response();
  }

The file_system service isn't used.

  public function setApiKey() {
    if (!isset($_POST["apiKey"]) || !isset($_POST["token"])) {
      throw new AccessDeniedHttpException();
    }

    $config = $this->configFactory->getEditable('n1ed.settings');
    $config->set('apikey', $_POST["apiKey"]);
    $config->set('token', $_POST["token"]);
    $config->save(TRUE);

    return new Response();
  }

Since the controller is using the request_stack service, it should use that service to get the value of $_POST["token"] and $_POST["apiKey"]. Modules should never use $_POST.

/**
 * Defines plugin.
 *
 * @CKEditorPlugin(
 *   id = "N1EDEco",
 *   label = @Translation("N1ED"),
 *   module = "n1ed"
 * )
 */
abstract class N1EDEcosystemCKEditorPlugin extends CKEditorPluginBase implements CKEditorPluginConfigurableInterface, CKEditorPluginContextualInterface {

That class cannot be a plugin, since it uses the wrong namespace. Also, an abstract class cannot be a plugin.

  /**
   * Returns plugin name.
   *
   * @return string
   *   Plugin name is a name of add-on in CKEditor terms
   */
  abstract public function getPluginName();

  /**
   * Returns module name.
   *
   * @return string
   *   Module name is a name of module in Drupal terms
   */
  abstract public function getModuleName();

  /**
   * Returns buttons list.
   *
   * @return array
   *   Associative array like in @CKEditorPlugin->getButtons
   */
  abstract public function getButtonsDef();

  /**
   * Adds controls to form using this class methods.
   */
  abstract public function addControlsToForm(&$form, $editor, $config);

Is there a reason those functions aren't defined in a interface?

1. n1ed.routing.yml: you are using "administer site configuration" here, but shouldn't you use your own permission that you define in the permission file?
2. /admin/config/n1ed/setApiKey is vulnerable to CSRF exploits. You need to confirm user intent either by a confirmation form or by using _csrf_token on the route. See https://www.drupal.org/docs/8/api/routing-system/access-checking-on-rout...
3. Same for FlmngrController. It performs data changing actions via the FlmngrServer class and never verifies user intent to protect against CSRF.

This is currently a security blocker.

Thanks!

1. routing file: all route names must be prefixed with your module name to avoid name clashes with other modules.
2. If you use _csrf_request_header_token then you also need the route requirement to only allow POST requests. Looking a the logic of CsrfRequestHeaderAccessCheck I see that it does not apply to GET requests, so the vulnerability still works?

Thanks!

1. class FlmngrServer: doc block just repeats the class name. What is this class used for and why? Please update all your class comments to descirbe what they are doing.
2. class N1ED: "Configure add-on" is user facing text and must run through $this->t() for translation.
3. The error handling of invalid requests seems broken. If I send a POST request with Postman to https://drupal-8.ddev.site/flmngr?f=/etc/passwd.jpg then I get {"error":"Array","data":null}. Please check your error handling and make sure you return meaningful messages.
4. In FMDiskFileSystem you hard-code "/" as directory separator. That looks like a security risk for Windows operating systems where "\" is used. Best use the PHP constant DIRECTORY_SEPARATOR instead.
5. FMDiskFileSystem::getRelativePath(): why do you check the root dir name for slashes here and throw an exception if it does not contain slashes? Please add a code comment. I tried to exploit https://drupal-8.ddev.site/flmngr?f=/etc/passwd.jpg here but could not get any further (which is good, just wondering how this is supposed to work even in non-malicious cases).

I played around a little bit with the file manager service, but could not get an exploit to work with uploading PHP files or trying to read arbitrary files on the server. So that looks safe from my testing.

Now I'm just worried about if an attacker could read arbitrary files somehow, please clarify my questions above.

OK thanks, looks good to me.

Thanks for your contribution, Dmitriy!

I updated your account so you can opt into security advisory coverage now.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on Slack or IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.