Skip to content

feat: implement Gitea webhook support#74

Open
jaysomani wants to merge 4 commits intoutopia-php:mainfrom
jaysomani:feat/gitea-webhook-support
Open

feat: implement Gitea webhook support#74
jaysomani wants to merge 4 commits intoutopia-php:mainfrom
jaysomani:feat/gitea-webhook-support

Conversation

@jaysomani
Copy link
Contributor

  • Implement getEvent() for push and pull_request events
  • Implement validateWebhookEvent() with HMAC-SHA256 validation
  • Parse webhook payloads to match GitHub adapter format
  • Add comprehensive test coverage for all webhook scenarios
  • Test push events with affected files tracking
  • Test pull request events including external/fork detection
  • Test signature validation (valid and invalid cases)
  • Test error handling for invalid payloads and unsupported events
  • Add abstract method stub for PHPStan compatibility

Resolves webhook functionality for Gitea adapter

- Implement getEvent() for push and pull_request events
- Implement validateWebhookEvent() with HMAC-SHA256 validation
- Parse webhook payloads to match GitHub adapter format
- Add comprehensive test coverage for all webhook scenarios
- Test push events with affected files tracking
- Test pull request events including external/fork detection
- Test signature validation (valid and invalid cases)
- Test error handling for invalid payloads and unsupported events
- Add abstract method stub for PHPStan compatibility

Resolves webhook functionality for Gitea adapter
@chatgpt-codex-connector
Copy link

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@greptile-apps
Copy link

greptile-apps bot commented Mar 20, 2026

Greptile Summary

This PR implements the previously stubbed getEvent() and validateWebhookEvent() methods on the Gitea adapter, parsing webhook payloads for push and pull_request events into the same shape used by the GitHub adapter, and adds a broad suite of unit tests.

Key observations:

  • The HMAC signature comparison in validateWebhookEvent() uses PHP's === operator instead of hash_equals(), leaving the endpoint open to timing-based side-channel attacks.
  • Two variables — $payloadPullRequestHeadUser and $payloadPullRequestBaseUser — are assigned in the pull_request case but never consumed, likely leftovers from copying the GitHub adapter structure.
  • pullRequestNumber is cast to string via strval(), while the GitHub adapter leaves it as the raw JSON integer; downstream consumers that switch between adapters may hit type inconsistencies.
  • The fork/external detection approach (comparing full_name values) is reasonable and arguably more reliable than the GitHub adapter's login-comparison approach.
  • testGetEvent is a trivially-passing assertTrue(true) with no actual assertions; it should either be removed or replaced with a meaningful test.

Confidence Score: 3/5

  • Safe to merge after addressing the timing-safe comparison issue in signature validation.
  • The implementation is largely correct and well-tested, but the use of === instead of hash_equals() for HMAC comparison is a security concern that should be fixed before merging. The other issues (unused variables, type inconsistency, trivial test) are minor but worth cleaning up.
  • Pay close attention to src/VCS/Adapter/Git/Gitea.php, specifically the validateWebhookEvent() method.

Important Files Changed

Filename Overview
src/VCS/Adapter/Git/Gitea.php Implements getEvent() for push and pull_request events and validateWebhookEvent() with HMAC-SHA256. Main concerns: signature validation uses === instead of hash_equals() (timing-attack risk), two unused variables in the pull_request case, and pullRequestNumber is cast to string inconsistently with the GitHub adapter.
tests/VCS/Adapter/GiteaTest.php Adds comprehensive webhook tests covering push events, pull request events (internal and fork/external), signature validation, invalid payloads, and unsupported events. The testGetEvent method is a trivially-passing no-op (assertTrue(true)) rather than a meaningful test.

Last reviewed commit: "feat: implement Gite..."

Comment on lines +644 to +648
public function testGetEvent(): void
{
$this->markTestSkipped('Will be implemented in follow-up PR');
// Base class requires this method - implemented via specific event tests below
$this->assertTrue(true);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Trivially-passing assertion provides no coverage

$this->assertTrue(true) is a no-op test — it will always pass regardless of the implementation state, and gives false confidence in test coverage. The PR description says "Base class requires this method — implemented via specific event tests below," but a comment explaining this (with no assertion) or delegating to one of the more specific tests would communicate intent more clearly without adding a meaningless green tick.

Comment on lines +644 to +646
public function testGetEvent(): void
{
$this->markTestSkipped('Will be implemented in follow-up PR');
// Base class requires this method - implemented via specific event tests below
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea of splitting it for different types of webhook pyloads.

Can you please remove this method, also from GitHub, and implement those specific event tests in GitHub as well?

$this->assertIsArray($result);
$this->assertEmpty($result);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All tests you implemented are great, but they serve unit-test purpose, by mocking expected payloads.

Let's add E2E tests that actually configured webhook on Gitea, and triggers it by making a push and pull request.

We can point webhook to request-catcher - we can introduce this container in docker-compose - just like in appwrite/appwrite.

We can then have method to get last request, and use it to get payload in tests after triggering it.
Something like this, but simpler: https://github.com/appwrite/appwrite/blob/dabe98d4a1cc150dda639767ba105da5b491dd3b/tests/e2e/Scopes/Scope.php#L314

(In Appwrite, we have multiple different request catchers, but here, we can just use one)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants