wip#266
Conversation
WalkthroughIntroduces GitHub webhook event handling infrastructure, including a new controller that validates and receives GitHub events, a GithubEvent model with status tracking, a service to process pull request and review events with status updates and transaction handling, database schema changes, routing configuration, and test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub
participant WC as Webhooks<br/>Controller
participant GE as GithubEvent<br/>Model
participant CS as CreateService
participant Cont as Contribution<br/>Model
participant Osbl as Osbl<br/>Model
GH->>WC: POST /webhooks/github_events<br/>+ X-Hub-Signature-256
WC->>WC: validate_signature()
alt Signature Invalid
WC-->>GH: 401 Unauthorized
else Signature Valid
WC->>GE: create!(event, payload, status: pending)
WC->>CS: new(github_event: GE)
WC->>CS: call()
alt pull_request event
CS->>Cont: find_by(github_resource_url)
alt action == "closed"
alt status != "validée"
CS->>GE: update(status: "rejetée")
end
else
CS->>GE: update(status: "ignored")
end
else pull_request_review event
CS->>Cont: find_by(github_resource_url)
alt review state == "changes_requested"
rect rgba(255,193,7,0.1)
note right of CS: Transaction
CS->>Cont: update(status: "modifications demandées")
CS->>GE: update(status: "processed")
end
else review state == "approved"
rect rgba(76,175,80,0.1)
note right of CS: Transaction
CS->>Cont: update(status: "validée")
CS->>Osbl: create!(from contribution data)
CS->>GE: update(status: "processed")
end
else
CS->>GE: update(status: "ignored")
end
else
CS->>GE: update(status: "ignored")
end
WC-->>GH: 200 OK
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 RuboCop (1.81.7)app/models/github_event.rbUnable to find gem rubocop-rails-omakase; is the gem installed? Gem::MissingSpecError spec/services/github_events/create_service_spec.rbUnable to find gem rubocop-rails-omakase; is the gem installed? Gem::MissingSpecError app/controllers/webhooks/github_events_controller.rbUnable to find gem rubocop-rails-omakase; is the gem installed? Gem::MissingSpecError
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -0,0 +1,34 @@ | |||
| class Webhooks::GithubEventsController < ApplicationController | |||
| skip_before_action :verify_authenticity_token | |||
Check failure
Code scanning / CodeQL
CSRF protection weakened or disabled High
Copilot Autofix
AI 7 months ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| return if contribution.status == "validée" | ||
|
|
||
| contribution.update!(status: "rejetée") | ||
| end |
There was a problem hiding this comment.
Bug: Event Ignored, Yet Processing Continues
The handle_pull_request_event method calls ignore_event! when github_action is not "closed" but doesn't return afterward. This causes execution to continue, attempting to access contribution and potentially updating it even after the event was marked as ignored. The method needs to return after calling ignore_event! to prevent further processing.
|
|
||
| unless ActiveSupport::SecurityUtils.secure_compare(expected, signature) | ||
| Rails.logger.warn "Invalid GitHub webhook signature" | ||
| head :unauthorized |
There was a problem hiding this comment.
Bug: Authentication Bypass: Malicious Webhooks Processed
After detecting an invalid webhook signature, validate_signature calls head :unauthorized but doesn't return, allowing execution to continue. This causes the webhook to be processed despite failing authentication, creating a GithubEvent record and executing the service logic with potentially malicious data.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
app/models/github_event.rb (1)
3-4: Consider uncommenting validations before production.The database migration enforces
NOT NULLconstraints on botheventandpayloadfields, but model-level validations are commented out. While the database will prevent invalid data, uncommenting these validations would provide clearer error messages to developers and fail faster during object creation.Apply this diff to enable validations:
- # validates :event, presence: true - # validates :payload, presence: true + validates :event, presence: true + validates :payload, presence: truespec/services/github_events/create_service_spec.rb (2)
23-23: Consider using a fresh factory instance for the approved scenario.Directly mutating
github_event.payload["review"]["state"]couples the test to the payload structure and could lead to shared state issues if the factory is reused elsewhere.Consider creating a separate factory trait or transient attribute:
# In spec/factories/github_events.rb factory :github_event do # existing attributes... trait :approved do payload do # duplicate payload structure with state: "approved" end end end # Then in the test: context "when the review is approved" do let(:github_event) { create(:github_event, :approved) } # ... end
4-32: Consider adding error case coverage.The current tests cover the happy paths for
changes_requestedandapprovedstates. Consider adding test cases for:
- When no matching contribution exists for the PR URL
- When the event type is not
pull_request_review(to verifyignore_event!behavior)- Edge cases like malformed payloads
spec/factories/github_events.rb (1)
1-460: LGTM! Comprehensive webhook payload factory.The factory provides a realistic and detailed GitHub webhook payload structure, which is essential for thorough integration testing. The level of detail matches actual GitHub webhook payloads.
As noted in the service spec review, consider adding factory traits for different review states (
approved,changes_requested,commented) to improve test clarity and reduce payload mutation in tests.app/controllers/webhooks/github_events_controller.rb (2)
8-18: Event persistence is fine; consider capturing delivery id and clarifying payload filteringPersisting
eventfromX-GitHub-Eventand a trimmedpayloadis reasonable, and delegating toGithubEvents::CreateServicekeeps the controller thin. Two optional improvements:
- Persist
X-GitHub-Delivery(or similar unique id) onGithubEventso you can dedupe and correlate retries/debugging at the DB level.- If the goal of
except("repository", "organization")is to avoid storing large or sensitive repo/org blobs, be aware nestedrepositorydata underpull_request.*is still kept; consider deeper filtering if that matters.
22-33: Signature validation is solid; add a small guard for malformed headersThe HMAC check with
X-Hub-Signature-256, Rails credentials, andSecureUtils.secure_compareis the right pattern. To harden this a bit against malformed/truncated headers and make intent clearer, you could add a length guard before comparing and bail out early:def validate_signature signature = request.headers["X-Hub-Signature-256"] head :unauthorized and return unless signature&.start_with?("sha256=") secret = Rails.application.credentials.github.webhook_secret expected = "sha256=" + OpenSSL::HMAC.hexdigest("sha256", secret, request.raw_post) + + return head :unauthorized if signature.bytesize != expected.bytesizeThis ensures obviously broken signatures are treated as unauthorized without relying on library‑specific compare behavior.
app/services/github_events/create_service.rb (2)
9-18: Event dispatch logic is clear; clean up the inline TODOThe
callcase statement cleanly routes bygithub_event.eventand defaults toignore_event!, which is good. The inline comment# I should probably not use this oneon"pull_request"is a bit opaque—either remove it or turn it into a clearer TODO explaining under which circumstancespull_requestevents will be enabled or can be removed.
30-51: Review handler works but could be more defensive and idempotentThe
"changes_requested"/"approved"handling with transactions and status updates looks good, but two improvements would make it more robust:
- Idempotency for approved reviews:
Osbl.create!(contribution.osbl_data)will run each time an"approved"review event is processed (including GitHub retries), which can create duplicate OSBLs or hit uniqueness constraints. If duplicates are undesired, consider basing this on a natural key (e.g., a slug or external id) and usingfind_or_create_by!or handlingActiveRecord::RecordNotUniqueso repeated events are harmless.- Payload shape guards:
github_event.payload["review"]["state"]assumes"review"is always present. If GitHub tweaks the payload or a misconfigured event reaches you, this will raise instead of gracefully ignoring it. A small guard (e.g., early return /ignore_event!when"review"or"state"is missing) would make this path more resilient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/controllers/webhooks/github_events_controller.rb(1 hunks)app/models/github_event.rb(1 hunks)app/services/github_events/create_service.rb(1 hunks)config/credentials.yml.enc(1 hunks)config/routes.rb(1 hunks)db/migrate/20250403123232_create_github_event.rb(1 hunks)db/schema.rb(3 hunks)spec/factories/github_events.rb(1 hunks)spec/models/github_event_spec.rb(1 hunks)spec/services/github_events/create_service_spec.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-02-06T08:19:10.396Z
Learnt from: mpressen
Repo: Benefactorum/app PR: 159
File: app/models/osbl_creation.rb:4-10
Timestamp: 2025-02-06T08:19:10.396Z
Learning: The `status` field belongs to the `Contribution` model, not to contributable models like `OsblCreation`. Status represents the state of the contribution itself, regardless of its contributable type.
Applied to files:
app/models/github_event.rb
🧬 Code graph analysis (3)
spec/services/github_events/create_service_spec.rb (3)
app/services/github_events/create_service.rb (2)
call(9-18)contribution(53-55)app/controllers/webhooks/github_events_controller.rb (1)
create(8-18)db/migrate/20250403123232_create_github_event.rb (2)
change(1-13)change(2-12)
app/controllers/webhooks/github_events_controller.rb (1)
app/services/github_events/create_service.rb (1)
call(9-18)
app/services/github_events/create_service.rb (1)
app/models/contribution.rb (1)
osbl_data(101-107)
🪛 GitHub Check: CodeQL
app/controllers/webhooks/github_events_controller.rb
[failure] 2-2: CSRF protection weakened or disabled
Potential CSRF vulnerability due to forgery protection being disabled or weakened.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: deploy
- GitHub Check: test
🔇 Additional comments (10)
config/credentials.yml.enc (1)
1-1: Cannot review encrypted credentials.Encrypted credentials files cannot be inspected during code review. Ensure the updated credentials include the required GitHub webhook secret for signature validation referenced in
Webhooks::GithubEventsController.app/models/github_event.rb (1)
6-10: LGTM!The status enum definition is clean and aligns well with the service processing flow.
config/routes.rb (1)
29-29: LGTM!The route definition correctly exposes the webhook endpoint and follows Rails conventions with proper namespacing.
spec/models/github_event_spec.rb (1)
4-7: LGTM!Basic factory validation test follows RSpec best practices.
db/migrate/20250403123232_create_github_event.rb (1)
3-11: LGTM on migration structure!The table structure, column types, constraints, and foreign key definition are all correctly implemented.
db/schema.rb (2)
122-130: LGTM!The
github_eventstable structure correctly reflects the migration with proper column types, constraints, and indexing.
314-314: LGTM!Foreign key constraint is properly defined.
spec/services/github_events/create_service_spec.rb (1)
14-18: LGTM on status transition validations!The tests properly verify that both the contribution status and github_event status are updated correctly for each review state.
Also applies to: 26-30
app/controllers/webhooks/github_events_controller.rb (1)
1-7: Webhook CSRF/auth skips look appropriate but should remain tightly scopedSkipping
verify_authenticity_token,authenticate, andrequire_authenticationis appropriate for a GitHub webhook that is authenticated via HMAC signature instead of cookies/session. Just make sure this controller is only used for webhook traffic (e.g., dedicated/webhooks/github_eventsPOST route) and not reused for browser‑initiated actions so the relaxed protections don’t leak into user flows.app/services/github_events/create_service.rb (1)
53-60: The specific URL mismatch concern is unfounded—no API URLs are stored or populatedBased on verification of the codebase:
github_resource_urlis set in only one place:app/jobs/create_pull_request_for_osbl_creation_job.rb:61usingpr.html_url(HTML URL)- All tests create contributions with
html_url, not the API URL- The frontend uses it as an
href, which requires HTML format- The commented line in the controller (
payload["pull_request"]["url"]) is dead code—a rejected approach, not activeThe lookup at line 54 using
payload["pull_request"]["html_url"]is correct and consistent with how the field is populated.However, there is a real issue: The
contribution()method usesfind_by!without defensive handling. If no matching contribution exists (e.g., webhook event arrives before the contribution is created, or URL format mismatches), the entire event processing crashes with a 500. Consider usingfind_by+ explicit error handling orexists?guard.For
ignore_event!, logging the contribution ID for consistency is a reasonable suggestion but not critical.Likely an incorrect or invalid review comment.
| def handle_pull_request_event | ||
| ignore_event! unless github_action == "closed" | ||
|
|
||
| return if contribution.status == "validée" | ||
|
|
||
| contribution.update!(status: "rejetée") | ||
| end |
There was a problem hiding this comment.
Control‑flow bug in handle_pull_request_event and missing status update
As written, non‑"closed" pull request actions will still execute the rest of the method after ignore_event!, meaning:
- Contributions can be set to
"rejetée"even when the event should be ignored. - For
"closed"actions, thegithub_eventstatus remains"pending"and is never marked"processed"or linked to the contribution.
This is both a correctness and observability issue.
A safer implementation that truly ignores non‑closed actions and marks processed closed events could be:
def handle_pull_request_event
- ignore_event! unless github_action == "closed"
-
- return if contribution.status == "validée"
-
- contribution.update!(status: "rejetée")
+ unless github_action == "closed"
+ ignore_event!
+ return
+ end
+
+ return if contribution.status == "validée"
+
+ ActiveRecord::Base.transaction do
+ contribution.update!(status: "rejetée")
+ github_event.update!(contribution: contribution, status: "processed")
+ end
endYou can decide whether a "closed" event for an already "validée" contribution should also mark the event as "processed" or remain "pending".
🤖 Prompt for AI Agents
In app/services/github_events/create_service.rb around lines 22-28, the method
must stop executing after ignore_event! for non-"closed" actions and must mark
the github_event as processed and link it to the contribution for all handled
"closed" cases; change control flow so that if github_action != "closed" you
call ignore_event! then return immediately, and for "closed" events always
update the github_event (e.g. status: "processed" and associate the
contribution_id) — if contribution.status == "validée" still mark the
github_event as processed (but don't change the contribution), otherwise update
the contribution to "rejetée" and then mark the github_event processed; use bang
updates (update!) so failures raise and persist changes.
| @@ -0,0 +1,13 @@ | |||
| class CreateGithubEvent < ActiveRecord::Migration[8.0] | |||
There was a problem hiding this comment.
Migration timestamp predates PR creation.
The migration timestamp 20250403123232 (April 3, 2025) is approximately 7.5 months before the PR creation date (November 16, 2025). This backwards-in-time timestamp could cause migration ordering issues, especially in teams where migrations are generated concurrently.
Regenerate the migration with the correct timestamp:
#!/bin/bash
# Verify if there are any migrations between April 2025 and November 2025
# that could conflict with this ordering
fd -e rb . db/migrate/ --exec basename | sort🤖 Prompt for AI Agents
db/migrate/20250403123232_create_github_event.rb lines 1-1: The migration file
timestamp is older than the PR date which can break migration ordering;
regenerate this migration with a current timestamp: create a new migration using
Rails generator (rails generate migration CreateGithubEvent), copy the migration
body from the old file into the newly generated file, delete the old
20250403123232_* file, verify the new filename/timestamp is correct and the
class name matches, run git add/commit the new file and removal of the old one,
and run your migration status check to ensure ordering is correct before
pushing.
2 participants
Note
Add GitHub webhook endpoint that stores events and updates contributions/OSBLs based on PR reviews.
POST /github_events(Webhooks::GithubEventsController#create) with HMAC signature validation; persistsGithubEventand triggers processing.GithubEvents::CreateServiceto handlepull_request_reviewandpull_requestevents:submittedwithchanges_requested→ set contribution to"modifications demandées"and mark eventprocessed.submittedwithapproved→ set contribution to"validée", createOsbl, and mark eventprocessed.closed(if not alreadyvalidée) → set contribution to"rejetée"; otherwise ignore events (status: "ignored").GithubEventmodel withstatusenum and optionalbelongs_to :contribution.github_eventstable (event,payload,status,contribution_id).resources :github_events, only: [:create], module: :webhooks.GithubEventandGithubEvents::CreateServicebehaviors.Written by Cursor Bugbot for commit 5eba857. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Database