Skip to content

Commit eeabaa8

Browse files
lego-technixlego-technix
authored
feat(api): trap forwarded origin related errors with a dedicated ForwardedOriginError
and return HTTP status code 400 instead of 500 when those errors arise.
1 parent f79ad4e commit eeabaa8

File tree

6 files changed

+135
-67
lines changed

6 files changed

+135
-67
lines changed

‎api/src/identity-access-management/infrastructure/utils/network.js

+32-6
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { HttpErrors } from '../../../shared/application/http-errors.js';
2+
13
const PIX_APP_APPLICATION_NAME = 'app';
24
const PIX_ADMIN_APPLICATION_NAME = 'admin';
35
const PIX_ORGA_APPLICATION_NAME = 'orga';
@@ -22,7 +24,7 @@ function getForwardedOrigin(headers) {
2224
const protoHeader = headers['x-forwarded-proto'];
2325
const hostHeader = headers['x-forwarded-host'];
2426
if (!protoHeader || !hostHeader) {
25-
return '';
27+
throw new ForwardedOriginError('Missing forwarded header(s)');
2628
}
2729

2830
return `${_getHeaderFirstValue(protoHeader)}://${_getHeaderFirstValue(hostHeader)}`;
@@ -61,23 +63,47 @@ class RequestedApplication {
6163
* @returns {RequestedApplication}
6264
*/
6365
static fromOrigin(origin) {
64-
const url = new URL(origin);
66+
let url;
67+
try {
68+
url = new URL(origin);
69+
} catch {
70+
throw new ForwardedOriginError(`Invalid URL: "${origin}"`);
71+
}
72+
6573
let applicationName;
6674

6775
if (url.hostname == 'localhost') {
6876
applicationName = localhostApplicationPortMapping[url.port];
6977
return new RequestedApplication(applicationName);
7078
}
7179

72-
const urlFirstLabel = url.hostname.split('.')[0];
73-
const reviewAppSubPart = urlFirstLabel.split('-')[0];
74-
applicationName = reviewAppSubPart;
80+
const hostnameParts = url.hostname.split('.');
81+
if (hostnameParts.length < 2) {
82+
throw new ForwardedOriginError(`Unsupported hostname: "${url.hostname}"`);
83+
}
84+
85+
const urlFirstLabel = hostnameParts[0];
86+
87+
const urlFirstLabelParts = urlFirstLabel.split('-');
88+
if (urlFirstLabelParts.length == 2) {
89+
const reviewAppSubPart = urlFirstLabelParts[0];
90+
applicationName = reviewAppSubPart;
91+
} else {
92+
applicationName = urlFirstLabel;
93+
}
94+
7595
return new RequestedApplication(applicationName);
7696
}
7797
}
7898

99+
class ForwardedOriginError extends HttpErrors.BadRequestError {
100+
constructor(message) {
101+
super(message);
102+
}
103+
}
104+
79105
function _getHeaderFirstValue(headerValue) {
80106
return headerValue.split(',')[0];
81107
}
82108

83-
export { getForwardedOrigin, RequestedApplication };
109+
export { ForwardedOriginError, getForwardedOrigin, RequestedApplication };

‎api/src/shared/application/healthcheck/healthcheck-controller.js

+6-2
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,12 @@ const checkRedisStatus = async function () {
3939
};
4040

4141
const checkForwardedOriginStatus = async function (request, h) {
42-
const forwardedOrigin = network.getForwardedOrigin(request.headers);
43-
if (!forwardedOrigin) {
42+
let forwardedOrigin;
43+
try {
44+
// network.getForwardedOrigin throws ForwardedOriginError which maps to a HTTP status code 400,
45+
// but for monitoring purpose we want this error to produce a 500.
46+
forwardedOrigin = network.getForwardedOrigin(request.headers);
47+
} catch {
4448
return h.response('Obtaining Forwarded Origin failed').code(500);
4549
}
4650

‎api/src/shared/infrastructure/plugins/pino.js

+7-2
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,14 @@ function requestSerializer(req) {
2222
const context = monitoringTools.getContext();
2323
if (context?.request?.route?.path === '/api/token') {
2424
const { username, refresh_token, grant_type } = context.request.payload || {};
25-
const origin = getForwardedOrigin(context.request.headers);
25+
let origin;
26+
try {
27+
origin = getForwardedOrigin(context.request.headers);
28+
} catch {
29+
origin = '-';
30+
}
31+
enhancedReq.audience = origin;
2632
enhancedReq.grantType = grant_type || '-';
27-
enhancedReq.audience = origin || '-';
2833
enhancedReq.usernameHash = generateHash(username) || '-';
2934
enhancedReq.refreshTokenHash = generateHash(refresh_token) || '-';
3035
}

‎api/tests/identity-access-management/unit/infrastructure/utils/network.test.js

+17-6
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import {
2+
ForwardedOriginError,
23
getForwardedOrigin,
34
RequestedApplication,
45
} from '../../../../../src/identity-access-management/infrastructure/utils/network.js';
@@ -75,15 +76,12 @@ describe('Unit | Identity Access Management | Infrastructure | Utils | network',
7576
});
7677

7778
context('when x-forwarded-proto and x-forwarded-port are not defined', function () {
78-
it('doesn’t choke and returns an empty string', async function () {
79+
it('throws a ForwardedOriginError', function () {
7980
// given
8081
const headers = {};
8182

82-
// when
83-
const origin = getForwardedOrigin(headers);
84-
85-
// then
86-
expect(origin).to.equal('');
83+
// when & then
84+
expect(() => getForwardedOrigin(headers)).to.throw(ForwardedOriginError, 'Missing forwarded header(s)');
8785
});
8886
});
8987
});
@@ -235,6 +233,19 @@ describe('Unit | Identity Access Management | Infrastructure | Utils | network',
235233
});
236234
});
237235
});
236+
237+
context('when the origin is unsupported', function () {
238+
it('throws a ForwardedOriginError', function () {
239+
// given
240+
const origin = 'https://someUnsupportedName';
241+
242+
// when & then
243+
expect(() => RequestedApplication.fromOrigin(origin)).to.throw(
244+
ForwardedOriginError,
245+
'Unsupported hostname: "someunsupportedname"',
246+
);
247+
});
248+
});
238249
});
239250
});
240251
});

‎api/tests/integration/infrastructure/plugins/pino_test.js

+70-42
Original file line numberDiff line numberDiff line change
@@ -150,53 +150,81 @@ describe('Integration | Infrastructure | plugins | pino', function () {
150150
});
151151

152152
context('when calling /api/token', function () {
153-
it('should log the message, version, user id, route, metrics and hashed username', async function () {
154-
// given
155-
const messages = [];
156-
await registerWithPlugin((data) => {
157-
messages.push(data);
153+
context('when there is a username', function () {
154+
it('logs the message, version, user id, route, metrics and hashed username', async function () {
155+
// given
156+
const messages = [];
157+
await registerWithPlugin((data) => {
158+
messages.push(data);
159+
});
160+
161+
const method = 'POST';
162+
const url = '/api/token';
163+
const payload = {
164+
username: 'toto',
165+
};
166+
const headers = { 'x-forwarded-proto': 'https', 'x-forwarded-host': 'app.pix.org' };
167+
168+
// when
169+
const response = await httpTestServer.request(method, url, payload, null, headers);
170+
171+
// then
172+
expect(response.statusCode).to.equal(200);
173+
expect(messages).to.have.lengthOf(1);
174+
expect(messages[0].msg).to.equal('request completed');
175+
expect(messages[0].req.version).to.equal('development');
176+
expect(messages[0].req.user_id).to.equal('-');
177+
expect(messages[0].req.route).to.equal('/api/token');
178+
expect(messages[0].req.usernameHash).to.equal(
179+
'31f7a65e315586ac198bd798b6629ce4903d0899476d5741a9f32e2e521b6a66', // echo -n 'toto'| shasum -a 256
180+
);
158181
});
182+
});
159183

160-
const method = 'POST';
161-
const url = '/api/token';
162-
const payload = {
163-
username: 'toto',
164-
};
165-
166-
// when
167-
const response = await httpTestServer.request(method, url, payload);
168-
// then
169-
expect(response.statusCode).to.equal(200);
170-
expect(messages).to.have.lengthOf(1);
171-
expect(messages[0].msg).to.equal('request completed');
172-
expect(messages[0].req.version).to.equal('development');
173-
expect(messages[0].req.user_id).to.equal('-');
174-
expect(messages[0].req.route).to.equal('/api/token');
175-
expect(messages[0].req.usernameHash).to.equal(
176-
'31f7a65e315586ac198bd798b6629ce4903d0899476d5741a9f32e2e521b6a66', // echo -n 'toto'| shasum -a 256
177-
);
184+
context('when there is no username', function () {
185+
it('logs the message, version, user id, route, metrics and default value for username', async function () {
186+
// given
187+
const messages = [];
188+
await registerWithPlugin((data) => {
189+
messages.push(data);
190+
});
191+
const method = 'POST';
192+
const url = '/api/token';
193+
const headers = { 'x-forwarded-proto': 'https', 'x-forwarded-host': 'app.pix.org' };
194+
195+
// when
196+
const response = await httpTestServer.request(method, url, null, null, headers);
197+
198+
// then
199+
expect(response.statusCode).to.equal(200);
200+
expect(messages).to.have.lengthOf(1);
201+
expect(messages[0].msg).to.equal('request completed');
202+
expect(messages[0].req.version).to.equal('development');
203+
expect(messages[0].req.user_id).to.equal('-');
204+
expect(messages[0].req.route).to.equal('/api/token');
205+
expect(messages[0].req.usernameHash).to.equal('-');
206+
});
178207
});
179208

180-
it('should log the message, version, user id, route, metrics and default value for username when not specified', async function () {
181-
// given
182-
const messages = [];
183-
await registerWithPlugin((data) => {
184-
messages.push(data);
209+
context('when there is no forwarded origin (no x-forwarded headers)', function () {
210+
it('handles the ForwardedOriginError error', async function () {
211+
// given
212+
const messages = [];
213+
await registerWithPlugin((data) => {
214+
messages.push(data);
215+
});
216+
const method = 'POST';
217+
const url = '/api/token';
218+
const noForwardedHeaders = {};
219+
220+
// when
221+
const response = await httpTestServer.request(method, url, null, null, noForwardedHeaders);
222+
223+
// then
224+
expect(response.statusCode).to.equal(200);
225+
expect(messages).to.have.lengthOf(1);
226+
expect(messages[0].msg).to.equal('request completed');
185227
});
186-
const method = 'POST';
187-
const url = '/api/token';
188-
189-
// when
190-
const response = await httpTestServer.request(method, url);
191-
192-
// then
193-
expect(response.statusCode).to.equal(200);
194-
expect(messages).to.have.lengthOf(1);
195-
expect(messages[0].msg).to.equal('request completed');
196-
expect(messages[0].req.version).to.equal('development');
197-
expect(messages[0].req.user_id).to.equal('-');
198-
expect(messages[0].req.route).to.equal('/api/token');
199-
expect(messages[0].req.usernameHash).to.equal('-');
200228
});
201229
});
202230
});

‎api/tests/unit/infrastructure/authentication_test.js

+3-9
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { authentication, validateClientApplication, validateUser } from '../../../lib/infrastructure/authentication.js';
22
import { RevokedUserAccess } from '../../../src/identity-access-management/domain/models/RevokedUserAccess.js';
33
import { revokedUserAccessRepository } from '../../../src/identity-access-management/infrastructure/repositories/revoked-user-access.repository.js';
4+
import { ForwardedOriginError } from '../../../src/identity-access-management/infrastructure/utils/network.js';
45
import { config } from '../../../src/shared/config.js';
56
import { tokenService } from '../../../src/shared/domain/services/token-service.js';
67
import { expect, sinon } from '../../test-helper.js';
@@ -221,7 +222,7 @@ describe('Unit | Infrastructure | Authentication', function () {
221222
aud: 'https://app.pix.fr',
222223
});
223224

224-
// when
225+
// when & then
225226
const { authenticate } = authentication.schemes.jwt.scheme(undefined, {
226227
key: 'dummy-secret',
227228
validate: (decodedAccessToken, options) =>
@@ -230,14 +231,7 @@ describe('Unit | Infrastructure | Authentication', function () {
230231
revokedUserAccessRepository,
231232
}),
232233
});
233-
const response = await authenticate(request, h);
234-
235-
// then
236-
expect(response.output.payload).to.include({
237-
statusCode: 401,
238-
error: 'Unauthorized',
239-
message: 'Unauthorized',
240-
});
234+
await expect(authenticate(request, h)).to.be.rejectedWith(ForwardedOriginError);
241235
});
242236
});
243237

0 commit comments

Comments
 (0)