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

Externalizing required frontend config to a dot env file #56

Merged

Conversation

claudioantonio
Copy link
Contributor

This PR contributes to #29

@claudioantonio
Copy link
Contributor Author

Hello @endersonmaia and @guidanoli , what do you think?
I will send a .env.local file for you to continue using the default projectId. 😉

@guidanoli
Copy link
Contributor

Great!

@endersonmaia
Copy link
Contributor

@claudioantonio I think it's better now, let's just keep a look at #51 to improve how we handle this sensitive information

@claudioantonio
Copy link
Contributor Author

claudioantonio commented Mar 16, 2024

@claudioantonio I think it's better now, let's just keep a look at #51 to improve how we handle this sensitive information

@endersonmaia (cc. @guidanoli ) I just checked that Cartesiscan uses public env vars for API keys on the frontend. Check here.
Same for the walletconnect projectId, check here.

It seems that the way to protect from malicious actors to get it and submit lots of calls to consume the api quota is by whitelisting the source of the call on the provider (Alchemy or Infura). See wagmi forum here.

I will talk with Bruno Menezes about it. 😉

@brunomenezes
Copy link

brunomenezes commented Mar 19, 2024

@claudioantonio I think it's better now, let's just keep a look at #51 to improve how we handle this sensitive information

@endersonmaia (cc. @guidanoli ) I just checked that Cartesiscan uses public env vars for API keys on the frontend. Check here. Same for the walletconnect projectId, check here.

It seems that the way to protect from malicious actors to get it and submit lots of calls to consume the api quota is by whitelisting the source of the call on the provider (Alchemy or Infura). See wagmi forum here.

I will talk with Bruno Menezes about it. 😉

@claudioantonio, I think that is an improved way to quickly cancel and replace keys when necessary if that is the "mitigation" accepted. We are also using that form in the Staking app here. Thanks for highlighting that. I'll see if make sense to move to an environment variable, but this one in my view is less important than the node-provider for money reasons but is good to be able to switch the value when needed quickly.

When I looked into the infura options, it gives a few possibilities like:

  • Request rate-limiting (May cause service unavailability)
  • User agents
  • Origins header (May work for requests coming from a browser, but if you are doing something on a node-js-like environment, you can easily modify this header to your liking and, in theory bypass this. Unless they are going really deep on checking CA and issuer to validate that the cross-domain request owns that origin domain, I bet it is simply checking the header).
  • JWT configurations (That would be the next step for security. In general terms, it would be to get this JWT (BE side) and somehow instruct the wallets (UI) to add the Authorization header on each request. I would need to research again because, if I am not mistaken, at the time 2021/2022, the wallet we were using was not extensible enough in that space.)

The allowlist, as mentioned in one of the comments, may cause a bit of extra work depending if you use a cloud provider like Vercel. When working on a PR branch, you have its deployed counterpart since it gets random domains assigned to it, and you would probably hit 401s. Not sure if that is the case for BugLess.

@claudioantonio
Copy link
Contributor Author

@claudioantonio, I think that is an improved way to quickly cancel and replace keys when necessary if that is the "mitigation" accepted. We are also using that form in the Staking app here. Thanks for highlighting that. I'll see if make sense to move to an environment variable, but this one in my view is less important than the node-provider for money reasons but is good to be able to switch the value when needed quickly.

When I looked into the infura options, it gives a few possibilities like:

  • Request rate-limiting (May cause service unavailability)
  • User agents
  • Origins header (May work for requests coming from a browser, but if you are doing something on a node-js-like environment, you can easily modify this header to your liking and, in theory bypass this. Unless they are going really deep on checking CA and issuer to validate that the cross-domain request owns that origin domain, I bet it is simply checking the header).
  • JWT configurations (That would be the next step for security. In general terms, it would be to get this JWT (BE side) and somehow instruct the wallets (UI) to add the Authorization header on each request. I would need to research again because, if I am not mistaken, at the time 2021/2022, the wallet we were using was not extensible enough in that space.)

The allowlist, as mentioned in one of the comments, may cause a bit of extra work depending if you use a cloud provider like Vercel. When working on a PR branch, you have its deployed counterpart since it gets random domains assigned to it, and you would probably hit 401s. Not sure if that is the case for BugLess.

Thanks a lot for your contribution @brunomenezes !
@endersonmaia and @guidanoli I will convert the PR into a ready to review one. 😉

@claudioantonio claudioantonio marked this pull request as ready for review March 19, 2024 16:14
@claudioantonio claudioantonio changed the title [WIP] Externalizing required frontend config to a dot env file Externalizing required frontend config to a dot env file Mar 19, 2024
@guidanoli
Copy link
Contributor

I believe this needs a rebase

@claudioantonio
Copy link
Contributor Author

I believe this needs a rebase

Done @guidanoli !

@guidanoli
Copy link
Contributor

@claudioantonio I think we should set a default value for the WalletConnect project ID environment variable, so that people can build the front-end without having ownership of the real value. If I run pnpm build without an .env.local file that sets this environment variable, I get the following compilation error:

Error: A WalletConnect projectId is required

From the Staking app example @brunomenezes gave, it sets the project ID to WC_PROJECT_ID by default. See here.

If I set this value in the .env file, the pnpm build succeeds. It's okay that I cannot use WalletConnect in a (local) developer environment. I can use Metamask to interact with the blockchain.

@claudioantonio
Copy link
Contributor Author

@claudioantonio I think we should set a default value for the WalletConnect project ID environment variable, so that people can build the front-end without having ownership of the real value. If I run pnpm build without an .env.local file that sets this environment variable, I get the following compilation error:

Error: A WalletConnect projectId is required

From the Staking app example @brunomenezes gave, it sets the project ID to WC_PROJECT_ID by default. See here.

If I set this value in the .env file, the pnpm build succeeds. It's okay that I cannot use WalletConnect in a (local) developer environment. I can use Metamask to interact with the blockchain.

Done @guidanoli ! 😉

@claudioantonio claudioantonio merged commit e8dec4e into crypto-bug-hunters:main Mar 27, 2024
@claudioantonio claudioantonio deleted the frontend-env-variables branch March 27, 2024 14:45
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.

4 participants