Skip to content

Commit

Permalink
Apply feedback from code review
Browse files Browse the repository at this point in the history
  • Loading branch information
@swissspidy
swissspidy committed May 16, 2024
1 parent c68b98c commit 5c8c81c
Show file tree
Hide file tree
Showing 7 changed files with 110 additions and 72 deletions.
8 changes: 4 additions & 4 deletions inc/ProjectLocator.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ class ProjectLocator {
*
* @since 2.0.0
*
* @param int|string|false|null|\Required\Traduttore\Project|\GP_Project $project Possible GlotPress project ID or path or source code repository path.
* @param int|string|\Required\Traduttore\Project|\GP_Project $project Possible GlotPress project ID or path or source code repository path.
*/
public function __construct( int|string|false|\Required\Traduttore\Project|\GP_Project|null $project ) {
public function __construct( int|string|Project|\GP_Project $project ) {
$this->project = $this->find_project( $project );
}

Expand All @@ -52,10 +52,10 @@ public function get_project(): ?Project {
*
* @since 2.0.0
*
* @param int|string|false|null|\Required\Traduttore\Project|\GP_Project $project Possible GlotPress project ID or path or source code repository path.
* @param int|string|\Required\Traduttore\Project|\GP_Project $project Possible GlotPress project ID or path or source code repository path.
* @return \Required\Traduttore\Project Project instance.
*/
protected function find_project( int|string|false|\Required\Traduttore\Project|\GP_Project|null $project ): ?Project {
protected function find_project( int|string|Project|\GP_Project $project ): ?Project {
if ( ! $project ) {
return null;
}
Expand Down
14 changes: 10 additions & 4 deletions inc/WebhookHandler/Bitbucket.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,15 @@ public function permission_callback(): bool {
return false;
}

$token = $this->request->get_header( 'x-hub-signature-256' );
$params = $this->request->get_params();
$locator = new ProjectLocator( $params['repository']['links']['html']['href'] ?? null );
$token = $this->request->get_header( 'x-hub-signature-256' );
$params = $this->request->get_params();
$repository = $params['repository']['links']['html']['href'] ?? null;

if ( ! $repository ) {
return false;
}

$locator = new ProjectLocator( $repository );
$project = $locator->get_project();
$secret = $this->get_secret( $project );

Expand All @@ -45,7 +51,7 @@ public function permission_callback(): bool {
return false;
}

$payload_signature = 'sha256=' . hash_hmac( 'sha256', (string) wp_json_encode( $params ), $secret );
$payload_signature = 'sha256=' . hash_hmac( 'sha256', $this->request->get_body(), $secret );

return hash_equals( $token, $payload_signature );
}
Expand Down
23 changes: 21 additions & 2 deletions inc/WebhookHandler/GitHub.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,27 @@ public function permission_callback(): bool {
return false;
}

$params = $this->request->get_params();
$locator = new ProjectLocator( $params['repository']['html_url'] ?? null );
$params = $this->request->get_params();
$content_type = $this->request->get_content_type();

// See https://developer.github.com/webhooks/creating/#content-type.
if ( ! empty( $content_type ) && 'application/x-www-form-urlencoded' === $content_type['value'] ) {
$params = json_decode( $params['payload'], true );
}

/**
* Request params.
*
* @var array{repository: array{ default_branch?: string, html_url?: string, full_name: string, ssh_url: string, clone_url: string, private: bool }, ref: string } $params
*/

$repository = $params['repository']['html_url'] ?? null;

if ( ! $repository ) {
return false;
}

$locator = new ProjectLocator( $repository );
$project = $locator->get_project();

$secret = $this->get_secret( $project );
Expand Down
10 changes: 8 additions & 2 deletions inc/WebhookHandler/GitLab.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,14 @@ public function permission_callback(): bool {
return false;
}

$params = $this->request->get_params();
$locator = new ProjectLocator( $params['project']['homepage'] ?? null );
$params = $this->request->get_params();
$repository = $params['project']['homepage'] ?? null;

if ( ! $repository ) {
return false;
}

$locator = new ProjectLocator( $repository );
$project = $locator->get_project();

$secret = $this->get_secret( $project );
Expand Down
6 changes: 0 additions & 6 deletions tests/phpunit/tests/ProjectLocator.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,6 @@ public function test_empty_string(): void {
$this->assertNull( $locator->get_project() );
}

public function test_false(): void {
$locator = new Locator( false );

$this->assertNull( $locator->get_project() );
}

public function test_invalid_project_id(): void {
$locator = new Locator( 0 );

Expand Down
119 changes: 66 additions & 53 deletions tests/phpunit/tests/WebhookHandler/Bitbucket.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,9 @@ public function test_invalid_event_header(): void {

public function test_invalid_signature(): void {
$request = new WP_REST_Request( 'POST', '/traduttore/v1/incoming-webhook' );
$request->set_body_params( [] );
$signature = 'sha256=' . hash_hmac( 'sha256', (string) wp_json_encode( $request->get_params() ), 'foo' );
$request->add_header( 'Content-Type', 'application/json' );
$request->set_body( (string) wp_json_encode( [] ) );
$signature = 'sha256=' . hash_hmac( 'sha256', $request->get_body(), 'foo' );
$request->add_header( 'x-event-key', 'repo:push' );
$request->add_header( 'x-hub-signature-256', $signature );
$response = rest_get_server()->dispatch( $request );
Expand All @@ -57,20 +58,23 @@ public function test_invalid_signature(): void {

public function test_missing_signature_is_valid(): void {
$request = new WP_REST_Request( 'POST', '/traduttore/v1/incoming-webhook' );
$request->set_body_params(
[
'ref' => 'refs/heads/master',
'repository' => [
'links' => [
'html' => [
'href' => 'https://bitbucket.org/wearerequired/not-traduttore',
$request->add_header( 'Content-Type', 'application/json' );
$request->set_body(
(string) wp_json_encode(
[
'ref' => 'refs/heads/master',
'repository' => [
'links' => [
'html' => [
'href' => 'https://bitbucket.org/wearerequired/not-traduttore',
],
],
'full_name' => 'wearerequired/not-traduttore',
'scm' => 'git',
'is_private' => false,
],
'full_name' => 'wearerequired/not-traduttore',
'scm' => 'git',
'is_private' => false,
],
]
]
)
);
$request->add_header( 'x-event-key', 'repo:push' );
$response = rest_get_server()->dispatch( $request );
Expand All @@ -80,22 +84,25 @@ public function test_missing_signature_is_valid(): void {

public function test_invalid_project(): void {
$request = new WP_REST_Request( 'POST', '/traduttore/v1/incoming-webhook' );
$request->set_body_params(
[
'ref' => 'refs/heads/master',
'repository' => [
'links' => [
'html' => [
'href' => 'https://bitbucket.org/wearerequired/not-traduttore',
$request->add_header( 'Content-Type', 'application/json' );
$request->set_body(
(string) wp_json_encode(
[
'ref' => 'refs/heads/master',
'repository' => [
'links' => [
'html' => [
'href' => 'https://bitbucket.org/wearerequired/not-traduttore',
],
],
'full_name' => 'wearerequired/not-traduttore',
'scm' => 'git',
'is_private' => false,
],
'full_name' => 'wearerequired/not-traduttore',
'scm' => 'git',
'is_private' => false,
],
]
]
)
);
$signature = 'sha256=' . hash_hmac( 'sha256', (string) wp_json_encode( $request->get_params() ), 'traduttore-test' );
$signature = 'sha256=' . hash_hmac( 'sha256', $request->get_body(), 'traduttore-test' );
$request->add_header( 'x-event-key', 'repo:push' );
$request->add_header( 'x-hub-signature-256', $signature );
$response = rest_get_server()->dispatch( $request );
Expand All @@ -105,22 +112,25 @@ public function test_invalid_project(): void {

public function test_valid_project(): void {
$request = new WP_REST_Request( 'POST', '/traduttore/v1/incoming-webhook' );
$request->set_body_params(
[
'ref' => 'refs/heads/master',
'repository' => [
'links' => [
'html' => [
'href' => 'https://bitbucket.org/wearerequired/traduttore',
$request->add_header( 'Content-Type', 'application/json' );
$request->set_body(
(string) wp_json_encode(
[
'ref' => 'refs/heads/master',
'repository' => [
'links' => [
'html' => [
'href' => 'https://bitbucket.org/wearerequired/traduttore',
],
],
'full_name' => 'wearerequired/traduttore',
'scm' => 'git',
'is_private' => false,
],
'full_name' => 'wearerequired/traduttore',
'scm' => 'git',
'is_private' => false,
],
]
]
)
);
$signature = 'sha256=' . hash_hmac( 'sha256', (string) wp_json_encode( $request->get_params() ), 'traduttore-test' );
$signature = 'sha256=' . hash_hmac( 'sha256', $request->get_body(), 'traduttore-test' );
$request->add_header( 'x-event-key', 'repo:push' );
$request->add_header( 'x-hub-signature-256', $signature );
$response = rest_get_server()->dispatch( $request );
Expand All @@ -140,22 +150,25 @@ public function test_valid_mercurial_project(): void {
$this->project->set_repository_vcs_type( Repository::VCS_TYPE_HG );

$request = new WP_REST_Request( 'POST', '/traduttore/v1/incoming-webhook' );
$request->set_body_params(
[
'ref' => 'refs/heads/master',
'repository' => [
'links' => [
'html' => [
'href' => 'https://bitbucket.org/wearerequired/traduttore',
$request->add_header( 'Content-Type', 'application/json' );
$request->set_body(
(string) wp_json_encode(
[
'ref' => 'refs/heads/master',
'repository' => [
'links' => [
'html' => [
'href' => 'https://bitbucket.org/wearerequired/traduttore',
],
],
'full_name' => 'wearerequired/traduttore',
'scm' => 'git',
'is_private' => false,
],
'full_name' => 'wearerequired/traduttore',
'scm' => 'git',
'is_private' => false,
],
]
]
)
);
$signature = 'sha256=' . hash_hmac( 'sha256', (string) wp_json_encode( $request->get_params() ), 'traduttore-test' );
$signature = 'sha256=' . hash_hmac( 'sha256', $request->get_body(), 'traduttore-test' );
$request->add_header( 'x-event-key', 'repo:push' );
$request->add_header( 'x-hub-signature-256', $signature );
$response = rest_get_server()->dispatch( $request );
Expand Down
2 changes: 1 addition & 1 deletion tests/phpunit/tests/WebhookHandler/GitHub.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ public function test_invalid_request(): void {
$request->add_header( 'x-hub-signature-256', $signature );
$response = rest_get_server()->dispatch( $request );

$this->assertErrorResponse( 400, $response );
$this->assertErrorResponse( 'rest_forbidden', $response, 401 );
}

public function test_valid_project(): void {
Expand Down

0 comments on commit 5c8c81c

Please sign in to comment.