after #1798296: Integrate CSRF link token directly into routing system we need to convert those.
following the shortcut example in the patch on this issue, we should do the same for

grep -r -E "1798296|755584" core/
core/modules/comment/lib/Drupal/comment/Controller/CommentController.php: // https://drupal.org/node/1798296.
core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.php: // http://drupal.org/node/1798296.
core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php: // generation. Add token support to routing: http://drupal.org/node/755584.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ParisLiakos’s picture

Issue tags: +WSCCI, +WSCCI novice

forgot the tags

damiankloip’s picture

Issue summary: View changes
Status: Postponed » Needs review
Related issues: +#1798296: Integrate CSRF link token directly into routing system
FileSize
17.9 KB

Here is a start on this. To convert Views UI ajax stuff too, I think we need to sort out how entity operation links work, as we are currently appending $op to the entity uri.. I have overridden the links build array in ViewListController here.

Status: Needs review » Needs work

The last submitted patch, 2: 2109303.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
22.25 KB
5.26 KB

This should fix those up.

damiankloip’s picture

damiankloip’s picture

Status: Postponed » Needs review
Issue tags: -WSCCI novice
FileSize
21.77 KB
1.28 KB

That issue is fixed now. Let's make those changes and see what happens.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

I really like how much code this patch removes, and how many dependencies it eliminates. It means we did it right. :-)

The last submitted patch, 6: 2109303-6.patch, failed testing.

damiankloip’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
23.02 KB
2.29 KB

Sorry, that will fail. Needs a couple of fixes that I had in my branch that then didn't include.

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

damiankloip’s picture

FileSize
24.52 KB
1.49 KB

And fix those tests to reflect the new fix.

Crell’s picture

Hm. I'm not sure about those path changes. We should be migrating things more toward always using the leading slash, not away. Why is that necessary?

damiankloip’s picture

Pretty much because of _system_path on the request.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Ugh. We've really got to fix that at some point.

damiankloip’s picture

Title: Use the CSRF in the routing system » Convert CSRF checks in controllers to the routing system
damiankloip’s picture

11: 2109303-11.patch queued for re-testing.

The last submitted patch, 11: 2109303-11.patch, failed testing.

damiankloip’s picture

FileSize
17.72 KB

rerolled. Patch is much smaller as overlay changes are not needed anymore!

damiankloip’s picture

Status: Reviewed & tested by the community » Needs review

Status: Needs review » Needs work

The last submitted patch, 18: 2109303-18.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review

18: 2109303-18.patch queued for re-testing.

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

Back to rtbc. That was just a reroll.

webchick’s picture

Title: Convert CSRF checks in controllers to the routing system » [Change notice+docs] Convert CSRF checks in controllers to the routing system
Priority: Normal » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record, +Needs documentation

Wow, this is really great clean-up!

+++ b/core/modules/aggregator/aggregator.routing.yml
@@ -61,6 +61,7 @@ aggregator.feed_refresh:
     _title: 'Update items'
   requirements:
     _permission: 'administer news feeds'
+    _csrf_token: 'TRUE'
 
 aggregator.opml_add:
   path: '/admin/config/services/aggregator/add/opml'

While this is undoubtedly better DX than all of the manual PHP checking that this patch removes, it also seems like it's super easy to miss adding this parameter to a route, which could lead to security issues. :(

OTOH, I can't really spot anything in the routing.yml declaration that would allow automated application of this property. We should make sure to document when to use it and why in the routing system docs, in addition to adding a change notice for this.

Committed and pushed to 8.x. Thanks!

webchick’s picture

Title: [Change notice+docs] Convert CSRF checks in controllers to the routing system » Convert CSRF checks in controllers to the routing system
Priority: Major » Normal
Status: Active » Fixed
Issue tags: -Needs change record, -Needs documentation

Ah, nevermind. Looks like CN/docs are handled over at #1798296: Integrate CSRF link token directly into routing system instead.

Crell’s picture

webchick: I don't think this makes it any easier or harder to know "I SHOULD use a CSRF token here". It just makes the step of actually doing so once you know you should about 10x easier, especially since you don't need to remember to do so in every place you have a link. So it's still a big win, although not the win you're talking about. (Which would be a win if we knew how to make that more obvious somehow, but that's a separate issue.)

Status: Fixed » Closed (fixed)

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