IDOR vulnerability fix #643
Conversation
WalkthroughAdds server-side security validation to two AJAX calendar view handlers: both now validate the calendar post exists and is of type Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 0
🧹 Nitpick comments (2)
includes/calendars/views/default-calendar-grid.php (1)
664-669: Redundant login check for private calendars.The
is_user_logged_in()check is redundant becausecurrent_user_can('read_post', $id)already returnsfalsefor logged-out users when the post is private.Apply this diff to simplify the logic:
} elseif ($post->post_status === 'private') { - // Private calendar - check if user has permission - if (!is_user_logged_in() || !current_user_can('read_post', $id)) { + // Private calendar - check if user has permission + if (!current_user_can('read_post', $id)) { wp_send_json_error('You do not have permission to view this calendar.'); return; }includes/calendars/views/default-calendar-list.php (1)
791-796: Redundant login check for private calendars.The
is_user_logged_in()check is redundant becausecurrent_user_can('read_post', $id)already returnsfalsefor logged-out users when the post is private.Apply this diff to simplify the logic:
} elseif ($post->post_status === 'private') { - // Private calendar - check if user has permission - if (!is_user_logged_in() || !current_user_can('read_post', $id)) { + // Private calendar - check if user has permission + if (!current_user_can('read_post', $id)) { wp_send_json_error('You do not have permission to view this calendar.'); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
includes/calendars/views/default-calendar-grid.php(1 hunks)includes/calendars/views/default-calendar-list.php(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: build
includes/calendars/views/default-calendar-list.php
[error] 1-1: Prettier formatting check failed on this file. Run 'prettier --write' to fix code style issues.
includes/calendars/views/default-calendar-grid.php
[error] 1-1: Prettier formatting check failed on this file. Run 'prettier --write' to fix code style issues.
🔇 Additional comments (1)
includes/calendars/views/default-calendar-list.php (1)
779-803: LGTM! Effective IDOR vulnerability fix.The security validation properly addresses the reported IDOR vulnerability by enforcing access control based on post status and user capabilities. The implementation correctly uses WordPress core functions and follows the platform's security model.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
includes/calendars/views/default-calendar-grid.php (2)
645-682: IDOR fix looks correct, but consider adding nonce verification for CSRF protection.The permission checks properly address the IDOR vulnerability by validating post existence, type, and user capabilities before rendering. However, this AJAX handler lacks nonce verification, which is a WordPress security best practice for AJAX endpoints to prevent CSRF attacks.
Consider adding nonce verification:
public function draw_grid_ajax() { if (isset($_POST['month']) && isset($_POST['year']) && isset($_POST['id'])) { + // Verify nonce for CSRF protection + if (!isset($_POST['nonce']) || !wp_verify_nonce($_POST['nonce'], 'simcal_default_calendar')) { + wp_send_json_error('Security check failed.'); + return; + } + $month = absint($_POST['month']); $year = absint($_POST['year']); $id = absint($_POST['id']);Note: This would require passing a nonce from the JavaScript side when making the AJAX request.
662-676: Consider refactoring to eliminate the empty if-block.The empty
ifblock for published status is functional but could be cleaner using early return or restructured conditions.- // Check if post is published (public) or user has permission to read it - if ($post->post_status === 'publish') { - // Public calendar - allow access - } elseif ($post->post_status === 'private') { - // Private calendar - check if user has permission - if (!is_user_logged_in() || !current_user_can('read_post', $id)) { - wp_send_json_error('You do not have permission to view this calendar.'); - return; - } - } else { - // Draft, pending, or other status - check if user has permission - if (!current_user_can('read_post', $id)) { - wp_send_json_error('You do not have permission to view this calendar.'); - return; - } - } + // Check if user has permission to view the calendar + if ($post->post_status !== 'publish') { + // Private calendars require login + if ($post->post_status === 'private' && !is_user_logged_in()) { + wp_send_json_error('You do not have permission to view this calendar.'); + return; + } + // Non-public calendars require read capability + if (!current_user_can('read_post', $id)) { + wp_send_json_error('You do not have permission to view this calendar.'); + return; + } + }includes/calendars/views/default-calendar-list.php (1)
773-809: IDOR fix correctly mirrors the grid view implementation.The permission checks are consistent with the grid view handler, properly addressing the IDOR vulnerability. The same recommendations apply:
- Consider adding nonce verification for CSRF protection
- The empty
ifblock forpublishstatus could be refactoredApply the same nonce verification pattern suggested for the grid view:
public function draw_list_ajax() { if (isset($_POST['ts']) && isset($_POST['id'])) { + // Verify nonce for CSRF protection + if (!isset($_POST['nonce']) || !wp_verify_nonce($_POST['nonce'], 'simcal_default_calendar')) { + wp_send_json_error('Security check failed.'); + return; + } + $ts = absint($_POST['ts']); $id = absint($_POST['id']);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
includes/calendars/views/default-calendar-grid.php(1 hunks)includes/calendars/views/default-calendar-list.php(1 hunks)package.json(1 hunks)readme.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- readme.txt
🔇 Additional comments (2)
package.json (1)
5-5: Version bump aligns with security fix.The version increment to 3.5.10 appropriately reflects the IDOR vulnerability fix being introduced.
includes/calendars/views/default-calendar-list.php (1)
139-156: The nonce is already available in the frontend. Thesimcal_common_scripts_variables()function atincludes/functions/shared.php:166creates and passes the nonce to the frontend viawp_create_nonce('simcal'), which is then localized as part of the script variables. The frontend JavaScript can access this nonce through the localizedsimcal_default_calendarobject.Likely an incorrect or invalid review comment.
Description: We got an message regarding IDOR vulnerability from patchstack, in this if any calendar is private assign still in ajax request someone can pass the private calendar ID and get the HTML for this private calendars events which is now fix by adding permission check and check for longed in user if the calendar is set to private for public it is working as it is.
Clickup: https://app.clickup.com/t/86d135nbm
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.