The last commit aligns the code with the jQuery proven code: https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#jquery

@Fedik can you review this one?

I think #39343 will be better, because it gives you more controll over the code. (and I would prefer to avoid use of rootFull)

hmhm, or can we combine both PRs? When sendCsrfToken is null then compare domains, when it true/false then ignore domain?
And use location.origin instead of rootFull.
And remove GET|HEAD|OPTIONS testing.

because it gives you more control over the code

Ehm, that PR is not actually solving the leak if the devs won't explicitly use the new option. BTW devs could still push whatever they want with a custom header (it will override as it happens later):

Joomla.request({
  method: 'POST',
  url: 'https://www.joomla.org/administrator/index.php?option=com_config&task=application.sendtestmail&format=json',
  headers: {'X-CSRF-Token': Joomla.getOptions('csrf.token', '')} // Manually force sending the CSRF on a foreign domain NOT SAFE!!!!
});

And remove GET|HEAD|OPTIONS testing.

Done

And use location.origin instead of rootFull.

Done

@Fedik any better now?

Yeap, looks better now, and much more clean :)

document.location.origin

btw, is part of Window object, window.location.origin, or in js-cs window is undefined?

if we get 2 tests we can merge this @SniperSister can you please have a look as jsst ?

I have tested this item ✅ successfully on 5cb5e38

Works for all test cases described in testing instructions.

I have tested this item ✅ successfully on 5cb5e38

RTC

Makes perfect sense, fine for me!

Thank you Dimitris @dgrammatiko for this PR!

See #39969 suspect we might need to head back to the original patch

See #39969 suspect we might need to head back to the original patch

Honestly the project should deprecate the Joomla.request and transition to fetch (without any magic wrapper).

FWIW if the patch doesn't cover all the case then just do complete revert of this ?‍♂️

Honestly the project should deprecate the Joomla.request and transition to fetch (without any magic wrapper).

I mean we'd still need to document all this stuff in CSRF tokens anyhow. Not really sure doing that fixes anything in respect of this (totally understand fetch is better than the xhr stuff we're wrapping and don't dispute moving to that makes a huge amount of sense)

I mean we'd still need to document all this stuff in CSRF tokens anyhow.

Yes, this is crucial to be communicated properly as the leaking of CSRF could easily lead to a complete compromise of the site.

Not really sure doing that fixes anything in respect of this

Actually, this bug reveals one more thing that is done awfully wrong in the XHR wrapper and also in the server side response. The XHR response on either the onSuccess or the onError should should be checked that is ok (ie the request asked for JSON and the response is not) also the response from the server should NOT be a 200 but something like 4XX or 5XX.
What is needed is the complete array of the valid url strings and figure out if we could code the conditional to cover all of them (obviously this PR overlooked the case that the url could be done against the base url).

Anyways what I was actually proposing was the immediate deprecation of the Joomla.request without a replacement and start migrating the ajax requests to fetch. BTW fetch already supports full duplex on Chrome stable (FF, safari probably on their beta but that's not a show stopper)