-
Notifications
You must be signed in to change notification settings - Fork 32
Enhance security checks and improve I18N handling #160
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.
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
base: master
Are you sure you want to change the base?
Changes from 16 commits
95e00ff
a60017c
e6fd88a
24752f0
acbc92f
c6714a3
e3ed4b9
8921f5d
854eb86
6cd801a
60bd5f3
c9b578a
9e9fe6d
6f54eea
9fa11c9
b996182
6e5a776
78478d0
2c86f7f
5e6657a
e41335c
8693705
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| # .ai/ — Agent Context Directory | ||
|
|
||
| This directory stores documentation and context generated or maintained by AI agents (Claude Code, etc.) during development work. | ||
|
|
||
| ## Purpose | ||
|
|
||
| - **Deep-dive architecture docs** — detail too verbose for `CLAUDE.md` | ||
| - **Investigation notes** — written while exploring complex features | ||
| - **Feature context** — background and rationale for non-obvious design decisions | ||
|
|
||
| This is NOT a replacement for code comments, git history, or `docs/`. It supplements `CLAUDE.md` with detail that would otherwise bloat it. | ||
|
|
||
| > **Agents:** When you discover context worth preserving, write it here — not into `CLAUDE.md`. Reference the file from `CLAUDE.md` with a one-line pointer. | ||
|
|
||
| ## Structure | ||
|
|
||
| ```text | ||
| .ai/ | ||
| README.md # This file — directory overview | ||
| security/ # Vulnerability reports and fix guidance (excluded from git) | ||
| ``` | ||
|
|
||
| ## Rules for Agents | ||
|
|
||
| - **Write here, not in `CLAUDE.md`** — keep CLAUDE.md as a concise rules + pointers file | ||
| - **One file per topic** — don't append unrelated notes to an existing file | ||
| - **This directory is excluded from distribution zips** — `.distignore` covers `.ai/` recursively |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,5 +8,7 @@ node_modules/ | |
| vendor/ | ||
| .vscode/ | ||
| .idea | ||
| .claude/worktrees/ | ||
| package-lock.json | ||
| pnpm-lock.yaml | ||
| .ai/security/ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| # Disable Comments — Plugin Development Guide | ||
|
|
||
| WordPress plugin by WPDeveloper. Allows administrators to globally disable comments by post type, with multisite network support. | ||
|
|
||
| - **WordPress.org:** <https://wordpress.org/plugins/disable-comments/> | ||
| - **Current version:** 2.6.2 | ||
| - **Main class:** `Disable_Comments` (singleton) in `disable-comments.php` | ||
|
|
||
| --- | ||
|
|
||
| ## Project Structure | ||
|
|
||
| ```text | ||
| disable-comments.php Main plugin file (~2000 lines), single class | ||
| includes/ | ||
| cli.php WP-CLI command definitions | ||
| class-plugin-usage-tracker.php | ||
| views/ | ||
| settings.php Main settings page shell | ||
| comments.php Tools/delete page shell | ||
| partials/ | ||
| _disable.php Disable-comments form (main settings form) | ||
| _delete.php Delete comments form | ||
| _sites.php Multisite sub-site list | ||
| _menu.php / _footer.php / _sidebar.php | ||
| assets/ | ||
| js/disable-comments-settings-scripts.js Settings page JS (role exclusion UI, AJAX calls) | ||
| js/disable-comments.js | ||
| css/ scss/ | ||
| tests/ | ||
| test-plugin.php PHPUnit tests (Brain/Monkey mocking) | ||
| bootstrap.php | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Key AJAX Handlers | ||
|
|
||
| All three AJAX handlers are registered in `__construct()` (~line 49): | ||
|
|
||
| | Action | Handler | Line | | ||
| | ------ | ------- | ---- | | ||
| | `disable_comments_save_settings` | `disable_comments_settings()` | ~1217 | | ||
| | `disable_comments_delete_comments` | `delete_comments_settings()` | ~1324 | | ||
| | `get_sub_sites` | `get_sub_sites()` | ~1157 | | ||
|
|
||
| **Nonce:** All handlers verify nonce `disable_comments_save_settings`. The nonce is created in `admin_enqueue_scripts()` (~line 799) and exposed to JS as `disableCommentsObj._nonce`. | ||
|
|
||
| **POST data parsing:** `get_form_array_escaped()` (~line 1202) reads `$_POST['data']` as a URL-encoded string, parses with `wp_parse_args()`, and sanitizes all values with `map_deep(..., 'sanitize_text_field')`. | ||
|
|
||
| **Network admin flag:** `$formArray['is_network_admin']` comes from POST data and controls network-wide operations — always verify server-side capability before acting on it. | ||
|
|
||
| --- | ||
|
|
||
| ## Development | ||
|
|
||
| ```bash | ||
| npm install # Install JS build deps | ||
| npm run build # Compile JS/CSS via Grunt + Babel | ||
| npm run release # Build + generate .pot + package release | ||
| ``` | ||
|
|
||
| ```bash | ||
| composer install # Install PHP dev deps (Brain/Monkey for tests) | ||
| ./vendor/bin/phpunit # Run tests | ||
| ``` | ||
|
|
||
| **Linting:** `phpcs.ruleset.xml` is configured for WordPress Coding Standards. | ||
|
|
||
| --- | ||
|
|
||
| ## Architecture Notes | ||
|
|
||
| - **Singleton pattern:** Always access via `Disable_Comments::get_instance()`. | ||
| - **CLI support:** `includes/cli.php` calls the same handler methods with `$_args` to bypass nonce (expected for WP-CLI context; nonce bypass is gated on `$this->is_CLI`). | ||
| - **Multisite vs single-site:** Plugin behaviour branches heavily on `$this->networkactive` (set in constructor) and `$this->sitewide_settings`. | ||
| - **Database queries:** Use `$wpdb->prepare()` throughout `delete_comments()`. Safe against SQL injection. | ||
| - **Input sanitization:** `get_form_array_escaped()` uses `wp_parse_args()` + `map_deep(sanitize_text_field)`. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -116,11 +116,7 @@ function __construct() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public function is_network_admin() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $sanitized_referer = isset($_SERVER['HTTP_REFERER']) ? sanitize_text_field(wp_unslash($_SERVER['HTTP_REFERER'])) : ''; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (is_network_admin() || !empty($sanitized_referer) && defined('DOING_AJAX') && DOING_AJAX && is_multisite() && preg_match('#^' . network_admin_url() . '#i', $sanitized_referer)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return is_network_admin(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Enable CLI | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -829,7 +825,7 @@ public function discussion_notice() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // translators: %s: disabled post types. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo '<div class="notice notice-warning"><p>' . sprintf(esc_html__('Note: The <em>Disable Comments</em> plugin is currently active, and comments are completely disabled on: %s. Many of the settings below will not be applicable for those post types.', 'disable-comments'), implode(esc_html__(', ', 'disable-comments'), $names_escaped)) . '</p></div>'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo '<div class="notice notice-warning"><p>' . sprintf(__('Note: The <em>Disable Comments</em> plugin is currently active, and comments are completely disabled on: %s. Many of the settings below will not be applicable for those post types.', 'disable-comments'), implode(__(', ', 'disable-comments'), $names_escaped)) . '</p></div>'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1119,7 +1115,7 @@ public function get_roles($selected) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| foreach ($editable_roles as $role => $details) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $roles[] = [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "id" => esc_attr($role), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "text" => translate_user_role($details['name']), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "text" => esc_html(translate_user_role($details['name'])), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "selected" => in_array($role, (array) $selected), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1162,6 +1158,10 @@ public function get_sub_sites() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| wp_send_json(['data' => [], 'totalNumber' => 0]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!current_user_can('manage_network_plugins') && !current_user_can('manage_options')) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| wp_send_json(['data' => [], 'totalNumber' => 0]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| wp_send_json(['data' => [], 'totalNumber' => 0]); | |
| } | |
| if (!current_user_can('manage_network_plugins') && !current_user_can('manage_options')) { | |
| wp_send_json(['data' => [], 'totalNumber' => 0]); | |
| wp_send_json_error( | |
| [ | |
| 'message' => __('Invalid request. Please refresh the page and try again.', 'disable-comments'), | |
| 'data' => [], | |
| 'totalNumber' => 0, | |
| ], | |
| 400 | |
| ); | |
| } | |
| // Restrict access more strictly on multisite: only network admins may enumerate subsites. | |
| if (is_multisite()) { | |
| if (!current_user_can('manage_network_plugins')) { | |
| wp_send_json_error( | |
| [ | |
| 'message' => __('Sorry, you are not allowed to access this resource.', 'disable-comments'), | |
| 'data' => [], | |
| 'totalNumber' => 0, | |
| ], | |
| 403 | |
| ); | |
| } | |
| } else { | |
| if (!current_user_can('manage_options')) { | |
| wp_send_json_error( | |
| [ | |
| 'message' => __('Sorry, you are not allowed to access this resource.', 'disable-comments'), | |
| 'data' => [], | |
| 'totalNumber' => 0, | |
| ], | |
| 403 | |
| ); | |
| } |
Copilot
AI
Mar 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wp_send_json_error() here doesn’t set an HTTP status code. Since this error is an authorization failure, returning a 403 status (and ensuring the JS handles non-2xx responses appropriately) makes the behavior consistent with get_sub_sites() and prevents UI states getting stuck on an error response.
| wp_send_json_error(['message' => __('Insufficient permissions.', 'disable-comments')]); | |
| wp_send_json_error(['message' => __('Insufficient permissions.', 'disable-comments')], 403); |
Copilot
AI
Mar 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New capability gating was added to the AJAX settings handlers, but there are no unit/integration tests covering the “network action requires manage_network_plugins vs. site action requires manage_options” behavior. Given this is security-sensitive logic, consider adding PHPUnit coverage to ensure unauthorized users receive an error and authorized users can still save settings.
Copilot
AI
Mar 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: this permission failure returns JSON without an HTTP 403 status. Using a 403 (and handling it client-side) improves correctness and aligns with the other AJAX handler (get_sub_sites) which already returns a 403 on insufficient permissions.
| wp_send_json_error(['message' => __('Insufficient permissions.', 'disable-comments')]); | |
| wp_send_json_error(['message' => __('Insufficient permissions.', 'disable-comments')], 403); |
Copilot
AI
Mar 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete_comments_settings() also returns wp_send_json_success() even when the nonce check fails, because the success response is sent unconditionally after the guarded block. This can report a successful deletion (often with a meaningless “.” message) for invalid/expired nonces. Send a JSON error (HTTP 403) and exit when nonce verification fails.
Copilot
AI
Mar 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new permission logic and multisite membership filtering in delete_comments_settings() isn’t covered by tests. Add PHPUnit tests to assert 403s for insufficient capability and nonce failure, plus a multisite test where a non-super-admin cannot delete comments on a site they’re not a member of (ensuring those sites are skipped/blocked as intended).
Copilot
AI
Mar 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This uses __() inside sprintf() for a value intended to be plain text in Site Health. Using an escaping i18n helper (e.g., esc_html__()) prevents translations from injecting markup into the debug output.
| 'value' => sprintf(__('%1$d of %2$d', 'disable-comments'), count($data['disabled_post_types']), $data['total_post_types']), | |
| 'value' => sprintf(esc_html__('%1$d of %2$d', 'disable-comments'), count($data['disabled_post_types']), $data['total_post_types']), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_network_admin()previously treated multisite AJAX requests originating from Network Admin as “network” by checking the request referer, which ensured network-initiated AJAX calls loaded/saved site options correctly. After simplifying toreturn is_network_admin();, network admin AJAX requests routed throughadmin-ajax.phpwill likely reportfalse, which can cause the constructor to load per-site options during network actions (e.g., affecting$old_optionsusage when saving network settings). Consider restoring an AJAX-safe network-context detection (e.g., checking for network admin AJAX endpoint or a verified server-side flag) or explicitly loading site options inside the network action handlers whenis_network_actionis true.