Skip to content

Commit bc7f7b2

Browse files
yakanechiyakanechi
authored
fix: Verify signature of payloads containing control characters (#102)
1 parent 4d0d098 commit bc7f7b2

File tree

2 files changed

+45
-28
lines changed

2 files changed

+45
-28
lines changed

‎index.js‎

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -371,22 +371,23 @@ class BitbucketScm extends Scm {
371371
* necessary data to execute a Screwdriver job with.
372372
* @method parseHook
373373
* @param {Object} headers The request headers associated with the webhook payload
374-
* @param {Object} payload The webhook payload received from the SCM service.
374+
* @param {String} payload The webhook payload received from the SCM service.
375375
* @return {Object} A key-map of data related to the received payload
376376
*/
377377
async _parseHook(headers, payload) {
378+
const parsedPayload = JSON.parse(payload);
378379
const [typeHeader, actionHeader] = headers['x-event-key'].split(':');
379380
const parsed = {};
380381
const scmContexts = this._getScmContexts();
381382

382383
parsed.hookId = headers['x-request-uuid'];
383384
parsed.scmContext = scmContexts[0];
384385

385-
if (hoek.reach(payload, 'repository.links.html.href') === undefined) {
386+
if (hoek.reach(parsedPayload, 'repository.links.html.href') === undefined) {
386387
throwError('Invalid webhook payload', 400);
387388
}
388389

389-
const link = Url.parse(hoek.reach(payload, 'repository.links.html.href'));
390+
const link = Url.parse(hoek.reach(parsedPayload, 'repository.links.html.href'));
390391
const checkoutUrl = `${link.protocol}//${link.hostname}${link.pathname}.git`;
391392

392393
if (!`${link.hostname}${link.pathname}.git`.startsWith(this.hostname)) {
@@ -398,11 +399,11 @@ class BitbucketScm extends Scm {
398399
if (actionHeader !== 'push') {
399400
return null;
400401
}
401-
const changes = hoek.reach(payload, 'push.changes');
402+
const changes = hoek.reach(parsedPayload, 'push.changes');
402403

403404
parsed.type = 'repo';
404405
parsed.action = 'push';
405-
parsed.username = hoek.reach(payload, 'actor.uuid');
406+
parsed.username = hoek.reach(parsedPayload, 'actor.uuid');
406407
parsed.checkoutUrl = checkoutUrl;
407408
parsed.branch = hoek.reach(changes[0], 'new.name');
408409
parsed.sha = hoek.reach(changes[0], 'new.target.hash');
@@ -422,14 +423,14 @@ class BitbucketScm extends Scm {
422423
}
423424

424425
parsed.type = 'pr';
425-
parsed.username = hoek.reach(payload, 'actor.uuid');
426+
parsed.username = hoek.reach(parsedPayload, 'actor.uuid');
426427
parsed.checkoutUrl = checkoutUrl;
427-
parsed.branch = hoek.reach(payload, 'pullrequest.destination.branch.name');
428-
parsed.sha = hoek.reach(payload, 'pullrequest.source.commit.hash');
429-
parsed.prNum = hoek.reach(payload, 'pullrequest.id');
430-
parsed.prRef = hoek.reach(payload, 'pullrequest.source.branch.name');
428+
parsed.branch = hoek.reach(parsedPayload, 'pullrequest.destination.branch.name');
429+
parsed.sha = hoek.reach(parsedPayload, 'pullrequest.source.commit.hash');
430+
parsed.prNum = hoek.reach(parsedPayload, 'pullrequest.id');
431+
parsed.prRef = hoek.reach(parsedPayload, 'pullrequest.source.branch.name');
431432

432-
const state = hoek.reach(payload, 'pullrequest.state');
433+
const state = hoek.reach(parsedPayload, 'pullrequest.state');
433434

434435
parsed.prMerged = state === 'MERGED';
435436

@@ -1065,7 +1066,7 @@ class BitbucketScm extends Scm {
10651066
* Determine if a scm module can handle the received webhook
10661067
* @method canHandleWebhook
10671068
* @param {Object} headers The request headers associated with the webhook payload
1068-
* @param {Object} payload The webhook payload received from the SCM service
1069+
* @param {String} payload The webhook payload received from the SCM service
10691070
* @return {Promise}
10701071
*/
10711072
_canHandleWebhook(headers, payload) {

‎test/index.test.js‎

Lines changed: 32 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,9 @@ describe('index', function () {
332332
'x-request-uuid': '1e8d4e8e-5fcf-4624-b091-b10bd6ecaf5e'
333333
};
334334

335-
return scm.parseHook(headers, testPayloadOpen).then(result => assert.deepEqual(result, expected));
335+
return scm
336+
.parseHook(headers, JSON.stringify(testPayloadOpen))
337+
.then(result => assert.deepEqual(result, expected));
336338
});
337339

338340
it('resolves the correct parsed config for sync PR (ammending commit)', () => {
@@ -354,7 +356,9 @@ describe('index', function () {
354356
'x-request-uuid': '1e8d4e8e-5fcf-4624-b091-b10bd6ecaf5e'
355357
};
356358

357-
return scm.parseHook(headers, testPayloadSync).then(result => assert.deepEqual(result, expected));
359+
return scm
360+
.parseHook(headers, JSON.stringify(testPayloadSync))
361+
.then(result => assert.deepEqual(result, expected));
358362
});
359363

360364
it('resolves the correct parsed config for closed PR after merged', () => {
@@ -376,7 +380,9 @@ describe('index', function () {
376380
'x-request-uuid': '1e8d4e8e-5fcf-4624-b091-b10bd6ecaf5e'
377381
};
378382

379-
return scm.parseHook(headers, testPayloadClose).then(result => assert.deepEqual(result, expected));
383+
return scm
384+
.parseHook(headers, JSON.stringify(testPayloadClose))
385+
.then(result => assert.deepEqual(result, expected));
380386
});
381387

382388
it('resolves the correct parsed config for closed PR after declined', () => {
@@ -398,7 +404,9 @@ describe('index', function () {
398404
'x-request-uuid': '1e8d4e8e-5fcf-4624-b091-b10bd6ecaf5e'
399405
};
400406

401-
return scm.parseHook(headers, testPayloadClose).then(result => assert.deepEqual(result, expected));
407+
return scm
408+
.parseHook(headers, JSON.stringify(testPayloadClose))
409+
.then(result => assert.deepEqual(result, expected));
402410
});
403411

404412
it('resolves the correct parsed config for push to repo event', () => {
@@ -418,31 +426,39 @@ describe('index', function () {
418426
'x-request-uuid': '1e8d4e8e-5fcf-4624-b091-b10bd6ecaf5e'
419427
};
420428

421-
return scm.parseHook(headers, testPayloadPush).then(result => assert.deepEqual(result, expected));
429+
return scm
430+
.parseHook(headers, JSON.stringify(testPayloadPush))
431+
.then(result => assert.deepEqual(result, expected));
422432
});
423433

424434
it('resolves null if events are not supported: repoFork', () => {
425435
const repoFork = {
426436
'x-event-key': 'repo:fork'
427437
};
428438

429-
return scm.parseHook(repoFork, testPayloadFork).then(result => assert.deepEqual(result, null));
439+
return scm
440+
.parseHook(repoFork, JSON.stringify(testPayloadFork))
441+
.then(result => assert.deepEqual(result, null));
430442
});
431443

432444
it('resolves null if events are not supported: prComment', () => {
433445
const prComment = {
434446
'x-event-key': 'pullrequest:comment_created'
435447
};
436448

437-
return scm.parseHook(prComment, testPayloadPrCommentCreate).then(result => assert.deepEqual(result, null));
449+
return scm
450+
.parseHook(prComment, JSON.stringify(testPayloadPrCommentCreate))
451+
.then(result => assert.deepEqual(result, null));
438452
});
439453

440454
it('resolves null if events are not supported: issueCreated', () => {
441455
const issueCreated = {
442456
'x-event-key': 'issue:created'
443457
};
444458

445-
return scm.parseHook(issueCreated, testPayloadIssueCreate).then(result => assert.deepEqual(result, null));
459+
return scm
460+
.parseHook(issueCreated, JSON.stringify(testPayloadIssueCreate))
461+
.then(result => assert.deepEqual(result, null));
446462
});
447463
});
448464

@@ -1861,47 +1877,47 @@ describe('index', function () {
18611877
it('returns a true for a opened PR.', () => {
18621878
headers['x-event-key'] = 'pullrequest:created';
18631879

1864-
return scm.canHandleWebhook(headers, testPayloadOpen).then(result => {
1880+
return scm.canHandleWebhook(headers, JSON.stringify(testPayloadOpen)).then(result => {
18651881
assert.strictEqual(result, true);
18661882
});
18671883
});
18681884

18691885
it('returns a true for a sync PR (ammending commit).', () => {
18701886
headers['x-event-key'] = 'pullrequest:updated';
18711887

1872-
return scm.canHandleWebhook(headers, testPayloadSync).then(result => {
1888+
return scm.canHandleWebhook(headers, JSON.stringify(testPayloadSync)).then(result => {
18731889
assert.isTrue(result);
18741890
});
18751891
});
18761892

18771893
it('returns a true for closed PR after merged.', () => {
18781894
headers['x-event-key'] = 'pullrequest:fullfilled';
18791895

1880-
return scm.canHandleWebhook(headers, testPayloadClose).then(result => {
1896+
return scm.canHandleWebhook(headers, JSON.stringify(testPayloadClose)).then(result => {
18811897
assert.isTrue(result);
18821898
});
18831899
});
18841900

18851901
it('returns a true for closed PR after declined.', () => {
18861902
headers['x-event-key'] = 'pullrequest:rejected';
18871903

1888-
return scm.canHandleWebhook(headers, testPayloadClose).then(result => {
1904+
return scm.canHandleWebhook(headers, JSON.stringify(testPayloadClose)).then(result => {
18891905
assert.isTrue(result);
18901906
});
18911907
});
18921908

18931909
it('returns a true for a push event payload.', () => {
18941910
headers['x-event-key'] = 'repo:push';
18951911

1896-
return scm.canHandleWebhook(headers, testPayloadPush).then(result => {
1912+
return scm.canHandleWebhook(headers, JSON.stringify(testPayloadPush)).then(result => {
18971913
assert.isTrue(result);
18981914
});
18991915
});
19001916

19011917
it('returns a true when _parseHook() returns null.', () => {
19021918
headers['x-event-key'] = 'issue:created';
19031919

1904-
return scm.canHandleWebhook(headers, testPayloadPush).then(result => {
1920+
return scm.canHandleWebhook(headers, JSON.stringify(testPayloadPush)).then(result => {
19051921
assert.isTrue(result);
19061922
});
19071923
});
@@ -1910,7 +1926,7 @@ describe('index', function () {
19101926
// eslint-disable-next-line no-underscore-dangle
19111927
scm._parseHook = () => Promise.reject(new Error('Test error'));
19121928

1913-
return scm.canHandleWebhook(headers, testPayloadPush).then(result => {
1929+
return scm.canHandleWebhook(headers, JSON.stringify(testPayloadPush)).then(result => {
19141930
assert.strictEqual(result, false);
19151931
});
19161932
});
@@ -1919,7 +1935,7 @@ describe('index', function () {
19191935
headers['x-event-key'] = 'repo:push';
19201936
testPayloadPush.repository.links.html.href = 'https://github.com/batman/test';
19211937

1922-
return scm.canHandleWebhook(headers, testPayloadPush).then(result => {
1938+
return scm.canHandleWebhook(headers, JSON.stringify(testPayloadPush)).then(result => {
19231939
assert.isFalse(result);
19241940
});
19251941
});

0 commit comments

Comments
 (0)