-
Notifications
You must be signed in to change notification settings - Fork 5
Github webhook implementation #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ALTER TABLE "reviewApplication" DROP CONSTRAINT "reviewApplication_opportunityId_fkey"; | ||
|
||
-- AlterTable | ||
ALTER TABLE "reviewApplication" ALTER COLUMN "opportunityId" SET DATA TYPE TEXT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider verifying if changing the data type of opportunityId
to TEXT aligns with the application's data integrity requirements, especially if it was previously a different type such as INTEGER.
|
||
-- CreateTable | ||
CREATE TABLE "gitWebhookLog" ( | ||
"id" VARCHAR(14) NOT NULL DEFAULT nanoid(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the nanoid()
function is supported by your database system and that it generates IDs that meet your application's requirements for uniqueness and length.
@@ -381,3 +381,15 @@ model reviewApplication { | |||
@@index([userId]) | |||
@@index([opportunityId]) | |||
} | |||
|
|||
model gitWebhookLog { | |||
id String @id @default(dbgenerated("nanoid()")) @db.VarChar(14) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a more descriptive name for the id
field, such as webhookLogId
, to make it clear that this ID is specific to the gitWebhookLog
model.
|
||
model gitWebhookLog { | ||
id String @id @default(dbgenerated("nanoid()")) @db.VarChar(14) | ||
eventId String // X-GitHub-Delivery header |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment // X-GitHub-Delivery header
could be misleading if the eventId
is not directly derived from the header. Ensure that the comment accurately reflects the source or purpose of the field.
model gitWebhookLog { | ||
id String @id @default(dbgenerated("nanoid()")) @db.VarChar(14) | ||
eventId String // X-GitHub-Delivery header | ||
event String // X-GitHub-Event header |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment // X-GitHub-Event header
should be verified to ensure it accurately describes the source of the event
field. If the field is not directly from the header, consider updating the comment.
id String @id @default(dbgenerated("nanoid()")) @db.VarChar(14) | ||
eventId String // X-GitHub-Delivery header | ||
event String // X-GitHub-Event header | ||
eventPayload Json // Complete webhook payload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that storing the complete webhook payload as JSON in the eventPayload
field does not exceed any database size limitations or performance constraints.
|
||
export interface WebhookRequest { | ||
headers: GitHubWebhookHeaders; | ||
body: any; // GitHub webhook payload (varies by event type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a more specific type instead of any
for the body
property to improve type safety. If the payload varies by event type, you might define a union type or use generics to handle different payload structures.
description: 'Internal Server Error - Processing failed', | ||
}) | ||
async handleGitHubWebhook( | ||
@Body() payload: any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a more specific type instead of any
for the payload
parameter to ensure type safety and better code readability.
* Placeholder for future event-specific processing logic | ||
* This method can be extended to handle different GitHub events differently | ||
*/ | ||
private handleEventSpecificProcessing(event: string, payload: any): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a more specific type for the payload
parameter instead of any
to improve type safety and maintainability.
startDate?: Date; | ||
endDate?: Date; | ||
}) { | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a return type annotation to the getWebhookLogs
method for better clarity and type safety.
event: string; // From X-GitHub-Event | ||
|
||
@IsNotEmpty() | ||
eventPayload: any; // Complete webhook payload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider specifying a more precise type for eventPayload
instead of using any
. This will help with type safety and provide better context for the expected structure of the payload.
|
||
@Injectable() | ||
export class GitHubSignatureGuard implements CanActivate { | ||
private readonly logger = LoggerService.forRoot('GitHubSignatureGuard'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using dependency injection for the LoggerService instead of calling forRoot
directly. This will make the code more testable and adhere to the dependency injection pattern used in NestJS.
|
||
try { | ||
// Get the raw body for signature verification | ||
const payload = request.body; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The request.body
might not always be available in the expected format, especially if middleware like body-parser
is not configured correctly. Consider ensuring that the raw body is captured correctly for signature verification, possibly by using a middleware to store the raw body.
const providedSignature = signature; | ||
|
||
// Perform timing-safe comparison to prevent timing attacks | ||
const isValid = crypto.timingSafeEqual( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The crypto.timingSafeEqual
function requires both buffers to be of the same length. Ensure that expectedSignature
and providedSignature
are always of the same length before calling this function to prevent runtime errors.
.env.sample
Outdated
@@ -1,6 +1,10 @@ | |||
POSTGRES_SCHEMA="public" | |||
DATABASE_URL="postgresql://johndoe:randompassword@localhost:5432/mydb?schema=${POSTGRES_SCHEMA}" | |||
|
|||
|
|||
# GitHub Webhook Configuration | |||
GITHUB_WEBHOOK_SECRET="your_webhook_secret_here" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a placeholder or example value for GITHUB_WEBHOOK_SECRET
that indicates it should be replaced with an actual secret, such as your_actual_webhook_secret_here
.
@@ -13,6 +13,9 @@ import { ReviewOpportunityService } from './review-opportunity/reviewOpportunity | |||
import { ReviewApplicationService } from './review-application/reviewApplication.service'; | |||
import { ReviewHistoryController } from './review-history/reviewHistory.controller'; | |||
import { ChallengeApiService } from 'src/shared/modules/global/challenge.service'; | |||
import { WebhookController } from './webhook/webhook.controller'; | |||
import { WebhookService } from './webhook/webhook.service'; | |||
import { GiteaSignatureGuard } from '../shared/guards/gitea-signature.guard'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import statement has been changed from GitHubSignatureGuard
to GiteaSignatureGuard
. Ensure that this change is intentional and that the GiteaSignatureGuard
is correctly implemented and compatible with the rest of the application. Verify that all references to GitHubSignatureGuard
have been updated accordingly throughout the codebase.
], | ||
providers: [ | ||
ReviewOpportunityService, | ||
ReviewApplicationService, | ||
ChallengeApiService, | ||
WebhookService, | ||
GiteaSignatureGuard, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change from GitHubSignatureGuard
to GiteaSignatureGuard
suggests a switch from GitHub to Gitea for signature verification. Ensure that the rest of the codebase, especially where GitHubSignatureGuard
was used, is updated accordingly to handle Gitea signatures. Verify that the GiteaSignatureGuard
is correctly implemented and tested for the intended use case.
WebhookEventDto, | ||
WebhookResponseDto, | ||
} from '../../dto/webhook-event.dto'; | ||
import { GiteaSignatureGuard } from '../../shared/guards/gitea-signature.guard'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import statement has been changed from GitHubSignatureGuard
to GiteaSignatureGuard
. Ensure that this change is intentional and that the rest of the codebase is updated accordingly to reflect this change. If this is part of a broader migration from GitHub to Gitea, verify that all related components and configurations are updated as well.
description: 'Internal Server Error - Processing failed', | ||
}) | ||
async handleGiteaWebhook( | ||
@Body() payload: any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a more specific type instead of any
for the payload
parameter to improve type safety and code readability.
|
||
canActivate(context: ExecutionContext): boolean { | ||
const request = context.switchToHttp().getRequest<Request>(); | ||
const signature = request.headers['x-hub-signature-256'] as string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the x-hub-signature-256
header is correct for Gitea. If Gitea uses a different header for the signature, it should be updated accordingly.
ALTER TABLE "reviewApplication" DROP CONSTRAINT "reviewApplication_opportunityId_fkey"; | ||
|
||
-- AlterTable | ||
ALTER TABLE "reviewApplication" ALTER COLUMN "opportunityId" SET DATA TYPE TEXT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider verifying that changing the data type of opportunityId
to TEXT does not affect any existing data integrity or application logic that depends on this column being of a specific type.
ALTER TABLE "reviewOpportunity" ALTER COLUMN "updatedAt" DROP DEFAULT; | ||
|
||
-- AddForeignKey | ||
ALTER TABLE "reviewApplication" ADD CONSTRAINT "reviewApplication_opportunityId_fkey" FOREIGN KEY ("opportunityId") REFERENCES "reviewOpportunity"("id") ON DELETE CASCADE ON UPDATE CASCADE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the new foreign key constraint with ON DELETE CASCADE ON UPDATE CASCADE
is intended, as it will automatically delete or update rows in reviewApplication
when corresponding rows in reviewOpportunity
are deleted or updated. This could have significant implications for data integrity.
No description provided.