Fix GraphQL injection and webhook verification bypass#283
Open
SshauryaaRocks19 wants to merge 4 commits intoOWASP-BLT:mainfrom
Open
Fix GraphQL injection and webhook verification bypass#283SshauryaaRocks19 wants to merge 4 commits intoOWASP-BLT:mainfrom
SshauryaaRocks19 wants to merge 4 commits intoOWASP-BLT:mainfrom
Conversation
📊 Monthly LeaderboardHi @SshauryaaRocks19! Here's how you rank for March 2026:
Scoring this month (across OWASP-BLT org): Open PRs (+1 each), Merged PRs (+10), Closed (not merged) (−2), Reviews (+5; first two per PR in-month), Comments (+2, excludes CodeRabbit). Run |
owasp-blt
bot
added
unresolved-conversations: 0
2 checks pending
needs-peer-review
|
👋 Hi @SshauryaaRocks19! This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:
Once a valid peer review is submitted, this check will pass automatically. Thank you!
|
🍃 PR Readiness CheckCheck the readiness of this PR on Leaf: Leaf reviews pull requests for operational readiness, security risks, and production-impacting changes before they ship. |
WalkthroughReworks batch PR fetching to use GraphQL variables and per-PR aliases, tightens webhook handling to skip upserts for Changes
Sequence Diagram(s)sequenceDiagram
participant WebhookSender as Client (GitHub webhook)
participant API as Server / Handlers
participant Verifier as verify_github_signature
participant Fetcher as fetch_multiple_prs_batch
participant GH as GitHub GraphQL API
participant DB as Database
WebhookSender->>API: POST /api/github/webhook (payload, sig)
API->>Verifier: verify_github_signature(payload, sig)
Verifier-->>API: valid / invalid
alt valid
API->>Fetcher: request PR data (batched variables + aliases)
Fetcher->>GH: POST GraphQL (variables + single query with aliases)
GH-->>Fetcher: batched PR results (alias -> PR)
Fetcher-->>API: parsed per-PR data (may include not_found/not_modified flags)
alt PR data valid
API->>DB: upsert_pr(per-PR data)
else not_found / not_modified
API-->>WebhookSender: skip upsert / return error
end
else invalid
API-->>WebhookSender: reject (invalid signature)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/handlers.py (2)
1177-1179: Same sentinel check should be applied here.For consistency with the fix at line 1080, consider adding sentinel checks before upserting.
🛡️ Suggested defensive fix
# Handle synchronized (new commits) or edited PRs - update data elif action in ['synchronize', 'edited']: # Fetch fresh PR data webhook_token = getattr(env, 'GITHUB_TOKEN', None) fetched_pr_data = await fetch_pr_data(repo_owner, repo_name, pr_number, webhook_token) - if fetched_pr_data: + if fetched_pr_data and not fetched_pr_data.get('not_found') and not fetched_pr_data.get('not_modified'): await upsert_pr(db, pr_url, repo_owner, repo_name, pr_number, fetched_pr_data)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers.py` around lines 1177 - 1179, The fetched_pr_data result should be validated against the same sentinel used earlier before calling upsert_pr; locate the sentinel constant used at the earlier fix (e.g., PR_NOT_FOUND / PR_FETCH_SENTINEL) and change the block around fetch_pr_data and upsert_pr to only call await upsert_pr(db, pr_url, repo_owner, repo_name, pr_number, fetched_pr_data) when fetched_pr_data is truthy and is not equal to that sentinel value (i.e., add the same sentinel check used at line ~1080).
1154-1156: Consider applying the same sentinel checks for consistency.The 'opened' handler at line 1080 now correctly checks for
not_foundandnot_modifiedsentinels before upserting. However, this handler and the 'synchronize'/'edited' handler (lines 1177-1179) still use the simplerif fetched_pr_data:check.While the signature verification fix in
verify_github_signature()blocks the primary attack vector, applying consistent sentinel handling across all upsert paths provides defense-in-depth.🛡️ Suggested defensive fix
# Handle reopened PRs - re-add to tracking if it was tracked before elif action == 'reopened': # Fetch fresh PR data webhook_token = getattr(env, 'GITHUB_TOKEN', None) fetched_pr_data = await fetch_pr_data(repo_owner, repo_name, pr_number, webhook_token) - if fetched_pr_data: + if fetched_pr_data and not fetched_pr_data.get('not_found') and not fetched_pr_data.get('not_modified'): await upsert_pr(db, pr_url, repo_owner, repo_name, pr_number, fetched_pr_data)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers.py` around lines 1154 - 1156, The upsert paths still use a simple truthy check ("if fetched_pr_data:") which skips the defensive sentinel checks used elsewhere; update the handlers that call fetch_pr_data (the 'opened', 'synchronize', and 'edited' handlers that call fetch_pr_data and then upsert_pr) to explicitly check for sentinel values instead of truthiness — e.g. replace "if fetched_pr_data:" with a check that fetched_pr_data is not equal to the not_found and not_modified sentinels (or use "if fetched_pr_data not in (not_found, not_modified):") before calling upsert_pr so all upsert paths consistently ignore sentinel responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/handlers.py`:
- Around line 1177-1179: The fetched_pr_data result should be validated against
the same sentinel used earlier before calling upsert_pr; locate the sentinel
constant used at the earlier fix (e.g., PR_NOT_FOUND / PR_FETCH_SENTINEL) and
change the block around fetch_pr_data and upsert_pr to only call await
upsert_pr(db, pr_url, repo_owner, repo_name, pr_number, fetched_pr_data) when
fetched_pr_data is truthy and is not equal to that sentinel value (i.e., add the
same sentinel check used at line ~1080).
- Around line 1154-1156: The upsert paths still use a simple truthy check ("if
fetched_pr_data:") which skips the defensive sentinel checks used elsewhere;
update the handlers that call fetch_pr_data (the 'opened', 'synchronize', and
'edited' handlers that call fetch_pr_data and then upsert_pr) to explicitly
check for sentinel values instead of truthiness — e.g. replace "if
fetched_pr_data:" with a check that fetched_pr_data is not equal to the
not_found and not_modified sentinels (or use "if fetched_pr_data not in
(not_found, not_modified):") before calling upsert_pr so all upsert paths
consistently ignore sentinel responses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6047af64-ba09-4301-9e2d-0886ac344172
📒 Files selected for processing (2)
src/github_api.pysrc/handlers.py
SshauryaaRocks19
changed the title
|
Good Security improvement, |
Contributor
|
Look into nitpicks too |
…indicates not found or not modified.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/handlers.py (2)
1178-1194:⚠️ Potential issue | 🟡 MinorMissing error handling when fetch fails for
synchronize/editedactions.Same issue as the
reopenedaction above - iffetched_pr_dataisNoneor a sentinel, no response is returned from this block, causing a misleading success response at line 1319.🛡️ Proposed fix to add error handling
return Response.new( json.dumps({ 'success': True, 'event': 'pr_updated', 'pr_id': pr_id, 'pr_number': pr_number, 'data': fetched_pr_data, 'message': f'PR #{pr_number} has been updated' }), {'headers': {'Content-Type': 'application/json'}} ) + else: + return Response.new( + json.dumps({ + 'success': False, + 'event': 'pr_updated', + 'pr_id': pr_id, + 'pr_number': pr_number, + 'message': f'Failed to fetch data for updated PR #{pr_number}' + }), + {'status': 500, 'headers': {'Content-Type': 'application/json'}} + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers.py` around lines 1178 - 1194, The block handling fetched_pr_data for synchronize/edited actions lacks an else path when fetched_pr_data is None or indicates failure, so requests fall through and later code returns a misleading success; update the conditional around fetched_pr_data (the branch that calls upsert_pr, invalidate_readiness_cache, invalidate_timeline_cache and returns Response.new) to add an explicit error response when fetched_pr_data is falsy or contains 'not_found'/'not_modified' (mirror the error handling used in the reopened action), returning a JSON Response.new with success:false, a clear error message referencing pr_number and pr_id, and appropriate headers so failed fetches don't produce a success result.
1155-1171:⚠️ Potential issue | 🟡 MinorMissing error handling when fetch fails for
reopenedaction.If
fetched_pr_dataisNoneor a sentinel value (not_found/not_modified), the function falls through without returning, eventually reaching line 1319 which returns a misleading"no handler configured"success response. This silently masks fetch failures.Compare to the
openedaction at lines 1100-1114 which properly handles this case with error notification and HTTP 500 response.🛡️ Proposed fix to add error handling
return Response.new( json.dumps({ 'success': True, 'event': 'pr_reopened', 'pr_id': pr_id, 'pr_number': pr_number, 'data': fetched_pr_data, 'message': f'PR #{pr_number} has been reopened' }), {'headers': {'Content-Type': 'application/json'}} ) + else: + return Response.new( + json.dumps({ + 'success': False, + 'event': 'pr_reopened', + 'pr_id': pr_id, + 'pr_number': pr_number, + 'message': f'Failed to fetch data for reopened PR #{pr_number}' + }), + {'status': 500, 'headers': {'Content-Type': 'application/json'}} + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers.py` around lines 1155 - 1171, The reopened-action branch currently only returns on success and falls through when fetched_pr_data is None or has not_found/not_modified, causing a misleading "no handler configured" success; update the reopened branch (the block using fetched_pr_data, upsert_pr, invalidate_readiness_cache, invalidate_timeline_cache and Response.new for 'pr_reopened') to mirror the opened-action error handling: detect when fetched_pr_data is falsy or has 'not_found'/'not_modified', call the same error/notification routine used in the opened flow (log/notify about fetch failure), and return an HTTP 500 JSON Response with success: False and an appropriate error message instead of falling through.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/handlers.py`:
- Around line 1178-1194: The block handling fetched_pr_data for
synchronize/edited actions lacks an else path when fetched_pr_data is None or
indicates failure, so requests fall through and later code returns a misleading
success; update the conditional around fetched_pr_data (the branch that calls
upsert_pr, invalidate_readiness_cache, invalidate_timeline_cache and returns
Response.new) to add an explicit error response when fetched_pr_data is falsy or
contains 'not_found'/'not_modified' (mirror the error handling used in the
reopened action), returning a JSON Response.new with success:false, a clear
error message referencing pr_number and pr_id, and appropriate headers so failed
fetches don't produce a success result.
- Around line 1155-1171: The reopened-action branch currently only returns on
success and falls through when fetched_pr_data is None or has
not_found/not_modified, causing a misleading "no handler configured" success;
update the reopened branch (the block using fetched_pr_data, upsert_pr,
invalidate_readiness_cache, invalidate_timeline_cache and Response.new for
'pr_reopened') to mirror the opened-action error handling: detect when
fetched_pr_data is falsy or has 'not_found'/'not_modified', call the same
error/notification routine used in the opened flow (log/notify about fetch
failure), and return an HTTP 500 JSON Response with success: False and an
appropriate error message instead of falling through.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 29f151e6-d026-478b-a5a1-9b1f97a66509
📒 Files selected for processing (1)
src/handlers.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/handlers.py (2)
1172-1176: Consider adding Slack notification for consistency with 'opened' action.The 'opened' action handler (lines 1101-1110) calls
notify_slack_errorwhen PR data fetch fails, but this 'reopened' path does not. For consistent observability and debugging, consider adding the same notification here.♻️ Suggested change for parity with 'opened' handler
else: + await notify_slack_error( + getattr(env, 'SLACK_ERROR_WEBHOOK', ''), + error_type='WebhookError', + error_message='Failed to fetch PR data from GitHub', + context={ + 'handler': 'handle_github_webhook', + 'action': 'reopened', + 'pr_number': str(pr_number), + 'repo': f'{repo_owner}/{repo_name}', + }, + ) return Response.new( json.dumps({'error': 'Failed to fetch PR data from GitHub'}), {'status': 500, 'headers': {'Content-Type': 'application/json'}} )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers.py` around lines 1172 - 1176, The reopened-PR error branch returns a 500 response but lacks the Slack notify call used in the opened-PR path; update the else branch that returns Response.new({...}) to also invoke notify_slack_error with the same arguments used in the opened handler (i.e., pass the error context/PR identifier and the error message) before returning so that notify_slack_error is called in the reopened PR flow just like in the opened flow.
1200-1204: Same observation: Missing Slack notification for consistency.Similar to the 'reopened' handler, this path lacks the
notify_slack_errorcall that exists in the 'opened' handler. Consider adding it for observability parity.♻️ Suggested change
else: + await notify_slack_error( + getattr(env, 'SLACK_ERROR_WEBHOOK', ''), + error_type='WebhookError', + error_message='Failed to fetch PR data from GitHub', + context={ + 'handler': 'handle_github_webhook', + 'action': action, + 'pr_number': str(pr_number), + 'repo': f'{repo_owner}/{repo_name}', + }, + ) return Response.new( json.dumps({'error': 'Failed to fetch PR data from GitHub'}), {'status': 500, 'headers': {'Content-Type': 'application/json'}} )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers.py` around lines 1200 - 1204, The else branch that returns Response.new(json.dumps({'error': 'Failed to fetch PR data from GitHub'}), ...) is missing a call to notify_slack_error for observability; update the handler (the same one that mirrors the 'opened'/'reopened' handlers) to invoke notify_slack_error with a clear message and contextual data (e.g., the error string and relevant PR/event identifiers or payload) immediately before returning the 500 Response so Slack is notified of the failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/handlers.py`:
- Around line 1172-1176: The reopened-PR error branch returns a 500 response but
lacks the Slack notify call used in the opened-PR path; update the else branch
that returns Response.new({...}) to also invoke notify_slack_error with the same
arguments used in the opened handler (i.e., pass the error context/PR identifier
and the error message) before returning so that notify_slack_error is called in
the reopened PR flow just like in the opened flow.
- Around line 1200-1204: The else branch that returns
Response.new(json.dumps({'error': 'Failed to fetch PR data from GitHub'}), ...)
is missing a call to notify_slack_error for observability; update the handler
(the same one that mirrors the 'opened'/'reopened' handlers) to invoke
notify_slack_error with a clear message and contextual data (e.g., the error
string and relevant PR/event identifiers or payload) immediately before
returning the 500 Response so Slack is notified of the failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b601ece5-4b70-4f51-a219-3744b24f4977
📒 Files selected for processing (1)
src/handlers.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #282
This pull request resolves a chained vulnerability that allowed unauthenticated attackers to execute arbitrary GraphQL queries against the GitHub API via forged webhooks.
Changes Made:
Parameterised GraphQL Queries (
src/github_api.py): Thefetch_multiple_prs_batch()function was redesigned to replace f-string interpolation with appropriate GraphQL variables. By securely passing variables alongside the query instead of injecting them directly into the query string, this totally mitigates the injection vector.Webhook "Opened" Handler Validation (
src/handlers.py): To appropriately handle not_found and not_modified sentinel values, the handle_github_webhook() function was updated. In order to stop attacker-controlled fake repository data from contaminating the database, the handler now explicitly verifies that the PR data is valid before callingupsert_pr().Enforced Webhook Security (
src/github_api.py):verify_github_signature()was modified toreturn FalsewhenGITHUB_WEBHOOK_SECRETis not configured.All three broken links in the attack chain are now patched, protecting private repository data and preventing API rate-limit exhaustion.
Summary by CodeRabbit
Bug Fixes
Refactor