Skip to content

fix(security): implement strict authorization for store settings update #3045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Shamim-97 wants to merge 6 commits into from
Closed

fix(security): implement strict authorization for store settings update #3045

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
42e0fd9
fix(security): implement strict authorization for store settings update
Shamim-97 Dec 24, 2025
54fc225
use dokan_get_current_user_id
Shamim-97 Dec 24, 2025
20e7c3f
remove error log
Shamim-97 Dec 24, 2025
ca2114e
Update Permission checked properly
Shamim-97 Dec 25, 2025
42f7568
Update Permission checked properly
Shamim-97 Dec 25, 2025
0c154af
Update Permission checked properly
Shamim-97 Dec 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 3 additions & 22 deletions includes/REST/StoreSettingController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
*
* @author weDevs <info@wedevs.com>
*/
class StoreSettingController extends WP_REST_Controller {
class StoreSettingController extends DokanBaseVendorController {
/**
* Endpoint namespace
*
Expand Down Expand Up @@ -42,7 +42,7 @@
[
'methods' => WP_REST_Server::READABLE,
'callback' => [ $this, 'get_settings' ],
'permission_callback' => [ $this, 'get_settings_permission_callback' ],
'permission_callback' => [ $this, 'check_permission' ],

@coderabbitai coderabbitai Bot Dec 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# First, find the StoreSettingController file
echo "=== Finding StoreSettingController.php ==="
fd -t f "StoreSettingController" 2>/dev/null

echo ""
echo "=== Searching for VendorAuthorizable trait ==="
fd -t f "VendorAuthorizable" 2>/dev/null

echo ""
echo "=== General search for check_permission in PHP files ==="
rg -l "check_permission" --type php 2>/dev/null | head -20

Repository: getdokan/dokan

Length of output: 1078

🏁 Script executed:

#!/bin/bash

echo "=== StoreSettingController.php context (lines 40-65) ==="
sed -n '40,65p' includes/REST/StoreSettingController.php

echo ""
echo "=== VendorAuthorizable.php - check_permission method ==="
rg -A 20 "function check_permission" includes/Traits/VendorAuthorizable.php

echo ""
echo "=== StoreSettingController.php - class definition and parent ==="
head -50 includes/REST/StoreSettingController.php

Repository: getdokan/dokan

Length of output: 2955

🏁 Script executed:

#!/bin/bash

echo "=== DokanBaseVendorController class definition ==="
fd -t f "DokanBaseVendorController" --exec head -60 {} \;

echo ""
echo "=== Search for VendorAuthorizable usage in DokanBaseVendorController ==="
rg -B 5 -A 15 "class DokanBaseVendorController" --type php

echo ""
echo "=== Check for any vendor-specific permission checks ==="
rg "dokandar" --type php includes/REST/StoreSettingController.php

Repository: getdokan/dokan

Length of output: 1519

🏁 Script executed:

#!/bin/bash

echo "=== get_settings method implementation ==="
rg -A 30 "function get_settings" includes/REST/StoreSettingController.php

echo ""
echo "=== update_settings method implementation ==="
rg -A 30 "function update_settings" includes/REST/StoreSettingController.php

Repository: getdokan/dokan

Length of output: 2794

🏁 Script executed:

#!/bin/bash

echo "=== Complete get_vendor method ==="
rg -A 50 "protected function get_vendor" includes/REST/StoreSettingController.php

echo ""
echo "=== Check for any authorization inside get_vendor ==="
rg -B 5 -A 50 "protected function get_vendor" includes/REST/StoreSettingController.php | grep -A 50 "function get_vendor"

Repository: getdokan/dokan

Length of output: 3583

Verify vendor-ownership authorization for cross-vendor access scenarios.

The check_permission method from VendorAuthorizable only checks current_user_can( 'dokandar' ) without differentiating between read and write operations. More critically, both get_settings() and update_settings() callbacks use the same permission check but lack explicit vendor-ownership validation when a vendor_id parameter is provided. This allows any user with the 'dokandar' capability to potentially read and update other vendors' settings. Ensure the callbacks validate that users can only access/modify their own vendor's settings, or implement operation-specific permission checks.

🤖 Prompt for AI Agents 
In includes/REST/StoreSettingController.php around line 45, the
permission_callback currently calls VendorAuthorizable::check_permission which
only checks current_user_can('dokandar') and is reused for both get_settings and
update_settings; you must enforce vendor-ownership and operation-specific
checks: update the permission logic so that when a vendor_id request parameter
is provided the code verifies the current user either owns that vendor (compare
current user's vendor id to the vendor_id) or has a higher capability (e.g.,
manage_woocommerce or an admin role), and differentiate read vs write by
requiring a stronger capability for update_settings (or a separate permission
callback that checks ownership + write capability); implement the ownership
check either inside specialized permission callbacks passed to
register_rest_route for the GET and POST/PUT routes or as the first step in
get_settings/update_settings handlers and return WP_Error( 'rest_forbidden', ...
) when ownership/permission fails.

'args' => [
'vendor_id' => [
'required' => false,
Expand All @@ -57,7 +57,7 @@
[
'methods' => WP_REST_Server::EDITABLE,
'callback' => [ $this, 'update_settings' ],
'permission_callback' => [ $this, 'get_settings_permission_callback' ],
'permission_callback' => [ $this, 'check_permission' ],
'args' => [
'vendor_id' => [
'required' => false,
Expand Down Expand Up @@ -123,25 +123,6 @@
return rest_ensure_response( $response );
}

/**
* Permission callback for vendor settings
*
* @return bool|WP_Error
*/
public function get_settings_permission_callback() {
$vendor = $this->get_vendor();

if ( is_wp_error( $vendor ) ) {
return $vendor;
}

if ( empty( $vendor->get_id() ) ) {
return new WP_Error( 'no_store_found', __( 'No vendor found', 'dokan-lite' ), [ 'status' => 404 ] );
}

return true;
}

/**
* Get vendor
*
Expand Down Expand Up @@ -176,7 +157,7 @@
*
* @return array Links for the given post.
*/
protected function prepare_links( $object, $request ) {

Check warning on line 160 in includes/REST/StoreSettingController.php

View workflow job for this annotation

GitHub Actions / Run PHPCS inspection 

It is recommended not to use reserved keyword "object" as function parameter name. Found: $object
$links = [
'self' => [
'href' => rest_url( sprintf( '/%s/%s/%d', $this->namespace, $this->rest_base, $object['id'] ) ),
Expand Down
7 changes: 7 additions & 0 deletions includes/Vendor/Manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
'order' => 'ASC',
'status' => [ 'approved' ],
'featured' => '', // yes or no
'meta_query' => [],

Check warning on line 55 in includes/Vendor/Manager.php

View workflow job for this annotation

GitHub Actions / Run PHPCS inspection 

Detected usage of meta_query, possible slow query.
'fields' => 'all',
];

Expand All @@ -69,7 +69,7 @@

$meta_query[] = [
'key' => 'dokan_enable_selling',
'value' => ( $stat == 'approved' ) ? 'yes' : 'no',

Check failure on line 72 in includes/Vendor/Manager.php

View workflow job for this annotation

GitHub Actions / Run PHPCS inspection 

Loose comparisons are not allowed. Expected: "==="; Found: "=="
'compare' => '=',
];
}
Expand All @@ -78,11 +78,11 @@
$args['meta_query']['relation'] = 'AND';
$args['meta_query'][] = $meta_query;
} else {
$args['meta_query'] = $meta_query;

Check warning on line 81 in includes/Vendor/Manager.php

View workflow job for this annotation

GitHub Actions / Run PHPCS inspection 

Detected usage of meta_query, possible slow query.
}

// if featured
if ( 'yes' == $args['featured'] ) {

Check failure on line 85 in includes/Vendor/Manager.php

View workflow job for this annotation

GitHub Actions / Run PHPCS inspection 

Loose comparisons are not allowed. Expected: "==="; Found: "=="
$args['meta_query']['relation'] = 'AND';
$args['meta_query'][] = [
'key' => 'dokan_feature_seller',
Expand Down Expand Up @@ -168,7 +168,7 @@
/**
* @since 3.2.7 added $data parameter
*/
$store_data = apply_filters( 'dokan_vendor_create_data', [

Check failure on line 171 in includes/Vendor/Manager.php

View workflow job for this annotation

GitHub Actions / Run PHPCS inspection 

Opening parenthesis of a multi-line function call must be the last content on the line
'store_name' => ! empty( $data['store_name'] ) ? $data['store_name'] : '',
'social' => ! empty( $data['social'] ) ? $data['social'] : [],
'payment' => ! empty( $data['payment'] ) ? $data['payment'] : [
Expand All @@ -187,7 +187,7 @@
'show_min_order_discount' => ! empty( $data['show_min_order_discount'] ) ? $data['show_min_order_discount'] : 'no',
'store_seo' => ! empty( $data['store_seo'] ) ? $data['store_seo'] : [],
'dokan_store_time' => ! empty( $data['store_open_close'] ) ? $data['store_open_close'] : [],
], $data );

Check failure on line 190 in includes/Vendor/Manager.php

View workflow job for this annotation

GitHub Actions / Run PHPCS inspection 

Closing parenthesis of a multi-line function call must be on a line by itself

$vendor = dokan()->vendor->get( $vendor_id );

Expand Down Expand Up @@ -260,6 +260,13 @@
if ( ! $data ) {
return $vendor;
}
//Allow if current user is the vendor OR is an administrator/shop manager.
$is_owner = dokan_get_current_user_id() === (int) $vendor->get_id();
$is_admin = current_user_can( 'manage_woocommerce' ) || current_user_can( 'manage_options' );

if ( ! $is_owner && ! $is_admin ) {
return new \WP_Error( 'dokan_permission_denied', __( 'You do not have permission to update this vendor.', 'dokan-lite' ) );
}

@coderabbitai coderabbitai Bot Dec 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Add explicit check to block modifications to administrator accounts.

According to issue #5286, the vulnerability allowed vendor staff to modify administrator accounts via the REST API, enabling account takeover through password reset. The recommended fix was to "block modifications to administrator accounts (return 403) before handling the email parameter."

The current implementation prevents unauthorized users from updating arbitrary vendors, but doesn't explicitly check whether the target vendor ($vendor_id) is an administrator. If an administrator user also has the seller role, or if there are other edge cases where an admin user ID could be targeted, the update would proceed.

🔎 Proposed fix 
 //Allow if current user is the vendor OR is an administrator/shop manager.
+$target_user = get_userdata( $vendor_id );
+if ( $target_user && in_array( 'administrator', $target_user->roles, true ) ) {
+    return new \WP_Error( 'dokan_permission_denied', __( 'Administrator accounts cannot be modified through this endpoint.', 'dokan-lite' ), [ 'status' => 403 ] );
+}
+
 $is_owner = dokan_get_current_user_id() === (int) $vendor->get_id();
 $is_admin = current_user_can( 'manage_woocommerce' ) || current_user_can( 'manage_options' );

 if ( ! $is_owner && ! $is_admin ) {
-    return new \WP_Error( 'dokan_permission_denied', __( 'You do not have permission to update this vendor.', 'dokan-lite' ) );
+    return new \WP_Error( 'dokan_permission_denied', __( 'You do not have permission to update this vendor.', 'dokan-lite' ), [ 'status' => 403 ] );
 }
🤖 Prompt for AI Agents 
In includes/Vendor/Manager.php around lines 263 to 269, add an explicit check
that the target vendor user is not an administrator and return a 403 WP_Error if
they are; specifically, obtain the target vendor user ID (the $vendor->get_id()
value), call user_can($vendor_id, 'manage_options') or user_can($vendor_id,
'administrator') to detect admin capability/role, and if true return new
\WP_Error('dokan_permission_denied', __('You do not have permission to update
this vendor.', 'dokan-lite'), array('status' => 403)); perform this check early
(before any handling of the email parameter or other updates) so admin accounts
cannot be modified via the REST API.


// default wp based user data
if ( ! empty( $data['user_pass'] ) && get_current_user_id() === $vendor->get_id() ) {

@coderabbitai coderabbitai Bot Dec 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check for other uses of get_current_user_id vs dokan_get_current_user_id in Manager.php
# to understand the intended pattern.

# Search for get_current_user_id() usage
echo "=== Uses of get_current_user_id() ==="
rg -n 'get_current_user_id\(\)' includes/Vendor/Manager.php

echo ""
echo "=== Uses of dokan_get_current_user_id() ==="
rg -n 'dokan_get_current_user_id\(\)' includes/Vendor/Manager.php

# Check if password updates are intended to be restricted differently
echo ""
echo "=== Password update logic context ==="
rg -n -B2 -A2 'user_pass' includes/Vendor/Manager.php

Repository: getdokan/dokan

Length of output: 1035

🏁 Script executed:

#!/bin/bash
# Find the definition of dokan_get_current_user_id
echo "=== Definition of dokan_get_current_user_id() ==="
rg -n 'function\s+dokan_get_current_user_id' --type php

echo ""
echo "=== Broader search for dokan_get_current_user_id definition ==="
rg -B2 -A5 'dokan_get_current_user_id\s*=' --type php | head -30

echo ""
echo "=== Context around the permission check and password update ==="
sed -n '260,280p' includes/Vendor/Manager.php

Repository: getdokan/dokan

Length of output: 1214

🏁 Script executed:

#!/bin/bash
# Read the dokan_get_current_user_id function definition
sed -n '48,70p' includes/functions.php

echo ""
echo "=== Also check WordPress's get_current_user_id usage in this file ==="
rg -n 'get_current_user_id\(\)' includes/functions.php | head -10

Repository: getdokan/dokan

Length of output: 1330

Use dokan_get_current_user_id() on line 273 to align with the permission check.

The permission check at line 265 uses dokan_get_current_user_id() which returns the associated vendor ID for vendor staff users. However, line 273 uses get_current_user_id() which returns the actual staff user's ID. This inconsistency prevents vendor staff from updating passwords despite passing the permission check at line 265. Change line 273 to use dokan_get_current_user_id() to match the permission logic and allow vendor staff to update vendor passwords as intended.

🤖 Prompt for AI Agents 
In includes/Vendor/Manager.php around lines 265 to 273, the permission check
uses dokan_get_current_user_id() but line 273 compares get_current_user_id() to
$vendor->get_id(), causing vendor staff to be denied password updates; replace
get_current_user_id() with dokan_get_current_user_id() (and ensure the same
integer casting as used above) so the user ID comparison matches the permission
logic and vendor staff can update vendor passwords.

Expand Down Expand Up @@ -383,7 +390,7 @@

// for backward compatibility we'll allow both `enable_tnc` and `toc_enabled` to set store trams and condition settings
if ( ( isset( $data['enable_tnc'] ) && dokan_validate_boolean( $data['enable_tnc'] ) )
|| ( isset( $data['toc_enabled'] ) && dokan_validate_boolean( $data['toc_enabled'] ) ) ) {

Check warning on line 393 in includes/Vendor/Manager.php

View workflow job for this annotation

GitHub Actions / Run PHPCS inspection 

Found precision alignment of 1 spaces.
$vendor->set_enable_tnc( 'on' );
} else {
$vendor->set_enable_tnc( 'off' );
Expand Down
Loading