[4.3] Use CSRF only for the site's domain

[dgrammatiko]

Pull Request is alternative to #39343

Summary of Changes

• CSRF is always sent with the Joomla.request ONLY for the site's domain
• The CSRF could be manually added for a foreign domain with a custom header headers: {'X-CSRF-Token': Joomla.getOptions('csrf.token', '')}

Testing Instructions

• Go to /administrator/index.php?option=com_config and try to send a test mail, it should work (assuming your test site is properly set the mail server)
• The server should respond with a 200
• the url was modified to remove the form token, to make sure that CSRF was used
• Relative local domain modify the url to:

$ajaxUri = \Joomla\CMS\Uri\Uri::base(true) . '/index.php?option=com_config&task=application.sendtestmail&format=json';

refresh the page and run click on the send test mail again, observe the request (in the browser console) that has the x-csrf

• Absolute local domain modify the url to:

$ajaxUri = \Joomla\CMS\Uri\Uri::base(false) . '/index.php?option=com_config&task=application.sendtestmail&format=json';

refresh the page and run click on the send test mail again, observe the request (in the browser console) that has the x-csrf

• Foreign domain modify the url to:

$ajaxUri = 'https://www.joomla.org/4/administrator/index.php?option=com_config&task=application.sendtestmail&format=json';

refresh the page and run click on the send test mail again, observe the request (in the browser console) that has the x-csrf

Actual result BEFORE applying this Pull Request

CSRF send to all domains

Expected result AFTER applying this Pull Request

CSRF is only for local use

Link to documentations

Please select:

• Documentation link for docs.joomla.org:

• No documentation changes for docs.joomla.org needed

• Pull Request link for manual.joomla.org:

• No documentation changes for manual.joomla.org needed

[dgrammatiko]

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

[dgrammatiko]

@Fedik can you review this one?

[Fedik]

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

[Fedik]

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.

[dgrammatiko]

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?

[Fedik]

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?

[HLeithner]

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

[joomdonation]

I have tested this item ✅ successfully on 5cb5e38

Works for all test cases described in testing instructions.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39580.}

[viocassel]

I have tested this item ✅ successfully on 5cb5e38

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39580.}

[joomdonation]

RTC

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39580.}

[SniperSister]

Makes perfect sense, fine for me!

[obuisard]

Thank you Dimitris @dgrammatiko for this PR!

[wilsonge]

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

[dgrammatiko]

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 🤷‍♂️

[wilsonge]

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)

[dgrammatiko]

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)