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 1 commit
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
8 changes: 8 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 @@ -261,6 +261,14 @@
return $vendor;
}

// Allow if current user is the vendor OR is an administrator/shop manager.
$is_owner = 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 HTTP 403 status code to the permission denied error.

The permission check logic correctly addresses the IDOR vulnerability. However, the WP_Error lacks a status code, causing WordPress REST API to return HTTP 500 instead of 403 Forbidden for authorization failures.

🔎 Proposed fix 
         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 ] );
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Allow if current user is the vendor OR is an administrator/shop manager.
$is_owner = 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' ) );
}
// Allow if current user is the vendor OR is an administrator/shop manager.
$is_owner = 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' ), [ 'status' => 403 ] );
}
🤖 Prompt for AI Agents 
In includes/Vendor/Manager.php around lines 264 to 270, the WP_Error returned
for permission failures lacks an HTTP status causing a 500 instead of 403;
update the WP_Error to include response data with a status of 403 (e.g. supply
an array with 'status' => 403 as the third argument) so the REST API returns
HTTP 403 Forbidden for unauthorized updates.


// default wp based user data
if ( ! empty( $data['user_pass'] ) && get_current_user_id() === $vendor->get_id() ) {
wp_update_user(
Expand Down Expand Up @@ -383,7 +391,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 394 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