Skip to content
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

fix(payments-plugin): Stripe controller crashes server instance #2454

Merged
merged 4 commits into from
Oct 17, 2023

Conversation

carathorys
Copy link
Contributor

@carathorys carathorys commented Oct 16, 2023

This patch is slightly related to the issue 2450, but fixes only the crash side of it:

I've put the last response.status(HttpStatus.OK).send('Ok') to the transaction block, otherwise it crashes the backend with the following error:

...
[server] Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
[server]     at new NodeError (node:internal/errors:387:5)
[server]     at ServerResponse.setHeader (node:_http_outgoing:644:11)
[server]     at ServerResponse.header (/usr/src/app/node_modules/express/lib/response.js:794:10)
[server]     at ServerResponse.send (/usr/src/app/node_modules/express/lib/response.js:174:12)
[server]     at StripeController.webhook (/usr/src/app/node_modules/@vendure/payments-plugin/package/stripe/stripe.controller.js:95:49)
[server]     at processTicksAndRejections (node:internal/process/task_queues:96:5) 
[server] node:internal/errors:478
...

Please note that this does not solves the issue that the rawBody is not defined because the middleware is not executed, but at least the backend won't crash because of any unidentified issues.

@netlify
Copy link

netlify bot commented Oct 16, 2023

Deploy Preview for effervescent-donut-4977b2 canceled.

Name Link
🔨 Latest commit a7104aa
🔍 Latest deploy log https://app.netlify.com/sites/effervescent-donut-4977b2/deploys/652e53605a6a4b000802a76b

@carathorys
Copy link
Contributor Author

carathorys commented Oct 16, 2023

Ok, now I fixed the issue (hope it will work for everybody else as well)
The solution is strange (and straightforward in the meantime):
The body-parser needs a type parameter in order to execute, but if you're using raw(...) middleware, it will put the Buffer into the request.body object.
So if you use json(...) instead, and in the verify(...) method you put the rawBody into the request it seems like it will solve the issue.

import { json } from 'body-parser';
import * as http from 'http';

import { RequestWithRawBody } from './types';

/**
 * Middleware which adds the raw request body to the incoming message object. This is needed by
 * Stripe to properly verify webhook events.
 */
export const rawBodyMiddleware = json({
    type: '*/*',
    verify(req: RequestWithRawBody, res: http.ServerResponse, buf: Buffer, encoding: string) {
        if (Buffer.isBuffer(buf)) {
            req.rawBody = Buffer.from(buf);
        }
        return true;
    },
});

@carathorys
Copy link
Contributor Author

Ok, the previos solution doesn't work (thankfully, the E2E tests catched it in time!)
I've reverted the body-parser, and made a change in the StripeController.
Now I'm trying to write at least an e2e test to the webhook verification, but I've ran into some issues with the order service:
It is unable to execute the findOneByCode(...) function as it first executes a query to fetch an order by it's code, and then it tries to fetch the order by ID from the database.

@michaelbromley do you have any tips how I can solve that?
Currently the test I wrote is failing in the in the stripe.controller.ts:61 because of that issue.
Also noticed that this function loads an order twice:
First it fetches the order by it's code, with one relation (Customer), then if it founds the record, it fetches the order again, this time by id, and with the relations parameters.

@carathorys
Copy link
Contributor Author

Ok, now it seems to be working! :)
Added some tests to validate wether the Stripe signature validation works or not. In the order payload it only works if I replace the "T_" from the orderId, I hope it's not a problem!

@michaelbromley michaelbromley merged commit b0ece21 into vendure-ecommerce:master Oct 17, 2023
14 checks passed
@michaelbromley
Copy link
Member

Thank you for the considerable work you must have put in for this investigation & fix! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants