The PayFlow Pro module inserts strings such as product titles directly into the XML document without escaping. If a product title contains a character such as &, this causes the payment to failure. Also, other fields like user name, email address, etc aren't escaped also.

I get a rather raw-looking XML parse error back from the server, telling me that I should correct my XML document and resubmit.

I've changed the code to use htmlspecialchars() on string fields before inserting them, which seems to work. It would be better possibly to build the entire request document using PHP's XMLWriter class instead, since it takes care of all that for you.

My patch from our local source control is attached anyway. This is against the current development release.

CommentFileSizeAuthor
#1 uc_payflowpro-318351-2.patch9.82 KBkwinters
uc_payflowpro.module.patch10.58 KBneilnz

Comments

kwinters’s picture

kwinters commented
Assigned: Unassigned » kwinters
StatusFileSize
new uc_payflowpro-318351-2.patch9.82 KB

Changed htmlspecialchars into check_plain (which does htmlspecialchars and some other stuff) and removed a debug line, patch attached.

I'll be setting it up in D6 as well and commit once I've tested.

If someone can verify for me that it works in the D5 branch then I will commit it there as well.

kwinters’s picture

kwinters commented

The encoding patch above now in CVS, and although I'm 98% sure that everything will work right in the D5 version, it would still be nice if someone could test it for me.

yktdan’s picture

yktdan commented
Priority: Normal » Critical

The beta1 version which claims to have this patch fails. No sales complete.

kwinters’s picture

kwinters commented

Are you using d5 or d6? Do sales of products with just alphanumeric characters in the product info complete? Do you get any watchdog / error log messages? Is this a new site, or did it used to use the ancient devel version (or some other)?

yktdan’s picture

yktdan commented
Version: 5.x-1.x-dev » 5.x-1.0-beta1

Was using ancient dev version. We are on Drupal 5.18. Log has some errors. It looks like it can't find the certs (.pem file). Did that move? It points to a directory with the .csr and .crt files of our SSL certificate. I do not see where the .pem file was on the old dev version.

I am changing to point to the cacert directory. Is this the right thing to do?

Are there some setup instructions somewhere?

yktdan’s picture

yktdan commented

Making some progress. It now seems to like the path to the .pem. The path is
site/all/modules/uc_payflowpro/cacert/cacert.pem
I now get error 1 "User authentication failed" for all transactions.

It looks like the upgrade process wiped out the password (which doesn't show a **** so you don't know if it is set.

Back working again.

kwinters’s picture

kwinters commented
Priority: Critical » Normal

The cacert does work a little differently now, and there are instructions for moving the file outside of web-accessible folders and configuring the location setting in the README. The PEM file is a list of public keys for signing authorities and such, but I think you may have pointed it at your key.

The password field is somewhat unfriendly, and in general fields like that should do nothing (retain old value) if nothing is entered, but I'm not sure what the correct way to go about it is in FAPI.

Did you have any test transactions with special characters in the product name, etc. go through?

yktdan’s picture

yktdan commented

Yes it was pointed at my key for the old code. We did have a product with & in its title but had fixed that by changing to "and" in the old code. However more recently we had someone enter John & Mary in the name field and their transaction failed. Since we have no control over what they type it was important to upgrade to the newer code.

My guess is that with the current code you have to retype the password any time you save new settings, e.g. change the cacert path. That seems like a bug. It would be better if it showed a bunch of ***s to indicate that it is set.

Thanks for the help.

kwinters’s picture

kwinters commented
Status: Needs review » Fixed

No problem.

I opened a separate issue for the password field, and since the original issue seems to be fine now I'm going to mark it fixed.

Status: Fixed » Closed (fixed)

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