Introduce experimental new internal authz API#1617
Introduce experimental new internal authz API#1617alilleybrinker wants to merge 11 commits intodevfrom
Conversation
| mw.authzApiSelector({ | ||
| oldAuthz: mw.onlySecretariat, | ||
| newAuthz: authz(orgHasRole(OrgRoles.SECRETARIAT)) | ||
| }), |
Check failure
Code scanning / CodeQL
Missing rate limiting
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, the fix is to add a rate‑limiting middleware into the route’s middleware chain so that expensive, authorized operations cannot be triggered arbitrarily often. In Express, this is commonly done using the well‑known express-rate-limit package. For minimal functional impact, we should define a limiter with reasonable defaults (e.g., window in minutes and a relatively high max count) and apply it only to this sensitive route (or a small set of similar routes) rather than globally.
Concretely for src/controller/cve.controller/index.js, we will:
- Import
express-rate-limitnear the top of the file, alongside the otherrequirestatements. - Define a limiter instance dedicated to CVE modification routes, e.g.,
const cveUpdateLimiter = rateLimit({ ... }). A sensible baseline is something like a 15‑minute window and a fairly generous max count so normal users are unaffected. - Insert this limiter into the route’s middleware chain before the controller, after authentication/authorization/validation middleware where appropriate. Since rate limiting is about how often the endpoint can be hit (regardless of payload validity), it can reasonably be placed early, but placing it after
mw.validateUserensures anonymous traffic cannot probe the limiter behavior; we will place it right aftermw.validateUserand before the authorization selector.
We will only modify the shown region of index.js: add the import and limiter definition at the top and inject cveUpdateLimiter into the middleware list around line 587. No existing behavior of controller.CVE_UPDATE_SINGLE or the other middlewares will be changed.
| @@ -14,6 +14,14 @@ | ||
| const CONSTANTS = getConstants() | ||
| const CHOICES = [CONSTANTS.CVE_STATES.REJECTED, CONSTANTS.CVE_STATES.PUBLISHED] | ||
| const toDate = require('../../utils/utils').toDate | ||
| const rateLimit = require('express-rate-limit') | ||
|
|
||
| const cveUpdateLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // limit each IP to 100 update requests per windowMs | ||
| standardHeaders: true, | ||
| legacyHeaders: false | ||
| }) | ||
| router.get('/cve/:id', | ||
| /* | ||
| #swagger.tags = ['CVE Record'] | ||
| @@ -584,6 +592,7 @@ | ||
| } | ||
| */ | ||
| mw.validateUser, | ||
| cveUpdateLimiter, | ||
| mw.authzApiSelector({ | ||
| oldAuthz: mw.onlySecretariat, | ||
| newAuthz: authz(orgHasRole(OrgRoles.SECRETARIAT)) |
2546ff3 to
967269f
Compare
Implements an assortment of refactors to the integration test helpers, constants, and the tests themselves with a few goals in mind: - Clarity and consistency of helper APIs - Separation of request builders from tests - Simplification of frequently used variable names Signed-off-by: Andrew Lilley Brinker <abrinker@mitre.org>
This adds a new API for authorization, defined in `src/middleware/authz.js`, which is centered around two key functions: `authz` and `authzLevel`. Each returns a middleware function which applies the requested authorization checks. For `authz`, if the authorization checks fail, then the request fails. For `authzLevel`, if the authorization checks fail, then the request continues but without an authorization level being set on the request context. In addition to these top-level APIs, this introduces a set of pre-defined checks, plus two check combinators, which collectively will enable CVE Services endpoints to define the authorization checks they require, all in one place. This is intended to replace the combination of existing authorization middleware functions and ad-hoc authorization checks performed throughout a number of endpoints. This commit *does not* include any replacement of existing authorization checks, only the introduction of the new API. Signed-off-by: Andrew Lilley Brinker <abrinker@mitre.org>
Signed-off-by: Andrew Lilley Brinker <abrinker@mitre.org>
Signed-off-by: Andrew Lilley Brinker <abrinker@mitre.org>
Signed-off-by: Andrew Lilley Brinker <abrinker@mitre.org>
Mocha doesn't isolate tests in their own process, which means when the tests are running they're actually all sharing a singleton instance of the Express app. This is a problem for the authz testing specifically, because it modifies a piece of global state (`useNewAuthzApi`) to select at runtime whether to use the old or new versions of the authorization API. The change in this commit manually isolates the authz tests by putting them under a separate invocation of Mocha. Signed-off-by: Andrew Lilley Brinker <abrinker@mitre.org>
Signed-off-by: Andrew Lilley Brinker <abrinker@mitre.org>
Signed-off-by: Andrew Lilley Brinker <abrinker@mitre.org>
Signed-off-by: Andrew Lilley Brinker <abrinker@mitre.org>
Signed-off-by: Andrew Lilley Brinker <abrinker@mitre.org>
Signed-off-by: Andrew Lilley Brinker <abrinker@mitre.org>
967269f to
8ffd904
Compare
This introduces a new experimental authorization API and begins the process of testing it. It also includes refactors to the integration test suite, and the introduction of a new
test:integration-localtask to run the integration test suite fully locally.This is best reviewed commit-by-commit.