Skip to content

Conversation

danielmorell
Copy link
Collaborator

Description of the change

This is a major update the WordPress plugin. It includes an overhaul of most everything. Here is a high level list of everything that changed.

  • Fixed CSRF vulnerability.
  • Removed support for PHP 8.0 and below.
  • Updated and improved the settings page.
  • Updated the Rollbar core PHP SDK to v4.1.
  • Added support for telemetry and added auto-instrumentation.
  • Added support for ROLLBAR_DISABLE_ADMIN to remove the plugin settings page from the admin.
  • Added support for ROLLBAR_SETTINGS to configure the plugin without the admin page.
  • 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_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.
  • Added 'framework' details with the WordPress version to the item payload.
  • Added unit tests.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Maintenance
  • New release

Related issues

Checklists

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests
  • All tests related to the changed code pass in development

Code review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
  • "Ready for review" label attached to the PR and reviewers assigned
  • Issue from task tracker has a link to this pull request
  • Changes have been reviewed by at least one other engineer

@danielmorell danielmorell added this to the v3.0.0 milestone Aug 16, 2025
@danielmorell danielmorell requested a review from brianr August 16, 2025 17:26
Copy link
Member

@brianr brianr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First round of review - added a few comments/questions.

src/Settings.php Outdated
esc_attr(trim($options['server_side_access_token'])) :
'',
'client_side_access_token' => (!empty($options['client_side_access_token'])) ?
trim($options['client_side_access_token']) :
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing esc_attr here?


// 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) {
if ($setting_value == Plugin::getInstance()->settingsInstance()->getDefaultOption($setting_name)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be ===?

name="<?= esc_attr($input->getName()) ?>"
id="<?= esc_attr($input->getId()) ?>"
data-setting="<?= esc_attr($input->getId()) ?>"
value="<?= $input->getValue() ?>"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing esc_attr?

name="<?= esc_attr($input->getName()) ?>"
id="<?= esc_attr($input->getId()) ?>"
data-setting="<?= esc_attr($input->getId()) ?>"
value="<?= $input->getValue() ?>"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing esc_attr?

?>
<hr>
<div class="rollbar-settings-section-header">
<h2 id="<?= $id ?>" class="section-heading">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Escaping needed in this file?

echo 'foo';
foreach ($messages as $message) : ?>
<div class="notice notice-<?= $message['type'] ?> is-dismissible">
<p><?= $message['message'] ?></p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Escaping needed in this file?

} catch (Exception $exception) {
FlashMessages::addMessage(
message: 'Rollbar is misconfigured. Please, fix your configuration here: <a href="'
. admin_url('/options-general.php?page=rollbar_wp') . '">',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This <a> tag looks incomplete?

`<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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Escape the data coming from the api?

* }[] $messages
*/

echo 'foo';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug print

@toineenzo
Copy link

toineenzo commented Aug 20, 2025

Just a heads up: perhaps you can take a look at the implementation / fix for the security issue in #129. It also has a nice markdown file with security advises

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