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

[bug] SiWe as safe app does not call the authenticate function #1119

Open
1 task done
malteish opened this issue Mar 27, 2023 · 10 comments
Open
1 task done

[bug] SiWe as safe app does not call the authenticate function #1119

malteish opened this issue Mar 27, 2023 · 10 comments

Comments

@malteish
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

RainbowKit Version

0.12.4

wagmi Version

0.12.4

Current Behavior

What works: Connecting (formerly gnosis) safe with WalletConnect

On connect modal select WalletConnect -> Official Modal -> copy link -> paste into app.safe.global WalletConnect app -> connection established -> sign login message -> approve in app.safe.global -> custom logic in pages/api/auth/[...nextauth].ts verifies the multisig has approved this message hash on-chain

What doesn't work: The same workflow as safe app

Rainbowkit support usage as (custom) safe app since version 0.12.4, so this is still brand new. We are excited about it, because we had been working on this very functionality for our dapp!
But when I try the workflow above as safe app, I get stuck with an open modal claiming to be "Verifying signature ...", without ever calling the backend.
grafik
So the non-working flow is:
Open app.safe.global -> open custom app -> paste dapp url -> accept disclaimers etc -> connect wallet (very smooth and nice!) -> sign message -> approve in safe -> get stuck at "Verifying signature ...", backend is never called.

Expected Behavior

Sign in workflow with safe app calls backend with 0x as signature, just as sign in workflow with WalletConnect does.

Steps To Reproduce

Not tested: enable an app to become a safe app by adding a manifest.json and configuring cors to allow loading of the same. Try signing the login message with WalletConnect and as safe app. Both should fail (unless the backend was modified to verify EIP1271), but only WalletConnect does fail whereas safe app gets stuck without ever proceeding to check the signature in the first place.

Link to Minimal Reproducible Example (CodeSandbox, StackBlitz, etc.)

No response

Anything else?

No response

@DanielSinclair
Copy link
Contributor

DanielSinclair commented Mar 28, 2023

@malteish Good find. I'm able to reproduce the behavior you're seeing. I believe the issue is related to calling Wagmi's signMessage hook in the Safe environment, with RainbowKit awaiting a response before it proceeds to the next step. I'll continue trying to debug this; if you have any additional findings, please do share!

I've also started an initial look at adopting EIP-1271, but am receiving errors that 1271 is unsupported in the Safe browser upon attempts to enable with a safe_setSettings RPC call. Would definitely like to adopt gasless signatures for the SIWE flow.

@malteish
Copy link
Author

Thanks for looking into this, @DanielSinclair. We are very interested in gasless signatures, too, and happy to help exploring how it can be done.
Fixing the on-chain signature should be much easier though, so I recommend doing that first. And I agree that it sounds like the return value of signMessage is not forwarded properly - possibly because it contains "0x" as signature (which is expected).

@malteish
Copy link
Author

malteish commented Mar 28, 2023

After some digging I have to challenge our assumption that the bug starts with signMessageAsync.
As the screenshot in the issue shows, I get stuck at "Verifying signature", which is only shown after the signature has been received.
This suggests the problem starts in authAdapter.verify(). Which is weird because I log any call to authorize in [...nextauth].ts and don't see that it is executed. The glue between these 2 places, according to my current understanding, is rainbowkit-siwe-next-auth.

@DanielSinclair what do you make of this?

@DanielSinclair
Copy link
Contributor

@malteish Yes, you're right. Was able to isolate the issue to next-auth session handling. const { status } = useSession() is not updating to authenticated in the Safe environment. Still hunting down the exact reason, but it might be related to the iframe. It looks like the RainbowKit side is working as expected, so a custom authentication adapter that doesn't rely on next-auth sessions should continue to work.

@malteish
Copy link
Author

malteish commented Mar 29, 2023

So we found out that CSRF and the nonce in the SiWe message were equal when using Metamask or WalletConnect, but different when using safe app. This is because next-auth enforces sameSite by default (which is a good choice!).

After updating cookie policy for testing, signing messages and authenticating them in the backend works. Added this to [...nextauth].ts:

cookies: {
      // WARNING: This disables the default cross-site protection for cookies!
      // These options were added to explore use of our app in a safe iframe.
      // https://www.npmjs.com/package/cookies-next
      // https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-03#section-4.1.2.7
      csrfToken: {
        name: `__Host-next-auth.csrf-token`,
        options: {
          httpOnly: true,
          sameSite: 'none', //'lax',
          path: '/',
          secure: true,
        },
      },
      sessionToken: {
        name: `__Secure-next-auth.session-token`,
        options: {
          httpOnly: true,
          sameSite: 'none',
          path: '/',
          secure: true,
        },
      },
    },

** WARNING: THESE SETTINGS ARE A VERY BAD CHOICE FOR PRODUCTION** in my opinion. Maybe someone with deeper knowledge in this area of expertise can chime in to either prove me wrong or suggest a better configuration.

Background:
nextauthjs/next-auth#7051
nextauthjs/next-auth#7077
https://next-auth.js.org/configuration/options#usesecurecookies

So this is not really a problem caused by Rainbowkit. Do you want to close the issue? I would like to keep it open a bit longer in hopes of someone finding a better solution.

@DanielSinclair
Copy link
Contributor

@malteish Will keep this issue open to see if a better solution is presented. I'll also relay to the Safe team to see if there is guidance on how to handle sessions/cookies in the Safe browser environment.

@nfts2me
Copy link

nfts2me commented Apr 25, 2023

We're also interested on this. We wanted to add support not only for Safe, but also for AA in general.

Do you know of other session manager (e.g. iron-session) that is able to properly handle sessions within an iframe?

@DanielSinclair , did the Safe team gave a proper answer? It may be something recurrent for them for the way they work with apps as iframes.

Regarding the signature, if been unable to make it work with Safe accounts using Wallet Connect. Instead of using the offchain signature with safe_setSettings, I was trying using the online EIP-1271 signature validation with no luck. But I've been able to make it work with other EIP-1271 compatible smart contracts accounts.

Why is that?
I believe that is due to the way Safe works. It has to create an online TX to get the signature validation into the Safe account. However, the Safe returns the control to "us" before the TX is completed on the blockchain.
This (I believe) is the flow:

  • Safe user connect to Rainbow app using Wallet Connect.
  • Rainbow app asks to sign to Safe user.
  • Safe app gets control.
  • Safe app asks the user(s) to sign.
  • User sign.
  • Safe app asks the user to send the TX into the blockchain to save the validation.
    *- Once the Tx is sent, control is sent back to Rainbow app via Wallet Connect.
    *- Rainbow app checks the signature, but it is not on-chain yet, as it is still on the mempool.

The two last steps are the possible ways to fix it.

@DanielSinclair , if you are talking to Safe team, ask them about this as well. You can create a Safe on testnet and test it yourself. If you need, using our dApp you can see a realworld example where is fails to sign. But other smart contract wallets can sign with no issues.

@pauliusuza
Copy link

@DanielSinclair curious if there was any feedback from the Safe on how to handle this correctly or a better workaround?

@nfts2me
Copy link

nfts2me commented Mar 7, 2024

Any news with regard to this issue?

@magiziz
Copy link
Contributor

magiziz commented Mar 11, 2024

@nfts2me Our team is looking at this issue. We'll report back once we have a solution 🙏

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

No branches or pull requests

5 participants