Merge pull request #805 from tdurieux/codex/express-5-migration

chore: upgrade Express to 5.2.1

package.json

@@ -50,7 +50,7 @@
     "crypto-js": "^4.2.0",
     "decompress-stream-to-s3": "^2.1.1",
     "dotenv": "^16.4.5",
-    "express": "^4.19.2",
+    "express": "^5.2.1",
     "express-rate-limit": "^6.11.2",
     "express-session": "^1.18.0",
     "express-slow-down": "^2.0.1",
@@ -79,7 +79,7 @@
     "@types/archiver": "^5.3.4",
     "@types/compression": "^1.7.5",
     "@types/crypto-js": "^4.2.2",
-    "@types/express": "^4.17.21",
+    "@types/express": "^5.0.6",
     "@types/express-session": "^1.18.0",
     "@types/got": "^9.6.12",
     "@types/inquirer": "^8.2.10",

scripts/dev-proxy.js

@@ -177,7 +177,8 @@ app.use(
   }),
 );
 
-app.listen(PORT, () => {
+app.listen(PORT, (error) => {
+  if (error) throw error;
   console.log(
     `\n  dev-proxy  http://localhost:${PORT}` +
       `\n  → local:   ${PUBLIC_DIR}` +

src/server/index.ts

@@ -99,7 +99,13 @@ function indexResponse(req: express.Request, res: express.Response) {
 
 export default async function start() {
   const app = express();
+  app.set("query parser", "extended");
   app.use(express.json());
+  // Preserve the empty body used by API validation when no JSON was parsed.
+  app.use((req, _res, next) => {
+    req.body ??= {};
+    next();
+  });
 
   app.use(
     compression({
@@ -349,10 +355,10 @@ export default async function start() {
     .get("/", indexResponse)
     .get("/404", indexResponse)
     .get("/anonymize", indexResponse)
-    .get("/r/:repoId/?*", indexResponse)
-    .get("/repository/:repoId/?*", indexResponse);
+    .get("/r/:repoId{/*path}", indexResponse)
+    .get("/repository/:repoId{/*path}", indexResponse);
 
-  app.get("*", indexResponse);
+  app.get("/{*path}", indexResponse);
 
   // start schedules
   conferenceStatusCheck();

src/server/routes/conference.ts

@@ -161,7 +161,7 @@ export function applyConferenceForm(
 }
 
 router.post(
-  "/:conferenceID?",
+  "/{:conferenceID}",
   async (req: express.Request, res: express.Response) => {
     try {
       const user = await getUser(req);

src/server/routes/file.ts

@@ -54,8 +54,8 @@ export function filePathFromRequestUrl(
 }
 
 router.get(
-  "/:repoId/file/:path*",
-  async (req: express.Request, res: express.Response) => {
+  "/:repoId/file/*path",
+  async (req, res) => {
     const anonymizedPath = filePathFromRequestUrl(
       req.url,
       req.protocol,

src/server/routes/pullRequest-private.ts

@@ -22,7 +22,7 @@ router.use(ensureAuthenticated);
 // refresh pullRequest
 router.post(
   "/:pullRequestId/refresh",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     try {
       const pullRequest = await getPullRequest(req, res, { nocheck: true });
       if (!pullRequest) return;
@@ -41,7 +41,7 @@ router.post(
 // online if it had expired
 router.post(
   "/:pullRequestId/extend",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     try {
       const pullRequest = await getPullRequest(req, res, { nocheck: true });
       if (!pullRequest) return;
@@ -81,7 +81,7 @@ router.post(
 // delete a pullRequest
 router.delete(
   "/:pullRequestId/",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     const pullRequest = await getPullRequest(req, res, { nocheck: true });
     if (!pullRequest) return;
     try {
@@ -102,7 +102,7 @@ router.delete(
 
 router.get(
   "/:owner/:repository/:pullRequestId",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     try {
       const user = await getUser(req);
       const pullRequest = new PullRequest(
@@ -126,7 +126,7 @@ router.get(
 // get pullRequest information
 router.get(
   "/:pullRequestId/",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     try {
       const pullRequest = await getPullRequest(req, res, { nocheck: true });
       if (!pullRequest) return;
@@ -214,7 +214,7 @@ function updatePullRequestModel(
 // update a pullRequest
 router.post(
   "/:pullRequestId/",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     try {
       const pullRequest = await getPullRequest(req, res, { nocheck: true });
       if (!pullRequest) return;
@@ -248,7 +248,7 @@ router.post(
 );
 
 // add pullRequest
-router.post("/", async (req: express.Request, res: express.Response) => {
+router.post("/", async (req, res) => {
   const pullRequestUpdate = req.body;
   try {
     const user = await getUser(req);

src/server/routes/repository-private.ts

@@ -55,7 +55,7 @@ async function getTokenForAdmin(user: User, req: express.Request) {
 }
 
 // claim a repository
-router.post("/claim", async (req: express.Request, res: express.Response) => {
+router.post("/claim", async (req, res) => {
   try {
     const user = await getUser(req);
     if (!req.body.repoId) {
@@ -129,7 +129,7 @@ router.post("/claim", async (req: express.Request, res: express.Response) => {
 // refresh repository
 router.post(
   "/:repoId/refresh",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     try {
       const repo = await getRepo(req, res, {
         nocheck: true,
@@ -157,7 +157,7 @@ router.post(
 // online if it had expired
 router.post(
   "/:repoId/extend",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     try {
       const repo = await getRepo(req, res, { nocheck: true });
       if (!repo) return;
@@ -216,7 +216,7 @@ router.post(
 // delete a repository
 router.delete(
   "/:repoId/",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     const repo = await getRepo(req, res, {
       nocheck: true,
     });
@@ -243,7 +243,7 @@ router.delete(
 
 router.get(
   "/:owner/:repo/",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     try {
       const user = await getUser(req);
       let token = await user.getAccessToken();
@@ -266,7 +266,7 @@ router.get(
 
 router.get(
   "/:owner/:repo/branches",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     try {
       const user = await getUser(req);
       let token = await user.getAccessToken();
@@ -294,7 +294,7 @@ router.get(
 
 router.get(
   "/:owner/:repo/readme",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     try {
       const user = await getUser(req);
       let token = await user.getAccessToken();
@@ -311,7 +311,7 @@ router.get(
       });
       if (!repo) {
         throw new AnonymousError("repo_not_found", {
-          object: req.params.repoId,
+          object: `${req.params.owner}/${req.params.repo}`,
           httpStatus: 404,
         });
       }
@@ -329,7 +329,7 @@ router.get(
 );
 
 // get repository information
-router.get("/:repoId/", async (req: express.Request, res: express.Response) => {
+router.get("/:repoId/", async (req, res) => {
   try {
     const repo = await getRepo(req, res, {
       nocheck: true,
@@ -473,7 +473,7 @@ export function shouldReactivateInactiveRepository(
 // update a repository
 router.post(
   "/:repoId/",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     try {
       const repo = await getRepo(req, res, {
         nocheck: true,
@@ -610,7 +610,7 @@ router.post(
 );
 
 // add repository
-router.post("/", async (req: express.Request, res: express.Response) => {
+router.post("/", async (req, res) => {
   const repoUpdate = req.body;
   try {
     const user = await getUser(req);
@@ -719,7 +719,7 @@ router.post("/", async (req: express.Request, res: express.Response) => {
 // list coauthors
 router.get(
   "/:repoId/coauthors",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     try {
       const repo = await getRepo(req, res, { nocheck: true });
       if (!repo) return;
@@ -735,7 +735,7 @@ router.get(
 // add a coauthor (owner/admin only)
 router.post(
   "/:repoId/coauthors",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     try {
       const repo = await getRepo(req, res, { nocheck: true });
       if (!repo) return;
@@ -798,7 +798,7 @@ router.post(
 // remove a coauthor (owner/admin only, or the coauthor themselves)
 router.delete(
   "/:repoId/coauthors/:username",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     try {
       const repo = await getRepo(req, res, { nocheck: true });
       if (!repo) return;

src/server/routes/route-utils.ts

@@ -17,6 +17,9 @@ export async function getGist(
   opt?: { nocheck?: boolean }
 ) {
   try {
+    if (typeof req.params.gistId !== "string") {
+      throw new AnonymousError("invalid_path", { httpStatus: 400 });
+    }
     const gist = await db.getGist(req.params.gistId);
     if (opt?.nocheck !== true) {
       if (
@@ -42,6 +45,9 @@ export async function getPullRequest(
   opt?: { nocheck?: boolean }
 ) {
   try {
+    if (typeof req.params.pullRequestId !== "string") {
+      throw new AnonymousError("invalid_path", { httpStatus: 400 });
+    }
     const pullRequest = await db.getPullRequest(req.params.pullRequestId);
     if (opt?.nocheck !== true) {
       // redirect if the repository is expired
@@ -72,6 +78,9 @@ export async function getRepo(
   }
 ) {
   try {
+    if (typeof req.params.repoId !== "string") {
+      throw new AnonymousError("invalid_path", { httpStatus: 400 });
+    }
     const repo = await db.getRepository(req.params.repoId);
     if (opt.nocheck !== true) {
       // redirect if the repository is expired

src/server/routes/user.ts

@@ -21,7 +21,7 @@ const router = express.Router();
 // user needs to be connected for all user API
 router.use(ensureAuthenticated);
 
-router.get("/logout", async (req: express.Request, res: express.Response) => {
+router.get("/logout", async (req, res) => {
   try {
     req.logout((error) => {
       if (error) {
@@ -34,7 +34,7 @@ router.get("/logout", async (req: express.Request, res: express.Response) => {
   }
 });
 
-router.get("/", async (req: express.Request, res: express.Response) => {
+router.get("/", async (req, res) => {
   try {
     const user = await getUser(req);
     res.json({
@@ -47,7 +47,7 @@ router.get("/", async (req: express.Request, res: express.Response) => {
   }
 });
 
-router.get("/quota", async (req: express.Request, res: express.Response) => {
+router.get("/quota", async (req, res) => {
   try {
     const user = await getUser(req);
     const repositories = (await user.getRepositories()).filter(
@@ -124,7 +124,7 @@ router.get("/quota", async (req: express.Request, res: express.Response) => {
   }
 });
 
-router.get("/default", async (req: express.Request, res: express.Response) => {
+router.get("/default", async (req, res) => {
   try {
     const user = await getUser(req);
 
@@ -134,7 +134,7 @@ router.get("/default", async (req: express.Request, res: express.Response) => {
   }
 });
 
-router.post("/default", async (req: express.Request, res: express.Response) => {
+router.post("/default", async (req, res) => {
   try {
     const user = await getUser(req);
 
@@ -156,7 +156,7 @@ router.post("/default", async (req: express.Request, res: express.Response) => {
 // the user record (#741). The record itself is kept (with a placeholder
 // username) so removed repoIds stay reserved and owner references remain
 // resolvable.
-router.delete("/", async (req: express.Request, res: express.Response) => {
+router.delete("/", async (req, res) => {
   try {
     const user = await getUser(req);
 
@@ -243,7 +243,7 @@ router.delete("/", async (req: express.Request, res: express.Response) => {
 
 router.get(
   "/anonymized_repositories",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     try {
       const user = await getUser(req);
       res.json(
@@ -260,7 +260,7 @@ router.get(
 );
 router.get(
   "/anonymized_gists",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     try {
       const user = await getUser(req);
       res.json(
@@ -275,7 +275,7 @@ router.get(
 );
 router.get(
   "/anonymized_pull_requests",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     try {
       const user = await getUser(req);
       res.json(
@@ -292,7 +292,7 @@ router.get(
 // search GitHub users (used by the coauthor picker)
 router.get(
   "/search/github-users",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     try {
       const user = await getUser(req);
       const q = (req.query.q as string) || "";
@@ -327,7 +327,7 @@ async function getAllRepositories(user: User, force: boolean) {
 }
 router.get(
   "/all_repositories",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     try {
       const user = await getUser(req);
       res.json(await getAllRepositories(user, req.query.force == "1"));
@@ -338,7 +338,7 @@ router.get(
 );
 router.get(
   "/:username/all_repositories",
-  async (req: express.Request, res: express.Response) => {
+  async (req, res) => {
     try {
       const loggedUser = await getUser(req);
       isOwnerOrAdmin([req.params.username], loggedUser);

src/server/routes/webview.ts

@@ -61,7 +61,10 @@ const indexPriority = [
   "readme",
 ];
 
-async function webView(req: express.Request, res: express.Response) {
+async function webView(
+  req: express.Request<{ repoId: string; path?: string[] }>,
+  res: express.Response
+) {
   res.header("Content-Security-Policy", "sandbox allow-popups allow-forms allow-modals");
   const repo = await getRepo(req, res);
   if (!repo) return;
@@ -182,7 +185,7 @@ async function webView(req: express.Request, res: express.Response) {
   }
 }
 
-router.get("/:repoId/*", webView);
+router.get("/:repoId/{*path}", webView);
 router.get("/:repoId", (req: express.Request, res: express.Response) => {
   res.redirect("/w" + req.url + "/");
 });

src/streamer/index.ts

@@ -33,7 +33,7 @@ app.get("/healthcheck", async (_, res) => {
   res.json({ status: "ok" });
 });
 
-app.all("*", (req, res) => {
+app.all("/{*path}", (req, res) => {
   handleError(
     new AnonymousError("file_not_found", {
       httpStatus: 404,
@@ -43,6 +43,7 @@ app.all("*", (req, res) => {
     req
   );
 });
-app.listen(config.PORT, () => {
+app.listen(config.PORT, (error?: Error) => {
+  if (error) throw error;
   logger.info("streamer started", { port: config.PORT });
 });

test/express-routing.test.js

@@ -0,0 +1,87 @@
+const { expect } = require("chai");
+const express = require("express");
+const http = require("node:http");
+require("ts-node/register/transpile-only");
+const routers = require("../src/server/routes").default;
+const routeUtils = require("../src/server/routes/route-utils");
+
+function request(server, path, method = "GET") {
+  return new Promise((resolve, reject) => {
+    const req = http.request({
+      host: "127.0.0.1", port: server.address().port, path, method,
+    }, (res) => {
+      let body = "";
+      res.setEncoding("utf8");
+      res.on("data", chunk => { body += chunk; });
+      res.on("end", () => resolve({ status: res.statusCode, headers: res.headers, body }));
+    });
+    req.on("error", reject);
+    req.end();
+  });
+}
+
+describe("Express 5 route matching", function () {
+  let server;
+  let originals;
+  before(async function () {
+    originals = { getRepo: routeUtils.getRepo, getUser: routeUtils.getUser };
+    routeUtils.getRepo = async (req, res) => {
+      res.json({ repoId: req.params.repoId, path: req.params.path || [] });
+      return null;
+    };
+    routeUtils.getUser = async (req) => {
+      // Stop before database access, after the conference route matched.
+      throw new (require("../src/core/AnonymousError").default)(
+        req.params.conferenceID ? "existing_conference" : "new_conference",
+        { httpStatus: 400 }
+      );
+    };
+    const app = express();
+    app.use((req, _res, next) => { req.isAuthenticated = () => true; next(); });
+    app.use("/api/repo", routers.file);
+    app.use("/w", routers.webview);
+    app.use("/api/conferences", routers.conference);
+    server = await new Promise(resolve => {
+      const listener = app.listen(0, "127.0.0.1", () => resolve(listener));
+    });
+  });
+  after(async function () {
+    Object.assign(routeUtils, originals);
+    if (server) await new Promise(resolve => server.close(resolve));
+  });
+
+  for (const path of ["README.md", "docs/nested/file.md", "docs/%E4%B8%AD%20a%3Fb.md"]) {
+    it(`routes repository file ${path}`, async function () {
+      const response = await request(server, `/api/repo/repo-id/file/${path}`);
+      expect(response.status).to.equal(200);
+      expect(JSON.parse(response.body)).to.deep.equal({
+        repoId: "repo-id", path: path.split("/").map(decodeURIComponent),
+      });
+    });
+  }
+  it("does not route an empty file path", async function () {
+    expect((await request(server, "/api/repo/repo-id/file/")).status).to.equal(404);
+  });
+  it("serves the web view root with a trailing slash", async function () {
+    const response = await request(server, "/w/repo-id/");
+    expect(response.status).to.equal(200);
+    expect(JSON.parse(response.body)).to.deep.equal({ repoId: "repo-id", path: [] });
+  });
+  it("redirects the web view root without a trailing slash", async function () {
+    const response = await request(server, "/w/repo-id");
+    expect(response.status).to.equal(302);
+    expect(response.headers.location).to.equal("/w/repo-id/");
+  });
+  it("routes nested web view files", async function () {
+    const response = await request(server, "/w/repo-id/docs/index.html");
+    expect(response.status).to.equal(200);
+    expect(JSON.parse(response.body).path).to.deep.equal(["docs", "index.html"]);
+  });
+  for (const [path, error] of [["/", "new_conference"], ["/conference-id", "existing_conference"]]) {
+    it(`routes conference POST ${path}`, async function () {
+      const response = await request(server, `/api/conferences${path}`, "POST");
+      expect(response.status).to.equal(400);
+      expect(JSON.parse(response.body).error).to.equal(error);
+    });
+  }
+});