Skip to content

Commit

Permalink
Check for instance creation while handling a request (#438)
Browse files Browse the repository at this point in the history
I'm not sure this will cover every case, but it should catch some, and I don't believe false positives will be very likely.
  • Loading branch information
nfriedly committed Feb 20, 2024
1 parent c252ae3 commit be7fe9c
Show file tree
Hide file tree
Showing 5 changed files with 92 additions and 0 deletions.
1 change: 1 addition & 0 deletions docs/reference/configuration.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,7 @@ Supported validations are:
- [draftPolliHeaders](/reference/error-codes#wrn-erl-deprecated-draft-polli-headers)
- [onLimitReached](/reference/error-codes#wrn-erl-deprecated-on-limit-reached)
- [headersResetTime](/reference/error-codes#err-erl-headers-no-reset)
- [creationStack](/reference/error-codes#err-erl-created-in-request-handler)
- [validationsConfig](/reference/error-codes#err-erl-unknown-validation)

Defaults to `true`.
Expand Down
52 changes: 52 additions & 0 deletions docs/reference/error-codes.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,58 @@ const limiter = rateLimit({

Set `validate: {validationsConfig: false}` in the options to disable the check.

### `ERR_ERL_CREATED_IN_REQUEST_HANDLER`

Instances of express-rate-limit should be created before a request comes in, not
in response to one.

Incorrect example:

```js
import { rateLimit } from 'express-rate-limit'

app.use((req, res, next) => {
// This won't work!
rateLimit({
/*...*/
})
next()
})
```

Correct example:

```js
import { rateLimit } from 'express-rate-limit'

app.use(rateLimit({/*...*/});
```
To only apply the rate limit to some requests, use the
[`skip`](/reference/configuration#skip),
[`skipSuccessfulRequests`](/reference/configuration#skipsuccessfulrequests), or
[`skipFailedRequests`](/reference/configuration#skipfailedrequests) options.
Example where the rate limit only applies to users who are not logged in:
```js
// first determine the user's logged-in status
app.use((req, res, next) => {
if (/* user is logged in */) {
req.isLoggedIn = true
} else {
req.isLoggedIn = false
}
next()
})

// next configure express-rate-limit to skip logged in users
app.use(rateLimit({
skip: (req, res) => req.isLoggedIn,
// ...
}))
```
## Warnings
### `WRN_ERL_MAX_ZERO`
Expand Down
2 changes: 2 additions & 0 deletions source/lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,8 @@ const rateLimit = (
const config = parseOptions(passedOptions ?? {})
const options = getOptionsFromConfig(config)

config.validations.creationStack()

// Call the `init` method on the store, if it exists
if (typeof config.store.init === 'function') config.store.init(options)

Expand Down
15 changes: 15 additions & 0 deletions source/validations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,21 @@ const validations = {
}
}
},

/**
* Checks to see if the instance was created inside of a request handler, which would prevent it from working correctly.
*/
creationStack() {
const { stack } = new Error(
'express-rate-limit validation check (set options.validate.creationStack=false to disable)',
)
if (stack?.includes('Layer.handle [as handle_request]')) {
throw new ValidationError(
'ERR_ERL_CREATED_IN_REQUEST_HANDLER',
`express-rate-limit instance should be created at app initialization, not when responding to a request.`,
)
}
},
}

export type Validations = typeof validations
Expand Down
22 changes: 22 additions & 0 deletions test/library/validation-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import {
beforeEach,
afterEach,
} from '@jest/globals'
import express from 'express'
import supertest from 'supertest'
import { getValidations } from '../../source/validations.js'
import type { Store } from '../../source/types'

Expand Down Expand Up @@ -332,4 +334,24 @@ describe('validations tests', () => {
expect(console.error).not.toBeCalled()
})
})

describe('creationStack', () => {
it('should log an error if called in an express request handler', async () => {
const app = express()
app.get('/', (request, response) => {
validations.creationStack()
response.send('hello')
})
await supertest(app).get('/').expect('hello')
expect(console.error).toHaveBeenCalledWith(
expect.objectContaining({ code: 'ERR_ERL_CREATED_IN_REQUEST_HANDLER' }),
)
expect(console.warn).not.toBeCalled()
})
it('should not log an error if called elsewhere', async () => {
validations.creationStack()
expect(console.error).not.toBeCalled()
expect(console.warn).not.toBeCalled()
})
})
})

0 comments on commit be7fe9c

Please sign in to comment.