User logout csrf

The _csrf_token addition closes the main attack vector. The downside is that a missing or expired token now gives a hard 403. This hits bookmarked logout links, expired sessions, or portals linking to /saml/logout. Drupal core solved the same problem for user.logout in #144538: User logout is vulnerable to CSRF with a confirmation form fallback, and since samlauth already requires ^10.3 || ^11, the same approach works here without any compatibility work. Core also switched to _user_is_logged_in: 'TRUE' instead of _access: 'TRUE', so the CSRF check no longer runs for anonymous users.

Happy to review if @alexortega_98 wants to extend the MR. Or I can open one, whichever works. :)

Thank you for the work and review + pointers into existing work.

Agreed with @scontzen / #5, and whoever wants to extend the MR, I'll happily review and merge. I plan to be back on this module + possibly release a new version this week.

Picking this up. I will extend MR !40 following the core pattern.

Extended MR !40 with the confirmation form fallback discussed in #3555406-5: User logout csrf. Follows the core pattern from #144538: User logout is vulnerable to CSRF: authenticated users visiting /saml/logout without a valid CSRF token are redirected to the confirmation form instead of getting a 403. _user_is_logged_in: 'TRUE' replaces _access: 'TRUE', so anonymous users can no longer trigger the logout route at all.

The test asserts 403 for anonymous access to /saml/logout/confirm, matching core's user.logout.confirm behavior (same _user_is_logged_in requirement, no custom redirect in UserLogoutConfirm).

Verified manually on Drupal 11. Setting to Needs review. Thanks to @alexortega_98 for the initial work.

Thanks! While reviewing I learned that (unsurprisingly) Core's CSRF protection is nice functionality, that can be added with minimal code.

I did take the liberty to add some details myself:

• Protection against a fatal "Leaked cacheability metadata" exception. (Just ignore it unless you like rabbit holes, I don't even know if it's still necessary but the rest of the module has it. Someday I'll muster the courage to dive into it again and update #3232577: Code leaked cacheability metadata.)
• An actual test of the logout code -- because I'm very slowly still adding tests where possible. (And then discovered that I can't actually test much of it. Oh well. And yes, I'm messing up the PR with details of cleaning up old tests...)

At this moment I'm just a bit confused about one detail: visiting /user/logout/confirm as a logged-out user does not produce a 403 (despite you @scontzen asserting that it does), but instead a 302 to the front page... on my admittedly very old test site, and according to core's UserLogoutTest (code at bottom). Detail, but we both agree it's good to have the same behavior as Core.

So I'll check tomorrow whether I should reinstall my test site, or check what Core does differently, or ....

You're right, I missed the redirect. Confirmed locally on Drupal 11.3.5: anon GET on /user/logout/confirm returns 302 to <front>, not 403.

The redirect lives in core/modules/user/src/EventSubscriber/AccessDeniedSubscriber.php, in the on403() handler: for anonymous users, it is hardcoded to redirect user.logout and user.logout.confirm to the front page. Not visible in the routing YAML or in UserLogoutConfirm, which is why I missed it. My #8 comment about "no custom redirect in UserLogoutConfirm" was technically right but misleading; the redirect sits one layer further out.

Thanks for catching this, and for the leaked-metadata protection plus the extra tests.

Thanks for checking. Yes that makes sense. (I guess it is somehow reasonable that this redirect is not tied automatically to all CSRF confirm forms...)

Equalized with Core now.

But... I only just realized that this introduces a regression. Which also indicates that I need to add another test for this.

Unlike Core, we can't have _user_is_logged_in: 'TRUE' on /saml/logout. (I am assuming) it is important that the 'logout flow' is not interrupted (i.e. the user is always redirected to the IdP SLO endpoint), so the IdP can do its thing. Also for an un-authenticated user, who might have locally logged out of Drupal by accident.

I have not tested yet whether the _csrf_token: 'TRUE' route requirement works for anonymously-accessible routes; if not, we'll need to pry apart the CSRF logic. EDIT: I think we'll need to add CSRF protection for logged-in users but not for anonymous users.

I consider this a fairly major issue, so I don't want to leave it hanging, but I might check some other issues/MRs first.

Since we hit this snag, I'm merging #3583791: Add option to perform a SAML logout when using Drupal logout '/user/logout' first.

PHPUnit tests for #3583791 still need to be added here, and this issue should also check if the redirected /user/logout keeps working properly. The PR gets ugly, but that's hopefully not an issue if I do it myself soon.

I'm extremely slow but working on things again. Just leaving a note.

The regression that I mentioned, is gone: only authenticated users get a CSRF token + check now.

I am seeing another regression though:

On 8.x-3.x, /saml/logout?destination=EVENTUAL-PATH results in a redirect to the IdP with a RelayState of "https://OURSITE/EVENTUAL-PATH" (which will in theory redirect back to us with that RelayState still intact, and then land the user on that URL.)

This is not working at the moment: /saml/logout?destination=EVENTUAL-PATH just immediately redirects to /EVENTUAL-PATH instead of the logout confirm form. (This is also the case for Core's /user/logout?destination=PATH, which also never ends up on the confirm screen, according to my manual test - and I find no reference in the Core issue besides #144538-69: User logout is vulnerable to CSRF, and the MR does not contain any test for it anymore.)

So, still to do:

• fix destination parameter, because I do not want to introduce that regression
• add PHPUnit tests for #3583791
• (unrelated: fix 8.x-3.x branch for (#3597302: RelayState Retrieval from IdP POST to ACS No Longer Works and add incomplete installation default for #3506668: Field description for value_map does not match actual behavior, before I merge this. And then hope that I will do a better job at keeping functionality clean, again.)

I've improved some things, and arguably wasted time on other things because I was being dumb.

1)

Fixed /saml/logout/confirm?destination=EVENTUAL-PATH, which redirected to EVENTUAL-PATH immediately after submitting the form, instead of to the IdP. (It now redirects to EVENTUAL-PATH after getting back from the IdP.)

Also, made /user/logout redirect properly to the SAML-logout confirm form, if the recently added (#3583791: Add option to perform a SAML logout when using Drupal logout '/user/logout') option is enabled.

2)

Added extra tests. More general testing of the login/logout endpoints, but also added a test successful logout through a /saml/logut?token=VALID link. (Maybe that wasn't necessary before I started writing non-Core code to do that; idk. Also, detail: it's impossible to test this link without a token, so I removed one test that suggested it was, and commented why.)

3)

I was wrong (or changed my mind) about the "another regression" in #13, so skipped 'fixing' it.

• /saml/logout/confirm?destination=EVENTUAL-PATH does end up at EVENTUAL-PATH without redirecting to the form, but
• URL generation code always converts this URL into /saml/logout/confirm?destination=EVENTUAL-PATH?token=...
• so only unprocessed URLs (like those part of a text, me typing directly into a browser address bar while I was testing) are affected
• and Core's /user/logout has the same behavior, and noone seems to have an issue with that.

So I'll keep it like this / won't consider it a regression. (I don't think this is documented anywhere, I couldn't find it mentioned in #144538: User logout is vulnerable to CSRF, so worked it out myself.)

4)

What I said about /saml/logout needing to stay accessible to all users... just isn't true. I think.

/saml/sms needs to stay accessible to all users (and does). Not /saml/logout.

So I think I just wasted a whole lot of time on writing the custom logic that is now behind /saml/logout. I'll keep it, because it keeps us from introducing a behavior change. And I might have a use for it in v4 of the module (not sure yet). But I likely wouldn't have spent this amount of time on it, if I had kept a clear head. And I might remove it again in v4.

--

So... at around comment #11, the code was actually good already, except for the "1)" mentioned above here. But hey, it's done now. With more extensive tests that cover/document the full logout behavior.

Tests are passing, and I'm not going to subject all these changes + various refactorings to someone's review. Merged.

What I said about /saml/logout needing to stay accessible to all users... just isn't true. I think.

Wait, no, it is. I just gave the wrong reason in #11.

After someone inadvertently logs out locally from Drupal, they still need to be able to start a SAML logout in Drupal to cancel the 'logged-in' state on the IdP.

(Maybe this isn't applicable to some systems... and true, we can't send the user's NameID/SessionIndex on to the IdP anymore... but still, I'm sure some organizations will complain if this doesn't work anymore.

So... Now I'm happy again that I invested that extra time.)