"Manage payment methods" permissions needs to have a warning for anonymous checkbox warning of the security issues that can result from being checked.

If admins check the permissions box for anonymous users to be able to manage payment methods (which could be checked by someone believing that for anonymous checkouts, this is necessary) results in a page with everyone who has ever checked out anonymously's last four of their credit card numbers and name etc. for all the world to see at www.yoursite.com/user/0/payment-methods

The permissions page interface should have a warning that checkin this box is NOT A GOOD IDEA. Furthermore, it shouldn't even be an option in my opinion.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

matthewmack created an issue. See original summary.

jsacksick’s picture

Sure we can add:

  'restrict access': TRUE

Thought.. It's probably not the right fix as it's a security problem only when granting this to anonymous users...
Perhaps PaymentMethodAccessCheck needs to be updated to grant access, only for authenticated users, regardless of the permission...

From:

    if ($result->isNeutral() && $current_user->id() == $account->id()) {

To :

    if ($result->isNeutral() && $current_user->isAuthenticated() && $current_user->id() == $account->id()) {
jsacksick’s picture

Status: Active » Needs review
FileSize
1.68 KB
2.49 KB

Attaching a failing test + another patch that has the fix + the test.

jsacksick’s picture

Updated test that also ensures that granting the "administer commerce_payment_method" doesn't give access to the page for managing payment methods.

jsacksick’s picture

The last submitted patch, 5: 3218783-5-tests-only.patch, failed testing. View results

jsacksick’s picture

Status: Needs review » Fixed

Committed the fix, thanks for the report.

jsacksick’s picture

Title: last four of their credit card numbers and name etc. for all the world to see » Prevent anonymous users from managing payment methods.

  • jsacksick committed f72daf0 on 8.x-2.x
    Issue #3218783 by jsacksick, matthewmack: Prevent anonymous users from...
matthewmack’s picture

Hey Jack,
Thanks so much for taking time for this.

matthewmack’s picture

Status: Fixed » Closed (fixed)