Home » Forums » Ubercart » Development

Do not store the full credit card number even at checkout?

Submitted by chadcrew on Thu, 03/06/2008 - 23:16
chadcrew
Posts: 84
 Joined: 12/28/2007
Bug FinderGetting busy with the Ubercode.

I was just looking at the way the "Do not store the full credit card number even at checkout" setting functions. It looks like if this is set, the entire uc_payment_credit entry will be deleted after an order is successfully submitted. This seems a little extreme to me - don't most credit card processors require merchants to keep records with this info (albeit masked)?

Should this setting instead just immediately mask out the credit card number and remove the CVV? Thoughts?
permalink #1 Fri, 03/07/2008 - 00:51
torgosPizza

Posts: 1376
 Joined: 08/14/2007
Bug FinderEarly adopter... addicted to alphas.Getting busy with the Ubercode.

Last time I checked this truncates the number to the last 4 digits, unless something has changed.

--

"Pain don't hurt." - Dalton

Mike Nelson's RiffTrax! www.rifftrax.com

permalink #2 Fri, 03/07/2008 - 05:45
chadcrew
Posts: 84
 Joined: 12/28/2007
Bug FinderGetting busy with the Ubercode.

That's what I thought too, until I saw this piece of code in uc_credit_order submit:

if (variable_get('uc_credit_checkout_no_store', FALSE)) {
   db_query("DELETE FROM {uc_payment_credit} WHERE order_id = %d", $arg1->order_id);
}

permalink #3 Fri, 03/07/2008 - 11:46
torgosPizza

Posts: 1376
 Joined: 08/14/2007
Bug FinderEarly adopter... addicted to alphas.Getting busy with the Ubercode.

Agh! You're right - it looks like it's blowing away the whole number now. I checked a recent CC order and there is NO card information stored for the order.

Uberdudes - why the change? It helps to have at least the last 4 digits of the card, I think in most cases that's all we need to issue a chargeback. It's not 100% necessary in our case, but I'm confused as to why this was changed between Beta versions.

--

"Pain don't hurt." - Dalton

Mike Nelson's RiffTrax! www.rifftrax.com

permalink #4 Mon, 03/10/2008 - 13:53
Ryan

Posts: 5617
 Joined: 08/07/2007
AdministratorHead Code Monkey - I eat bugs.

Not entirely sure why the change... maybe b/c I just didn't feel like putting encryption there for that at the time. Let me know what it was before and I can consider changing it back. Sticking out tongue

permalink #5 Thu, 03/13/2008 - 11:15
chadcrew
Posts: 84
 Joined: 12/28/2007
Bug FinderGetting busy with the Ubercode.

Hi Ryan,

I see 2 places where this was changed: the uc_credit_order function mentioned above and the uc_credit_cron function which masks the credit cards after time passes. These used to be:

uc_credit_order:

if (variable_get('uc_credit_checkout_no_store', FALSE)) {
          db_query("UPDATE {uc_payment_credit} SET cc_number = RIGHT(cc_number, 4), "
                  ."cc_cvv = '000' WHERE order_id = %d", $arg1->order_id);
        }

uc_credit_cron:

function uc_credit_cron() {
  // Empty anonymous carts.
  $time = strtotime(variable_get('uc_cart_anon_duration', '4') .' '
                  . variable_get('uc_cart_anon_unit', 'hours') .' ago');
  db_query("DELETE upc.* FROM {uc_orders} AS uo LEFT JOIN {uc_payment_credit} "
          ."AS upc ON uo.order_id = upc.order_id WHERE uo.modified <= %d AND "
          ."uo.order_status = '%s'", $time, uc_order_state_default('in_checkout'));

  // Mask old stored credit card numbers.
  $time = strtotime(variable_get('uc_credit_number_duration', '1') .' '
                  . variable_get('uc_credit_number_unit', 'years') .' ago');
  db_query("UPDATE {uc_orders} AS uo LEFT JOIN {uc_payment_credit} AS upc "
          ."ON uo.order_id = upc.order_id SET upc.cc_number = "
          ."RIGHT(upc.cc_number, 4), upc.cc_cvv = '000' WHERE uo.modified "
          ."<= %d AND uo.order_status = '%s' AND CHAR_LENGTH(upc.cc_number) > 4",
           $time, variable_get('uc_credit_clear_status', uc_order_state_default('completed')));
}

Currently these wouldn't work very well if encryption is turned on (I don't think), but I'm not sure they'd do more harm than deleting everything as it is now. To really make it work right with encryption though, my guess is we'd need to have an new query that updates encrypted masked cc_nums if encryption is turned on. Something like this:

if ($key !== FALSE) {
          $cc_number = substr($cc_number, -4);  //Shorten cc num to masked length
          $crypt = new uc_encryption_class;
          $cc_type = $crypt->encrypt($key, $cc_type, 64);
          $cc_owner = $crypt->encrypt($key, $cc_owner);
          $cc_number = $crypt->encrypt($key, $cc_number, 32);
          $cc_exp_month = $crypt->encrypt($key, $cc_exp_month, 32);
          $cc_exp_year = $crypt->encrypt($key, $cc_exp_year, 32);
          $cc_cvv = '000';  //DON'T STORE CVV
          $cc_bank = $crypt->encrypt($key, $cc_bank);
        }

        db_query("UPDATE {uc_payment_credit} SET cc_type = '%s', cc_owner = '%s', "
                ."cc_number = '%s', cc_exp_month = '%s', cc_exp_year = '%s', "
                ."cc_cvv = '%s', cc_bank = '%s' WHERE order_id = %d", $cc_type,
                 $cc_owner, $cc_number, $cc_exp_month, $cc_exp_year, $cc_cvv,
                 $cc_bank, $arg1->order_id);
        if (db_affected_rows() == 0) {
          db_query("INSERT INTO {uc_payment_credit} (credit_id, order_id, "
                  ."cc_type, cc_owner, cc_number, cc_exp_month, cc_exp_year, "
                  ."cc_cvv, cc_bank) VALUES (%d, %d, '%s', '%s', '%s', '%s', "
                  ."'%s', '%s', '%s')", db_next_id('{uc_payment_credit}_credit_id'),
                   $arg1->order_id, $cc_type, $cc_owner, $cc_number,
                   $cc_exp_month, $cc_exp_year, $cc_cvv, $cc_bank);
        }

...but i haven't tested that at all.

Chad