Fix gateway user verification (#186)

Gateway does not detect the user id param in the path, causing verification to fail and redirect to the frontend address. Let's add a new header to store the current user id, which should be the same as that of the param uid.
CS3219-AY2324S1 · Oct 26, 2023 · 0f81576 · 0f81576
1 parent ae0c485
commit 0f81576
Show file tree

Hide file tree

Showing 7 changed files with 158 additions and 38 deletions.
diff --git a/frontend/src/firebase-client/useDeleteOwnAccount.ts b/frontend/src/firebase-client/useDeleteOwnAccount.ts
@@ -15,6 +15,7 @@ export const useDeleteOwnAccount = () => {
           method: "DELETE",
           headers: {
             "User-Id-Token": idToken,
+            "User-Id": currentUser.uid
           },
         });
         // This will delete the user from the Firebase Authentication database

diff --git a/frontend/src/pages/api/userHandler.ts b/frontend/src/pages/api/userHandler.ts
@@ -3,7 +3,7 @@ import { EditableUser } from "@/types/UserTypes";
 
 export const updateUserByUid = async (user: EditableUser, currentUser: any) => {
   try {
-    const url = `${userApiPathAddress}${currentUser.uid}}`;
+    const url = `${userApiPathAddress}${currentUser.uid}`;
     const idToken = await currentUser.getIdToken(true);
 
     console.log("user", user);
@@ -14,6 +14,7 @@ export const updateUserByUid = async (user: EditableUser, currentUser: any) => {
       headers: {
         "Content-Type": "application/json",
         "User-Id-Token": idToken,
+        "User-Id": currentUser.uid
       },
       body: JSON.stringify(user),
     });

diff --git a/services/gateway/src/auth/auth.ts b/services/gateway/src/auth/auth.ts
@@ -4,6 +4,7 @@ import {frontendAddress} from "../proxied_routes/service_names";
 
 const redirectLink = frontendAddress;
 const userIdTokenHeader = "User-Id-Token";
+const userIdHeader = "User-Id";
 
 export const setupIsLoggedIn = (app : Express, routes : any[]) => {
   routes.forEach(r => {
@@ -32,8 +33,9 @@ export const setupUserIdMatch = (app : Express, routes : any[]) => {
   routes.forEach(r => {
     app.use(r.url, function(req : express.Request, res : express.Response, next : express.NextFunction) {
       if (r.user_match_required_methods.includes(req.method)) {
+        console.log(req.params)
         const idToken = req.get(userIdTokenHeader);
-        const paramUid = req.params.uid;
+        const paramUid = req.get(userIdHeader);
         if (!idToken || !paramUid) {
           res.redirect(redirectLink)
         } else {

diff --git a/services/user-service/openapiDoc.json b/services/user-service/openapiDoc.json
@@ -68,6 +68,9 @@
           "200": {
             "description": "OK"
           },
+          "400": {
+            "description": "Bad Request"
+          },
           "404": {
             "description": "Not Found"
           },
@@ -92,6 +95,9 @@
           "204": {
             "description": "No Content"
           },
+          "400": {
+            "description": "Bad Request"
+          },
           "404": {
             "description": "Not Found"
           },

diff --git a/services/user-service/src/routes/index.ts b/services/user-service/src/routes/index.ts
@@ -41,38 +41,72 @@ indexRouter.get(
 indexRouter.put(
   "/:uid",
   function (req: express.Request, res: express.Response) {
-    userDatabaseFunctions
-      .updateUserByUid(req.params.uid, req.body)
-      .then((result) => {
-        res.status(200).json(result);
-      })
-      .catch((error) => {
-        if (error.code === "P2025") {
-          res.status(404).end();
-        } else {
-          // Server side error such as database not being available
-          res.status(500).end();
-        }
-      });
+    /**
+     * Need to check that header UID was not tampered with.
+     *
+     * Attack Scenario:
+     *   1) User 2 wants to edit profile of user 1.
+     *   2) This should be blocked by the gateway since path param uid = 1 and header uid = 1
+     *      but user 2 only has authentication token for user 2
+     *   3) User 2 could change header uid = 2 to pass authentication, but retain path param uid = 1
+     *      to attempt to change user 1's data
+     *   4) Hence, need to check that param uid = header uid
+     */
+    const pathUid = req.params.uid;
+    const headerUid = req.get("User-Id");
+    if (pathUid !== headerUid) {
+      res.status(400).end();
+    } else {
+      userDatabaseFunctions
+        .updateUserByUid(req.params.uid, req.body)
+        .then((result) => {
+          res.status(200).json(result);
+        })
+        .catch((error) => {
+          if (error.code === "P2025") {
+            res.status(404).end();
+          } else {
+            // Server side error such as database not being available
+            res.status(500).end();
+          }
+        });
+    }
   }
 );
 
 indexRouter.delete(
   "/:uid",
   function (req: express.Request, res: express.Response) {
-    userDatabaseFunctions
-      .deleteUserByUid(req.params.uid)
-      .then(() => {
-        res.status(204).end();
-      })
-      .catch((error) => {
-        if (error.code === "P2025") {
-          res.status(404).end();
-        } else {
-          // Server side error such as database not being available
-          res.status(500).end();
-        }
-      });
+    /**
+     * Need to check that header UID was not tampered with.
+     *
+     * Attack Scenario:
+     *   1) User 2 wants to edit profile of user 1.
+     *   2) This should be blocked by the gateway since path param uid = 1 and header uid = 1
+     *      but user 2 only has authentication token for user 2
+     *   3) User 2 could change header uid = 2 to pass authentication, but retain path param uid = 1
+     *      to attempt to change user 1's data
+     *   4) Hence, need to check that param uid = header uid
+     */
+    const pathUid = req.params.uid;
+    const headerUid = req.get("User-Id");
+    if (pathUid !== headerUid) {
+      res.status(400).end();
+    } else {
+      userDatabaseFunctions
+        .deleteUserByUid(req.params.uid)
+        .then(() => {
+          res.status(204).end();
+        })
+        .catch((error) => {
+          if (error.code === "P2025") {
+            res.status(404).end();
+          } else {
+            // Server side error such as database not being available
+            res.status(500).end();
+          }
+        })
+    }
   }
 );
 

diff --git a/services/user-service/systemtest/app.test.ts b/services/user-service/systemtest/app.test.ts
@@ -12,6 +12,8 @@ const updatedNewUser = { uid: '1', displayName: 'Test User', photoUrl: "fakeUrl"
 
 const updatePayload = { matchDifficulty: 1 };
 
+const userIdHeader = "User-Id";
+
 describe('/index', () => {
   describe('Sample App Workflow', () => {
     it('Step 1: Add user 1 to database should pass with status 201', async () => {
@@ -28,9 +30,21 @@ describe('/index', () => {
       expect(response.body).toStrictEqual(fullNewUser);
     })
 
+    it('Step 3a: Update details of user 1 from database by user 2 should fail with error 400', async () => {
+      // The function being tested
+      const response = await request(app)
+        .put('/api/user-service/1')
+        .set(userIdHeader, "2")
+        .send(updatePayload);
+      expect(response.status).toStrictEqual(400);
+    })
+
     it('Step 3: Update details of user 1 from database should pass', async () => {
       // The function being tested
-      const response = await request(app).put('/api/user-service/1').send(updatePayload);
+      const response = await request(app)
+        .put('/api/user-service/1')
+        .set(userIdHeader, "1")
+        .send(updatePayload);
       expect(response.status).toStrictEqual(200);
       expect(response.body).toStrictEqual(updatedNewUser);
     })
@@ -47,8 +61,19 @@ describe('/index', () => {
       expect(response.status).toStrictEqual(200);
     })
 
+    it('Step 6a: Delete user 1 from database by user 2 should fail with status 400', async () => {
+      const response = await request(app)
+        .delete('/api/user-service/1')
+        .set(userIdHeader, "2")
+        .send();
+      expect(response.status).toStrictEqual(400);
+    })
+
     it('Step 6: Delete user 1 from database', async () => {
-      const response = await request(app).delete('/api/user-service/1').send();
+      const response = await request(app)
+        .delete('/api/user-service/1')
+        .set(userIdHeader, "1")
+        .send();
       expect(response.status).toStrictEqual(204);
     })
 
@@ -60,13 +85,19 @@ describe('/index', () => {
 
     it('Step 8: Update details of now deleted user 1 should fail', async () => {
       // The function being tested
-      const response = await request(app).put('/api/user-service/1').send(updatePayload);
+      const response = await request(app)
+        .put('/api/user-service/1')
+        .set(userIdHeader, "1")
+        .send(updatePayload);
       expect(response.status).toStrictEqual(404);
     })
 
     it('Step 9: Deleting the now deleted user 1 should fail', async () => {
       // The function being tested
-      const response = await request(app).delete('/api/user-service/1').send();
+      const response = await request(app)
+        .delete('/api/user-service/1')
+        .set(userIdHeader, "1")
+        .send();
       expect(response.status).toStrictEqual(404);
     })
   })

diff --git a/services/user-service/test/routes/index.test.ts b/services/user-service/test/routes/index.test.ts
@@ -9,6 +9,7 @@ import request from 'supertest';
 vi.mock('../../src/db/functions')
 
 const app = express();
+const userIdHeader = "User-Id";
 app.use(indexRouter);
 
 const fullNewUser = { uid: '1', displayName: 'Test User', photoUrl: "fakeUrl", matchDifficulty: 0,
@@ -93,7 +94,10 @@ describe('/index', () => {
       userDatabaseFunctionsMock.updateUserByUid.mockResolvedValueOnce(fullNewUser);
 
       // The function being tested
-      const response = await request(app).put('/1').send();
+      const response = await request(app)
+        .put('/1')
+        .set(userIdHeader, "1")
+        .send();
       expect(response.status).toStrictEqual(200);
       expect(response.body).toStrictEqual(fullNewUser);
     })
@@ -106,7 +110,10 @@ describe('/index', () => {
       }));
 
       // The function being tested
-      const response = await request(app).put('/1').send();
+      const response = await request(app)
+        .put('/1')
+        .set(userIdHeader, "1")
+        .send();
       expect(response.status).toStrictEqual(404);
     })
 
@@ -115,9 +122,25 @@ describe('/index', () => {
       userDatabaseFunctionsMock.updateUserByUid.mockRejectedValueOnce(new Error());
 
       // The function being tested
-      const response = await request(app).put('/1').send();
+      const response = await request(app)
+        .put('/1')
+        .set(userIdHeader, "1")
+        .send();
       expect(response.status).toStrictEqual(500);
     })
+
+    it('[PUT] /1 but the header UID does not match path param UID', async () => {
+
+      // Used to get back the user
+      userDatabaseFunctionsMock.updateUserByUid.mockResolvedValueOnce(fullNewUser);
+
+      // The function being tested
+      const response = await request(app)
+        .put('/1')
+        .set(userIdHeader, "2")
+        .send();
+      expect(response.status).toStrictEqual(400);
+    })
   })
 
   describe('deleteUserByUid', () => {
@@ -127,7 +150,10 @@ describe('/index', () => {
       userDatabaseFunctionsMock.deleteUserByUid.mockResolvedValueOnce(fullNewUser);
 
       // The function being tested
-      const response = await request(app).delete('/1').send();
+      const response = await request(app)
+        .delete('/1')
+        .set(userIdHeader, "1")
+        .send();
       expect(response.status).toStrictEqual(204);
     })
 
@@ -139,7 +165,10 @@ describe('/index', () => {
       }));
 
       // The function being tested
-      const response = await request(app).delete('/1').send();
+      const response = await request(app)
+        .delete('/1')
+        .set(userIdHeader, "1")
+        .send();
       expect(response.status).toStrictEqual(404);
     })
 
@@ -148,8 +177,24 @@ describe('/index', () => {
       userDatabaseFunctionsMock.deleteUserByUid.mockRejectedValueOnce(new Error());
 
       // The function being tested
-      const response = await request(app).delete('/1').send();
+      const response = await request(app)
+        .delete('/1')
+        .set(userIdHeader, "1")
+        .send();
       expect(response.status).toStrictEqual(500);
     })
+
+    it('[DELETE] /1 but the header UID does not match path param UID', async () => {
+
+      // Used to get back the user
+      userDatabaseFunctionsMock.deleteUserByUid.mockResolvedValueOnce(fullNewUser);
+
+      // The function being tested
+      const response = await request(app)
+        .delete('/1')
+        .set(userIdHeader, "2")
+        .send();
+      expect(response.status).toStrictEqual(400);
+    })
   })
 })