diff --git a/app/client/lib/uploads.ts b/app/client/lib/uploads.ts index 2fc2e652..20e7deb6 100644 --- a/app/client/lib/uploads.ts +++ b/app/client/lib/uploads.ts @@ -119,6 +119,7 @@ export async function uploadFiles( return new Promise((resolve, reject) => { const xhr = new XMLHttpRequest(); xhr.open('post', docUrl(options.docWorkerUrl, UPLOAD_URL_PATH), true); + xhr.setRequestHeader('X-Requested-With', 'XMLHttpRequest'); xhr.withCredentials = true; xhr.upload.addEventListener('progress', (e) => { if (e.lengthComputable) { @@ -170,3 +171,33 @@ export async function fetchURL( const res = await uploadFiles([fileObj], {docWorkerUrl: docComm.docWorkerUrl}, onProgress); return res!; } + +// Submit a form using XHR. Send inputs as JSON, and interpret any reply as JSON. +export async function submitForm(form: HTMLFormElement): Promise { + return new Promise((resolve, reject) => { + const xhr = new XMLHttpRequest(); + const data: {[key: string]: string} = {}; + for (const element of [...form.getElementsByTagName('input')]) { + data[element.name] = element.value; + } + xhr.open('post', form.action, true); + xhr.setRequestHeader('Content-Type', 'application/json'); + xhr.setRequestHeader('X-Requested-With', 'XMLHttpRequest'); + xhr.withCredentials = true; + xhr.send(JSON.stringify(data)); + xhr.addEventListener('error', (e: ProgressEvent) => { + console.warn("Form error", e); // tslint:disable-line:no-console + reject(new Error('Form error, please try again')); + }); + xhr.addEventListener('load', () => { + if (xhr.status !== 200) { + // tslint:disable-next-line:no-console + console.warn("Form failed", xhr.status, xhr.responseText); + const err = safeJsonParse(xhr.responseText, null); + reject(new UserError('Form failed: ' + (err && err.error || xhr.status))); + } else { + resolve(safeJsonParse(xhr.responseText, null)); + } + }); + }); +} diff --git a/app/client/models/errors.ts b/app/client/models/errors.ts index 4ced2358..847dee7d 100644 --- a/app/client/models/errors.ts +++ b/app/client/models/errors.ts @@ -139,7 +139,8 @@ function _logError(error: Error|string) { }), credentials: 'include', headers: { - 'Content-Type': 'application/json' + 'Content-Type': 'application/json', + 'X-Requested-With': 'XMLHttpRequest', } }).catch(e => { // There ... isn't much we can do about this. diff --git a/app/client/ui/WelcomePage.ts b/app/client/ui/WelcomePage.ts index d8e0a113..4534919f 100644 --- a/app/client/ui/WelcomePage.ts +++ b/app/client/ui/WelcomePage.ts @@ -1,5 +1,6 @@ import { Computed, Disposable, dom, domComputed, DomContents, input, MultiHolder, Observable, styled } from "grainjs"; +import { submitForm } from "app/client/lib/uploads"; import { AppModel, reportError } from "app/client/models/AppModel"; import { urlState } from "app/client/models/gristUrlState"; import { AccountWidget } from "app/client/ui/AccountWidget"; @@ -59,11 +60,16 @@ export class WelcomePage extends Disposable { return form = dom( 'form', { method: "post" }, + dom.on('submit', (e) => { + e.preventDefault(); + this._submitForm(form).catch(reportError); + return false; + }), cssLabel('Your full name, as you\'d like it displayed to your collaborators.'), inputEl = cssInput( value, { onInput: true, }, { name: "username" }, - dom.onKeyDown({Enter: () => isNameValid.get() && form.submit()}), + dom.onKeyDown({Enter: () => isNameValid.get() && this._submitForm(form).catch(reportError)}), ), dom.maybe((use) => use(value) && !use(isNameValid), buildNameWarningsDom), cssButtonGroup( @@ -76,6 +82,15 @@ export class WelcomePage extends Disposable { ); } + private async _submitForm(form: HTMLFormElement) { + const result = await submitForm(form); + const redirectUrl = result.redirectUrl; + if (!redirectUrl) { + throw new Error('form failed to redirect'); + } + window.location.assign(redirectUrl); + return false; + } private async _fetchOrgs() { this._orgs = await this._appModel.api.getOrgs(true); diff --git a/app/common/BaseAPI.ts b/app/common/BaseAPI.ts index 18fe4b76..82713f47 100644 --- a/app/common/BaseAPI.ts +++ b/app/common/BaseAPI.ts @@ -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 { diff --git a/app/common/UserAPI.ts b/app/common/UserAPI.ts index ac56aa7e..70da0844 100644 --- a/app/common/UserAPI.ts +++ b/app/common/UserAPI.ts @@ -540,19 +540,21 @@ export class UserAPIImpl extends BaseAPI implements UserAPI { } public async fetchApiKey(): Promise { - 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 { - 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 { - 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 { 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; diff --git a/app/server/lib/Authorizer.ts b/app/server/lib/Authorizer.ts index f9907c36..01edd41c 100644 --- a/app/server/lib/Authorizer.ts +++ b/app/server/lib/Authorizer.ts @@ -135,6 +135,16 @@ export async function addRequestUser(dbManager: HomeDBManager, permitStore: IPer } } + // If we haven't already been authenticated, and this is not a GET/HEAD/OPTIONS, then + // require that the X-Requested-With header field be set to XMLHttpRequest. + // This is trivial for legitimate web clients to do, and an obstacle to + // nefarious ones. + // https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#use-of-custom-request-headers + // https://markitzeroday.com/x-requested-with/cors/2017/06/29/csrf-mitigation-for-ajax-requests.html + if (!mreq.userId && !mreq.xhr && !['GET', 'HEAD', 'OPTIONS'].includes(mreq.method)) { + return res.status(401).send('Bad request (missing header)'); + } + // A bit of extra info we'll add to the "Auth" log message when this request passes the check // for custom-host-specific sessionID. let customHostSession = ''; @@ -461,10 +471,12 @@ export function getTransitiveHeaders(req: Request): {[key: string]: string} { const Cookie = req.get('Cookie'); const PermitHeader = req.get('Permit'); const Organization = (req as RequestWithOrg).org; + const XRequestedWith = req.get('X-Requested-With'); return { ...(Authorization ? { Authorization } : undefined), ...(Cookie ? { Cookie } : undefined), ...(Organization ? { Organization } : undefined), ...(PermitHeader ? { Permit: PermitHeader } : undefined), + ...(XRequestedWith ? { 'X-Requested-With': XRequestedWith } : undefined), }; } diff --git a/app/server/lib/FlexServer.ts b/app/server/lib/FlexServer.ts index 07463658..2ac213cb 100644 --- a/app/server/lib/FlexServer.ts +++ b/app/server/lib/FlexServer.ts @@ -942,7 +942,6 @@ export class FlexServer implements GristServer { public addWelcomePaths() { const middleware = [ - bodyParser.urlencoded({ extended: true }), this._redirectToHostMiddleware, this._userIdMiddleware, this._redirectToLoginWithoutExceptionsMiddleware, @@ -967,7 +966,7 @@ export class FlexServer implements GristServer { const pathname = orgs && orgs.length > 1 ? '/welcome/teams' : '/'; const mergedOrgDomain = this.dbManager.mergedOrgDomain(); const redirectUrl = this._getOrgRedirectUrl(mreq, mergedOrgDomain, pathname); - resp.redirect(redirectUrl); + resp.json({redirectUrl}); })); this.app.get('/welcome/teams', ...middleware, expressWrap(async (req, resp, next) => { @@ -1078,7 +1077,7 @@ export class FlexServer implements GristServer { // Add the handling for the /upload route. Most uploads are meant for a DocWorker: they are put // in temporary files, and the DocWorker needs to be on the same machine to have access to them. // This doesn't check for doc access permissions because the request isn't tied to a document. - addUploadRoute(this, this.app, ...basicMiddleware); + addUploadRoute(this, this.app, this._trustOriginsMiddleware, ...basicMiddleware); this.app.get('/gen_csv', ...docAccessMiddleware, expressWrap(async (req, res) => { return this._docWorker.getCSV(req, res); @@ -1408,7 +1407,9 @@ function trustOriginHandler(req: express.Request, res: express.Response, next: e if (trustOrigin(req, res)) { res.header("Access-Control-Allow-Credentials", "true"); res.header("Access-Control-Allow-Methods", "GET, PATCH, POST, DELETE, OPTIONS"); - res.header("Access-Control-Allow-Headers", "Authorization, Content-Type"); + res.header("Access-Control-Allow-Headers", "Authorization, Content-Type, X-Requested-With"); + } else { + throw new Error('Unrecognized origin'); } if ('OPTIONS' === req.method) { res.sendStatus(200); diff --git a/app/server/lib/uploads.ts b/app/server/lib/uploads.ts index 6923889c..783a9db5 100644 --- a/app/server/lib/uploads.ts +++ b/app/server/lib/uploads.ts @@ -8,7 +8,7 @@ import {RequestWithGrist} from 'app/server/lib/FlexServer'; import {GristServer} from 'app/server/lib/GristServer'; import {guessExt} from 'app/server/lib/guessExt'; import * as log from 'app/server/lib/log'; -import {optStringParam, trustOrigin} from 'app/server/lib/requestUtils'; +import {optStringParam} from 'app/server/lib/requestUtils'; import {isPathWithin} from 'app/server/lib/serverUtils'; import * as shutdown from 'app/server/lib/shutdown'; import {fromCallback} from 'bluebird'; @@ -42,18 +42,13 @@ export function addUploadRoute(server: GristServer, expressApp: Application, ... // When doing a cross-origin post, the browser will check for access with options prior to posting. // We need to reassure it that the request will be accepted before it will go ahead and post. - expressApp.options([`/${UPLOAD_URL_PATH}`, '/copy'], async (req: Request, res: Response) => { - if (!trustOrigin(req, res)) { return res.status(500).send(); } - res.header("Access-Control-Allow-Credentials", "true"); + expressApp.options([`/${UPLOAD_URL_PATH}`, '/copy'], ...handlers, async (req, res) => { + // Origin is checked by middleware - if we get this far, we are ok. res.status(200).send(); }); expressApp.post(`/${UPLOAD_URL_PATH}`, ...handlers, expressWrap(async (req: Request, res: Response) => { try { - if (!trustOrigin(req, res)) { - throw new Error('Unrecognized origin'); - } - res.header("Access-Control-Allow-Credentials", "true"); const uploadResult: UploadResult = await handleUpload(req, res); res.status(200).send(JSON.stringify(uploadResult)); } catch (err) { @@ -67,10 +62,6 @@ export function addUploadRoute(server: GristServer, expressApp: Application, ... // Like upload, but copy data from a document already known to us. expressApp.post(`/copy`, ...handlers, expressWrap(async (req: Request, res: Response) => { - if (!trustOrigin(req, res)) { - throw new Error('Unrecognized origin'); - } - res.header("Access-Control-Allow-Credentials", "true"); const docId = optStringParam(req.query.doc); const name = optStringParam(req.query.name); if (!docId) { throw new Error('doc must be specified'); }