Differential D260

Reject requests without X-Hub-Signature when needed
ClosedPublic
Actions

Authored by dereckson on Jan 24 2016, 07:07.

Details

Reviewers

dereckson

Maniphest Tasks

T695: Send a non JSON error to /gate/GitHub should fail sooner

Commits

Unknown Object (Diffusion Commit)
rNOTIF47efed1d9a30: Reject requests without X-Hub-Signature when needed

Summary

A request to /gate/GitHub/<service with secret defined> must has
a X-Hub-Signature header, so we can directly consider not legit
those without.

This fixes the following exception:

ErrorException in XHubSignature.php line 83:
hash_equals(): Expected user_string to be a string, null given

Test Plan

Fire a Phabricator payload to /gate/GitHub/<door with secret>

Diff Detail

Repository

rNOTIF Notifications center

Lint

Lint Passed

Unit

No Test Coverage

Branch

T695

Unit TestsFailed
View All

	Time	Test
	3,609 ms	Jenkins > Nasqueron\Notifications\Tests\Http\Controllers\GitHubGateControllerTest::Unknown Unit Message ("")
	3,281 ms	Jenkins > Nasqueron\Notifications\Tests\Http\Controllers\GitHubGateControllerTest::Unknown Unit Message ("")
	3,154 ms	Jenkins > Nasqueron\Notifications\Tests\PayloadFullTest::Unknown Unit Message ("")
	3,136 ms	Jenkins > Nasqueron\Notifications\Tests\PayloadFullTest::Unknown Unit Message ("")
	3,095 ms	Jenkins > Nasqueron\Notifications\Tests\PayloadFullTest::Unknown Unit Message ("")
		View Full Test Results (16 Failed · 21 Passed)

Event Timeline

dereckson updated this revision to Diff 606.Jan 24 2016, 07:07

dereckson added a task: T695: Send a non JSON error to /gate/GitHub should fail sooner.

dereckson retitled this revision from to Reject requests without X-Hub-Signature when needed.

dereckson updated this object.

dereckson edited the test plan for this revision. (Show Details)

dereckson added a reviewer: dereckson.

Build started: https://ci.nasqueron.org/job/test-notifications-phab/49/ (console: https://ci.nasqueron.org/job/test-notifications-phab/49/console)

dereckson planned changes to this revision.Jan 24 2016, 07:08

dereckson added inline comments.

app/Http/Controllers/Gate/GitHubGateController.php
130	"signature is missing" or "no signature is included"
131	… perform any other validation

Improve code comment

dereckson accepted this revision.Jan 24 2016, 07:11

dereckson edited edge metadata.

This revision is now accepted and ready to land.Jan 24 2016, 07:11

Build has FAILED

Link to build: https://ci.nasqueron.org/job/test-notifications-phab/49/
See console output for more information: https://ci.nasqueron.org/job/test-notifications-phab/49/console

Build started: https://ci.nasqueron.org/job/test-notifications-phab/50/ (console: https://ci.nasqueron.org/job/test-notifications-phab/50/console)

Build has FAILED

Link to build: https://ci.nasqueron.org/job/test-notifications-phab/50/
See console output for more information: https://ci.nasqueron.org/job/test-notifications-phab/50/console

In D260#4122, @alken-orin wrote:

Build has FAILED

Link to build: https://ci.nasqueron.org/job/test-notifications-phab/50/
See console output for more information: https://ci.nasqueron.org/job/test-notifications-phab/50/console

Some caching issue for tests, see Q4.

Build started: https://ci.nasqueron.org/job/test-notifications-phab/51/ (console: https://ci.nasqueron.org/job/test-notifications-phab/51/console)