Updated: Comment #14

Problem/Motivation

User hits an access denied error, when clicking on "Back to store" link on the paypal confirm page after a successful paypment.

Steps to reproduce:

  1. Create a PPS payment method via the UI
  2. Create a payment form with as in paypal_payment_pps_test_payment_form()
  3. User hits payment button and gets redirected to paypal
  4. User logs into the paypal account and confirms the payment
  5. Paypal calls in on /paypal_payment_ipn and a record is created in the table paypal_payment_ipn
  6. User clicks on 'back to store'-link
  7. /paypal_payment_pps/return is hit and function paypal_payment_pps_return_access() is called, which subsequently calls PayPalPaymentIPNController::validate($_POST)
  8. PayPalPaymentIPNController::validate returns FALSE because in line 141 in PayPalPaymentIPNController.inc the load function returns the record created in step 3

Is that a correct analysis of what happens? Is this how it should happen? It seems wrong that the validate method fails when the POST-Request is valid but has already been processed.

Proposed resolution

The proposed solution in #8 adds a test to discover this issue as well as a solution based on the following:

  • Grant access to the return function when valid POST data as expected from PayPal is present in the request, so as to identify the transaction.
  • Grant access to the return function only to the user associated with the transaction.
  • Process the incoming data only if it has not been processed, so to say on the first request (in the given scenario this is the request generated by IPN).

Remaining tasks

Review patch #8.

Ideally also test the test coverage. For that you can apply the patch #6.
With this patch you should run the paypal_payment tests and it should fail. Then revert the changes, apply patch #8, run the tests again. They should pass. Follow the steps above to reproduce the error. It should not produce an access denied when coming back from the paypal site.

Files: 
CommentFileSizeAuthor
#56 paypal_payment_pps-HTTP-404-on-return-uri-2052361-56.patch0 bytesaapis
PASSED: [[SimpleTest]]: [MySQL] 167 pass(es). View
#56 paypal_payment_pps-HTTP-404-on-return-uri-2052361-56.patch0 bytesaapis
PASSED: [[SimpleTest]]: [MySQL] 167 pass(es). View
#55 paypal_payment_pps-HTTP-404-on-return-uri-2052361-55.patch2.79 KBmxr576
PASSED: [[SimpleTest]]: [MySQL] 167 pass(es). View
#36 interdiff-33-36.txt7.77 KBtorotil
#6 paypal_payment_pps-ipn_test-6.patch5.1 KBberliner
FAILED: [[SimpleTest]]: [MySQL] 179 pass(es), 2 fail(s), and 0 exception(s). View
#3 paypal_payment_pps-return_access-3.patch1.38 KBberliner
PASSED: [[SimpleTest]]: [MySQL] 167 pass(es). View
#1 paypal_payment_pps-return_access-1.patch1.27 KBberliner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch paypal_payment_pps-return_access-1.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

berliner’s picture

Category: support » bug
Status: Active » Needs review
FileSize
1.27 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch paypal_payment_pps-return_access-1.patch. Unable to apply patch. See the log in the details link for more information. View

I consider this a bug now. Please correct me if I'm wrong.

I came up with the following that fixes this issue for me. I think that this keeps the same level of access security as before.

Status: Needs review » Needs work

The last submitted patch, paypal_payment_pps-return_access-1.patch, failed testing.

berliner’s picture

FileSize
1.38 KB
PASSED: [[SimpleTest]]: [MySQL] 167 pass(es). View
berliner’s picture

Status: Needs work » Needs review
Xano’s picture

Status: Needs review » Needs work

Thanks for reporting this problem! Can you start with writing a test that exposes this bug? We can then prove the fix actually works by simply running the test.

berliner’s picture

Version: 7.x-1.0 » 7.x-1.x-dev
Status: Needs work » Needs review
FileSize
5.1 KB
FAILED: [[SimpleTest]]: [MySQL] 179 pass(es), 2 fail(s), and 0 exception(s). View

Ok, try this. My experience with tests is rather limited, but I hope this helps.
The patch includes only the test. Without the patch from #3 the test should fail, with the patch applied it should succeed.
Or should I open a new issue for the test?

Status: Needs review » Needs work

The last submitted patch, paypal_payment_pps-ipn_test-6.patch, failed testing.

berliner’s picture

Status: Needs work » Needs review
FileSize
6.48 KB
PASSED: [[SimpleTest]]: [MySQL] 181 pass(es). View

ok, that worked as it should, now trying with a combination of both patches

Xano’s picture

Thanks! I have one question: you created a completely new test case, while there already is a test case for PPS payment execution. Is there a reason why you did not add the IPN test there?

berliner’s picture

Main reason is, that I couldn't figure out a way to keep the dependency to paypal_payment_pps_test module, but instead needed to use the paypal_payment_ipn_test module. Both modules implement hook_outbound_url_alter and point every call to the paypal website to different insite urls. Then, semantically, it seemed ok to create a new test case, because the usage of IPN is kind of optional.

Feel free to rearrange the test cases. As I said before, my knowledge of tests is limited and every time I touch them I run into strange things that I have difficulties to understand.

berliner’s picture

Is there anything else I can help with?

juggernautsei’s picture

HI, could you please post some instructions on how to apply the patch. I would greatly appreciate it.

Thank you for you fast / swift reply.

Sherwin

berliner’s picture

@juggernautsei: Please read Applying patches for detailed instructions.

fredeee’s picture

Hi

can confirm this issue on two independent sites.

it OCCURS using paypal standard and NOT with paypal express.

on returning after paypal on url ... paypal_payment_pps/return

get message 'Access denied' you are not authorized to access this page.

when the message is displayed the payment still occurs correctly and shows as completed in payment reference field.

can close the 'access denied' page and continue on with the original node and save the page.

Configuration items:
Drupal 7.22
Domain Access
Payment v7.x-1.8
paypal for payment
plus other stuff

have not looked into it any further.

Cheers

berliner’s picture

@fredeee: Have you tried to apply the patch from #8?

Xano’s picture

What if we just decouple the existence of an IPN in the database from its validation? PayPalPaymentIPNController::validate() would no longer check for an existing IPN, but instead just validate the IPN's contents, and paypal_payment_ipn_post() would check if the received IPN already exists in the database, before acknowledging and saving it.

berliner’s picture

Just my 2cents:
As I understand the purpose of PayPalPaymentIPNController::validate() it is not only about validating post data structurally (meaning checking if it contains the keys txn_id and invoice) but also to verify that the referenced transaction has actually been issued by the site. That makes sense to me. But then again, I can't claim to see all implications.

My idea would be to solve this bug as soon as possible in a pragmatic way and to restructure the code later on in order to provide a better architecture if you feel the need for that. That's something that I personally do not have the ressources right now (even if I would like to help out with this).

berliner’s picture

FileSize
5.82 KB
6.9 KB
FAILED: [[SimpleTest]]: [MySQL] 128 pass(es), 22 fail(s), and 34 exception(s). View

After giving it a little more thought, I guess you are right. So I tried to decouple it. Unfortunately that turned out to be more difficult than I initially thought. I didn't realize at first, that the acknowledging is only working if the call comes from IPN. If the context is the return call that leads the customer back to the site, then the acknowledging will fail. That's why I had to refactor quite a lot.

It's late here, so I will let the bot do the testing.

Status: Needs review » Needs work
Xano’s picture

Never mind.

berliner’s picture

Ok, that's a lot more than I expected.

Xano’s picture

Assigned: Unassigned » Xano

I'm puzzling with a patch a bit.

Xano’s picture

A short review of the patch in #18. I'm working on a patch that addresses these issues.

  1. +++ b/paypal_payment_ipn/includes/PayPalPaymentIPNController.inc
    @@ -95,7 +95,7 @@ abstract class PayPalPaymentIPNController {
    -    $data = '';
    +    $data = array();
    

    Good catch, but not in the scope of this patch.

  2. +++ b/paypal_payment_ipn/includes/PayPalPaymentIPNController.inc
    @@ -136,29 +136,24 @@ abstract class PayPalPaymentIPNController {
    +    if (!isset($ipn_variables['txn_id']) || !isset($ipn_variables['invoice'])) {
    

    We no longer need the txn_id.

  3. +++ b/paypal_payment_ipn/paypal_payment_ipn.module
    @@ -26,16 +26,38 @@ function paypal_payment_ipn_menu() {
    +  if (PayPalPaymentIPNController::validate($ipn_variables) && PayPalPaymentIPNController::acknowledge($ipn_variables)) {
    

    Validation before acknowledgement makes sense in a way, but there was *some* documentation that said you had to acknowledge every IPN you got regardless of its contents. Say you create a payment that is executed, and then deleted. You will still be getting IPNs for it, but validation will fail, because the invoice_id no longer matches a payment ID. However, PayPal requires you to confirm that you received the IPN.

  4. +++ b/paypal_payment_ipn/paypal_payment_ipn.module
    @@ -26,16 +26,38 @@ function paypal_payment_ipn_menu() {
    +function paypal_payment_ipn_get_decoded_data() {
    +  $ipn_variables = $_POST;
    

    Ideally $_POST would be passed on as a function argument, because we do not want a tight coupling between logic and anything outside the direct Drupal environment. When testing, for instance, the data does not come from POST.

  5. +++ b/paypal_payment_ipn_test/paypal_payment_ipn_test.module
    @@ -60,4 +60,13 @@ function paypal_payment_ipn_test_paypal_server() {
    +function paypal_pament_ipn_finish_callback(Payment $payment) {
    

    This function is never used.

  6. +++ b/paypal_payment_pps/paypal_payment_pps.info
    @@ -7,6 +7,7 @@ core = 7.x
    +files[] = tests/PayPalPaymentPPSPaymentIPNExecution.test
    

    This file was not added to the patch.

berliner’s picture

to 2. Why is that?
to 5. The function paypal_pament_ipn_finish_callback is part of the paypal_payment_ipn_test module.
to 6. Sorry, there was something wrong with the patch I guess. That file was originally introduced in Patch #6.

I have updated the patch width 1, 3, 4 and 6 fixed, just to see if that changes something on the test results.

Xano’s picture

2) the txn_id key was only used to check if an IPN already existed in the DB. That is no longer done, so we don't need to check for the key's existence anymore.

5) If it's part pf paypal_payment_ipn_test, it will need to be namespaced as such.

Also, I assigned the issue to myself to aggregate the results of the patches and our discussions into one smaller patch. Yours unfortunately tries to do too many things that make understanding the solution harder to do.

berliner’s picture

2) I thought that checking for the txn_id key still made sense for validation, as we can't really do anything with the data if this key is not present.
5) Obviously you are right. Besides from that it is misspelled.

I didn't mean to interfere with your work, but having passed multiple hours yesterday on trying to figure out how this decoupling could be done, I wanted at least to provide a more complete patch.

Also I already thought about proposing to split this issue in multiple subtasks for better readability. But having few experience with paypal_payment and larger issues I didn't see how. As I always want to learn you might be able to educate me. What is my patch trying to do that is too much? Originally I only wanted to fix the return issue. Maybe you could proceed as I proposed in #2052361-17: paypal_payment_pps: HTTP 404 on return uri and commit the simple patch to fix the original issue (if it can be confirmed that it works) and then create a new issue for the decoupling?

I let you work now and am curious for the outcome.

Xano’s picture

Just a quick reply while making dinner: your patch does not only try to solve the problem, but also clean things up a bit. Parts of the code definitely need some cleaning up, but let's not do that here, so we can focus on fixing the problem. Too much code in the patch that is no part of the fix can distract us from seeing the important bits.

I'm hoping to dig into this a bit more later tonight. The biggest problem (which I believe you found as well), is that the communication with the remote server is not properly tested. paypal_payment_pps_test already reroutes requests to Paypal's servers, but that will need to be extended.

berliner’s picture

Ok, that's completely understandable. Thanks for explaining.

berliner’s picture

Issue summary: View changes

updated issue summary

ppelgrims’s picture

What is the best way to go about this problem? Could I use one of the patches or is there a new patch (or release) on the way? I can confirm this problem still exists in 7.x-1.1, unless I missed something.

freakalis’s picture

I tried the patch in #24 but found som problems that i fixed.

  1. function paypal_payment_ipn_process() is actually a Drupal hook_process() and gets run on every request. Replaced function name to paypal_payment_ipn_process_data().
  2. business was not set in the variables from paypal receiver_email seemed to be the right variable name. I don't know if something has changed in the API or if this just was a typing error?
    Old
    return strtolower($ipn_variables['business']) == strtolower($payment->method->controller_data['email_address']);
    

    New

    return strtolower($ipn_variables['receiver_email']) == strtolower($payment->method->controller_data['email_address']);
    
Xano’s picture

Thanks for looking into this! Let's keep this issue focused on the HTTP version problem and fix different problems in different issues. Can you post a new patch that doesn't contain all the other fixes?

torotil’s picture

I just had to re-roll the patch to work with -p1 (instead of -p2). It did the job for me.

ppelgrims’s picture

Status: Needs work » Needs review
FileSize
8.41 KB
FAILED: [[SimpleTest]]: [MySQL] 179 pass(es), 2 fail(s), and 0 exception(s). View

I think this should be it, made the namespace fit and got rid of some refactoring (the one that fixed 6 nested control structures for instance.) Didn't test it yet I'm going to wait for the automated testing to pass it. I'm not sure what is going on in #32

Status: Needs review » Needs work

The last submitted patch, 33: paypal_payment-pps_HTTP_404_on_return_uri-2052361-33.patch, failed testing.

ppelgrims’s picture

Feedback on the test is:
Access denied
You are not authorized to access this page.

torotil’s picture

Status: Needs work » Needs review
FileSize
7.77 KB
10.37 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch paypal_payment-pps_HTTP_404_on_return_uri-2052361-36.patch. Unable to apply patch. See the log in the details link for more information. View

My understanding of the paypal protocol for PPS is still a bit limited, but I've made the tests in #33 pass. For that I've simply modified PayPalPaymentIPNController::validate() to not check for an existing database entry (ie. any valid post array is fine).

Xano: We no longer need the txn_id.

Do you mean we don't need the txn_id at all (not even store it) or do you mean we don't need it for validation?

Status: Needs review » Needs work

The last submitted patch, 36: paypal_payment-pps_HTTP_404_on_return_uri-2052361-36.patch, failed testing.

torotil’s picture

Status: Needs work » Needs review
FileSize
11.08 KB
FAILED: [[SimpleTest]]: [MySQL] 182 pass(es), 1 fail(s), and 0 exception(s). View

This time rolling the patch with git format-patch :) -- The interdiff is still the same.

Status: Needs review » Needs work

The last submitted patch, 38: paypal_payment-pps_HTTP_404_on_return_uri-2052361-38.patch, failed testing.

torotil’s picture

Status: Needs work » Needs review
FileSize
11.96 KB
PASSED: [[SimpleTest]]: [MySQL] 183 pass(es). View

The existing test caught the change in behavior of ::validate() -- I've changed the assertion to reflect the new behavior: validate() only checks for a valid $_POST-array not for whether it has been saved already or not.

torotil’s picture

I'm still getting a 403 when returning from the paypal payment page. The reason is that I'm sent there via a GET-request. I think it's time to consult some API documentation.

torotil’s picture

Ok I've found the reason. It seems that there is several ways for paypal to return.

<meta http-equiv="refresh" content="5;url=http://server/paypal_payment/pps/return"/>

This one will redirect after 5 seconds without sending the post variables.

setTimeout("document.forms.merchantredirectform.submit()", 4000);

This one will redirect after 4 seconds sending the post variables.

Since my test-site doesn't have HTTPS my firefox asks me whether I really want to submit the form. This stalls the submission long enough for the meta-tag to take effect - leading to a 403 error.

welly’s picture

I've just tested out the patch at #40 and I'm still getting the same 404 error.

Fidelix’s picture

I'm not getting a 404 error.

Tried patch from #40 and I'm getting a 403 "Not Authorized" error.

drupix’s picture

FileSize
6.26 KB
PASSED: [[SimpleTest]]: [MySQL] 167 pass(es). View

Hi all,

First at all sorry for my approximative english...
Secondly sorry for my approximative explanation, even I'm a developper and I can understand technical things, I have big problems when I need technical talking... so :

I just spent a few days trying to solve this issue, especially with the information given by berliner (main problem), torotil at #42 and of course Xano at #31 .
After 2 days of dpm, watchdog and headaches, I could not really go on with the issue... So I decided to investigate how this works in commerce_payment and commerce_paypal module. Two more days of dpm, watchdog and headaches and I noticed that the same problem exists in commerce_paypal! Thank's torotil to point on this here #42 . So why it work with commerce_paypal payments?

I noticed that in the function commerce_payment_redirect_pane_checkout_form (that is in our case more or less the equivalent of paypal_payment_pps_return and paypal_payment_pps_return_access) they don't deal with the $_POST variable but they pass the order and other stuff as a params to the function and then they directlly deals with the order who contains a value named payment_redirect_key that was stored just before the submission to PayPal.
I wrote a patch that seems to work but I'm not really sure... so if someone can test it and give feed-back I will appreciate.

Thanks everyone for the works done.

drupix’s picture

FileSize
6.27 KB
PASSED: [[SimpleTest]]: [MySQL] 167 pass(es). View

Little changes...

drupix’s picture

Humm...

Sorry guys, it seems that my patch does not work after I activate the IPN in PayPal settings...

I keep trying...

jasen’s picture

Any updates on this issue?

marc.groth’s picture

FWIW, I was able to get this working in sandbox mode by applying the patch in #8.

Slightly off topic but I thought worth noting in case anybody else is having issues with the status not updating properly. It's probably common knowledge but just in case it helps someone else, here is a little gotcha. Note this is specifically in relation to sandbox mode:

* For the payment method email address, make sure the email address you enter here is one set up in sandbox mode (you can see a list of these by signing in to your developer account and clicking on 'Accounts' (under 'Sandbox') . I initially set this to the actual PayPal account email address and couldn't understand why the response (when completing payment) was always 'Pending'. I dug a bit further and realised that the 'pending_reason' value was 'unilateral' which means the payment was made to an email address that is not registered/confirmed. Full details on all the return values (and what they mean) are here: https://developer.paypal.com/docs/classic/ipn/integration-guide/IPNandPD...
* The payment method 'Capture' option needs to be 'Automatic' (at least in my case). Whenever I set this to 'Manual' I was never able to complete the transaction.

These are just my 2 cents in case anybody else finds themselves banging their head against the wall trying to figure out why payment wasn't being completed when it was a successful transaction!

wastrilith2k’s picture

Any updates? I applied the patch in #8 and I still receive the access denied.

wastrilith2k’s picture

A little more digging turned up the issue for me (I believe anyways).

I am using the PayPal Sandbox and it doesn't send the invoice back. paypal_payment_pps_return_access returns FALSE if this is missing.

Here is what the Sandbox was returning:

tx:9UR90742E29782031
st:Completed
amt:80.00
cc:USD
cm:
item_number:

Once I tested with production account settings it worked perfectly. Please note that this is still after the patch in #8 was applied.

mxr576’s picture

FileSize
2.82 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch paypal_payment_pps-HTTP-404-on-return-uri-2052361-52.patch. Unable to apply patch. See the log in the details link for more information. View

Greetings!

I've spent a whole bunch of time with debugging the #46 patch, when I finally realized, it can not be the solution anyway... but I've learned a lot during this time about how this should be work and because of that, I think I've found the root of this problem.

In the original code the PayPalPaymentIPNController::validate() function was used to validate whether the user should have access to the PPN's return page in paypal_payment_pps_return_access(), which is a mistake... This function should be used only for IPN calls, because as the first comment says in the function "// Make sure this IPN was not processed before." , so if the Paypal returned the payment information through the IPN first (which usually happens) and the user try to return to the site from the Paypal, then s/he gets an access denied page, because this validation function returns FALSE, since the Payment information already arrived already and processed through the IPN call, so there is nothing to do with this PPS call.
So I've replaced the paypal_payment_pps_return_access() function (as you can see, this is quite the same as the PayPalPaymentIPNController::validate(), without the problematic part) and I've updated the paypal_payment_pps_return() function as well (moved the other part of the validation there).

Feedbacks and reviews are warmly welcomed! Thx!

Status: Needs review » Needs work

The last submitted patch, 52: paypal_payment_pps-HTTP-404-on-return-uri-2052361-52.patch, failed testing.

mxr576’s picture

Status: Needs work » Needs review
FileSize
2.66 KB
PASSED: [[SimpleTest]]: [MySQL] 167 pass(es). View

Previous patch was made for the latest stable, here is the updated one for the latest dev.

mxr576’s picture

FileSize
2.79 KB
PASSED: [[SimpleTest]]: [MySQL] 167 pass(es). View

Minor fix.

aapis’s picture

FileSize
0 bytes
PASSED: [[SimpleTest]]: [MySQL] 167 pass(es). View
0 bytes
PASSED: [[SimpleTest]]: [MySQL] 167 pass(es). View

This solved the problem for me. $ipn_variables here is actually $_POST, but PayPal doesn't send 'business' back in the post data. Instead, it sends 'receiver_email'. I was am unable to determine if this is a configuration issue or a change to PayPal's API which I cannot find documentation for, but comparing against only the 'business' field threw an error on screen (unable to find index business of $ipn_variables) and denied access as paypal_payment_pps_return_access failed.

Uploaded the same patch twice for some reason. Sorry.

DigitalFrontiersMedia’s picture

@aapis, For whatever reason, your patches in #56 aren't showing up when I follow the links.

astutonet’s picture

Hello guys.

I'm implementing a payment system on a site that uses this module. At the end of the payment, the same error registered here occurs on the project.

Is there any news about this? Why the patches in #56 aren't displayed?

Can anyone help us, please?

mxr576’s picture

Please try my patch from #55. #56 contains empty patch files...

astutonet’s picture

Tks, I will try it.

torotil’s picture

I think this will be resolved with #2550693: IPN problem with PayPal's payment review since PayPalPaymentIPNController::validate() now allows multiple calls with the same invoice.

torotil’s picture

Status: Needs review » Fixed

The original issue should be solved with the latest release. There is perhaps still the issue with the 403 as described in #42. I think it's better to open a separate issue for this though.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.