feat: Implement auto bounty payout for merged PRs (fixes #3941) #4633
Conversation
- Add preferred_payment_method field to UserProfile model - Create /api/bounty_payout/ endpoint with authentication - Add GitHub Action workflow to trigger on PR merge - Implement duplicate payment prevention - Add comprehensive error handling and validation Addresses issues found in CodeRabbit review of PR OWASP-BLT#4236: - Uses curl instead of github.request() for API calls - Proper header authentication (X-BLT-API-TOKEN) - No duplicate class definitions - Consistent data types - Fixed migration dependencies
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an automated bounty payout flow: a GitHub Actions workflow posts linked issues from merged PRs to a new CSRF-exempt API endpoint which validates and executes GitHub Sponsors payouts; introduces UserProfile and GitHubIssue fields and migrations, settings/env, admin UI updates, docs, and CI/test adjustments. Changes
Sequence DiagramsequenceDiagram
participant GH as GitHub
participant WF as GitHub Actions Workflow
participant BLT as BLT App (API)
participant GHS as GitHub Sponsors
participant DB as Database
participant IC as Issue Comments
GH->>WF: PR closed (merged)
WF->>WF: Extract linked issue numbers (Fixes/Closes/Resolves)
alt linked issues found
loop per issue
WF->>BLT: POST /api/bounty_payout/ (issue_url, pr_url, token)
BLT->>BLT: Validate token & parse URLs
BLT->>GH: GET issue (and PR if provided)
alt issue closed & bounty label ($...)
BLT->>DB: Map assignee via SocialAccount
BLT->>DB: Check duplicates / preferred_payment_method
alt eligible for sponsors payout
BLT->>GHS: Create sponsorship (GraphQL)
GHS-->>BLT: sponsorship created
BLT->>GHS: Cancel sponsorship (with retries)
GHS-->>BLT: cancellation result
BLT->>DB: Persist transaction fields on GitHubIssue
BLT->>IC: Post success comment on issue
BLT-->>WF: 200 OK
else duplicate or unsupported method
BLT->>IC: Post warning/comment
BLT-->>WF: 4xx/err
end
else no bounty / issue not closed
BLT->>IC: Skip/post note
end
end
else no linked issues
WF->>WF: Skip payout processing
end
WF->>WF: Final summary step (always)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
.github/workflows/auto-bounty-payout.yml(1 hunks)blt/urls.py(2 hunks)website/migrations/0247_userprofile_preferred_payment_method.py(1 hunks)website/models.py(1 hunks)website/views/organization.py(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.8)
.github/workflows/auto-bounty-payout.yml
18-18: "github.event.pull_request.body" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/reference/security/secure-use#good-practices-for-mitigating-script-injection-attacks for more details
(expression)
🪛 GitHub Check: CodeQL
website/views/organization.py
[warning] 3951-3951: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
.github/workflows/auto-bounty-payout.yml
[warning] 10-104: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{}}
🪛 Ruff (0.14.0)
website/views/organization.py
3766-3766: f-string without any placeholders
Remove extraneous f prefix
(F541)
3915-3915: Unpacked variable created is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
3948-3948: Do not catch blind exception: Exception
(BLE001)
3949-3949: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
3949-3949: Use explicit conversion flag
Replace with conversion flag
(RUF010)
website/migrations/0247_userprofile_preferred_payment_method.py
8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
12-25: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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: auto-assign
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (4)
website/models.py (1)
734-741: LGTM!The
preferred_payment_methodfield is correctly defined with appropriate choices, default value, and help text. The field configuration aligns with the migration and the bounty payout workflow requirements.website/migrations/0247_userprofile_preferred_payment_method.py (1)
6-25: LGTM!The migration correctly adds the
preferred_payment_methodfield to the UserProfile model with proper configuration matching the model definition. The dependency on migration 0246 is appropriate.Note: The Ruff warnings about
ClassVarannotations are false positives specific to Django migrations and can be safely ignored.blt/urls.py (1)
215-215: Authentication implementation verified
Theprocess_bounty_payoutview readsX-BLT-API-TOKEN, compares it tosettings.BLT_API_TOKEN, and returns 401 on mismatch..github/workflows/auto-bounty-payout.yml (1)
56-56: Confirm bounty label precedenceThe workflow selects only the first
$-prefixed label when multiple are present; we found no historical issues with more than one. Ensure this aligns with your expected payout logic.
github-project-automation
bot
moved this from Backlog
to Ready
in 📌 OWASP BLT Project Board
- Add explicit permissions block to workflow (least privilege) - Fix script injection vulnerability by using environment variable for PR body - Make API URL configurable via BLT_API_URL secret - Fix exception handling to not expose stack traces - Fix linting issues (unused variable, f-string without placeholders) - Use logger.exception instead of logger.error for better debugging
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
website/views/organization.py (3)
3915-3915: Prefix unused variable with underscore.The
_createdvariable fromupdate_or_createis never used. While already prefixed with underscore, this was also flagged in the past review comments.If you need to use the created flag for logging, keep it. Otherwise, the underscore prefix is sufficient to indicate it's intentionally unused.
3798-3812: Validate that the issue is closed before processing payment.Per the previous review comment, the endpoint must verify the issue state is "closed" before initiating any payout. Currently, it fetches the issue but never checks
issue_data["state"], allowing payments for open issues.Apply this diff after line 3812:
issue_data = response.json() + +# Verify issue is closed +if issue_data.get("state") != "closed": + logger.warning(f"Attempted payout for non-closed issue #{issue_number}") + return JsonResponse( + {"success": False, "error": "Issue must be closed before payout"}, + status=400 + )
3772-3775: Validate that the linked PR is actually merged.Per the previous review comment, the endpoint should verify that the PR mentioned in
pr_urlis merged before processing payment. Currently,pr_urlis parsed but never used for validation.Add PR merge validation after checking the issue state. You can add this new function before
process_bounty_payout:def verify_pr_merged(pr_url): """Verify that a pull request is merged.""" if not pr_url: return True # If no PR URL provided, skip validation # Extract PR details from URL parts = pr_url.rstrip("/").split("/") if len(parts) < 7 or parts[5] != "pull": return False owner = parts[3] repo_name = parts[4] pr_number = parts[6] # Fetch PR from GitHub API headers = {} if settings.GITHUB_TOKEN: headers["Authorization"] = f"token {settings.GITHUB_TOKEN}" api_url = f"https://api.github.com/repos/{owner}/{repo_name}/pulls/{pr_number}" try: response = requests.get(api_url, headers=headers, timeout=10) if response.status_code == 200: pr_data = response.json() return pr_data.get("merged", False) except requests.RequestException: pass return FalseThen add this check after the issue state validation:
if issue_data.get("state") != "closed": logger.warning(f"Attempted payout for non-closed issue #{issue_number}") return JsonResponse( {"success": False, "error": "Issue must be closed before payout"}, status=400 ) + +# Verify linked PR is merged if provided +if pr_url and not verify_pr_merged(pr_url): + logger.warning(f"Attempted payout for issue #{issue_number} with unmerged PR") + return JsonResponse( + {"success": False, "error": "Linked PR must be merged before payout"}, + status=400 + )
🧹 Nitpick comments (3)
website/views/organization.py (3)
3903-3913: Move duplicate payment check earlier to avoid unnecessary GitHub API calls.The duplicate payment check occurs after fetching and validating the issue from GitHub. If the issue was already paid, we've wasted an API call and processing time. Check for duplicates immediately after parsing the issue_url.
Move the duplicate check to right after line 3796:
owner = parts[3] repo_name = parts[4] issue_number = parts[6] + +# Check for duplicate payment early to avoid unnecessary API calls +from website.models import Repo, GitHubIssue +repo_url = f"https://github.com/{owner}/{repo_name}" +repo = Repo.objects.filter(repo_url=repo_url).first() +if repo: + existing_issue = GitHubIssue.objects.filter(url=issue_url, repo=repo).first() + if existing_issue and (existing_issue.sponsors_tx_id or existing_issue.bch_tx_id): + logger.warning(f"Bounty already paid for issue {issue_url}") + return JsonResponse({ + "success": False, + "error": "Bounty already paid for this issue", + "paid_at": existing_issue.p2p_payment_created_at.isoformat() if existing_issue.p2p_payment_created_at else None + }, status=400) # Fetch issue from GitHub API headers = {}Then remove the duplicate check at lines 3903-3913.
3876-3876: Remove unused hardcoded variable.The
sponsor_usernamevariable is assigned but never used. Either remove it or use it in the actual GitHub Sponsors API implementation.-# Process GitHub Sponsors payment -# Hardcoded to DonnieBLT as per issue requirements -sponsor_username = "DonnieBLT" - # Call GitHub Sponsors API to create payment
3934-3936: Document the placeholder transaction ID format.The placeholder transaction ID uses a timestamp-based format that isn't documented. This could cause confusion when the real GitHub Sponsors API is implemented, or if someone tries to validate the transaction ID format.
Add a comment explaining the format:
# Mark as paid with sponsors (placeholder transaction ID) # In production, this would be the actual transaction ID from GitHub Sponsors API +# Placeholder format: pending_{unix_timestamp} - will be replaced with actual Sponsors tx ID (format: ch_XXXX) github_issue.sponsors_tx_id = f"pending_{timezone.now().timestamp()}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
.github/workflows/auto-bounty-payout.yml(1 hunks)website/views/organization.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/auto-bounty-payout.yml
🧰 Additional context used
🪛 Ruff (0.14.0)
website/views/organization.py
3948-3948: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
⏰ 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). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
website/views/organization.py
Outdated
| } | ||
| ) | ||
|
|
||
| # Mark as paid with sponsors (placeholder transaction ID) |
There was a problem hiding this comment.
Please add the logic to process the payment
There was a problem hiding this comment.
Pull Request Overview
This PR implements automated bounty payout processing when PRs are merged that close issues with bounty labels. The system detects linked issues from PR descriptions, validates bounty labels, and initiates payments through GitHub Sponsors API integration.
Key Changes
- Added preferred payment method field to user profiles for bounty payout routing
- Created API endpoint for processing bounty payouts with validation and duplicate prevention
- Implemented GitHub Actions workflow to automatically trigger payouts on merged PRs
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| website/views/organization.py | Implements bounty payout API endpoint with issue validation, user lookup, and payment processing |
| website/models.py | Adds preferred_payment_method field to UserProfile model for payment routing |
| website/migrations/0247_userprofile_preferred_payment_method.py | Database migration to add the new payment preference field |
| blt/urls.py | Registers the new bounty payout API endpoint |
| .github/workflows/auto-bounty-payout.yml | GitHub Actions workflow to detect bounty issues and trigger automated payouts |
website/views/organization.py
Outdated
| return JsonResponse( | ||
| {"success": False, "error": f"Failed to fetch issue from GitHub: {response.status_code}"}, |
There was a problem hiding this comment.
Error message should not include generic exception details. Provide more specific information about the GitHub API failure.
| return JsonResponse( | |
| {"success": False, "error": f"Failed to fetch issue from GitHub: {response.status_code}"}, | |
| if response.status_code == 404: | |
| error_msg = "GitHub issue not found." | |
| elif response.status_code in (401, 403): | |
| error_msg = "Authentication with GitHub failed or access denied." | |
| elif response.status_code == 429: | |
| error_msg = "GitHub API rate limit exceeded. Please try again later." | |
| else: | |
| error_msg = "Failed to fetch issue from GitHub." | |
| return JsonResponse( | |
| {"success": False, "error": error_msg}, |
website/views/organization.py
Outdated
|
|
||
| # Mark as paid with sponsors (placeholder transaction ID) | ||
| # In production, this would be the actual transaction ID from GitHub Sponsors API | ||
| github_issue.sponsors_tx_id = f"pending_{timezone.now().timestamp()}" |
There was a problem hiding this comment.
Using timestamp as a placeholder transaction ID could create conflicts. Consider using UUIDs or a more robust identifier format.
| logger.exception("Error processing bounty payout") | ||
| return JsonResponse( | ||
| {"success": False, "error": "Internal server error"}, | ||
| status=500 | ||
| ) |
There was a problem hiding this comment.
The error message follows the custom guideline to not include exception details but should be more descriptive about what operation failed.
website/views/organization.py
Outdated
| logger.info(f"Processing payment for assignee: {assignee_username}") | ||
|
|
||
| # Find user profile by GitHub username | ||
| from allauth.socialaccount.models import SocialAccount |
There was a problem hiding this comment.
Import statements should be placed at the top of the file rather than inside functions for better readability and performance.
website/views/organization.py
Outdated
| from website.models import Repo | ||
|
|
There was a problem hiding this comment.
Import statements should be placed at the top of the file rather than inside functions for better readability and performance.
| from website.models import Repo | |
| # BLT_API_URL can be set in repository secrets/variables for testing | ||
| # Defaults to production if not set | ||
| # Add BLT_API_TOKEN to repository secrets for authentication | ||
| api_url="${{ secrets.BLT_API_URL || 'https://blt.owasp.org' }}" |
There was a problem hiding this comment.
The hardcoded production URL should be moved to a configuration variable or environment-specific setting to avoid accidental production calls during testing.
| api_url="${{ secrets.BLT_API_URL || 'https://blt.owasp.org' }}" | |
| if [ -z "${{ secrets.BLT_API_URL }}" ]; then | |
| echo "❌ BLT_API_URL is not set. Refusing to call production API by default. Please set BLT_API_URL in repository secrets/variables." | |
| exit 1 | |
| fi | |
| api_url="${{ secrets.BLT_API_URL }}" |
…w feedback Addresses feedback from: - @DonnieBLT: 'Please add the logic to process the payment' - CodeRabbit: URL validation, unused variables, import order, error messages - Copilot AI: Import placement, UUID for transaction IDs, error specificity GitHub Sponsors Integration: - Fetch user's sponsorable ID and available tiers via GraphQL - Find matching tier for bounty amount (or closest available) - Create sponsorship via createSponsorship mutation - Store real sponsorship ID from GitHub API - Handle GitHub's broken one-time payment API (uses tier-based instead) Security & Code Quality Fixes: - Move duplicate payment check BEFORE API call (prevents double charges) - Add URL validation with regex for owner/repo/issue_number - Move imports to top of file (SocialAccount, uuid) - Make sponsor username configurable via GITHUB_SPONSOR_USERNAME setting - Use UUID instead of timestamp for better uniqueness - Improve error messages with specific details - Remove unused exception variable - Fix import order for isort compliance - Add comprehensive error handling for all scenarios Technical Details: - Uses GitHub GraphQL API (not REST) - Validates user has GitHub Sponsors enabled - Checks for existing tiers before payment - Handles network errors and API failures gracefully - Logs all operations for debugging Requirements: - GITHUB_TOKEN with user/read:org scopes - Recipients must have GitHub Sponsors with tiers configured - GITHUB_SPONSOR_USERNAME setting (defaults to DonnieBLT) - BLT_API_TOKEN for webhook authentication (optional) Known Limitations: - GitHub one-time payment API is broken (documented issue) - Uses recurring sponsorships (can be cancelled manually) - Requires matching or closest tier amount - No sandbox mode (creates real sponsorships)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
website/views/organization.py (1)
3826-3859: Block payout when the issue is still open.We still skip any state check, so a misfired Action (or a crafted request) can pay bounties on open issues. Please bail out unless
issue_data.get("state") == "closed"(and ideally confirm the linked PR merged) before touching payments. This was called out earlier and remains unresolved — let’s close it out now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
PR_DESCRIPTION.md(1 hunks)blt/urls.py(2 hunks)website/views/organization.py(3 hunks)
🧰 Additional context used
🪛 LanguageTool
PR_DESCRIPTION.md
[grammar] ~9-~9: There might be a mistake here.
Context: ... Sponsors API. ## Changes ### Database - Added preferred_payment_method field t...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...rred_payment_method.py ### Backend API - Created new endpoint/api/bounty_payout...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...d new endpoint /api/bounty_payout/ in website/views/organization.py - Accepts POST requests with issue URL and...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...uests with issue URL and optional PR URL - Validates issue has bounty label and ass...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...ates issue has bounty label and assignee - Checks if user exists in BLT with linked...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...exists in BLT with linked GitHub account - Prevents duplicate payments by checking ...
(QB_NEW_EN)
[grammar] ~18-~18: There might be a mistake here.
Context: ...nts by checking existing transaction IDs - Returns appropriate error messages for v...
(QB_NEW_EN)
[grammar] ~19-~19: There might be a mistake here.
Context: ...error messages for various failure cases - Added optional authentication via `X-BLT...
(QB_NEW_EN)
[grammar] ~20-~20: There might be a mistake here.
Context: ...hentication via X-BLT-API-TOKEN header - Added URL route in blt/urls.py ### Gi...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ...verifies the assignee has a BLT account with connected GitHub profile, checks their ...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...rsion addresses several critical issues: - Uses curl for API calls instead of githu...
(QB_NEW_EN)
[uncategorized] ~40-~40: The official name of this software platform is spelled with a capital “H”.
Context: ...s: - Uses curl for API calls instead of github.request() which only works for GitHub's...
(GITHUB)
[grammar] ~50-~50: There might be a mistake here.
Context: ...ntation will be added in future work. - Only GitHub Sponsors payment method is curre...
(QB_NEW_EN)
[grammar] ~52-~52: There might be a mistake here.
Context: ... - Workflow API URL needs to be updated to production URL before deployment. ## T...
(QB_NEW_EN)
[grammar] ~56-~56: There might be a mistake here.
Context: ... Migration dependency has been verified against latest migration (0246). Error handling...
(QB_NEW_EN)
[grammar] ~62-~62: There might be a mistake here.
Context: ... ## Deployment Notes 1. Run migration: python manage.py migrate 2. Add BLT_API_TOKEN to Django settings 3. ...
(QB_NEW_EN)
🪛 Ruff (0.14.0)
website/views/organization.py
3999-3999: Redundant exception object included in logging.exception call
(TRY401)
4070-4070: Redundant exception object included in logging.exception call
(TRY401)
⏰ 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). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
website/views/organization.py
Outdated
| mutation($sponsorableId: ID!, $tierId: ID!) { | ||
| createSponsorship(input: { | ||
| sponsorableId: $sponsorableId | ||
| tierId: $tierId | ||
| privacyLevel: PUBLIC | ||
| }) { | ||
| sponsorship { | ||
| id | ||
| tier { | ||
| monthlyPriceInCents | ||
| name | ||
| } | ||
| createdAt | ||
| } | ||
| } | ||
| } | ||
| """ | ||
|
|
||
| graphql_variables = { | ||
| "sponsorableId": sponsorable_id, | ||
| "tierId": tier_id, | ||
| } | ||
|
|
||
| try: | ||
| graphql_response = requests.post( | ||
| graphql_url, | ||
| json={"query": graphql_mutation, "variables": graphql_variables}, | ||
| headers=graphql_headers, | ||
| timeout=30 | ||
| ) | ||
|
|
||
| if graphql_response.status_code != 200: | ||
| logger.error(f"GitHub Sponsors API returned status {graphql_response.status_code}: {graphql_response.text}") | ||
| return JsonResponse({ | ||
| "success": False, | ||
| "error": f"Failed to process GitHub Sponsors payment (HTTP {graphql_response.status_code}). The user may not have GitHub Sponsors enabled." | ||
| }, status=400) | ||
|
|
||
| graphql_data = graphql_response.json() | ||
|
|
||
| # Check for GraphQL errors | ||
| if "errors" in graphql_data: | ||
| error_messages = [err.get("message", "Unknown error") for err in graphql_data["errors"]] | ||
| logger.error(f"GitHub Sponsors GraphQL errors: {error_messages}") | ||
| return JsonResponse({ | ||
| "success": False, | ||
| "error": f"GitHub Sponsors API error: {'; '.join(error_messages)}" | ||
| }, status=400) | ||
|
|
||
| # Extract sponsorship ID from response | ||
| sponsorship_data = graphql_data.get("data", {}).get("createSponsorship", {}).get("sponsorship", {}) | ||
| sponsorship_id = sponsorship_data.get("id") | ||
|
|
||
| if not sponsorship_id: | ||
| logger.error(f"No sponsorship ID in response: {graphql_data}") | ||
| return JsonResponse({ | ||
| "success": False, | ||
| "error": "Failed to create sponsorship. Please ensure the recipient has GitHub Sponsors enabled." | ||
| }, status=400) | ||
|
|
||
| logger.info(f"Successfully created GitHub Sponsors payment: {sponsorship_id}") | ||
|
|
||
| except requests.exceptions.RequestException as e: | ||
| logger.exception(f"Network error calling GitHub Sponsors API: {e}") | ||
| return JsonResponse({ | ||
| "success": False, | ||
| "error": "Network error while processing payment. Please try again later." | ||
| }, status=500) |
There was a problem hiding this comment.
Creating a GitHub Sponsors payout here opens a recurring subscription.
createSponsorship creates a monthly sponsorship that keeps billing until explicitly cancelled. Calling it for every bounty will enroll the sponsor in recurring charges for each issue — that’s a catastrophic correctness bug. Until we have a safe one-time payout flow (or an immediate cancel step), we must not invoke this mutation. Revert to the placeholder/record-only behaviour or add proper cancellation logic and guardrails.
🧰 Tools
🪛 Ruff (0.14.0)
4070-4070: Redundant exception object included in logging.exception call
(TRY401)
🤖 Prompt for AI Agents
website/views/organization.py lines 4007-4074: the code currently calls GitHub's
createSponsorship GraphQL mutation which creates a recurring monthly
subscription for the sponsor; stop invoking that mutation. Revert this flow to a
record-only / placeholder behavior: remove the external requests.post that
executes createSponsorship, instead persist a local "intended_payout" record (or
update existing placeholder logic) with sponsorable_id, tier_id, amount, and
metadata; return a safe JsonResponse indicating the payout was recorded and
requires manual or deferred processing. If you must enable an automated flow,
gate the external mutation behind a strict feature flag and implement an
immediate cancellation step (call the appropriate GitHub cancellation mutation),
idempotency checks, confirmation/consent UI, and tests — otherwise do not call
GitHub and do not create subscriptions.
PR_DESCRIPTION.md
Outdated
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
website/views/organization.py (5)
3749-3761: Add @csrf_exempt decorator to allow GitHub Actions to call this endpoint.This endpoint is called by a GitHub Action workflow that cannot provide a CSRF token. Without
@csrf_exempt, all requests will fail with 403 Forbidden due to Django's CSRF protection.Apply this diff:
+from django.views.decorators.csrf import csrf_exempt + @require_POST +@csrf_exempt def process_bounty_payout(request): """ API endpoint to process bounty payouts for merged PRs with attached issues.
3826-3846: Verify that the issue is closed before processing payment.After fetching the issue data, the code immediately proceeds to validate the bounty and process payment without checking if the issue is actually closed. A misfired workflow or crafted request could mark a bounty as paid while the issue is still open.
Apply this diff after line 3826:
issue_data = response.json() + +# Verify issue is closed before processing payment +if issue_data.get("state") != "closed": + return JsonResponse( + {"success": False, "error": "Issue must be closed before bounty payout"}, + status=400 + ) # Check if issue has dollar tag (bounty)
3831-3840: Fix bounty label parsing to handle common label formats.The current parsing will fail on labels like "$50 – medium" or "$1,000 reward" and incorrectly default to $5, potentially underpaying bounties. Extract the leading numeric portion instead.
Apply this diff:
+import re + # Check if issue has dollar tag (bounty) has_bounty = False bounty_amount = 0 +amount_pattern = re.compile(r"^\$?\s*(\d+)") for label in issue_data.get("labels", []): label_name = label.get("name", "") - if label_name.startswith("$"): + match = amount_pattern.match(label_name) + if match: has_bounty = True - # Extract amount from label (e.g., "$5" -> 5) - try: - bounty_amount = int(label_name.replace("$", "").strip()) - except ValueError: - bounty_amount = 5 # Default to $5 + bounty_amount = int(match.group(1)) break
3974-3994: Fix tier selection to never downgrade the bounty amount.The current logic using
min(..., key=abs(diff))can select a tier LOWER than the bounty amount (e.g., paying $5 for a $25 bounty). Require an exact match or the next tier up, and fail if no suitable tier exists.Apply this diff:
# Find exact match or closest tier matching_tier = None for tier in tiers: if tier.get("monthlyPriceInCents") == target_amount_cents: matching_tier = tier break -# If no exact match, find closest tier +# If no exact match, find next tier up (never downgrade) if not matching_tier and tiers: - matching_tier = min(tiers, key=lambda t: abs(t.get("monthlyPriceInCents", 0) - target_amount_cents)) - logger.warning(f"No exact tier match for ${bounty_amount}, using closest: ${matching_tier.get('monthlyPriceInCents', 0) / 100}") + # Sort tiers by price ascending and find first tier >= target + sorted_tiers = sorted(tiers, key=lambda t: t.get("monthlyPriceInCents", 0)) + for tier in sorted_tiers: + tier_amount = tier.get("monthlyPriceInCents", 0) + if tier_amount >= target_amount_cents: + matching_tier = tier + logger.warning(f"No exact tier match for ${bounty_amount}, using next tier up: ${tier_amount / 100}") + break if not matching_tier: return JsonResponse({ "success": False, - "error": f"No sponsorship tiers available for {assignee_username}. They need to set up sponsor tiers first." + "error": f"No suitable sponsorship tier found for ${bounty_amount}. User needs a tier at or above this amount." }, status=400)
4005-4074: CRITICAL: createSponsorship creates a recurring subscription, not a one-time payment.Calling
createSponsorshipfor every bounty creates a recurring monthly subscription that charges the sponsor indefinitely until explicitly cancelled. This is catastrophic: if 10 bounties are paid, the sponsor is enrolled in 10 recurring monthly subscriptions that will continue charging forever.This mutation must NOT be invoked for bounty payments. Revert to a placeholder/record-only flow until a safe one-time payout mechanism is implemented, or add immediate cancellation logic, idempotency checks, and proper safeguards.
Apply this diff:
- # Step 3: Create sponsorship - graphql_mutation = """ - mutation($sponsorableId: ID!, $tierId: ID!) { - createSponsorship(input: { - sponsorableId: $sponsorableId - tierId: $tierId - privacyLevel: PUBLIC - }) { - sponsorship { - id - tier { - monthlyPriceInCents - name - } - createdAt - } - } - } - """ - - graphql_variables = { - "sponsorableId": sponsorable_id, - "tierId": tier_id, - } - - try: - graphql_response = requests.post( - graphql_url, - json={"query": graphql_mutation, "variables": graphql_variables}, - headers=graphql_headers, - timeout=30 - ) - - if graphql_response.status_code != 200: - logger.error(f"GitHub Sponsors API returned status {graphql_response.status_code}: {graphql_response.text}") - return JsonResponse({ - "success": False, - "error": f"Failed to process GitHub Sponsors payment (HTTP {graphql_response.status_code}). The user may not have GitHub Sponsors enabled." - }, status=400) - - graphql_data = graphql_response.json() - - # Check for GraphQL errors - if "errors" in graphql_data: - error_messages = [err.get("message", "Unknown error") for err in graphql_data["errors"]] - logger.error(f"GitHub Sponsors GraphQL errors: {error_messages}") - return JsonResponse({ - "success": False, - "error": f"GitHub Sponsors API error: {'; '.join(error_messages)}" - }, status=400) - - # Extract sponsorship ID from response - sponsorship_data = graphql_data.get("data", {}).get("createSponsorship", {}).get("sponsorship", {}) - sponsorship_id = sponsorship_data.get("id") - - if not sponsorship_id: - logger.error(f"No sponsorship ID in response: {graphql_data}") - return JsonResponse({ - "success": False, - "error": "Failed to create sponsorship. Please ensure the recipient has GitHub Sponsors enabled." - }, status=400) - - logger.info(f"Successfully created GitHub Sponsors payment: {sponsorship_id}") - - except requests.exceptions.RequestException as e: - logger.exception(f"Network error calling GitHub Sponsors API: {e}") - return JsonResponse({ - "success": False, - "error": "Network error while processing payment. Please try again later." - }, status=500) + # TODO: Implement safe one-time payment flow + # For now, use placeholder transaction ID pending full implementation + sponsorship_id = f"pending_{assignee_username}_{int(time.time())}" + + logger.warning(f"Using placeholder sponsorship ID: {sponsorship_id}. Full GitHub Sponsors integration pending.")
🧹 Nitpick comments (1)
website/views/organization.py (1)
3999-3999: Remove redundant exception object from logging.exception calls.
logger.exception()automatically logs the exception traceback, so passing the exception object explicitly is redundant.Apply this diff:
- logger.exception(f"Network error fetching user info: {e}") + logger.exception("Network error fetching user info")- logger.exception(f"Network error calling GitHub Sponsors API: {e}") + logger.exception("Network error calling GitHub Sponsors API")Also applies to: 4070-4070
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
website/views/organization.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
website/views/organization.py
3999-3999: Redundant exception object included in logging.exception call
(TRY401)
4070-4070: Redundant exception object included in logging.exception call
(TRY401)
⏰ 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). (1)
- GitHub Check: Run Tests
|
Thanks for the feedback. I propose two options: Option 1: Revert to Safe Placeholder (Recommended)
Option 2: Add Immediate Cancellation Logic (Risky)
RecommendationI strongly recommend Option 1 for safety, reliability, and ease of testing while keeping admin control over payouts. Would you like me to proceed with Option 1? If you prefer Option 2, I can implement it with precautions. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
website/views/organization.py (2)
3748-3749: CRITICAL: Add @csrf_exempt decorator to allow GitHub Actions to call this endpoint.This endpoint is invoked by GitHub Actions workflows that cannot provide a CSRF token. Without
@csrf_exempt, all requests will fail with 403 Forbidden errors.Apply this diff to add the decorator:
+from django.views.decorators.csrf import csrf_exempt + @require_POST +@csrf_exempt def process_bounty_payout(request): """ API endpoint to process bounty payouts for merged PRs with attached issues.Note: The endpoint already has token-based authentication via
X-BLT-API-TOKENheader check (lines 3762-3771), so CSRF protection is not needed.Based on past review comments.
3827-3832: Verify that a linked PR was actually merged before paying.The code checks if the issue is closed (line 3828), but doesn't verify that a pull request was merged to close it. Issues can be closed manually or for other reasons (duplicate, won't fix, etc.) without merged code.
Add PR merge verification after the closed state check:
if issue_data.get("state") != "closed": return JsonResponse( {"success": False, "error": "Issue must be closed before bounty payout"}, status=400 ) + +# Verify at least one linked PR was merged +# GitHub includes timeline events in issue data when requested with proper headers +# Alternative: Check if PR URL was provided and fetch its merge status +if pr_url: + pr_parts = pr_url.rstrip("/").split("/") + if len(pr_parts) >= 7: + pr_owner, pr_repo, pr_number = pr_parts[3], pr_parts[4], pr_parts[6] + pr_api_url = f"https://api.github.com/repos/{pr_owner}/{pr_repo}/pulls/{pr_number}" + pr_response = requests.get(pr_api_url, headers=headers, timeout=10) + if pr_response.status_code == 200: + pr_data = pr_response.json() + if not pr_data.get("merged", False): + return JsonResponse( + {"success": False, "error": "Linked PR must be merged before bounty payout"}, + status=400 + ) + else: + return JsonResponse( + {"success": False, "error": "Failed to verify PR merge status"}, + status=400 + )Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
.github/workflows/auto-bounty-payout.yml(1 hunks)website/views/organization.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/auto-bounty-payout.yml
🧰 Additional context used
🪛 Ruff (0.14.0)
website/views/organization.py
3837-3837: Comment contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF003)
4015-4015: Redundant exception object included in logging.exception call
(TRY401)
4136-4136: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
4155-4155: Redundant exception object included in logging.exception call
(TRY401)
⏰ 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). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
website/views/organization.py
Outdated
| # Step 3: Create sponsorship | ||
| graphql_mutation = """ | ||
| mutation($sponsorableId: ID!, $tierId: ID!) { | ||
| createSponsorship(input: { | ||
| sponsorableId: $sponsorableId | ||
| tierId: $tierId | ||
| privacyLevel: PUBLIC | ||
| }) { | ||
| sponsorship { | ||
| id | ||
| tier { | ||
| monthlyPriceInCents | ||
| name | ||
| } | ||
| createdAt | ||
| } | ||
| } | ||
| } | ||
| """ | ||
|
|
||
| graphql_variables = { | ||
| "sponsorableId": sponsorable_id, | ||
| "tierId": tier_id, | ||
| } | ||
|
|
||
| try: | ||
| graphql_response = requests.post( | ||
| graphql_url, | ||
| json={"query": graphql_mutation, "variables": graphql_variables}, | ||
| headers=graphql_headers, | ||
| timeout=30 | ||
| ) | ||
|
|
||
| if graphql_response.status_code != 200: | ||
| logger.error(f"GitHub Sponsors API returned status {graphql_response.status_code}: {graphql_response.text}") | ||
| return JsonResponse({ | ||
| "success": False, | ||
| "error": f"Failed to process GitHub Sponsors payment (HTTP {graphql_response.status_code}). The user may not have GitHub Sponsors enabled." | ||
| }, status=400) | ||
|
|
||
| graphql_data = graphql_response.json() | ||
|
|
||
| # Check for GraphQL errors | ||
| if "errors" in graphql_data: | ||
| error_messages = [err.get("message", "Unknown error") for err in graphql_data["errors"]] | ||
| logger.error(f"GitHub Sponsors GraphQL errors: {error_messages}") | ||
| return JsonResponse({ | ||
| "success": False, | ||
| "error": f"GitHub Sponsors API error: {'; '.join(error_messages)}" | ||
| }, status=400) | ||
|
|
||
| # Extract sponsorship ID from response | ||
| sponsorship_data = graphql_data.get("data", {}).get("createSponsorship", {}).get("sponsorship", {}) | ||
| sponsorship_id = sponsorship_data.get("id") | ||
|
|
||
| if not sponsorship_id: | ||
| logger.error(f"No sponsorship ID in response: {graphql_data}") | ||
| return JsonResponse({ | ||
| "success": False, | ||
| "error": "Failed to create sponsorship. Please ensure the recipient has GitHub Sponsors enabled." | ||
| }, status=400) | ||
|
|
||
| logger.info(f"Successfully created GitHub Sponsors payment: {sponsorship_id}") | ||
|
|
||
| # IMMEDIATELY cancel the sponsorship to prevent recurring charges | ||
| # This creates a one-time payment effect | ||
| cancel_mutation = """ | ||
| mutation($sponsorshipId: ID!) { | ||
| cancelSponsorship(input: { | ||
| sponsorshipId: $sponsorshipId | ||
| }) { | ||
| sponsorsTier { | ||
| monthlyPriceInCents | ||
| name | ||
| } | ||
| } | ||
| } | ||
| """ | ||
|
|
||
| cancel_variables = { | ||
| "sponsorshipId": sponsorship_id, | ||
| } | ||
|
|
||
| # Attempt to cancel with retries | ||
| cancel_success = False | ||
| max_retries = 3 | ||
|
|
||
| for attempt in range(max_retries): | ||
| try: | ||
| logger.info(f"Attempting to cancel sponsorship {sponsorship_id} (attempt {attempt + 1}/{max_retries})") | ||
|
|
||
| cancel_response = requests.post( | ||
| graphql_url, | ||
| json={"query": cancel_mutation, "variables": cancel_variables}, | ||
| headers=graphql_headers, | ||
| timeout=30 | ||
| ) | ||
|
|
||
| if cancel_response.status_code == 200: | ||
| cancel_data = cancel_response.json() | ||
|
|
||
| if "errors" not in cancel_data: | ||
| logger.info(f"Successfully cancelled sponsorship {sponsorship_id}") | ||
| cancel_success = True | ||
| break | ||
| else: | ||
| logger.error(f"GraphQL errors cancelling sponsorship: {cancel_data['errors']}") | ||
| else: | ||
| logger.error(f"Cancel API returned status {cancel_response.status_code}: {cancel_response.text}") | ||
|
|
||
| # Wait before retry (exponential backoff) | ||
| if attempt < max_retries - 1: | ||
| time.sleep(2 ** attempt) | ||
|
|
||
| except requests.exceptions.RequestException as cancel_error: | ||
| logger.error(f"Network error cancelling sponsorship (attempt {attempt + 1}): {cancel_error}") | ||
| if attempt < max_retries - 1: | ||
| time.sleep(2 ** attempt) | ||
|
|
||
| # CRITICAL: If cancellation failed, log and alert | ||
| if not cancel_success: | ||
| logger.critical( | ||
| f"CRITICAL: Failed to cancel sponsorship {sponsorship_id} after {max_retries} attempts. " | ||
| f"Recurring charges will continue! Issue: #{issue_number}, User: {assignee_username}, Amount: ${bounty_amount}" | ||
| ) | ||
| # Return error to prevent marking as paid | ||
| return JsonResponse({ | ||
| "success": False, | ||
| "error": "Payment was created but cancellation failed. Please manually cancel the sponsorship to prevent recurring charges.", | ||
| "sponsorship_id": sponsorship_id, | ||
| "action_required": "Manual cancellation needed" | ||
| }, status=500) |
There was a problem hiding this comment.
CRITICAL: Recurring charge risk - recommend reverting to safe placeholder flow.
Creating a GitHub Sponsors subscription (line 4024) initiates recurring monthly charges. Even with the cancellation logic (lines 4085-4152) and retries, there are multiple failure scenarios that leave recurring charges active:
- Network timeouts during cancellation requests
- GitHub API rate limiting or errors
- Temporary API outages
- Database transaction failures after cancellation succeeds
When cancellation fails after all retries, the code returns an error (lines 4147-4152), but the sponsorship is already created and will continue charging indefinitely until manually cancelled.
Risk Assessment:
- Financial Impact: Recurring charges for every bounty could accumulate rapidly
- Manual Intervention Required: No automated rollback mechanism
- Testing Difficulty: Hard to safely test with real money in production
Recommended Solution:
As noted in the PR objectives, implement Option 1 (safe placeholder flow) instead:
- Validate issue state, assignee, and user profile mapping
- Record the intended payout in the database with a "pending" status
- Return success to the workflow
- Require manual approval/processing by admins or implement a safer deferred payment system
This approach retains automation benefits while eliminating financial risk.
Would you like me to generate a diff implementing the safer placeholder approach?
Based on past review comments.
🧰 Tools
🪛 Ruff (0.14.0)
4136-4136: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
|
hey @DonnieBLT hope u are fine! |
|
Let's try immediate cancellation and if we find it's not reliable, we can switch to manual. Thank you! |
- Verify PR merge status before processing bounty payouts - Improve error logging with logger.exception() in exception handlers - Fix EN DASH character in bounty amount comment Addresses security issues identified in CodeRabbit review: - Prevents CSRF failures for automated GitHub Actions calls - Ensures only merged PRs trigger bounty payments - Provides better error context with stack traces
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
website/views/organization.py (1)
3821-3825: Consider more specific error messages for GitHub API failures.The current error message is generic and doesn't differentiate between failure types. More specific messages would improve troubleshooting:
if response.status_code != 200: + if response.status_code == 404: + error_msg = "GitHub issue not found. Please verify the issue URL." + elif response.status_code in (401, 403): + error_msg = "GitHub API authentication failed. Please contact support." + elif response.status_code == 429: + error_msg = "GitHub API rate limit exceeded. Please try again later." + else: + error_msg = f"Failed to fetch issue from GitHub API (HTTP {response.status_code}). Please verify the issue URL and try again." + return JsonResponse( - {"success": False, "error": f"Failed to fetch issue from GitHub API (HTTP {response.status_code}). Please verify the issue URL and try again."}, + {"success": False, "error": error_msg}, status=400 )This provides clearer guidance to users when issues occur.
Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
website/views/organization.py(19 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
website/views/organization.py
2855-2855: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
3482-3482: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
3489-3489: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
4046-4046: Redundant exception object included in logging.exception call
(TRY401)
4167-4167: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
4186-4186: Redundant exception object included in logging.exception call
(TRY401)
⏰ 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). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
| @require_POST | ||
| @csrf_exempt | ||
| def process_bounty_payout(request): | ||
| """ | ||
| API endpoint to process bounty payouts for merged PRs with attached issues. | ||
| Called by GitHub Action when a PR is merged. | ||
| Expected POST data: | ||
| - issue_url: URL of the GitHub issue with bounty | ||
| - pr_url: URL of the merged PR (optional, for logging) | ||
| Expected Headers: | ||
| - X-BLT-API-TOKEN: API token for authentication (optional but recommended) | ||
| """ | ||
| try: | ||
| # Optional: Validate API token if provided | ||
| api_token = request.headers.get("X-BLT-API-TOKEN") | ||
| expected_token = getattr(settings, "BLT_API_TOKEN", None) | ||
|
|
||
| if expected_token and api_token != expected_token: | ||
| logger.warning("Invalid API token provided for bounty payout") | ||
| return JsonResponse( | ||
| {"success": False, "error": "Invalid API token"}, | ||
| status=401 | ||
| ) | ||
|
|
||
| # Parse request data | ||
| data = json.loads(request.body) if request.body else {} | ||
| issue_url = data.get("issue_url") | ||
| pr_url = data.get("pr_url", "") | ||
|
|
||
| if not issue_url: | ||
| return JsonResponse( | ||
| {"success": False, "error": "issue_url is required"}, | ||
| status=400 | ||
| ) | ||
|
|
||
| logger.info(f"Processing bounty payout for issue: {issue_url}, PR: {pr_url}") | ||
|
|
||
| # Extract issue details from URL | ||
| # URL format: https://github.com/owner/repo/issues/number | ||
| parts = issue_url.rstrip("/").split("/") | ||
| if len(parts) < 7 or parts[5] != "issues": | ||
| return JsonResponse( | ||
| {"success": False, "error": "Invalid issue URL format"}, | ||
| status=400 | ||
| ) | ||
|
|
||
| owner = parts[3] | ||
| repo_name = parts[4] | ||
| issue_number = parts[6] | ||
|
|
||
| # Validate URL components | ||
| if not re.match(r'^[\w-]+$', owner) or not re.match(r'^[\w-]+$', repo_name): | ||
| return JsonResponse( | ||
| {"success": False, "error": "Invalid repository owner or name"}, | ||
| status=400 | ||
| ) | ||
| if not issue_number.isdigit(): | ||
| return JsonResponse( | ||
| {"success": False, "error": "Invalid issue number"}, | ||
| status=400 | ||
| ) | ||
|
|
||
| # Fetch issue from GitHub API | ||
| headers = {} | ||
| if settings.GITHUB_TOKEN: | ||
| headers["Authorization"] = f"token {settings.GITHUB_TOKEN}" | ||
|
|
||
| api_url = f"https://api.github.com/repos/{owner}/{repo_name}/issues/{issue_number}" | ||
| response = requests.get(api_url, headers=headers, timeout=10) | ||
|
|
||
| if response.status_code != 200: | ||
| return JsonResponse( | ||
| {"success": False, "error": f"Failed to fetch issue from GitHub API (HTTP {response.status_code}). Please verify the issue URL and try again."}, | ||
| status=400 | ||
| ) | ||
|
|
||
| issue_data = response.json() | ||
|
|
||
| # Check if issue is closed (don't pay for open issues) | ||
| if issue_data.get("state") != "closed": | ||
| return JsonResponse( | ||
| {"success": False, "error": "Issue must be closed before bounty payout"}, | ||
| status=400 | ||
| ) | ||
|
|
||
| # Verify PR is merged if pr_url is provided | ||
| if pr_url: | ||
| # Extract PR details from URL | ||
| # URL format: https://github.com/owner/repo/pull/number | ||
| pr_parts = pr_url.rstrip("/").split("/") | ||
| if len(pr_parts) >= 7 and pr_parts[5] == "pull": | ||
| pr_owner = pr_parts[3] | ||
| pr_repo_name = pr_parts[4] | ||
| pr_number = pr_parts[6] | ||
|
|
||
| if pr_number.isdigit(): | ||
| # Fetch PR from GitHub API | ||
| pr_api_url = f"https://api.github.com/repos/{pr_owner}/{pr_repo_name}/pulls/{pr_number}" | ||
| pr_response = requests.get(pr_api_url, headers=headers, timeout=10) | ||
|
|
||
| if pr_response.status_code == 200: | ||
| pr_data = pr_response.json() | ||
| if not pr_data.get("merged"): | ||
| return JsonResponse( | ||
| {"success": False, "error": "PR must be merged before bounty payout"}, | ||
| status=400 | ||
| ) | ||
| else: | ||
| logger.warning(f"Failed to fetch PR data: {pr_response.status_code}") | ||
| else: | ||
| logger.warning("Invalid PR number format") | ||
| else: | ||
| logger.warning("Invalid PR URL format") | ||
|
|
||
| # Check if issue has dollar tag (bounty) | ||
| has_bounty = False | ||
| bounty_amount = 0 | ||
| # Use regex to extract bounty amount from labels like "$50 - medium" or "$1,000 reward" | ||
| amount_pattern = re.compile(r"^\$?\s*(\d+(?:,\d{3})*)") | ||
| for label in issue_data.get("labels", []): | ||
| label_name = label.get("name", "") | ||
| match = amount_pattern.match(label_name) | ||
| if match: | ||
| has_bounty = True | ||
| # Extract amount and remove commas (e.g., "1,000" -> 1000) | ||
| amount_str = match.group(1).replace(",", "") | ||
| try: | ||
| bounty_amount = int(amount_str) | ||
| except ValueError: | ||
| bounty_amount = 5 # Fallback to $5 | ||
| break | ||
|
|
||
| if not has_bounty: | ||
| return JsonResponse( | ||
| {"success": False, "error": "Issue does not have a bounty label"}, | ||
| status=400 | ||
| ) | ||
|
|
||
| # Get assignee username (the person who will receive payment) | ||
| assignee_username = None | ||
| if issue_data.get("assignee"): | ||
| assignee_username = issue_data["assignee"]["login"] | ||
| elif issue_data.get("assignees") and len(issue_data["assignees"]) > 0: | ||
| assignee_username = issue_data["assignees"][0]["login"] | ||
|
|
||
| if not assignee_username: | ||
| return JsonResponse( | ||
| {"success": False, "error": "Issue has no assignee to pay"}, | ||
| status=400 | ||
| ) | ||
|
|
||
| logger.info(f"Processing payment for assignee: {assignee_username}") | ||
|
|
||
| # Find user profile by GitHub username | ||
| try: | ||
| social_account = SocialAccount.objects.get( | ||
| provider="github", | ||
| extra_data__login=assignee_username | ||
| ) | ||
| user_profile = social_account.user.userprofile | ||
| except SocialAccount.DoesNotExist: | ||
| return JsonResponse( | ||
| {"success": False, "error": f"User with GitHub username {assignee_username} not found in BLT"}, | ||
| status=404 | ||
| ) | ||
|
|
||
| # Check preferred payment method | ||
| payment_method = user_profile.preferred_payment_method or "sponsors" | ||
|
|
||
| # For this task, only handle GitHub Sponsors | ||
| if payment_method != "sponsors": | ||
| return JsonResponse( | ||
| {"success": False, "error": f"User prefers {payment_method} payment method, but only sponsors is supported in this implementation"}, | ||
| status=400 | ||
| ) | ||
|
|
||
| # Check for duplicate payment BEFORE processing to prevent double charges | ||
| repo_url = f"https://github.com/{owner}/{repo_name}" | ||
| repo, _ = Repo.objects.get_or_create( | ||
| repo_url=repo_url, | ||
| defaults={ | ||
| "name": repo_name, | ||
| "slug": f"{owner}-{repo_name}", | ||
| }, | ||
| ) | ||
|
|
||
| try: | ||
| existing_issue = GitHubIssue.objects.get(issue_id=issue_data["id"], repo=repo) | ||
| if existing_issue.sponsors_tx_id or existing_issue.bch_tx_id: | ||
| logger.warning(f"Bounty already paid for issue #{issue_number}") | ||
| return JsonResponse({ | ||
| "success": False, | ||
| "error": "Bounty already paid for this issue", | ||
| "paid_at": existing_issue.p2p_payment_created_at.isoformat() if existing_issue.p2p_payment_created_at else None | ||
| }, status=400) | ||
| except GitHubIssue.DoesNotExist: | ||
| pass # Issue doesn't exist yet, proceed with payment | ||
|
|
||
| # Process GitHub Sponsors payment | ||
| # Get sponsor username from settings (defaults to DonnieBLT as per issue requirements) | ||
| sponsor_username = getattr(settings, "GITHUB_SPONSOR_USERNAME", "DonnieBLT") | ||
|
|
||
| logger.info(f"Creating GitHub Sponsors payment: {sponsor_username} -> {assignee_username}, Amount: ${bounty_amount}") | ||
|
|
||
| # Call GitHub Sponsors GraphQL API | ||
| graphql_url = "https://api.github.com/graphql" | ||
| graphql_headers = { | ||
| "Authorization": f"Bearer {settings.GITHUB_TOKEN}", | ||
| "Content-Type": "application/json", | ||
| } | ||
|
|
||
| # Step 1: Get user's sponsorable ID and available tiers | ||
| query_user = """ | ||
| query($login: String!) { | ||
| user(login: $login) { | ||
| id | ||
| sponsorsListing { | ||
| id | ||
| tiers(first: 20) { | ||
| nodes { | ||
| id | ||
| monthlyPriceInCents | ||
| name | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| """ | ||
|
|
||
| try: | ||
| user_response = requests.post( | ||
| graphql_url, | ||
| json={"query": query_user, "variables": {"login": assignee_username}}, | ||
| headers=graphql_headers, | ||
| timeout=10 | ||
| ) | ||
|
|
||
| if user_response.status_code != 200: | ||
| logger.error(f"Failed to fetch user info: {user_response.status_code}") | ||
| return JsonResponse({ | ||
| "success": False, | ||
| "error": "Failed to fetch recipient information from GitHub" | ||
| }, status=400) | ||
|
|
||
| user_data = user_response.json() | ||
|
|
||
| if "errors" in user_data: | ||
| logger.error(f"GraphQL errors: {user_data['errors']}") | ||
| return JsonResponse({ | ||
| "success": False, | ||
| "error": "User not found or does not have GitHub Sponsors enabled" | ||
| }, status=400) | ||
|
|
||
| user_info = user_data.get("data", {}).get("user", {}) | ||
| sponsorable_id = user_info.get("id") | ||
| sponsors_listing = user_info.get("sponsorsListing") | ||
|
|
||
| if not sponsorable_id or not sponsors_listing: | ||
| return JsonResponse({ | ||
| "success": False, | ||
| "error": f"User {assignee_username} does not have GitHub Sponsors enabled" | ||
| }, status=400) | ||
|
|
||
| # Step 2: Find matching tier or use closest one | ||
| tiers = sponsors_listing.get("tiers", {}).get("nodes", []) | ||
| target_amount_cents = bounty_amount * 100 | ||
|
|
||
| # Find exact match or next tier up (never downgrade) | ||
| matching_tier = None | ||
| for tier in tiers: | ||
| if tier.get("monthlyPriceInCents") == target_amount_cents: | ||
| matching_tier = tier | ||
| break | ||
|
|
||
| # If no exact match, find next tier up (never pay less than bounty) | ||
| if not matching_tier and tiers: | ||
| # Sort tiers by price ascending | ||
| sorted_tiers = sorted(tiers, key=lambda t: t.get("monthlyPriceInCents", 0)) | ||
| # Find first tier >= target amount | ||
| for tier in sorted_tiers: | ||
| if tier.get("monthlyPriceInCents", 0) >= target_amount_cents: | ||
| matching_tier = tier | ||
| logger.warning(f"No exact tier match for ${bounty_amount}, using next tier up: ${tier.get('monthlyPriceInCents', 0) / 100}") | ||
| break | ||
|
|
||
| if not matching_tier: | ||
| return JsonResponse({ | ||
| "success": False, | ||
| "error": f"No sponsorship tiers available for {assignee_username}. They need to set up sponsor tiers first." | ||
| }, status=400) | ||
|
|
||
| tier_id = matching_tier.get("id") | ||
|
|
||
| except requests.exceptions.RequestException as e: | ||
| logger.exception(f"Network error fetching user info: {e}") | ||
| return JsonResponse({ | ||
| "success": False, | ||
| "error": "Network error while fetching recipient information" | ||
| }, status=500) | ||
|
|
||
| # Step 3: Create sponsorship | ||
| graphql_mutation = """ | ||
| mutation($sponsorableId: ID!, $tierId: ID!) { | ||
| createSponsorship(input: { | ||
| sponsorableId: $sponsorableId | ||
| tierId: $tierId | ||
| privacyLevel: PUBLIC | ||
| }) { | ||
| sponsorship { | ||
| id | ||
| tier { | ||
| monthlyPriceInCents | ||
| name | ||
| } | ||
| createdAt | ||
| } | ||
| } | ||
| } | ||
| """ | ||
|
|
||
| graphql_variables = { | ||
| "sponsorableId": sponsorable_id, | ||
| "tierId": tier_id, | ||
| } | ||
|
|
||
| try: | ||
| graphql_response = requests.post( | ||
| graphql_url, | ||
| json={"query": graphql_mutation, "variables": graphql_variables}, | ||
| headers=graphql_headers, | ||
| timeout=30 | ||
| ) | ||
|
|
||
| if graphql_response.status_code != 200: | ||
| logger.error(f"GitHub Sponsors API returned status {graphql_response.status_code}: {graphql_response.text}") | ||
| return JsonResponse({ | ||
| "success": False, | ||
| "error": f"Failed to process GitHub Sponsors payment (HTTP {graphql_response.status_code}). The user may not have GitHub Sponsors enabled." | ||
| }, status=400) | ||
|
|
||
| graphql_data = graphql_response.json() | ||
|
|
||
| # Check for GraphQL errors | ||
| if "errors" in graphql_data: | ||
| error_messages = [err.get("message", "Unknown error") for err in graphql_data["errors"]] | ||
| logger.error(f"GitHub Sponsors GraphQL errors: {error_messages}") | ||
| return JsonResponse({ | ||
| "success": False, | ||
| "error": f"GitHub Sponsors API error: {'; '.join(error_messages)}" | ||
| }, status=400) | ||
|
|
||
| # Extract sponsorship ID from response | ||
| sponsorship_data = graphql_data.get("data", {}).get("createSponsorship", {}).get("sponsorship", {}) | ||
| sponsorship_id = sponsorship_data.get("id") | ||
|
|
||
| if not sponsorship_id: | ||
| logger.error(f"No sponsorship ID in response: {graphql_data}") | ||
| return JsonResponse({ | ||
| "success": False, | ||
| "error": "Failed to create sponsorship. Please ensure the recipient has GitHub Sponsors enabled." | ||
| }, status=400) | ||
|
|
||
| logger.info(f"Successfully created GitHub Sponsors payment: {sponsorship_id}") | ||
|
|
||
| # IMMEDIATELY cancel the sponsorship to prevent recurring charges | ||
| # This creates a one-time payment effect | ||
| cancel_mutation = """ | ||
| mutation($sponsorshipId: ID!) { | ||
| cancelSponsorship(input: { | ||
| sponsorshipId: $sponsorshipId | ||
| }) { | ||
| sponsorsTier { | ||
| monthlyPriceInCents | ||
| name | ||
| } | ||
| } | ||
| } | ||
| """ | ||
|
|
||
| cancel_variables = { | ||
| "sponsorshipId": sponsorship_id, | ||
| } | ||
|
|
||
| # Attempt to cancel with retries | ||
| cancel_success = False | ||
| max_retries = 3 | ||
|
|
||
| for attempt in range(max_retries): | ||
| try: | ||
| logger.info(f"Attempting to cancel sponsorship {sponsorship_id} (attempt {attempt + 1}/{max_retries})") | ||
|
|
||
| cancel_response = requests.post( | ||
| graphql_url, | ||
| json={"query": cancel_mutation, "variables": cancel_variables}, | ||
| headers=graphql_headers, | ||
| timeout=30 | ||
| ) | ||
|
|
||
| if cancel_response.status_code == 200: | ||
| cancel_data = cancel_response.json() | ||
|
|
||
| if "errors" not in cancel_data: | ||
| logger.info(f"Successfully cancelled sponsorship {sponsorship_id}") | ||
| cancel_success = True | ||
| break | ||
| else: | ||
| logger.error(f"GraphQL errors cancelling sponsorship: {cancel_data['errors']}") | ||
| else: | ||
| logger.error(f"Cancel API returned status {cancel_response.status_code}: {cancel_response.text}") | ||
|
|
||
| # Wait before retry (exponential backoff) | ||
| if attempt < max_retries - 1: | ||
| time.sleep(2 ** attempt) | ||
|
|
||
| except requests.exceptions.RequestException as cancel_error: | ||
| logger.error(f"Network error cancelling sponsorship (attempt {attempt + 1}): {cancel_error}") | ||
| if attempt < max_retries - 1: | ||
| time.sleep(2 ** attempt) | ||
|
|
||
| # CRITICAL: If cancellation failed, log and alert | ||
| if not cancel_success: | ||
| logger.critical( | ||
| f"CRITICAL: Failed to cancel sponsorship {sponsorship_id} after {max_retries} attempts. " | ||
| f"Recurring charges will continue! Issue: #{issue_number}, User: {assignee_username}, Amount: ${bounty_amount}" | ||
| ) | ||
| # Return error to prevent marking as paid | ||
| return JsonResponse({ | ||
| "success": False, | ||
| "error": "Payment was created but cancellation failed. Please manually cancel the sponsorship to prevent recurring charges.", | ||
| "sponsorship_id": sponsorship_id, | ||
| "action_required": "Manual cancellation needed" | ||
| }, status=500) | ||
|
|
||
| except requests.exceptions.RequestException as e: | ||
| logger.exception(f"Network error calling GitHub Sponsors API: {e}") | ||
| return JsonResponse({ | ||
| "success": False, | ||
| "error": "Network error while processing payment. Please try again later." | ||
| }, status=500) | ||
|
|
||
| # Create/update the GitHubIssue record with actual transaction ID | ||
| # Note: repo was already created earlier during duplicate check | ||
| github_issue, _created = GitHubIssue.objects.update_or_create( | ||
| issue_id=issue_data["id"], | ||
| repo=repo, | ||
| defaults={ | ||
| "title": issue_data["title"], | ||
| "body": issue_data.get("body", ""), | ||
| "state": issue_data["state"], | ||
| "url": issue_url, | ||
| "has_dollar_tag": True, | ||
| "p2p_amount_usd": bounty_amount, | ||
| "created_at": parse_datetime(issue_data["created_at"]), | ||
| "updated_at": parse_datetime(issue_data["updated_at"]), | ||
| "closed_at": parse_datetime(issue_data["closed_at"]) if issue_data.get("closed_at") else None, | ||
| "user_profile": user_profile, | ||
| } | ||
| ) | ||
|
|
||
| # Mark as paid with actual GitHub Sponsors transaction ID | ||
| github_issue.sponsors_tx_id = sponsorship_id | ||
| github_issue.p2p_payment_created_at = timezone.now() | ||
| github_issue.save() | ||
|
|
||
| logger.info(f"Successfully processed bounty payout for issue #{issue_number} with sponsorship ID: {sponsorship_id}") | ||
|
|
||
| return JsonResponse({ | ||
| "success": True, | ||
| "message": f"Bounty payout completed for {assignee_username}", | ||
| "amount": bounty_amount, | ||
| "payment_method": "sponsors", | ||
| "transaction_id": sponsorship_id, | ||
| "issue_number": issue_number, | ||
| }) | ||
|
|
||
| except Exception: | ||
| logger.exception("Error processing bounty payout") | ||
| return JsonResponse( | ||
| {"success": False, "error": "Failed to process bounty payout. Please contact support if the issue persists."}, | ||
| status=500 | ||
| ) |
There was a problem hiding this comment.
Critical: Recurring charge risk remains despite error handling.
The implementation creates a GitHub Sponsors subscription (line 4078) and then attempts to cancel it (lines 4143-4148) to simulate a one-time payment. While the code includes good safeguards (duplicate checks, retry logic, critical logging, error returns when cancellation fails), a fundamental risk remains: if cancellation fails, the sponsorship exists in GitHub's system and will charge monthly until manually cancelled.
Current mitigations in place:
- ✅ Duplicate check prevents multiple sponsorship attempts (lines 3937-3947)
- ✅ Exponential backoff retry logic with 3 attempts (lines 4139-4169)
- ✅ Returns error and prevents database record if cancellation fails (lines 4172-4183)
- ✅ Critical logging for failed cancellations (lines 4173-4176)
Missing critical safeguards:
-
No automated alerting - When cancellation fails, only a log entry is created. Past reviewer suggested email alerts to admins (previous comment on lines 4171-4183). Consider adding:
from django.core.mail import mail_admins mail_admins( subject=f"URGENT: Failed to cancel GitHub Sponsorship {sponsorship_id}", message=( f"Sponsorship cancellation failed after {max_retries} attempts.\n" f"IMMEDIATE ACTION REQUIRED to prevent recurring charges.\n\n" f"Details:\n" f"- Sponsorship ID: {sponsorship_id}\n" f"- Issue: #{issue_number} ({issue_url})\n" f"- Recipient: {assignee_username}\n" f"- Amount: ${bounty_amount}/month\n" ), fail_silently=True )
-
No failure tracking system - Consider creating a dedicated model or adding a status field to track failed cancellations that require manual intervention.
-
No monitoring dashboard - Admins have no way to view pending manual actions.
Note: Per PR objectives, the maintainer approved proceeding with this approach (Option 2) with a fallback to manual processing if it proves unreliable. Given the "Chill" review mode, I'm flagging this as a critical issue that should be monitored closely in production, with strong recommendations for the additional safeguards above.
Would you like me to generate code for automated alerts, a failure tracking model, or a monitoring dashboard?
Based on past review comments.
🧰 Tools
🪛 Ruff (0.14.0)
4046-4046: Redundant exception object included in logging.exception call
(TRY401)
4167-4167: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
4186-4186: Redundant exception object included in logging.exception call
(TRY401)
…01/403, 429) to provide clearer troubleshooting guidance for users - Replace generic error message with actionable feedback for common failure scenarios - Improves user experience when GitHub API calls fail during bounty payout processing
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
website/views/organization.py (2)
4116-4214: Cancellation logic implements strong safeguards per maintainer approval.The immediate cancellation approach (Option 2) includes:
- ✅ Retry logic with exponential backoff (3 attempts)
- ✅ Critical error logging when all retries fail
- ✅ Automated admin email alerts with actionable instructions
- ✅ Returns error and prevents marking as paid if cancellation fails
Remaining considerations for production monitoring:
While the maintainer approved this approach with fallback to manual processing if unreliable, consider adding:
- Database tracking for failed cancellations - Create a model or add status field to GitHubIssue to track failures requiring manual intervention
- Monitoring dashboard - Provide admins visibility into pending manual actions
- Feature flag - Add a setting to disable automatic payment processing if the approach proves unreliable in production
Minor issue: Line 4170 captures
cancel_errorbut never uses it. Consider removing the variable assignment:- except requests.exceptions.RequestException as cancel_error: + except requests.exceptions.RequestException: logger.exception(f"Network error cancelling sponsorship (attempt {attempt + 1})")Based on past review comments and PR objectives acknowledging maintainer approval of Option 2.
4040-4040: Fix minor logging best practices issues.Address the following static analysis findings:
- Line 4040: Remove redundant exception parameter from
logger.exception():- except requests.exceptions.RequestException as e: - logger.exception(f"Network error fetching user info: {e}") + except requests.exceptions.RequestException: + logger.exception("Network error fetching user info")
- Line 4216: Remove unused exception variable:
- except requests.exceptions.RequestException as e: + except requests.exceptions.RequestException: logger.exception("Network error calling GitHub Sponsors API")Note:
logger.exception()automatically captures the full traceback, so explicitly passing the exception object is redundant.Also applies to: 4216-4216, 4262-4270
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
website/migrations/0247_userprofile_preferred_payment_method.py(1 hunks)website/views/organization.py(19 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
website/migrations/0247_userprofile_preferred_payment_method.py
7-9: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
11-24: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
website/views/organization.py
2855-2855: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
3482-3482: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
3489-3489: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
4040-4040: Redundant exception object included in logging.exception call
(TRY401)
4170-4170: Local variable cancel_error is assigned to but never used
Remove assignment to unused variable cancel_error
(F841)
4216-4216: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
⏰ 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). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (11)
website/migrations/0247_userprofile_preferred_payment_method.py (1)
1-24: LGTM! Migration structure is correct.The migration properly adds the
preferred_payment_methodfield to UserProfile with appropriate choices, default value, and nullability for backfilling existing records.Note: The static analysis warnings (RUF012) suggesting
ClassVarannotations are false positives—Django migrations follow this standard structure and don't require those annotations.website/views/organization.py (10)
12-12: LGTM! New imports are appropriate.The added imports support the bounty payout functionality:
SocialAccountmaps GitHub usernames to BLT user profilesmail_adminsenables critical failure alertscsrf_exemptallows GitHub Actions to call the webhook endpointAlso applies to: 20-20, 38-38
505-505: Good refactoring of logging calls.The updated
logger.exception()calls now rely on automatic traceback capture rather than explicitly interpolating the exception object, which is the recommended pattern and improves log consistency.Also applies to: 567-567, 580-580, 775-775, 902-902, 992-992, 1447-1447, 1543-1543, 1581-1581, 2712-2712
3749-3763: LGTM! Function structure and decorators are correct.The
@csrf_exemptdecorator properly allows GitHub Actions to call this endpoint without CSRF tokens, and the docstring clearly documents the API contract.
3764-3796: Strong input validation and security.The function properly:
- Validates optional API tokens to prevent unauthorized access
- Sanitizes URL components with regex patterns before use
- Provides clear, specific error messages for invalid inputs
3798-3874: Excellent validation flow.The implementation properly:
- Provides specific, actionable error messages for different GitHub API failures
- Verifies the issue is closed before processing payment
- Optionally confirms the linked PR is merged
- Robustly extracts bounty amounts from various label formats (e.g., "$50 - medium", "$1,000 reward")
This addresses previous reviewer concerns about ensuring issues are closed and PRs are merged before payout.
3876-3909: Solid user mapping and validation.The assignee resolution correctly:
- Maps GitHub usernames to BLT accounts via django-allauth's
SocialAccount- Enforces the user has a BLT profile before proceeding
- Validates the preferred payment method matches the implementation
- Provides clear error messages when requirements aren't met
3911-3936: Critical safeguard correctly implemented.The duplicate payment check is positioned before any GitHub Sponsors API calls, which is essential to prevent creating multiple subscriptions for the same issue. This prevents costly financial errors.
3938-4043: Proper tier selection—never downgrades bounty amount.The implementation correctly:
- Fetches available sponsor tiers via GraphQL
- Selects exact tier match or next tier up
- Never selects a lower-priced tier (addresses previous critical review comment)
- Returns clear errors if user doesn't have Sponsors enabled or suitable tiers
4045-4114: Sponsorship creation is correct but enters critical section.The GraphQL mutation properly creates a GitHub Sponsors subscription. However, this initiates a recurring monthly charge that will continue until successfully cancelled in the next section. The comprehensive error handling here is good.
Note: Monitor closely in production—if the process crashes or is interrupted between this point and successful cancellation (lines 4116-4214), the subscription remains active and will charge monthly.
4223-4260: Payment recording is correct and complete.The implementation properly:
- Only persists payment data after successful cancellation
- Records the actual GitHub Sponsors transaction ID (sponsorship_id)
- Timestamps the payment for audit trails
- Returns comprehensive success response including transaction details
- Add GitHubIssue fields for sponsors cancellation observability - Expose failure tracking in admin for quick triage - Clean up error messages to avoid exposing exception details - Set/clear cancellation flags in process_bounty_payout
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
website/admin.py(2 hunks)website/migrations/0248_githubissue_sponsors_cancel_flags.py(1 hunks)website/models.py(2 hunks)website/views/organization.py(23 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- website/models.py
🧰 Additional context used
🪛 Ruff (0.14.0)
website/migrations/0248_githubissue_sponsors_cancel_flags.py
5-7: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
9-25: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
website/views/organization.py
3481-3481: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
3488-3488: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
4036-4036: Redundant exception object included in logging.exception call
(TRY401)
4166-4166: Local variable cancel_error is assigned to but never used
Remove assignment to unused variable cancel_error
(F841)
4242-4242: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
⏰ 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). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
- Remove tier rounding up logic - Only proceed with payment if exact tier exists - Return clear error if no matching tier found
CRITICAL: Prevent unauthorized repos from draining sponsors budget - Add BLT_ALLOWED_BOUNTY_REPOS setting (default: OWASP-BLT/BLT) - Reject payout requests for repos not in allowlist (403) - Log unauthorized attempts for security monitoring - Add env-based configuration for multiple repos Without this, attackers could create fake issues in their own repos with high dollar labels and drain the entire sponsors budget.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
website/views/organization.py (2)
3481-3483: Remove unused exception variables and redundant logger parameters.Static analysis correctly identifies several issues with exception handling:
- Lines 3481-3483, 3488-3490: Exception variables assigned but never used
- Line 4034: Redundant exception parameter in
logger.exception()- Line 4164: Unused
cancel_errorvariable- Line 4240: Unused
evariable
logger.exception()automatically captures the traceback, so you don't need to pass the exception object.Apply these fixes:
- except requests.exceptions.RequestException as e: - logger.exception(f"Error fetching PR details for {pr_url}") + except requests.exceptions.RequestException: + logger.exception(f"Error fetching PR details for {pr_url}")- except requests.exceptions.RequestException as e: - logger.exception(f"Error fetching timeline for issue #{issue_number}") + except requests.exceptions.RequestException: + logger.exception(f"Error fetching timeline for issue #{issue_number}")- except requests.exceptions.RequestException as e: - logger.exception(f"Network error fetching user info: {e}") + except requests.exceptions.RequestException: + logger.exception("Network error fetching user info")- except requests.exceptions.RequestException as cancel_error: - logger.exception("Network error cancelling sponsorship") + except requests.exceptions.RequestException: + logger.exception("Network error cancelling sponsorship")- except requests.exceptions.RequestException as e: - logger.exception("Network error calling GitHub Sponsors API") + except requests.exceptions.RequestException: + logger.exception("Network error calling GitHub Sponsors API")Also applies to: 3488-3490, 4034-4037, 4164-4168, 4240-4245
4110-4238: Monitor closely in production - Recurring charge risk with good safeguards.This implementation follows "Option 2" from the PR objectives: create a GitHub Sponsors subscription, then immediately cancel it to simulate a one-time payment. The maintainer explicitly approved this approach despite recurring charge risks.
Current safeguards (good):
- ✅ Retry logic: 3 attempts with exponential backoff (lines 4129-4168)
- ✅ Critical logging when cancellation fails (line 4175)
- ✅ Automated admin email alerts (lines 4178-4197)
- ✅ Database tracking:
sponsors_cancellation_failed,sponsors_cancellation_attempts,sponsors_cancellation_last_attemptfields (lines 4222-4224)- ✅ Returns error and prevents marking as paid if cancellation fails (lines 4229-4238)
Remaining risk:
If cancellation fails after all retries due to network issues, API outages, or rate limiting, the sponsorship remains active and charges monthly until manually cancelled. Admins receive email alerts, but manual intervention is required.Recommendations for production:
- Set up monitoring dashboard for failed cancellations:
# Add to admin.py to surface issues requiring manual action list_filter = [..., 'sponsors_cancellation_failed']
- Create a management command to retry failed cancellations:
# python manage.py retry_failed_cancellations
Consider adding Slack/Discord webhook notifications in addition to email for faster response.
Monitor closely for first few weeks and be ready to revert to "Option 1" (placeholder/manual flow) if cancellation failures occur frequently.
Based on past review comments and PR objectives.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
blt/settings.py(1 hunks)website/views/organization.py(23 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
website/views/organization.py
3481-3481: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
3488-3488: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
4034-4034: Redundant exception object included in logging.exception call
(TRY401)
4164-4164: Local variable cancel_error is assigned to but never used
Remove assignment to unused variable cancel_error
(F841)
4240-4240: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🔇 Additional comments (8)
website/views/organization.py (8)
12-12: LGTM - Imports support new bounty payout functionality.The new imports are properly used in the
process_bounty_payoutendpoint:
SocialAccount: Maps GitHub usernames to BLT usersmail_admins: Sends critical alerts for failed cancellationscsrf_exempt: Allows GitHub Actions to call the endpoint without CSRF tokenAlso applies to: 20-20, 38-38
3748-3769: LGTM - Proper authentication and CSRF exemption.The endpoint correctly uses:
@csrf_exempt: Necessary for GitHub Actions to POST without CSRF token- Optional token authentication: If
BLT_API_TOKENis configured in settings, requests must provide matchingX-BLT-API-TOKENheaderThis addresses past review feedback about CSRF protection blocking GitHub Actions.
3797-3806: Excellent - Repository allowlist prevents budget drain attacks.The allowlist check is critical security: without it, an attacker could create issues in their own repo with bounty labels and drain the sponsors budget. This properly restricts payouts to authorized repositories only.
As noted in past review comments, this addresses a "catastrophic payout integrity bug."
Based on past review comments.
3830-3832: LGTM - Proper validation of issue state and PR merge status.The code correctly:
- Line 3831: Rejects payment if issue is not closed (prevents paying for open/incomplete work)
- Lines 3834-3860: Verifies PR is merged if
pr_urlis provided (prevents paying for abandoned/rejected PRs)This addresses past review feedback to "Block payouts until the issue is actually closed" and confirm PR merge.
Based on past review comments.
Also applies to: 3834-3860
3862-3878: LGTM - Robust bounty amount parsing.The regex-based parsing correctly handles various label formats:
$50→ 50$50 - medium→ 50$1,000 reward→ 1000This addresses past review feedback: "Robustly parse the bounty amount label" to avoid ValueError on labels with suffixes.
Based on past review comments.
3896-3903: LGTM - Correct user lookup via SocialAccount.The code properly maps GitHub assignee to BLT user profile using
SocialAccountwith provider="github" and matching the GitHub login. Returns clear 404 error if user is not registered in BLT.
4011-4029: LGTM - Exact tier matching prevents underpayment.The code correctly finds only exact matching sponsorship tiers (line 4018:
monthlyPriceInCents == target_amount_cents). This prevents the bug mentioned in past reviews wheremin(tiers, key=abs(diff))could select a lower-priced tier and silently underpay the bounty.Returns clear error if no exact match exists, prompting the user to create the appropriate tier.
Based on past review comments.
3928-3943: LGTM - Duplicate payment prevention before processing.The duplicate check runs before creating the sponsorship (not after), preventing double charges. This is critical for avoiding repeated payments if the workflow runs multiple times or if there's a retry.
Returns clear error with payment timestamp if bounty was already paid.
|
|
||
| # Bounty Payout Configuration | ||
| # API token for authenticating bounty payout requests | ||
| BLT_API_TOKEN = os.environ.get("BLT_API_TOKEN") | ||
| # GitHub username that sponsors will be created from (default: DonnieBLT) | ||
| GITHUB_SPONSOR_USERNAME = os.environ.get("GITHUB_SPONSOR_USERNAME", "DonnieBLT") | ||
| # Allowlist of repositories eligible for automated bounty payouts (prevents budget drain attacks) | ||
| # Format: {"owner/repo", "another-org/another-repo"} | ||
| BLT_ALLOWED_BOUNTY_REPOS = { | ||
| os.environ.get("BLT_ALLOWED_BOUNTY_REPO_1", "OWASP-BLT/BLT"), | ||
| } | ||
| # Add additional repos from environment if configured | ||
| if os.environ.get("BLT_ALLOWED_BOUNTY_REPO_2"): | ||
| BLT_ALLOWED_BOUNTY_REPOS.add(os.environ.get("BLT_ALLOWED_BOUNTY_REPO_2")) | ||
| if os.environ.get("BLT_ALLOWED_BOUNTY_REPO_3"): | ||
| BLT_ALLOWED_BOUNTY_REPOS.add(os.environ.get("BLT_ALLOWED_BOUNTY_REPO_3")) |
There was a problem hiding this comment.
🧩 Analysis chain
Document these settings for production deployment.
The bounty payout configuration requires environment variables to function properly. Ensure deployment documentation includes:
BLT_API_TOKEN: Required for authenticating bounty payout requests from GitHub ActionsGITHUB_SPONSOR_USERNAME: The GitHub account that will sponsor users (currently defaults to "DonnieBLT")BLT_ALLOWED_BOUNTY_REPO_1,BLT_ALLOWED_BOUNTY_REPO_2,BLT_ALLOWED_BOUNTY_REPO_3: Repositories eligible for automated bounty payouts (prevents unauthorized repos from draining budget)
Consider verifying these are documented in the deployment guide or README:
🏁 Script executed:
#!/bin/bash
# Check if deployment documentation mentions these new environment variables
rg -i "BLT_API_TOKEN|GITHUB_SPONSOR_USERNAME|BLT_ALLOWED_BOUNTY_REPO" --type=mdLength of output: 0
The markdown search returned no results. Let me conduct a broader search to verify whether these environment variables are documented anywhere in the codebase, including non-markdown formats and environment configuration files.
🏁 Script executed:
#!/bin/bash
# Search more broadly for documentation of these variables
# Check all file types, not just markdown
echo "=== Searching entire codebase for these variables ==="
rg -i "BLT_API_TOKEN|GITHUB_SPONSOR_USERNAME|BLT_ALLOWED_BOUNTY_REPO" -C2
echo ""
echo "=== Checking for .env files or documentation templates ==="
fd -e env -e sample -e example -e txt -e rst | head -20
echo ""
echo "=== Looking for docs or documentation directories ==="
fd -t d "doc|guide|config" | head -20Length of output: 2678
Let me verify what deployment documentation exists:
🏁 Script executed:
#!/bin/bash
# Check deployment and README documentation
echo "=== Files in docs/ directory ==="
find docs -type f | head -30
echo ""
echo "=== Root README files ==="
ls -la | grep -i readme
echo ""
echo "=== Checking for deployment/setup docs ==="
rg -i "deployment|setup|environment|configure" docs/ --type=md 2>/dev/null | head -20Length of output: 1571
Let me check the specific content of the setup documentation to see if these bounty variables are mentioned:
🏁 Script executed:
#!/bin/bash
# Check Setup.md for bounty variables
echo "=== Searching Setup.md for bounty-related variables ==="
rg "BLT_API_TOKEN|GITHUB_SPONSOR|ALLOWED_BOUNTY" docs/Setup.md
echo ""
echo "=== Searching bot-setup.md for bounty-related variables ==="
rg "BLT_API_TOKEN|GITHUB_SPONSOR|ALLOWED_BOUNTY" docs/bot-setup.md
echo ""
echo "=== Check if .env.example exists ==="
find . -name ".env.example" -o -name "*.env*"
echo ""
echo "=== View Setup.md environment variables section ==="
sed -n '/environment variables/,/^## /Ip' docs/Setup.md | head -40Length of output: 679
🏁 Script executed:
#!/bin/bash
# Check .env.example for bounty configuration
echo "=== Full content of .env.example ==="
cat .env.exampleLength of output: 1412
Add bounty payout environment variables to .env.example and deployment documentation.
The bounty payout configuration is missing from .env.example and deployment documentation. Since these variables are used in website/views/organization.py for critical bounty payout functionality, they must be documented:
BLT_API_TOKEN: Required for authenticating bounty payout requests from GitHub ActionsGITHUB_SPONSOR_USERNAME: The GitHub account that will sponsor users (defaults to "DonnieBLT")BLT_ALLOWED_BOUNTY_REPO_1/2/3: Repositories eligible for automated bounty payouts (prevents unauthorized repos from draining budget)
Add these to .env.example with example values and document them in docs/Setup.md environment variables section.
🤖 Prompt for AI Agents
In blt/settings.py around lines 634 to 649 the bounty payout env vars used by
website/views/organization.py are not documented; add BLT_API_TOKEN,
GITHUB_SPONSOR_USERNAME (default DonnieBLT), BLT_ALLOWED_BOUNTY_REPO_1,
BLT_ALLOWED_BOUNTY_REPO_2, and BLT_ALLOWED_BOUNTY_REPO_3 to .env.example with
example values (e.g., BLT_API_TOKEN=changeme, GITHUB_SPONSOR_USERNAME=DonnieBLT,
BLT_ALLOWED_BOUNTY_REPO_1=OWASP-BLT/BLT, BLT_ALLOWED_BOUNTY_REPO_2=org/repo,
BLT_ALLOWED_BOUNTY_REPO_3=org/repo), and update docs/Setup.md in the environment
variables section to document each key, its purpose, required/optional status,
and the default for GITHUB_SPONSOR_USERNAME so deployers know to set them before
enabling bounty payouts.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.env.example (1)
42-51: Reorder bounty payout environment variables alphabetically.The dotenv-linter flags that
BLT_ALLOWED_BOUNTY_REPO_*keys should appear beforeBLT_API_TOKENfor consistent alphabetical ordering. While this does not affect functionality, it improves consistency and maintainability.+ # --- Bounty payout configuration --- + # Repositories eligible for automated bounty payouts (prevents unauthorized repos from draining budget) + BLT_ALLOWED_BOUNTY_REPO_1=OWASP-BLT/BLT + # Optionally allow more repos: + BLT_ALLOWED_BOUNTY_REPO_2=other-org/other-repo + BLT_ALLOWED_BOUNTY_REPO_3=another-org/another-repo + # Required for authenticating bounty payout requests from GitHub Actions + BLT_API_TOKEN=your-bounty-api-token-here + # The GitHub account that will sponsor users (defaults to "DonnieBLT") + GITHUB_SPONSOR_USERNAME=DonnieBLT
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
.env.example(1 hunks)docs/Setup.md(1 hunks)
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 48-48: [UnorderedKey] The BLT_ALLOWED_BOUNTY_REPO_1 key should go before the BLT_API_TOKEN key
(UnorderedKey)
[warning] 50-50: [UnorderedKey] The BLT_ALLOWED_BOUNTY_REPO_2 key should go before the BLT_API_TOKEN key
(UnorderedKey)
[warning] 51-51: [UnorderedKey] The BLT_ALLOWED_BOUNTY_REPO_3 key should go before the BLT_API_TOKEN key
(UnorderedKey)
⏰ 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: Run Tests
- GitHub Check: docker-test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
.env.example (1)
42-51: Environment variables and documentation are clear and appropriate.Comments accurately describe the purpose of each variable, security implications (repo allowlist), and default values. The three-slot allowlist design aligns with the PR objectives and provides flexibility for multi-repo configurations.
docs/Setup.md (2)
28-48: Well-structured documentation for bounty payout configuration.The new section clearly explains the purpose, usage, and security implications of each environment variable. The example configuration matches
.env.exampleentries, and the security notes appropriately emphasize token confidentiality and the importance of the repo allowlist. Documentation is well-placed in the setup flow.
26-48: Documentation formatting is consistent and professional.The section uses appropriate markdown conventions (heading level, code blocks, inline code formatting, bullet lists), maintains visual hierarchy, and fits seamlessly with the existing documentation style. The visual separator clearly delineates the new content.
|
Hi @DonnieBLT,
Please review the new safeguards and let me know if further changes are needed. |
- Switch test database to file-based SQLite to avoid transaction conflicts - Add @tag('serial') to MySeleniumTests to prevent parallel execution - Add serialized_rollback=True for proper transaction handling - Update .gitignore to exclude test_db.sqlite3 - Add cleanup step in CI/CD workflow Fixes: sqlite3.OperationalError: cannot open savepoint - SQL statements in progress
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
website/tests.py (1)
41-42: Consider annotating withtyping.ClassVarfor clarity.The
serialized_rollbackclass attribute should be annotated withtyping.ClassVar[bool]to distinguish it as a class-level configuration rather than an instance attribute, per PEP 526.Apply this diff to add the annotation:
+from typing import ClassVar + @tag("serial") class MySeleniumTests(LiveServerTestCase): fixtures = ["initial_data.json"] # Ensure database operations are serialized for Selenium tests - serialized_rollback = True + serialized_rollback: ClassVar[bool] = TrueBased on static analysis hints.
blt/settings.py (1)
659-666: Consider building the repository set more elegantly.The current pattern of creating a set and mutating it works but could be more concise.
Apply this diff for a more functional approach:
-# Allowlist of repositories eligible for automated bounty payouts (prevents budget drain attacks) -# Format: {"owner/repo", "another-org/another-repo"} -BLT_ALLOWED_BOUNTY_REPOS = { - os.environ.get("BLT_ALLOWED_BOUNTY_REPO_1", "OWASP-BLT/BLT"), -} -# Add additional repos from environment if configured -if os.environ.get("BLT_ALLOWED_BOUNTY_REPO_2"): - BLT_ALLOWED_BOUNTY_REPOS.add(os.environ.get("BLT_ALLOWED_BOUNTY_REPO_2")) -if os.environ.get("BLT_ALLOWED_BOUNTY_REPO_3"): - BLT_ALLOWED_BOUNTY_REPOS.add(os.environ.get("BLT_ALLOWED_BOUNTY_REPO_3")) +# Allowlist of repositories eligible for automated bounty payouts (prevents budget drain attacks) +# Format: {"owner/repo", "another-org/another-repo"} +BLT_ALLOWED_BOUNTY_REPOS = { + repo + for repo in [ + os.environ.get("BLT_ALLOWED_BOUNTY_REPO_1", "OWASP-BLT/BLT"), + os.environ.get("BLT_ALLOWED_BOUNTY_REPO_2"), + os.environ.get("BLT_ALLOWED_BOUNTY_REPO_3"), + ] + if repo +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
.github/workflows/ci-cd.yml(1 hunks).gitignore(1 hunks)blt/settings.py(2 hunks)website/tests.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🪛 Ruff (0.14.0)
website/tests.py
40-40: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (5)
website/tests.py (1)
9-9: LGTM - Tag import and decorator correctly applied.The
@tag("serial")decorator ensures Selenium tests run serially, avoiding SQLite concurrency issues that can cause "cannot open savepoint" errors.Also applies to: 38-38
.github/workflows/ci-cd.yml (2)
107-109: LGTM - Clear documentation of test configuration.The comments helpfully explain the Selenium test setup and why file-based SQLite is used to avoid concurrency issues.
110-112: LGTM - Proper test artifact cleanup.The cleanup step correctly uses
if: always()to ensuretest_db.sqlite3is removed after tests complete, regardless of test outcome.blt/settings.py (2)
333-360: LGTM - Database configuration correctly isolates test mode.The conditional logic properly ensures:
- File-based SQLite (
test_db.sqlite3) is used during testing to avoid concurrency/savepoint issues- The
if not TESTINGguard at line 359 preventsDATABASE_URLfrom overwriting the test database configuration- The timeout increase to 30 seconds provides better handling for concurrent test access
652-666: Documentation verified—no issues found.The bounty payout environment variables are properly documented in
.env.example(lines 44–51) anddocs/Setup.md(lines 32–34), including security warnings. The configuration code is correct.
2 participants
Fixes #3941
Summary
This PR implements automatic bounty payment processing when a pull request is merged that closes an issue with a bounty label. The system detects merged PRs, extracts linked issues, verifies bounty labels, and initiates payment through the GitHub Sponsors API.
Changes
Database
preferred_payment_methodfield to UserProfile model to store user's payment preference (GitHub Sponsors or Bitcoin Cash)0247_userprofile_preferred_payment_method.pyBackend API
/api/bounty_payout/inwebsite/views/organization.pyX-BLT-API-TOKENheaderblt/urls.pyGitHub Actions
.github/workflows/auto-bounty-payout.ymlImplementation Details
The workflow looks for PR descriptions containing "Fixes #123", "Closes #456", or "Resolves #789" patterns to identify linked issues. For each linked issue with a bounty label, it makes an API call to process the payment.
The API endpoint validates the request, fetches issue details from GitHub, verifies the assignee has a BLT account with connected GitHub profile, checks their payment preference, and records the transaction. Currently uses placeholder transaction IDs pending full GitHub Sponsors GraphQL API integration.
Improvements Over Previous Attempt (PR #4236)
Based on CodeRabbit's review of the previous implementation, this version addresses several critical issues:
Known Limitations
Testing
All Python files compile without syntax errors. Migration dependency has been verified against latest migration (0246). Error handling covers all expected failure cases including missing assignee, invalid issue URL, user not found, and duplicate payments.
Manual testing required after deployment to verify end-to-end workflow.
Deployment Notes
python manage.py migrateThis is my first contribution to the project. I've tried to follow the existing code patterns and address feedback from previous attempts. Happy to make any changes based on review feedback.
Summary by CodeRabbit
New Features
Admin
Documentation
Chores
Tests