Skip to content

Conversation

jerrybarry
Copy link

Description of the change

Please include a summary of the change and which issues are fixed.
Please also include relevant motivation and context.

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

Shortcut stories and GitHub issues (delete irrelevant)

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

@toineenzo toineenzo mentioned this pull request Aug 20, 2025
12 tasks
@danielmorell danielmorell self-assigned this Aug 22, 2025
@danielmorell danielmorell self-requested a review August 22, 2025 21:21
@danielmorell danielmorell changed the base branch from master to next/2.x/main August 29, 2025 22:36
Copy link
Collaborator

@danielmorell danielmorell left a comment

Choose a reason for hiding this comment

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

I have a few comments / questions.

I also changed the base from master to our v2 branch.

}

// Additional security check - ensure we're in admin context
if (!is_admin()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is a REST endpoint in the admin context?

'permission_callback' => '__return_true',
'permission_callback' => function() {
// Check if user is logged in and has manage_options capability
return is_user_logged_in() && current_user_can('manage_options');
Copy link
Collaborator

Choose a reason for hiding this comment

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

💯

@@ -350,6 +380,11 @@ public static function restoreDefaultsAction()

public static function flashRedirect($type, $message)
{
// Security check - ensure user has proper capabilities
if (!current_user_can('manage_options')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason we are adding this here when it is only called from restoreDefaultsAction() which has the same check?

}

// Verify nonce if provided (for admin menu link)
if (isset($_GET['_wpnonce']) && !wp_verify_nonce($_GET['_wpnonce'], 'rollbar_wp_admin_link')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

With this check being optional, what is the benefit of having it?

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

Successfully merging this pull request may close these issues.

Test the plugin with latest WordPress
2 participants