cblgh / cerca

lean forum software
Other
127 stars 18 forks source link

"Prod readiness" -> anti spam and input validation #59

Open alexwennerberg opened 9 months ago

alexwennerberg commented 9 months ago

There's a number of features I'm looking to make cerca more "production ready", ie, resilient to a few classes of attacks:

  1. Size limit on every form input
  2. Rate limit on every POST route

This may be a somewhat substantial PR, so I wanted to touch base before writing thi

alexwennerberg commented 9 months ago

It appears that the current rate limiting ware will hang a connection (for up to 15 minutes) instead of dropping it and e.g. returning a 429. Is this intended? https://github.com/alexwennerberg/cerca/blob/d5ab0387c5c5b2282d3eaa4155e0b9344bfa8215/server/server.go#L137

I think what I'm looking for is to limit a number of routes (with more aggressive parameters than the existing route) and return a 429

I may put together a demo PR soon

alexwennerberg commented 9 months ago

WIP PR https://github.com/alexwennerberg/cerca/tree/prod-ready

cblgh commented 9 months ago

thanks for touching base btw! i read the issues when i see them, but since it takes a bit of effort to respond&review it's been this long until i had the extra energy for that :]

It appears that the current rate limiting ware will hang a connection (for up to 15 minutes) instead of dropping it and e.g. returning a 429. Is this intended?

yeah but idk why X) i mainly wanted to prevent overzelous rss clients from being naughty consumers of the rss feed

specifics:

other than that, looks good! did this arise in conjunction with wanting to explore open signups for the fish?

ps don't remove the rss rate-limiter pls

alexwennerberg commented 9 months ago

other than that, looks good! did this arise in conjunction with wanting to explore open signups for the fish?

Yeah, but I think with the approval based signups it's less of a problem. I just don't want to make it that trivial for a bad user to spam the site with garbage. Or even a regular user just messing around or making a mistake

ps don't remove the rss rate-limiter pls

Oh yea I didn't mean to, was just rushing out an MVP for a public launch, I'll add it back in. I am thinking there should be some way to set different rate limits per endpoint, but I haven't looked into it much.

Will respond to the rest later!

cblgh commented 9 months ago

I am thinking there should be some way to set different rate limits per endpoint, but I haven't looked into it much.

i was thinking limiter could have its constructor further parameterized, in addition to just the routes, and then instantiate multiple ratelimiting wares and give them useful names. but as it is i think the rss bits qualify under the slightly changed params so extra fancy schmancy stuff isn't needed yet

alexwennerberg commented 9 months ago

Added the other two changes on my fork! I had a question on this one

make sure the post author does not lose information when posting and being denied due to the validation Do you mean making sure the form is not wiped? In my experience, that is generally handled client-side? Or did you mean something else

cblgh commented 9 months ago

Do you mean making sure the form is not wiped? In my experience, that is generally handled client-side? Or did you mean something else

my experience is the opposite ^^ if seeking to validate data it's done at the server because any client-side control can be surpassed by e.g. a simple curl command.

for cerca and concerning max lengths: i think it's fine if you want to add maximum length attributes to e.g. the title input and textbox elements (e.g. like the min password length is encoded now). in that case it's more like codifying community conventions rather than maintaining strictness and rigor in data validation