Skip to content

BC-10757 - content authorization#3711

Draft
virgilchiriac wants to merge 2 commits intomainfrom
BC-10757-content-authorization
Draft

BC-10757 - content authorization#3711
virgilchiriac wants to merge 2 commits intomainfrom
BC-10757-content-authorization

Conversation

@virgilchiriac
Copy link
Contributor

@virgilchiriac virgilchiriac commented Oct 30, 2025

Description

Links to Tickets or other pull requests

BC-10757

hpi-schul-cloud/schulcloud-server#5943

Changes

Data Security

Deployment

New Repos, NPM packages or vendor scripts

Screenshots of UI changes

Approval for review

  • QA: In addition to review, the code has been manually tested (if manual testing is possible)
  • All points were discussed with the ticket creator, support-team or product owner. The code upholds all quality guidelines from the PR-template.
  • DEV: Every new component is implemented having accessibility in mind (e.g. aria-label, role property)

Notice: Please remove the WIP label if the PR is ready to review, otherwise nobody will review it.

@sonarqubecloud
Copy link

// secure routes
router.use(authHelper.authChecker);

router.use(preventCourseLocked);

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
authorization
, but is not rate-limited.
This route handler performs
authorization
, but is not rate-limited.

Copilot Autofix

AI 3 months ago

To address this, implement rate limiting for the router defined in this file using a popular, well-supported middleware such as express-rate-limit. This should be applied just after authentication and before (or with) preventCourseLocked to ensure excessive requests to these endpoints do not overwhelm the system.

Specifically:

  • Import express-rate-limit at the top of the file.
  • Create a rate limiter instance (configurable, e.g., 100 requests per 15 minutes per IP).
  • Insert router.use(limiter); between the authentication middleware and the rest (or strictly before preventCourseLocked).
  • No changes are needed to route handlers or existing middleware ordering/functionality.

Suggested changeset 2
controllers/coursegroups.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/controllers/coursegroups.js b/controllers/coursegroups.js
--- a/controllers/coursegroups.js
+++ b/controllers/coursegroups.js
@@ -1,5 +1,6 @@
 const _ = require('lodash');
 const express = require('express');
+const rateLimit = require('express-rate-limit');
 const moment = require('moment');
 const { Configuration } = require('@hpi-schul-cloud/commons');
 
@@ -71,6 +72,15 @@
 // secure routes
 router.use(authHelper.authChecker);
 
+// rate limiting: max 100 requests per 15min window per IP
+const groupLimiter = rateLimit({
+    windowMs: 15 * 60 * 1000, // 15 minutes
+    max: 100, // limit each IP to 100 requests per windowMs
+    standardHeaders: true,
+    legacyHeaders: false,
+});
+router.use(groupLimiter);
+
 router.use(preventCourseLocked);
 
 router.get('/add', editCourseGroupHandler);
EOF
@@ -1,5 +1,6 @@
const _ = require('lodash');
const express = require('express');
const rateLimit = require('express-rate-limit');
const moment = require('moment');
const { Configuration } = require('@hpi-schul-cloud/commons');

@@ -71,6 +72,15 @@
// secure routes
router.use(authHelper.authChecker);

// rate limiting: max 100 requests per 15min window per IP
const groupLimiter = rateLimit({
windowMs: 15 * 60 * 1000, // 15 minutes
max: 100, // limit each IP to 100 requests per windowMs
standardHeaders: true,
legacyHeaders: false,
});
router.use(groupLimiter);

router.use(preventCourseLocked);

router.get('/add', editCourseGroupHandler);
package.json
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/package.json b/package.json
--- a/package.json
+++ b/package.json
@@ -107,7 +107,8 @@
 		"string-strip-html": "8.3.0",
 		"ua-parser-js": "^0.7.33",
 		"video.js": "^7.20.3",
-		"winston": "^3.2.0"
+		"winston": "^3.2.0",
+		"express-rate-limit": "^8.2.0"
 	},
 	"devDependencies": {
 		"@babel/core": "^7.28.0",
EOF
@@ -107,7 +107,8 @@
"string-strip-html": "8.3.0",
"ua-parser-js": "^0.7.33",
"video.js": "^7.20.3",
"winston": "^3.2.0"
"winston": "^3.2.0",
"express-rate-limit": "^8.2.0"
},
"devDependencies": {
"@babel/core": "^7.28.0",
This fix introduces these dependencies
Package Version Security advisories
express-rate-limit (npm) 8.2.0 None
Copilot is powered by AI and may make mistakes. Always verify output.
// secure routes
router.use(authHelper.authChecker);

router.use(preventCourseLocked);

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
authorization
, but is not rate-limited.
This route handler performs
authorization
, but is not rate-limited.

Copilot Autofix

AI 3 months ago

To address the missing rate limiting, we should add a rate-limiting middleware to this router (the Express router instance in controllers/courses.js). The standard solution in Express is the express-rate-limit package, which should be required and configured. The rate limiter middleware should be added as soon as possible after the authentication and authorization middleware (authChecker and preventCourseLocked).
This can be accomplished by:

  1. Adding a require('express-rate-limit') at the top.
  2. Creating a rate limiter instance with settings appropriate for the sensitivity of these routes (e.g., a reasonable number of requests per window per user).
  3. Applying the limiter with router.use(limiter); just after authChecker and preventCourseLocked middlewares.

This change will ensure requests to secured course routes are subject to rate limiting, and protect against brute-force/DoS attacks.

Suggested changeset 2
controllers/courses.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/controllers/courses.js b/controllers/courses.js
--- a/controllers/courses.js
+++ b/controllers/courses.js
@@ -3,6 +3,7 @@
 /* eslint-disable no-underscore-dangle */
 const _ = require('lodash');
 const express = require('express');
+const rateLimit = require('express-rate-limit');
 const moment = require('moment');
 const { decode } = require('html-entities');
 
@@ -488,6 +489,15 @@
 
 router.use(preventCourseLocked);
 
+// Apply rate limiting to all subsequent routes (after auth and preventCourseLocked)
+const courseLimiter = rateLimit({
+  windowMs: 15 * 60 * 1000, // 15 minutes
+  max: 100, // 100 requests per windowMs per IP
+  standardHeaders: true, // Return rate limit info in the `RateLimit-*` headers
+  legacyHeaders: false, // Disable the `X-RateLimit-*` headers
+});
+router.use(courseLimiter);
+
 /**
  *
  * @param {*} courses, string userId
EOF
@@ -3,6 +3,7 @@
/* eslint-disable no-underscore-dangle */
const _ = require('lodash');
const express = require('express');
const rateLimit = require('express-rate-limit');
const moment = require('moment');
const { decode } = require('html-entities');

@@ -488,6 +489,15 @@

router.use(preventCourseLocked);

// Apply rate limiting to all subsequent routes (after auth and preventCourseLocked)
const courseLimiter = rateLimit({
windowMs: 15 * 60 * 1000, // 15 minutes
max: 100, // 100 requests per windowMs per IP
standardHeaders: true, // Return rate limit info in the `RateLimit-*` headers
legacyHeaders: false, // Disable the `X-RateLimit-*` headers
});
router.use(courseLimiter);

/**
*
* @param {*} courses, string userId
package.json
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/package.json b/package.json
--- a/package.json
+++ b/package.json
@@ -107,7 +107,8 @@
 		"string-strip-html": "8.3.0",
 		"ua-parser-js": "^0.7.33",
 		"video.js": "^7.20.3",
-		"winston": "^3.2.0"
+		"winston": "^3.2.0",
+		"express-rate-limit": "^8.2.0"
 	},
 	"devDependencies": {
 		"@babel/core": "^7.28.0",
EOF
@@ -107,7 +107,8 @@
"string-strip-html": "8.3.0",
"ua-parser-js": "^0.7.33",
"video.js": "^7.20.3",
"winston": "^3.2.0"
"winston": "^3.2.0",
"express-rate-limit": "^8.2.0"
},
"devDependencies": {
"@babel/core": "^7.28.0",
This fix introduces these dependencies
Package Version Security advisories
express-rate-limit (npm) 8.2.0 None
Copilot is powered by AI and may make mistakes. Always verify output.
// secure routes
router.use(authHelper.authChecker);

router.use(preventCourseLocked);

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
authorization
, but is not rate-limited.
This route handler performs
authorization
, but is not rate-limited.

Copilot Autofix

AI 3 months ago

To fix the issue, a rate limiting middleware should be applied before (router.use) the expensive or sensitive route middleware, such as preventCourseLocked. In Express, the standard is to use the widely adopted express-rate-limit package. The most sensible approach is to import express-rate-limit, configure a reasonable rate limiter (e.g., 100 requests per 15 minutes per IP), and apply it before the code that currently calls preventCourseLocked. This approach globally limits all endpoints covered by this router, requiring only minimal code changes and no alterations in existing behavior, except defending against DoS. The following changes are needed:

  • Import express-rate-limit.
  • Add the rate limiter middleware before preventCourseLocked.

Suggested changeset 2
controllers/files.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/controllers/files.js b/controllers/files.js
--- a/controllers/files.js
+++ b/controllers/files.js
@@ -5,14 +5,13 @@
 const url = require('url');
 const rp = require('request-promise');
 const express = require('express');
+const rateLimit = require('express-rate-limit');
 const multer = require('multer');
-const shortid = require('shortid');
 const _ = require('lodash');
 const { Configuration } = require('@hpi-schul-cloud/commons');
 
 const upload = multer({ storage: multer.memoryStorage() });
 
-const api = require('../api');
 const authHelper = require('../helpers/authentication');
 const redirectHelper = require('../helpers/redirect');
 const { logger, formatError } = require('../helpers');
@@ -379,6 +372,9 @@
 // secure routes
 router.use(authHelper.authChecker);
 
+// Apply rate limiter before potentially expensive operations
+router.use(limiter);
+
 router.use(preventCourseLocked);
 
 const getSignedUrl = (req, res, next) => {
EOF
@@ -5,14 +5,13 @@
const url = require('url');
const rp = require('request-promise');
const express = require('express');
const rateLimit = require('express-rate-limit');
const multer = require('multer');
const shortid = require('shortid');
const _ = require('lodash');
const { Configuration } = require('@hpi-schul-cloud/commons');

const upload = multer({ storage: multer.memoryStorage() });

const api = require('../api');
const authHelper = require('../helpers/authentication');
const redirectHelper = require('../helpers/redirect');
const { logger, formatError } = require('../helpers');
@@ -379,6 +372,9 @@
// secure routes
router.use(authHelper.authChecker);

// Apply rate limiter before potentially expensive operations
router.use(limiter);

router.use(preventCourseLocked);

const getSignedUrl = (req, res, next) => {
package.json
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/package.json b/package.json
--- a/package.json
+++ b/package.json
@@ -107,7 +107,8 @@
 		"string-strip-html": "8.3.0",
 		"ua-parser-js": "^0.7.33",
 		"video.js": "^7.20.3",
-		"winston": "^3.2.0"
+		"winston": "^3.2.0",
+		"express-rate-limit": "^8.2.0"
 	},
 	"devDependencies": {
 		"@babel/core": "^7.28.0",
EOF
@@ -107,7 +107,8 @@
"string-strip-html": "8.3.0",
"ua-parser-js": "^0.7.33",
"video.js": "^7.20.3",
"winston": "^3.2.0"
"winston": "^3.2.0",
"express-rate-limit": "^8.2.0"
},
"devDependencies": {
"@babel/core": "^7.28.0",
This fix introduces these dependencies
Package Version Security advisories
express-rate-limit (npm) 8.2.0 None
Copilot is powered by AI and may make mistakes. Always verify output.

router.use(authHelper.authChecker);

router.use(preventCourseLocked);

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
authorization
, but is not rate-limited.
This route handler performs
authorization
, but is not rate-limited.

Copilot Autofix

AI 3 months ago

The recommended fix is to add rate limiting middleware to the router, as early as possible (before other resource-intensive or authorization middlewares). The express-rate-limit package is a widely used solution for Express apps.

  • Add a rate limiter instance, for example:
    const rateLimit = require('express-rate-limit');
    const limiter = rateLimit({
      windowMs: 15 * 60 * 1000, // 15 minutes
      max: 100, // limit each IP to 100 requests per windowMs
    });
  • Apply the limiter to the router (e.g., router.use(limiter);) before resource-intensive or authorization middlewares.
  • Update the imports section at the top of the file to include express-rate-limit.

This conservatively protects all routes within this controller against DOS attacks, without interfering with existing functionality.


Suggested changeset 2
controllers/homework.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/controllers/homework.js b/controllers/homework.js
--- a/controllers/homework.js
+++ b/controllers/homework.js
@@ -5,6 +5,7 @@
  */
 
 const express = require('express');
+const rateLimit = require('express-rate-limit');
 const handlebars = require('handlebars');
 const _ = require('lodash');
 const api = require('../api');
@@ -20,6 +21,13 @@
 
 const router = express.Router();
 
+// Apply rate limiter to all requests in this router (limit each IP to 100 requests per 15 min)
+const limiter = rateLimit({
+    windowMs: 15 * 60 * 1000, // 15 minutes
+    max: 100, // limit each IP to 100 requests per windowMs
+});
+router.use(limiter);
+
 handlebars.registerHelper('ifvalue', (conditional, options) => {
 	if (options.hash.value === conditional) {
 		return options.fn(this);
EOF
@@ -5,6 +5,7 @@
*/

const express = require('express');
const rateLimit = require('express-rate-limit');
const handlebars = require('handlebars');
const _ = require('lodash');
const api = require('../api');
@@ -20,6 +21,13 @@

const router = express.Router();

// Apply rate limiter to all requests in this router (limit each IP to 100 requests per 15 min)
const limiter = rateLimit({
windowMs: 15 * 60 * 1000, // 15 minutes
max: 100, // limit each IP to 100 requests per windowMs
});
router.use(limiter);

handlebars.registerHelper('ifvalue', (conditional, options) => {
if (options.hash.value === conditional) {
return options.fn(this);
package.json
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/package.json b/package.json
--- a/package.json
+++ b/package.json
@@ -107,7 +107,8 @@
 		"string-strip-html": "8.3.0",
 		"ua-parser-js": "^0.7.33",
 		"video.js": "^7.20.3",
-		"winston": "^3.2.0"
+		"winston": "^3.2.0",
+		"express-rate-limit": "^8.2.0"
 	},
 	"devDependencies": {
 		"@babel/core": "^7.28.0",
EOF
@@ -107,7 +107,8 @@
"string-strip-html": "8.3.0",
"ua-parser-js": "^0.7.33",
"video.js": "^7.20.3",
"winston": "^3.2.0"
"winston": "^3.2.0",
"express-rate-limit": "^8.2.0"
},
"devDependencies": {
"@babel/core": "^7.28.0",
This fix introduces these dependencies
Package Version Security advisories
express-rate-limit (npm) 8.2.0 None
Copilot is powered by AI and may make mistakes. Always verify output.
// secure routes
router.use(authHelper.authChecker);

router.use(preventCourseLocked);

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
authorization
, but is not rate-limited.
This route handler performs
authorization
, but is not rate-limited.

Copilot Autofix

AI 3 months ago

To address the detected vulnerability, rate-limiting should be added to the router so that potentially expensive endpoint access is restricted. The best way is to use a middleware such as express-rate-limit, as recommended in CodeQL’s guidance. Specifically, create a rate-limiter middleware instance (e.g., allowing 100 requests per 15 minutes per IP), and apply it before the authorization and preventCourseLocked middleware using router.use. This means inserting the necessary import, creating the limiter, and placing router.use(limiter) at the correct point so that all routes handled by this router are protected. No other code needs to be changed, and existing functionality will remain untouched.

The following regions/lines in controllers/topics.js need edits:

  • Add an import for express-rate-limit.
  • Add the rate-limiter configuration.
  • Use router.use(limiter) before security and expensive handlers.

Suggested changeset 2
controllers/topics.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/controllers/topics.js b/controllers/topics.js
--- a/controllers/topics.js
+++ b/controllers/topics.js
@@ -1,5 +1,6 @@
 const moment = require('moment');
 const express = require('express');
+const RateLimit = require('express-rate-limit');
 const shortId = require('shortid');
 const { randomBytes } = require('crypto');
 const { decode } = require('html-entities');
@@ -157,6 +158,12 @@
 	return path.substring(path.lastIndexOf('/') + 1);
 };
 
+// set up rate limiter: maximum of 100 requests per 15 minutes per IP
+const limiter = RateLimit({
+	windowMs: 15 * 60 * 1000, // 15 minutes
+	max: 100, // limit each IP to 100 requests per windowMs
+});
+router.use(limiter);
 // secure routes
 router.use(authHelper.authChecker);
 
EOF
@@ -1,5 +1,6 @@
const moment = require('moment');
const express = require('express');
const RateLimit = require('express-rate-limit');
const shortId = require('shortid');
const { randomBytes } = require('crypto');
const { decode } = require('html-entities');
@@ -157,6 +158,12 @@
return path.substring(path.lastIndexOf('/') + 1);
};

// set up rate limiter: maximum of 100 requests per 15 minutes per IP
const limiter = RateLimit({
windowMs: 15 * 60 * 1000, // 15 minutes
max: 100, // limit each IP to 100 requests per windowMs
});
router.use(limiter);
// secure routes
router.use(authHelper.authChecker);

package.json
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/package.json b/package.json
--- a/package.json
+++ b/package.json
@@ -107,7 +107,8 @@
 		"string-strip-html": "8.3.0",
 		"ua-parser-js": "^0.7.33",
 		"video.js": "^7.20.3",
-		"winston": "^3.2.0"
+		"winston": "^3.2.0",
+		"express-rate-limit": "^8.2.0"
 	},
 	"devDependencies": {
 		"@babel/core": "^7.28.0",
EOF
@@ -107,7 +107,8 @@
"string-strip-html": "8.3.0",
"ua-parser-js": "^0.7.33",
"video.js": "^7.20.3",
"winston": "^3.2.0"
"winston": "^3.2.0",
"express-rate-limit": "^8.2.0"
},
"devDependencies": {
"@babel/core": "^7.28.0",
This fix introduces these dependencies
Package Version Security advisories
express-rate-limit (npm) 8.2.0 None
Copilot is powered by AI and may make mistakes. Always verify output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant