Skip to content

Commit 8d1183d

Browse files
kriswestkriswest
committed
fix: improve handling of errors in proxy filter functions and 403 errors on block
1 parent db3840b commit 8d1183d

File tree

3 files changed

+89
-53
lines changed

3 files changed

+89
-53
lines changed

‎src/proxy/routes/index.ts‎

Lines changed: 30 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -29,14 +29,19 @@ const logAction = (
2929
const proxyFilter: ProxyOptions['filter'] = async (req, res) => {
3030
try {
3131
const urlComponents = processUrlPath(req.url);
32-
3332
if (
3433
!urlComponents ||
3534
urlComponents.gitPath === undefined ||
3635
!validGitRequest(urlComponents.gitPath, req.headers)
3736
) {
38-
res.status(400).send('Invalid request received');
39-
console.log('action blocked');
37+
logAction(
38+
req.url,
39+
req.headers.host,
40+
req.headers['user-agent'],
41+
'Invalid request received',
42+
null,
43+
);
44+
res.status(400).send(handleMessage('Invalid request received'));
4045
return false;
4146
}
4247

@@ -51,17 +56,7 @@ const proxyFilter: ProxyOptions['filter'] = async (req, res) => {
5156
res.set('x-frame-options', 'DENY');
5257
res.set('connection', 'close');
5358

54-
let message = '';
55-
56-
if (action.error) {
57-
message = action.errorMessage!;
58-
console.error(message);
59-
}
60-
if (action.blocked) {
61-
message = action.blockedMessage!;
62-
}
63-
64-
const packetMessage = handleMessage(message);
59+
const packetMessage = handleMessage(action.errorMessage ?? action.blockedMessage ?? '');
6560

6661
logAction(
6762
req.url,
@@ -70,9 +65,7 @@ const proxyFilter: ProxyOptions['filter'] = async (req, res) => {
7065
action.errorMessage,
7166
action.blockedMessage,
7267
);
73-
74-
res.status(200).send(packetMessage);
75-
68+
res.status(403).send(packetMessage);
7669
return false;
7770
}
7871

@@ -84,16 +77,20 @@ const proxyFilter: ProxyOptions['filter'] = async (req, res) => {
8477
action.blockedMessage,
8578
);
8679

80+
// this is the only case where we do not respond directly, instead we return true to proxy the request
8781
return true;
8882
} catch (e) {
89-
console.error('Error occurred in proxy filter function ', e);
83+
const packetMessage = handleMessage(`Error occurred in proxy filter function ${e}`);
84+
9085
logAction(
9186
req.url,
9287
req.headers.host,
9388
req.headers['user-agent'],
9489
'Error occurred in proxy filter function: ' + ((e as Error).message ?? e),
9590
null,
9691
);
92+
93+
res.status(500).send(packetMessage);
9794
return false;
9895
}
9996
};
@@ -140,7 +137,9 @@ const isPackPost = (req: Request) =>
140137
/^(?:\/[^/]+)*\/[^/]+\.git\/(?:git-upload-pack|git-receive-pack)$/.test(req.url);
141138

142139
const teeAndValidate = async (req: Request, res: Response, next: NextFunction) => {
143-
if (!isPackPost(req)) return next();
140+
if (!isPackPost(req)) {
141+
return next();
142+
}
144143

145144
const proxyStream = new PassThrough();
146145
const pluginStream = new PassThrough();
@@ -152,17 +151,16 @@ const teeAndValidate = async (req: Request, res: Response, next: NextFunction) =
152151
const buf = await getRawBody(pluginStream, { limit: '1gb' });
153152
(req as any).body = buf;
154153
const verdict = await executeChain(req, res);
155-
console.log('action processed');
156154
if (verdict.error || verdict.blocked) {
157-
let msg = '';
155+
const msg = verdict.errorMessage ?? verdict.blockedMessage ?? '';
158156

159-
if (verdict.error) {
160-
msg = verdict.errorMessage!;
161-
console.error(msg);
162-
}
163-
if (verdict.blocked) {
164-
msg = verdict.blockedMessage!;
165-
}
157+
logAction(
158+
req.url,
159+
req.headers?.host,
160+
req.headers?.['user-agent'],
161+
verdict.errorMessage,
162+
verdict.blockedMessage,
163+
);
166164

167165
res
168166
.set({
@@ -174,7 +172,7 @@ const teeAndValidate = async (req: Request, res: Response, next: NextFunction) =
174172
'x-frame-options': 'DENY',
175173
connection: 'close',
176174
})
177-
.status(200)
175+
.status(403)
178176
.send(handleMessage(msg));
179177
return;
180178
}
@@ -233,8 +231,9 @@ const getRouter = async () => {
233231
console.log('proxy keys registered: ', JSON.stringify(proxyKeys));
234232

235233
router.use('/', (req, res, next) => {
236-
console.log(`processing request URL: '${req.url}'`);
237-
console.log('proxy keys registered: ', JSON.stringify(proxyKeys));
234+
console.log(
235+
`processing request URL: '${req.url}' against registered proxy keys: ${JSON.stringify(proxyKeys)}`,
236+
);
238237

239238
for (let i = 0; i < proxyKeys.length; i++) {
240239
if (req.url.startsWith(proxyKeys[i])) {

‎test/teeAndValidation.test.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ describe('teeAndValidate middleware', () => {
5656
expect(next.called).to.be.false;
5757

5858
expect(res.set.called).to.be.true;
59-
expect(res.status.calledWith(200)).to.be.true;
59+
expect(res.status.calledWith(403)).to.be.true;
6060
expect(res.send.calledWith(handleMessage('denied!'))).to.be.true;
6161
});
6262

‎test/testProxyRoute.test.js‎

Lines changed: 58 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,26 @@ import Proxy from '../src/proxy';
1818
const TEST_DEFAULT_REPO = {
1919
url: 'https://github.com/finos/git-proxy.git',
2020
name: 'git-proxy',
21-
project: 'finos/gitproxy',
21+
project: 'finos/git-proxy',
2222
host: 'github.com',
23+
proxyUrlPrefix: '/github.com/finos/git-proxy.git',
2324
};
2425

2526
const TEST_GITLAB_REPO = {
2627
url: 'https://gitlab.com/gitlab-community/meta.git',
2728
name: 'gitlab',
2829
project: 'gitlab-community/meta',
2930
host: 'gitlab.com',
30-
proxyUrlPrefix: 'gitlab.com/gitlab-community/meta.git',
31+
proxyUrlPrefix: '/gitlab.com/gitlab-community/meta.git',
32+
};
33+
34+
const TEST_UNKNOWN_REPO = {
35+
url: 'https://github.com/finos/fdc3.git',
36+
name: 'fdc3',
37+
project: 'finos/fdc3',
38+
host: 'github.com',
39+
proxyUrlPrefix: '/github.com/finos/fdc3.git',
40+
fallbackUrlPrefix: '/finos/fdc3.git',
3141
};
3242

3343
describe('proxy route filter middleware', () => {
@@ -42,6 +52,10 @@ describe('proxy route filter middleware', () => {
4252
sinon.restore();
4353
});
4454

55+
after(() => {
56+
sinon.restore();
57+
});
58+
4559
it('should reject invalid git requests with 400', async () => {
4660
const res = await chai
4761
.request(app)
@@ -50,7 +64,7 @@ describe('proxy route filter middleware', () => {
5064
.set('accept', 'application/x-git-upload-pack-request');
5165

5266
expect(res).to.have.status(400);
53-
expect(res.text).to.equal('Invalid request received');
67+
expect(res.text).to.contain('Invalid request received');
5468
});
5569

5670
it('should handle blocked requests and return custom packet message', async () => {
@@ -68,7 +82,7 @@ describe('proxy route filter middleware', () => {
6882
.send(Buffer.from('0000'))
6983
.buffer();
7084

71-
expect(res.status).to.equal(200);
85+
expect(res.status).to.equal(403);
7286
expect(res.text).to.contain('You shall not push!');
7387
expect(res.headers['content-type']).to.include('application/x-git-receive-pack-result');
7488
expect(res.headers['x-frame-options']).to.equal('DENY');
@@ -321,13 +335,6 @@ describe('proxy express application', async () => {
321335
};
322336

323337
before(async () => {
324-
// pass through requests
325-
sinon.stub(chain, 'executeChain').resolves({
326-
blocked: false,
327-
blockedMessage: '',
328-
error: false,
329-
});
330-
331338
// start the API and proxy
332339
proxy = new Proxy();
333340
apiApp = await service.start(proxy);
@@ -364,7 +371,7 @@ describe('proxy express application', async () => {
364371
// proxy a fetch request
365372
const res = await chai
366373
.request(proxy.getExpressApp())
367-
.get('/github.com/finos/git-proxy.git/info/refs?service=git-upload-pack')
374+
.get(`${TEST_DEFAULT_REPO.proxyUrlPrefix}/info/refs?service=git-upload-pack`)
368375
.set('user-agent', 'git/2.42.0')
369376
.set('accept', 'application/x-git-upload-pack-request')
370377
.buffer();
@@ -373,11 +380,11 @@ describe('proxy express application', async () => {
373380
expect(res.text).to.contain('git-upload-pack');
374381
});
375382

376-
it('should proxy requests for the default GitHub repository using the backwards compatibility URL', async function () {
383+
it('should proxy requests for the default GitHub repository using the fallback URL', async function () {
377384
// proxy a fetch request using a fallback URL
378385
const res = await chai
379386
.request(proxy.getExpressApp())
380-
.get('/finos/git-proxy.git/info/refs?service=git-upload-pack')
387+
.get(`${TEST_DEFAULT_REPO.proxyUrlPrefix}/info/refs?service=git-upload-pack`)
381388
.set('user-agent', 'git/2.42.0')
382389
.set('accept', 'application/x-git-upload-pack-request')
383390
.buffer();
@@ -415,7 +422,7 @@ describe('proxy express application', async () => {
415422
// proxy a request to the new repo
416423
const res2 = await chai
417424
.request(proxy.getExpressApp())
418-
.get(`/${TEST_GITLAB_REPO.proxyUrlPrefix}/info/refs?service=git-upload-pack`)
425+
.get(`${TEST_GITLAB_REPO.proxyUrlPrefix}/info/refs?service=git-upload-pack`)
419426
.set('user-agent', 'git/2.42.0')
420427
.set('accept', 'application/x-git-upload-pack-request')
421428
.buffer();
@@ -442,23 +449,53 @@ describe('proxy express application', async () => {
442449
res.should.have.status(200);
443450

444451
// confirm that its gone from the DB
445-
repo = await db.getRepoByUrl(
446-
TEST_GITLAB_REPO.url,
452+
repo = await db.getRepoByUrl(TEST_GITLAB_REPO.url);
453+
expect(
454+
repo,
447455
'The GitLab repo still existed in the database after it should have been deleted...',
448-
);
449-
expect(repo).to.be.null;
456+
).to.be.null;
450457

451458
// give the proxy half a second to restart
452459
await new Promise((resolve) => setTimeout(resolve, 500));
453460

454461
// try (and fail) to proxy a request to gitlab.com
455462
const res2 = await chai
456463
.request(proxy.getExpressApp())
457-
.get(`/${TEST_GITLAB_REPO.proxyUrlPrefix}/info/refs?service=git-upload-pack`)
464+
.get(`${TEST_GITLAB_REPO.proxyUrlPrefix}/info/refs?service=git-upload-pack`)
458465
.set('user-agent', 'git/2.42.0')
459466
.set('accept', 'application/x-git-upload-pack-request')
460467
.buffer();
461468

462-
res2.should.have.status(404);
469+
res2.should.have.status(403);
470+
}).timeout(5000);
471+
472+
it('should not proxy requests for an unknown project', async function () {
473+
// We are testing that the proxy stops proxying requests for a particular origin
474+
// The chain is stubbed and will always passthrough requests, hence, we are only checking what hosts are proxied.
475+
476+
// the gitlab test repo should already exist
477+
const repo = await db.getRepoByUrl(TEST_UNKNOWN_REPO.url);
478+
expect(
479+
repo,
480+
'The unknown (but real) repo existed in the database which is not expected for this test',
481+
).to.be.null;
482+
483+
// try (and fail) to proxy a request to the repo directly
484+
const res = await chai
485+
.request(proxy.getExpressApp())
486+
.get(`${TEST_UNKNOWN_REPO.proxyUrlPrefix}/info/refs?service=git-upload-pack`)
487+
.set('user-agent', 'git/2.42.0')
488+
.set('accept', 'application/x-git-upload-pack-request')
489+
.buffer();
490+
res.should.have.status(403);
491+
492+
// try (and fail) to proxy a request to the repo via the fallback URL directly
493+
const res2 = await chai
494+
.request(proxy.getExpressApp())
495+
.get(`${TEST_UNKNOWN_REPO.fallbackUrlPrefix}/info/refs?service=git-upload-pack`)
496+
.set('user-agent', 'git/2.42.0')
497+
.set('accept', 'application/x-git-upload-pack-request')
498+
.buffer();
499+
res2.should.have.status(403);
463500
}).timeout(5000);
464501
});

0 commit comments

Comments
 (0)