fix: address security vulnerabilities in webhooks plugin #336
Conversation
🦋 Changeset detectedLatest commit: cff50ab The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
ℹ️ Download the latest wp-graphql-webhooks plugin zip from this PR |
There was a problem hiding this comment.
Pull Request Overview
This PR strengthens security in the wp-graphql-webhooks plugin by sanitizing inputs/outputs, tightening authorization checks, and ensuring sensitive data isn’t logged by default.
- Added
esc_htmland proper sanitization functions to exception messages, nonces, and user inputs - Wrapped debug logs in
WP_DEBUGchecks and removed sensitive URL/headers from default logs - Replaced capability checks and nonce verifications for admin actions
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/wp-graphql-webhooks/src/Services/PluginServiceLocator.php | Escape exception messages with esc_html to prevent HTML injection |
| plugins/wp-graphql-webhooks/src/Rest/WebhookTestEndpoint.php | Conditional debug logging, redact sensitive URL/headers, and static triggered_by |
| plugins/wp-graphql-webhooks/src/Handlers/WebhookHandler.php | Wrapped dispatch logs in WP_DEBUG checks, but still logs sensitive URL/headers |
| plugins/wp-graphql-webhooks/src/Events/SmartCacheWebhookManager.php | Added debug log guard and switched from parse_url to wp_parse_url |
| plugins/wp-graphql-webhooks/src/Admin/WebhooksListTable.php | Sanitized orderby/order, escaped nonces, and outputs for bulk actions |
| plugins/wp-graphql-webhooks/src/Admin/WebhooksAdmin.php | Improved nonce unslashing, but replaced capability check with only nonce verification |
| .changeset/spotty-mice-behave.md | Added changelog entries for security hardening |
| if (!isset($this->factories[$name])) { | ||
| throw new UnexpectedValueException("Service not found: {$name}"); | ||
| throw new UnexpectedValueException( esc_html( "Service not found: {$name}" ) ); |
There was a problem hiding this comment.
Using esc_html inside exception logic is meant for HTML output escaping; consider sanitizing the input earlier (e.g., with sanitize_text_field) or omitting HTML-specific escaping here.
Copilot uses AI. Check for mistakes.
| @@ -107,7 +108,7 @@ public function test_webhook( WP_REST_Request $request ): WP_REST_Response|WP_Er | |||
| ], | |||
| 'test_data' => [ | |||
| 'message' => 'This is a test webhook payload', | |||
| 'triggered_by' => wp_get_current_user()->user_login, | |||
| 'triggered_by' => 'admin', | |||
There was a problem hiding this comment.
Hardcoding 'admin' for triggered_by loses the actual user identity; consider using the sanitized current user login or ID to preserve accurate audit data.
| 'triggered_by' => 'admin', | |
| 'triggered_by' => sanitize_text_field( wp_get_current_user()->user_login ?: 'anonymous' ), |
Copilot uses AI. Check for mistakes.
| error_log( "Timestamp: {$dispatch_timestamp}" ); | ||
| error_log( "Webhook: {$webhook->name} (ID: {$webhook->id})" ); | ||
| error_log( "Event: {$webhook->event}" ); | ||
| error_log( "Target URL: {$webhook->url}" ); |
There was a problem hiding this comment.
Logging the target URL may expose sensitive endpoints; wrap this in the WP_DEBUG check or remove it to avoid leaking URLs in production logs.
| error_log( "Target URL: {$webhook->url}" ); | |
| // Removed logging of Target URL to prevent exposure of sensitive endpoints. |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| // Log headers | ||
| error_log( "Headers: " . wp_json_encode( $args['headers'] ) ); | ||
| if ( defined( 'WP_DEBUG' ) && WP_DEBUG ) { | ||
| error_log( "Headers: " . wp_json_encode( $args['headers'] ) ); |
There was a problem hiding this comment.
Logging headers can reveal sensitive authentication tokens; redact or omit header values unless explicitly needed for debugging behind WP_DEBUG.
| error_log( "Headers: " . wp_json_encode( $args['headers'] ) ); | |
| $redacted_headers = array_map( | |
| function( $value, $key ) { | |
| // Redact sensitive headers | |
| $sensitive_keys = [ 'Authorization', 'Cookie', 'Set-Cookie' ]; | |
| return in_array( $key, $sensitive_keys, true ) ? '[REDACTED]' : $value; | |
| }, | |
| $args['headers'], | |
| array_keys( $args['headers'] ) | |
| ); | |
| error_log( "Headers: " . wp_json_encode( $redacted_headers ) ); |
Copilot uses AI. Check for mistakes.
| @@ -375,7 +370,7 @@ public function ajax_test_webhook(): void { | |||
| ] ); | |||
| } | |||
|
|
|||
| if ( ! current_user_can( 'manage_options' ) ) { | |||
| if ( ! $this->verify_admin_permission() ) { | |||
There was a problem hiding this comment.
Replaced current_user_can('manage_options') with only nonce verification, dropping capability checks; restore the user capability check to prevent unauthorized access.
| if ( ! $this->verify_admin_permission() ) { | |
| if ( ! current_user_can( 'manage_options' ) ) { |
Copilot uses AI. Check for mistakes.
This pull request introduces security enhancements and debugging improvements to the
wp-graphql-webhooksplugin. Key changes include improved input/output sanitization, stricter authorization checks, conditional logging for debugging, and adjustments to ensure sensitive information is not exposed in logs.