#Security improvements

TBH I never understood why Xss is not a service. Almost everything else in Drupal can at least be overridden/extended by a module. I find it a bit "un-Drupalish" that we say Xss::filterAdmin and not \Drupal::service("xss")->filterAdmin(...). This way, other modules like response_image could at least easily extend the list of allowed tags.

As this issue is still stalling, does anyone have a patch for 10.3 at hand, otherwise I can try to prepare one later?

Reroll of #34 against 10.3.x

morgannc → changed the visibility of the branch 3456271-security-flaw-allowing to hidden.

Have not reviewed yet but issue summary appears to be incomplete could that be updated please

Can use https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... → . for help if needed.

@neclimdul re caching see RouteProcessorCsrf::processOutbound() which handles lazy building - so as long as the URL is built using the routing system, that should be fine.

Obviously it won't work if a logout link is sent in an email or something, but that shouldn't happen for logout links really (and if it did, you'd get the form fallback).

Belatedly saving additional credits for folks who helped sort the release note.

Okay great, thanks! Let's mark this issue back to fixed, then.

It already has the "Needs followup" tag, which looks like it was left on by accident when @catch filed a different followup in #206. However, it looks like we have two additional potential followups to file, for #230 and the last paragraph in #23. I'll levae it to the respective contributors to file those followups since they will be best able to articulate the issues. Thanks again!

@xjm with the exception of my concern the link generated from the code in the changelog might store csrf links in caches, yes that addresses it.

@acbramley, sorry, I just realized this was actually my fault. I thought I had specifically stated where the comment had come from but apparently it never made it from my brain to the issue comment. :) My bad!

While we're discussing it, I haven't don't much testing but I think I echo some concerns from 14 years ago about caching. I haven't tested yet but is the CSRF token going to end up cached in templates using the code in the change log? Should core provide a lazy builder?

This is probably worth opening a followup issue. Discussion on this issue should only be about what needs to go in the release note so that it can be marked back to fixed. @neclimdul, does the current release note snippet in the IS give sufficient info for a site owner to address what you encountered? Thanks!

I see how my comment was misunderstood. It was technically in the sites themes but we have several form designs so a new form has to be assigned to a specific look or it just ends up "unthemed". On our site, unthemed was quite ugly.

More my point was more that any form isn't desirable and a fair few years of building Drupal sites has made me lazy about dropping a menu link in a template.

While we're discussing it, I haven't don't much testing but I think I echo some concerns from 14 years ago about caching. I haven't tested yet but is the CSRF token going to end up cached in templates using the code in the change log? Should core provide a lazy builder?

Added test case .

Please review , moved NR

@Prashant.c That issue hasn't been finished yet, so the service doesn't exist.

I have a few points here:

1. Can we update the procedural user_logout() in
   

   core/modules/user/src/Form/UserLogoutConfirm.php 

   with the service from https://www.drupal.org/project/drupal/issues/2012976 📌 Deprecate user_logout() and user_login_finalize() and replace with a service Needs review ?

2. CancelURL is always <front>, Should not this be conditional based on the destination parameter?

Presumably it will have whatever styles apply to other forms though, and if you have users logging in I'd expect your login form to have some styles. I see the snippet is already updated to remove the part about the site's theme. I do think the rest can go in a follow up but just wondering about why a theme would have no styles or why this would look particularly ugly when it's just a heading and two buttons.

again, I linked the issue where the comment was posted above.

Sorry, I saw you link the issue but didn't (and still don't) see that was where the comments were coming from. Should've checked it I guess.

I think the issue is that not all front end themes, particularly custom themes, will render the confirmation form nicely. Previously a link to /user/logout would log the user out immediately, whereas now it shows a confirmation form. So custom themes should follow the steps in the change record → to avoid this.

Link to the comment 📌 10.3.0 release notes Active

Not much information to go on.

I asked @neclimdul to comment here so that he can speak for himself.

Meanwhile, fixed the link text accessibility issue.

@acbramley, again, I linked the issue where the comment was posted above.

Re #218 where are these comments coming from? The form is definitely themed as a frontend page (i.e the site's themed). Reworded the release note to remove references to this and provide better guidance. I was trying to keep the note brief as more information is available in the CR (which I linked to).

"a pretty ugly unthemed confirmation form"

It looked OK to me in testing but that was using Olivero (see screenshot in IS). I'm not sure why it would not appear in the site's theme? I think a follow up would be good with some additional details and what the proposed fixes would be to address the concern. But it would definitely be good to clarify for the release note as I think the current text is not very clear.

Thanks!

One improvement that we could probably add is explaining who is affected, and how. For example, one audience that is affected is site users, who will encounter a behavior change. I added a bit about that to the release note.

@neclimdul also described it as "a pretty ugly unthemed confirmation form", so I added a bit about that to the release note too. I wonder if there is something we could do to mitigate the theming issue? Separate possible followup, I guess.

Could someone check my modified release note above and ensure it's accurate? Meanwhile, I'm adding it to the 10.3.0 draft. Thanks!

Thanks Adam! This looks good to me.

Instructions for writing release notes: https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... →

Adding the tag as well.

Per @neclimdul, this is potentially disruptive and merits a release note. Thanks!