fix(security): implement strict authorization for store settings update #3045
Conversation
📝 WalkthroughWalkthroughPermission check for updating vendor store settings was made request-aware: StoreSettingController now resolves the target vendor from the incoming request and enforces owner-or-admin authorization, returning a 403 WP_Error for unauthorized callers. Vendor::Manager change is a whitespace-only edit. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant REST as StoreSettingController (REST)
participant Vendor as Vendor\Manager / DB
participant Auth as AuthChecker
Client->>REST: Update store settings request
REST->>Vendor: get_vendor(request)
alt vendor not found
Vendor-->>REST: null / error
REST-->>Client: 404 WP_Error
else vendor found
REST->>Auth: get current_user_id / is_admin?
Auth-->>REST: user id, roles
REST->>Vendor: compare target_vendor_id vs current_user_id
alt owner or admin
REST-->>Client: proceed (200 / allow update)
else not owner nor admin
REST-->>Client: 403 WP_Error (access denied)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
includes/REST/StoreSettingController.php (1)
90-92: Propagate error data to preserve HTTP status code.When
Manager::update()returns aWP_Errorwith status data, the current code creates a newWP_Errorthat loses the originalerror_data(including the HTTP status code). Return the error directly to preserve the response status.🔎 Proposed fix
if ( is_wp_error( $store_id ) ) { - return new WP_Error( $store_id->get_error_code(), $store_id->get_error_message() ); + return $store_id; }
🧹 Nitpick comments (1)
includes/REST/StoreSettingController.php (1)
6-6: Remove unused import.The
WP_REST_Controllerimport on line 6 is now dead code since the class extendsDokanBaseVendorController.🔎 Proposed fix
use WeDevs\Dokan\Vendor\Vendor; -use WP_REST_Controller; use WP_REST_Server; use WP_Error;Also applies to: 17-17
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
includes/REST/StoreSettingController.phpincludes/Vendor/Manager.php
🧰 Additional context used
🧬 Code graph analysis (2)
includes/REST/StoreSettingController.php (1)
includes/REST/DokanBaseVendorController.php (1)
DokanBaseVendorController(14-23)
includes/Vendor/Manager.php (1)
includes/Vendor/Vendor.php (1)
get_id(312-314)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: api tests (1, 1)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (3, 3)
🔇 Additional comments (1)
includes/REST/StoreSettingController.php (1)
109-124: LGTM!The
get_settingsmethod correctly retrieves vendor information and payment method data with proper authorization now handled by the inheritedcheck_permissioncallback.
| 'methods' => WP_REST_Server::READABLE, | ||
| 'callback' => [ $this, 'get_settings' ], | ||
| 'permission_callback' => [ $this, 'get_settings_permission_callback' ], | ||
| 'permission_callback' => [ $this, 'check_permission' ], |
There was a problem hiding this comment.
🧩 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 -20Repository: 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.phpRepository: 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.phpRepository: 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.phpRepository: 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.
includes/Vendor/Manager.php
Outdated
| // 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' ) ); | ||
| } |
There was a problem hiding this comment.
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.
| // 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.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
includes/Vendor/Manager.php (1)
264-270: Duplicate: Missing HTTP 403 status code already flagged.The permission check logic correctly addresses the IDOR vulnerability by verifying ownership via
dokan_get_current_user_id()and admin capabilities. However, as noted in the previous review, theWP_Errorlacks an HTTP status code, causing the REST API to return HTTP 500 instead of 403 Forbidden.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Vendor/Manager.php
🧰 Additional context used
🧬 Code graph analysis (1)
includes/Vendor/Manager.php (2)
includes/functions.php (1)
dokan_get_current_user_id(48-61)includes/Vendor/Vendor.php (1)
get_id(312-314)
🪛 GitHub Check: Run PHPCS inspection
includes/Vendor/Manager.php
[failure] 263-263:
Line indented incorrectly; expected at least 2 tabs, found 1 tabs and 3 spaces
[warning] 263-263:
Found precision alignment of 3 spaces.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: api tests (1, 1)
includes/Vendor/Manager.php
Outdated
| $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' ) ); | ||
| } | ||
|
|
||
| // default wp based user data | ||
| if ( ! empty( $data['user_pass'] ) && get_current_user_id() === $vendor->get_id() ) { |
There was a problem hiding this comment.
🧩 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.phpRepository: 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.phpRepository: 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 -10Repository: 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.
Shamim-97
added
Needs: Testing
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
includes/Vendor/Manager.php (2)
267-268: Add HTTP 403 status code to the permission denied error.The
WP_Errorlacks a status code, causing the WordPress REST API to return HTTP 500 instead of HTTP 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 ] ); }
272-272: Usedokan_get_current_user_id()to align with the permission check.The permission check at line 264 uses
dokan_get_current_user_id(), which returns the associated vendor ID for vendor staff users. However, line 272 usesget_current_user_id(), which returns the actual staff user's ID. This inconsistency prevents vendor staff from updating passwords despite passing the permission check.🔎 Proposed fix
-if ( ! empty( $data['user_pass'] ) && get_current_user_id() === $vendor->get_id() ) { +if ( ! empty( $data['user_pass'] ) && dokan_get_current_user_id() === (int) $vendor->get_id() ) { wp_update_user( [ 'ID' => $vendor->get_id(), 'user_pass' => $data['user_pass'], ] ); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Vendor/Manager.php
🧰 Additional context used
🧬 Code graph analysis (1)
includes/Vendor/Manager.php (2)
includes/functions.php (1)
dokan_get_current_user_id(48-61)includes/Vendor/Vendor.php (1)
get_id(312-314)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: api tests (1, 1)
- GitHub Check: e2e tests (2, 3)
🔇 Additional comments (2)
includes/Vendor/Manager.php (2)
322-365: LGTM: Admin-only operations are properly protected.The admin-only metadata updates (enabled, featured, trusted, commission settings) are correctly wrapped in the
current_user_can( 'manage_woocommerce' )capability check, ensuring only authorized administrators can modify these sensitive settings.
308-319: The email update code is properly protected against unauthorized access.The vulnerability from issue #5286 is prevented by the permission checks at the REST endpoint level (lines 359-368 in StoreController.php). Vendor staff with the
dokandarrole can only update their own account (dokan_get_current_user_id() === absint(request->get_param('id'))), preventing them from modifying administrator accounts via IDOR. The secondary permission check in Manager::update() at line 267 provides additional protection. Administrator accounts remain protected from unauthorized email changes.
includes/Vendor/Manager.php
Outdated
| //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' ) ); | ||
| } |
There was a problem hiding this comment.
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.
mrabbani
left a comment
mrabbani
left a comment
There was a problem hiding this comment.
- Permission should be checked properly.
- Ensure that the current user has access to the requested user for the following roles
- Admin
- Vendor
- Staff
mrabbani
added
Needs: Author Reply
and removed
Needs: Dev Review
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
includes/REST/StoreSettingController.php (1)
131-156: Critical: Missing$requestparameter in method signature breaks the security fix.The method signature declares no parameters, but line 133 uses an undefined
$requestvariable. This will cause a PHP error and the authorization check will fail to work as intended—completely defeating the purpose of this security fix.WordPress REST API permission callbacks receive the
WP_REST_Requestobject as their first argument; the signature must declare it.🔎 Proposed fix
- public function get_settings_permission_callback() { -// 2. Get the vendor object based on the REQUEST (this captures the target vendor_id) + public function get_settings_permission_callback( $request ) { + // Get the vendor object based on the request (this captures the target vendor_id) $vendor = $this->get_vendor( $request );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
includes/REST/StoreSettingController.phpincludes/Vendor/Manager.php
💤 Files with no reviewable changes (1)
- includes/Vendor/Manager.php
🧰 Additional context used
🪛 GitHub Actions: Inspections
includes/REST/StoreSettingController.php
[error] 1-1: PHPCS: Line indented incorrectly; expected at least 2 tabs, found 0.
🪛 GitHub Check: Run PHPCS inspection
includes/REST/StoreSettingController.php
[failure] 151-151:
Whitespace found at end of line
[failure] 150-150:
Whitespace found at end of line
[failure] 149-149:
Whitespace found at end of line
[failure] 148-148:
Whitespace found at end of line
[failure] 132-132:
Line indented incorrectly; expected at least 2 tabs, found 0
🪛 PHPMD (2.15.0)
includes/REST/StoreSettingController.php
133-133: Avoid unused local variables such as '$request'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (1)
includes/REST/StoreSettingController.php (1)
140-153: Authorization logic looks correct once the$requestparameter is added.The ownership and admin checks are well-structured:
- Uses
dokan_get_current_user_id()which correctly handles vendor staff scenarios (returns the associated vendor ID)- Admin check covers both
manage_woocommerceandmanage_optionscapabilities- Properly returns a 403 error with an appropriate message when access is denied
This addresses the IDOR vulnerability by ensuring only the vendor owner or an admin can access/modify settings.
| */ | ||
| public function get_settings_permission_callback() { | ||
| $vendor = $this->get_vendor(); | ||
| // 2. Get the vendor object based on the REQUEST (this captures the target vendor_id) |
There was a problem hiding this comment.
Fix PHPCS failures: incorrect indentation and trailing whitespace.
The pipeline is failing due to formatting issues:
- Line 132: Comment is not indented (expected at least 2 tabs)
- Lines 148-151: Trailing whitespace at end of lines
🔎 Proposed fix for WP_Error block (remove trailing whitespace)
- return new WP_Error(
- 'dokan_permission_denied',
- __( 'You do not have permission to access these settings.', 'dokan-lite' ),
- [ 'status' => 403 ]
+ return new WP_Error(
+ 'dokan_permission_denied',
+ __( 'You do not have permission to access these settings.', 'dokan-lite' ),
+ [ 'status' => 403 ]
);Also applies to: 148-152
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 132-132:
Line indented incorrectly; expected at least 2 tabs, found 0
🤖 Prompt for AI Agents
In includes/REST/StoreSettingController.php around lines 132 and 148-152, fix
PHPCS failures by indenting the comment on line 132 with at least two tabs to
match surrounding block indentation, and remove any trailing whitespace on lines
148-152 (trim end-of-line spaces); after making these whitespace/indentation
corrections, run phpcs (or the project's formatting tool) to verify the fixes.
|
@Shamim-97 Not. work, still customer can change vendor setting data. make sure to test after an fix |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
includes/REST/StoreSettingController.php (1)
208-208: Address PHPCS warning: avoid reserved keyword as parameter name.The pipeline reports that
$objectis a reserved keyword in PHP 7.2+. For consistency with the rest of the codebase, consider renaming to$store:🔎 Suggested fix
- protected function prepare_links( $object, $request ) { + protected function prepare_links( $store, $request ) { $links = [ 'self' => [ - 'href' => rest_url( sprintf( '/%s/%s/%d', $this->namespace, $this->rest_base, $object['id'] ) ), + 'href' => rest_url( sprintf( '/%s/%s/%d', $this->namespace, $this->rest_base, $store['id'] ) ), ], 'collection' => [ 'href' => rest_url( sprintf( '/%s/%s', $this->namespace, $this->rest_base ) ), ], ]; return $links; }
🧹 Nitpick comments (1)
includes/REST/StoreSettingController.php (1)
145-178: Excellent: Authorization logic is sound and comprehensive.The permission callback correctly enforces vendor ownership:
- ✅ Validates vendor exists before authorization check
- ✅ Uses
dokan_get_current_user_id()to properly handle vendor staff- ✅ Strict comparison prevents type juggling issues
- ✅ Admin override via
manage_woocommercecapability- ✅ Clear 403 error for unauthorized access
This properly addresses the IDOR vulnerability reported in issue #5293.
Optional: Consider array syntax consistency.
Lines 160 and 173 use
array()while the rest of the file uses short array syntax[]. For consistency and readability:🔎 Suggested array syntax alignment
if ( empty( $vendor->get_id() ) ) { - return new WP_Error( 'no_store_found', __( 'No vendor found', 'dokan-lite' ), array( 'status' => 404 ) ); + return new WP_Error( 'no_store_found', __( 'No vendor found', 'dokan-lite' ), [ 'status' => 404 ] ); }if ( ! $is_admin && ! $is_owner ) { return new WP_Error( 'dokan_permission_denied', __( 'You do not have permission to access these settings.', 'dokan-lite' ), - array( 'status' => 403 ) + [ 'status' => 403 ] ); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/REST/StoreSettingController.php
🧰 Additional context used
🧬 Code graph analysis (1)
includes/REST/StoreSettingController.php (2)
includes/Withdraw/Withdraw.php (1)
get_id(82-84)includes/functions.php (1)
dokan_get_current_user_id(48-61)
🪛 GitHub Actions: Inspections
includes/REST/StoreSettingController.php
[warning] 1-1: PHPCS: It is recommended not to use reserved keyword "object" as function parameter name. Found: $object
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: api tests (1, 1)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: e2e tests (2, 3)
🔇 Additional comments (1)
includes/REST/StoreSettingController.php (1)
60-60: LGTM: Permission callback correctly separated for write operations.The update route now enforces vendor ownership through the new
update_settings_permission_callback, properly addressing the IDOR vulnerability.
Shamim-97
added
Needs: Dev Review
|
❌ VULNERABILITY CONFIRMED: A logged in customer/vendor can read another vendor's settings! If Manager::update() is called directly (bypassing REST API), an admin's email can be changedNo check for administrator role Duplicate email validation missingOriginal code: No duplicate email check
bhai, here also need Manager layer lacks protection |
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
Title
Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.
Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
Security
Refactor
Style
✏️ Tip: You can customize this high-level summary in your review settings.