Addressed items in code review.

Author: danielmorell

README.md

@@ -111,24 +111,15 @@ The plugin provides a number of filters that allow you to customize the behavior
 
 ### `apply_filters('rollbar_api_admin_permission', bool $value, string $route, WP_REST_Request $request)`
 
-Filter to allow or deny access to a Rollbar route in the WordPress REST API used in the WordPress Admin.
+Filter to allow or deny access to a Rollbar route in the WordPress REST API used in the WordPress Admin. Generally,
+this should be the same as the `rollbar_user_can_view_admin` filter.
 
 **Parameters**
 
 * `bool $value` - The initial value. Defaults is `true` for admin users, `false` for non-admin users.
 * `string $route` - The route being accessed.
 * `WP_REST_Request $request` - The REST request object.
 
-### `apply_filters('rollbar_disable_admin', bool $disable)`
-
-Filter to disable the admin settings page of the plugin.
-
-This filter cannot override the `ROLLBAR_DISABLE_ADMIN` constant.
-
-**Parameters**
-
-* `bool $disable` - `true` to disable the admin settings page, `false` to enable it.
-
 ### `apply_filters('rollbar_js_config', array $config)`
 
 Filters the Rollbar JavaScript configuration.
@@ -174,6 +165,16 @@ select the action on the settings page, or add it to the list of actions using t
 * `array<string, callable(string, mixed...):string> $handlers` - An associative array where the keys are action names 
   and the values are the custom event handler.
 
+### `apply_filters('rollbar_user_can_view_admin', bool $disable)`
+
+Filter to enable / disable the admin settings page of the plugin for the current user.
+
+This filter cannot override the `ROLLBAR_DISABLE_ADMIN` constant.
+
+**Parameters**
+
+* `bool $allow` - `true` to enable the admin settings page, `false` to disable it.
+
 ### Telemetry
 
 Starting in version 3.0.0 of the Rollbar plugin, Telemetry is enabled by default. Telemetry is a feature that allows 

public/admin/rollbar.js

@@ -70,16 +70,41 @@ document.addEventListener('DOMContentLoaded', () => {
 
     const testMessageContainer = document.getElementById('rollbar_test_message_container');
 
+    /**
+     * Escapes HTML characters in the string.
+     *
+     * @param {string} str The string to escape.
+     * @returns {string}
+     */
+    const escapeHtml = (str) => {
+        return new Option(str).innerHTML;
+    };
+
+    /**
+     * Escapes HTML characters in the string for use in an attribute.
+     *
+     * @param {string} str The string to escape.
+     * @returns {string}
+     */
+    const escapeHtmlAttr = (str) => {
+        return ('' + str)
+            .replace(/&/g, '&')
+            .replace(/"/g, '"')
+            .replace(/'/g, ''')
+            .replace(/</g, '&lt;')
+            .replace(/>/g, '&gt;');
+    };
+
     /**
      * Returns a formatted notice message HTML Node.
      *
      * @param {string} type    The notice type class.
-     * @param {string} message The notice message.
+     * @param {string} message The notice message. The message should be escaped before being passed in.
      * @returns {ChildNode}
      */
     const makeNotice = (type, message) => {
         const el = document.createElement('div');
-        el.innerHTML = `<div class="notice ${type} is-dismissible">${message}</div>`;
+        el.innerHTML = `<div class="notice ${escapeHtmlAttr(type)} is-dismissible">${message}</div>`;
         return el.firstChild;
     };
 
@@ -114,8 +139,8 @@ document.addEventListener('DOMContentLoaded', () => {
                     'notice-error',
                     `<p><strong>PHP Test:</strong> There was a problem accessing Rollbar service.</p>
                               <ul>
-                                  <li>Code:<code>${data.code}</code></li>
-                                  <li>Message:<code>${data.message}</code></li>
+                                  <li>Code:<code>${escapeHtml(data.code)}</code></li>
+                                  <li>Message:<code>${escapeHtml(data.message)}</code></li>
                               </ul>`,
                 ));
             })
@@ -174,26 +199,46 @@ document.addEventListener('DOMContentLoaded', () => {
         }
 
         // Depending on the plugin status prior to loading the page, the Rollbar object may not exist. So we fetch it.
-        if (window.Rollbar === undefined) {
-
-            fetch(rollbarSettings.plugin_url + "public/js/rollbar.snippet.js")
-                .then(response => {
-                    if (!response.ok) {
-                        throw new Error('Network response was not ok');
-                    }
-                    return response.text();
-                })
-                .then(data => {
-                    eval(data);
-                    sendTestJsMessage(clientSideAccessToken, environment);
-                })
-                .catch(() => {
-                    jsFailNotice();
-                });
-        } else {
-            // This is inside an "else" since the fetch is async.
+        if (window.Rollbar !== undefined) {
             sendTestJsMessage(clientSideAccessToken, environment);
+            return;
+        }
+        // If the Rollbar object doesn't exist, we need to load and configure it.
+        window._rollbarConfig = {
+            accessToken: clientSideAccessToken,
+            captureUncaught: true,
+            captureUnhandledRejections: true,
+            payload: {
+                environment: environment
+            }
         }
+        // Load the Rollbar JS library.
+        const script = document.createElement('script');
+        script.src = rollbarSettings.plugin_url + "public/js/rollbar.snippet.js";
+        let mainRollbarScript = document.querySelector('script[src$="rollbar.min.js"]');
+
+        // Wait for both Rollbar JS libraries to load before sending the test message.
+        script.addEventListener('load', () => {
+            if (window.Rollbar !== undefined) {
+                sendTestJsMessage(clientSideAccessToken, environment);
+                return;
+            }
+            document.querySelector('script[src$="rollbar.min.js"]')?.addEventListener('load', () => {
+                sendTestJsMessage(clientSideAccessToken, environment);
+            });
+        });
+
+        // Handle error loading either the snippet or main Rollbar JS library.
+        window.addEventListener('error', (e) => {
+            if (e.target === script || e.target === mainRollbarScript) {
+                testMessageContainer.appendChild(makeNotice(
+                    'notice-error',
+                    `<p><strong>JS Test:</strong> There was an error loading the Rollbar JS library.</p>`,
+                ));
+            }
+        }, true);
+
+        document.body.appendChild(script);
     };
 
     // Send Test Message

readme.txt

@@ -116,7 +116,7 @@ Yes. It's actually the recommended method of installation.
 * Added support for `ROLLBAR_CLIENT_ACCESS_TOKEN` constant or environment variable to set the client access token.
 * Added support for `WP_PROXY_BYPASS_HOSTS`, `WP_PROXY_USERNAME`, and `WP_PROXY_PASSWORD` for better proxy management.
 * Added `rollbar_api_admin_permission` filter to allow custom authorization of the admin API.
-* Added `rollbar_disable_admin` filter to allow custom disabling of the admin page.
+* Added `rollbar_user_can_view_admin` filter to allow custom disabling of the admin page.
 * Added `rollbar_php_config` filter to allow more exact control over Rollbar PHP configurations.
 * Added `rollbar_telemetry_actions` filter to allow control of which actions are logged via telemetry.
 * Added `rollbar_telemetry_custom_handlers` filter to allow custom control over what is logged in telemetry messages.

src/API/AdminAPI.php

@@ -83,31 +83,32 @@ public function handleTestPhpLogging(WP_REST_Request $request): WP_REST_Response
         $plugin = Plugin::getInstance();
 
         try {
-            $plugin->initPhpLogging();
-            $response = Rollbar::report(
+            $plugin->initPhpLogging(ignoreEnabledSetting: true);
+            $rollbarResponse = Rollbar::report(
                 Level::INFO,
                 'Test message from Rollbar WordPress plugin using PHP: ' .
                 'integration with WordPress successful',
             );
         } catch (Throwable $exception) {
             return new WP_REST_Response(
                 [
-                    'message' => $exception->getMessage(),
+                    'code' => 500,
                     'success' => false,
+                    // Send the exception message to the client to the admin user so they can debug the issue.
+                    'message' => $exception->getMessage(),
                 ],
                 500,
             );
         }
 
-        $info = $response->getInfo();
-
         $response = [
-            'code' => $response->getStatus(),
-            'success' => true,
+            'code' => $rollbarResponse->getStatus(),
+            'success' => $rollbarResponse->getStatus() === 200,
+            'message' => '',
         ];
-        if (is_array($info)) {
-            $response = array_merge($response, $info);
-        } else {
+
+        $info = $rollbarResponse->getInfo();
+        if (is_string($info)) {
             $response['message'] = $info;
         }
 

src/Admin/SettingsPage.php

@@ -39,7 +39,7 @@ final class SettingsPage extends AbstractSingleton
      */
     protected function __construct()
     {
-        if (Plugin::hideAdmin()) {
+        if (!Plugin::userCanViewAdmin()) {
             return;
         }
         $this->hooks();
@@ -283,12 +283,16 @@ public function optionsPage(): void
      */
     public static function restoreDefaultsAction(): void
     {
+        if (!check_admin_referer('rollbar_wp_restore_defaults')) {
+            return;
+        }
         Settings::getInstance()->restoreDefaults();
 
         FlashMessages::addMessage(
             message: 'Default Rollbar settings restored.',
         );
 
         wp_redirect(admin_url('/options-general.php?page=rollbar_wp'));
+        exit();
     }
 }

src/Plugin.php

@@ -6,6 +6,7 @@
 use Rollbar\Rollbar;
 use Rollbar\RollbarJsHelper;
 use Rollbar\WordPress\Admin\FlashMessages;
+use Rollbar\WordPress\Admin\SettingsPage;
 use Rollbar\WordPress\API\AdminAPI;
 use Rollbar\WordPress\Lib\AbstractSingleton;
 use Rollbar\WordPress\Settings\SettingType;
@@ -68,14 +69,24 @@ protected function __construct()
      */
     protected function postInit(): void
     {
-        // Set up admin views and API.
-        AdminAPI::getInstance();
         // Set up the telemetry listener.
         if ($this->getSetting('enable_telemetry_listener')) {
             $this->listener = Listener::getInstance();
         }
     }
 
+    /**
+     * Handles the 'init' action hook.
+     *
+     * @return void
+     */
+    public function onInit(): void
+    {
+        // Set up admin views and API.
+        SettingsPage::getInstance();
+        AdminAPI::getInstance();
+    }
+
     /**
      * Updates the plugin configuration
      *
@@ -115,21 +126,21 @@ public static function disabledAdmin(): bool
      * @return bool
      * @since 3.0.0
      */
-    public static function hideAdmin(): bool
+    public static function userCanViewAdmin(): bool
     {
         if (self::disabledAdmin()) {
             return false;
         }
 
         /**
-         * Filter to disable the admin settings page of the plugin.
+         * Filter to enable / disable the admin settings page of the plugin for the current user.
          *
          * This filter cannot override the `ROLLBAR_DISABLE_ADMIN` constant.
          *
-         * @param bool $disable `true` to disable the admin settings page, `false` to enable it.
+         * @param bool $allow `true` to enable the admin settings page, `false` to disable it.
          * @since 3.0.0
          */
-        return apply_filters('rollbar_disable_admin', !current_user_can('manage_options')) === true;
+        return apply_filters('rollbar_user_can_view_admin', current_user_can('manage_options')) === true;
     }
 
     /**
@@ -192,6 +203,7 @@ public function settingsInstance(): Settings
      */
     private function hooks(): void
     {
+        add_action('init', $this->onInit(...));
         add_action('wp_head', $this->initJsLogging(...));
         add_action('admin_head', $this->initJsLogging(...));
     }
@@ -242,12 +254,14 @@ public static function buildIncludedErrno(int $cutoff): int
      * Sets up the Rollbar PHP error handler if PHP logging is enabled.
      * Handles configuration errors by displaying appropriate error messages.
      *
+     * @param bool $ignoreEnabledSetting If true, the plugin will not check the 'php_logging_enabled' setting first.
+     *
      * @return void
      */
-    public function initPhpLogging(): void
+    public function initPhpLogging(bool $ignoreEnabledSetting = false): void
     {
         // Return if logging is not enabled
-        if (0 === $this->getSetting('php_logging_enabled')) {
+        if (!$ignoreEnabledSetting && false === $this->getSetting('php_logging_enabled')) {
             return;
         }
 
@@ -321,15 +335,15 @@ public function buildPHPConfig(): array
     public function initJsLogging(): void
     {
         // Return if logging is not enabled
-        if ($this->getSetting('js_logging_enabled') === 0) {
+        if (false === $this->getSetting('js_logging_enabled')) {
             return;
         }
 
         // Return if access token is not set
-        if ($this->getSetting('client_side_access_token') == '') {
+        if (empty($this->getSetting('client_side_access_token'))) {
             FlashMessages::addMessage(
                 message: 'Rollbar is misconfigured. Please, fix your configuration here: <a href="'
-                . admin_url('/options-general.php?page=rollbar_wp') . '">',
+                . admin_url('/options-general.php?page=rollbar_wp') . '">Rollbar Settings</a>.',
                 type: 'error',
             );
             return;
@@ -352,7 +366,7 @@ public function buildJsConfig(): array
     {
         $config = [
             'accessToken' => $this->getSetting('client_side_access_token'),
-          'captureUncaught' => true,
+            'captureUncaught' => true,
             'payload' => [
                 'environment' => $this->getSetting('environment'),
             ],

src/Settings.php

@@ -438,6 +438,8 @@ public static function preUpdate(array $settings): array
 
         // Don't store default values in the database, so future updates to default values in the SDK get propagated.
         foreach ($settings as $setting_name => $setting_value) {
+            // Loose comparison to allow for boolean values to be set to 0 or 1 and integers to be strings, which is how
+            // they are posted via HTTP.
             if ($setting_value == Plugin::getInstance()->settingsInstance()->getDefaultOption($setting_name)) {
                 unset($settings[$setting_name]);
             }
@@ -579,13 +581,16 @@ private function fetchSettings(): void
                 esc_attr(trim($options['server_side_access_token'])) :
                 '',
             'client_side_access_token' => (!empty($options['client_side_access_token'])) ?
-                trim($options['client_side_access_token']) :
+                esc_attr(trim($options['client_side_access_token'])) :
                 '',
             'included_errno' => (!empty($options['included_errno'])) ?
                 esc_attr(trim($options['included_errno'])) :
                 self::getDefaultOption('included_errno'),
         ];
 
+        // Filter out options that are not in the list of options.
+        $options = array_intersect_key($options, array_flip(self::listOptions()));
+
         foreach (self::listOptions() as $option) {
             // 'access_token' and 'enabled' are different in WordPress plugin
             // look for 'server_side_access_token' and 'php_logging_enabled' above