admin_toolbar_tools.module is very broken, overly-complicated, and wrong on several levels. My apologies for creating an issue like this, I wouldn't normally, but the non-standard issue queue practices make it very hard to write any patches at all.

CommentFileSizeAuthor
#2 admin_toolbar-rewrite-2537016-2.patch24.77 KBtwistor
#1 admin_toolbar-rewrite-2537016-1.patch22.26 KBtwistor

Comments

twistor’s picture

twistor commented
Status: Active » Needs review
StatusFileSize
new admin_toolbar-rewrite-2537016-1.patch22.26 KB

I've lost track of how many bugs this fixes, but it's a lot.

twistor’s picture

twistor commented
StatusFileSize
new admin_toolbar-rewrite-2537016-2.patch24.77 KB

This fixes overzealous use of csrf tokens, as well as the usless /admin/tools to front page redirect.

twistor’s picture

twistor commented

Trying to enumerate the bug fixes:

  • Lots of coding standards.
  • Unnecessary use of CSRF tokens.
  • Unnecessary redirect to front page.
  • Unnecessary checks for system and user modules.
  • Move entity bundle enumeration to happen in the module check.
  • Remove unnecessary bundle enumeration code.
  • Fix logout link.
  • Fix user role enumeration.
  • Fix menu link namespacing.
  • Removed unnecessary weights.
  • Contact links linking to comment forms.

That's just off the top of my head.

matio89’s picture

matio89 commented
Status: Needs review » Fixed

  • matio89 committed 50b2849 on 8.x-1.x authored by twistor 
    Issue #2537016 by twistor: Rewrite admin_toolbar_tools.module
twistor’s picture

twistor commented
Assigned: twistor » Unassigned

Hey thanks!

I'm curious, why are we still checking for the system and user modules? Is there some odd bug that I'm missing? The system and user modules are required, so they will always be installed.

matio89’s picture

matio89 commented

All the CSRF tokens have been removed, but I do not see anything fixing back #2533442: Flush links vulnerable to CSRF ?

Agree for removing dependance for user & system. I'll do that asap.

  • matio89 committed 85c6207 on 8.x-1.x 
    Issue #2537016 by matio89: Removing dependance for user & system
eme’s picture

eme commented

I think we should bring back csrf_token unless there is a better way to proceed in D8...

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

I still see CSRF token protection with the current code (in admin_toolbar_tools.routing.yml). I don't think anything needs to be addressed.

Status: Fixed » Closed (fixed)

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