From 3c898a0d6dda284a7e9ebee424bf8362f1b4f667 Mon Sep 17 00:00:00 2001 From: Tony Drake Date: Thu, 16 Jun 2022 13:40:38 -0400 Subject: [PATCH] Do not send CSRF token for external requests This library could be used for any AJAX request, however it was reported that some 3rd party endpoints reject the request if the CSRF token is included in the headers. This change excludes the CSRF token from the headers by comparing the request URL and `window.location.hostname`. - If the URL is a relative path (ie doesn't begin with "http:"), include the token. - If the URL is not parseable with `new URL()`, then we'll continue on and include the token. - If the hostname of the URL and from `window.location` are equal, include the token. - If the hostname of the URL and from `window.location` are different, do not include the token. I beleive this covers and maintains the existing expectations of the library so existing applications shouldn't be caught off gaurd as we are including the token more often than not. --- __tests__/fetch_request.js | 43 +++++++++++++++++++++++++++++--------- src/fetch_request.js | 30 +++++++++++++++++++------- 2 files changed, 56 insertions(+), 17 deletions(-) diff --git a/__tests__/fetch_request.js b/__tests__/fetch_request.js index 789faf7..38a1df9 100644 --- a/__tests__/fetch_request.js +++ b/__tests__/fetch_request.js @@ -22,11 +22,11 @@ describe('perform', () => { const testRequest = new FetchRequest("get", "localhost") const testResponse = await testRequest.perform() - + expect(window.fetch).toHaveBeenCalledTimes(1) expect(window.fetch).toHaveBeenCalledWith("localhost", testRequest.fetchOptions) expect(testResponse).toStrictEqual(new FetchResponse(mockResponse)) - }) + }) test('request is performed with 401', async () => { const mockResponse = new Response(undefined, { status: 401, headers: {'WWW-Authenticate': 'https://localhost/login'}}) @@ -111,20 +111,20 @@ describe('header handling', () => { expect(customRequest.fetchOptions.headers) .toStrictEqual({ ...defaultHeaders, "Content-Type": 'any/thing'}) }) - test('is not set by formData', () => { + test('is not set by formData', () => { const formData = new FormData() formData.append("this", "value") const formDataRequest = new FetchRequest("get", "localhost", { body: formData }) expect(formDataRequest.fetchOptions.headers) .toStrictEqual(defaultHeaders) }) - test('is set by file body', () => { + test('is set by file body', () => { const file = new File(["contenxt"], "file.txt", { type: "text/plain" }) const fileRequest = new FetchRequest("get", "localhost", { body: file }) expect(fileRequest.fetchOptions.headers) - .toStrictEqual({ ...defaultHeaders, "Content-Type": "text/plain"}) + .toStrictEqual({ ...defaultHeaders, "Content-Type": "text/plain"}) }) - test('is set by json body', () => { + test('is set by json body', () => { const jsonRequest = new FetchRequest("get", "localhost", { body: { some: "json"} }) expect(jsonRequest.fetchOptions.headers) .toStrictEqual({ ...defaultHeaders, "Content-Type": "application/json"}) @@ -138,13 +138,13 @@ describe('header handling', () => { request.addHeader("test", "header") expect(request.fetchOptions.headers) .toStrictEqual({ ...defaultHeaders, custom: "Header", "Content-Type": "application/json", "test": "header"}) - }) + }) test('headers win over contentType', () => { const request = new FetchRequest("get", "localhost", { contentType: "application/json", headers: { "Content-Type": "this/overwrites" } }) expect(request.fetchOptions.headers) .toStrictEqual({ ...defaultHeaders, "Content-Type": "this/overwrites"}) - }) + }) test('serializes JSON to String', () => { const jsonBody = { some: "json" } @@ -169,7 +169,7 @@ describe('header handling', () => { request = new FetchRequest("get", "localhost", { redirect }) expect(request.fetchOptions.redirect).toBe(redirect) } - + request = new FetchRequest("get", "localhost") expect(request.fetchOptions.redirect).toBe("follow") }) @@ -191,7 +191,30 @@ describe('header handling', () => { // has no effect request = new FetchRequest("get", "localhost", { credentials: "omit"}) expect(request.fetchOptions.credentials).toBe('same-origin') - }) + }) + + describe('csrf token inclusion', () => { + // window.location.hostname is "localhost" in the test suite + test('csrf token is not included in headers if url hostname is not the same as window.location', () => { + const request = new FetchRequest("get", "http://removeservice.com/test.json") + expect(request.fetchOptions.headers).not.toHaveProperty("X-CSRF-Token") + }) + + test('csrf token is included in headers if url hostname is the same as window.location', () => { + const request = new FetchRequest("get", "http://localhost/test.json") + expect(request.fetchOptions.headers).toHaveProperty("X-CSRF-Token") + }) + + test('csrf token is included if url is a realative path', async () => { + const defaultRequest = new FetchRequest("get", "/somepath") + expect(defaultRequest.fetchOptions.headers).toHaveProperty("X-CSRF-Token") + }) + + test('csrf token is included if url is not parseable', async () => { + const defaultRequest = new FetchRequest("get", "not-a-url") + expect(defaultRequest.fetchOptions.headers).toHaveProperty("X-CSRF-Token") + }) + }) }) describe('query params are parsed', () => { diff --git a/src/fetch_request.js b/src/fetch_request.js index 085ea19..e522397 100644 --- a/src/fetch_request.js +++ b/src/fetch_request.js @@ -38,6 +38,18 @@ export class FetchRequest { this.options.headers = headers } + sameHostname () { + if (!this.originalUrl.startsWith('http:')) { + return true + } + + try { + return new URL(this.originalUrl).hostname === window.location.hostname + } catch (_) { + return true + } + } + get fetchOptions () { return { method: this.method.toUpperCase(), @@ -50,14 +62,18 @@ export class FetchRequest { } get headers () { + const baseHeaders = { + 'X-Requested-With': 'XMLHttpRequest', + 'Content-Type': this.contentType, + Accept: this.accept + } + + if (this.sameHostname()) { + baseHeaders['X-CSRF-Token'] = this.csrfToken + } + return compact( - Object.assign({ - 'X-Requested-With': 'XMLHttpRequest', - 'X-CSRF-Token': this.csrfToken, - 'Content-Type': this.contentType, - Accept: this.accept - }, - this.additionalHeaders) + Object.assign(baseHeaders, this.additionalHeaders) ) }