Introduce experimental new internal authz API#1617
Introduce experimental new internal authz API#1617alilleybrinker wants to merge 9 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 2 months 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
967269f to
8ffd904
Compare
8ffd904 to
136dbab
Compare
This bumps the version of the ECMAScript standard used by eslint to a newer version, so eslint doesn't throw up its hands when some newer idioms are used. Signed-off-by: Andrew Lilley Brinker <abrinker@mitre.org>
This makes sure we explicitly specify the MongoDB connection string in `action:coverage` to address an issue I observed in CI for the action would sometimes fail to connect. It also modifies the `action:coverage` command to delegate to the `test` command, so that any future updates to how tests are run is automatically picked up by the coverage checker. Signed-off-by: Andrew Lilley Brinker <abrinker@mitre.org>
422ee51 to
d0d5ff9
Compare
This modifies the script `populate.js` to be an ECMAScript module (`.mjs`), and updates `package.json` accordingly. This is done to ensure predictable "await" behavior within the script, including explicit support for top-level "await". See the MDN docs on modules for more [1]. [1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules Signed-off-by: Andrew Lilley Brinker <abrinker@mitre.org>
d0d5ff9 to
de11a5d
Compare
This introduces a new "npm run" command: "test:integration-local" which is equivalent to "test:integration" except that it uses the proper connection string to connect to a local instance of MongoDB during development. Signed-off-by: Andrew Lilley Brinker <abrinker@mitre.org>
Previously, some logging calls would stringify a JSON structure before writing it to the log. This is incorrect, and makes for some awkward logs when reaching. Instead, if you include a message field in your struct, then it will get printed while the other fields are printed as structured information in the log. So this updates logging calls throughout the application to do the correct thing. Signed-off-by: Andrew Lilley Brinker <abrinker@mitre.org>
This is step 1 of adopting "jsdoc" for documentation, which *only* introduces the dependency, updates the .gitignore, and applies basic configuration so we can use Markdown in jsdoc comments. This includes no actual documentation, which will be added next. Signed-off-by: Andrew Lilley Brinker <abrinker@mitre.org>
This adopts jsdoc as cve-services' mechanism for internal API documentation. This mostly means annotating modules, but not actually writing documentation for them, except for the new authz middleware which is the initial test case. One of the challenges I've had since joining the CVE Services team has been discovering and understanding the internal structure of the codebase, and this is in part my attempt to help ease that problem for others. Signed-off-by: Andrew Lilley Brinker <abrinker@mitre.org>
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. For test, note the following: 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. To deal with this, this commit also ensures that authz tests are isolated in their own, separate run of Mocha. We also shim the method Set.prototype.intersection. The Set.prototype.intersection method was added to the Set type in Node.js version 22. Currently, CVE Services uses an older version of Node and so we need this shim to ensure the API runs. We are planning to upgrade to Node 24 soon, in which case this shim will become unecessary. Signed-off-by: Andrew Lilley Brinker <abrinker@mitre.org>
de11a5d to
12e5717
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.