Merge pull request #1834 from codidact/0valt/csrf

Switch to cookie-based CSRF

Author: Oaphi

app/assets/javascripts/posts.js

@@ -57,6 +57,7 @@ $(() => {
 
     const $fileInput = $tgt.find('input[type="file"]');
     const files = /** @type {HTMLInputElement} */ ($fileInput[0]).files;
+    const form = /** @type {HTMLFormElement} */ ($tgt[0]);
 
     // TODO: MaxUploadSize is a site setting and can be changed
     if (files.length > 0 && files[0].size >= 2000000) {
@@ -81,40 +82,27 @@ $(() => {
       $tgt.find('.js-max-size').removeClass('has-color-red-700');
     }
 
-    const resp = await fetch($tgt.attr('action'), {
-      method: $tgt.attr('method'),
-      body: new FormData(/** @type {HTMLFormElement} */ ($tgt[0])),
-    });
+    const data = await QPixel.upload($tgt.attr('action'), form);
 
-    const data = await resp.json();
+    QPixel.handleJSONResponse(
+      data,
+      (data) => {
+        form.reset();
 
-    if (resp.status === 200) {
-      $tgt.trigger('ajax:success', data);
-    }
-    else {
-      $tgt.trigger('ajax:failure', data);
-    }
-  });
+        const $postField = $('.js-post-field');
+        const postText = $postField.val()?.toString();
+        $postField.val(postText.replace(placeholder, `![Image_alt_text](${data.link})`));
+        $tgt.parents('.modal').removeClass('is-active');
 
-  $uploadForm.on('ajax:success', async (evt, data) => {
-    const $tgt = $(evt.target);
-    /** @type {HTMLFormElement} */ ($tgt[0]).reset();
-
-    const $postField = $('.js-post-field');
-    const postText = $postField.val()?.toString();
-    $postField.val(postText.replace(placeholder, `![Image_alt_text](${data.link})`));
-    $tgt.parents('.modal').removeClass('is-active');
-
-    $postFields.trigger('change');
-  });
-
-  $uploadForm.on('ajax:failure', async (evt, data) => {
-    const $tgt = $(evt.target);
-    const $postField = $('.js-post-field');
-    const error = data['error'];
-    QPixel.createNotification('danger', error);
-    $tgt.parents('.modal').removeClass('is-active');
-    $postField.val($postField.val()?.toString().replace(placeholder, ''));
+        $postFields.trigger('change');
+      },
+      (data) => {
+        if (data.status === 'failed') {
+          const $postField = $('.js-post-field');
+          $postField.val($postField.val()?.toString().replace(placeholder, ''));
+        }
+      },
+    );
   });
 
   /**

app/assets/javascripts/qpixel_api.js

@@ -7,12 +7,6 @@ const validators = [];
 let popped_modals_ct = 0;
 
 window.QPixel = {
-  csrfToken: () => {
-    const token = $('meta[name="csrf-token"]').attr('content');
-    QPixel.csrfToken = () => token;
-    return token;
-  },
-
   createNotification: function (type, message) {
     // Some messages include a date stamp, `append_date` governs that.
     let append_date = false;
@@ -320,10 +314,8 @@ window.QPixel = {
 
   fetch: async (uri, init) => {
     const defaultHeaders = {
-      'X-CSRF-Token': QPixel.csrfToken(),
       // X-Requested-With is necessary for request.xhr? to work
       'X-Requested-With': 'XMLHttpRequest',
-      'Content-Type': 'application/json',
     };
 
     const { headers = {}, ...restInit } = init ?? {};
@@ -342,11 +334,17 @@ window.QPixel = {
   },
 
   fetchJSON: async (uri, data, options = {}) => {
+    const { headers = {}, ...restOptions } = options
+
     /** @type {RequestInit} */
     const requestInit = {
       method: 'POST',
       body: options.method === 'GET' ? void 0 : JSON.stringify(data),
-      ...options,
+      headers: {
+          'Content-Type': 'application/json',
+          ...headers,
+      },
+      ...restOptions,
     };
 
     return QPixel.fetch(uri, requestInit);
@@ -464,6 +462,15 @@ window.QPixel = {
     return QPixel.parseJSONResponse(resp, 'Failed to vote');
   },
 
+  upload: async (url, form) => {
+    const resp = await QPixel.fetch(url, {
+      method: 'POST',
+      body: new FormData(form)
+    });
+
+    return QPixel.parseJSONResponse(resp, 'Failed to upload');
+  },
+
   archiveThread: async (id) => {
     const resp = await QPixel.fetchJSON(`/comments/thread/${id}/archive`, {}, {
       headers: { 'Accept': 'application/json' },

app/controllers/application_controller.rb

@@ -4,7 +4,7 @@
 class ApplicationController < ActionController::Base
   # Prevent CSRF attacks by raising an exception.
   # For APIs, you may want to use :null_session instead.
-  protect_from_forgery with: :exception
+  protect_from_forgery with: :exception, store: :cookie
   before_action :configure_permitted_parameters, if: :devise_controller?
   before_action :set_globals
   before_action :enforce_signed_in, unless: :devise_controller?
@@ -417,4 +417,10 @@ def require_sudo
       redirect_to user_sudo_path
     end
   end
+
+  # default request_authenticity_tokens only checks form tokens and request.x_csrf_token
+  # for some reason, even if cookie-based strategy is officially supported, it's not checked here
+  def request_authenticity_tokens
+    super << csrf_token_storage_strategy.fetch(request)
+  end
 end

app/controllers/errors_controller.rb

@@ -1,7 +1,7 @@
 # Provides web actions that represent errors. Rails' standard error pages are static HTML with inline CSS; by using
 # a custom error controller we get all the layouts and CSS.
 class ErrorsController < ApplicationController
-  protect_from_forgery except: [:error]
+  protect_from_forgery with: :exception, except: [:error], store: :cookie
 
   def error
     @exception = request.env['action_dispatch.exception']

app/controllers/posts_controller.rb

@@ -510,14 +510,16 @@ def document
 
   def upload
     unless helpers.valid_upload?(params[:file])
-      render json: { error: "Images must be one of #{helpers.allowed_upload_extensions.join(', ')}" },
+      render json: { status: 'failed',
+                     message: "Images must be one of #{helpers.allowed_upload_extensions.join(', ')}" },
              status: :bad_request
       return
     end
 
     @blob = ActiveStorage::Blob.create_and_upload!(io: params[:file], filename: params[:file].original_filename,
                                                    content_type: params[:file].content_type)
-    render json: { link: uploaded_url(@blob.key) }
+    render json: { status: 'success',
+                   link: uploaded_url(@blob.key) }
   end
 
   def help_center

app/controllers/users/sessions_controller.rb

@@ -1,7 +1,7 @@
 class Users::SessionsController < Devise::SessionsController
   include Devise::Controllers::Rememberable
 
-  protect_from_forgery except: [:create]
+  protect_from_forgery with: :exception, except: [:create, :destroy], store: :cookie
 
   mattr_accessor :first_factor, default: [], instance_writer: false, instance_reader: false
 

app/views/posts/_image_upload.html.erb

@@ -7,7 +7,6 @@
     </div>
     <div class="modal--body">
       <%= form_tag upload_path, multipart: true, class: 'form-inline upload-form js-upload-form' do %>
-        <%= hidden_field_tag :authenticity_token, form_authenticity_token %>
         <div class="form-group">
           <%= label_tag :file, 'Insert an image', class: 'form-element' %>
           <div class="form-caption js-max-size">Max file size <%= SiteSetting['MaxUploadSize'] %>.</div>

config/environments/production.rb

@@ -16,6 +16,7 @@
   # Full error reports are disabled and caching is turned on.
   config.consider_all_requests_local       = false
   config.action_controller.perform_caching = true
+  Rack::MiniProfiler.config.disable_caching = false
 
   # Ensures that a master key has been made available in either ENV["RAILS_MASTER_KEY"]
   # or in config/master.key. This key is used to decrypt credentials (and other encrypted files).

global.d.ts

@@ -213,6 +213,10 @@ type QPixelResponseJSON<
   Success extends object = object
 > = (Success & QPixelSuccessResponseJSON) | QPixelFailedResponseJSON
 
+type QPixelUploadResponseJSON = QPixelResponseJSON<{
+  link: string
+}>
+
 type QPixelVoteResponseJSON = QPixelResponseJSON<{
   vote_id: number
   upvotes: number
@@ -411,12 +415,6 @@ interface QPixel {
    */
   addPrePostValidation?: (callback: PostValidator) => void;
 
-  /**
-   * Get the current CSRF anti-forgery token. Should be passed as the X-CSRF-Token header when
-   * making AJAX POST requests.
-   */
-  csrfToken?: () => string;
-
   /**
    * Create a notification popup - not an inbox notification.
    * @param type the type to apply to the popup - warning, danger, etc.
@@ -646,6 +644,12 @@ interface QPixel {
    */
   vote?: (postId: string, voteType: string) => Promise<QPixelVoteResponseJSON>
 
+  /**
+   * @param url upload endpoint URL (differs between routes)
+   * @param form upload form to get the file from
+   */
+  upload?: (url: string, form: HTMLFormElement) => Promise<QPixelUploadResponseJSON>
+
   // qpixel_dom
   DOM?: QPixelDOM;
   // qpixel Markdown

test/controllers/posts/upload_test.rb

@@ -0,0 +1,28 @@
+require 'test_helper'
+
+class PostsControllerTest < ActionController::TestCase
+  include Devise::Test::ControllerHelpers
+
+  test 'upload should correctly check file types' do
+    sign_in users(:standard_user)
+
+    try_upload_file('not_uploadable.json', 'application/json')
+
+    assert_response(:bad_request)
+    assert_valid_json_response
+    assert_not_nil JSON.parse(response.body)['message']
+
+    try_upload_file('uploadable_png.png', 'image/png')
+
+    assert_json_success
+    assert_not_nil JSON.parse(response.body)['link']
+  end
+
+  private
+
+  def try_upload_file(path, mime)
+    post :upload, params: {
+      file: file_fixture_upload(path, mime)
+    }
+  end
+end