fix: safe auth #1651
Merged
fix: safe auth #1651
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jpangelle
commented
Oct 20, 2023
Comment on lines
228
to
255
| let body | ||
|
|
||
| if (signedMessage === '0x') { | ||
| const messageHash = hashMessage(message) | ||
|
|
||
| const isValid = await isValidSignature(signer, address, messageHash, evmChainId || 1) | ||
|
|
||
| if (isValid) { | ||
| body = JSON.stringify({ | ||
| safeAddress: address, | ||
| messageHash, | ||
| evmChainId, | ||
| }) | ||
| } else { | ||
| throw new Error('Invalid signature') | ||
| } | ||
| } else { | ||
| body = JSON.stringify({ | ||
| message, | ||
| signature: signedMessage, | ||
| address, | ||
| nonce, | ||
| network: isEvmOnSubstrate ? 'evmOnSubstrate' : 'evm', | ||
| chainId: evmChainId || 1, | ||
| }), | ||
| }) | ||
| } |
There was a problem hiding this comment.
The contents of the body are conditional based on if the user is using a Safe wallet or a vanilla wallet.
|
PR deployed in Google Cloud |
There was a problem hiding this comment.
Very nice job!!
Since we're not using siwe directly it seems we may open up a vulnerability for replay attacks - we might want to consider implementing a similar strategy as we used for substrate auth where:
- frontend calls /nonce where nonce is returned and set as server-only cookie
- frontend calls /authenticateWallet and passes the returned nonce in request body
- backend checks for presence of server-only cookie and confirms that the nonce in the cookie is the same as the once passed in the api request
jpangelle
force-pushed
the
fix-safe-auth
branch
from
c895b9e to
49b28c7
Compare
jpangelle
force-pushed
the
fix-safe-auth
branch
from
443c7d6 to
d601bc5
Compare
jpangelle
force-pushed
the
fix-safe-auth
branch
from
d601bc5 to
ee8ec50
Compare
hieronx
approved these changes
Nov 1, 2023
hieronx
reviewed
Nov 1, 2023
| @@ -249,3 +274,50 @@ Issued At: ${new Date().toISOString()}` | |||
| ) | |||
| } | |||
| } | |||
|
|
|||
| const isValidSignature = async (provider: any, safeAddress: string, messageHash: string, evmChainId: number) => { | |||
There was a problem hiding this comment.
Would be nice if this was abstracted into a helper that could be re-used between the onboarding API and the app at some point. Definitely a non-blocking improvement though!
gpmayorga
pushed a commit
that referenced
this pull request
Nov 23, 2023
hieronx
added a commit
that referenced
this pull request
Dec 5, 2023
* abstract deployments/triggers to separate files * remove triggers and some code cleanup * Release centrifuge-react and Fabric (#1600) * Centrifuge App: Fix connecting single account (#1620) * feat: implement oracle-v2 (#1558) * Centrifuge App: Remove accrued interest from portfolio card (#1625) * Moonbeam to demo (#1627) * CentrifugeJS: Fix transactions (#1622) * Centrifuge App: Tinlake assets (#1631) * Centrifuge App: Multicall for Tinlake assets (#1634) * Centrifuge App: Get all proxies (#1637) * Create LICENSE (#1646) * Update support email (#1645) * Centrifuge App: Pending write-off and epoch changes (#1628) * Show anamoy pool after chain upgrade (#1648) * Centrifuge App: Portfolio asset allocation (#1641) * Centrifuge App: Fix asset class (#1650) * Centrifuge App: Fix asset detail when subquery is down (#1653) * Centrifuge App: Fix asset class in pool table (#1652) * Fix connection guard for evm wallets on cent pool (#1661) * swap to goldsky subgraph (#1662) * feat: composition table (#1623) * Centrifuge App: Table redesign (#1663) * Centrifuge App: Code splitting and cleaning up unused files (#1660) * fix: fix trx bug (#1667) * Centrifuge App: Prime overview page (#1659) * CentrifugeApp: Portfolio transaction table (#1605) * Centrifuge App: Liquidity pools investments (#1542) * Centrifuge App: Fix investor transaction report (#1675) * fix: safe auth (#1651) * Centrifuge App: Fixes (#1673) * fix: use new window (#1676) * fix: use new window * use query param * fix data sharing agreement text * genericize * rename * Centrifuge App: Fix getting loan NFT data (#1685) * Centrifuge App: Fix formatting for investor transactions (#1686) * Centrifuge App: Prime detail (#1684) * CentrifugeJS: Fix query for domain routers (#1692) * Fix closed asset handling (#1693) * Centrifuge App: Fix env var (#1694) * fix: portfolio column spacing (#1698) * Centrifuge App: Restricted transfers (#1697) * remove moonbean alpha * debug commenting on PR * update demo endpoints --------- Co-authored-by: Onno Visser <[email protected]> Co-authored-by: JP <[email protected]> Co-authored-by: Jeroen <[email protected]> Co-authored-by: Sophia <[email protected]> Co-authored-by: Adam Stox <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This pull request fixes an issue when authenticating on the
/onboardingpage with a Safe wallet. Since Safe wallets are smart contracts, they can not generate ECDSA signatures. Therefore, Safe uses EIP-1271 for message signing and validating signatures. I mostly followed this guide from Safe's documentation in order to implement the signature validation for when a user is using Safe. However, since we generate jwt's on the server side, I needed to do the check on both the frontend/backend.Closes #1649
Approvals