Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dev/core#1931 Prevent PayPal from double-encoding the IPN Notify URL #18980

Merged
merged 1 commit into from
Nov 19, 2020

Conversation

christianwach
Copy link
Member

Overview

Solves this issue on the Lab.

Before

PayPal IPN return URLs contain double-encoded entities, e.g.

"POST /2020/09/04/donation-shortcode/?civiwp=CiviCRM&q=civicrm%252Fpayment%252Fipn%252F3 HTTP/1.1" 200

After

PayPal IPN return URLs do not contain double-encoded entities, e.g.

"POST /2020/09/04/donation-shortcode/?civiwp=CiviCRM&q=civicrm/payment/ipn/3 HTTP/1.1"

Technical Details

When CiviCRM does not use Clean URLs (or, in WordPress, if a Shortcode is used for a Contribution Page) then IPN notifications fail to register contributions as "Completed". This applies to all CMSes where Clean URLs are not in use.

@civibot
Copy link

civibot bot commented Nov 16, 2020

(Standard links)

@civibot civibot bot added the master label Nov 16, 2020
@mattwire
Copy link
Contributor

@christianwach style warning

@christianwach
Copy link
Member Author

@mattwire Argh, thanks for the heads up.

@agileware-justin
Copy link
Contributor

Thanks @christianwach

@eileenmcnaughton eileenmcnaughton changed the title Prevent PayPal from double-encoding the IPN Notify URL dev/core#1931 Prevent PayPal from double-encoding the IPN Notify URL Nov 19, 2020
@eileenmcnaughton
Copy link
Contributor

@agileware-justin are you able to test this?

@agileware-justin
Copy link
Contributor

@eileenmcnaughton patch has been deployed to production for 2 days - no issues reported. OK to merge AFAIK

@kcristiano
Copy link
Member

I have done r-run on this as well. I have been adding to our PayPal sites and it fixes the issues as described.

OK to merge.

This should also be a considered for 5.32.

@seamuslee001
Copy link
Contributor

Merging based on review of @kcristiano and @agileware-justin

@seamuslee001 seamuslee001 merged commit 1b2e98a into civicrm:master Nov 19, 2020
@eileenmcnaughton
Copy link
Contributor

@kcristiano @christianwach per "This should also be a considered for 5.32." - if you want to put up a PR against 5.32 I'm happy to merge it to the rc / look at it for any future 5.31 releases. It's not clear to what extent it's a regression but I guess it at least sort of is

@kcristiano
Copy link
Member

kcristiano commented Nov 19, 2020

@eileenmcnaughton I think it's a PayPal regression :) It might be an issue related to the work we did in 5.26 for the WP URL changes, or something PayPal did. It's a big enough deal that I'd like it in sooner rather than later.

#18996

@agileware-justin
Copy link
Contributor

@christianwach christianwach deleted the lab-core-1931 branch March 22, 2022 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants