Add CSRF into gem

[dzirtusss]

Referencing issue #16, first draft.

Added JS helpers:
getAuthenticityToken() - returns CSRF token
authenticityHeader(header) - returns complete header with X-CSRF-Token and X-Requested-With: XMLHttpRequest

Waiting for comments.

---

This change is

[coveralls]

Changes Unknown when pulling ad7fb92 on dzirtusss:add-csrf into * on shakacode:master*.

[justin808]

We probably don't want to suck in lodash, at least not all of it. Are we using lodash elsewhere in this module? We could consider bringing in a tiny bit of it.

---

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[dzirtusss]

Temporary. Lodash not needed.

---

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[coveralls]

Changes Unknown when pulling 6b57be0 on dzirtusss:add-csrf into * on shakacode:master*.

[AlexKVal]

👍

[dzirtusss]

Actually I think we should not need to include jsonHelper into gem and specifically include json Accept and inContent-Type into headers because it is already included by ??? as follows:

Content-Type: application/json;charset=UTF-8
Accept: application/json, text/plain, */*
Cookie: _rails-react-tutorial_session=...

And everything works fine.

Maybe it will not work with fetch same way but will need to update first from axios to isomorphic-fetch in tutorial.

I think it will be ok if we use fetch like:

credentials: 'same-origin',
header: RubyOnRails.authenticityHeader(),

[dzirtusss]

Was also thinking does this sounds better names? It better be more clear. Any opinions?

getCSRFHeader()
getCSRFToken()

[coveralls]

Coverage remained the same at 82.36% when pulling e202218 on dzirtusss:add-csrf into a6373f0 on shakacode:master.

[coveralls]

Coverage remained the same at 82.36% when pulling e202218 on dzirtusss:add-csrf into a6373f0 on shakacode:master.

[coveralls]

Coverage remained the same at 82.36% when pulling 3af481d on dzirtusss:add-csrf into a6373f0 on shakacode:master.

[AlexKVal]

var and const
Not a big deal. It is rather matter of style.
🍒

[justin808]

Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions.

---

CHANGELOG.md, line 11 [r5] (raw file):

- React on Rails now correctly parses single-digit version strings from package.json [#491](https://github.com/shakacode/react_on_rails/pull/491) by [samphilipd ](https://github.com/samphilipd ).
- Fixed assets symlinking to correctly use filenames with spaces. Begining in [#510](https://github.com/shakacode/react_on_rails/pull/510), ending in [#513](https://github.com/shakacode/react_on_rails/pull/513) by [dzirtusss](https://github.com/dzirtusss) 
- Added getAuthenticityToken() and authenticityHeader() javascript helpers for easier use when working with CSRF protection tag generated by Rails [#517](https://github.com/shakacode/react_on_rails/pull/517) by [dzirtusss](https://github.com/dzirtusss) 

update names?

---

node_package/src/ReactOnRails.js, line 82 [r2] (raw file):

Previously, AlexKVal (Alexander Shemetovsky) wrote…

👍

Maybe we don't need the `get` so ==> `authenticityToken()`

---

node_package/src/ReactOnRails.js, line 96 [r5] (raw file):

authenticityHeader(options) {

addAuthenticityHeaders(options = {})

---

Comments from Reviewable

[justin808]

Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions.

---

node_package/src/ReactOnRails.js, line 96 [r5] (raw file):

Previously, justin808 (Justin Gordon) wrote…

authenticityHeader(options) {

addAuthenticityHeaders(options = {})

Maybe better is

withAuthenticityHeaders(otherHeaders = {})

---

Comments from Reviewable

[justin808]

Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions.

---

node_package/src/ReactOnRails.js, line 96 [r5] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Maybe better is

withAuthenticityHeaders(otherHeaders = {})

From the example https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch

Looks best if we go with authenticityHeaders(otherHeaders)

var myInit = { method: 'GET',
               headers: ReactOnRails.authenticityHeaders(),
               mode: 'cors',
               cache: 'default' };

fetch('flowers.jpg',myInit)
.then(function(response) {
  return response.blob();
})
.then(function(myBlob) {
  var objectURL = URL.createObjectURL(myBlob);
  myImage.src = objectURL;
});

or with other options:

var otherHeaders = { foo: 'bar' };
var headers = ReactOnRails.authenticityHeaders(otherHeaders);
var myInit = { method: 'GET',
               headers: headers,
               mode: 'cors',
               cache: 'default' };

fetch('flowers.jpg',myInit)
.then(function(response) {
  return response.blob();
})
.then(function(myBlob) {
  var objectURL = URL.createObjectURL(myBlob);
  myImage.src = objectURL;
});

---

Comments from Reviewable

[justin808]

We're ready to go!

1. Adjust the naming for the docs.

---

Reviewed 1 of 2 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

node_package/src/ReactOnRails.js, line 82 [r2] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Maybe we don't need the get so ==> authenticityToken()

@dzirtusss We need to copy the docs here: https://github.com/shakacode/react_on_rails/blob/master/docs/api/javascript-api.md

Note, this one is missing. Please add with the other apis

/**
*  Allow directly calling the page loaded script in case the default events that trigger react rendering are not sufficient, such as when loading JavaScript asynchronously with TurboLinks:
* More details can be found here: https://github.com/shakacode/react_on_rails/blob/master/docs/additional-reading/turbolinks.md
*/
  reactOnRailsPageLoaded() {
    ClientStartup.reactOnRailsPageLoaded();
  },

---

node_package/src/ReactOnRails.js, line 76 [r5] (raw file):

  /**
   * Returns CSRF authenticity token inserted by Rails csrf_meta_tags

We can consider putting this in the internal API.

const token = ReactOnRails.authenticityHeaders()['X-CSRF-Token'];

That way we only add one method to the API.

@robwise How does this fit with https://www.friendsandguests.com/

CC: @alexfedoseev

---

node_package/src/ReactOnRails.js, line 82 [r5] (raw file):

  getAuthenticityToken() {
    const token = document.querySelector('meta[name="csrf-token"]');
    return token ? token.content : null;

Very important: Let's put in this code in a small file, called Authenticity.js which contains the two authenticity functions. See how this file delegates other chunks.

Then there's the test for this in a separate file.

We also still need a basic test at the top level to verify the public api calls exist.

---

node_package/src/ReactOnRails.js, line 86 [r5] (raw file):

  /**
   * Returns header with csrf authenticity token

I'd like to see some detailed doc on this:

References
* http://guides.rubyonrails.org/security.html#cross-site-request-forgery-csrf
* https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch
* https://github.com/matthew-andrews/isomorphic-fetch

Example:

blah, blah

and we need this material duplicated: https://github.com/shakacode/react_on_rails/blob/master/docs/api/javascript-api.md

@AlexKVal Know of any way that we can generate this file?

The README.md should have a couple sentence blurb on about using isormorphic-fetch vs. using the built in jQuery fetch (https://github.com/rails/jquery-ujs) that already puts in the headers.

---

node_package/tests/ReactOnRails.test.js, line 138 [r5] (raw file):

var otherHeaders = { foo: 'bar' };
var headers = ReactOnRails.authenticityHeaders(otherHeaders);
var myInit = { method: 'GET',
headers: headers,
mode: 'cors',
cache: 'default' };
Yes -- let's stick to ES6 syntax.

---

node_package/tests/ReactOnRails.test.js, line 148 [r5] (raw file):

    'authenticityHeader returns valid header with CFRS token'
  );
});

Nice tests!

See comment above on putting this into a smaller file.

We still need a test here that verifies this as well. I'm OK with some duplication for this small amount of code.

Maybe @AlexKVal has an opinion.

---

Comments from Reviewable

[robwise]

Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

node_package/src/ReactOnRails.js, line 82 [r2] (raw file):

Previously, justin808 (Justin Gordon) wrote…

@dzirtusss We need to copy the docs here:
https://github.com/shakacode/react_on_rails/blob/master/docs/api/javascript-api.md

Note, this one is missing. Please add with the other apis

/**
*  Allow directly calling the page loaded script in case the default events that trigger react rendering are not sufficient, such as when loading JavaScript asynchronously with TurboLinks:
* More details can be found here: https://github.com/shakacode/react_on_rails/blob/master/docs/additional-reading/turbolinks.md
*/
  reactOnRailsPageLoaded() {
    ClientStartup.reactOnRailsPageLoaded();
  },

`get` is better IMO because you're actually going out to the DOM to get it, as opposed to it just somehow being already inside of ReactOnRails

---

node_package/src/ReactOnRails.js, line 76 [r5] (raw file):

Previously, justin808 (Justin Gordon) wrote…

We can consider putting this in the internal API.

const token = ReactOnRails.authenticityHeaders()['X-CSRF-Token'];

That way we only add one method to the API.

@robwise How does this fit with https://www.friendsandguests.com/

CC: @alexfedoseev

If you want to only have one method in the API, I would make it just the thing that gets the token, and call it `getRailsCSRFToken`.

The rest is not necessarily specific to rails and therefore is a bit out of scope for this project IMO. You're making assumptions that everyone using ReactOnRails wants X-Requested-With. Not sure that's really true, for example they may be trying to debug something. This also would make it extremely clear what the method does just by the name, whereas authenticityHeaders could mean pretty much anything under the sun.

---

node_package/src/ReactOnRails.js, line 86 [r5] (raw file):

Previously, justin808 (Justin Gordon) wrote…

I'd like to see some detailed doc on this:

References
* http://guides.rubyonrails.org/security.html#cross-site-request-forgery-csrf
* https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch
* https://github.com/matthew-andrews/isomorphic-fetch

Example:

blah, blah

and we need this material duplicated: https://github.com/shakacode/react_on_rails/blob/master/docs/api/javascript-api.md

@AlexKVal Know of any way that we can generate this file?

The README.md should have a couple sentence blurb on about using isormorphic-fetch vs. using the built in jQuery fetch (https://github.com/rails/jquery-ujs) that already puts in the headers.

There are many libraries besides isomorphic-fetch, I think you could mention the jQuery ujs does it automatically, and then have instructions for _any_ non-jQuery-ujs Ajax library

---

Comments from Reviewable

[justin808]

Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

node_package/src/ReactOnRails.js, line 82 [r2] (raw file):

Previously, robwise (Rob Wise) wrote…

get is better IMO because you're actually going out to the DOM to get it, as opposed to it just somehow being already inside of ReactOnRails

IMO, ReactOnRails is there to help with Rails things, and the CSRF is on the rails page, so `ReactOnRails.authenticityToken()` is fine.

Java is notorious for calling everything getXXXX and setXXXXX.

---

node_package/src/ReactOnRails.js, line 76 [r5] (raw file):

Previously, robwise (Rob Wise) wrote…

If you want to only have one method in the API, I would make it just the thing that gets the token, and call it getRailsCSRFToken.

The rest is not necessarily specific to rails and therefore is a bit out of scope for this project IMO. You're making assumptions that everyone using ReactOnRails wants X-Requested-With. Not sure that's really true, for example they may be trying to debug something. This also would make it extremely clear what the method does just by the name, whereas authenticityHeaders could mean pretty much anything under the sun.

Rob, Rails calls the exception: `ActionController::InvalidAuthenticityToken`, so "authenticity" is consistent

Let's add the two methods

authenticityToken()

ajaxHeaders(otherHeaders)

---

node_package/src/ReactOnRails.js, line 86 [r5] (raw file):

Previously, robwise (Rob Wise) wrote…

There are many libraries besides isomorphic-fetch, I think you could mention the jQuery ujs does it automatically, and then have instructions for any non-jQuery-ujs Ajax library

@robwise I agree.

---

Comments from Reviewable

[justin808]

Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

node_package/src/ReactOnRails.js, line 76 [r5] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Rob, Rails calls the exception: ActionController::InvalidAuthenticityToken, so "authenticity" is consistent

Let's add the two methods

authenticityToken()

ajaxHeaders(otherHeaders)

I did a bit more reading on this, and Angular had an issue because [it was always including](https://github.com/angular/angular.js/issues/11008) the `X-Requested-With`. These bits are very specific to Rails on the back end, so I do think it makes sense to make the function names reflect that this is the **rails** authenticity token, and the **rails** authenticity headers. A very slight issue is that these headers might not be needed for get requests. However, it's probably not an issue to always include it.

railsAuthenticityToken()

and

railsAuthenticityHeaders(otheHeaders)

---

node_package/src/ReactOnRails.js, line 95 [r5] (raw file):

    return Object.assign(options, {
      'X-CSRF-Token': ReactOnRails.getAuthenticityToken(),
      'X-Requested-With': 'XMLHttpRequest'

I did some research on the X-Requested-With header. This is specific to Rails. ReactOnRails is specific to Rails. So this is OK and appropriate.

Naturally, for API requests to other servers, we won't send this or the X-CSRF-Token.

---

Comments from Reviewable

[dzirtusss]

Done.

---

Review status: 0 of 6 files reviewed at latest revision, 10 unresolved discussions.

---

CHANGELOG.md, line 11 [r5] (raw file):

Previously, justin808 (Justin Gordon) wrote…

update names?

Done.

---

node_package/src/ReactOnRails.js, line 82 [r2] (raw file):

Previously, justin808 (Justin Gordon) wrote…

IMO, ReactOnRails is there to help with Rails things, and the CSRF is on the rails page, so ReactOnRails.authenticityToken() is fine.

Java is notorious for calling everything getXXXX and setXXXXX.

Done.

---

node_package/src/ReactOnRails.js, line 76 [r5] (raw file):

Previously, justin808 (Justin Gordon) wrote…

I did a bit more reading on this, and Angular had an issue because it was always including the X-Requested-With. These bits are very specific to Rails on the back end, so I do think it makes sense to make the function names reflect that this is the rails authenticity token, and the rails authenticity headers. A very slight issue is that these headers might not be needed for get requests. However, it's probably not an issue to always include it.

railsAuthenticityToken()

and

railsAuthenticityHeaders(otheHeaders)

For GET both functions are not needed. This is in Rails core that GET goes w/o token, which is recommended practice for csrf - GET only retrieves info

---

node_package/src/ReactOnRails.js, line 82 [r5] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Very important: Let's put in this code in a small file, called Authenticity.js which contains the two authenticity functions. See how this file delegates other chunks.

Then there's the test for this in a separate file.

We also still need a basic test at the top level to verify the public api calls exist.

Done.

---

node_package/src/ReactOnRails.js, line 95 [r5] (raw file):

Previously, justin808 (Justin Gordon) wrote…

I did some research on the X-Requested-With header. This is specific to Rails. ReactOnRails is specific to Rails. So this is OK and appropriate.

Naturally, for API requests to other servers, we won't send this or the X-CSRF-Token.

Done.

---

node_package/src/ReactOnRails.js, line 96 [r5] (raw file):

Previously, justin808 (Justin Gordon) wrote…

From the example https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch

Looks best if we go with authenticityHeaders(otherHeaders)

var myInit = { method: 'GET',
               headers: ReactOnRails.authenticityHeaders(),
               mode: 'cors',
               cache: 'default' };

fetch('flowers.jpg',myInit)
.then(function(response) {
  return response.blob();
})
.then(function(myBlob) {
  var objectURL = URL.createObjectURL(myBlob);
  myImage.src = objectURL;
});

or with other options:

var otherHeaders = { foo: 'bar' };
var headers = ReactOnRails.authenticityHeaders(otherHeaders);
var myInit = { method: 'GET',
               headers: headers,
               mode: 'cors',
               cache: 'default' };

fetch('flowers.jpg',myInit)
.then(function(response) {
  return response.blob();
})
.then(function(myBlob) {
  var objectURL = URL.createObjectURL(myBlob);
  myImage.src = objectURL;
});

Done.

---

node_package/tests/ReactOnRails.test.js, line 138 [r5] (raw file):

Previously, justin808 (Justin Gordon) wrote…

var otherHeaders = { foo: 'bar' };
var headers = ReactOnRails.authenticityHeaders(otherHeaders);
var myInit = { method: 'GET',
headers: headers,
mode: 'cors',
cache: 'default' };
Yes -- let's stick to ES6 syntax.

Done.

---

node_package/tests/ReactOnRails.test.js, line 148 [r5] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Nice tests!

See comment above on putting this into a smaller file.

We still need a test here that verifies this as well. I'm OK with some duplication for this small amount of code.

Maybe @AlexKVal has an opinion.

Done.

---

Comments from Reviewable

[justin808]

---

Reviewed 6 of 6 files at r6.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable

[justin808]

Reviewed 1 of 1 files at r7.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks broke.

---

README.md, line 420 [r7] (raw file):

#### Using Rails built-in CSRF protection in JavaScript

Rails has built-in protection for Cross-Site Request Forgery (CSRF), see [Rails Documentation](http://guides.rubyonrails.org/security.html#cross-site-request-forgery-csrf). To nicely utilize this feature in JavaScript requests, React on Rails is offerring two helpers that can be used as following:

Should we mention somewhere that jquery-ujs adds this jQuery's ajax methods?

---

README.md, line 426 [r7] (raw file):

// reads from DOM csrf token generated by Rails in <%= csrf_meta_tags %>
csrfToken = ReactOnRails.authenticityToken();               

extra trailing spaces here -- see red dots.

---

Comments from Reviewable

[dzirtusss]

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks broke.

---

README.md, line 420 [r7] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Should we mention somewhere that jquery-ujs adds this jQuery's ajax methods?

Done.

---

README.md, line 426 [r7] (raw file):

Previously, justin808 (Justin Gordon) wrote…

extra trailing spaces here -- see red dots.

Done.

---

node_package/src/ReactOnRails.js, line 86 [r5] (raw file):

Previously, justin808 (Justin Gordon) wrote…

@robwise I agree.

Done.

---

Comments from Reviewable

[AlexKVal]

and we need this material duplicated:
https://github.com/shakacode/react_on_rails/blob/master/docs/api/javascript-api.md
@AlexKVal Know of any way that we can generate this file?

@justin808 I am definitely not an expert in automated documentation although I dare to suppose smth like that https://github.com/cbou/markdox would be fit for it.
And o/c it is for another issue.
🍒

[coveralls]

Coverage remained the same at 82.36% when pulling 426adaa on dzirtusss:add-csrf into e293a94 on shakacode:master.

[justin808]

Review status: 6 of 7 files reviewed at latest revision, 7 unresolved discussions.

---

README.md, line 433 [r8] (raw file):

If you are using jquery-ujs for AJAX calls, than these helpers are not needed and jquery-ujs library updates header automatically, see jquery-ujs documentation.

Minor grammar fix:

If you are using [jquery-ujs](https://github.com/rails/jquery-ujs) for AJAX calls, than these helpers are not needed because the [jquery-ujs](https://github.com/rails/jquery-ujs) library updates header automatically, see [jquery-ujs documentation](https://robots.thoughtbot.com/a-tour-of-rails-jquery-ujs#cross-site-request-forgery-protection).

---

Comments from Reviewable

[justin808]

One tiny change!

---

Reviewed 1 of 1 files at r8.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

Comments from Reviewable

[dzirtusss]

Review status: 6 of 7 files reviewed at latest revision, 5 unresolved discussions.

---

README.md, line 433 [r8] (raw file):

Previously, justin808 (Justin Gordon) wrote…

If you are using jquery-ujs for AJAX calls, than these helpers are not needed and jquery-ujs library updates header automatically, see jquery-ujs documentation.

Minor grammar fix:

If you are using [jquery-ujs](https://github.com/rails/jquery-ujs) for AJAX calls, than these helpers are not needed because the [jquery-ujs](https://github.com/rails/jquery-ujs) library updates header automatically, see [jquery-ujs documentation](https://robots.thoughtbot.com/a-tour-of-rails-jquery-ujs#cross-site-request-forgery-protection).

Done.

---

Comments from Reviewable

[coveralls]

Coverage remained the same at 82.36% when pulling e02676e on dzirtusss:add-csrf into 52f0614 on shakacode:master.

[justin808]

---

Reviewed 1 of 1 files at r9.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable

[coveralls]

Coverage remained the same at 82.36% when pulling 52104d5 on dzirtusss:add-csrf into 52f0614 on shakacode:master.

[justin808]

---

Reviewed 1 of 1 files at r10.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable