Home » Forums » Ubercart » Development
5 replies [Last post]
Fri, 03/07/2008 - 00:16
chadcrew
chadcrew's picture
Offline
Bug FinderGetting busy with the Ubercode.
Joined: 12/28/2007
Juice: 195
Was this information Helpful?

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?

Top
Fri, 03/07/2008 - 01:51
#1 View page
torgosPizza
torgosPizza's picture
Offline
Bug FinderEarly adopter... addicted to alphas.Getting busy with the Ubercode.
Joined: 08/14/2007
Juice: 4110
Re: Do not store the full credit card number even at checkout?

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

--
Help directly fund development: Donate via PayPal!

Top
Fri, 03/07/2008 - 06:45
#2 View page
chadcrew
chadcrew's picture
Offline
Bug FinderGetting busy with the Ubercode.
Joined: 12/28/2007
Juice: 195
Re: Re: Do not store the full credit card number even at checkou

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);
}
Top
Fri, 03/07/2008 - 12:46
#3 View page
torgosPizza
torgosPizza's picture
Offline
Bug FinderEarly adopter... addicted to alphas.Getting busy with the Ubercode.
Joined: 08/14/2007
Juice: 4110
Re: Re: Re: Do not store the full credit card number even at che

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.

--
Help directly fund development: Donate via PayPal!

Top
Mon, 03/10/2008 - 14:53
#4 View page
Ryan
Ryan's picture
Offline
Joined: 08/07/2007
Juice: 15438
Re: Re: Re: Re: Do not store the full credit card number even at

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

Top
Thu, 03/13/2008 - 12:15
#5 View page
chadcrew
chadcrew's picture
Offline
Bug FinderGetting busy with the Ubercode.
Joined: 12/28/2007
Juice: 195
Hi Ryan, I see 2 places

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

Top