Skip to content

Fixed vulnerable to Cross Site Request Forgery (CSRF) - #129

Closed
jerrybarry wants to merge 1 commit into
rollbar:next/2.x/mainfrom
jerrybarry:master
Closed

Fixed vulnerable to Cross Site Request Forgery (CSRF)#129
jerrybarry wants to merge 1 commit into
rollbar:next/2.x/mainfrom
jerrybarry:master

Conversation

@jerrybarry

Copy link
Copy Markdown

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

@danielmorell danielmorell left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment thread src/Plugin.php
}

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

Copy link
Copy Markdown
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?

Comment thread src/Plugin.php
'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
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

💯

Comment thread src/Settings.php
public static function flashRedirect($type, $message)
{
// Security check - ensure user has proper capabilities
if (!current_user_can('manage_options')) {

Copy link
Copy Markdown
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?

Comment thread src/Settings.php
}

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

Copy link
Copy Markdown
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?

@toineenzo

Copy link
Copy Markdown

@jerrybarry fyi :)

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

3 participants