From df3182efbda02a16456afd51ccfca6f727f09172 Mon Sep 17 00:00:00 2001 From: seaerchin <44049504+seaerchin@users.noreply.github.com> Date: Thu, 23 Feb 2023 12:25:23 +0800 Subject: [PATCH] fix(server): shift api logger to be initialised within each subrouter (#621) * fix(server): shift api logger to be initialised within each subrouter * refactor(auth): add logging back into `/auth` endpoint this will log the ip with a username of `undefined` but this is expected so that we can monitor and potentially block malicious actors who are trying to ddos us --- src/routes/v1/authenticated/index.js | 10 ++++++++-- src/routes/v1/authenticatedSites/index.js | 5 ++++- src/routes/v2/auth.js | 4 +++- src/routes/v2/authenticated/index.js | 6 ++++-- src/routes/v2/authenticatedSites/index.js | 4 ++++ src/server.js | 11 ++++++----- 6 files changed, 29 insertions(+), 11 deletions(-) diff --git a/src/routes/v1/authenticated/index.js b/src/routes/v1/authenticated/index.js index 8701aa02e..115832aee 100644 --- a/src/routes/v1/authenticated/index.js +++ b/src/routes/v1/authenticated/index.js @@ -3,14 +3,20 @@ const express = require("express") const sitesRouter = require("@routes/v1/authenticated/sites") const { UsersRouter } = require("@routes/v2/authenticated/users") -const getAuthenticatedSubrouter = ({ authMiddleware, usersService }) => { +const getAuthenticatedSubrouter = ({ + authMiddleware, + usersService, + apiLogger, +}) => { // Workaround - no v1 users router exists const usersRouter = new UsersRouter({ usersService }) const authenticatedSubrouter = express.Router({ mergeParams: true }) authenticatedSubrouter.use(authMiddleware.verifyJwt) - + // NOTE: apiLogger needs to be after `verifyJwt` as it logs the github username + // which is only available after verifying that the jwt is valid + authenticatedSubrouter.use(apiLogger) authenticatedSubrouter.use("/sites", sitesRouter) authenticatedSubrouter.use("/user", usersRouter.getRouter()) diff --git a/src/routes/v1/authenticatedSites/index.js b/src/routes/v1/authenticatedSites/index.js index 43eb1854b..31ce5d616 100644 --- a/src/routes/v1/authenticatedSites/index.js +++ b/src/routes/v1/authenticatedSites/index.js @@ -16,11 +16,14 @@ const resourceRoomRouter = require("@routes/v1/authenticatedSites/resourceRoom") const resourcesRouter = require("@routes/v1/authenticatedSites/resources") const settingsRouter = require("@routes/v1/authenticatedSites/settings") -const getAuthenticatedSitesSubrouter = ({ authMiddleware }) => { +const getAuthenticatedSitesSubrouter = ({ authMiddleware, apiLogger }) => { const authenticatedSitesSubrouter = express.Router({ mergeParams: true }) authenticatedSitesSubrouter.use(authMiddleware.verifyJwt) authenticatedSitesSubrouter.use(authMiddleware.useSiteAccessTokenIfAvailable) + // NOTE: apiLogger needs to be after `verifyJwt` as it logs the github username + // which is only available after verifying that the jwt is valid + authenticatedSitesSubrouter.use(apiLogger) authenticatedSitesSubrouter.use("/pages", pagesRouter) authenticatedSitesSubrouter.use("/collections", collectionsRouter) diff --git a/src/routes/v2/auth.js b/src/routes/v2/auth.js index 27f3cd64b..1ddc9ad9f 100644 --- a/src/routes/v2/auth.js +++ b/src/routes/v2/auth.js @@ -16,9 +16,10 @@ const CSRF_COOKIE_NAME = "isomer-csrf" const COOKIE_NAME = "isomercms" class AuthRouter { - constructor({ authService, authMiddleware }) { + constructor({ authService, authMiddleware, apiLogger }) { this.authService = authService this.authMiddleware = authMiddleware + this.apiLogger = apiLogger // We need to bind all methods because we don't invoke them from the class directly autoBind(this) } @@ -90,6 +91,7 @@ class AuthRouter { getRouter() { const router = express.Router() + router.use(this.apiLogger) router.get( "/github-redirect", diff --git a/src/routes/v2/authenticated/index.js b/src/routes/v2/authenticated/index.js index b118eb102..fb042d709 100644 --- a/src/routes/v2/authenticated/index.js +++ b/src/routes/v2/authenticated/index.js @@ -1,5 +1,3 @@ -import InfraService from "@services/infra/InfraService" - const express = require("express") const { @@ -16,6 +14,7 @@ const getAuthenticatedSubrouter = ({ gitHubService, configYmlService, usersService, + apiLogger, }) => { const sitesService = new SitesService({ gitHubService, configYmlService }) const netlifyTomlService = new NetlifyTomlService() @@ -27,6 +26,9 @@ const getAuthenticatedSubrouter = ({ const authenticatedSubrouter = express.Router({ mergeParams: true }) authenticatedSubrouter.use(authMiddleware.verifyJwt) + // NOTE: apiLogger needs to be after `verifyJwt` as it logs the github username + // which is only available after verifying that the jwt is valid + authenticatedSubrouter.use(apiLogger) authenticatedSubrouter.use("/sites", sitesV2Router.getRouter()) authenticatedSubrouter.use("/user", usersRouter.getRouter()) diff --git a/src/routes/v2/authenticatedSites/index.js b/src/routes/v2/authenticatedSites/index.js index 82d578056..fa1698b46 100644 --- a/src/routes/v2/authenticatedSites/index.js +++ b/src/routes/v2/authenticatedSites/index.js @@ -85,6 +85,7 @@ const getAuthenticatedSitesSubrouter = ({ authMiddleware, gitHubService, configYmlService, + apiLogger, }) => { const collectionYmlService = new CollectionYmlService({ gitHubService }) const homepagePageService = new HomepagePageService({ gitHubService }) @@ -186,6 +187,9 @@ const getAuthenticatedSitesSubrouter = ({ authenticatedSitesSubrouter.use(authMiddleware.verifyJwt) authenticatedSitesSubrouter.use(authMiddleware.useSiteAccessTokenIfAvailable) + // NOTE: apiLogger needs to be after `verifyJwt` as it logs the github username + // which is only available after verifying that the jwt is valid + authenticatedSitesSubrouter.use(apiLogger) authenticatedSitesSubrouter.use( "/collections/:collectionName", diff --git a/src/server.js b/src/server.js index 3ebd54045..d2190ed32 100644 --- a/src/server.js +++ b/src/server.js @@ -28,6 +28,7 @@ import QueueService from "@services/identity/QueueService" import ReposService from "@services/identity/ReposService" import InfraService from "@services/infra/InfraService" +import { apiLogger } from "./middleware/apiLogger" import getAuthenticatedSubrouterV1 from "./routes/v1/authenticated" import getAuthenticatedSitesSubrouterV1 from "./routes/v1/authenticatedSites" import getAuthenticatedSubrouter from "./routes/v2/authenticated" @@ -62,7 +63,6 @@ const { FRONTEND_URL } = process.env // Import middleware // Import routes -const { apiLogger } = require("@middleware/apiLogger") const { errorHandler } = require("@middleware/errorHandler") const { FormsgRouter } = require("@routes/formsgSiteCreation") @@ -109,9 +109,11 @@ const authMiddleware = getAuthMiddleware({ identityAuthService }) const authenticatedSubrouterV1 = getAuthenticatedSubrouterV1({ authMiddleware, usersService, + apiLogger, }) const authenticatedSitesSubrouterV1 = getAuthenticatedSitesSubrouterV1({ authMiddleware, + apiLogger, }) const authenticatedSubrouterV2 = getAuthenticatedSubrouter({ @@ -121,13 +123,15 @@ const authenticatedSubrouterV2 = getAuthenticatedSubrouter({ usersService, reposService, deploymentsService, + apiLogger, }) const authenticatedSitesSubrouterV2 = getAuthenticatedSitesSubrouter({ authMiddleware, gitHubService, configYmlService, + apiLogger, }) -const authV2Router = new AuthRouter({ authMiddleware, authService }) +const authV2Router = new AuthRouter({ authMiddleware, authService, apiLogger }) const formsgRouter = new FormsgRouter({ usersService, infraService }) const formsgSiteLaunchRouter = new FormsgSiteLaunchRouter({ usersService, @@ -148,9 +152,6 @@ app.use(express.urlencoded({ extended: false })) app.use(cookieParser()) app.use(express.static(path.join(__dirname, "public"))) -// Log api requests -app.use(apiLogger) - // Health endpoint app.use("/v2/ping", (req, res, next) => res.status(200).send("Ok"))