mirror of
https://github.com/gristlabs/grist-core.git
synced 2026-03-02 04:09:24 +00:00
(core) mitigate csrf by requiring custom header for unsafe methods
Summary: For methods other than `GET`, `HEAD`, and `OPTIONS`, allow cookie-based authentication only if a certain custom header is present. Specifically, we check that `X-Requested-With` is set to `XMLHttpRequest`. This is somewhat arbitrary, but allows us to use https://expressjs.com/en/api.html#req.xhr. A request send from a browser that sets a custom header will prompt a preflight check, giving us a chance to check if the origin is trusted. This diff deals with getting the header in place. There will be more work to do after this: * Make sure that all important endpoints are checking origin. Skimming code, /api endpoint check origin, and some but not all others. * Add tests spot-testing origin checks. * Check on cases that authenticate differently. - Check the websocket endpoint - it can be connected to from an arbitrary site; there is per-doc access control but probably better to lock it down more. - There may be old endpoints that authenticate based on knowledge of a client id rather than cookies. Test Plan: added a test Reviewers: dsagal Reviewed By: dsagal Differential Revision: https://phab.getgrist.com/D2631
This commit is contained in:
@@ -51,6 +51,7 @@ export class BaseAPI {
|
||||
this._logger = options.logger || console;
|
||||
this._headers = {
|
||||
'Content-Type': 'application/json',
|
||||
'X-Requested-With': 'XMLHttpRequest',
|
||||
...options.headers
|
||||
};
|
||||
this._extraParameters = options.extraParameters;
|
||||
@@ -61,6 +62,16 @@ export class BaseAPI {
|
||||
return this.request(url, init);
|
||||
}
|
||||
|
||||
public defaultHeaders() {
|
||||
return this._headers;
|
||||
}
|
||||
|
||||
public defaultHeadersWithoutContentType() {
|
||||
const headers = {...this.defaultHeaders()};
|
||||
delete headers['Content-Type'];
|
||||
return headers;
|
||||
}
|
||||
|
||||
// Similar to request, but uses the axios library, and supports progress indicator.
|
||||
@BaseAPI.countRequest
|
||||
protected async requestAxios(url: string, config: AxiosRequestConfig): Promise<AxiosResponse> {
|
||||
|
||||
@@ -540,19 +540,21 @@ export class UserAPIImpl extends BaseAPI implements UserAPI {
|
||||
}
|
||||
|
||||
public async fetchApiKey(): Promise<string> {
|
||||
const resp = await this.fetch(`${this._url}/api/profile/apiKey`, {
|
||||
credentials: 'include'
|
||||
});
|
||||
const resp = await this.request(`${this._url}/api/profile/apiKey`);
|
||||
return await resp.text();
|
||||
}
|
||||
|
||||
public async createApiKey(): Promise<string> {
|
||||
const res = await this.fetch(`${this._url}/api/profile/apiKey`, {credentials: 'include', method: 'POST'});
|
||||
const res = await this.request(`${this._url}/api/profile/apiKey`, {
|
||||
method: 'POST'
|
||||
});
|
||||
return await res.text();
|
||||
}
|
||||
|
||||
public async deleteApiKey(): Promise<void> {
|
||||
await this.fetch(`${this._url}/api/profile/apiKey`, {credentials: 'include', method: 'DELETE'});
|
||||
await this.request(`${this._url}/api/profile/apiKey`, {
|
||||
method: 'DELETE'
|
||||
});
|
||||
}
|
||||
|
||||
// This method is not strictly needed anymore, but is widely used by
|
||||
@@ -578,10 +580,13 @@ export class UserAPIImpl extends BaseAPI implements UserAPI {
|
||||
formData.append('upload', material as any, options.filename);
|
||||
if (options.timezone) { formData.append('timezone', options.timezone); }
|
||||
const resp = await this.requestAxios(`${this._url}/api/docs`, {
|
||||
headers: this._options.headers,
|
||||
method: 'POST',
|
||||
data: formData,
|
||||
onUploadProgress: options.onUploadProgress,
|
||||
// On browser, it is important not to set Content-Type so that the browser takes care
|
||||
// of setting HTTP headers appropriately. Outside browser, requestAxios has logic
|
||||
// for setting the HTTP headers.
|
||||
headers: {...this.defaultHeadersWithoutContentType()},
|
||||
});
|
||||
return resp.data;
|
||||
}
|
||||
@@ -606,7 +611,7 @@ export class UserAPIImpl extends BaseAPI implements UserAPI {
|
||||
}
|
||||
|
||||
export class DocWorkerAPIImpl extends BaseAPI implements DocWorkerAPI {
|
||||
constructor(readonly url: string, private _options: IOptions = {}) {
|
||||
constructor(readonly url: string, _options: IOptions = {}) {
|
||||
super(_options);
|
||||
}
|
||||
|
||||
@@ -622,7 +627,10 @@ export class DocWorkerAPIImpl extends BaseAPI implements DocWorkerAPI {
|
||||
const formData = this.newFormData();
|
||||
formData.append('upload', material as any, filename);
|
||||
const json = await this.requestJson(`${this.url}/uploads`, {
|
||||
headers: this._options.headers,
|
||||
// On browser, it is important not to set Content-Type so that the browser takes care
|
||||
// of setting HTTP headers appropriately. Outside of browser, node-fetch also appears
|
||||
// to take care of this - https://github.github.io/fetch/#request-body
|
||||
headers: {...this.defaultHeadersWithoutContentType()},
|
||||
method: 'POST',
|
||||
body: formData
|
||||
});
|
||||
@@ -632,7 +640,6 @@ export class DocWorkerAPIImpl extends BaseAPI implements DocWorkerAPI {
|
||||
public async downloadDoc(docId: string, template: boolean = false): Promise<Response> {
|
||||
const extra = template ? '&template=1' : '';
|
||||
const result = await this.request(`${this.url}/download?doc=${docId}${extra}`, {
|
||||
headers: this._options.headers,
|
||||
method: 'GET',
|
||||
});
|
||||
if (!result.ok) { throw new Error(await result.text()); }
|
||||
@@ -648,7 +655,6 @@ export class DocWorkerAPIImpl extends BaseAPI implements DocWorkerAPI {
|
||||
url.searchParams.append('name', name);
|
||||
}
|
||||
const json = await this.requestJson(url.href, {
|
||||
headers: this._options.headers,
|
||||
method: 'POST',
|
||||
});
|
||||
return json.uploadId;
|
||||
|
||||
Reference in New Issue
Block a user