Skip to content

Commit 025cb77

Browse files
DinamikoDinamiko
authored
Merge pull request #4534 from woocommerce/dev/PCP-6446-checkout-order-approved-webhook-fails-with-uncaught-pay-pal-order-missing-exception-to-http-500-loop-pending-orders-apple-pay-never-captured-user-report
fix(webhooks): catch uncaught exceptions in incoming webhook handler (6446)
2 parents 662fe5f + a9b23b9 commit 025cb77

2 files changed

Lines changed: 164 additions & 0 deletions

File tree

‎modules/ppcp-webhooks/src/IncomingWebhookEndpoint.php‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,37 @@ public function verify_request( \WP_REST_Request $request ): bool {
206206
* @return \WP_REST_Response
207207
*/
208208
public function handle_request( \WP_REST_Request $request ): \WP_REST_Response {
209+
try {
210+
return $this->process_request( $request );
211+
} catch ( \Throwable $exception ) {
212+
/**
213+
* Catches any exception a handler's business logic might throw
214+
* (not just the types it documents), so an unexpected error
215+
* returns a graceful failure response instead of an uncaught
216+
* fatal. Without this, PayPal would retry the webhook
217+
* indefinitely since it never receives a response.
218+
*/
219+
$this->logger->error(
220+
sprintf(
221+
'Uncaught %s while handling incoming webhook: %s',
222+
get_class( $exception ),
223+
$exception->getMessage()
224+
)
225+
);
226+
227+
return $this->failure_response();
228+
}
229+
}
230+
231+
/**
232+
* Processes the request. Split out from handle_request() so the latter
233+
* can wrap it in a single catch-all try/catch.
234+
*
235+
* @param \WP_REST_Request $request The request.
236+
*
237+
* @return \WP_REST_Response
238+
*/
239+
private function process_request( \WP_REST_Request $request ): \WP_REST_Response {
209240
$event = $this->event_from_request( $request );
210241

211242
$this->logger->debug(

‎tests/PHPUnit/Webhooks/IncomingWebhookEndpointTest.php‎

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace WooCommerce\PayPalCommerce\Webhooks;
6+
7+
use Mockery;
8+
use Psr\Log\LoggerInterface;
9+
use WooCommerce\PayPalCommerce\ApiClient\Endpoint\WebhookEndpoint;
10+
use WooCommerce\PayPalCommerce\ApiClient\Entity\WebhookEvent;
11+
use WooCommerce\PayPalCommerce\ApiClient\Factory\WebhookEventFactory;
12+
use WooCommerce\PayPalCommerce\TestCase;
13+
use WooCommerce\PayPalCommerce\WcGateway\Exception\PayPalOrderMissingException;
14+
use WooCommerce\PayPalCommerce\Webhooks\Handler\RequestHandler;
15+
use WooCommerce\PayPalCommerce\Webhooks\Status\WebhookSimulation;
16+
use WP_REST_Request;
17+
use WP_REST_Response;
18+
19+
/**
20+
* @covers \WooCommerce\PayPalCommerce\Webhooks\IncomingWebhookEndpoint
21+
*/
22+
class IncomingWebhookEndpointTest extends TestCase
23+
{
24+
/** @var WebhookEventFactory&Mockery\MockInterface */
25+
private $webhook_event_factory;
26+
27+
/** @var WebhookSimulation&Mockery\MockInterface */
28+
private $simulation;
29+
30+
/** @var WebhookEventStorage&Mockery\MockInterface */
31+
private $last_webhook_event_storage;
32+
33+
/** @var LoggerInterface&Mockery\MockInterface */
34+
private $logger;
35+
36+
public function setUp(): void
37+
{
38+
parent::setUp();
39+
40+
$this->webhook_event_factory = Mockery::mock(WebhookEventFactory::class);
41+
$this->simulation = Mockery::mock(WebhookSimulation::class);
42+
$this->last_webhook_event_storage = Mockery::mock(WebhookEventStorage::class);
43+
$this->logger = Mockery::mock(LoggerInterface::class);
44+
45+
$this->logger->shouldReceive('debug');
46+
$this->logger->shouldReceive('info');
47+
48+
$this->last_webhook_event_storage->shouldReceive('save');
49+
$this->simulation->shouldReceive('is_simulation_event')->andReturn(false);
50+
}
51+
52+
private function createRequest(): WP_REST_Request
53+
{
54+
/** @var WP_REST_Request&Mockery\MockInterface $request */
55+
$request = Mockery::mock('WP_REST_Request, ArrayAccess');
56+
$request->allows('get_params')->andReturn([]);
57+
$request->allows('offsetExists')->andReturn(true);
58+
$request->allows('offsetGet')->with('event_type')->andReturn('CHECKOUT.ORDER.APPROVED');
59+
60+
return $request;
61+
}
62+
63+
private function createEndpoint(RequestHandler ...$handlers): IncomingWebhookEndpoint
64+
{
65+
$webhook_endpoint = Mockery::mock(WebhookEndpoint::class);
66+
67+
return new IncomingWebhookEndpoint(
68+
$webhook_endpoint,
69+
null,
70+
$this->logger,
71+
false,
72+
$this->webhook_event_factory,
73+
$this->simulation,
74+
$this->last_webhook_event_storage,
75+
...$handlers
76+
);
77+
}
78+
79+
/**
80+
* @scenario When a handler's handle_request() throws an exception that is not a
81+
* RuntimeException (e.g. PayPalOrderMissingException, which extends the plain
82+
* \Exception class), the exception must not propagate uncaught. Previously this
83+
* caused an uncaught fatal (translating to an HTTP 500 with no framework-level
84+
* catch), which made PayPal retry the webhook indefinitely.
85+
*/
86+
public function test_unexpected_exception_from_handler_does_not_propagate_uncaught(): void
87+
{
88+
$event = new WebhookEvent('evt-1', null, 'checkout-order', '1.0', 'CHECKOUT.ORDER.APPROVED', '', '', (object) []);
89+
$this->webhook_event_factory->shouldReceive('from_array')->andReturn($event);
90+
91+
$handler = Mockery::mock(RequestHandler::class);
92+
$handler->shouldReceive('responsible_for_request')->andReturn(true);
93+
$handler->shouldReceive('event_types')->andReturn(['CHECKOUT.ORDER.APPROVED']);
94+
$handler->shouldReceive('handle_request')->andThrow(
95+
new PayPalOrderMissingException('There was an error processing your order.')
96+
);
97+
98+
$this->logger->shouldReceive('error')->once();
99+
100+
$sut = $this->createEndpoint($handler);
101+
102+
$response = $sut->handle_request($this->createRequest());
103+
104+
$this->assertInstanceOf(WP_REST_Response::class, $response);
105+
$this->assertSame(200, $response->get_status());
106+
$this->assertFalse($response->get_data()['success']);
107+
}
108+
109+
/**
110+
* @scenario The happy path (handler returns a response normally) must keep working
111+
* unchanged after wrapping handle_request() in a try/catch.
112+
*/
113+
public function test_handler_response_is_returned_unchanged_on_success(): void
114+
{
115+
$event = new WebhookEvent('evt-1', null, 'checkout-order', '1.0', 'CHECKOUT.ORDER.APPROVED', '', '', (object) []);
116+
$this->webhook_event_factory->shouldReceive('from_array')->andReturn($event);
117+
118+
$expected_response = new WP_REST_Response(['success' => true]);
119+
120+
$handler = Mockery::mock(RequestHandler::class);
121+
$handler->shouldReceive('responsible_for_request')->andReturn(true);
122+
$handler->shouldReceive('event_types')->andReturn(['CHECKOUT.ORDER.APPROVED']);
123+
$handler->shouldReceive('handle_request')->andReturn($expected_response);
124+
125+
$this->logger->shouldNotReceive('error');
126+
127+
$sut = $this->createEndpoint($handler);
128+
129+
$response = $sut->handle_request($this->createRequest());
130+
131+
$this->assertSame($expected_response, $response);
132+
}
133+
}

0 commit comments

Comments
 (0)