Enhance rate limit handling, add email config param #483

sbocinec · 2020-03-29T21:54:06Z

Changes

Add rate limit count as a config parameter: RATE_LIMIT_COUNT
Add rate limit window length as a config parameter: RATE_LIMIT_WINDOW
Add email as a config parameter: EMAIL
Add limit route & page view
Add contact partial
Update 404 & 500 views to use the contact partial
Update SK translations

Details

Rate limit enhancements

The original express-rate-limit code returns default generic message on exceeding the configured rate limit: Too many requests, please try again later. (English only). What's worse, administrators don't have any clue this is happening.

Users often share their IPv4 IP address as operators are using IPv6 and provide a shared IPv4 IP for hundreds of users. I've already experienced this on the SK deployment when users were affected unable to submit their reports until one user notified me having no clue what's going on.

This PR tries to address this situation and brings new /limit route, that can provide more localized info for users and also give us (admins) information, the rate limit has been reached (seeing /limit route in the analytics).

Email as a config param

Twitter handler was the only configurable contact available on the site. There are countries where twitter is not very popular or used, or some people don't have twitter account unable to contact us.

michaelmcmillan · 2020-03-29T22:05:16Z

app/routes/report-routes.ts

@@ -95,9 +96,12 @@ const extractTestResult = (req: Request): TestResult | undefined => {
 };

 const createReportRateLimit = rateLimit({
- max: 20, // allowed requests per window
+ max: config.RATE_LIMIT, // allowed requests per window
 windowMs: 24 * 60 * 60 * 1000, // 24 hour window,


Maybe we should add windowMs to the config as well? Makes sense as RATE_LIMIT option by itself doesn't really indicate how strict it is.

@michaelmcmillan perfect idea! I've added it in the latest commit, now we have:
RATE_LIMIT_COUNT and RATE_LIMIT_WINDOW new params - feel free to suggest better names :-)

I might later add config params README as the number of params starts to grow 😅

fossecode · 2020-03-30T20:17:52Z

app/routes/report-routes.ts

+ windowMs: config.RATE_LIMIT_WINDOW, // window length in miliseconds
+ keyGenerator: req => determineRemoteAddress(req),
+ onLimitReached(req, res) {
+ return res.redirect(`${res.locals.urls.limit}`);


Nice, you don't really need the template literals here, return res.redirect(res.locals.urls.limit); will do

nice catch, thank you, fixing

fossecode · 2020-03-30T20:18:35Z

app/routes/various-routes.ts

@@ -22,6 +22,10 @@ router.get(`${urls.contributors}`, (req, res) => {
 return res.render('pages/contributors');
 });

+router.get(`${urls.limit}`, (req, res) => {


Suggested change

router.get(`${urls.limit}`, (req, res) => {

router.get(urls.limit, (req, res) => {

@fossecode i see also all other router.get above are using the same notation, why to change it specifically for this route?

Probably just a copy/paste error. Would have made sense to use template literals if we were to have a variable in the route, e.g.
router.get(`/${countryCode}/privacy-statement`, (req, res) => {

I guess it has been copied from
router.get(`${urls.profile}/:passcode`, async (req, res) => {

ah, okay, thanks for explanation :-) I can change & fix all

Fix commited

fossecode · 2020-03-30T20:36:00Z

app/config.ts

@@ -26,6 +26,7 @@ const fallbackConfig: Config = {
 MAP_CENTER: process.env.MAP_CENTER || '10.7522, 63.9139',
 MAP_ZOOM: parseInt(process.env.MAP_ZOOM || '4', 10),
 MAP_MAX_ZOOM: parseInt(process.env.MAP_MAX_ZOOM || '13', 10),
+ EMAIL: process.env.EMAIL,


I don't know about the rest of you, but I feel we have been a bit inconsistent on what we put in the config vs what we put in the translations. We discussed this at one point, and I think we decided to put URLs, emails etc in the translation file. I also believe that is easier to maintain than the config (although both is hard 😓). In my opinion, we could at least move EMAIL, TWITTER (and possibly ZIP_PLACEHOLDER) to the translations. If we e.g. use __(`[email protected]`) when rendering the email, it would automatically fallback to [email protected] if it is not overriden. Thoughts on this?

I would support the idea, but- AFAIK minority of the domains have email system configured for the domain. That's why I made it optional and only enable when explicitely enabled in the config. In this case I don't think it's good to move it to the translation files as most of those will use [email protected] what can be a bit misleading.

I tried to create the contact partial dynamic and only display email when it's set.

Yeah I get the point. But do you think that it is better to don't display a contact email at all if it is not set? We have gotten a few questions and comments from different countries on our email, so it could be nice to have a fallback?

Ah, I didn't realize that, it makes sense to have the fallback email 👍
So, should I move it to the translations then?

Yeah or you could also do EMAIL: process.env.EMAIL || '[email protected]'. I think I would prefer translations, but you can use the config if it is easier :)

Okay. According to these info, I will use the default value here and:

Keep EMAIL in the config to have it next to the TWITTER

Move both EMAIL and TWITTER in another PR to the translations (I guess moving the TW would require much more effort due to the testing of all other parts where TW is actually used)

change commited

Not sure if it's worth to add the option to config.example.json yet, not to confuse people if it's gonna move out soon.

fossecode · 2020-03-30T21:14:43Z

Since this a page that is not part of the ordinary flow, I guess it is ok to merge it without getting translations for all languages first. In the rare case where people hit the rate limit, it should be ok that it is in english.

fossecode · 2020-03-31T10:39:59Z

Could you fix the conflicts, and then we'll merge!

fossecode · 2020-04-01T22:25:28Z

Is this one ready to be merged?

sbocinec · 2020-04-02T17:45:01Z

Is this one ready to be merged?

Going to rebase and will be ready soon :-)

fossecode · 2020-04-02T18:15:01Z

app/server.ts

@@ -82,6 +83,8 @@ app.use((req, res, next) => {
 res.locals.localeToFlag = localeToFlag;
 res.locals.currentLocale = req.getLocale();
 res.locals.formatNumber = createNumberFormatter(config.THOUSAND_SEPARATOR);
+ res.locals.rateLimitCount = config.RATE_LIMIT_COUNT;


This and the next line can be removed

thanks, removed!

fossecode mentioned this pull request Mar 29, 2020

Show error page to user if IP is rate limited #82

Closed

michaelmcmillan reviewed Mar 29, 2020

View reviewed changes

michaelmcmillan self-requested a review March 29, 2020 22:05

michaelmcmillan mentioned this pull request Mar 29, 2020

Sync missing translation keys in app/locales/*.json files to a Google Sheet #481

Open

michaelmcmillan requested a review from fossecode March 29, 2020 23:41

sbocinec force-pushed the rate-limit-tweaks branch 2 times, most recently from b7a8a14 to bf69942 Compare March 30, 2020 17:27

sbocinec changed the title ~~Enhance rate limit handling~~ Enhance rate limit handling, add optional email config param Mar 30, 2020

sbocinec marked this pull request as ready for review March 30, 2020 18:40

fossecode reviewed Mar 30, 2020

View reviewed changes

sbocinec changed the title ~~Enhance rate limit handling, add optional email config param~~ Enhance rate limit handling, add email config param Mar 30, 2020

fossecode approved these changes Mar 30, 2020

View reviewed changes

sbocinec added 9 commits April 2, 2020 19:48

Add report rate limit as a config parameter

140cfd3

Add rate limit route & page

1f59a2e

Add rate limit window as a config parameter

f7d2a08

Add email as opt. cfg param & add contacts partial

0a98b43

Fix style in the contact partial

fbd6ce4

Update SK translations

01f3ce8

Fix report route

c308e51

Fix various routes

d398ae2

Set default value for EMAIL

06378b6

sbocinec force-pushed the rate-limit-tweaks branch from 7c1c3b8 to c883f7f Compare April 2, 2020 18:09

Sort locales, add View Website Stats translation

2f074a6

sbocinec force-pushed the rate-limit-tweaks branch from c883f7f to 2f074a6 Compare April 2, 2020 18:12

fossecode reviewed Apr 2, 2020

View reviewed changes

sbocinec added 2 commits April 2, 2020 20:28

Remove unused locals

bc9dc26

Fix domain urls

c1a7c89

fossecode merged commit 3fee78d into BustByte:master Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance rate limit handling, add email config param #483

Enhance rate limit handling, add email config param #483

sbocinec commented Mar 29, 2020 •

edited

Loading

michaelmcmillan Mar 29, 2020

sbocinec Mar 30, 2020

fossecode Mar 30, 2020

sbocinec Mar 30, 2020

fossecode Mar 30, 2020

sbocinec Mar 30, 2020

fossecode Mar 30, 2020

fossecode Mar 30, 2020 •

edited

Loading

sbocinec Mar 30, 2020

sbocinec Mar 30, 2020

fossecode Mar 30, 2020

sbocinec Mar 30, 2020 •

edited

Loading

fossecode Mar 30, 2020

sbocinec Mar 30, 2020

fossecode Mar 30, 2020

sbocinec Mar 30, 2020

sbocinec Mar 30, 2020

sbocinec Mar 30, 2020

fossecode commented Mar 30, 2020

fossecode commented Mar 31, 2020

fossecode commented Apr 1, 2020

sbocinec commented Apr 2, 2020

fossecode Apr 2, 2020

sbocinec Apr 2, 2020

	router.get(`${urls.limit}`, (req, res) => {
	router.get(urls.limit, (req, res) => {

Enhance rate limit handling, add email config param #483

Enhance rate limit handling, add email config param #483

Conversation

sbocinec commented Mar 29, 2020 • edited Loading

Changes

Details

Rate limit enhancements

Email as a config param

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fossecode Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbocinec Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fossecode commented Mar 30, 2020

fossecode commented Mar 31, 2020

fossecode commented Apr 1, 2020

sbocinec commented Apr 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbocinec commented Mar 29, 2020 •

edited

Loading

fossecode Mar 30, 2020 •

edited

Loading

sbocinec Mar 30, 2020 •

edited

Loading