From e590e65a3fe915ed95099c94c2e1852d56e4e33a Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Thu, 3 Nov 2022 12:15:42 +0200 Subject: [PATCH] (core) Allow requests from untrusted origins but without credentials Summary: Allow requests from untrusted origins instead of returning an error, but don't allow credentials (Cookie header) or API keys (Authorization header). Allow setting the header `Content-type: application/json` as an alternative to `X-Requested-With: XMLHttpRequest` to make it easier for clients to make POST/PUT/PATCH/DELETE requests without authentication. Discussion: https://grist.slack.com/archives/C0234CPPXPA/p1666355281535479 Test Plan: Added and updated DocApi tests. Tested manually how this affects requests made from a browser. Reviewers: paulfitz, dsagal Reviewed By: paulfitz, dsagal Differential Revision: https://phab.getgrist.com/D3678 --- app/server/lib/Authorizer.ts | 20 ++++++-- app/server/lib/FlexServer.ts | 14 +++++- test/server/lib/DocApi.ts | 93 ++++++++++++++++++++++++++++++++---- 3 files changed, 111 insertions(+), 16 deletions(-) diff --git a/app/server/lib/Authorizer.ts b/app/server/lib/Authorizer.ts index 4f6e0d48..98632f85 100644 --- a/app/server/lib/Authorizer.ts +++ b/app/server/lib/Authorizer.ts @@ -210,13 +210,23 @@ 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. + // require a header that would trigger a CORS pre-flight request, either: + // - X-Requested-With: XMLHttpRequest + // - 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 + // - Content-Type: application/json + // - https://www.directdefense.com/csrf-in-the-age-of-json/ // 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).json({error: 'Bad request (missing header)'}); + if ( + !mreq.userId && + !(mreq.xhr || mreq.get("content-type") === "application/json") && + !['GET', 'HEAD', 'OPTIONS'].includes(mreq.method) + ) { + return res.status(401).json({ + error: "Unauthenticated requests require one of the headers" + + "'Content-Type: application/json' or 'X-Requested-With: XMLHttpRequest'" + }); } // For some configurations, the user profile can be determined from the request. diff --git a/app/server/lib/FlexServer.ts b/app/server/lib/FlexServer.ts index 6c3f3be4..34a08bee 100644 --- a/app/server/lib/FlexServer.ts +++ b/app/server/lib/FlexServer.ts @@ -1752,12 +1752,22 @@ function allowTestLogin() { // Check OPTIONS requests for allowed origins, and return heads to allow the browser to proceed // with a POST (or other method) request. function trustOriginHandler(req: express.Request, res: express.Response, next: express.NextFunction) { + res.header("Access-Control-Allow-Methods", "GET, PATCH, PUT, POST, DELETE, OPTIONS"); if (trustOrigin(req, res)) { res.header("Access-Control-Allow-Credentials", "true"); - res.header("Access-Control-Allow-Methods", "GET, PATCH, PUT, POST, DELETE, OPTIONS"); res.header("Access-Control-Allow-Headers", "Authorization, Content-Type, X-Requested-With"); } else { - throw new ApiError('Unrecognized origin', 403); + // Any origin is allowed, but if it isn't trusted, then we don't allow credentials, + // i.e. no Cookie or Authorization header. + res.header("Access-Control-Allow-Origin", "*"); + res.header("Access-Control-Allow-Headers", "Content-Type, X-Requested-With"); + if (req.get("Cookie") || req.get("Authorization")) { + // In practice we don't expect to actually reach this point, + // as the browser should not include credentials in preflight (OPTIONS) requests, + // and should block real requests with credentials based on the preflight response. + // But having this means not having to rely on our understanding of browsers and CORS too much. + throw new ApiError("Credentials not supported for cross-origin requests", 403); + } } if ('OPTIONS' === req.method) { res.sendStatus(200); diff --git a/test/server/lib/DocApi.ts b/test/server/lib/DocApi.ts index 0ec6feaa..c17ee7ff 100644 --- a/test/server/lib/DocApi.ts +++ b/test/server/lib/DocApi.ts @@ -3025,20 +3025,95 @@ function testDocApi() { describe("Allowed Origin", () => { it('should allow only example.com', async () => { - async function checkOrigin(origin: string, status: number, error?: string) { + async function checkOrigin(origin: string, allowed: boolean) { const resp = await axios.get(`${serverUrl}/api/docs/${docIds.Timesheets}/tables/Table1/data`, {...chimpy, headers: {...chimpy.headers, "Origin": origin}} ); - error && assert.deepEqual(resp.data, {error}); - assert.equal(resp.status, status); + assert.equal(resp.headers['access-control-allow-credentials'], allowed ? 'true' : undefined); + assert.equal(resp.status, allowed ? 200 : 403); } - await checkOrigin("https://www.toto.com", 403, "Unrecognized origin"); - await checkOrigin("https://badexample.com", 403, "Unrecognized origin"); - await checkOrigin("https://bad.com/example.com/toto", 403, "Unrecognized origin"); - await checkOrigin("https://example.com:3000/path", 200); - await checkOrigin("https://example.com/path", 200); - await checkOrigin("https://good.example.com/toto", 200); + + await checkOrigin("https://www.toto.com", false); + await checkOrigin("https://badexample.com", false); + await checkOrigin("https://bad.com/example.com/toto", false); + await checkOrigin("https://example.com/path", true); + await checkOrigin("https://example.com:3000/path", true); + await checkOrigin("https://good.example.com/toto", true); }); + + it("should respond with correct CORS headers", async function() { + const wid = await getWorkspaceId(userApi, 'Private'); + const docId = await userApi.newDoc({name: 'CorsTestDoc'}, wid); + await userApi.updateDocPermissions(docId, { + users: { + 'everyone@getgrist.com': 'owners', + } + }); + + const chimpyConfig = configForUser("Chimpy"); + const anonConfig = configForUser("Anonymous"); + delete chimpyConfig.headers["X-Requested-With"]; + delete anonConfig.headers["X-Requested-With"]; + + const url = `${serverUrl}/api/docs/${docId}/tables/Table1/records`; + const data = {records: [{fields: {}}]}; + + // Normal same origin requests + anonConfig.headers.Origin = serverUrl; + let response: AxiosResponse; + for (response of [ + await axios.post(url, data, anonConfig), + await axios.get(url, anonConfig), + await axios.options(url, anonConfig), + ]) { + assert.equal(response.status, 200); + assert.equal(response.headers['access-control-allow-methods'], 'GET, PATCH, PUT, POST, DELETE, OPTIONS'); + assert.equal(response.headers['access-control-allow-headers'], 'Authorization, Content-Type, X-Requested-With'); + assert.equal(response.headers['access-control-allow-origin'], serverUrl); + assert.equal(response.headers['access-control-allow-credentials'], 'true'); + } + + // Cross origin requests from untrusted origin. + for (const config of [anonConfig, chimpyConfig]) { + config.headers.Origin = "https://evil.com/"; + for (response of [ + await axios.post(url, data, config), + await axios.get(url, config), + await axios.options(url, config), + ]) { + if (config === anonConfig) { + // Requests without credentials are still OK. + assert.equal(response.status, 200); + } else { + assert.equal(response.status, 403); + assert.deepEqual(response.data, {error: 'Credentials not supported for cross-origin requests'}); + } + assert.equal(response.headers['access-control-allow-methods'], 'GET, PATCH, PUT, POST, DELETE, OPTIONS'); + // Authorization header is not allowed + assert.equal(response.headers['access-control-allow-headers'], 'Content-Type, X-Requested-With'); + // Origin is not echoed back. Arbitrary origin is allowed, but credentials are not. + assert.equal(response.headers['access-control-allow-origin'], '*'); + assert.equal(response.headers['access-control-allow-credentials'], undefined); + } + } + + // POST requests without credentials require a custom header so that a CORS preflight request is triggered. + // One possible header is X-Requested-With, which we removed at the start of the test. + // The other is Content-Type: application/json, which we have been using implicitly above because axios + // automatically treats the given data object as data. Passing a string instead prevents this. + response = await axios.post(url, JSON.stringify(data), anonConfig); + assert.equal(response.status, 401); + assert.deepEqual(response.data, { + error: "Unauthenticated requests require one of the headers" + + "'Content-Type: application/json' or 'X-Requested-With: XMLHttpRequest'" + }); + + // ^ that's for requests without credentials, otherwise we get the same 403 as earlier. + response = await axios.post(url, JSON.stringify(data), chimpyConfig); + assert.equal(response.status, 403); + assert.deepEqual(response.data, {error: 'Credentials not supported for cross-origin requests'}); + }); + }); // PLEASE ADD MORE TESTS HERE