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

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

Open
alexwennerberg opened this issue Feb 16, 2024 · 7 comments
Open

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

alexwennerberg opened this issue Feb 16, 2024 · 7 comments

Comments

@alexwennerberg
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@cblgh
Copy link
Owner

cblgh commented Feb 21, 2024

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:

  • set maxThreadTitle = 100 is more suitable (250 is longlong)
  • not a fan of enforcing lowercase usernames, specifically, in cerca. this seems like an aesthetic choice on fishbb's behalf that doesn't need to be upstreamed
  • make sure the post author does not lose information when posting and being denied due to the validation

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
Copy link
Contributor Author

alexwennerberg commented Feb 21, 2024

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
Copy link
Owner

cblgh commented Feb 21, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

cblgh commented Feb 23, 2024

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

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

2 participants