Skip to content

Do not send CSRF token for external requests #46

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

Merged
merged 1 commit into from
Jun 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 33 additions & 10 deletions __tests__/fetch_request.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'}})
Expand Down Expand Up @@ -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"})
Expand All @@ -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" }
Expand All @@ -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")
})
Expand All @@ -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', () => {
Expand Down
30 changes: 23 additions & 7 deletions src/fetch_request.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels ugly, but new URL() bombs on relative paths and invalid URLs. Rather have soemthing further down the line fail because of a bad URL than it throwing an error here.

}

get fetchOptions () {
return {
method: this.method.toUpperCase(),
Expand All @@ -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)
)
}

Expand Down