Skip to content

Introduce experimental new internal authz API#1617

Open
alilleybrinker wants to merge 9 commits intodevfrom
alilleybrinker/authz-refactor
Open

Introduce experimental new internal authz API#1617
alilleybrinker wants to merge 9 commits intodevfrom
alilleybrinker/authz-refactor

Conversation

@alilleybrinker
Copy link
Copy Markdown
Collaborator

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-local task to run the integration test suite fully locally.

This is best reviewed commit-by-commit.

@alilleybrinker alilleybrinker self-assigned this Jan 23, 2026
@alilleybrinker alilleybrinker added the javascript Pull requests that update javascript code label Jan 23, 2026
@alilleybrinker alilleybrinker marked this pull request as draft February 4, 2026 16:10
Comment on lines +587 to +590
mw.authzApiSelector({
oldAuthz: mw.onlySecretariat,
newAuthz: authz(orgHasRole(OrgRoles.SECRETARIAT))
}),

Check failure

Code scanning / CodeQL

Missing rate limiting

This route handler performs [authorization](1), but is not rate-limited. This route handler performs [authorization](2), but is not rate-limited.

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-limit near the top of the file, alongside the other require statements.
  • 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.validateUser ensures anonymous traffic cannot probe the limiter behavior; we will place it right after mw.validateUser and 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.

Suggested changeset 1
src/controller/cve.controller/index.js

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/controller/cve.controller/index.js b/src/controller/cve.controller/index.js
--- a/src/controller/cve.controller/index.js
+++ b/src/controller/cve.controller/index.js
@@ -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))
EOF
@@ -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))
Copilot is powered by AI and may make mistakes. Always verify output.
@alilleybrinker alilleybrinker force-pushed the alilleybrinker/authz-refactor branch 5 times, most recently from 2546ff3 to 967269f Compare February 4, 2026 23:16
@alilleybrinker alilleybrinker marked this pull request as ready for review February 4, 2026 23:24
@alilleybrinker alilleybrinker force-pushed the alilleybrinker/authz-refactor branch from 967269f to 8ffd904 Compare March 11, 2026 22:09
@alilleybrinker alilleybrinker force-pushed the alilleybrinker/authz-refactor branch from 8ffd904 to 136dbab Compare March 31, 2026 21:00
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>
@alilleybrinker alilleybrinker force-pushed the alilleybrinker/authz-refactor branch 2 times, most recently from 422ee51 to d0d5ff9 Compare March 31, 2026 21:48
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>
@alilleybrinker alilleybrinker force-pushed the alilleybrinker/authz-refactor branch from d0d5ff9 to de11a5d Compare March 31, 2026 21:50
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>
@alilleybrinker alilleybrinker force-pushed the alilleybrinker/authz-refactor branch from de11a5d to 12e5717 Compare March 31, 2026 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

javascript Pull requests that update javascript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants