Problem/Motivation
There is no validation of logout requests, so users can be unknowingly logged out, by clicking on a misleading link or (as in OP) if there is an image on the page with the logout path as the source (<img src="/user/logout">
)
We should add a method to validate that logout requests are legitimate, or show a confirmation page to the user to ensure they know they are logging out.
Steps to reproduce
- Log in to a Drupal site
- Create a page with
<img srg="/user/logout">
in the markup - Visit the page, see you are logged out
Proposed resolution
The current MR adds a token as a query parameter to programatic logout links, e.g. those created by Drupal in the menus. If the link is to /user/logout
without a token, the user sees a confirmation page:
Remaining tasks
- Fix failing tests
- Review
- Commit!
User interface changes
Adds confirmation page to the logout process if the link does not contain a token, as shown above.
API changes
Adds a new route option _csrf_token_or_confirm
with a token for programatic links to logout.
Data model changes
N/A
Release notes snippet
?
Original report by XANO
One of the users on my site posted the following code yesterday: <img src="/logout">
. This causes every user to log out when visiting the page that code is on. He suggested making a special page at that address that uses a form to log out.
While we're at that, a system similar to that at Tweakers.net might be made. There you can select the session you want to log out. In that way you can log out the session you started on work bur forgot to end and you can do it from your home computer.
Comment | File | Size | Author |
---|---|---|---|
#176 | Screenshot 2024-03-26 at 4.47.30 pm.png | 71.42 KB | pameeela |
Issue fork drupal-144538
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
XanoJust curious: has this already been fixed?
Comment #2
drummThe security team has decided to re-open this as a public issue. It is well-known and a good solution requires community input.
Please do report any other security issues to security@drupal.org and not the public issue queue.
Comment #3
drummWe can't add a token, many themes that expect the path as-is. Detecting if the request was a forgery is impossible. Specifically filtering for that tag with that attribute might be doable.
I guess this explains how img tags can be used maliciously. Users posting images can be useful, I would like to see a solution implemented if something is possible.
Comment #4
XanoHow do you suggest we filter this? HTML is themable and users may create exactly the same links in their posts as the official logout link.
Comment #5
alexanderpas CreditAttribution: alexanderpas commentedif we implement tweakers.net solution, we certainly need usability testing for it!
however, a simpler might be a form on /logout, just like we handle delete actions.
also we can catch the logout link on other pages with JS to add a (session-specific) token so it can be handled automatically (can be combined with previous option.)
Comment #6
wretched sinner - saved by grace CreditAttribution: wretched sinner - saved by grace commentedWould this be a good use of [#3374646] for D7, if we put the confirmation page in place - it would then degrade to the current delete style confirmation page for non-JS browsers and D6 (and D5 I guess?)
It will need to have a graceful degredation, otherwise the issue will still be there for non-JS browsers.
Comment #7
alexanderpas CreditAttribution: alexanderpas commentedindeed a good place to implement #374646: Popbox (Popups Lite): Adding Modal Dialogs to Confirmations
Comment #8
sunThe filter system should prevent this.
Comment #9
XanoHow? There's nothing wrong with an element pointing to /logout, as long as the page isn't requested without the user wanting it.
Comment #10
alexanderpas CreditAttribution: alexanderpas commentedfilter is not a solution, as this does not protect against malicious external websites
for example
<img src="http://drupal.org/logout" />
on http://buytaert.net/ ( fictional!!! )Comment #11
sunGood point. I agree that the filter system can't handle this.
So, the usual thing to do in cases like this is to ask: How do others do it?
Looking at some other system, I found an approach that really is bad-ass simple:
Just append a query string to the logout link, a md5 hash of $user->login (timestamp of last login). So the menu callback of user/logout simply validates that query string before proceeding.
If we really want or have to aid in theming of custom logout links, we can simply use and add a helper function
user_logout_query_string()
, which only builds the query string bit that can be used both inuser_translated_menu_link_alter()
as well as in custom themes:Comment #12
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet's try that. Dead simple.
Comment #14
Damien Tournoud CreditAttribution: Damien Tournoud commentedDear DamZ, at least run the code you wrote before throwing it at my face next time.
- The test bot
Comment #15
sunAwesome! Even simpler than I envisioned :)
I guess that
router_path
should use%token
as well?Comment #16
Damien Tournoud CreditAttribution: Damien Tournoud commentedNope, paths are stored in their simple forms (% instead of %token) in the database. The loader functions themselves are in
menu_router.load_functions
.Comment #17
sunI think this is ready to go...
Only remaining idea: Would a test to ensure that user/logout returns a 403 make sense? We would be testing the menu system actually... Not sure.
Comment #18
Damien Tournoud CreditAttribution: Damien Tournoud commentedWell, it wasn't working, because the load function was missing from the previous patch. Added a very simple login / logout test (this is very well tested already by DrupalWebTestCase).
Comment #20
Damien Tournoud CreditAttribution: Damien Tournoud commentedErm. This looks like a bug in the menu module:
menu_reset_item() is calling menu_get_item() with a $path like 'node/%', where menu_get_item() is expected a non-replaced path, like 'node/123'.
I'm not sure how to fix that.
Comment #21
andypostI use following in my own module:
Comment #22
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis requires #204077: Allow menu links pointing to dynamic paths.
Comment #23
grendzy CreditAttribution: grendzy commentedsubscribing
Comment #24
hunmonk CreditAttribution: hunmonk commentedposting this here for reference -- perhaps somebody could turn Damien's patch into a small module as a workaround for now?
#620280-1: Protect the logout link against CSRF attacks
Comment #25
moshe weitzman CreditAttribution: moshe weitzman commentedComment #26
Damien Tournoud CreditAttribution: Damien Tournoud commentedReroll.
Comment #27
sunInteresting. This patch contains at least 50% of what would be required for #755584: Built-in support for csrf tokens in links and menu router
Comment #29
Damien Tournoud CreditAttribution: Damien Tournoud commentedWe now need to store the state of the browser (session ID, cookies, isLoggedIn) when switching back from one session to the other. Fixed.
Comment #30
andypostGreat fix!
Interesting, how does it works before without notices ($form_item is undefined)
Another file that makes simpletest getting slower
Suppose drupalLogin() should be drupalLogout()
Comment #31
Damien Tournoud CreditAttribution: Damien Tournoud commentedI was neither working nor tested before.
Not measurable. And this only concerns the session tests.
D'oh. Rerolled for that.
Comment #32
andypostA patch is good to go, also tests are fixed.
this change should be documented at http://drupal.org/node/719612
Comment #33
grendzy CreditAttribution: grendzy commentedshouldn't the calls to drupal_get_token() pass a value (a fixed string like 'user/logout' would be fine), to prevent a sort of replay attack? (e.g. if a hypothetical contrib module used drupal_get_token() with no value to authenticate a different action, a user might be able to forge the token by copying the argument from the logout link).
Comment #34
meba CreditAttribution: meba commented@grendzy good catch
Somebody suggested a logout function to help users creating the link. Shouldn't we create it?
Comment #35
kwinters CreditAttribution: kwinters commentedIs this going to have an impact on menu block caching? It seems like blocks that once cached per role (anon vs not) now must cache per uid AND the cache has to be cleared every time their last login time changes.
Comment #36
grendzy CreditAttribution: grendzy commentedAccording to http://api.drupal.org/api/function/menu_block_info/7 , menu blocks are not cached.
Comment #37
kwinters CreditAttribution: kwinters commentedWell, it also says that "menu.inc manages its own caching." So, there is still the question of whether the menu caching system will handle it appropriately.
Custom blocks that don't use the menu module directly, but still want to link to to the logout function still seem like they're going to be affected. Or for that matter, it will no longer be possible to embed a logout link into a node body (like a page explaining to users how to manage their accounts or security or something along those lines).
If the logout function worked automatically with the token and displayed a delete-form-like confirm if the token was missing or incorrect, that would be acceptable. It may have some caching effects but they'd be greatly mitigated (the cost of an incorrectly cached link is very low then).
Comment #38
jhodgdonRemoving the Needs Documentation tag, as I think this patch didn't go in and it's clogging the Needs Doc issue queue. Does it need to be moved to D8 at this point?
Comment #39
deviantintegral CreditAttribution: deviantintegral commentedHere's a rerolled patch against 8.x. The patch seems to work fine with manual testing, but I'm running into an issue with the test. For some reason, the "Log out" link doesn't render in the simpletest environment. The token load function doesn't even get called in in the test. Any ideas?
This patch also adds 'user/logout' as a token value as suggested in #33.
Comment #40
deviantintegral CreditAttribution: deviantintegral commentedSetting to needs review so others can see the test failure.
Comment #42
sunThis issue is kind of a duplicate of #1344078: Local image input filter in core now, which attacks the original cause instead (malicious user posting HTML containing arbitrary references). It's the same code that runs on drupal.org today.
I'm not sure whether adding and requiring a token for user/logout, as proposed here, is a sensible answer and solution, and whether this issue shouldn't be marked as duplicate.
Comment #43
kwinters CreditAttribution: kwinters commentedNo such luck, Sun. This issue is for a CSRF exploit, and the other issue can't prevent posting of said image tag on random forums, etc.
Comment #44
andypostMaybe better to postpone this as pointed in #22 for #204077: Allow menu links pointing to dynamic paths
Comment #45
gregglesThe title described one potential way to exploit the issue rather than the fundamental nature (which probably caused the confusion in 42/43).
Better title.
Comment #46
XanoI believe this has been fixed in Drupal 7 by adding a random hash path component to the logout URL.
Comment #47
webchickNope. /user/logout still logs you out.
Comment #48
Abhishek Verma CreditAttribution: Abhishek Verma commented#39: 144538.39-harden-logout-link.patch queued for re-testing.
Comment #50
chx CreditAttribution: chx commentedActually, while the CSRF is valid, the local image filter does protect because it checks whether the image source is a valid image :
Comment #51
chx CreditAttribution: chx commentedOf course there can be many other ways to exploit this but -- this is more a nuisance than anyhing else.
Comment #52
Fabianx CreditAttribution: Fabianx commentedNeeds work:
To fallback it should be:
user/logout => Page which says: Do you really want to logout? Click here to logout, such the image is harmless
user/logout/hash => Logout directly
That way it is backwards compatible and links to /user/logout still work and then themes can update.
The extra step does not hurt.
Comment #53
webchickThat could be a security hole in the other direction, though... someone clicks "log out" before leaving a public computer, doesn't notice the confirm page, since you typically don't get one elsewhere on the web. H4X0R3D.
Comment #54
larowlanTagging, I recall seeing auto CSRF protection discussions in/around router patch.
Comment #55
gregglesI think the discussion with CSRF and WSCCI was "this will need a re-roll post the router patch" so wait for that.
Comment #56
Crell CreditAttribution: Crell commentedWSCCI is looking at a CSRF token integrated into routes: #1798296: Integrate CSRF link token directly into routing system
Otherwise this issue is entirely new to me. :-)
Comment #57
b8x CreditAttribution: b8x commentedso what?
problem still exist, you can put in code of any site :
<img src="https://drupal.org/logout" border="0" alt="" />
and it will log out you from drupal.org. all i want to know is how i can disable or modify reactions on "user/logout" path? i can simply use token but i don't know where to put this code to preprocess logout. or i can create my own logout page, but how to disable core "user/logout" path?
Comment #58
yannickooYou could download the Menu token protect (CSRF protection) module :/
Comment #59
b8x CreditAttribution: b8x commentedNow I comment out the code in user.module:
and created my own log out page. my project already has 100 contrib. modules, and increasing its number is last i want.
Comment #60
Anonymous (not verified) CreditAttribution: Anonymous commentedBeyond the potential for CSRF attacks, we should still consider whether we want to switch to using a POST request for logout instead of GET.
http://stackoverflow.com/questions/3521290/logout-get-or-post
(Not sure if this is the best issue for addressing this, but wanted to make a quick note).
Comment #61
damiankloip CreditAttribution: damiankloip commentedSo, now we have csrf integrated into the routing system. We can do something just like this. I guess alot of tests will break with this. Should work in a normal site env though, with a token being added to the logout link (in the toolbar user section in this patch, as a demo).
Comment #62
damiankloip CreditAttribution: damiankloip commentedComment #64
XanoComment #66
damiankloip CreditAttribution: damiankloip commentedThis interdiff confuses me.. :)
Comment #67
Xano64: drupal_144538_64.patch queued for re-testing.
Comment #69
dawehnerSo I guess this will not longer work?
Comment #70
dibyadel CreditAttribution: dibyadel commentedI corrected all files as above and it worked well for my D 7.22 site. Thanks a lot.
d dt
Comment #71
regilero CreditAttribution: regilero commentedA good way to fix that is to require a POST usage. On the client side you can capture the click on logout buttons (maybe with a central "a.drupal-logout" javascript behavior) and transform it in POST requests via jQuery.
For user not having js activated, so still using a GET, or modules not implementing this class on the logout link, a confirmation form should be presented, with a POST confirmation.
This would prevent nasty images and will be transparent for most users.
Comment #72
gregglesI don't think POST alone completely solves the problem unless the post includes a csrf-token. 3rd party sites could still have javascript that executes the POST and logs someone out.
Comment #73
Crell CreditAttribution: Crell commentedYes, the POST would need a token. The same logic applies to any link-action we have in Drupal: We should set a CSRF token in a meta or header or JS setting or something; then the link itself goes to a confirmation form. Then JS can, if enabled, mutate that link to just send a POST request itself, using the provided token, sans confirmation form.
That is probably worth a standard operation in Drupal that we can then leverage for logout and various other places (eg, flag module's various flag links, etc.)
Comment #76
XanoRe-roll.
Comment #78
damiankloip CreditAttribution: damiankloip commentedSo I think we may need to add a drupalGetRoute() or something that we can use to generate the user logout link in this case. If we use the path we will not get outbound url processing.
A couple of other fixes too, like the user edit route name.
So I think now we might just have a problem with the token seeds or something?
Comment #82
XanoRe-roll.
Comment #84
XanoWe've run into the old problem of session synchronization. After the user is logged in, the seeds used for CSRF token generation are different within the environment that runs the tests and the site under test. As such a token generated in a test can never be validated by the site under test.
Comment #85
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedNot sure that test addition makes sense - or you want to avoid breaking drupalLogout()?
Comment #87
klausi#2403307: RPC endpoints for user authentication: log in, check login status, log out introduced another instance of a CSRF vulnerable logout route.
Comment #88
Wim LeersI don't understand why this is not major.
Comment #89
klausiBecause the attack does not have severe consequences. The user gets logged out, which is just an annoyance.
Comment #90
Wim LeersBut I could perform that very attack on this very issue… which would literally mean nobody would be able to read & comment on this issue (unless they manually construct that URL).
/me is now tempted :P
Comment #92
g.oechsler CreditAttribution: g.oechsler at Wunder commentedReroll against 8.3.x
Comment #93
Wim LeersComment #94
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented.
Comment #96
klausiHa, so this fails because we are running into our own CSRF protection, which is good :)
The point is that a logout link can only be used if it has been rendered on a page with the CSRF token. But we don't have a dedicated page in Drupal where we can guarantee that a logout link will even be shown. Logout links are in menus, and the menu might not even be shown.
I propose that we show a confirm form on /user/logout when the CSRF token is not present. This would mean that we cannot use the automatic _csrf_token of the routing system but we would have a better experience for existing sites which render /user/logout links wrongly without token. That way users don't see a 403 access denied page when they click such an old wrong link that is printed somewhere.
And this solution helps us with automated testing: WebTestBase::drupalLogout() can always work by just going to this confirm form and clicking the button. We do not rely on any logout links in menus to be present, yay!
Then we just need one explicit test which has the correct CSRF logout link in some menu. There we can test that logging out directly from a menu link also works.
What do you think about that approach?
Comment #97
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#96+1 to that, I think that was even proposed before in some issue somewhere.
So, yes +10 to showing a form in case no CSRF token is present.
That is a good solution :).
Comment #98
tuutti CreditAttribution: tuutti as a volunteer commentedNot sure if there is an easier way to add CSRF token to the route, but here's an initial patch.
Comment #100
tuutti CreditAttribution: tuutti as a volunteer commentedLets try again.
Comment #102
tuutti CreditAttribution: tuutti as a volunteer commentedComment #103
tuutti CreditAttribution: tuutti as a volunteer commentedComment #104
klausiOverall makes sense to me.
Hm, _csrf_token_optional is a route requirement? That does not seem right. The user logout route does not strictly require a CSRF token, because it falls back to a confirm form. Can it be a route option instead?
What would be a good name for a route option? Stay with _csrf_token_optional? Don't have a better idea yet.
Why do we even need _csrf_token_optional? We could also fix our cache contexts in user_toolbar() and LoginLogoutMenuLink.php, right? We might have to duplicate the renderPlaceholderCsrfToken logic from RouteProcessorCsrf, so not sure if that approach would be better. At least we would avoid introducing a new _csrf_token_optional routing concept with this patch ...
use ::class syntax instead of putting the class name into a string.
That message is not useful to end users. Maybe we don't need a message at all?
I think we should also return the confirm form in this case.
getDescription() should always return a string, so the empty string in this case.
the comment should say why we have to enable Bartik. Because of the menu with logout link or something else? Shouldn't that also work with the default stark theme?
Instead of \Drupal we should use $this-get() instead.
Never use t() in tests, we are not testing the translation system here.
"Make sure the user gets logged out."
why do we need to specify the route name AND the url here? That confuses me. Shouldn't the route name be enough? Why do we need both?
never use t() in tests.
Comment #105
tuutti CreditAttribution: tuutti as a volunteer commentedI left _csrf_token_optional unchanged (for now) and fixed other things you pointed out.
It was an easiest way I could think of how to add a CSRF token parameter to the user.logout route.
Comment #106
tuutti CreditAttribution: tuutti as a volunteer commentedComment #107
klausiComment #108
klausiHm, I think the button label should be "Log out" not "Confirm" to make the action clear.
Comment #109
tuutti CreditAttribution: tuutti as a volunteer commentedComment #110
klausiThanks a lot! The patch looks ready to me now.
I looked a bit into avoiding _csrf_token_optional in the routing processor, but it just would make this more complicated and we could not leverage the code that is already there.
We should probably add test coverage to RouteProcessorCsrfTest for _csrf_token_optional.
And we also need a change record where we document the changes to user logout behavior and what people need to change in their code (for example if they hard coded /user/logout in a template what they need to do instead).
It looks like we have no special places in core where we document _csrf_token, so I guess it is fine if we don't document _csrf_token_optional in the source code? We can later add it to https://www.drupal.org/docs/8/api/routing-system/access-checking-on-routes for example.
I'm +1 to this. Before we put more work into developing this further somebody other than me should probably as well confirm that this is the way we want to go.
Comment #111
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedLooks indeed really good. I also think that csrf_token_optional is not really yet describing what it does. I am giving a +1, but leaving for someone else to RTBC it.
Comment #112
Crell CreditAttribution: Crell at Platform.sh commentedcsrf_token_passthrough? (Ie, it passes through to a confirmation form.)
In hindsight, we should have made ALL CSRF-ish-links do that; go to a confirm form, and then JS-enhance with a token to turn into a direct link. Notes to self for Drupal 9. :-)
Comment #113
dawehnerWhat about making it explicit:
_csrf_token_or_confirm
Comment #114
tuutti CreditAttribution: tuutti as a volunteer commentedComment #115
dawehnerThank you @tuutti !
We certainly still needs the change record though
Comment #116
klausi_csrf_token_or_confirm is a bit better than _csrf_token_optional, yay!
As I said this should not be a route requirement. It should be a route option instead because the routing system does never block access with this.
Comment #117
tuutti CreditAttribution: tuutti as a volunteer commentedComment #118
tuutti CreditAttribution: tuutti as a volunteer and at KWD Digital commentedSetting back to 'Needs work' due to #115.
Comment #119
tuutti CreditAttribution: tuutti as a volunteer and at KWD Digital commentedComment #120
klausiAdded a change record draft. I didn't mention _csrf_token_or_confirm, not sure if we want to say something about that?
Comment #121
dawehnerIMHO we should educate people about the new feature. Maybe having a second change record could be useful?
Comment #122
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedAre we sure we want _requirement vs. _option?
Comment #123
klausiI would say it should be a route option, because you really have to do the token validation yourself in the route callback. A route requirement usually implies that some checking will we performed on the incoming request, which is not the case with our _csrf_token_or_confirm.
The _csrf_token_or_confirm route option only makes sure that a CSRF token is added when the URL is generated outbound and the appropriate cache contexts are set.
Comment #125
klausiThis issue is still ready to be reviewed, please check the latest patch and change notice proposal :)
Comment #126
larowlanWorks well, screenshots of manual testing
Showing token in logout link URI
Showing confirm form with a made up token
Comment #127
pwolanin CreditAttribution: pwolanin as a volunteer and commentedDon't we have a better way of verifying the user is logged out - like the session cookie gets deleted?
Comment #128
klausi@pwolanin: that is a good question! From the philosophy of functional/system level tests we are doing here ideally you should only test the behavior that is observable by the user. A session cookie is a technical implementation detail the user does not care about. The user expects to see a login form element again after they have logged out and that is exactly what we are testing here. So I would argue that assertion is perfectly fine and in line with testing best practices.
Comment #129
pwolanin CreditAttribution: pwolanin as a volunteer and commentedOk, I see the point of testing it from the standpoint of user-observable behaviors.
Hopefully we will add to the doc on d.o to warn people away from this route option unless they know what they are doing.
Comment #130
tuutti CreditAttribution: tuutti as a volunteer and at KWD Digital commentedRe-rolled the patch against 8.4.x.
Comment #131
klausiSomething went wrong with this comment line?
Comment #132
tuutti CreditAttribution: tuutti as a volunteer and at KWD Digital commentedGood catch! I was wondering why the new patch was bigger than the previous one, but couldn't see any difference.
Comment #133
klausiThanks, looks good!
Comment #135
tuutti CreditAttribution: tuutti as a volunteer and at Druid commentedLooks like a testbot glitch.
https://dispatcher.drupalci.org/job/drupal_patches/9567/console
Comment #136
klausiYep, that was not a test fail but some Jenkins problem.
Comment #137
cilefen CreditAttribution: cilefen commentedI updated credit because a lot of people have contributed to the direction this issue has taken.
Comment #139
tuutti CreditAttribution: tuutti as a volunteer and at Druid commentedRe-rolled the patch.
Comment #141
tuutti CreditAttribution: tuutti as a volunteer and at Druid commentedComment #143
tuutti CreditAttribution: tuutti as a volunteer and at Druid commentedRerolled for 8.5.x
Comment #144
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedEdit
I applied 140 since that it passed Drupal 8.4 tests.
I will try the one that passed 8.5 tests too.
---
Please reroll against 8.4.2 also
git apply 144538-140.patch
error: patch failed: core/tests/Drupal/Tests/BrowserTestBase.php:739
error: core/tests/Drupal/Tests/BrowserTestBase.php: patch does not apply
patch -p1 < 144538-140.patch
patching file core/lib/Drupal/Core/Access/RouteProcessorCsrf.php
patching file core/modules/simpletest/src/WebTestBase.php
Hunk #1 succeeded at 326 (offset -51 lines).
patching file core/modules/toolbar/tests/src/Functional/ToolbarCacheContextsTest.php
Hunk #1 succeeded at 63 (offset -1 lines).
patching file core/modules/user/src/Controller/UserController.php
patching file core/modules/user/src/Form/UserLogoutConfirm.php
patching file core/modules/user/tests/src/Functional/UserLogoutTest.php
patching file core/modules/user/user.routing.yml
patching file core/tests/Drupal/Tests/BrowserTestBase.php
Hunk #1 FAILED at 739.
1 out of 1 hunk FAILED -- saving rejects to file core/tests/Drupal/Tests/BrowserTestBase.php.rej
more core/tests/Drupal/Tests/BrowserTestBase.php.rej
--- core/tests/Drupal/Tests/BrowserTestBase.php
+++ core/tests/Drupal/Tests/BrowserTestBase.php
@@ -739,7 +739,7 @@
// idea being if you were properly logged out you should be seeing a login
// screen.
$assert_session = $this->assertSession();
- $this->drupalGet('user/logout', ['query' => ['destination' => 'user']]);
+ $this->drupalPostForm('user/logout', [], 'Log out', ['query' => ['destination' => 'user']]);
$assert_session->statusCodeEquals(200);
$assert_session->fieldExists('name');
$assert_session->fieldExists('pass');
Comment #145
tuutti CreditAttribution: tuutti as a volunteer and at Druid commented#143 seems to apply against 8.4.x and 8.4.2 as well.
Comment #149
L_VanDamme CreditAttribution: L_VanDamme at Dropsolid commentedRerolled for later versions of 8.6.x
Comment #150
klausiThanks, still looking good!
Comment #151
alexpottThis issue does look very nearly ready.
There's no test coverage that clicking on the button on the confirm form actually logs the user out. That looks like pretty important functionality to test.
We also should have a separate change record for the new route option -
_csrf_token_or_confirm
that describes how it works and should be used.Also the docs in RouteProcessorCsrf feel a bit light.
Comment #152
tuutti CreditAttribution: tuutti as a volunteer and at Druid commentedRe-rolled for 8.8.x.
This should already be covered by drupalLogout(), but I added a test for it to be sure.
Comment #153
mpp CreditAttribution: mpp at AmeXio for District09 commentedGlad to see this is nearly ready!
Shouldn't we inject stringtranslation into UserLogoutConfirm?
Comment #155
TwoDRe-rolled against 8.9.x and used the same pattern for the new injected dependency as was done for
$flood
.Not sure what documentation to expand in RouteProcessorCsrf.
Comment #156
klausiTest coverage looks good to me. Some minor things:
needs to be updated to version 8.9.0
we need to update this comment. "A redirect response if a CSRF token was provided, otherwise a logout confirmation form render array."
Then please write the second change record for the new _csrf_token_or_confirm route option as Alex said.
For the RouteProcessorCsrf docs: We could add to the class "This class will add a token URL query parameter to all routes that have a _csrf_token or _csrf_token_or_confirm requirement set."
We could add to the method before the first if(): "Add a CSRF token URL query parameter to all routes that have a _csrf_token or _csrf_token_or_confirm requirement set."
That is a bit redundant, but does not hurt :)
@alexpott: any more specific documentation you would like to see?
Comment #158
TwoDRe-rolled for 9.1.x and 8.9.x.
I guess we're stuck with the dependency deprecation message until D10 now?
Created a change record draft and added comments similar to those mentioned in #156.
Comment #160
tuutti CreditAttribution: tuutti as a volunteer and at Druid commentedRe-rolled for 9.2.x.
Comment #161
quietone CreditAttribution: quietone as a volunteer commentedThis also needs an issue summary update. If nothing else, it would help if it included the proposed resolution and the remaining tasks.
Comment #162
Chi CreditAttribution: Chi commentedThere is a simple way to mitigate this issue on HTTPS sites.
https://w3c.github.io/webappsec-fetch-metadata
Chrome based browsers already support this.
Eventually we could implement a global access checker to allow any modules to specify fetch metadata requirements for their routes. Something like this.
Comment #164
daften CreditAttribution: daften at Dropsolid commentedRe-rolled the patch for 9.2.6
Comment #169
mxr576Comment #170
karishmaamin CreditAttribution: karishmaamin at Specbee commentedRe-rolled patch against 11.x and Tried to fix custom command failure at #164
Comment #171
smustgrave CreditAttribution: smustgrave at Mobomo commentedMoving to NW for the issue summary update and change record
Did not review.
Comment #172
solideogloria CreditAttribution: solideogloria commentedThe patch should be converted into a merge request.
Comment #173
solideogloria CreditAttribution: solideogloria commentedThe new
UserLogoutConfirm
form callsuser_logout()
, which would need to be changed if/when this issue is committed.Comment #175
solideogloria CreditAttribution: solideogloria commentedThough still NW for the change record and issue summary, please review the changes.
Comment #176
pameeela CreditAttribution: pameeela commentedUpdated issue summary.
In my manual testing, the token links didn't seem to work. Clicking on them just refreshed the page, so I'm not sure whether that needs another look? I didn't debug. The confirmation page works well.
Comment #177
pameeela CreditAttribution: pameeela at Technocrat commentedComment #178
pameeela CreditAttribution: pameeela at Technocrat commentedComment #179
pameeela CreditAttribution: pameeela at Technocrat commentedJust noticed there are already two draft change records:
User logout route is now CSRF protected
New route option for adding CSRF tokens to URLs without enforced validation.
This was suggested in #121 to have one for the new feature and one for the change to logout links.
I have reviewed them both and made minor tweaks to the wording but I think they are all good. So we just need to fix the tests.
Comment #180
pameeela CreditAttribution: pameeela at Technocrat commentedComment #181
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedWorking on the fix for those tests
Comment #182
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedConfirmed - logout links currently do not work, luckily the UserLogoutTest proves this as it fails. Now working on a fix for that.
Also hiding all patch files.
Comment #183
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedFinishing up for the day so will post a summary:
1. Fixed the user/logout callback when there was a valid token
2. Fixed majority of test failures due to an invalid parameter passed to submitForm in drupalLogout
3. Started fixing all the other test failures. There were some strange ones like JsonApiFunctionalTest which randomly tests some auto logout stuff with maintenance mode. Really unsure why this is there but it needed a drupalResetSession. A couple of other spots need this same call to ensure the session is properly cleaned up when things in the background log the user out.
4. Marked the new confirm form as workspace safe - this fixed a few workspace related failures
5. Attempted to fix Nightwatch and Performance tests - neither of which I can successfully run locally (yet) so they may not pass
Not sure what's going on with that unit test though...
Comment #184
catchPerformance tests have a merge conflict so might need a rebase, we added state caching so some of the state queries aren't executed at all any more, which could mean no changes here after all. If you're able to run functional javascript tests, you should be able to run performance tests, but please ping me in slack if they're specifically failing for you.
Comment #185
pameeela CreditAttribution: pameeela at Technocrat commentedRebase is done, let's see how it goes...
Comment #186
smustgrave CreditAttribution: smustgrave at Mobomo commentedI pushed a change to add some typehint returns. If we aren't adding a description I removed that function.
Actually found an issue that return '' doesn't match the docs of the inherited docs.
But still needs fixes for performance in the javascript tests. Will see if anyone else can pick to not lose review ability.
Comment #187
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedLast time I tried running these locally I was getting different counts than in CI, looks like that is no longer the case! :)
Comment #188
smustgrave CreditAttribution: smustgrave at Mobomo commentedFeedback appears to be addressed and all tests passing
Only concern was the deprecation link goes to the link, but based on other conversations that may be okay for new parameters. And a 3rd CR seems a lot.
Comment #189
pameeela CreditAttribution: pameeela at Technocrat commentedComment #190
catchAhh yeah that is fixed for a few weeks now after we separated cache / cache tags etc. out from queries. It was a problem with the chained fast backend sometimes executing a database query and sometimes not (by design for the chained fast backend, not ideal for trying to count queries).
Comment #191
alexpottThe thing that gives me pause about this change is relying on the controller to do the csrf check. It feels like the wrong architecture.
I would if we could change it to work something like this:
And then have event that catches the access denied and changes it to a form. This way the CSRF api works as expected and don't rely on the controller to do the security correctly.
Also added a comment to the MR - not sure why the performance test has a logout removed.
Comment #192
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedYeah it felt a bit wrong to me as well, will look at how involved it would be to refactor with what you've suggested.
Comment #193
acbramley CreditAttribution: acbramley at PreviousNext commentedI think the best approach would be:
1. Change user.logout to have a _csrf_token requirement
2. Add a new
_csrf_confirm_form_route
option, which is detected in a new (or existing) EventSubscriber (that probably lives in a generic place like core/lib/Drupal along side the CsrfAccessCheck)3. Add a user.logout.confirm route in user.routing.yml which is confirm form route. That will have a requirement on _user_is_logged_in only
4. Detect AccessDeniedHttpException and _csrf_confirm_form_route in that and return a RedirectResponse
5. Add a case for user.logout.confirm in user module's AccessDeniedSubscriber so logged out users hitting the confirm form are also redirected to the homepage.
The user.logout route then becomes
We can then revert all the handling for the _csrf_token_or_confirm option.
Comment #194
acbramley CreditAttribution: acbramley at PreviousNext commentedPushed the new approach in #193
Comment #195
smustgrave CreditAttribution: smustgrave at Mobomo commentedFeedback appears to be addressed
Comment #196
alexpottWe need to update rhe CR and I left a minro comment on the MR.
Comment #197
alexpott@acbramley also fantastic work implementing #191
Comment #198
acbramley CreditAttribution: acbramley at PreviousNext commentedCRs updated and nightwatch fix applied.
Comment #199
smustgrave CreditAttribution: smustgrave at Mobomo commentedAppears to be two open threads on the MR.
Will leave in review but CR updates seem fine.
Comment #200
smustgrave CreditAttribution: smustgrave at Mobomo commentedAppears that all threads have been resolved
Saw a javascript error and I can't re-run the tests so I rebased the MR. All green
Comment #201
alexpottUpdated for #3208390: Add an API for allowing modules to mark their forms as workspace-safe
Comment #202
alexpottI think #162/ @Chi's suggestion is pretty interesting. As the standard is still in draft I don't think we can use it yet but I think we should open a follow-up to track it and if it does become a standard that is supported by all the browsers we support then we should use it.
Added issue credits to commenters and MR contributors.
Comment #203
catchhttps://caniuse.com/?search=sec-fetch-dest is pretty good now but not 100%, tagging for follow-up.
Comment #204
alexpottThe three new cache IDs being requested on login are css:stark:enNXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM1, js:stark:en:NXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM11 and js:stark:en:4-hTlRsZH1cF9ra9AHElDWka9bDSVy0q3vkHIjrTKIU11 which seems interesting because standard should not be using stark... but StandardPerformanceTest has
protected $defaultTheme = 'stark';
which is unnecessary and wrong as the theme can be derived from the profile.Anyhow I think we get these extra cache gets because the logout link is placeholdered so bigpipe gets involved.
Comment #205
catchIt's on purpose so that we're testing the modules in the standard profile rather than Olivero, but we should probably have separate stark and Olivero tests or something.
I checked that we weren't somehow generating extra CSS/JS aggregates because of the placeholdering, and we're not, we're running into #3414398: AssetResolver::getJsAssets() and AssetResolver::getCssAssets() can repeatedly try to calculate the same set of assets, specifically bigpipe responses with no assets to add, do an extra cache get each time. We should fix that over there, and that will then bring the numbers back down.
edit: updated that MR #3414398: AssetResolver::getJsAssets() and AssetResolver::getCssAssets() can repeatedly try to calculate the same set of assets. Once this gets in, the test changes on there will need updating (to show a bigger relative improvement).
Comment #206
catchOpened #3442917: Use sec-fetch-dest header for CSRF protection.
Had a look at commit credit, this is a very long issue so apologies if anyone got overlooked etc.
Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!
Comment #208
catch