[#14952] - Add CSRF token for Ajax
- Fixed in Code Base
- 29 Jul 2017
- Medium
- Build: staging
- Easy Test
- # 14952
- Diff
- asika32764:feature-csrf
User tests: Successful: Unsuccessful:
Summary of Changes
An idea inspired by Laravel when I'm writing drag and drop installation in #14924
Add CSRF token to head as a meta tag, so we can get the value by Javascript.
Use JHtml::_('form.csrf') to add <meta name="csrf-token" content="14044d42e0488e8362c2ee40f3dcf2d1" /> in <head>
Then we can select this tag to get value:
var token = jQuery('meta[name=csrf-token]').val();Also we can use X-Csrf-Token: xxx header to validate token.
JHtml_('jquery.token') help us add header to ajaxSetup so we can auto inject token in every $.ajax() calls.
;(function ($) {
$.ajaxSetup({
headers: {
'X-Csrf-Token': $('meta[name="csrf-token"]').attr('content')
}
});
})(jQuery);Testing Instructions
I rewrite sendmail test in configuration to use this pattern, just send a test.
If we remove JHtml::_('jquery.token') in administrator/components/com_config/view/application/tmpl/default_mail.php, will get fail result.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Administration com_config Libraries Unit Tests |
| Labels |
Added:
?
?
|
||
| Category | Administration com_config Libraries Unit Tests | ⇒ | Administration com_config Templates (admin) Libraries Unit Tests |
OK, but what class and method should we locate the php code? JHtmlBehavior ???
JHtmlForm::csrf()is fine, you already oded that part
I mean, the core.js is included by JHtmlBehavior::core() but it has no not added CSRF token yet.
Should I call JHtmlForm::csrf() also in JHtmlBehavior::core() or add a new method JHtmlBehavior::token() similar as JHtmlJquery::token() which I wrote in this PR?
Personally I would extend the JHtmlBehavior::core() so everything comes up magically...
OK. I just think there may be someone want to use core.js but do not hope CSRF auto inject to <head>.
But I will follow your comment first if there has no opinion from others.
| Category | Administration com_config Libraries Unit Tests Templates (admin) | ⇒ | Administration com_config Templates (admin) Libraries JavaScript Unit Tests |
Done. Please test with this code in console:
Joomla.request({
url: '.',
method: 'POST',
data: {foo: 'bar'}
});
I'm wonder that our request method do not support PUT, PATCH and DELETE???
I'm wonder that our request method does not support PUT, PATCH and DELETE???
Maybe forcing this behaviour only to POST is not a great idea...
Maybe forcing this behaviour only to POST is not a great idea...
I think we can leave this issue for J4 since JInput in J3 is not good at handling pure PUT, PATCH and DELETE data.
JInput handles them just fine. The magic getter in JInput looks for superglobals or subclasses, and there isn't something matching that definition for any request method except GET and POST.
Maybe I'm miss something but to my knowledge, PUT request data must read by fopen('php://input'), it cannot get from $_PUT. But JInput didn't handle it. (I only see JInputJSON did it)
Right, so we already cover that case. GET and POST requests have corresponding superglobals, the other request types don't, and I don't think we should be changing JInput's magic getter to proxy a request method into another object instance personally.
The thing that falls over is if you blindly try to do $input->$method->get() without validating $method (which, sadly, core actually does this). IMO it's not hard to do something like this:
$input = JFactory::getApplication()->input;
$method = $input->getMethod();
if ($method === 'PUT') {
$data = $input->json->get('foo');
elseif (in_array($method, ['GET', 'POST'])) {
$data = $input->$method->get('foo');
} else {
throw new InvalidArgumentException(sprintf('The "%s" request method is not supported by this route, please send a GET, POST, or PUT request.', $method), 405);
}
Actually I think $input->put->get() is a nice pattern since the main subject of JInput is to help us integrate all system INPUT in one interface.
Maybe add a JinputFormdata to parse php://input instead get value from $_REQUEST. This is what I'm working in my framework
But I think it is too big subject for this PR, let's leave this issue for the Framework design, not discussion here.
Here I have a security concern, if we auto inject CSRF into all request, can there be a case that developer or 3rd extensions send a request with CSRF token to outside of Joomla without aware?
Use script option to handle CSRF
| Status | Pending | ⇒ | Discussion |
| Labels |
Added:
?
Removed: ? |
||
| Category | Administration com_config Libraries Unit Tests Templates (admin) JavaScript | ⇒ | Unit Tests Repository Administration com_admin SQL Postgresql |
| Category | Administration Unit Tests Repository com_admin SQL Postgresql | ⇒ | Administration com_config Templates (admin) Libraries JavaScript Unit Tests |
| Status | Discussion | ⇒ | Pending |
| Labels | |||
I have tested this item
After installing a new site based on that branch here the send mail button still works. Thanks
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14952.
I don't know about mamp but in xampp there is a folder where all mails get saved. Just check how it works before the patch. As it should work like the same after the patch
Without Patch using Default-Path "/usr/sbin/sendmail" get after Click on Button "Send Test Mail" Error Test mail could not be send and Notice Could not execute: /usr/sbin/sendmail.
If i get same Result using PR is this a successfully Test?
hmm IMO it would better to do a working test.
thanks, hopefully another one will test.
If you are using MAMP in MacOS, I guess select the PHP Mail option may work.
@asika32764 none of the 3 works, this is from "PHP Mail":
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-07-29 15:57:36 |
| Closed_By | ⇒ | asika32764 | |
| Labels | |||
@asika32764 Isn't the above error coming from the local envoirment? As this should have nothing todo with the patch here?
i haven't applied the PR, so this Notice and Errors don't come from PR.
| Status | Closed | ⇒ | New |
| Closed_Date | 2017-07-29 15:57:36 | ⇒ | |
| Closed_By | asika32764 | ⇒ |
| Status | New | ⇒ | Pending |
A mistake that deleted the branch from forked repo, just restored it.
@franz-wohlkoenig Actually I think it is successfully test if it return same message before and after patch.
I have tested this item
Got same Messages with PR as without.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14952.
| Status | Pending | ⇒ | Ready to Commit |
| Easy | No | ⇒ | Yes |
RTC after two successful tests.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-07-29 16:57:50 |
| Closed_By | ⇒ | mbabker | |
| Labels |
Added:
?
|
||
I have not tested this item.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14952.
I have not tested this item.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14952.
@asika32764 can you add the post header also to core Joomla.request ?
https://github.com/joomla/joomla-cms/blob/staging/media/system/js/core-uncompressed.js#L792-L888