Skip to content

Commit

Permalink
Handle unauthorized requests accordingly
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
tvdeyen authored and sascha-karnatz committed Dec 14, 2023
1 parent 2f99d20 commit 504f5d1
Show file tree
Hide file tree
Showing 13 changed files with 89 additions and 39 deletions.
4 changes: 3 additions & 1 deletion app/controllers/alchemy/base_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 1 addition & 10 deletions app/controllers/concerns/alchemy/admin/uploader_responses.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand All @@ -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
Expand Down
1 change: 0 additions & 1 deletion app/javascript/alchemy_admin.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ setDefaultAnimation("tooltip.hide", {
}
})


// Global Alchemy object
if (typeof window.Alchemy === "undefined") {
window.Alchemy = {}
Expand Down
44 changes: 27 additions & 17 deletions app/javascript/alchemy_admin/components/uploader.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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}
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down
4 changes: 2 additions & 2 deletions app/javascript/alchemy_admin/components/uploader/progress.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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}
Expand Down
3 changes: 2 additions & 1 deletion app/javascript/alchemy_admin/utils/ajax.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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") {
Expand Down
2 changes: 1 addition & 1 deletion lib/alchemy/test_support/shared_uploader_examples.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
4 changes: 2 additions & 2 deletions spec/controllers/alchemy/admin/attachments_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
17 changes: 17 additions & 0 deletions spec/controllers/alchemy/admin/base_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand Down
2 changes: 1 addition & 1 deletion spec/controllers/alchemy/admin/pictures_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
30 changes: 30 additions & 0 deletions spec/javascript/alchemy_admin/components/uploader.spec.js
Original file line number Diff line number Diff line change
@@ -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}
Expand Down Expand Up @@ -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", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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")
Expand Down

0 comments on commit 504f5d1

Please sign in to comment.