From 3980b9e5806defb63e45ae2696e6c1899f3a8431 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Thu, 14 May 2026 11:38:14 +0200 Subject: [PATCH] Fix IDOR in exports by adding token verification (Issue #31637) (#32883) * Fix IDOR in exports by adding token verification Implement jdleesmiller's suggested fix for Issue #31637: - V1: Return export token in create response - V1: Verify token in get_export using secure_compare - Web: Pass token through fetchExport and fetchDownload - Web: Return token from exportProject to frontend - Frontend: Pass token as query param on status/download requests - Add tests for both services Agent-Logs-Url: https://github.com/overleaf/internal/sessions/7ba5f535-fba2-49a8-91d4-c87bd332d3a0 Co-authored-by: briangough <7457354+briangough@users.noreply.github.com> Fix window.location.pathname to .href to preserve query params Code review correctly identified that window.location.pathname strips query parameters. Switch to window.location.href so the token query parameter is preserved in download URLs. Agent-Logs-Url: https://github.com/overleaf/internal/sessions/7ba5f535-fba2-49a8-91d4-c87bd332d3a0 Co-authored-by: briangough <7457354+briangough@users.noreply.github.com> Fix test mocks to include token in POST responses Agent-Logs-Url: https://github.com/overleaf/internal/sessions/0350c6ef-0fff-4e98-8464-812cd92c523f Co-authored-by: briangough <7457354+briangough@users.noreply.github.com> fix formatting Fix token assignment in initiateExport to use pollResponse token if available Add requireExportToken config setting and tests for invalid/missing token cases Agent-Logs-Url: https://github.com/overleaf/internal/sessions/059bdba2-4f7a-4407-a5a5-cfcffd888739 Co-authored-by: briangough <7457354+briangough@users.noreply.github.com> fix formatting Add tests for export status and token validation in ExportsController and MockV1Api Co-authored-by: Copilot * Update services/v1/main/app/controllers/api/v1/overleaf/exports_controller.rb Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix linting * fix fetchString response handling in ExportsHandler tests --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Copilot Co-authored-by: Brian Gough Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Brian Gough GitOrigin-RevId: 399aef8eaa15ab3655f0905482f3a31fe94e2251 --- .../Features/Exports/ExportsController.mjs | 36 +++- .../src/Features/Exports/ExportsHandler.mjs | 11 +- .../test/acceptance/src/mocks/MockV1Api.mjs | 28 ++- .../src/Exports/ExportsController.test.mjs | 166 +++++++++++++++++- .../unit/src/Exports/ExportsHandler.test.mjs | 98 +++++++++-- 5 files changed, 318 insertions(+), 21 deletions(-) diff --git a/services/web/app/src/Features/Exports/ExportsController.mjs b/services/web/app/src/Features/Exports/ExportsController.mjs index f38b161da6..d5ba10dbbe 100644 --- a/services/web/app/src/Features/Exports/ExportsController.mjs +++ b/services/web/app/src/Features/Exports/ExportsController.mjs @@ -3,6 +3,7 @@ import { expressify } from '@overleaf/promise-utils' import SessionManager from '../Authentication/SessionManager.mjs' import logger from '@overleaf/logger' import OError from '@overleaf/o-error' +import settings from '@overleaf/settings' async function exportProject(req, res, next) { const { project_id: projectId, brand_variation_id: brandVariationId } = @@ -52,6 +53,7 @@ async function exportProject(req, res, next) { ) return res.json({ export_v1_id: exportData.v1_id, + token: exportData.token, message: exportData.message, }) } catch (err) { @@ -70,9 +72,18 @@ async function exportProject(req, res, next) { async function exportStatus(req, res) { const { export_id: exportId } = req.params + const { token } = req.query + if (!token && settings.exports?.requireToken) { + return res.status(403).json({ + export_json: { + status_summary: 'failed', + status_detail: 'token is required', + }, + }) + } let exportJson try { - exportJson = await ExportsHandler.fetchExport(exportId) + exportJson = await ExportsHandler.fetchExport(exportId, token) } catch (err) { const json = { status_summary: 'failed', @@ -96,10 +107,27 @@ async function exportStatus(req, res) { async function exportDownload(req, res, next) { const { type, export_id: exportId } = req.params + const { token } = req.query + if (!token && settings.exports?.requireToken) { + return res.sendStatus(403) + } - SessionManager.getLoggedInUserId(req.session) - const exportFileUrl = await ExportsHandler.fetchDownload(exportId, type) - return res.redirect(exportFileUrl) + try { + SessionManager.getLoggedInUserId(req.session) + const exportFileUrl = await ExportsHandler.fetchDownload( + exportId, + type, + token + ) + return res.redirect(exportFileUrl) + } catch (err) { + const info = OError.getFullInfo(err) + // A bad/spoofed token is rejected by v1 as 404; expose as 400 to clients. + if (info?.statusCode === 404) { + return res.sendStatus(400) + } + throw err + } } export default { diff --git a/services/web/app/src/Features/Exports/ExportsHandler.mjs b/services/web/app/src/Features/Exports/ExportsHandler.mjs index b398701429..80b3875df6 100644 --- a/services/web/app/src/Features/Exports/ExportsHandler.mjs +++ b/services/web/app/src/Features/Exports/ExportsHandler.mjs @@ -19,6 +19,7 @@ export default ExportsHandler = { const body = await ExportsHandler._requestExport(exportData) exportData.v1_id = body.exportId + exportData.token = body.token exportData.message = body.message // TODO: possibly store the export data in Mongo return exportData @@ -161,9 +162,12 @@ export default ExportsHandler = { } }, - async fetchExport(exportId) { + async fetchExport(exportId, token) { const url = new URL(settings.apis.v1.url) url.pathname = `/api/v1/overleaf/exports/${exportId}` + if (token) { + url.searchParams.append('token', token) + } try { return await fetchString(url, { @@ -186,9 +190,12 @@ export default ExportsHandler = { } }, - async fetchDownload(exportId, type) { + async fetchDownload(exportId, type, token) { const url = new URL(settings.apis.v1.url) url.pathname = `/api/v1/overleaf/exports/${exportId}/${type}_url` + if (token) { + url.searchParams.append('token', token) + } try { return await fetchString(url, { diff --git a/services/web/test/acceptance/src/mocks/MockV1Api.mjs b/services/web/test/acceptance/src/mocks/MockV1Api.mjs index 0e803151f4..a0c7e35e9e 100644 --- a/services/web/test/acceptance/src/mocks/MockV1Api.mjs +++ b/services/web/test/acceptance/src/mocks/MockV1Api.mjs @@ -14,6 +14,7 @@ class MockV1Api extends AbstractMockApi { this.existingEmails = [] this.exportId = null this.exportParams = null + this.exportToken = null this.institutionDomains = {} this.institutionId = 1000 this.institutions = {} @@ -49,6 +50,10 @@ class MockV1Api extends AbstractMockApi { this.exportId = id } + setExportToken(token) { + this.exportToken = token + } + getLastExportParams() { return this.exportParams } @@ -209,7 +214,28 @@ class MockV1Api extends AbstractMockApi { this.app.post('/api/v1/overleaf/exports', (req, res) => { this.exportParams = Object.assign({}, req.body) - res.json({ exportId: this.exportId }) + res.json({ exportId: this.exportId, token: this.exportToken }) + }) + + this.app.get('/api/v1/overleaf/exports/:id', (req, res) => { + const { token } = req.query + if (token && token !== this.exportToken) { + return res.sendStatus(404) + } + res.json({ + id: parseInt(req.params.id, 10), + status_summary: 'succeeded', + token: this.exportToken, + }) + }) + + this.app.get('/api/v1/overleaf/exports/:id/zip_url', (req, res) => { + const { token } = req.query + if (token && token !== this.exportToken) { + return res.sendStatus(404) + } + res.set('Content-Type', 'text/plain') + return res.end('https://example.com/export.zip') }) this.app.get('/api/v2/users/:userId/affiliations', (req, res) => { diff --git a/services/web/test/unit/src/Exports/ExportsController.test.mjs b/services/web/test/unit/src/Exports/ExportsController.test.mjs index 99e744d0f4..77c25df599 100644 --- a/services/web/test/unit/src/Exports/ExportsController.test.mjs +++ b/services/web/test/unit/src/Exports/ExportsController.test.mjs @@ -20,11 +20,13 @@ describe('ExportsController', function () { beforeEach(async function (ctx) { ctx.handler = { getUserNotifications: sinon.stub().callsArgWith(1) } + ctx.settings = {} ctx.req = { params: { project_id: projectId, brand_variation_id: brandVariationId, }, + query: {}, body: { firstName, lastName, @@ -41,6 +43,8 @@ describe('ExportsController', function () { ctx.res = { json: sinon.stub(), status: sinon.stub(), + sendStatus: sinon.stub(), + redirect: sinon.stub(), } ctx.res.status.returns(ctx.res) ctx.next = sinon.stub() @@ -62,6 +66,10 @@ describe('ExportsController', function () { }) ) + vi.doMock('@overleaf/settings', () => ({ + default: ctx.settings, + })) + ctx.controller = (await import(modulePath)).default }) @@ -69,7 +77,7 @@ describe('ExportsController', function () { it('should ask the handler to perform the export', async function (ctx) { ctx.handler.exportProject = sinon .stub() - .resolves({ iAmAnExport: true, v1_id: 897 }) + .resolves({ iAmAnExport: true, v1_id: 897, token: 'mock-token' }) const expected = { project_id: projectId, user_id: userId, @@ -85,6 +93,7 @@ describe('ExportsController', function () { expect(ctx.handler.exportProject.args[0][0]).to.deep.equal(expected) expect(res.json.args[0][0]).to.deep.equal({ export_v1_id: 897, + token: 'mock-token', message: undefined, }) }) @@ -95,6 +104,7 @@ describe('ExportsController', function () { ctx.handler.exportProject = sinon.stub().resolves({ iAmAnExport: true, v1_id: 897, + token: 'mock-token', message: 'RESUBMISSION', }) const expected = { @@ -114,6 +124,7 @@ describe('ExportsController', function () { expect(ctx.handler.exportProject.args[0][0]).to.deep.equal(expected) expect(res.json.args[0][0]).to.deep.equal({ export_v1_id: 897, + token: 'mock-token', message: 'RESUBMISSION', }) }) @@ -131,7 +142,7 @@ describe('ExportsController', function () { it('should ask the handler to perform the export', async function (ctx) { ctx.handler.exportProject = sinon .stub() - .resolves({ iAmAnExport: true, v1_id: 897 }) + .resolves({ iAmAnExport: true, v1_id: 897, token: 'mock-token' }) const expected = { project_id: projectId, user_id: userId, @@ -153,6 +164,7 @@ describe('ExportsController', function () { expect(ctx.handler.exportProject.args[0][0]).to.deep.equal(expected) expect(res.json.args[0][0]).to.deep.equal({ export_v1_id: 897, + token: 'mock-token', message: undefined, }) }) @@ -192,7 +204,9 @@ describe('ExportsController', function () { } ctx.req.params = { project_id: projectId, export_id: 897 } + ctx.req.query = { token: 'mock-token' } await ctx.controller.exportStatus(ctx.req, res) + expect(ctx.handler.fetchExport).to.have.been.calledWith(897, 'mock-token') expect(res.json.args[0][0]).to.deep.equal({ export_json: { status_summary: 'completed', @@ -206,4 +220,152 @@ describe('ExportsController', function () { }, }) }) + + describe('exportStatus token validation', function () { + beforeEach(function (ctx) { + ctx.req.params = { project_id: projectId, export_id: 897 } + ctx.handler.fetchExport = sinon.stub().resolves( + `{ + "id":897, + "status_summary":"completed", + "status_detail":"all done", + "partner_submission_id":"abc123", + "v2_user_email":"la@tex.com", + "v2_user_first_name":"Arthur", + "v2_user_last_name":"Author", + "title":"my project", + "token":"token" + }` + ) + }) + + describe('when requireToken is enabled', function () { + beforeEach(function (ctx) { + ctx.settings.exports = { requireToken: true } + }) + + it('should return 403 when no token is provided', async function (ctx) { + ctx.req.query = {} + await ctx.controller.exportStatus(ctx.req, ctx.res) + expect(ctx.res.status.args[0][0]).to.equal(403) + expect(ctx.res.json.args[0][0]).to.deep.equal({ + export_json: { + status_summary: 'failed', + status_detail: 'token is required', + }, + }) + expect(ctx.handler.fetchExport).not.to.have.been.called + }) + + it('should proceed when a token is provided', async function (ctx) { + ctx.req.query = { token: 'mock-token' } + const res = { + json: sinon.stub(), + } + await ctx.controller.exportStatus(ctx.req, res) + expect(ctx.handler.fetchExport).to.have.been.calledWith( + 897, + 'mock-token' + ) + }) + }) + + describe('when requireToken is not enabled', function () { + it('should proceed without token', async function (ctx) { + ctx.req.query = {} + const res = { + json: sinon.stub(), + } + await ctx.controller.exportStatus(ctx.req, res) + expect(ctx.handler.fetchExport).to.have.been.calledWith(897, undefined) + }) + }) + + describe('when a spoofed token is provided', function () { + it('should return a failed status when v1 rejects the token', async function (ctx) { + ctx.req.query = { token: 'wrong-token' } + ctx.handler.fetchExport = sinon + .stub() + .rejects(new Error('Request failed: 404')) + await ctx.controller.exportStatus(ctx.req, ctx.res) + expect(ctx.handler.fetchExport).to.have.been.calledWith( + 897, + 'wrong-token' + ) + expect(ctx.res.json.args[0][0]).to.deep.equal({ + export_json: { + status_summary: 'failed', + status_detail: 'Error: Request failed: 404', + }, + }) + }) + }) + }) + + describe('exportDownload token validation', function () { + beforeEach(function (ctx) { + ctx.req.params = { + project_id: projectId, + export_id: 897, + type: 'zip', + } + ctx.handler.fetchDownload = sinon.stub().resolves('https://example.com') + }) + + describe('when requireToken is enabled', function () { + beforeEach(function (ctx) { + ctx.settings.exports = { requireToken: true } + }) + + it('should return 403 when no token is provided', async function (ctx) { + ctx.req.query = {} + await ctx.controller.exportDownload(ctx.req, ctx.res) + expect(ctx.res.sendStatus.args[0][0]).to.equal(403) + expect(ctx.handler.fetchDownload).not.to.have.been.called + }) + + it('should proceed when a token is provided', async function (ctx) { + ctx.req.query = { token: 'mock-token' } + await ctx.controller.exportDownload(ctx.req, ctx.res) + expect(ctx.handler.fetchDownload).to.have.been.calledWith( + 897, + 'zip', + 'mock-token' + ) + expect(ctx.res.redirect).to.have.been.calledWith('https://example.com') + }) + }) + + describe('when requireToken is not enabled', function () { + it('should proceed without token', async function (ctx) { + ctx.req.query = {} + await ctx.controller.exportDownload(ctx.req, ctx.res) + expect(ctx.handler.fetchDownload).to.have.been.calledWith( + 897, + 'zip', + undefined + ) + expect(ctx.res.redirect).to.have.been.calledWith('https://example.com') + }) + }) + + describe('when a spoofed token is provided', function () { + it('should return 400 when v1 rejects the token', async function (ctx) { + ctx.req.query = { token: 'wrong-token' } + ctx.handler.fetchDownload = sinon.stub().rejects( + OError.tag(new Error('Request failed: 404'), 'v1 error', { + statusCode: 404, + }) + ) + await ctx.controller.exportDownload(ctx.req, ctx.res, ctx.next) + expect(ctx.handler.fetchDownload).to.have.been.calledWith( + 897, + 'zip', + 'wrong-token' + ) + expect(ctx.res.sendStatus).to.have.been.calledWith(400) + expect(ctx.next).not.to.have.been.called + }) + }) + }) }) diff --git a/services/web/test/unit/src/Exports/ExportsHandler.test.mjs b/services/web/test/unit/src/Exports/ExportsHandler.test.mjs index a059b9718d..255d3805a4 100644 --- a/services/web/test/unit/src/Exports/ExportsHandler.test.mjs +++ b/services/web/test/unit/src/Exports/ExportsHandler.test.mjs @@ -466,7 +466,11 @@ describe('ExportsHandler', function () { } ctx.export_data = { iAmAnExport: true } ctx.export_id = 4096 - ctx.fetchUtils.fetchJson.resolves({ exportId: ctx.export_id }) + ctx.export_token = 'mock-export-token' + ctx.fetchUtils.fetchJson.resolves({ + exportId: ctx.export_id, + token: ctx.export_token, + }) }) describe('when all goes well', function () { @@ -490,8 +494,11 @@ describe('ExportsHandler', function () { ) }) - it('should return the body with v1 export id', function (ctx) { - expect(response).to.eql({ exportId: ctx.export_id }) + it('should return the body with v1 export id and token', function (ctx) { + expect(response).to.eql({ + exportId: ctx.export_id, + token: ctx.export_token, + }) }) }) @@ -554,19 +561,50 @@ describe('ExportsHandler', function () { }, } ctx.export_id = 897 + ctx.export_token = 'mock-export-token' ctx.body = '{"id":897, "status_summary":"completed"}' - ctx.fetchUtils.fetchString = sinon - .stub() - .resolves(JSON.stringify({ body: ctx.body })) + ctx.fetchUtils.fetchString = sinon.stub().resolves(ctx.body) }) - describe('when all goes well', function () { + describe('when all goes well with token', function () { + let exportResponse + beforeEach(async function (ctx) { + exportResponse = await ctx.ExportsHandler.fetchExport( + ctx.export_id, + ctx.export_token + ) + }) + + it('should issue the request with token', function (ctx) { + const expectedUrl = new URL( + '/api/v1/overleaf/exports/' + ctx.export_id, + ctx.settings.apis.v1.url + ) + expectedUrl.searchParams.append('token', ctx.export_token) + expect(ctx.fetchUtils.fetchString).to.have.been.calledWith( + expectedUrl, + { + basicAuth: { + user: ctx.settings.apis.v1.user, + password: ctx.settings.apis.v1.pass, + }, + signal: sinon.match.instanceOf(AbortSignal), + } + ) + }) + + it('should return the v1 export id', function (ctx) { + expect(exportResponse).to.eql(ctx.body) + }) + }) + + describe('when called without token', function () { let exportResponse beforeEach(async function (ctx) { exportResponse = await ctx.ExportsHandler.fetchExport(ctx.export_id) }) - it('should issue the request', function (ctx) { + it('should issue the request without token', function (ctx) { expect(ctx.fetchUtils.fetchString).to.have.been.calledWith( new URL( '/api/v1/overleaf/exports/' + ctx.export_id, @@ -583,7 +621,7 @@ describe('ExportsHandler', function () { }) it('should return the v1 export id', function (ctx) { - expect(exportResponse).to.eql(JSON.stringify({ body: ctx.body })) + expect(exportResponse).to.eql(ctx.body) }) }) }) @@ -599,12 +637,48 @@ describe('ExportsHandler', function () { }, } ctx.export_id = 897 + ctx.export_token = 'mock-export-token' ctx.body = 'https://writelatex-conversions-dev.s3.amazonaws.com/exports/ieee_latexqc/tnb/2912/xggmprcrpfwbsnqzqqmvktddnrbqkqkr.zip?X-Amz-Expires=14400&X-Amz-Date=20180730T181003Z&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAJDGDIJFGLNVGZH6A/20180730/us-east-1/s3/aws4_request&X-Amz-SignedHeaders=host&X-Amz-Signature=dec990336913cef9933f0e269afe99722d7ab2830ebf2c618a75673ee7159fee' ctx.fetchUtils.fetchString = sinon.stub().resolves(ctx.body) }) - describe('when all goes well', function () { + describe('when all goes well with token', function () { + let downloadResponse + beforeEach(async function (ctx) { + downloadResponse = await ctx.ExportsHandler.fetchDownload( + ctx.export_id, + 'zip', + ctx.export_token + ) + }) + + it('should issue the request with token', function (ctx) { + const expectedUrl = new URL( + ctx.settings.apis.v1.url + + '/api/v1/overleaf/exports/' + + ctx.export_id + + '/zip_url' + ) + expectedUrl.searchParams.append('token', ctx.export_token) + expect(ctx.fetchUtils.fetchString).to.have.been.calledWith( + expectedUrl, + { + basicAuth: { + user: ctx.settings.apis.v1.user, + password: ctx.settings.apis.v1.pass, + }, + signal: sinon.match.instanceOf(AbortSignal), + } + ) + }) + + it('should return the download URL', function (ctx) { + expect(downloadResponse).to.eql(ctx.body) + }) + }) + + describe('when called without token', function () { let downloadResponse beforeEach(async function (ctx) { downloadResponse = await ctx.ExportsHandler.fetchDownload( @@ -613,7 +687,7 @@ describe('ExportsHandler', function () { ) }) - it('should issue the request', function (ctx) { + it('should issue the request without token', function (ctx) { expect(ctx.fetchUtils.fetchString).to.have.been.calledWith( new URL( ctx.settings.apis.v1.url + @@ -631,7 +705,7 @@ describe('ExportsHandler', function () { ) }) - it('should return the v1 export id', function (ctx) { + it('should return the download URL', function (ctx) { expect(downloadResponse).to.eql(ctx.body) }) })