feat(core): Rate limit forgot password endpoint

[RicardoE105]

Github issue / Community forum post (link here to close automatically):

[netroy]

Should we also update the frontend to handle 429s and show some other message instead of

Please contact your administrator (problem with your SMTP setup)

[csuermann]

Great PR! Just a few comments:

• What will the user experience look like when the rate-limit kicks in?
• We should consider deployment scenarios with more than 1 primary n8n instance and consider externalizing the rate-limiter's state to Redis in those cases
• Will rate-limiting based on IP addresses also work reliably if requests come in through proxy servers or load balancers? (Docs)

[netroy]

Sorry, clicked on the wrong button.

[RicardoE105]

Great PR! Just a few comments:

• What will the user experience look like when the rate-limit kicks in?
• We should consider deployment scenarios with more than 1 primary n8n instance and consider externalizing the rate-limiter's state to Redis in those cases
• Will rate-limiting based on IP addresses also work reliably if requests come in through proxy servers or load balancers? (Docs)

1. as it's right now, it would show "Please contact your administrator (problem with your SMTP setup)", but Adi suggested above to show a proper error.
2. Had a small chat with @netroy about this and this limiting would still work just that each instance would its individual count. @netroy any other thought here?
3. Ah very good question. Had not thought about this. The solution they present have you find out the numbers of trusted proxy manually?! Will have a sync with the cloud team to see how we can address this at least taking in consideration our infra. This might not be relevant to us if k8 deploys one LB per namespace

[netroy]

1. as it's right now, it would show "Please contact your administrator (problem with your SMTP setup)", but Adi suggested above to show a proper error.

We don't need a specific error in this case. We should update the frontend to show the "problem with SMTP" error only when there is an actual error with SMTP, and for everything else we could have a generic "Sorry, an error occurred" message.

2. Had a small chat with @netroy about this and this limiting would still work just that each instance would its individual count. @netroy any other thought here?

We could setup the rate-limit-redis package at some point if really needed to, but IMO, we can let each instance rate limit individually. Synchronizing the limits across instances isn't really necessary.

3. Ah very good question. Had not thought about this. The solution they present have you find out the numbers of trusted proxy manually?! Will have a sync with the cloud team to see how we can address this at least taking in consideration our infra. This might not be relevant to us if k8 deploys one LB per namespace

We should update the core app to accept a generic env variable (defaulting to 0) to be passed into express's trust proxy setting.
Let's not make this cloud specific. Cloud instance could simply define this additional env variable, and set it to 1.

[netroy]

LGTM.
But, should we also add the new config variable in schema

	proxy_hops: {
		format: Number,
		default: 0,
		env: 'N8N_PROXY_HOPS',
		doc: 'Number of reverse-proxies n8n is running behind',
	},

and update the code after this line to include

const proxyHops = config.getEnv('proxy_hops');
if (proxyHops > 0) this.app.set('trust proxy', proxyHops);

[netroy]

I suppose we should make windowMs and limit configurable. But, I also think that these defaults should be enough for almost everyone, and we could consider making these configurable if/when there is an actual demand for it.

Thanks for patching this so quickly 🙏🏽

[RicardoE105]

Thanks for all the help @netroy 🙏

[cypress]

3 flaky tests on run #2726 ↗︎

0

269

0

0

3

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 RicardoE105 🗃️ e2e/*
Project: n8n	Commit: edace34aaa
Status: Passed	Duration: 10:44 💡
Started: Nov 3, 2023 5:04 PM	Ended: Nov 3, 2023 5:15 PM

  6-code-node.cy.ts • 2 flaky tests

View Output Video

Test		Artifacts
Code node > Code editor > should execute the placeholder successfully in both modes		Output Screenshots Video
... > generate code button should have correct state & tooltips		Output Screenshots Video

  28-resource-mapper.cy.ts • 1 flaky test

View Output Video

Test		Artifacts
Resource Mapper > should correctly delete single field		Output Screenshots Video

Review all test suite changes for PR #7604 ↗︎

[github-actions]

✅ All Cypress E2E specs passed

[codecov]

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (0bd4e74) 33.87% compared to head (edace34) 32.03%.
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7604      +/-   ##
==========================================
- Coverage   33.87%   32.03%   -1.85%     
==========================================
  Files        3408     3410       +2     
  Lines      208643   208693      +50     
  Branches    22568    22656      +88     
==========================================
- Hits        70680    66845    -3835     
- Misses     136808   140683    +3875     
- Partials     1155     1165      +10     

Files	Coverage Δ
packages/cli/src/config/schema.ts	83.33% <ø> (ø)
...es/cli/src/controllers/passwordReset.controller.ts	75.00% <100.00%> (+0.43%)	⬆️
packages/cli/src/AbstractServer.ts	48.54% <50.00%> (+0.02%)	⬆️
...kages/editor-ui/src/views/ForgotMyPasswordView.vue	0.00% <0.00%> (-28.85%)	⬇️

... and 65 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[janober]

Got released with [email protected]