Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider a CSRF replacement for $.ajax #16

Closed
justin808 opened this issue Sep 5, 2015 · 28 comments
Closed

Consider a CSRF replacement for $.ajax #16

justin808 opened this issue Sep 5, 2015 · 28 comments

Comments

@justin808
Copy link
Member

See:
shakacode/react-webpack-rails-tutorial#73 (comment)

We could provide a something helpful in this gem that facilitates true promises via 'fetch'.

@dphaener
Copy link

dphaener commented Oct 2, 2015

My two cents. I use this:

import _ from 'underscore'

/* globals document */

export default class BaseData {
  constructor() {
    this.defaultParams = {
      credentials: 'same-origin'
    }
  }

  getCSRFToken() {
    return _.find(document.getElementsByTagName("meta"), (meta) => {
      return meta.name === "csrf-token"
    }).content
  }

  defaultHeaders = () => {
    return {
      'X-CSRF-Token': this.getCSRFToken(),
      'Accept': 'application/vnd.api+json',
      'Content-Type': 'application/vnd.api+json'
    }
  }

  post = (url, props) => {
    return fetch(url, {
      method: 'post',
      headers: this.defaultHeaders(),
      body: JSON.stringify(props),
      ...this.defaultParams
    })
    .then((response) => {
      return response.json()
    })
  }

  put = (url, props) => {
    return fetch(url, {
      method: 'put',
      headers: this.defaultHeaders(),
      body: JSON.stringify(props),
      ...this.defaultParams
    })
    .then((response) => {
      return response.json()
    })
  }

  delete = (url) => {
    return fetch(url, {
      method: 'delete',
      headers: this.defaultHeaders(),
      ...this.defaultParams
    })
    .then((response) => {
      return response.json()
    })
  }
}

And this module: "whatwg-fetch": "^0.9.0"

And in the webpack config:

new webpack.ProvidePlugin({
      'fetch': 'imports?this=>global!exports?global.fetch!whatwg-fetch'
    })

@justin808
Copy link
Member Author

We're using axios here:

https://github.com/shakacode/react-webpack-rails-tutorial/blob/master/client/package.json

@alexfedoseev Are we missing the headers for:

     'Accept': 'application/vnd.api+json',
      'Content-Type': 'application/vnd.api+json'

Here's our source: https://github.com/shakacode/react-webpack-rails-tutorial/blob/master/client%2Fapp%2Futils%2FcommentsManager.js

import request from 'axios';

const API_URL = 'comments.json';

const CommentsManager = {

  /**
   * Retrieve comments from server using AJAX call.
   *
   * @returns {Promise} - result of ajax call.
   */
  fetchComments() {
    return request({
      method: 'GET',
      url: API_URL,
      responseType: 'json',
    });
  },

  /**
   * Submit new comment to server using AJAX call.
   *
   * @param {Object} comment - Comment body to post.
   * @returns {Promise} - result of ajax call.
   */
  submitComment(comment) {
    return request({
      method: 'POST',
      url: API_URL,
      responseType: 'json',
      headers: {
        'X-CSRF-Token': this.getCSRFToken(),
      },
      data: { comment },
    });
  },

  /**
   * Get CSRF Token from the DOM.
   *
   * @returns {String} - CSRF Token.
   */
  getCSRFToken() {
    const metas = document.getElementsByTagName('meta');
    for (let i = 0; i < metas.length; i++) {
      const meta = metas[i];
      if (meta.getAttribute('name') === 'csrf-token') {
        return meta.getAttribute('content');
      }
    }

    return null;
  },

};

export default CommentsManager;

@alex35mil
Copy link
Member

Are we missing the headers for

No, since we don't use Vendor MIME Type to match the api version (we use URL instead).
More on subject: https://medium.com/@alexfedoseev/isomorphic-react-with-rails-part-ii-614980b65aef

And example of apiCall helper with axios:

import http from 'axios';
import getCSRFToken from './getCSRFToken';

const apiURLPrefix = '/api/v1';

// Transforming Immutable data to plain JS
http.defaults.transformRequest.unshift(data => (
  data && data.toJS === 'function' ? data.toJS() : data
));

http.interceptors.request.use(config => {
  // If it's not remote call, adding api prefix
  if (!config.remote) {
    config.url = `${apiURLPrefix}${config.url}`;
  }

  // Adding CSRF Token header
  config.headers = {
    'X-CSRF-Token': getCSRFToken(),
  };
  return config;
});

export default params => http(params);

@dphaener
Copy link

dphaener commented Oct 2, 2015

👍 I'm using that because I'm using a jsonapi-resources backend, which requires that MIME type

@justin808
Copy link
Member Author

@alexfedoseev @josiasds @mapreal19 How about we release a tiny npm module to encapsulation axios and the ability to get the CRSF token from Rails? Then we can put the inclusion of the tiny npm module in the template setup.

@alex35mil
Copy link
Member

I'd rather made getCRSFToken module and left axios in user land.

Reasons:

  1. axios is opinionated.
  2. axios's setup is opinionated.

@justin808
Copy link
Member Author

This is probably the one we want to to support:

https://github.com/matthew-andrews/isomorphic-fetch

@justin808
Copy link
Member Author

@jbhatab @alexfedoseev Once we put in the NPM library, I'll try to merge in the code from the: https://github.com/shakacode/react-webpack-rails-tutorial/blob/master/client/app/libs/metaTagsManager.js

@justin808 justin808 added this to the 2.0 milestone Dec 17, 2015
@justin808 justin808 self-assigned this Dec 17, 2015
@robwise
Copy link
Contributor

robwise commented Dec 17, 2015

@justin808 @alexfedoseev @jbhatab

Regardless though, I vote we do not put this in the npm module for the same reason that Alex chose not to use Axios's implementation. Some people may not even be doing sessions this way.

@rescribet
Copy link

Just to leave another implementation here for the sake of brainstorm. We only use helper methods that don't wrap fetch, but aid in parameter setting. Implementation in this gist.

For regular non-safe json requests:

fetch('/foobar.json', jsonHeader(/*fetch options*/));

With just the Rails CSRF headers:

fetch('/foobar.json', authenticityHeader(/*fetch options*/));

For safe calls (includes CSRF and credentials & same-origin header):

fetch('/foobar.json', safeCredentials(/*fetch options*/));

@robwise robwise modified the milestone: 2.0 Jan 14, 2016
@justin808
Copy link
Member Author

We'll soon be adding a JS helper method for the CRSF to the ReactOnRails npm module for this.

We probably want to be agnostic about the choice of fetch vs. axios.

@justin808 justin808 added this to the 2.2 milestone Jan 21, 2016
@robwise
Copy link
Contributor

robwise commented Jan 21, 2016

agreed

@justin808
Copy link
Member Author

We simply need to add this helper method to ReactOnRails.

From: https://github.com/shakacode/react-webpack-rails-tutorial/blob/master/client/app/libs/metaTagsManager.js

import _ from 'lodash';

export default {

  /**
   * Get CSRF Token from the DOM.
   *
   * @returns {String} - CSRF Token.
   */
  getCSRFToken() {
    const token = _.find(document.querySelectorAll('meta'), ['name', 'csrf-token']);
    return token ? token.content : null;
  },

};

To:

We should also remove the dependency on lodash.

We need some docs and tests.

This gist from @Fletcher91 also looks good: https://gist.github.com/fletcher91/c62b865c6aa9f710531e

@justin808 justin808 added the easy label Feb 24, 2016
@justin808
Copy link
Member Author

I'll probably add @Fletcher91's code to the default package for ReactOnRails. Yes, we could have a separate node module. It's a tiny enough amount of code that I don't deem that necessary.

@Fletcher91 Any changes to your gist? I copied below.

/**
 * 
 * A regular non-safe get request:
 * fetch('/profiles/foobar.json', jsonHeader());
 * 
 * How this would look in a safe fetch request:
 * fetch('/profiles.json', safeCredentials({
 *              method: 'POST',
 *              body: JSON.stringify({
 *                  q: input,
 *                  thing: this.props.thing
 *              })
 *          }));
 *
 * 
 */

/**
 * For use with window.fetch
 */
export function jsonHeader (options) {
    options = options || {};
    return Object.assign(options, {
        'Accept': 'application/json',
        'Content-Type': 'application/json'
    });
}

/**
 * Lets fetch include credentials in the request. This includes cookies and other possibly sensitive data.
 * Note: Never use for requests across (untrusted) domains.
 */
export function safeCredentials (options) {
    options = options || {};
    return Object.assign(options, {
        credentials: 'include',
        mode: 'same-origin',
        headers: Object.assign((options['headers'] || {}), authenticityHeader(), jsonHeader())
    });
}

// Additional helper methods

export function authenticityHeader (options) {
    options = options || {};
    return Object.assign(options, {
        'X-CSRF-Token': getAuthenticityToken(),
        'X-Requested-With': 'XMLHttpRequest'
    });
}

export function getAuthenticityToken () {
    return getMetaContent('csrf-token');
}

export function getMetaContent (name) {
    let header = document.querySelector(`meta[name="${name}"]`);
    return header && header.content;
}

@rescribet
Copy link

@justin808 Aside from some minor things like let to const and some JSdocs, it's still the same 👍 I've updated the gist accordingly

@justin808
Copy link
Member Author

@Fletcher91, @jbhatab does this make sense to go into React on Rails from the perspective that the CRSF thing is so standard for Rails apps? or more of a doc thing?

@anpr
Copy link

anpr commented Jun 29, 2016

I'd be happy to have this as part of react_on_rails. It's really a very common requirement.

@jbhatab
Copy link
Member

jbhatab commented Jul 8, 2016

@anpr With the direction were going, I think this shouldn't be covered with our gem and can be covered in a docs if even needed at all. it depends on your architecture style.

We can relook into this when we redo the docs stuff.

@jbhatab jbhatab closed this as completed Jul 8, 2016
@justin808 justin808 reopened this Jul 8, 2016
@justin808
Copy link
Member Author

@jbhatab, we're going to provide a default for this. I really think this is so common that it makes sense to include in our library, if it's non-obtrusive.

@anpr I can help you with this. Please email me at [email protected] and I can add you to our Slack room.

@rescribet
Copy link

We're working on restructuring our front-end (splitting into packages), so I can publish these helpers publicly to NPM if that helps

@anpr
Copy link

anpr commented Jul 8, 2016

I basically copied the code above to my project and it solved the problem (with small adjustments/additions).

Still, I see this being a common requirement, so I'd publish it either as part of the gem or as a separate npm package (I don't really care which of the two).

@justin808
Copy link
Member Author

@anpr I think part of the standard npm package for react-on-rails is fine if it's very small.

@justin808
Copy link
Member Author

@dzirtusss Please take a look at this one.

@dzirtusss
Copy link
Contributor

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.

@dzirtusss
Copy link
Contributor

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

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

@AlexKVal
Copy link
Contributor

Resolved by #517

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants