Skip to content
This repository was archived by the owner on Jul 20, 2020. It is now read-only.
37 changes: 18 additions & 19 deletions php/PaypalIPN.php
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public function getPaypalUri()
* Verification Function
* Sends the incoming post data back to PayPal using the cURL library.
*
* @return bool
* @return array or false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docblock should be array|bool

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments below.

* @throws Exception
*/
public function verifyIPN()
Expand All @@ -75,31 +75,31 @@ public function verifyIPN()
$raw_post_data = file_get_contents('php://input');
$raw_post_array = explode('&', $raw_post_data);
$myPost = array();
$magic_quotes_enabled = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic quotes has been DEPRECATED as of PHP 5.3.0 and REMOVED as of PHP 5.4.0.
I don't see the value in adding a check here - I would rather specify a minimum PHP version,
http://php.net/manual/en/security.magicquotes.php

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this.

if (function_exists('get_magic_quotes_gpc') && get_magic_quotes_gpc() == 1) {
$magic_quotes_enabled = true;
}
foreach ($raw_post_array as $keyval) {
$keyval = explode('=', $keyval);
if (count($keyval) == 2) {
// Since we do not want the plus in the datetime string to be encoded to a space, we manually encode it.
if ($keyval[0] === 'payment_date') {
if (substr_count($keyval[1], '+') === 1) {
$keyval[1] = str_replace('+', '%2B', $keyval[1]);
// Since we do not want the plus signs in the date and email strings to be encoded to a space, we use rawurldecode instead of urldecode
if (($keyval[0] === 'payment_date' && substr_count($keyval[1], '+') === 1) || strpos(rawurldecode($keyval[1]), ' ') !== false || ($_POST["test_ipn"] == 1 && filter_var($keyval[1], FILTER_VALIDATE_EMAIL))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is also much too long.

// Keep plus signs
$myPost[$keyval[0]] = rawurldecode($keyval[1]);
} else {
// Convert plus signs to spaces
$myPost[$keyval[0]] = urldecode($keyval[1]);
if ($magic_quotes_enabled) {
$myPost[$keyval[0]] = stripslashes($myPost[$keyval[0]]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my option the logic above has to much nesting, and it's hard to understand what is going on.
I'm not sure why this is required, I have been running the current version of this script in production for > 1 year without any issues.

Can you update the PR with some more information about the issue, and some replication steps?

}
}
$myPost[$keyval[0]] = urldecode($keyval[1]);
}
}

// Build the body of the verification post request, adding the _notify-validate command.
$req = 'cmd=_notify-validate';
$get_magic_quotes_exists = false;
if (function_exists('get_magic_quotes_gpc')) {
$get_magic_quotes_exists = true;
}
foreach ($myPost as $key => $value) {
if ($get_magic_quotes_exists == true && get_magic_quotes_gpc() == 1) {
$value = urlencode(stripslashes($value));
} else {
$value = urlencode($value);
}
$value = rawurlencode($value);
$req .= "&$key=$value";
}

Expand Down Expand Up @@ -136,11 +136,10 @@ public function verifyIPN()

curl_close($ch);

// Check if PayPal verifies the IPN data, and if so, return true.
// Check if PayPal verifies the IPN data, and if so, return data array.
if ($res == self::VALID) {
return true;
} else {
return false;
return $myPost;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this class should have a single responsibility, to verify that the IPN data is valid.
You can then retrieve the post data from $__POST or your frameworks request object. I do not see a reason why this needs to return data.

Copy link
Contributor Author

@BigRedBot BigRedBot Jan 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least from the sandbox, it is in some cases receiving incorrectly encoded data. This can cause the data in $_POST to be incorrect. Since we can compensate for this and rebuild the data, then we should at that point use the validated rebuilt data.

There is pretty much no reason for it not to include the verified array. It already has it, so why not just return it?

Copy link
Contributor Author

@BigRedBot BigRedBot Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only other option (that has not been done yet), is to have paypal's servers always make sure that $_POST is always correct no matter where the data is coming from. This would still require the use of the rawurlencode function, to be sure that plus signs are correctly verified.

}
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's best not to return multiple types from a function - it would make more sense to return null vs false here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

}
}