From 504f5d1b1a539c2ea305649a8361cb375ca1a56c Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Tue, 12 Dec 2023 10:22:01 +0100 Subject: [PATCH] Handle unauthorized requests accordingly The XHR token was not set in the HTTP header, which could lead to an unauthorized uploaded (and after that a rejection, because the CSRF - token is also in the submitted form). Now the server is rejecting before the upload, if the CSRF - token is not correct. --- app/controllers/alchemy/base_controller.rb | 4 +- .../alchemy/admin/uploader_responses.rb | 11 +---- app/javascript/alchemy_admin.js | 1 - .../alchemy_admin/components/uploader.js | 44 ++++++++++++------- .../components/uploader/file_upload.js | 2 +- .../components/uploader/progress.js | 4 +- app/javascript/alchemy_admin/utils/ajax.js | 3 +- .../test_support/shared_uploader_examples.rb | 2 +- .../admin/attachments_controller_spec.rb | 4 +- .../alchemy/admin/base_controller_spec.rb | 17 +++++++ .../alchemy/admin/pictures_controller_spec.rb | 2 +- .../alchemy_admin/components/uploader.spec.js | 30 +++++++++++++ .../components/uploader/file_upload.spec.js | 4 +- 13 files changed, 89 insertions(+), 39 deletions(-) diff --git a/app/controllers/alchemy/base_controller.rb b/app/controllers/alchemy/base_controller.rb index 4b0269cc79..726638abc0 100644 --- a/app/controllers/alchemy/base_controller.rb +++ b/app/controllers/alchemy/base_controller.rb @@ -53,7 +53,9 @@ def permission_denied(exception = nil) #{current_alchemy_user.inspect} WARN end - if current_alchemy_user + if request.format.json? + render json: {message: Alchemy.t("You are not authorized")}, status: :unauthorized + elsif current_alchemy_user handle_redirect_for_user else handle_redirect_for_guest diff --git a/app/controllers/concerns/alchemy/admin/uploader_responses.rb b/app/controllers/concerns/alchemy/admin/uploader_responses.rb index df2e1304a9..f4101df33d 100644 --- a/app/controllers/concerns/alchemy/admin/uploader_responses.rb +++ b/app/controllers/concerns/alchemy/admin/uploader_responses.rb @@ -5,10 +5,6 @@ module Admin module UploaderResponses extend ActiveSupport::Concern - included do - rescue_from ActiveRecord::ActiveRecordError, with: :error_uploaded_response - end - def successful_uploader_response(file:, status: :created) message = Alchemy.t(:upload_success, scope: [:uploader, file.class.model_name.i18n_key], @@ -34,15 +30,10 @@ def failed_uploader_response(file:) private - def error_uploaded_response(exception) - message = Alchemy.t(:error, scope: :uploader, error: exception.message) - render json: {growl_message: message}, status: 500 - end - def uploader_response(file:, message:) { files: [file.to_jq_upload], - growl_message: message + message: message } end end diff --git a/app/javascript/alchemy_admin.js b/app/javascript/alchemy_admin.js index e2d4242c4d..71799f9e80 100644 --- a/app/javascript/alchemy_admin.js +++ b/app/javascript/alchemy_admin.js @@ -62,7 +62,6 @@ setDefaultAnimation("tooltip.hide", { } }) - // Global Alchemy object if (typeof window.Alchemy === "undefined") { window.Alchemy = {} diff --git a/app/javascript/alchemy_admin/components/uploader.js b/app/javascript/alchemy_admin/components/uploader.js index afeb7b6942..62eaf998c3 100644 --- a/app/javascript/alchemy_admin/components/uploader.js +++ b/app/javascript/alchemy_admin/components/uploader.js @@ -7,6 +7,7 @@ import { AlchemyHTMLElement } from "./alchemy_html_element" import { Progress } from "./uploader/progress" import { FileUpload } from "./uploader/file_upload" import { translate } from "alchemy_admin/i18n" +import { getToken } from "alchemy_admin/utils/ajax" export class Uploader extends AlchemyHTMLElement { static properties = { @@ -63,45 +64,54 @@ export class Uploader extends AlchemyHTMLElement { */ _uploadFiles(files) { // prepare file progress bars and server request - let globalErrorMessage = undefined let fileUploadCount = 0 - const fileUploads = files.map((file) => { - const form = this.querySelector("form") - const formData = new FormData(form) - formData.set(this.fileInput.name, file) + const fileUploads = files.map((file) => { const request = new XMLHttpRequest() const fileUpload = new FileUpload(file, request) if (Alchemy.uploader_defaults.upload_limit - 1 < fileUploadCount) { fileUpload.valid = false fileUpload.errorMessage = translate("Maximum number of files exceeded") - globalErrorMessage = fileUpload.errorMessage } else if (fileUpload.valid) { fileUploadCount++ - - request.open("POST", form.action) - request.send(formData) + this._submitFile(request, file) } return fileUpload }) - if (globalErrorMessage) { - Alchemy.growl(globalErrorMessage, "error") - } - // create progress bar this.uploadProgress = new Progress(fileUploads) - this.uploadProgress.onComplete = () => { - // wait three seconds to see the result of the progressbar - setTimeout(() => (this.uploadProgress.visible = false), 1500) - setTimeout(() => this.dispatchCustomEvent("Upload.Complete"), 2000) + this.uploadProgress.onComplete = (status) => { + if (status === "successful") { + // wait three seconds to see the result of the progressbar + setTimeout(() => (this.uploadProgress.visible = false), 1500) + setTimeout(() => this.dispatchCustomEvent("Upload.Complete"), 2000) + } else { + this.dispatchCustomEvent("Upload.Failure") + } } document.body.append(this.uploadProgress) } + /** + * @param {XMLHttpRequest} request + * @param {File} file + * @private + */ + _submitFile(request, file) { + const form = this.querySelector("form") + const formData = new FormData(form) + formData.set(this.fileInput.name, file) + request.open("POST", form.action) + request.setRequestHeader("X-CSRF-Token", getToken()) + request.setRequestHeader("X-Requested-With", "XMLHttpRequest") + request.setRequestHeader("Accept", "application/json") + request.send(formData) + } + /** * @returns {HTMLInputElement} */ diff --git a/app/javascript/alchemy_admin/components/uploader/file_upload.js b/app/javascript/alchemy_admin/components/uploader/file_upload.js index c0fe63cf5a..6c04626cff 100644 --- a/app/javascript/alchemy_admin/components/uploader/file_upload.js +++ b/app/javascript/alchemy_admin/components/uploader/file_upload.js @@ -203,7 +203,7 @@ export class FileUpload extends AlchemyHTMLElement { get responseMessage() { try { const response = JSON.parse(this.request.responseText) - return response["growl_message"] + return response["message"] } catch (error) { return translate("Could not parse JSON result") } diff --git a/app/javascript/alchemy_admin/components/uploader/progress.js b/app/javascript/alchemy_admin/components/uploader/progress.js index 47fd9b4fa1..6d3a0996eb 100644 --- a/app/javascript/alchemy_admin/components/uploader/progress.js +++ b/app/javascript/alchemy_admin/components/uploader/progress.js @@ -78,7 +78,7 @@ export class Progress extends AlchemyHTMLElement { this.querySelector(`.overall-upload-value`).textContent = overallUploadSize if (this.finished) { - this.onComplete() + this.onComplete(status) } this.className = status @@ -90,7 +90,7 @@ export class Progress extends AlchemyHTMLElement { * it would be possible to trigger the event here, but the dispatching would happen * in the scope of that component and can't be cached o uploader - component level */ - onComplete() {} + onComplete(_status) {} /** * @returns {boolean} diff --git a/app/javascript/alchemy_admin/utils/ajax.js b/app/javascript/alchemy_admin/utils/ajax.js index 4b08b69c60..87e3ae2366 100644 --- a/app/javascript/alchemy_admin/utils/ajax.js +++ b/app/javascript/alchemy_admin/utils/ajax.js @@ -24,7 +24,7 @@ function buildPromise(xhr) { }) } -function getToken() { +export function getToken() { const metaTag = document.querySelector('meta[name="csrf-token"]') return metaTag.attributes.content.textContent } @@ -53,6 +53,7 @@ export default function ajax(method, path, data, accept = "application/json") { xhr.open(method, url.toString()) xhr.setRequestHeader("Content-type", "application/json; charset=utf-8") xhr.setRequestHeader("Accept", accept) + xhr.setRequestHeader("X-Requested-With", "XMLHttpRequest") xhr.setRequestHeader("X-CSRF-Token", getToken()) if (data && method.toLowerCase() !== "get") { diff --git a/lib/alchemy/test_support/shared_uploader_examples.rb b/lib/alchemy/test_support/shared_uploader_examples.rb index e41511d8d7..f1b67320b5 100644 --- a/lib/alchemy/test_support/shared_uploader_examples.rb +++ b/lib/alchemy/test_support/shared_uploader_examples.rb @@ -6,7 +6,7 @@ expect(response.media_type).to eq("application/json") expect(response.status).to eq(422) json = JSON.parse(response.body) - expect(json).to have_key("growl_message") + expect(json).to have_key("message") expect(json).to have_key("files") end end diff --git a/spec/controllers/alchemy/admin/attachments_controller_spec.rb b/spec/controllers/alchemy/admin/attachments_controller_spec.rb index 77cd12eb3c..e617ffd7e3 100644 --- a/spec/controllers/alchemy/admin/attachments_controller_spec.rb +++ b/spec/controllers/alchemy/admin/attachments_controller_spec.rb @@ -87,7 +87,7 @@ module Alchemy expect(response.media_type).to eq("application/json") expect(response.status).to eq(201) json = JSON.parse(response.body) - expect(json).to have_key("growl_message") + expect(json).to have_key("message") expect(json).to have_key("files") end end @@ -134,7 +134,7 @@ module Alchemy expect(response.media_type).to eq("application/json") expect(response.status).to eq(202) json = JSON.parse(response.body) - expect(json).to have_key("growl_message") + expect(json).to have_key("message") expect(json).to have_key("files") end diff --git a/spec/controllers/alchemy/admin/base_controller_spec.rb b/spec/controllers/alchemy/admin/base_controller_spec.rb index 2beb782f58..bd34afbf94 100644 --- a/spec/controllers/alchemy/admin/base_controller_spec.rb +++ b/spec/controllers/alchemy/admin/base_controller_spec.rb @@ -61,6 +61,7 @@ context "when called with an AccessDenied exception" do before do allow(controller).to receive(:redirect_to) + allow(controller).to receive(:render) end it "redirects to login_path if no user" do @@ -73,6 +74,22 @@ controller.send(:permission_denied, CanCan::AccessDenied.new) expect(controller).to have_received(:redirect_to).with(Alchemy.unauthorized_path) end + + context "for a json request" do + before do + expect(controller).to receive(:request) do + double(format: double(json?: true)) + end + end + + it "returns 'not authorized' message" do + controller.send(:permission_denied, CanCan::AccessDenied.new) + expect(controller).to have_received(:render).with( + json: {message: Alchemy.t("You are not authorized")}, + status: :unauthorized + ) + end + end end end diff --git a/spec/controllers/alchemy/admin/pictures_controller_spec.rb b/spec/controllers/alchemy/admin/pictures_controller_spec.rb index c3d681342f..782f5c5285 100644 --- a/spec/controllers/alchemy/admin/pictures_controller_spec.rb +++ b/spec/controllers/alchemy/admin/pictures_controller_spec.rb @@ -147,7 +147,7 @@ module Alchemy expect(response.media_type).to eq("application/json") expect(response.status).to eq(201) json = JSON.parse(response.body) - expect(json).to have_key("growl_message") + expect(json).to have_key("message") expect(json).to have_key("files") end end diff --git a/spec/javascript/alchemy_admin/components/uploader.spec.js b/spec/javascript/alchemy_admin/components/uploader.spec.js index 6d7f7ca618..e068b4f247 100644 --- a/spec/javascript/alchemy_admin/components/uploader.spec.js +++ b/spec/javascript/alchemy_admin/components/uploader.spec.js @@ -1,5 +1,12 @@ import { Uploader } from "alchemy_admin/components/uploader" +jest.mock("alchemy_admin/utils/ajax", () => { + return { + __esModule: true, + getToken: () => "123" + } +}) + describe("alchemy-uploader", () => { /** * @type {Uploader} @@ -126,6 +133,29 @@ describe("alchemy-uploader", () => { xhrMock.upload.onprogress(progress) expect(progressBar.value).toBe(50) }) + + describe("request header", () => { + it("sends a CSRF token", () => { + expect(xhrMock.setRequestHeader).toHaveBeenCalledWith( + "X-CSRF-Token", + "123" + ) + }) + + it("should mark the request as XHR for Rails request handling", () => { + expect(xhrMock.setRequestHeader).toHaveBeenCalledWith( + "X-Requested-With", + "XMLHttpRequest" + ) + }) + + it("should request json as answer", () => { + expect(xhrMock.setRequestHeader).toHaveBeenCalledWith( + "Accept", + "application/json" + ) + }) + }) }) describe("Validate", () => { diff --git a/spec/javascript/alchemy_admin/components/uploader/file_upload.spec.js b/spec/javascript/alchemy_admin/components/uploader/file_upload.spec.js index bd62e7c7e3..94d30dc0f5 100644 --- a/spec/javascript/alchemy_admin/components/uploader/file_upload.spec.js +++ b/spec/javascript/alchemy_admin/components/uploader/file_upload.spec.js @@ -159,7 +159,7 @@ describe("alchemy-file-upload", () => { describe("successful server response", () => { beforeEach(() => { const xhrMock = mockXMLHttpRequest(200, { - growl_message: "Foo Bar" + message: "Foo Bar" }) renderComponent(testFile, xhrMock) component.request.open("post", "/admin/pictures") @@ -183,7 +183,7 @@ describe("alchemy-file-upload", () => { describe("failed server response", () => { beforeEach(() => { const xhrMock = mockXMLHttpRequest(400, { - growl_message: "Error: Foo Bar" + message: "Error: Foo Bar" }) renderComponent(testFile, xhrMock) component.request.open("post", "/admin/pictures")