fix(express): signPayload API to handle stringified payload as req #7024
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
parvaparikh-30
force-pushed
the
GNA-2162
branch
from
760e857 to
eff0aae
Compare
| }); | ||
| it('should return a signed payload with type as json string', async function () { | ||
| // TODO(GO-1015): unskip test | ||
| return; |
There was a problem hiding this comment.
https://bitgoinc.atlassian.net/browse/GO-1015
Its causing some problem in CI, it was already defined so kept it as it is
But ran locally, which ran perfectly, you can find it in description
There was a problem hiding this comment.
Its causing some problem in CI,
But ran locally, which ran perfectly,
This is not an acceptable approach. Please fix the failing test.
There was a problem hiding this comment.
If the test failure is consistent, that points to this PR as the cause.
There was a problem hiding this comment.
Thanks Zahin for the feedback, it was from different team so kept the same
But I will look into the issue & rectify it
parvaparikh-30
force-pushed
the
GNA-2162
branch
from
eff0aae to
3f047b5
Compare
| @@ -630,7 +639,7 @@ export async function handleV2OFCSignPayload(req: express.Request): Promise<{ pa | |||
|
|
|||
| const walletPassphrase = bodyWalletPassphrase || getWalletPwFromEnv(wallet.id()); | |||
| const tradingAccount = wallet.toTradingAccount(); | |||
| const stringifiedPayload = JSON.stringify(req.body.payload); | |||
| const stringifiedPayload = isJsonString(req.body.payload) ? req.body.payload : JSON.stringify(req.body.payload); | |||
There was a problem hiding this comment.
As a follow up, please consider migrating this api to an api-ts typed route! This way you can define your input codec and have your input validation done automatically!
There was a problem hiding this comment.
io-ts and io-ts-types already have codecs for json and json strings.
| }); | ||
| it('should return a signed payload with type as json string', async function () { | ||
| // TODO(GO-1015): unskip test | ||
| return; |
There was a problem hiding this comment.
Its causing some problem in CI,
But ran locally, which ran perfectly,
This is not an acceptable approach. Please fix the failing test.
| }); | ||
| it('should return a signed payload with type as json string', async function () { | ||
| // TODO(GO-1015): unskip test | ||
| return; |
There was a problem hiding this comment.
If the test failure is consistent, that points to this PR as the cause.
modules/express/src/clientRoutes.ts
Outdated
| @@ -589,6 +589,15 @@ export async function handleV2OFCSignPayloadInExtSigningMode( | |||
| } | |||
| } | |||
|
|
|||
| const isJsonString = (str: any): boolean => { | |||
There was a problem hiding this comment.
Can we move one-off functions like this out to a util file? clientRoutes is huge as it is already 😭
parvaparikh-30
force-pushed
the
GNA-2162
branch
from
3f047b5 to
2a259a8
Compare
parvaparikh-30
force-pushed
the
GNA-2162
branch
from
2a259a8 to
6bc558c
Compare
One of the client was using Bitgo Express to sign DvP settlements
This API Helps them to retrieve payload in json string
In docs, its mentioned that using /api/v2/ofc/signPayload users would be able to create signature & then sign a settlement
While debugging found that, the already stringified payload (body) gets stringified again(backend) which mess up the signature & thus client is getting error - Signature Verification Failed
More context - slack thread
Fix:
Validate if the payload in req is already stringified or not, if yes then ignore else stringify it
Added test cases to confirm its working
Logic -
Test Cases Verfifcation -
Ticket: GNA-2162