-
Notifications
You must be signed in to change notification settings - Fork 20.5k
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
cmd/faucet: use Twitter API instead of website #21850
Conversation
d691dff
to
3f58312
Compare
I'm trying to figure out if this is already deployed on e.g. https://goerli-faucet.dappnode.net/ , is it? Would be good to know, actual deployed production use beats any testing/review I can do on it... :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general, a couple of nitpicks only. And thanks a lot for fixing this!
Thanks for the review @holiman, I've applied your suggestions. I host https://faucet.goerli.mudit.blog/ and it has been using this patch for ~12 hours now. The attack I described above has stopped since the update. I do not use puppeth for managing the faucet though so that remains untested. |
b713ab8
to
159b7c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The DAppNode Goerli Faucet is now also patched with this PR. |
Wondering if we'd need to create a custom Twitter user for this? Just don't want to end up blocked for too intensive API usage. |
You'll need to create a new "Twitter app" to get a bearer token and that token will be tied to that particular app. Twitter rate limits on basis of per-app so even if you get spammed, Twitter will just block your app's API access for a while. If the spam is extensive, Twitter might completely ban your app but I don't see any reason why they'd ban the users' account. That being said, they banned Nick for months for no reason so idk.... I am currently using an app created from my main account but I might switch to a new account later just to be super safe. |
@q9f For this PR to work, you need to provide a twitter API token to the faucet. Without that, the node falls back to the old (exploitable) method. AFAICT, the DAppNode Goerli Faucet is still using the old method. If you use puppeth for deploying the faucet, it should ask you for the Twitter bearer token now. If you are deploying it directly, you need to pass the bearer token with the |
Well, might be an idea to have the faucet use e.g. https://pkg.go.dev/go.uber.org/ratelimit . I've been using it in the nodemonitor (forkmon.ethdevops.io), it's a really nifty little lib to provide some rate limiting. |
We'll have to make the API asynchronous if we go down this route. That will cause a slightly worse UX and more complex architecture. Given that we already have ReCaptcha protection, I doubt we'll need any more rate-limiting. There's no real benefit for an attacker to dos a faucet. I think adding queuing and API rate limiting to the faucet might be over-engineering. We can always visit it again if the need arises. |
I agree |
This PR adds support for using Twitter API to query the tweet and author details. There are two reasons behind this change: