From eeabaa8fee03859da6c2d232c49a293c37ff3b14 Mon Sep 17 00:00:00 2001 From: LEGO Technix <109212476+lego-technix@users.noreply.github.com> Date: Wed, 5 Mar 2025 16:41:19 +0100 Subject: [PATCH] feat(api): trap forwarded origin related errors with a dedicated ForwardedOriginError and return HTTP status code 400 instead of 500 when those errors arise. --- .../infrastructure/utils/network.js | 38 +++++- .../healthcheck/healthcheck-controller.js | 8 +- api/src/shared/infrastructure/plugins/pino.js | 9 +- .../unit/infrastructure/utils/network.test.js | 23 +++- .../infrastructure/plugins/pino_test.js | 112 +++++++++++------- .../infrastructure/authentication_test.js | 12 +- 6 files changed, 135 insertions(+), 67 deletions(-) diff --git a/api/src/identity-access-management/infrastructure/utils/network.js b/api/src/identity-access-management/infrastructure/utils/network.js index 49e7643f291..0dd205ccd73 100644 --- a/api/src/identity-access-management/infrastructure/utils/network.js +++ b/api/src/identity-access-management/infrastructure/utils/network.js @@ -1,3 +1,5 @@ +import { HttpErrors } from '../../../shared/application/http-errors.js'; + const PIX_APP_APPLICATION_NAME = 'app'; const PIX_ADMIN_APPLICATION_NAME = 'admin'; const PIX_ORGA_APPLICATION_NAME = 'orga'; @@ -22,7 +24,7 @@ function getForwardedOrigin(headers) { const protoHeader = headers['x-forwarded-proto']; const hostHeader = headers['x-forwarded-host']; if (!protoHeader || !hostHeader) { - return ''; + throw new ForwardedOriginError('Missing forwarded header(s)'); } return `${_getHeaderFirstValue(protoHeader)}://${_getHeaderFirstValue(hostHeader)}`; @@ -61,7 +63,13 @@ class RequestedApplication { * @returns {RequestedApplication} */ static fromOrigin(origin) { - const url = new URL(origin); + let url; + try { + url = new URL(origin); + } catch { + throw new ForwardedOriginError(`Invalid URL: "${origin}"`); + } + let applicationName; if (url.hostname == 'localhost') { @@ -69,15 +77,33 @@ class RequestedApplication { return new RequestedApplication(applicationName); } - const urlFirstLabel = url.hostname.split('.')[0]; - const reviewAppSubPart = urlFirstLabel.split('-')[0]; - applicationName = reviewAppSubPart; + const hostnameParts = url.hostname.split('.'); + if (hostnameParts.length < 2) { + throw new ForwardedOriginError(`Unsupported hostname: "${url.hostname}"`); + } + + const urlFirstLabel = hostnameParts[0]; + + const urlFirstLabelParts = urlFirstLabel.split('-'); + if (urlFirstLabelParts.length == 2) { + const reviewAppSubPart = urlFirstLabelParts[0]; + applicationName = reviewAppSubPart; + } else { + applicationName = urlFirstLabel; + } + return new RequestedApplication(applicationName); } } +class ForwardedOriginError extends HttpErrors.BadRequestError { + constructor(message) { + super(message); + } +} + function _getHeaderFirstValue(headerValue) { return headerValue.split(',')[0]; } -export { getForwardedOrigin, RequestedApplication }; +export { ForwardedOriginError, getForwardedOrigin, RequestedApplication }; diff --git a/api/src/shared/application/healthcheck/healthcheck-controller.js b/api/src/shared/application/healthcheck/healthcheck-controller.js index 3f12d36390c..8d5942cf2ce 100644 --- a/api/src/shared/application/healthcheck/healthcheck-controller.js +++ b/api/src/shared/application/healthcheck/healthcheck-controller.js @@ -39,8 +39,12 @@ const checkRedisStatus = async function () { }; const checkForwardedOriginStatus = async function (request, h) { - const forwardedOrigin = network.getForwardedOrigin(request.headers); - if (!forwardedOrigin) { + let forwardedOrigin; + try { + // network.getForwardedOrigin throws ForwardedOriginError which maps to a HTTP status code 400, + // but for monitoring purpose we want this error to produce a 500. + forwardedOrigin = network.getForwardedOrigin(request.headers); + } catch { return h.response('Obtaining Forwarded Origin failed').code(500); } diff --git a/api/src/shared/infrastructure/plugins/pino.js b/api/src/shared/infrastructure/plugins/pino.js index 8b8e7d09f60..7b15fdcea78 100644 --- a/api/src/shared/infrastructure/plugins/pino.js +++ b/api/src/shared/infrastructure/plugins/pino.js @@ -22,9 +22,14 @@ function requestSerializer(req) { const context = monitoringTools.getContext(); if (context?.request?.route?.path === '/api/token') { const { username, refresh_token, grant_type } = context.request.payload || {}; - const origin = getForwardedOrigin(context.request.headers); + let origin; + try { + origin = getForwardedOrigin(context.request.headers); + } catch { + origin = '-'; + } + enhancedReq.audience = origin; enhancedReq.grantType = grant_type || '-'; - enhancedReq.audience = origin || '-'; enhancedReq.usernameHash = generateHash(username) || '-'; enhancedReq.refreshTokenHash = generateHash(refresh_token) || '-'; } diff --git a/api/tests/identity-access-management/unit/infrastructure/utils/network.test.js b/api/tests/identity-access-management/unit/infrastructure/utils/network.test.js index a844b9f0082..f214913cc6d 100644 --- a/api/tests/identity-access-management/unit/infrastructure/utils/network.test.js +++ b/api/tests/identity-access-management/unit/infrastructure/utils/network.test.js @@ -1,4 +1,5 @@ import { + ForwardedOriginError, getForwardedOrigin, RequestedApplication, } from '../../../../../src/identity-access-management/infrastructure/utils/network.js'; @@ -75,15 +76,12 @@ describe('Unit | Identity Access Management | Infrastructure | Utils | network', }); context('when x-forwarded-proto and x-forwarded-port are not defined', function () { - it('doesn’t choke and returns an empty string', async function () { + it('throws a ForwardedOriginError', function () { // given const headers = {}; - // when - const origin = getForwardedOrigin(headers); - - // then - expect(origin).to.equal(''); + // when & then + expect(() => getForwardedOrigin(headers)).to.throw(ForwardedOriginError, 'Missing forwarded header(s)'); }); }); }); @@ -235,6 +233,19 @@ describe('Unit | Identity Access Management | Infrastructure | Utils | network', }); }); }); + + context('when the origin is unsupported', function () { + it('throws a ForwardedOriginError', function () { + // given + const origin = 'https://someUnsupportedName'; + + // when & then + expect(() => RequestedApplication.fromOrigin(origin)).to.throw( + ForwardedOriginError, + 'Unsupported hostname: "someunsupportedname"', + ); + }); + }); }); }); }); diff --git a/api/tests/integration/infrastructure/plugins/pino_test.js b/api/tests/integration/infrastructure/plugins/pino_test.js index dda95545266..06713391f2f 100644 --- a/api/tests/integration/infrastructure/plugins/pino_test.js +++ b/api/tests/integration/infrastructure/plugins/pino_test.js @@ -150,53 +150,81 @@ describe('Integration | Infrastructure | plugins | pino', function () { }); context('when calling /api/token', function () { - it('should log the message, version, user id, route, metrics and hashed username', async function () { - // given - const messages = []; - await registerWithPlugin((data) => { - messages.push(data); + context('when there is a username', function () { + it('logs the message, version, user id, route, metrics and hashed username', async function () { + // given + const messages = []; + await registerWithPlugin((data) => { + messages.push(data); + }); + + const method = 'POST'; + const url = '/api/token'; + const payload = { + username: 'toto', + }; + const headers = { 'x-forwarded-proto': 'https', 'x-forwarded-host': 'app.pix.org' }; + + // when + const response = await httpTestServer.request(method, url, payload, null, headers); + + // then + expect(response.statusCode).to.equal(200); + expect(messages).to.have.lengthOf(1); + expect(messages[0].msg).to.equal('request completed'); + expect(messages[0].req.version).to.equal('development'); + expect(messages[0].req.user_id).to.equal('-'); + expect(messages[0].req.route).to.equal('/api/token'); + expect(messages[0].req.usernameHash).to.equal( + '31f7a65e315586ac198bd798b6629ce4903d0899476d5741a9f32e2e521b6a66', // echo -n 'toto'| shasum -a 256 + ); }); + }); - const method = 'POST'; - const url = '/api/token'; - const payload = { - username: 'toto', - }; - - // when - const response = await httpTestServer.request(method, url, payload); - // then - expect(response.statusCode).to.equal(200); - expect(messages).to.have.lengthOf(1); - expect(messages[0].msg).to.equal('request completed'); - expect(messages[0].req.version).to.equal('development'); - expect(messages[0].req.user_id).to.equal('-'); - expect(messages[0].req.route).to.equal('/api/token'); - expect(messages[0].req.usernameHash).to.equal( - '31f7a65e315586ac198bd798b6629ce4903d0899476d5741a9f32e2e521b6a66', // echo -n 'toto'| shasum -a 256 - ); + context('when there is no username', function () { + it('logs the message, version, user id, route, metrics and default value for username', async function () { + // given + const messages = []; + await registerWithPlugin((data) => { + messages.push(data); + }); + const method = 'POST'; + const url = '/api/token'; + const headers = { 'x-forwarded-proto': 'https', 'x-forwarded-host': 'app.pix.org' }; + + // when + const response = await httpTestServer.request(method, url, null, null, headers); + + // then + expect(response.statusCode).to.equal(200); + expect(messages).to.have.lengthOf(1); + expect(messages[0].msg).to.equal('request completed'); + expect(messages[0].req.version).to.equal('development'); + expect(messages[0].req.user_id).to.equal('-'); + expect(messages[0].req.route).to.equal('/api/token'); + expect(messages[0].req.usernameHash).to.equal('-'); + }); }); - it('should log the message, version, user id, route, metrics and default value for username when not specified', async function () { - // given - const messages = []; - await registerWithPlugin((data) => { - messages.push(data); + context('when there is no forwarded origin (no x-forwarded headers)', function () { + it('handles the ForwardedOriginError error', async function () { + // given + const messages = []; + await registerWithPlugin((data) => { + messages.push(data); + }); + const method = 'POST'; + const url = '/api/token'; + const noForwardedHeaders = {}; + + // when + const response = await httpTestServer.request(method, url, null, null, noForwardedHeaders); + + // then + expect(response.statusCode).to.equal(200); + expect(messages).to.have.lengthOf(1); + expect(messages[0].msg).to.equal('request completed'); }); - const method = 'POST'; - const url = '/api/token'; - - // when - const response = await httpTestServer.request(method, url); - - // then - expect(response.statusCode).to.equal(200); - expect(messages).to.have.lengthOf(1); - expect(messages[0].msg).to.equal('request completed'); - expect(messages[0].req.version).to.equal('development'); - expect(messages[0].req.user_id).to.equal('-'); - expect(messages[0].req.route).to.equal('/api/token'); - expect(messages[0].req.usernameHash).to.equal('-'); }); }); }); diff --git a/api/tests/unit/infrastructure/authentication_test.js b/api/tests/unit/infrastructure/authentication_test.js index 74e53f52ba7..d7739912b3b 100644 --- a/api/tests/unit/infrastructure/authentication_test.js +++ b/api/tests/unit/infrastructure/authentication_test.js @@ -1,6 +1,7 @@ import { authentication, validateClientApplication, validateUser } from '../../../lib/infrastructure/authentication.js'; import { RevokedUserAccess } from '../../../src/identity-access-management/domain/models/RevokedUserAccess.js'; import { revokedUserAccessRepository } from '../../../src/identity-access-management/infrastructure/repositories/revoked-user-access.repository.js'; +import { ForwardedOriginError } from '../../../src/identity-access-management/infrastructure/utils/network.js'; import { config } from '../../../src/shared/config.js'; import { tokenService } from '../../../src/shared/domain/services/token-service.js'; import { expect, sinon } from '../../test-helper.js'; @@ -221,7 +222,7 @@ describe('Unit | Infrastructure | Authentication', function () { aud: 'https://app.pix.fr', }); - // when + // when & then const { authenticate } = authentication.schemes.jwt.scheme(undefined, { key: 'dummy-secret', validate: (decodedAccessToken, options) => @@ -230,14 +231,7 @@ describe('Unit | Infrastructure | Authentication', function () { revokedUserAccessRepository, }), }); - const response = await authenticate(request, h); - - // then - expect(response.output.payload).to.include({ - statusCode: 401, - error: 'Unauthorized', - message: 'Unauthorized', - }); + await expect(authenticate(request, h)).to.be.rejectedWith(ForwardedOriginError); }); });