In the current 7.x version, we have a CSRF protection added using drupal_get/valid_token functions. This prevents someone from tricking an administrator from enabling or disabling a country supported in uc_address module.

The attached patch sets _csrf_token as a requirement, making the links carry a token, and the page callback checks for that key.

CommentFileSizeAuthor
#18 2671048-18.patch5.63 KBAyesh
#18 interdiff-2671048-14-18.txt1.38 KBAyesh
#14 interdiff-2671048.txt1.95 KBlongwave
#14 2671048-csrf.patch4.81 KBlongwave
#9 uc_address_add_csrf_protection_to-2671048-8.patch4.08 KBAyesh
#5 uc_address_add_csrf_protection_to-2671048-5.patch1.43 KBAyesh
uc_address_csrf.patch599 bytesAyesh
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Ayesh created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
Status: Active » Needs review

It looks like this should also be added to other enable/disable links, such as the ones in uc_payment.

TR’s picture

TR CreditAttribution: TR commented
Title: Add CSRF protection to UC Address action links » Add CSRF protection to country action links
Category: Bug report » Task
TR’s picture

TR CreditAttribution: TR commented

Yes, it should be added in a lot of places. It's on my to-do list to review all the routes to see which ones need tokens.

Ayesh’s picture

Ayesh CreditAttribution: Ayesh commented
FileSize
uc_address_add_csrf_protection_to-2671048-5.patch1.43 KB

You are right there were some other places that needed a CSRF token as well.
Attached a patch suggestion, but please do focus on other important issues (as you have said it's already in your list).

Thanks.

Ayesh’s picture

Ayesh CreditAttribution: Ayesh commented
Title: Add CSRF protection to country action links » Add CSRF protection to entity and configuration enable, disable and other operation links

The last submitted patch, uc_address_csrf.patch, failed testing.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
Status: Needs review » Needs work

#5 looks like it was intended for #2662956: Do not show a country dropdown when only one is enabled !

Ayesh’s picture

Ayesh CreditAttribution: Ayesh commented
FileSize
uc_address_add_csrf_protection_to-2671048-8.patch4.08 KB

I'm terribly sorry it was a wrong patch, updated tests to use the new CSRF-token URL in the tests.

Ayesh’s picture

Ayesh CreditAttribution: Ayesh commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 9: uc_address_add_csrf_protection_to-2671048-8.patch, failed testing.

TR’s picture

TR CreditAttribution: TR commented

You are right there were some other places that needed a CSRF token as well.
Attached a patch suggestion, but please do focus on other important issues (as you have said it's already in your list).

To be clear, my list says "Check all routes to see if CSRF token is needed". I don't currently have a list of known routes that need to be fixed.

So if you know of any other places, or if you're willing to look and find more of these, your input and patches would be welcome!

TR’s picture

TR CreditAttribution: TR commented
Issue tags: +beta blocker

... let's make sure this gets done for the beta release.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
Status: Needs work » Needs review
FileSize
2671048-csrf.patch4.81 KB
interdiff-2671048.txt1.95 KB

Fixed the test failures. This helper might be useful elsewhere in long tables of identical links!

TR’s picture

TR CreditAttribution: TR commented

Nice helper function. I wish I had that months ago ...

The only comments I have are we don't need this:
+use Drupal\Core\Url;

And the entity links in uc_tax also need the tokens (including the clone op).

Ayesh’s picture

Ayesh CreditAttribution: Ayesh commented

Thanks @longwave! Tests passed!!

You are right @TR we are missing tokens in several of the routes from uc_tax module too. I don't know how I missed it from my last commit. Sorry I thought I had every route covered.

I will try to upload another patch after going the gh each of the routing files and with the new test class method to test them.

TR’s picture

TR CreditAttribution: TR commented

Ayesh, the routes in uc_tax are new, they were added after your original post.

Ayesh’s picture

Ayesh CreditAttribution: Ayesh commented
FileSize
interdiff-2671048-14-18.txt1.38 KB
2671048-18.patch5.63 KB

Trying with the uc_tax routes included (enable, disable and clone links).

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
Status: Needs review » Reviewed & tested by the community

Looks good to me. Will commit it later unless TR beats me to it.

  • longwave committed 0fdc1ac on 8.x-4.x authored by Ayesh 
    Issue #2671048 by Ayesh, longwave: Add CSRF protection to entity and...
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
Status: Reviewed & tested by the community » Fixed

Committed, thanks for working on this. Hopefully we remember to add _csrf_token to any similar links we add in the future.

Status: Fixed » Closed (fixed)

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