Skip to content

[PM-36079] fix: re-introduce PayPal IPN postback verification#7784

Draft
connerbw wants to merge 1 commit into
mainfrom
billing/pm-36079/paypal-ipn-postback-verification
Draft

[PM-36079] fix: re-introduce PayPal IPN postback verification#7784
connerbw wants to merge 1 commit into
mainfrom
billing/pm-36079/paypal-ipn-postback-verification

Conversation

@connerbw

@connerbw connerbw commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

🎟️ Tracking

Resolves PM-36079 (originating finding: VULN-550 / HackerOne).

📔 Objective

POST /paypal/ipn previously authenticated inbound PayPal IPN messages only with the static ?key= query-string secret (plus a ReceiverId == BusinessId check, where BusinessId is public). The repository shipped a real PayPal postback validator (IPayPalIPNClient.VerifyIPN, the cmd=_notify-validate round-trip), but it was dead code — wired up in #3619 and removed in #3882 because the previous implementation broke constantly and, being fail-closed, dropped legitimate payments.

This PR re-introduces postback verification as a defense-in-depth layer, implemented to PayPal's current spec so it no longer regresses payment reliability:

  • Correct endpoint: posts to the dedicated ipnpb.paypal.com / ipnpb.sandbox.paypal.com host (the legacy www.paypal.com/cgi-bin/webscr redirects and silently drops the POST body).
  • Required header: the typed HttpClient now sends a descriptive User-Agent (PayPal returns 403 without one) and a 10s timeout.
  • Safe failure semantics: VerifyIPN returns a tri-state result. Invalid (PayPal reports the message is not genuine) blocks processing; a transient failure (Unverified — network error / timeout / non-200) fails open and still processes, so a PayPal-side hiccup can never again drop legitimate credit.
  • Feature-flagged kill-switch: enforcement is gated behind FeatureFlagKeys.PM36079_PayPalIpnVerification (default off) for gradual rollout and instant disable without a deploy.

A forged / INVALID IPN now returns 200 OK without crediting — this acknowledges PayPal (avoiding retry storms) and gives an attacker no detection oracle. Verification is placed before the credit/refund switch, so both the credit and refund paths are protected.

Ops follow-up: alert on the Unverified warning log line — a sustained spike is the only signature of the fail-open path being exercised.

Testing

  • PayPalIPNClientTests: updated for the ipnpb endpoint and tri-state result, including a transient-exception case.
  • PayPalControllerTests: added coverage for INVALID blocking a payment, INVALID blocking a refund, Unverified still processing (fail-open), Verified processing, and the flag-disabled path skipping verification.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@connerbw connerbw added ai-review Request a Claude code review t:bugfix Change Type - Bugfix labels Jun 8, 2026
@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Re-introduces PayPal IPN postback verification (cmd=_notify-validate) as a defense-in-depth layer behind the existing webhook key and ReceiverId checks, addressing VULN-550. Implementation targets the documented ipnpb postback host, adds the required User-Agent header and a 10s timeout, and uses a tri-state PayPalIPNVerificationResult so transient failures fail open (preserving payment reliability) while INVALID responses block both the credit and refund paths. Enforcement is gated by FeatureFlagKeys.PM36079_PayPalIpnVerification (default off) for staged rollout. Test coverage is thorough — Verified, Invalid (payment + refund), Unverified fail-open, and flag-disabled paths are all exercised, and PayPalIPNClientTests adds a transient-exception case.

Code Review Details

No findings. The fail-open semantics on Unverified, the broad catch (Exception) in VerifyIPN, and the Unhandled response mapping to Unverified are all deliberate, documented choices that prioritize payment reliability behind the network ACL and webhook key. The PR description correctly flags the ops follow-up (alerting on the Unverified warning log) as the operational signal for fail-open exercise.

@sonarqubecloud

sonarqubecloud Bot commented Jun 8, 2026

Copy link
Copy Markdown

Comment thread src/Billing/Services/Implementations/PayPalIPNClient.cs Dismissed
@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.85106% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.05%. Comparing base (582ce40) to head (6a0b8ba).

Files with missing lines Patch % Lines
src/Billing/Startup.cs 0.00% 6 Missing ⚠️
...illing/Services/Implementations/PayPalIPNClient.cs 86.36% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7784   +/-   ##
=======================================
  Coverage   61.05%   61.05%           
=======================================
  Files        2173     2173           
  Lines       96580    96609   +29     
  Branches     8691     8693    +2     
=======================================
+ Hits        58963    58987   +24     
- Misses      35516    35521    +5     
  Partials     2101     2101           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Labels

ai-review Request a Claude code review t:bugfix Change Type - Bugfix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant