naturalcrit / homebrewery

Create authentic looking D&D homebrews using only markdown
https://homebrewery.naturalcrit.com
MIT License
1.07k stars 326 forks source link

Fix admin vulnerability to Brute Force #3492

Open 5e-Cleric opened 4 months ago

5e-Cleric commented 4 months ago

Added express-rate-limit package and implemented rate limiting for admin API login attempts

Have NOT tested this as of yet, feel free to do so. Should be a limit of 10 attempts per day.

5e-Cleric commented 4 months ago

It works!

image

G-Ambatte commented 3 months ago

Here's a suggested tweak for admin.api.js:

// Define rate limiter options
const loginLimiter = rateLimit({
    timeWindow : 24 * 60 * 60 * 1000, // 24 hours window
    max        : 10, // limit each IP to 10 requests per timeWindow
    handler    : ()=>{throw { HBErrorCode: '100', code: 429, message: 'Too many requests' }; }
});

which results in: image

I've just re-used HBErrorCode 100 on the proof of concept, we'd need to give this particular error its own code to use the Error Page. We might also want to do something similar for the Access Denied result.

G-Ambatte commented 3 months ago

On the actual error message, I think Too many failed login attempts, try again later is probably better than specifically stating that the ban is tied to their IP address.

5e-Cleric commented 3 months ago

I'd rebuild the UI for this, it is not a brew that is being accessed, but the admin page. But yeah i can work with that, good point.

5e-Cleric commented 3 months ago

image

This is better. I could use some advice on it anyway.

Gazook89 commented 3 weeks ago

There are some spelling errors and awkward sentences that I think could be improved. I'm no wordsmith (I use 10 words where will 1 will do), but here is my take:

## 401 ``` ## Authorization Required You need to provide correct administrator credentials to access this page. : If you should have access but are not able to log-in, please reach out via [reddit mod mail](https://old.reddit.com/message/compose/?to=/r/homebrewery) or in the [hb_issues channel of the Discord of Many Things server](https://discord.gg/domt). ```
## 403 ``` ## Access Denied You need to provide correct administrator credentials to access this page. *Too many unsuccessful attempts will lock you out for a period of time.* : If you should have access but are not able to log-in, please reach out via [reddit mod mail](https://old.reddit.com/message/compose/?to=/r/homebrewery) or in the [hb_issues channel of the Discord of Many Things server](https://discord.gg/domt). ```
## 470 ``` ## You have run out of attempts You have failed to provide correct credentials to access the page too many times. Please wait a bit before retrying again. : If you should have access but are not able to log-in, please reach out via [reddit mod mail](https://old.reddit.com/message/compose/?to=/r/homebrewery) or in the [hb_issues channel of the Discord of Many Things server](https://discord.gg/domt). ```
Gazook89 commented 3 weeks ago

Likely conversation about error codes has already happened-- my eyes usually gloss over when it comes up-- so feel free to wave this away:

Is 403 - Forbidden the correct error code to return if the credentials are bad? My reading from [MDN] is that 403 is for when a user of a system is logged in, but tries to access a resource they don't have permission to access. Like if a logged-in homebrewery user tried to navigate to the /admin/ pages and we didn't require an extra password, they just needed a special permission on their account which they didn't have. (The flip side of this is if the developers had a special permission on their normal homebrewery account that allowed them access to the admin pages, rather than them having different username/password just to access admin pages).

I think just returning the same 401 - Unauthorized error after authentication failed would be fine, in which case the 401 error would just be what I wrote for the 403 error in the previous comment (basically just telling them at the outset about the possibility of lockout).

5e-Cleric commented 1 week ago

Yes, this PR was not my top most priority, but it is more so now.

5e-Cleric commented 1 week ago

I am not being able to test this at all, my python bruteforce script is failing, and i am not knowledgeable enough to understand why. The PR should work.