From 2002fd6b4b2e9241572501307c81d5f2ef2e1eea Mon Sep 17 00:00:00 2001
From: Ben Thomson <bennothommo@users.noreply.github.com>
Date: Wed, 5 Dec 2018 01:22:07 +0800
Subject: [PATCH] Prevent plugins that cannot be instantiated from being loaded
 (#3956)

Credit to @bennothommo
---
 modules/system/classes/PluginManager.php      | 26 ++++++++++++++-----
 .../plugins/testvendor/goto/Plugin.php        | 16 ++++++++++++
 .../unit/system/classes/PluginManagerTest.php | 13 +++++++++-
 3 files changed, 47 insertions(+), 8 deletions(-)
 create mode 100644 tests/fixtures/plugins/testvendor/goto/Plugin.php

diff --git a/modules/system/classes/PluginManager.php b/modules/system/classes/PluginManager.php
index 7a4a9bcfd..35300f6bf 100644
--- a/modules/system/classes/PluginManager.php
+++ b/modules/system/classes/PluginManager.php
@@ -5,6 +5,7 @@ use App;
 use Str;
 use File;
 use Lang;
+use Log;
 use View;
 use Config;
 use Schema;
@@ -121,17 +122,28 @@ class PluginManager
         $className = $namespace.'\Plugin';
         $classPath = $path.'/Plugin.php';
 
-        // Autoloader failed?
-        if (!class_exists($className)) {
-            include_once $classPath;
-        }
+        try {
+            // Autoloader failed?
+            if (!class_exists($className)) {
+                include_once $classPath;
+            }
 
-        // Not a valid plugin!
-        if (!class_exists($className)) {
+            // Not a valid plugin!
+            if (!class_exists($className)) {
+                return;
+            }
+
+            $classObj = new $className($this->app);
+        } catch (\Throwable $e) {
+            Log::error('Plugin ' . $className . ' could not be instantiated.', [
+                'message' => $e->getMessage(),
+                'file' => $e->getFile(),
+                'line' => $e->getLine(),
+                'trace' => $e->getTraceAsString()
+            ]);
             return;
         }
 
-        $classObj = new $className($this->app);
         $classId = $this->getIdentifier($classObj);
 
         /*
diff --git a/tests/fixtures/plugins/testvendor/goto/Plugin.php b/tests/fixtures/plugins/testvendor/goto/Plugin.php
new file mode 100644
index 000000000..829c56435
--- /dev/null
+++ b/tests/fixtures/plugins/testvendor/goto/Plugin.php
@@ -0,0 +1,16 @@
+<?php namespace TestVendor\Goto;
+
+use System\Classes\PluginBase;
+
+class Plugin extends PluginBase
+{
+
+    public function pluginDetails()
+    {
+        return [
+            'name' => 'Invalid Test Plugin',
+            'description' => 'Test plugin used by unit tests to detect plugins with invalid namespaces.',
+            'author' => 'Test Vendor'
+        ];
+    }
+}
diff --git a/tests/unit/system/classes/PluginManagerTest.php b/tests/unit/system/classes/PluginManagerTest.php
index b34817c3f..7b472d583 100644
--- a/tests/unit/system/classes/PluginManagerTest.php
+++ b/tests/unit/system/classes/PluginManagerTest.php
@@ -58,6 +58,7 @@ class PluginManagerTest extends TestCase
         $this->assertArrayHasKey('October.Tester', $result);
         $this->assertArrayHasKey('Database.Tester', $result);
         $this->assertArrayHasKey('TestVendor.Test', $result);
+        $this->assertArrayNotHasKey('TestVendor.Goto', $result);
 
         $this->assertInstanceOf('October\NoUpdates\Plugin', $result['October.NoUpdates']);
         $this->assertInstanceOf('October\Sample\Plugin', $result['October.Sample']);
@@ -66,6 +67,14 @@ class PluginManagerTest extends TestCase
         $this->assertInstanceOf('TestVendor\Test\Plugin', $result['TestVendor.Test']);
     }
 
+    public function testUnloadablePlugin()
+    {
+        $manager = PluginManager::instance();
+        $pluginNamespaces = $manager->getPluginNamespaces();
+        $result = $manager->loadPlugin('\\testvendor\\goto', $pluginNamespaces['\\testvendor\\goto']);
+        $this->assertNull($result);
+    }
+
     public function testGetPluginPath()
     {
         $manager = PluginManager::instance();
@@ -85,6 +94,7 @@ class PluginManagerTest extends TestCase
         $this->assertArrayHasKey('October.Tester', $result);
         $this->assertArrayHasKey('Database.Tester', $result);
         $this->assertArrayHasKey('TestVendor.Test', $result);
+        $this->assertArrayNotHasKey('TestVendor.Goto', $result);
 
         $this->assertInstanceOf('October\NoUpdates\Plugin', $result['October.NoUpdates']);
         $this->assertInstanceOf('October\Sample\Plugin', $result['October.Sample']);
@@ -115,12 +125,13 @@ class PluginManagerTest extends TestCase
         $manager = PluginManager::instance();
         $result = $manager->getPluginNamespaces();
 
-        $this->assertCount(5, $result);
+        $this->assertCount(6, $result);
         $this->assertArrayHasKey('\october\noupdates', $result);
         $this->assertArrayHasKey('\october\sample', $result);
         $this->assertArrayHasKey('\october\tester', $result);
         $this->assertArrayHasKey('\database\tester', $result);
         $this->assertArrayHasKey('\testvendor\test', $result);
+        $this->assertArrayHasKey('\testvendor\goto', $result);
     }
 
     public function testGetVendorAndPluginNames()