From 460d34d659022ca03b13ab352a40d78139bbb8c9 Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Thu, 13 Feb 2025 15:04:30 +0200 Subject: [PATCH 1/3] Redirect to app root if an unsupported auth endpoint is called For some reason we sometimes receive GET requests to SAML login endpoints, but there's no handler for the request ends up getting proxied to evaka-service, which logs noisy errors. It's better to just redirect to the corresponding app root in apigw, because all valid auth routes are there so we know any other routes don't exist and will just lead to an error later. --- apigw/src/app.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/apigw/src/app.ts b/apigw/src/app.ts index 1e591cc8f2b..bfc978b4582 100644 --- a/apigw/src/app.ts +++ b/apigw/src/app.ts @@ -321,6 +321,7 @@ export function apiRouter(config: Config, redisClient: RedisClient) { redisClient ) ) + router.all('/citizen/auth/*', (_, res) => res.redirect('/')) router.use('/citizen/public/map-api', mapRoutes) router.all('/citizen/public/*', citizenProxy) router.all('/citizen/*', citizenSessions.requireAuthentication, citizenProxy) @@ -341,6 +342,7 @@ export function apiRouter(config: Config, redisClient: RedisClient) { router.use('/employee/', employeeSessions.middleware) router.get('/employee/auth/status', internalAuthStatus(employeeSessions)) + router.all('/employee/auth/*', (_, res) => res.redirect('/employee')) router.all('/employee/public/*', employeeProxy) router.all( '/employee/*', @@ -376,6 +378,9 @@ export function apiRouter(config: Config, redisClient: RedisClient) { express.json(), pinLogoutRequestHandler(employeeMobileSessions, redisClient) ) + router.all('/employee-mobile/auth/*', (_, res) => + res.redirect('/employee/mobile') + ) router.all('/employee-mobile/public/*', employeeMobileProxy) router.all( '/employee-mobile/*', From aede4d079fc19629c6872a75777c391ab748cbce Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Thu, 13 Feb 2025 15:08:34 +0200 Subject: [PATCH 2/3] Stop logging anti-CSRF errors These errors are noisy, and seem to nowadays happen only when some bots hammer our APIs with random requests. --- apigw/src/shared/middleware/error-handler.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/apigw/src/shared/middleware/error-handler.ts b/apigw/src/shared/middleware/error-handler.ts index 56c16192294..7ab2e140109 100644 --- a/apigw/src/shared/middleware/error-handler.ts +++ b/apigw/src/shared/middleware/error-handler.ts @@ -19,7 +19,6 @@ interface LogResponse { export const errorHandler: (v: boolean) => ErrorRequestHandler = (includeErrorMessage: boolean) => (error, req, res, next) => { if (error instanceof InvalidAntiCsrfToken) { - logError('Anti-CSRF token error', req, undefined, error) if (!res.headersSent) { res .status(403) From a46eef24e3627ba572bc4a8fb5c5ff95c6d1d8a8 Mon Sep 17 00:00:00 2001 From: Joonas Javanainen Date: Thu, 13 Feb 2025 15:15:19 +0200 Subject: [PATCH 3/3] Rearrange SAML login message validation - move to a separate function - call the function inside try/catch -> errors are handled instead of thrown to generic error middleware --- apigw/src/shared/routes/saml.ts | 52 ++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 23 deletions(-) diff --git a/apigw/src/shared/routes/saml.ts b/apigw/src/shared/routes/saml.ts index 37243b22046..de2998ea3a0 100755 --- a/apigw/src/shared/routes/saml.ts +++ b/apigw/src/shared/routes/saml.ts @@ -104,6 +104,30 @@ export function createSamlIntegration( return samlMessage.profile } + const validateSamlLogoutMessage = async ( + req: express.Request + ): Promise => { + let samlMessage: { profile: Profile | null; loggedOut: boolean } + if (req.method === 'GET') { + const originalQuery = url.parse(req.url).query ?? '' + samlMessage = await saml.validateRedirectAsync(req.query, originalQuery) + } else if (req.method === 'POST') { + samlMessage = isSamlPostRequest(req) + ? // eslint-disable-next-line @typescript-eslint/no-unsafe-argument + await saml.validatePostRequestAsync(req.body) + : // eslint-disable-next-line @typescript-eslint/no-unsafe-argument + await saml.validatePostResponseAsync(req.body) + } else { + throw new SamlError(`Unsupported HTTP method ${req.method}`) + } + if (!samlMessage.loggedOut) { + throw new SamlError( + 'Invalid SAML message type: expected logout request/response' + ) + } + return samlMessage.profile + } + const login: AsyncRequestHandler = async (req, res) => { logAuditEvent(eventCode('sign_in_started'), req, 'Login endpoint called') try { @@ -234,33 +258,15 @@ export function createSamlIntegration( const logoutCallback: AsyncRequestHandler = async (req, res) => { logAuditEvent(eventCode('sign_out'), req, 'Logout callback called') - let samlMessage: { profile: Profile | null; loggedOut: boolean } - if (req.method === 'GET') { - const originalQuery = url.parse(req.url).query ?? '' - samlMessage = await saml.validateRedirectAsync(req.query, originalQuery) - } else if (req.method === 'POST') { - samlMessage = isSamlPostRequest(req) - ? // eslint-disable-next-line @typescript-eslint/no-unsafe-argument - await saml.validatePostRequestAsync(req.body) - : // eslint-disable-next-line @typescript-eslint/no-unsafe-argument - await saml.validatePostResponseAsync(req.body) - } else { - throw new SamlError(`Unsupported HTTP method ${req.method}`) - } - if (!samlMessage.loggedOut) { - throw new SamlError( - 'Invalid SAML message type: expected logout request/response' - ) - } - try { + const profile = await validateSamlLogoutMessage(req) let url: string // There are two scenarios: // 1. IDP-initiated logout, and we've just received a logout request -> profile is not null, the SAML transaction // is still in progress, and we should redirect the user back to the IDP // 2. SP-initiated logout, and we've just received a logout response -> profile is null, the SAML transaction // is complete, and we should redirect the user to some meaningful page - if (samlMessage.profile) { + if (profile) { let user: unknown const sessionUser = sessions.getUser(req) if (sessionUser) { @@ -271,16 +277,16 @@ export function createSamlIntegration( } else { // We're possibly doing SLO without a real session (e.g. browser has // 3rd party cookies disabled) - const logoutToken = createLogoutToken(samlMessage.profile) + const logoutToken = createLogoutToken(profile) const sessionUser = await sessions.logoutWithToken(logoutToken) const userId = SamlProfileIdSchema.safeParse(sessionUser) user = userId.success ? userId.data : undefined } - const profileId = SamlProfileIdSchema.safeParse(samlMessage.profile) + const profileId = SamlProfileIdSchema.safeParse(profile) const success = profileId.success && _.isEqual(user, profileId.data) url = await saml.getLogoutResponseUrlAsync( - samlMessage.profile, + profile, // not validated, because the value and its format are specified by the IDP and we're supposed to // just pass it back getRawUnvalidatedRelayState(req) ?? '',