Skip to content

Commit 8a9be18

Browse files
josephfuscojosephfusco
authored
Merge pull request #336 from wpengine/webhooks-plugin-security-review
fix: address security vulnerabilities in webhooks plugin
2 parents d404e2c + cff50ab commit 8a9be18

7 files changed

Lines changed: 72 additions & 48 deletions

File tree

‎.changeset/spotty-mice-behave.md‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
"@wpengine/wpgraphql-webhooks-wordpress-plugin": patch
3+
---
4+
5+
fix: security improvements for webhooks plugin
6+
7+
- Enhanced input validation and sanitization
8+
- Improved output escaping
9+
- Strengthened authorization checks
10+
- Added additional security hardening measures

‎plugins/wp-graphql-webhooks/src/Admin/WebhooksAdmin.php‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ private function verify_admin_permission(): bool {
168168
* @return bool True if nonce is valid, false otherwise.
169169
*/
170170
private function verify_nonce( string $nonce_name, string $action ): bool {
171-
if ( ! isset( $_REQUEST[ $nonce_name ] ) || ! wp_verify_nonce( $_REQUEST[ $nonce_name ], $action ) ) {
171+
if ( ! isset( $_REQUEST[ $nonce_name ] ) || ! wp_verify_nonce( wp_unslash( $_REQUEST[ $nonce_name ] ), $action ) ) {
172172
wp_die( __( 'Security check failed.', 'wp-graphql-webhooks' ) );
173173
return false;
174174
}
@@ -185,11 +185,6 @@ public function handle_webhook_save() {
185185
wp_die( __( 'Unauthorized', 'wp-graphql-webhooks' ) );
186186
}
187187

188-
$webhook_id = isset( $_POST['webhook_id'] ) ? intval( $_POST['webhook_id'] ) : 0;
189-
if ( ! $this->verify_admin_permission() || ! $this->verify_nonce( 'webhook_nonce', 'webhook_save' ) ) {
190-
wp_die( __( 'Unauthorized', 'wp-graphql-webhooks' ) );
191-
}
192-
193188
$webhook_id = isset( $_POST['webhook_id'] ) ? intval( $_POST['webhook_id'] ) : 0;
194189
$webhook = new Webhook(
195190
$webhook_id,
@@ -375,7 +370,7 @@ public function ajax_test_webhook(): void {
375370
] );
376371
}
377372

378-
if ( ! current_user_can( 'manage_options' ) ) {
373+
if ( ! $this->verify_admin_permission() ) {
379374
wp_send_json_error( [
380375
'message' => __( 'You do not have permission to test webhooks.', 'wp-graphql-webhooks' ),
381376
'error_code' => 'insufficient_permissions'

‎plugins/wp-graphql-webhooks/src/Admin/WebhooksListTable.php‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -92,13 +92,13 @@ public function process_bulk_action() {
9292
}
9393

9494
// Verify nonce
95-
if ( ! isset( $_REQUEST['_wpnonce'] ) || ! wp_verify_nonce( $_REQUEST['_wpnonce'], 'bulk-' . $this->_args['plural'] ) ) {
96-
wp_die( __( 'Security check failed.', 'wp-graphql-webhooks' ) );
95+
if ( ! isset( $_REQUEST['_wpnonce'] ) || ! wp_verify_nonce( wp_unslash( $_REQUEST['_wpnonce'] ), 'bulk-' . $this->_args['plural'] ) ) {
96+
wp_die( esc_html__( 'Security check failed.', 'wp-graphql-webhooks' ) );
9797
}
9898

9999
// Check permissions
100100
if ( ! current_user_can( 'manage_options' ) ) {
101-
wp_die( __( 'You do not have sufficient permissions to access this page.', 'wp-graphql-webhooks' ) );
101+
wp_die( esc_html__( 'You do not have sufficient permissions to access this page.', 'wp-graphql-webhooks' ) );
102102
}
103103

104104
// Get selected webhooks
@@ -137,8 +137,8 @@ public function prepare_items() {
137137
$total_items = count( $webhooks );
138138

139139
// Handle sorting
140-
$orderby = ! empty( $_GET['orderby'] ) ? $_GET['orderby'] : 'name';
141-
$order = ! empty( $_GET['order'] ) ? $_GET['order'] : 'asc';
140+
$orderby = ! empty( $_GET['orderby'] ) ? sanitize_key( $_GET['orderby'] ) : 'name';
141+
$order = ! empty( $_GET['order'] ) ? sanitize_key( $_GET['order'] ) : 'asc';
142142

143143
usort( $webhooks, function( $a, $b ) use ( $orderby, $order ) {
144144
$result = 0;
@@ -253,7 +253,7 @@ public function column_name( $item ) {
253253
* Display when no items
254254
*/
255255
public function no_items() {
256-
_e( 'No webhooks found.', 'wp-graphql-webhooks' );
256+
esc_html_e( 'No webhooks found.', 'wp-graphql-webhooks' );
257257
}
258258

259259
/**

‎plugins/wp-graphql-webhooks/src/Events/SmartCacheWebhookManager.php‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,9 @@ private function trigger_webhooks( string $event, array $payload ): void {
116116
$payload['uri'] = $payload['path'] ?? '';
117117
}
118118

119-
error_log( "[Webhook] Triggering webhooks for event: $event with payload: " . var_export( $payload, true ) );
119+
if ( defined( 'WP_DEBUG' ) && WP_DEBUG ) {
120+
error_log( "[Webhook] Triggering webhooks for event: $event with payload: " . var_export( $payload, true ) );
121+
}
120122

121123
do_action( 'graphql_webhooks_before_trigger', $event, $payload );
122124

@@ -240,7 +242,7 @@ public function get_path_from_key( $key ) {
240242
}
241243

242244
if ( ! empty( $permalink ) && is_string( $permalink ) && ! is_wp_error( $permalink ) ) {
243-
$parsed_path = parse_url( $permalink, PHP_URL_PATH );
245+
$parsed_path = wp_parse_url( $permalink, PHP_URL_PATH );
244246
if ( $parsed_path !== false ) {
245247
$path = $parsed_path;
246248
error_log( "[Webhook] Final path for key $key: $path" );

‎plugins/wp-graphql-webhooks/src/Handlers/WebhookHandler.php‎

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,14 @@ class WebhookHandler implements Handler {
2222
public function handle( Webhook $webhook, array $payload ): void {
2323
// Log webhook dispatch initiation
2424
$dispatch_timestamp = current_time( 'mysql' );
25-
error_log( "\n========== WEBHOOK DISPATCH ==========" );
26-
error_log( "Timestamp: {$dispatch_timestamp}" );
27-
error_log( "Webhook: {$webhook->name} (ID: {$webhook->id})" );
28-
error_log( "Event: {$webhook->event}" );
29-
error_log( "Target URL: {$webhook->url}" );
30-
error_log( "Method: {$webhook->method}" );
25+
if ( defined( 'WP_DEBUG' ) && WP_DEBUG ) {
26+
error_log( "\n========== WEBHOOK DISPATCH ==========" );
27+
error_log( "Timestamp: {$dispatch_timestamp}" );
28+
error_log( "Webhook: {$webhook->name} (ID: {$webhook->id})" );
29+
error_log( "Event: {$webhook->event}" );
30+
error_log( "Target URL: {$webhook->url}" );
31+
error_log( "Method: {$webhook->method}" );
32+
}
3133

3234
$args = [
3335
'headers' => $webhook->headers ?: [ 'Content-Type' => 'application/json' ],
@@ -52,7 +54,9 @@ public function handle( Webhook $webhook, array $payload ): void {
5254
if ( strtoupper( $webhook->method ) === 'GET' ) {
5355
$url = add_query_arg( $payload, $webhook->url );
5456
$args['method'] = 'GET';
55-
error_log( "Payload (GET query params): " . wp_json_encode( $payload ) );
57+
if ( defined( 'WP_DEBUG' ) && WP_DEBUG ) {
58+
error_log( "Payload (GET query params): " . wp_json_encode( $payload ) );
59+
}
5660
} else {
5761
$url = $webhook->url;
5862
$args['method'] = strtoupper( $webhook->method );
@@ -63,20 +67,28 @@ public function handle( Webhook $webhook, array $payload ): void {
6367
$args['headers']['Content-Type'] = 'application/json';
6468
}
6569

66-
error_log( "Payload ({$args['method']} body): " . $args['body'] );
67-
error_log( "Payload size: " . strlen( $args['body'] ) . " bytes" );
70+
if ( defined( 'WP_DEBUG' ) && WP_DEBUG ) {
71+
error_log( "Payload ({$args['method']} body): " . $args['body'] );
72+
error_log( "Payload size: " . strlen( $args['body'] ) . " bytes" );
73+
}
6874
}
6975

7076
// Log headers
71-
error_log( "Headers: " . wp_json_encode( $args['headers'] ) );
77+
if ( defined( 'WP_DEBUG' ) && WP_DEBUG ) {
78+
error_log( "Headers: " . wp_json_encode( $args['headers'] ) );
79+
}
7280

7381
// For test mode or debugging, optionally use blocking mode
7482
if ( apply_filters( 'graphql_webhooks_test_mode', false, $webhook ) ) {
7583
$args['blocking'] = true;
76-
error_log( "Test mode enabled - using blocking request" );
84+
if ( defined( 'WP_DEBUG' ) && WP_DEBUG ) {
85+
error_log( "Test mode enabled - using blocking request" );
86+
}
7787
}
7888

79-
error_log( "====================================\n" );
89+
if ( defined( 'WP_DEBUG' ) && WP_DEBUG ) {
90+
error_log( "====================================\n" );
91+
}
8092

8193
// Send the webhook
8294
$start_time = microtime( true );
@@ -85,7 +97,7 @@ public function handle( Webhook $webhook, array $payload ): void {
8597
$duration = round( ( $end_time - $start_time ) * 1000, 2 );
8698

8799
// Log response if in blocking mode
88-
if ( $args['blocking'] ) {
100+
if ( $args['blocking'] && defined( 'WP_DEBUG' ) && WP_DEBUG ) {
89101
if ( is_wp_error( $response ) ) {
90102
error_log( "\n========== WEBHOOK ERROR ==========" );
91103
error_log( "❌ ERROR: " . $response->get_error_message() );

‎plugins/wp-graphql-webhooks/src/Rest/WebhookTestEndpoint.php‎

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -86,15 +86,16 @@ public function test_webhook( WP_REST_Request $request ): WP_REST_Response|WP_Er
8686

8787
// Log test initiation
8888
$test_timestamp = current_time( 'mysql' );
89-
error_log( "\n========== WEBHOOK TEST INITIATED ==========" );
90-
error_log( "Timestamp: {$test_timestamp}" );
91-
error_log( "Webhook ID: {$webhook_id}" );
92-
error_log( "Webhook Name: {$webhook->name}" );
93-
error_log( "Target URL: {$webhook->url}" );
94-
error_log( "HTTP Method: {$webhook->method}" );
95-
error_log( "Event: {$webhook->event}" );
96-
error_log( "Headers: " . wp_json_encode( $webhook->headers ) );
97-
error_log( "==========================================\n" );
89+
if ( defined( 'WP_DEBUG' ) && WP_DEBUG ) {
90+
error_log( "\n========== WEBHOOK TEST INITIATED ==========" );
91+
error_log( "Timestamp: {$test_timestamp}" );
92+
error_log( "Webhook ID: {$webhook_id}" );
93+
error_log( "Webhook Name: {$webhook->name}" );
94+
// Do not log sensitive URL and headers
95+
error_log( "HTTP Method: {$webhook->method}" );
96+
error_log( "Event: {$webhook->event}" );
97+
error_log( "==========================================\n" );
98+
}
9899

99100
// Create test payload
100101
$test_payload = [
@@ -107,7 +108,7 @@ public function test_webhook( WP_REST_Request $request ): WP_REST_Response|WP_Er
107108
],
108109
'test_data' => [
109110
'message' => 'This is a test webhook payload',
110-
'triggered_by' => wp_get_current_user()->user_login,
111+
'triggered_by' => 'admin',
111112
'site_url' => get_site_url(),
112113
],
113114
];
@@ -124,11 +125,13 @@ public function test_webhook( WP_REST_Request $request ): WP_REST_Response|WP_Er
124125
$end_time = microtime( true );
125126
$duration = round( ( $end_time - $start_time ) * 1000, 2 ); // Convert to milliseconds
126127

127-
error_log( "\n========== WEBHOOK TEST COMPLETED ==========" );
128-
error_log( "✅ SUCCESS: Test webhook dispatched" );
129-
error_log( "Duration: {$duration}ms" );
130-
error_log( "Completed at: " . current_time( 'mysql' ) );
131-
error_log( "==========================================\n" );
128+
if ( defined( 'WP_DEBUG' ) && WP_DEBUG ) {
129+
error_log( "\n========== WEBHOOK TEST COMPLETED ==========" );
130+
error_log( "✅ SUCCESS: Test webhook dispatched" );
131+
error_log( "Duration: {$duration}ms" );
132+
error_log( "Completed at: " . current_time( 'mysql' ) );
133+
error_log( "==========================================\n" );
134+
}
132135

133136
return new WP_REST_Response(
134137
[
@@ -147,10 +150,12 @@ public function test_webhook( WP_REST_Request $request ): WP_REST_Response|WP_Er
147150
200
148151
);
149152
} catch ( \Exception $e ) {
150-
error_log( "\n========== WEBHOOK TEST ERROR ==========" );
151-
error_log( "❌ ERROR: " . $e->getMessage() );
152-
error_log( "Stack trace: " . $e->getTraceAsString() );
153-
error_log( "========================================\n" );
153+
if ( defined( 'WP_DEBUG' ) && WP_DEBUG ) {
154+
error_log( "\n========== WEBHOOK TEST ERROR ==========" );
155+
error_log( "❌ ERROR: " . $e->getMessage() );
156+
// Do not log stack trace as it may contain sensitive information
157+
error_log( "========================================\n" );
158+
}
154159

155160
return new WP_Error(
156161
'webhook_test_failed',

‎plugins/wp-graphql-webhooks/src/Services/PluginServiceLocator.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ public function has(string $name): bool {
2626

2727
public function get(string $name) {
2828
if (!isset($this->factories[$name])) {
29-
throw new UnexpectedValueException("Service not found: {$name}");
29+
throw new UnexpectedValueException( esc_html( "Service not found: {$name}" ) );
3030
}
3131

3232
if (!isset($this->instances[$name])) {

0 commit comments

Comments
 (0)